diff --git a/docs/tools/exec-approvals.md b/docs/tools/exec-approvals.md index cec00599e2a..30eae3221c5 100644 --- a/docs/tools/exec-approvals.md +++ b/docs/tools/exec-approvals.md @@ -131,17 +131,20 @@ Custom safe bins must define an explicit profile in `tools.exec.safeBinProfiles. Validation is deterministic from argv shape only (no host filesystem existence checks), which prevents file-existence oracle behavior from allow/deny differences. File-oriented options are denied for default safe bins (for example `sort -o`, `sort --output`, -`sort --files0-from`, `sort --compress-program`, `wc --files0-from`, `jq -f/--from-file`, +`sort --files0-from`, `sort --compress-program`, `sort --random-source`, +`sort --temporary-directory`/`-T`, `wc --files0-from`, `jq -f/--from-file`, `grep -f/--file`). Safe bins also enforce explicit per-binary flag policy for options that break stdin-only behavior (for example `sort -o/--output/--compress-program` and grep recursive flags). +Long options are validated fail-closed in safe-bin mode: unknown flags and ambiguous +abbreviations are rejected. Denied flags by safe-bin profile: - `grep`: `--dereference-recursive`, `--directories`, `--exclude-from`, `--file`, `--recursive`, `-R`, `-d`, `-f`, `-r` - `jq`: `--argfile`, `--from-file`, `--library-path`, `--rawfile`, `--slurpfile`, `-L`, `-f` -- `sort`: `--compress-program`, `--files0-from`, `--output`, `-o` +- `sort`: `--compress-program`, `--files0-from`, `--output`, `--random-source`, `--temporary-directory`, `-T`, `-o` - `wc`: `--files0-from` diff --git a/src/infra/exec-approvals-safe-bins.test.ts b/src/infra/exec-approvals-safe-bins.test.ts index 64e44c91ab4..7f1b3dec746 100644 --- a/src/infra/exec-approvals-safe-bins.test.ts +++ b/src/infra/exec-approvals-safe-bins.test.ts @@ -81,6 +81,41 @@ describe("exec approvals safe bins", () => { takesValue: true, label: "blocks sort external program flag", }), + ...buildDeniedFlagVariantCases({ + executableName: "sort", + resolvedPath: "/usr/bin/sort", + flag: "--compress-prog", + takesValue: true, + label: "blocks sort denied flag abbreviations", + }), + ...buildDeniedFlagVariantCases({ + executableName: "sort", + resolvedPath: "/usr/bin/sort", + flag: "--files0-fro", + takesValue: true, + label: "blocks sort denied flag abbreviations", + }), + ...buildDeniedFlagVariantCases({ + executableName: "sort", + resolvedPath: "/usr/bin/sort", + flag: "--random-source", + takesValue: true, + label: "blocks sort filesystem-dependent flags", + }), + ...buildDeniedFlagVariantCases({ + executableName: "sort", + resolvedPath: "/usr/bin/sort", + flag: "--temporary-directory", + takesValue: true, + label: "blocks sort filesystem-dependent flags", + }), + ...buildDeniedFlagVariantCases({ + executableName: "sort", + resolvedPath: "/usr/bin/sort", + flag: "-T", + takesValue: true, + label: "blocks sort filesystem-dependent flags", + }), ...buildDeniedFlagVariantCases({ executableName: "grep", resolvedPath: "/usr/bin/grep", @@ -123,6 +158,13 @@ describe("exec approvals safe bins", () => { takesValue: true, label: "blocks wc file-list flag", }), + ...buildDeniedFlagVariantCases({ + executableName: "wc", + resolvedPath: "/usr/bin/wc", + flag: "--files0-fro", + takesValue: true, + label: "blocks wc denied flag abbreviations", + }), ]; const cases: SafeBinCase[] = [ @@ -163,6 +205,22 @@ describe("exec approvals safe bins", () => { safeBins: ["grep"], executableName: "grep", }, + { + name: "rejects unknown long options in safe-bin mode", + argv: ["sort", "--totally-unknown=1"], + resolvedPath: "/usr/bin/sort", + expected: false, + safeBins: ["sort"], + executableName: "sort", + }, + { + name: "rejects ambiguous long-option abbreviations in safe-bin mode", + argv: ["sort", "--f=1"], + resolvedPath: "/usr/bin/sort", + expected: false, + safeBins: ["sort"], + executableName: "sort", + }, ]; for (const testCase of cases) { diff --git a/src/infra/exec-safe-bin-policy.test.ts b/src/infra/exec-safe-bin-policy.test.ts index a300c20b7bd..886e95ccce0 100644 --- a/src/infra/exec-safe-bin-policy.test.ts +++ b/src/infra/exec-safe-bin-policy.test.ts @@ -48,12 +48,32 @@ describe("exec safe bin policy sort", () => { it("allows stdin-only sort flags", () => { expect(validateSafeBinArgv(["-S", "1M"], sortProfile)).toBe(true); expect(validateSafeBinArgv(["--key=1,1"], sortProfile)).toBe(true); + expect(validateSafeBinArgv(["--ke=1,1"], sortProfile)).toBe(true); }); it("blocks sort --compress-program in safe-bin mode", () => { expect(validateSafeBinArgv(["--compress-program=sh"], sortProfile)).toBe(false); expect(validateSafeBinArgv(["--compress-program", "sh"], sortProfile)).toBe(false); }); + + it("blocks denied long-option abbreviations in safe-bin mode", () => { + expect(validateSafeBinArgv(["--compress-prog=sh"], sortProfile)).toBe(false); + expect(validateSafeBinArgv(["--files0-fro=list.txt"], sortProfile)).toBe(false); + }); + + it("rejects unknown or ambiguous long options in safe-bin mode", () => { + expect(validateSafeBinArgv(["--totally-unknown=1"], sortProfile)).toBe(false); + expect(validateSafeBinArgv(["--f=1"], sortProfile)).toBe(false); + }); +}); + +describe("exec safe bin policy wc", () => { + const wcProfile = SAFE_BIN_PROFILES.wc; + + it("blocks wc --files0-from abbreviations in safe-bin mode", () => { + expect(validateSafeBinArgv(["--files0-fro=list.txt"], wcProfile)).toBe(false); + expect(validateSafeBinArgv(["--files0-fro", "list.txt"], wcProfile)).toBe(false); + }); }); describe("exec safe bin policy denied-flag matrix", () => { diff --git a/src/infra/exec-safe-bin-policy.ts b/src/infra/exec-safe-bin-policy.ts index 878c0a55e5c..35ba2e1723a 100644 --- a/src/infra/exec-safe-bin-policy.ts +++ b/src/infra/exec-safe-bin-policy.ts @@ -134,17 +134,23 @@ export const SAFE_BIN_PROFILE_FIXTURES: Record = "--key", "--field-separator", "--buffer-size", - "--temporary-directory", "--parallel", "--batch-size", - "--random-source", "-k", "-t", "-S", - "-T", ], // --compress-program can invoke an external executable and breaks stdin-only guarantees. - deniedFlags: ["--compress-program", "--files0-from", "--output", "-o"], + // --random-source/--temporary-directory/-T are filesystem-dependent and not stdin-only. + deniedFlags: [ + "--compress-program", + "--files0-from", + "--output", + "--random-source", + "--temporary-directory", + "-T", + "-o", + ], }, uniq: { maxPositional: 0, @@ -293,6 +299,38 @@ function isInvalidValueToken(value: string | undefined): boolean { return !value || !isSafeLiteralToken(value); } +function collectKnownLongFlags( + allowedValueFlags: ReadonlySet, + deniedFlags: ReadonlySet, +): string[] { + const known = new Set(); + for (const flag of allowedValueFlags) { + if (flag.startsWith("--")) { + known.add(flag); + } + } + for (const flag of deniedFlags) { + if (flag.startsWith("--")) { + known.add(flag); + } + } + return Array.from(known); +} + +function resolveCanonicalLongFlag(flag: string, knownLongFlags: string[]): string | null { + if (!flag.startsWith("--") || flag.length <= 2) { + return null; + } + if (knownLongFlags.includes(flag)) { + return flag; + } + const matches = knownLongFlags.filter((candidate) => candidate.startsWith(flag)); + if (matches.length !== 1) { + return null; + } + return matches[0] ?? null; +} + function consumeLongOptionToken( args: string[], index: number, @@ -301,13 +339,22 @@ function consumeLongOptionToken( allowedValueFlags: ReadonlySet, deniedFlags: ReadonlySet, ): number { - if (deniedFlags.has(flag)) { + const knownLongFlags = collectKnownLongFlags(allowedValueFlags, deniedFlags); + const canonicalFlag = resolveCanonicalLongFlag(flag, knownLongFlags); + if (!canonicalFlag) { return -1; } + if (deniedFlags.has(canonicalFlag)) { + return -1; + } + const expectsValue = allowedValueFlags.has(canonicalFlag); if (inlineValue !== undefined) { + if (!expectsValue) { + return -1; + } return isSafeLiteralToken(inlineValue) ? index + 1 : -1; } - if (!allowedValueFlags.has(flag)) { + if (!expectsValue) { return index + 1; } return isInvalidValueToken(args[index + 1]) ? -1 : index + 2;