Skip to content

Commit 117f28f

Browse files
pgondhi987pgondhi987
committed
fix: block side-effecting command wrappers
1 parent 4d89e00 commit 117f28f

8 files changed

Lines changed: 193 additions & 15 deletions

‎src/infra/dispatch-wrapper-resolution.ts‎

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -235,12 +235,44 @@ function unwrapTimeInvocation(argv: string[]): string[] | null {
235235
});
236236
}
237237

238+
function timeInvocationWritesOutputFile(argv: string[]): boolean {
239+
let expectsOptionValue = false;
240+
for (let idx = 1; idx < argv.length; idx += 1) {
241+
const token = argv[idx]?.trim() ?? "";
242+
if (!token) {
243+
continue;
244+
}
245+
if (expectsOptionValue) {
246+
expectsOptionValue = false;
247+
continue;
248+
}
249+
if (token === "--") {
250+
return false;
251+
}
252+
if (!token.startsWith("-") || token === "-") {
253+
return false;
254+
}
255+
const lower = normalizeLowercaseStringOrEmpty(token);
256+
const [flag] = lower.split("=", 2);
257+
if (flag === "-o" || flag === "--output") {
258+
return true;
259+
}
260+
if (TIME_OPTIONS_WITH_VALUE.has(flag) && !lower.includes("=")) {
261+
expectsOptionValue = true;
262+
}
263+
}
264+
return false;
265+
}
266+
238267
function supportsScriptPositionalCommand(platform: NodeJS.Platform = process.platform): boolean {
239268
return platform === "darwin" || platform === "freebsd";
240269
}
241270

