From 40460faf2adc56e184b9e90b2013307eed592c80 Mon Sep 17 00:00:00 2001
From: Michael Bolin <mbolin@openai.com>
Date: Mon, 28 Apr 2025 13:42:04 -0700
Subject: [PATCH] fix: tighten up check for /usr/bin/sandbox-exec (#710)

* In both TypeScript and Rust, we now invoke `/usr/bin/sandbox-exec`
explicitly rather than whatever `sandbox-exec` happens to be on the
`PATH`.
* Changed `isSandboxExecAvailable` to use `access()` rather than
`command -v` so that:
  *  We only do the check once over the lifetime of the Codex process.
  * The check is specific to `/usr/bin/sandbox-exec`.
* We now do a syscall rather than incur the overhead of spawning a
process, dealing with timeouts, etc.

I think there is still room for improvement here where we should move
the `isSandboxExecAvailable` check earlier in the CLI, ideally right
after we do arg parsing to verify that we can provide the Seatbelt
sandbox if that is what the user has requested.
---
 .../src/utils/agent/handle-exec-command.ts    | 33 ++++++++++---------
 .../src/utils/agent/sandbox/macos-seatbelt.ts | 10 +++++-
 codex-rs/core/src/exec.rs                     |  8 ++++-
 3 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/codex-cli/src/utils/agent/handle-exec-command.ts b/codex-cli/src/utils/agent/handle-exec-command.ts
index 6cb48016..ec0ba617 100644
--- a/codex-cli/src/utils/agent/handle-exec-command.ts
+++ b/codex-cli/src/utils/agent/handle-exec-command.ts
@@ -11,8 +11,8 @@ import { exec, execApplyPatch } from "./exec.js";
 import { ReviewDecision } from "./review.js";
 import { isLoggingEnabled, log } from "../logger/log.js";
 import { SandboxType } from "./sandbox/interface.js";
-import { access } from "fs/promises";
-import { execFile } from "node:child_process";
+import { PATH_TO_SEATBELT_EXECUTABLE } from "./sandbox/macos-seatbelt.js";
+import fs from "fs/promises";
 
 // ---------------------------------------------------------------------------
 // Session‑level cache of commands that the user has chosen to always approve.
@@ -218,7 +218,7 @@ async function execCommand(
   let { workdir } = execInput;
   if (workdir) {
     try {
-      await access(workdir);
+      await fs.access(workdir);
     } catch (e) {
       log(`EXEC workdir=${workdir} not found, use process.cwd() instead`);
       workdir = process.cwd();
@@ -271,18 +271,19 @@ async function execCommand(
   };
 }
 
-/**
- * Return `true` if the `sandbox-exec` binary can be located. This intentionally does **not**
- * spawn the binary – we only care about its presence.
- */
-export const isSandboxExecAvailable = (): Promise<boolean> =>
-  new Promise((res) =>
-    execFile(
-      "command",
-      ["-v", "sandbox-exec"],
-      { signal: AbortSignal.timeout(200) },
-      (err) => res(!err), // exit 0 ⇒ found
-    ),
+/** Return `true` if the `/usr/bin/sandbox-exec` is present and executable. */
+const isSandboxExecAvailable: Promise<boolean> = fs
+  .access(PATH_TO_SEATBELT_EXECUTABLE, fs.constants.X_OK)
+  .then(
+    () => true,
+    (err) => {
+      if (!["ENOENT", "ACCESS", "EPERM"].includes(err.code)) {
+        log(
+          `Unexpected error for \`stat ${PATH_TO_SEATBELT_EXECUTABLE}\`: ${err.message}`,
+        );
+      }
+      return false;
+    },
   );
 
 async function getSandbox(runInSandbox: boolean): Promise<SandboxType> {
@@ -295,7 +296,7 @@ async function getSandbox(runInSandbox: boolean): Promise<SandboxType> {
       // instance, inside certain CI images).  Attempting to spawn a missing
       // binary makes Node.js throw an *uncaught* `ENOENT` error further down
       // the stack which crashes the whole CLI.
-      if (await isSandboxExecAvailable()) {
+      if (await isSandboxExecAvailable) {
         return SandboxType.MACOS_SEATBELT;
       } else {
         throw new Error(
diff --git a/codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts b/codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts
index 934056d9..a01e2c63 100644
--- a/codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts
+++ b/codex-cli/src/utils/agent/sandbox/macos-seatbelt.ts
@@ -12,6 +12,14 @@ function getCommonRoots() {
   ];
 }
 
+/**
+ * When working with `sandbox-exec`, only consider `sandbox-exec` in `/usr/bin`
+ * to defend against an attacker trying to inject a malicious version on the
+ * PATH. If /usr/bin/sandbox-exec has been tampered with, then the attacker
+ * already has root access.
+ */
+export const PATH_TO_SEATBELT_EXECUTABLE = "/usr/bin/sandbox-exec";
+
 export function execWithSeatbelt(
   cmd: Array<string>,
   opts: SpawnOptions,
@@ -57,7 +65,7 @@ export function execWithSeatbelt(
   );
 
   const fullCommand = [
-    "sandbox-exec",
+    PATH_TO_SEATBELT_EXECUTABLE,
     "-p",
     fullPolicy,
     ...policyTemplateParams,
diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs
index 4ce07acf..952b4453 100644
--- a/codex-rs/core/src/exec.rs
+++ b/codex-rs/core/src/exec.rs
@@ -35,6 +35,12 @@ const TIMEOUT_CODE: i32 = 64;
 
 const MACOS_SEATBELT_READONLY_POLICY: &str = include_str!("seatbelt_readonly_policy.sbpl");
 
+/// When working with `sandbox-exec`, only consider `sandbox-exec` in `/usr/bin`
+/// to defend against an attacker trying to inject a malicious version on the
+/// PATH. If /usr/bin/sandbox-exec has been tampered with, then the attacker
+/// already has root access.
+const MACOS_PATH_TO_SEATBELT_EXECUTABLE: &str = "/usr/bin/sandbox-exec";
+
 #[derive(Deserialize, Debug, Clone)]
 pub struct ExecParams {
     pub command: Vec<String>,
@@ -186,7 +192,7 @@ pub fn create_seatbelt_command(
     };
 
     let mut seatbelt_command: Vec<String> = vec![
-        "sandbox-exec".to_string(),
+        MACOS_PATH_TO_SEATBELT_EXECUTABLE.to_string(),
         "-p".to_string(),
         full_policy.to_string(),
     ];