Skip to content

Commit 9ac4272

Browse files
authored
fix: harden safe-bin argument validation [AI] (#80999)
* fix: reject shell expansion in safe-bin tokens * fix: complete safe-bin shell payload handling * addressing codex review * addressing ci * addressing ci * addressing codex review * docs: add changelog entry for PR merge
1 parent 186de9d commit 9ac4272

8 files changed

Lines changed: 434 additions & 33 deletions

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ Docs: https://docs.openclaw.ai
66

77
### Fixes
88

9+
- fix: harden safe-bin argument validation [AI]. (#80999) Thanks @pgondhi987.
910
- fix: scan plugin runtime entries during install [AI]. (#80998) Thanks @pgondhi987.
1011
- Require auth for sandbox browser CDP relay [AI]. (#81002) Thanks @pgondhi987.
1112
- fix: detect carried exec command forms [AI]. (#81000) Thanks @pgondhi987.

src/infra/exec-approvals-allowlist.ts

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,13 @@ export type ExecAllowlistEvaluation = {
128128
segmentSatisfiedBy: ExecSegmentSatisfiedBy[];
129129
};
130130

131-
export type ExecSegmentSatisfiedBy = "allowlist" | "safeBins" | "skills" | "skillPrelude" | null;
131+
export type ExecSegmentSatisfiedBy =
132+
| "allowlist"
133+
| "safeBins"
134+
| "inlineChain"
135+
| "skills"
136+
| "skillPrelude"
137+
| null;
132138
export type SkillBinTrustEntry = {
133139
name: string;
134140
resolvedPath: string;
@@ -382,7 +388,7 @@ const MAX_SHELL_WRAPPER_INLINE_EVAL_DEPTH = 3;
382388

383389
type InlineChainAllowlistEvaluation = {
384390
matches: ExecAllowlistEntry[];
385-
satisfiedBy: "allowlist";
391+
satisfiedBy: "allowlist" | "inlineChain";
386392
};
387393

388394
type SegmentMatchEvaluation = {
@@ -679,6 +685,7 @@ function evaluateShellWrapperInlineCommands(params: {
679685
}
680686

681687
const matches: ExecAllowlistEntry[] = [];
688+
const segmentSatisfiedBy: ExecSegmentSatisfiedBy[] = [];
682689
for (const inlineCommand of params.inlineCommands) {
683690
const analysis = analyzeShellCommand({
684691
command: inlineCommand,
@@ -694,8 +701,12 @@ function evaluateShellWrapperInlineCommands(params: {
694701
return null;
695702
}
696703
matches.push(...result.matches);
704+
segmentSatisfiedBy.push(...result.segmentSatisfiedBy);
697705
}
698-
return { matches, satisfiedBy: "allowlist" };
706+
const hasLiteralizedInnerSegment = segmentSatisfiedBy.some(
707+
(entry) => entry === "safeBins" || entry === "inlineChain",
708+
);
709+
return { matches, satisfiedBy: hasLiteralizedInnerSegment ? "inlineChain" : "allowlist" };
699710
}
700711

701712
function evaluateShellWrapperInlineCommand(params: {

src/infra/exec-approvals-analysis.ts

Lines changed: 118 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,11 @@ import {
44
resolveCommandResolutionFromArgv,
55
type CommandResolution,
66
} from "./exec-command-resolution.js";
7+
import {
8+
extractShellWrapperInlineCommand,
9+
resolveShellWrapperTransportArgv,
10+
} from "./exec-wrapper-resolution.js";
11+
import { POSIX_INLINE_COMMAND_FLAGS, resolveInlineCommandMatch } from "./shell-inline-command.js";
712

813
export {
914
matchAllowlist,
@@ -996,6 +1001,97 @@ function renderSafeBinSegmentArgv(
9961001
return renderQuotedArgv(argv, platform);
9971002
}
9981003

1004+
function findSubsequence(haystack: readonly string[], needle: readonly string[]): number {
1005+
if (needle.length === 0 || needle.length > haystack.length) {
1006+
return -1;
1007+
}
1008+
for (let start = 0; start <= haystack.length - needle.length; start += 1) {
1009+
let matches = true;
1010+
for (let offset = 0; offset < needle.length; offset += 1) {
1011+
if (haystack[start + offset] !== needle[offset]) {
1012+
matches = false;
1013+
break;
1014+
}
1015+
}
1016+
if (matches) {
1017+
return start;
1018+
}
1019+
}
1020+
return -1;
1021+
}
1022+
1023+
function replaceShellInlineCommandArgv(params: {
1024+
argv: string[];
1025+
oldCommand: string;
1026+
nextCommand: string;
1027+
}): string[] | null {
1028+
const transportArgv = resolveShellWrapperTransportArgv(params.argv);
1029+
if (!transportArgv) {
1030+
return null;
1031+
}
1032+
const transportStart = findSubsequence(params.argv, transportArgv);
1033+
if (transportStart < 0) {
1034+
return null;
1035+
}
1036+
const match = resolveInlineCommandMatch(transportArgv, POSIX_INLINE_COMMAND_FLAGS, {
1037+
allowCombinedC: true,
1038+
});
1039+
if (match.valueTokenIndex === null) {
1040+
return null;
1041+
}
1042+
const absoluteValueIndex = transportStart + match.valueTokenIndex;
1043+
const token = params.argv[absoluteValueIndex];
1044+
if (token === undefined) {
1045+
return null;
1046+
}
1047+
const rewritten = [...params.argv];
1048+
if (token === params.oldCommand) {
1049+
rewritten[absoluteValueIndex] = params.nextCommand;
1050+
return rewritten;
1051+
}
1052+
if (token.endsWith(params.oldCommand)) {
1053+
rewritten[absoluteValueIndex] =
1054+
token.slice(0, token.length - params.oldCommand.length) + params.nextCommand;
1055+
return rewritten;
1056+
}
1057+
return null;
1058+
}
1059+
1060+
function renderInlineChainSegmentArgv(params: {
1061+
segment: ExecCommandSegment;
1062+
cwd?: string;
1063+
env?: NodeJS.ProcessEnv;
1064+
platform?: string | null;
1065+
}): string | null {
1066+
const inlineCommand = extractShellWrapperInlineCommand(params.segment.argv);
1067+
if (!inlineCommand) {
1068+
return null;
1069+
}
1070+
const analysis = analyzeShellCommand({
1071+
command: inlineCommand,
1072+
cwd: params.cwd,
1073+
env: params.env,
1074+
platform: params.platform,
1075+
});
1076+
if (!analysis.ok) {
1077+
return null;
1078+
}
1079+
const rebuilt = buildEnforcedShellCommand({
1080+
command: inlineCommand,
1081+
segments: analysis.segments,
1082+
platform: params.platform,
1083+
});
1084+
if (!rebuilt.ok || !rebuilt.command) {
1085+
return null;
1086+
}
1087+
const rewrittenArgv = replaceShellInlineCommandArgv({
1088+
argv: params.segment.argv,
1089+
oldCommand: inlineCommand,
1090+
nextCommand: rebuilt.command,
1091+
});
1092+
return rewrittenArgv ? renderQuotedArgv(rewrittenArgv, params.platform) : null;
1093+
}
1094+
9991095
/**
10001096
* Rebuilds a shell command and selectively single-quotes argv tokens for segments that
10011097
* must be treated as literal (safeBins hardening) while preserving the rest of the
@@ -1004,7 +1100,16 @@ function renderSafeBinSegmentArgv(
10041100
export function buildSafeBinsShellCommand(params: {
10051101
command: string;
10061102
segments: ExecCommandSegment[];
1007-
segmentSatisfiedBy: ("allowlist" | "safeBins" | "skills" | "skillPrelude" | null)[];
1103+
segmentSatisfiedBy: (
1104+
| "allowlist"
1105+
| "safeBins"
1106+
| "inlineChain"
1107+
| "skills"
1108+
| "skillPrelude"
1109+
| null
1110+
)[];
1111+
cwd?: string;
1112+
env?: NodeJS.ProcessEnv;
10081113
platform?: string | null;
10091114
}): { ok: boolean; command?: string; reason?: string } {
10101115
if (params.segments.length !== params.segmentSatisfiedBy.length) {
@@ -1020,6 +1125,18 @@ export function buildSafeBinsShellCommand(params: {
10201125
return { ok: false, reason: "segment mapping failed" };
10211126
}
10221127
const needsLiteral = by === "safeBins";
1128+
if (by === "inlineChain") {
1129+
const rendered = renderInlineChainSegmentArgv({
1130+
segment: seg,
1131+
cwd: params.cwd,
1132+
env: params.env,
1133+
platform: params.platform,
1134+
});
1135+
if (!rendered) {
1136+
return { ok: false, reason: "inline chain execution plan unavailable" };
1137+
}
1138+
return { ok: true, rendered };
1139+
}
10231140
if (!needsLiteral) {
10241141
return { ok: true, rendered: raw.trim() };
10251142
}

src/infra/exec-approvals-safe-bins.test.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,30 @@ describe("exec approvals safe bins", () => {
223223
expected: false,
224224
setup: (cwd) => fs.writeFileSync(path.join(cwd, "secret.json"), "{}"),
225225
},
226+
{
227+
name: "blocks POSIX parameter expansion in safe-bin value tokens",
228+
argv: ["head", "-c${IFS}16${IFS}${OPENCLAW_CONFIG_PATH}"],
229+
resolvedPath: "/usr/bin/head",
230+
expected: false,
231+
safeBins: ["head"],
232+
executableName: "head",
233+
},
234+
{
235+
name: "blocks POSIX parameter expansion in safe-bin long option values",
236+
argv: ["head", "--bytes=${IFS}16"],
237+
resolvedPath: "/usr/bin/head",
238+
expected: false,
239+
safeBins: ["head"],
240+
executableName: "head",
241+
},
242+
{
243+
name: "blocks POSIX parameter expansion in safe-bin positional tokens",
244+
argv: ["tr", "${IFS}", "_"],
245+
resolvedPath: "/usr/bin/tr",
246+
expected: false,
247+
safeBins: ["tr"],
248+
executableName: "tr",
249+
},
226250
{
227251
name: "blocks safe bins resolved from untrusted directories",
228252
argv: ["jq", ".foo"],

src/infra/exec-safe-bin-policy-validator.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,17 @@ function hasGlobToken(value: string): boolean {
2929
return /[*?[\]]/.test(value);
3030
}
3131

32+
function hasShellExpansionToken(value: string): boolean {
33+
return /\$(?:[A-Za-z0-9_@*?!$#-]|\{|\(|\[)/.test(value);
34+
}
35+
3236
const NO_FLAGS: ReadonlySet<string> = new Set();
3337

3438
function isSafeLiteralToken(value: string): boolean {
3539
if (!value || value === "-") {
3640
return true;
3741
}
38-
return !hasGlobToken(value) && !isPathLikeToken(value);
42+
return !hasGlobToken(value) && !hasShellExpansionToken(value) && !isPathLikeToken(value);
3943
}
4044

4145
function isInvalidValueToken(value: string | undefined): boolean {

0 commit comments

Comments
 (0)