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.
This commit is contained in:
Michael Bolin
2025-04-28 13:42:04 -07:00
committed by GitHub
parent 38575ed8aa
commit 40460faf2a
3 changed files with 33 additions and 18 deletions

View File

@@ -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";
// ---------------------------------------------------------------------------
// Sessionlevel 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(

View File

@@ -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,