Skip to content

Commit 3e69e72

Browse files
IshaanIshaan
Ishaan
authored and
Ishaan
committed
fix: harden shell wrapper approval binding
1 parent 9ed8dac commit 3e69e72

10 files changed

Lines changed: 187 additions & 92 deletions

‎src/infra/command-explainer/extract.ts‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import {
1010
import { normalizeExecutableToken } from "../exec-wrapper-resolution.js";
1111
import {
1212
extractShellWrapperCommand,
13+
extractShellWrapperInlineCommand,
1314
isShellWrapperExecutable,
1415
POSIX_SHELL_WRAPPERS,
1516
resolveShellWrapperTransportArgv,
@@ -876,14 +877,11 @@ function shellWrapperPayloadForParsing(
876877
dynamicArguments: DynamicArgument[],
877878
): { command: string; spanBase: SpanBase } | null {
878879
const shellWrapper = extractShellWrapperCommand(argv);
879-
if (
880-
!shellWrapper.isWrapper ||
881-
!shellWrapper.command ||
882-
isDynamicPayload(shellWrapper.command, dynamicArguments)
883-
) {
880+
const payload = shellWrapper.command ?? extractShellWrapperInlineCommand(argv);
881+
if (!shellWrapper.isWrapper || !payload || isDynamicPayload(payload, dynamicArguments)) {
884882
return null;
885883
}
886-
const spanBase = payloadBaseFromArguments(shellWrapper.command, argumentsList);
884+
const spanBase = payloadBaseFromArguments(payload, argumentsList);
887885
if (!spanBase) {
888886
return null;
889887
}
@@ -892,7 +890,7 @@ function shellWrapperPayloadForParsing(
892890
if (!canParseShellWrapperPayload(transportArgv, commandFlag?.flag ?? null)) {
893891
return null;
894892
}
895-
return { command: shellWrapper.command, spanBase };
893+
return { command: payload, spanBase };
896894
}
897895

898896
type InlineEvalHit = InterpreterInlineEvalHit;
@@ -947,7 +945,8 @@ function recordCommandRisks(
947945
}
948946

949947
const shellWrapper = extractShellWrapperCommand(argv);
950-
if (shellWrapper.isWrapper && shellWrapper.command) {
948+
const shellWrapperPayload = shellWrapper.command ?? extractShellWrapperInlineCommand(argv);
949+
if (shellWrapper.isWrapper && shellWrapperPayload) {
951950
const transportArgv = resolveShellWrapperTransportArgv(argv) ?? argv;
952951
const shellExecutable = transportArgv[0] ?? executable;
953952
const commandFlag = shellCommandFlag(transportArgv, 1) ?? shellCommandFlag(argv, 1);
@@ -956,7 +955,7 @@ function recordCommandRisks(
956955
kind: "shell-wrapper",
957956
executable: shellExecutable,
958957
flag: commandFlag?.flag ?? "-c",
959-
payload: shellWrapper.command,
958+
payload: shellWrapperPayload,
960959
text,
961960
span,
962961
});

‎src/infra/exec-approvals-allow-always.test.ts‎

Lines changed: 78 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -324,8 +324,8 @@ describe("resolveAllowAlwaysPatterns", () => {
324324
const patterns = resolveAllowAlwaysPatterns({
325325
segments: [
326326
{
327-
raw: "/bin/zsh -lc 'whoami'",
328-
argv: ["/bin/zsh", "-lc", "whoami"],
327+
raw: "/bin/zsh -c 'whoami'",
328+
argv: ["/bin/zsh", "-c", "whoami"],
329329
resolution: makeMockCommandResolution({
330330
execution: makeMockExecutableResolution({
331331
rawExecutable: "/bin/zsh",
@@ -353,8 +353,8 @@ describe("resolveAllowAlwaysPatterns", () => {
353353
const patterns = resolveAllowAlwaysPatterns({
354354
segments: [
355355
{
356-
raw: "/bin/zsh -lc 'whoami && ls && whoami'",
357-
argv: ["/bin/zsh", "-lc", "whoami && ls && whoami"],
356+
raw: "/bin/zsh -c 'whoami && ls && whoami'",
357+
argv: ["/bin/zsh", "-c", "whoami && ls && whoami"],
358358
resolution: makeMockCommandResolution({
359359
execution: makeMockExecutableResolution({
360360
rawExecutable: "/bin/zsh",
@@ -437,12 +437,49 @@ describe("resolveAllowAlwaysPatterns", () => {
437437
}
438438
});
439439

440+
it("rejects startup shell inline payloads for allow-always and inline-chain allowlist fallback", () => {
441+
if (process.platform === "win32") {
442+
return;
443+
}
444+
const dir = makeTempDir();
445+
const tool = makeExecutable(dir, "openclaw-ok");
446+
const env = { PATH: `${dir}${path.delimiter}${process.env.PATH ?? ""}` };
447+
const safeBins = resolveSafeBins(undefined);
448+
449+
for (const command of [
450+
`bash --login -c "openclaw-ok && openclaw-ok"`,
451+
`bash -i -c "openclaw-ok && openclaw-ok"`,
452+
`bash -lc "openclaw-ok && openclaw-ok"`,
453+
`bash --login -c '$0 "$1"' ${tool} marker`,
454+
`bash -i -c '$0 "$1"' ${tool} marker`,
455+
`bash -lc '$0 "$1"' ${tool} marker`,
456+
]) {
457+
const { persisted } = resolvePersistedPatterns({
458+
command,
459+
dir,
460+
env,
461+
safeBins,
462+
});
463+
expect(persisted).toEqual([]);
464+
465+
const second = evaluateShellAllowlist({
466+
command,
467+
allowlist: [{ pattern: tool }],
468+
safeBins,
469+
cwd: dir,
470+
env,
471+
platform: process.platform,
472+
});
473+
expect(second.allowlistSatisfied).toBe(false);
474+
}
475+
});
476+
440477
it("rejects shell-wrapper positional argv carriers", () => {
441478
if (process.platform === "win32") {
442479
return;
443480
}
444481
expectPositionalArgvCarrierResult({
445-
command: `sh -lc '$0 "$1"' touch {marker}`,
482+
command: `sh -c '$0 "$1"' touch {marker}`,
446483
expectPersisted: true,
447484
});
448485
});
@@ -452,7 +489,7 @@ describe("resolveAllowAlwaysPatterns", () => {
452489
return;
453490
}
454491
expectPositionalArgvCarrierResult({
455-
command: `sh -lc 'exec -- "$0" "$1"' touch {marker}`,
492+
command: `sh -c 'exec -- "$0" "$1"' touch {marker}`,
456493
expectPersisted: true,
457494
});
458495
});
@@ -462,7 +499,7 @@ describe("resolveAllowAlwaysPatterns", () => {
462499
return;
463500
}
464501
expectPositionalArgvCarrierResult({
465-
command: `sh -lc "'$0' "$1"" touch {marker}`,
502+
command: `sh -c "'$0' "$1"" touch {marker}`,
466503
expectPersisted: false,
467504
});
468505
});
@@ -472,7 +509,7 @@ describe("resolveAllowAlwaysPatterns", () => {
472509
return;
473510
}
474511
expectPositionalArgvCarrierResult({
475-
command: `sh -lc "exec
512+
command: `sh -c "exec
476513
$0 \\"$1\\"" touch {marker}`,
477514
expectPersisted: false,
478515
});
@@ -489,15 +526,15 @@ $0 \\"$1\\"" touch {marker}`,
489526
const marker = path.join(dir, "marker");
490527

491528
const { persisted } = resolvePersistedPatterns({
492-
command: `sh -lc 'echo blocked; $0 "$1"' touch ${marker}`,
529+
command: `sh -c 'echo blocked; $0 "$1"' touch ${marker}`,
493530
dir,
494531
env,
495532
safeBins,
496533
});
497534
expect(persisted).not.toContain(touch);
498535

499536
const second = evaluateShellAllowlist({
500-
command: `sh -lc 'echo blocked; $0 "$1"' touch ${marker}`,
537+
command: `sh -c 'echo blocked; $0 "$1"' touch ${marker}`,
501538
allowlist: [{ pattern: touch }],
502539
safeBins,
503540
cwd: dir,
@@ -515,7 +552,7 @@ $0 \\"$1\\"" touch {marker}`,
515552
expectAllowAlwaysBypassBlocked({
516553
dir,
517554
firstCommand: "bash scripts/save_crystal.sh",
518-
secondCommand: "bash -lc 'scripts/save_crystal.sh'",
555+
secondCommand: "bash -c 'scripts/save_crystal.sh'",
519556
env,
520557
persistedPattern: script,
521558
});
@@ -564,8 +601,8 @@ $0 \\"$1\\"" touch {marker}`,
564601
const patterns = resolveAllowAlwaysPatterns({
565602
segments: [
566603
{
567-
raw: "/usr/local/bin/zsh -lc whoami",
568-
argv: ["/usr/local/bin/zsh", "-lc", "whoami"],
604+
raw: "/usr/local/bin/zsh -c whoami",
605+
argv: ["/usr/local/bin/zsh", "-c", "whoami"],
569606
resolution: makeMockCommandResolution({
570607
execution: makeMockExecutableResolution({
571608
rawExecutable: "/usr/local/bin/zsh",
@@ -591,8 +628,8 @@ $0 \\"$1\\"" touch {marker}`,
591628
const patterns = resolveAllowAlwaysPatterns({
592629
segments: [
593630
{
594-
raw: "/usr/bin/nice /bin/zsh -lc whoami",
595-
argv: ["/usr/bin/nice", "/bin/zsh", "-lc", "whoami"],
631+
raw: "/usr/bin/nice /bin/zsh -c whoami",
632+
argv: ["/usr/bin/nice", "/bin/zsh", "-c", "whoami"],
596633
resolution: makeMockCommandResolution({
597634
execution: makeMockExecutableResolution({
598635
rawExecutable: "/usr/bin/nice",
@@ -619,8 +656,8 @@ $0 \\"$1\\"" touch {marker}`,
619656
const patterns = resolveAllowAlwaysPatterns({
620657
segments: [
621658
{
622-
raw: "/usr/bin/time -p /bin/zsh -lc whoami",
623-
argv: ["/usr/bin/time", "-p", "/bin/zsh", "-lc", "whoami"],
659+
raw: "/usr/bin/time -p /bin/zsh -c whoami",
660+
argv: ["/usr/bin/time", "-p", "/bin/zsh", "-c", "whoami"],
624661
resolution: makeMockCommandResolution({
625662
execution: makeMockExecutableResolution({
626663
rawExecutable: "/usr/bin/time",
@@ -650,8 +687,8 @@ $0 \\"$1\\"" touch {marker}`,
650687
const patterns = resolveAllowAlwaysPatterns({
651688
segments: [
652689
{
653-
raw: `${busybox} sh -lc whoami`,
654-
argv: [busybox, "sh", "-lc", "whoami"],
690+
raw: `${busybox} sh -c whoami`,
691+
argv: [busybox, "sh", "-c", "whoami"],
655692
resolution: makeMockCommandResolution({
656693
execution: makeMockExecutableResolution({
657694
rawExecutable: busybox,
@@ -744,8 +781,8 @@ $0 \\"$1\\"" touch {marker}`,
744781
const env = makePathEnv(dir);
745782
expectAllowAlwaysBypassBlocked({
746783
dir,
747-
firstCommand: "/usr/bin/caffeinate -d -w 42 /bin/zsh -lc 'echo warmup-ok'",
748-
secondCommand: "/usr/bin/caffeinate -d -w 42 /bin/zsh -lc 'id > marker'",
784+
firstCommand: "/usr/bin/caffeinate -d -w 42 /bin/zsh -c 'echo warmup-ok'",
785+
secondCommand: "/usr/bin/caffeinate -d -w 42 /bin/zsh -c 'id > marker'",
749786
env,
750787
persistedPattern: echo,
751788
});
@@ -761,8 +798,8 @@ $0 \\"$1\\"" touch {marker}`,
761798
const env = makePathEnv(dir);
762799
expectAllowAlwaysBypassBlocked({
763800
dir,
764-
firstCommand: "/usr/bin/nice /bin/zsh -lc 'echo warmup-ok'",
765-
secondCommand: "/usr/bin/nice /bin/zsh -lc 'id > marker'",
801+
firstCommand: "/usr/bin/nice /bin/zsh -c 'echo warmup-ok'",
802+
secondCommand: "/usr/bin/nice /bin/zsh -c 'id > marker'",
766803
env,
767804
persistedPattern: echo,
768805
});
@@ -779,8 +816,8 @@ $0 \\"$1\\"" touch {marker}`,
779816
expectAllowAlwaysBypassBlocked({
780817
dir,
781818
firstCommand:
782-
"/usr/bin/sandbox-exec -p '(deny default) (allow process*)' /bin/zsh -lc 'echo warmup-ok'",
783-
secondCommand: "/usr/bin/sandbox-exec -p '(allow default)' /bin/zsh -lc 'id > marker'",
819+
"/usr/bin/sandbox-exec -p '(deny default) (allow process*)' /bin/zsh -c 'echo warmup-ok'",
820+
secondCommand: "/usr/bin/sandbox-exec -p '(allow default)' /bin/zsh -c 'id > marker'",
784821
env,
785822
persistedPattern: echo,
786823
});
@@ -796,8 +833,8 @@ $0 \\"$1\\"" touch {marker}`,
796833
const env = makePathEnv(dir);
797834
expectAllowAlwaysBypassBlocked({
798835
dir,
799-
firstCommand: "/usr/bin/time -p /bin/zsh -lc 'echo warmup-ok'",
800-
secondCommand: "/usr/bin/time -p /bin/zsh -lc 'id > marker'",
836+
firstCommand: "/usr/bin/time -p /bin/zsh -c 'echo warmup-ok'",
837+
secondCommand: "/usr/bin/time -p /bin/zsh -c 'id > marker'",
801838
env,
802839
persistedPattern: echo,
803840
});
@@ -813,15 +850,15 @@ $0 \\"$1\\"" touch {marker}`,
813850
const env = makePathEnv(dir);
814851
expectAllowAlwaysBypassBlocked({
815852
dir,
816-
firstCommand: "/usr/bin/arch -arm64 /bin/zsh -lc 'echo warmup-ok'",
817-
secondCommand: "/usr/bin/arch -arm64 /bin/zsh -lc 'id > marker-arch'",
853+
firstCommand: "/usr/bin/arch -arm64 /bin/zsh -c 'echo warmup-ok'",
854+
secondCommand: "/usr/bin/arch -arm64 /bin/zsh -c 'id > marker-arch'",
818855
env,
819856
persistedPattern: echo,
820857
});
821858
expectAllowAlwaysBypassBlocked({
822859
dir,
823-
firstCommand: "/usr/bin/xcrun /bin/zsh -lc 'echo warmup-ok'",
824-
secondCommand: "/usr/bin/xcrun /bin/zsh -lc 'id > marker-xcrun'",
860+
firstCommand: "/usr/bin/xcrun /bin/zsh -c 'echo warmup-ok'",
861+
secondCommand: "/usr/bin/xcrun /bin/zsh -c 'id > marker-xcrun'",
825862
env,
826863
persistedPattern: echo,
827864
});
@@ -873,15 +910,15 @@ $0 \\"$1\\"" touch {marker}`,
873910
const safeBins = resolveSafeBins(undefined);
874911

875912
const { persisted } = resolvePersistedPatterns({
876-
command: `sh -lc '$0 "$@"' awk '{print $1}' data.csv`,
913+
command: `sh -c '$0 "$@"' awk '{print $1}' data.csv`,
877914
dir,
878915
env,
879916
safeBins,
880917
});
881918
expect(persisted).toEqual([]);
882919

883920
const second = evaluateShellAllowlist({
884-
command: `sh -lc '$0 "$@"' awk 'BEGIN{system("id > /tmp/pwned")}'`,
921+
command: `sh -c '$0 "$@"' awk 'BEGIN{system("id > /tmp/pwned")}'`,
885922
allowlist: persisted.map((pattern) => ({ pattern })),
886923
safeBins,
887924
cwd: dir,
@@ -901,8 +938,8 @@ $0 \\"$1\\"" touch {marker}`,
901938
const env = makePathEnv(dir);
902939
expectAllowAlwaysBypassBlocked({
903940
dir,
904-
firstCommand: "/usr/bin/script -q /dev/null /bin/sh -lc 'echo warmup-ok'",
905-
secondCommand: "/usr/bin/script -q /dev/null /bin/sh -lc 'id > marker'",
941+
firstCommand: "/usr/bin/script -q /dev/null /bin/sh -c 'echo warmup-ok'",
942+
secondCommand: "/usr/bin/script -q /dev/null /bin/sh -c 'id > marker'",
906943
env,
907944
persistedPattern: echo,
908945
});
@@ -935,15 +972,15 @@ $0 \\"$1\\"" touch {marker}`,
935972
const safeBins = resolveSafeBins(undefined);
936973

937974
const { persisted } = resolvePersistedPatterns({
938-
command: `sh -lc '$0 "$@"' env echo SAFE`,
975+
command: `sh -c '$0 "$@"' env echo SAFE`,
939976
dir,
940977
env,
941978
safeBins,
942979
});
943980
expect(persisted).toEqual([]);
944981

945982
const second = evaluateShellAllowlist({
946-
command: `sh -lc '$0 "$@"' env BASH_ENV=/tmp/payload.sh bash -lc 'id > /tmp/pwned'`,
983+
command: `sh -c '$0 "$@"' env BASH_ENV=/tmp/payload.sh bash -c 'id > /tmp/pwned'`,
947984
allowlist: [{ pattern: envPath }],
948985
safeBins,
949986
cwd: dir,
@@ -963,15 +1000,15 @@ $0 \\"$1\\"" touch {marker}`,
9631000
const safeBins = resolveSafeBins(undefined);
9641001

9651002
const { persisted } = resolvePersistedPatterns({
966-
command: `sh -lc '$0 "$@"' bash -lc 'echo safe'`,
1003+
command: `sh -c '$0 "$@"' bash -c 'echo safe'`,
9671004
dir,
9681005
env,
9691006
safeBins,
9701007
});
9711008
expect(persisted).toEqual([]);
9721009

9731010
const second = evaluateShellAllowlist({
974-
command: `sh -lc '$0 "$@"' bash -lc 'id > /tmp/pwned'`,
1011+
command: `sh -c '$0 "$@"' bash -c 'id > /tmp/pwned'`,
9751012
allowlist: [{ pattern: bashPath }],
9761013
safeBins,
9771014
cwd: dir,
@@ -991,15 +1028,15 @@ $0 \\"$1\\"" touch {marker}`,
9911028
const safeBins = resolveSafeBins(undefined);
9921029

9931030
const { persisted } = resolvePersistedPatterns({
994-
command: `sh -lc '$0 "$@"' xargs echo SAFE`,
1031+
command: `sh -c '$0 "$@"' xargs echo SAFE`,
9951032
dir,
9961033
env,
9971034
safeBins,
9981035
});
9991036
expect(persisted).toEqual([]);
10001037

10011038
const second = evaluateShellAllowlist({
1002-
command: `sh -lc '$0 "$@"' xargs sh -lc 'id > /tmp/pwned'`,
1039+
command: `sh -c '$0 "$@"' xargs sh -c 'id > /tmp/pwned'`,
10031040
allowlist: [{ pattern: xargsPath }],
10041041
safeBins,
10051042
cwd: dir,

0 commit comments

Comments
 (0)