242-
function unwrapScriptInvocation(argv: string[]): string[] | null {
243-
if (!supportsScriptPositionalCommand()) {
271+
function unwrapScriptInvocation(
272+
argv: string[],
273+
platform: NodeJS.Platform = process.platform,
274+
): string[] | null {
275+
if (!supportsScriptPositionalCommand(platform)) {
244276
return null;
245277
}
246278
return scanWrapperInvocation(argv, {
@@ -368,12 +400,16 @@ const DISPATCH_WRAPPER_SPECS: readonly DispatchWrapperSpec[] = [
368400
{ name: "nice", unwrap: unwrapNiceInvocation, transparentUsage: true },
369401
{ name: "nohup", unwrap: unwrapNohupInvocation, transparentUsage: true },
370402
{ name: "sandbox-exec", unwrap: unwrapSandboxExecInvocation, transparentUsage: true },
371-
{ name: "script", unwrap: unwrapScriptInvocation, transparentUsage: true },
403+
{ name: "script", unwrap: unwrapScriptInvocation, transparentUsage: false },
372404
{ name: "setsid" },
373405
{ name: "stdbuf", unwrap: unwrapStdbufInvocation, transparentUsage: true },
374406
{ name: "sudo" },
375407
{ name: "taskset" },
376-
{ name: "time", unwrap: unwrapTimeInvocation, transparentUsage: true },
408+
{
409+
name: "time",
410+
unwrap: unwrapTimeInvocation,
411+
transparentUsage: (argv) => !timeInvocationWritesOutputFile(argv),
412+
},
377413
{ name: "timeout", unwrap: unwrapTimeoutInvocation, transparentUsage: true },
378414
{
379415
name: "xcrun",

‎src/infra/exec-approvals-allowlist.ts‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -408,6 +408,7 @@ function resolveSegmentAllowlistMatch(params: {
408408
segment: allowlistSegment,
409409
cwd: params.context.cwd,
410410
env: params.context.env,
411+
platform: params.context.platform,
411412
})
412413
: undefined;
413414
const shellPositionalArgvMatch = shellPositionalArgvCandidatePath
@@ -822,6 +823,7 @@ function resolveShellWrapperPositionalArgvCandidatePath(params: {
822823
segment: ExecCommandSegment;
823824
cwd?: string;
824825
env?: NodeJS.ProcessEnv;
826+
platform?: string | null;
825827
}): string | undefined {
826828
if (!isShellWrapperSegment(params.segment)) {
827829
return undefined;
@@ -860,7 +862,12 @@ function resolveShellWrapperPositionalArgvCandidatePath(params: {
860862
return undefined;
861863
}
862864

863-
const resolution = resolveCommandResolutionFromArgv([carriedExecutable], params.cwd, params.env);
865+
const resolution = resolveCommandResolutionFromArgv(
866+
[carriedExecutable],
867+
params.cwd,
868+
params.env,
869+
(params.platform ?? undefined) as NodeJS.Platform | undefined,
870+
);
864871
return resolveExecutionTargetCandidatePath(resolution, params.cwd);
865872
}
866873

@@ -965,7 +972,11 @@ function collectAllowAlwaysPatterns(params: {
965972
return;
966973
}
967974

968-
const trustPlan = resolveExecWrapperTrustPlan(params.segment.argv);
975+
const trustPlan = resolveExecWrapperTrustPlan(
976+
params.segment.argv,
977+
undefined,
978+
(params.platform ?? undefined) as NodeJS.Platform | undefined,
979+
);
969980
if (trustPlan.policyBlocked) {
970981
return;
971982
}
@@ -976,7 +987,12 @@ function collectAllowAlwaysPatterns(params: {
976987
raw: trustPlan.argv.join(" "),
977988
argv: trustPlan.argv,
978989
sourceArgv: params.segment.sourceArgv,
979-
resolution: resolveCommandResolutionFromArgv(trustPlan.argv, params.cwd, params.env),
990+
resolution: resolveCommandResolutionFromArgv(
991+
trustPlan.argv,
992+
params.cwd,
993+
params.env,
994+
(params.platform ?? undefined) as NodeJS.Platform | undefined,
995+
),
980996
};
981997

982998
const candidatePath = resolveExecutionTargetTrustPath(segment.resolution, params.cwd);
@@ -1005,6 +1021,7 @@ function collectAllowAlwaysPatterns(params: {
10051021
segment,
10061022
cwd: params.cwd,
10071023
env: params.env,
1024+
platform: params.platform,
10081025
})
10091026
: undefined;
10101027
if (positionalArgvPath) {

‎src/infra/exec-approvals-analysis.test.ts‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -624,6 +624,49 @@ describe("exec approvals shell analysis", () => {
624624
expect(result.segmentSatisfiedBy).toEqual(["allowlist"]);
625625
});
626626

627+
it.each([
628+
{
629+
name: "BSD script transcript",
630+
command: "script ~/.zshenv git log -1 --format='payload'",
631+
platform: "darwin" as const,
632+
blockedWrapper: "script",
633+
},
634+
{
635+
name: "GNU time output file",
636+
command: "/usr/bin/time -o ~/.bashrc -a -f 'payload' git status",
637+
platform: "linux" as const,
638+
blockedWrapper: "time",
639+
},
640+
])("rejects side-effecting dispatch wrapper allowlist bypasses for $name", (testCase) => {
641+
const result = evaluateShellAllowlist({
642+
command: testCase.command,
643+
allowlist: [{ pattern: "git" }],
644+
safeBins: new Set(),
645+
cwd: "/tmp",
646+
platform: testCase.platform,
647+
});
648+
649+
expect(result.analysisOk).toBe(true);
650+
expect(result.allowlistSatisfied).toBe(false);
651+
expect(result.segments[0]?.resolution?.policyBlocked).toBe(true);
652+
expect(result.segments[0]?.resolution?.blockedWrapper).toBe(testCase.blockedWrapper);
653+
expect(result.segmentSatisfiedBy).toEqual([null]);
654+
});
655+
656+
it("keeps GNU time transparent when it only reports to stderr", () => {
657+
const result = evaluateShellAllowlist({
658+
command: "/usr/bin/time -p git status",
659+
allowlist: [{ pattern: "git" }],
660+
safeBins: new Set(),
661+
cwd: "/tmp",
662+
platform: "linux",
663+
});
664+
665+
expect(result.analysisOk).toBe(true);
666+
expect(result.allowlistSatisfied).toBe(true);
667+
expect(result.segmentSatisfiedBy).toEqual(["allowlist"]);
668+
});
669+
627670
it("rejects the legacy skill display prelude when only the wrapper is allowlisted", () => {
628671
if (process.platform === "win32") {
629672
return;

‎src/infra/exec-approvals-analysis.ts‎

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -644,6 +644,7 @@ function analyzeWindowsShellCommand(params: {
644644
command: string;
645645
cwd?: string;
646646
env?: NodeJS.ProcessEnv;
647+
platform?: string | null;
647648
}): ExecCommandAnalysis {
648649
const effective = stripWindowsShellWrapper(params.command.trim());
649650
const unsupported = findWindowsUnsupportedToken(effective);
@@ -664,7 +665,12 @@ function analyzeWindowsShellCommand(params: {
664665
{
665666
raw: params.command,
666667
argv,
667-
resolution: resolveCommandResolutionFromArgv(argv, params.cwd, params.env),
668+
resolution: resolveCommandResolutionFromArgv(
669+
argv,
670+
params.cwd,
671+
params.env,
672+
(params.platform ?? undefined) as NodeJS.Platform | undefined,
673+
),
668674
},
669675
],
670676
};
@@ -679,6 +685,7 @@ function parseSegmentsFromParts(
679685
parts: string[],
680686
cwd?: string,
681687
env?: NodeJS.ProcessEnv,
688+
platform?: string | null,
682689
): ExecCommandSegment[] | null {
683690
const segments: ExecCommandSegment[] = [];
684691
for (const raw of parts) {
@@ -689,7 +696,12 @@ function parseSegmentsFromParts(
689696
segments.push({
690697
raw,
691698
argv,
692-
resolution: resolveCommandResolutionFromArgv(argv, cwd, env),
699+
resolution: resolveCommandResolutionFromArgv(
700+
argv,
701+
cwd,
702+
env,
703+
(platform ?? undefined) as NodeJS.Platform | undefined,
704+
),
693705
});
694706
}
695707
return segments;
@@ -1206,7 +1218,12 @@ export function analyzeShellCommand(params: {
12061218
if (!pipelineSplit.ok) {
12071219
return { ok: false, reason: pipelineSplit.reason, segments: [] };
12081220
}
1209-
const segments = parseSegmentsFromParts(pipelineSplit.segments, params.cwd, params.env);
1221+
const segments = parseSegmentsFromParts(
1222+
pipelineSplit.segments,
1223+
params.cwd,
1224+
params.env,
1225+
params.platform,
1226+
);
12101227
if (!segments) {
12111228
return { ok: false, reason: "unable to parse shell segment", segments: [] };
12121229
}
@@ -1222,7 +1239,7 @@ export function analyzeShellCommand(params: {
12221239
if (!split.ok) {
12231240
return { ok: false, reason: split.reason, segments: [] };
12241241
}
1225-
const segments = parseSegmentsFromParts(split.segments, params.cwd, params.env);
1242+
const segments = parseSegmentsFromParts(split.segments, params.cwd, params.env, params.platform);
12261243
if (!segments) {
12271244
return { ok: false, reason: "unable to parse shell segment", segments: [] };
12281245
}
@@ -1233,6 +1250,7 @@ export function analyzeArgvCommand(params: {
12331250
argv: string[];
12341251
cwd?: string;
12351252
env?: NodeJS.ProcessEnv;
1253+
platform?: string | null;
12361254
}): ExecCommandAnalysis {
12371255
const argv = params.argv.filter((entry) => entry.trim().length > 0);
12381256
if (argv.length === 0) {
@@ -1245,7 +1263,12 @@ export function analyzeArgvCommand(params: {
12451263
raw: argv.join(" "),
12461264
argv,
12471265
sourceArgv: [...params.argv],
1248-
resolution: resolveCommandResolutionFromArgv(argv, params.cwd, params.env),
1266+
resolution: resolveCommandResolutionFromArgv(
1267+
argv,
1268+
params.cwd,
1269+
params.env,
1270+
(params.platform ?? undefined) as NodeJS.Platform | undefined,
1271+
),
12491272
},
12501273
],
12511274
};

‎src/infra/exec-command-resolution.test.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,32 @@ describe("exec-command-resolution", () => {
189189
expect(timeResolution?.execution.executableName).toBe(fixture.exeName);
190190
});
191191

192+
it("keeps file-writing dispatch wrappers on the policy boundary", () => {
193+
const timeResolution = resolveCommandResolutionFromArgv([
194+
"/usr/bin/time",
195+
"-o",
196+
"/tmp/time.log",
197+
"-a",
198+
"-f",
199+
"payload",
200+
"git",
201+
"status",
202+
]);
203+
expect(timeResolution?.policyBlocked).toBe(true);
204+
expect(timeResolution?.blockedWrapper).toBe("time");
205+
expect(timeResolution?.execution.rawExecutable).toBe("/usr/bin/time");
206+
207+
const scriptResolution = resolveCommandResolutionFromArgv(
208+
["script", "/tmp/session.log", "git", "status"],
209+
undefined,
210+
undefined,
211+
"darwin",
212+
);
213+
expect(scriptResolution?.policyBlocked).toBe(true);
214+
expect(scriptResolution?.blockedWrapper).toBe("script");
215+
expect(scriptResolution?.execution.rawExecutable).toBe("script");
216+
});
217+
192218
it("keeps shell multiplexer wrappers as a separate policy target", () => {
193219
if (process.platform === "win32") {
194220
return;

‎src/infra/exec-command-resolution.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,8 +145,9 @@ export function resolveCommandResolutionFromArgv(
145145
argv: string[],
146146
cwd?: string,
147147
env?: NodeJS.ProcessEnv,
148+
platform: NodeJS.Platform = process.platform,
148149
): CommandResolution | null {
149-
const plan = resolveExecWrapperTrustPlan(argv);
150+
const plan = resolveExecWrapperTrustPlan(argv, undefined, platform);
150151
const effectiveArgv = plan.argv;
151152
const rawExecutable = effectiveArgv[0]?.trim();
152153
if (!rawExecutable) {

‎src/infra/exec-wrapper-resolution.test.ts‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,33 @@ describe("resolveDispatchWrapperTrustPlan", () => {
366366
});
367367
});
368368

369+
test("blocks script transcript wrappers even when the inner command is parseable", () => {
370+
expect(
371+
resolveDispatchWrapperTrustPlan(
372+
["script", "-q", "/tmp/session.log", "bash", "-lc", "echo hi"],
373+
undefined,
374+
"darwin",
375+
),
376+
).toEqual({
377+
argv: ["script", "-q", "/tmp/session.log", "bash", "-lc", "echo hi"],
378+
wrappers: ["script"],
379+
policyBlocked: true,
380+
blockedWrapper: "script",
381+
});
382+
});
383+
384+
test.each([
385+
["short output option", ["time", "-o", "/tmp/time.log", "bash", "-lc", "echo hi"]],
386+
["long output option", ["time", "--output=/tmp/time.log", "bash", "-lc", "echo hi"]],
387+
])("blocks GNU time file-output wrappers for %s", (_name, argv) => {
388+
expect(resolveDispatchWrapperTrustPlan(argv)).toEqual({
389+
argv,
390+
wrappers: ["time"],
391+
policyBlocked: true,
392+
blockedWrapper: "time",
393+
});
394+
});
395+
369396
test("blocks wrapper overflow beyond the configured depth", () => {
370397
expect(
371398
resolveDispatchWrapperTrustPlan(["nohup", "timeout", "5s", "bash", "-lc", "echo hi"], 1),

‎src/infra/exec-wrapper-trust-plan.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,13 +65,18 @@ function finalizeExecWrapperTrustPlan(
6565
export function resolveExecWrapperTrustPlan(
6666
argv: string[],
6767
maxDepth = MAX_DISPATCH_WRAPPER_DEPTH,
68+
platform: NodeJS.Platform = process.platform,
6869
): ExecWrapperTrustPlan {
6970
let current = argv;
7071
let policyArgv = argv;
7172
let sawShellMultiplexer = false;
7273
const wrapperChain: string[] = [];
7374
for (let depth = 0; depth < maxDepth; depth += 1) {
74-
const dispatchPlan = resolveDispatchWrapperTrustPlan(current, maxDepth - wrapperChain.length);
75+
const dispatchPlan = resolveDispatchWrapperTrustPlan(
76+
current,
77+
maxDepth - wrapperChain.length,
78+
platform,
79+
);
7580
if (dispatchPlan.policyBlocked) {
7681
return blockedExecWrapperTrustPlan({
7782
argv: dispatchPlan.argv,
@@ -119,7 +124,7 @@ export function resolveExecWrapperTrustPlan(
119124
}
120125

121126
if (wrapperChain.length >= maxDepth) {
122-
const dispatchOverflow = unwrapKnownDispatchWrapperInvocation(current);
127+
const dispatchOverflow = unwrapKnownDispatchWrapperInvocation(current, platform);
123128
if (dispatchOverflow.kind === "blocked" || dispatchOverflow.kind === "unwrapped") {
124129
return blockedExecWrapperTrustPlan({
125130
argv: current,

0 commit comments

Comments
 (0)