vultr/openclaw
1
0
Fork 0
mirror of https://github.com/openclaw/openclaw.git synced 2026-05-01 19:40:21 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(security): refine safeBins hardening

Browse Source
This commit is contained in:
Peter Steinberger
2026-02-14 19:59:03 +01:00
parent eed6113359
commit 24d2c6292e
6 changed files with 173 additions and 121 deletions
Download Patch File Download Diff File Expand all files Collapse all files

33
src/agents/bash-tools.exec.ts
View File

@@ -16,6 +16,7 @@ import {
resolveExecApprovals,
resolveExecApprovalsFromFile,
buildSafeShellCommand,
buildSafeBinsShellCommand,
} from "../infra/exec-approvals.js";
import { buildNodeShellCommand } from "../infra/node-shell.js";
import {
@@ -806,23 +807,41 @@ export function createExecTool(
throw new Error("exec denied: allowlist miss");
}
// If allowlist is satisfied only via safeBins (no explicit allowlist match),
// run a sanitized `shell -c` command that disables glob/var expansion by
// forcing every argv token to be literal via single-quoting.
// If allowlist uses safeBins, sanitize only those stdin-only segments:
// disable glob/var expansion by forcing argv tokens to be literal via single-quoting.
if (
hostSecurity === "allowlist" &&
analysisOk &&
allowlistSatisfied &&
allowlistMatches.length === 0
allowlistEval.segmentSatisfiedBy.some((by) => by === "safeBins")
) {
const safe = buildSafeShellCommand({
const safe = buildSafeBinsShellCommand({
command: params.command,
segments: allowlistEval.segments,
segmentSatisfiedBy: allowlistEval.segmentSatisfiedBy,
platform: process.platform,
});
if (!safe.ok || !safe.command) {
throw new Error(`exec denied: safeBins sanitize failed (${safe.reason ?? "unknown"})`);
// Fallback: quote everything (safe, but may change glob behavior).
const fallback = buildSafeShellCommand({
command: params.command,
platform: process.platform,
});
if (!fallback.ok || !fallback.command) {
throw new Error(
`exec denied: safeBins sanitize failed (${safe.reason ?? "unknown"})`,
);
}
warnings.push(
"Warning: safeBins hardening used fallback quoting due to parser mismatch.",
);
execCommandOverride = fallback.command;
} else {
warnings.push(
"Warning: safeBins hardening disabled glob/variable expansion for stdin-only segments.",
);
execCommandOverride = safe.command;
}
execCommandOverride = safe.command;
}
if (allowlistMatches.length > 0) {

61
src/infra/exec-approvals-allowlist.ts
View File

@@ -55,6 +55,12 @@ export function resolveSafeBins(entries?: string[] | null): Set<string> {
return normalizeSafeBins(entries ?? []);
}
function hasGlobToken(value: string): boolean {
// Safe bins are stdin-only; globbing is both surprising and a historical bypass vector.
// Note: we still harden execution-time expansion separately.
return /[*?[\]]/.test(value);
}
export function isSafeBinUsage(params: {
argv: string[];
resolution: CommandResolution | null;
@@ -62,6 +68,11 @@ export function isSafeBinUsage(params: {
cwd?: string;
fileExists?: (filePath: string) => boolean;
}): boolean {
// Windows host exec uses PowerShell, which has different parsing/expansion rules.
// Keep safeBins conservative there (require explicit allowlist entries).
if (isWindowsPlatform(process.platform)) {
return false;
}
if (params.safeBins.size === 0) {
return false;
}
@@ -94,12 +105,18 @@ export function isSafeBinUsage(params: {
const eqIndex = token.indexOf("=");
if (eqIndex > 0) {
const value = token.slice(eqIndex + 1);
if (value && hasGlobToken(value)) {
return false;
}
if (value && (isPathLikeToken(value) || exists(path.resolve(cwd, value)))) {
return false;
}
}
continue;
}
if (hasGlobToken(token)) {
return false;
}
if (isPathLikeToken(token)) {
return false;
}
@@ -113,8 +130,11 @@ export function isSafeBinUsage(params: {
export type ExecAllowlistEvaluation = {
allowlistSatisfied: boolean;
allowlistMatches: ExecAllowlistEntry[];
segmentSatisfiedBy: ExecSegmentSatisfiedBy[];
};
export type ExecSegmentSatisfiedBy = "allowlist" | "safeBins" | "skills" | null;
function evaluateSegments(
segments: ExecCommandSegment[],
params: {
@@ -124,9 +144,14 @@ function evaluateSegments(
skillBins?: Set<string>;
autoAllowSkills?: boolean;
},
): { satisfied: boolean; matches: ExecAllowlistEntry[] } {
): {
satisfied: boolean;
matches: ExecAllowlistEntry[];
segmentSatisfiedBy: ExecSegmentSatisfiedBy[];
} {
const matches: ExecAllowlistEntry[] = [];
const allowSkills = params.autoAllowSkills === true && (params.skillBins?.size ?? 0) > 0;
const segmentSatisfiedBy: ExecSegmentSatisfiedBy[] = [];
const satisfied = segments.every((segment) => {
const candidatePath = resolveAllowlistCandidatePath(segment.resolution, params.cwd);
@@ -148,10 +173,18 @@ function evaluateSegments(
allowSkills && segment.resolution?.executableName
? params.skillBins?.has(segment.resolution.executableName)
: false;
return Boolean(match || safe || skillAllow);
const by: ExecSegmentSatisfiedBy = match
? "allowlist"
: safe
? "safeBins"
: skillAllow
? "skills"
: null;
segmentSatisfiedBy.push(by);
return Boolean(by);
});
return { satisfied, matches };
return { satisfied, matches, segmentSatisfiedBy };
}
export function evaluateExecAllowlist(params: {
@@ -163,8 +196,9 @@ export function evaluateExecAllowlist(params: {
autoAllowSkills?: boolean;
}): ExecAllowlistEvaluation {
const allowlistMatches: ExecAllowlistEntry[] = [];
const segmentSatisfiedBy: ExecSegmentSatisfiedBy[] = [];
if (!params.analysis.ok || params.analysis.segments.length === 0) {
return { allowlistSatisfied: false, allowlistMatches };
return { allowlistSatisfied: false, allowlistMatches, segmentSatisfiedBy };
}
// If the analysis contains chains, evaluate each chain part separately
@@ -178,11 +212,12 @@ export function evaluateExecAllowlist(params: {
autoAllowSkills: params.autoAllowSkills,
});
if (!result.satisfied) {
return { allowlistSatisfied: false, allowlistMatches: [] };
return { allowlistSatisfied: false, allowlistMatches: [], segmentSatisfiedBy: [] };
}
allowlistMatches.push(...result.matches);
segmentSatisfiedBy.push(...result.segmentSatisfiedBy);
}
return { allowlistSatisfied: true, allowlistMatches };
return { allowlistSatisfied: true, allowlistMatches, segmentSatisfiedBy };
}
// No chains, evaluate all segments together
@@ -193,7 +228,11 @@ export function evaluateExecAllowlist(params: {
skillBins: params.skillBins,
autoAllowSkills: params.autoAllowSkills,
});
return { allowlistSatisfied: result.satisfied, allowlistMatches: result.matches };
return {
allowlistSatisfied: result.satisfied,
allowlistMatches: result.matches,
segmentSatisfiedBy: result.segmentSatisfiedBy,
};
}
export type ExecAllowlistAnalysis = {
@@ -201,6 +240,7 @@ export type ExecAllowlistAnalysis = {
allowlistSatisfied: boolean;
allowlistMatches: ExecAllowlistEntry[];
segments: ExecCommandSegment[];
segmentSatisfiedBy: ExecSegmentSatisfiedBy[];
};
/**
@@ -230,6 +270,7 @@ export function evaluateShellAllowlist(params: {
allowlistSatisfied: false,
allowlistMatches: [],
segments: [],
segmentSatisfiedBy: [],
};
}
const evaluation = evaluateExecAllowlist({
@@ -245,11 +286,13 @@ export function evaluateShellAllowlist(params: {
allowlistSatisfied: evaluation.allowlistSatisfied,
allowlistMatches: evaluation.allowlistMatches,
segments: analysis.segments,
segmentSatisfiedBy: evaluation.segmentSatisfiedBy,
};
}
const allowlistMatches: ExecAllowlistEntry[] = [];
const segments: ExecCommandSegment[] = [];
const segmentSatisfiedBy: ExecSegmentSatisfiedBy[] = [];
for (const part of chainParts) {
const analysis = analyzeShellCommand({
@@ -264,6 +307,7 @@ export function evaluateShellAllowlist(params: {
allowlistSatisfied: false,
allowlistMatches: [],
segments: [],
segmentSatisfiedBy: [],
};
}
@@ -277,12 +321,14 @@ export function evaluateShellAllowlist(params: {
autoAllowSkills: params.autoAllowSkills,
});
allowlistMatches.push(...evaluation.allowlistMatches);
segmentSatisfiedBy.push(...evaluation.segmentSatisfiedBy);
if (!evaluation.allowlistSatisfied) {
return {
analysisOk: true,
allowlistSatisfied: false,
allowlistMatches,
segments,
segmentSatisfiedBy,
};
}
}
@@ -292,5 +338,6 @@ export function evaluateShellAllowlist(params: {
allowlistSatisfied: true,
allowlistMatches,
segments,
segmentSatisfiedBy,
};
}

158
src/infra/exec-approvals-analysis.ts
View File

@@ -772,109 +772,75 @@ export function buildSafeShellCommand(params: { command: string; platform?: stri
return { ok: true, command: out };
}
function renderQuotedArgv(argv: string[]): string {
return argv.map((token) => shellEscapeSingleArg(token)).join(" ");
}
/**
* Rebuilds a shell command and selectively single-quotes argv tokens for segments that
* must be treated as literal (safeBins hardening) while preserving the rest of the
* shell syntax (pipes + chaining).
*/
export function buildSafeBinsShellCommand(params: {
command: string;
segments: ExecCommandSegment[];
segmentSatisfiedBy: ("allowlist" | "safeBins" | "skills" | null)[];
platform?: string | null;
}): { ok: boolean; command?: string; reason?: string } {
const platform = params.platform ?? null;
if (isWindowsPlatform(platform)) {
return { ok: false, reason: "unsupported platform" };
}
if (params.segments.length !== params.segmentSatisfiedBy.length) {
return { ok: false, reason: "segment metadata mismatch" };
}
const chain = splitCommandChainWithOperators(params.command.trim());
const chainParts: ShellChainPart[] = chain ?? [{ part: params.command.trim(), opToNext: null }];
let segIndex = 0;
let out = "";
for (const part of chainParts) {
const pipelineSplit = splitShellPipeline(part.part);
if (!pipelineSplit.ok) {
return { ok: false, reason: pipelineSplit.reason ?? "unable to parse pipeline" };
}
const rendered: string[] = [];
for (const raw of pipelineSplit.segments) {
const seg = params.segments[segIndex];
const by = params.segmentSatisfiedBy[segIndex];
if (!seg || by === undefined) {
return { ok: false, reason: "segment mapping failed" };
}
const needsLiteral = by === "safeBins";
rendered.push(needsLiteral ? renderQuotedArgv(seg.argv) : raw.trim());
segIndex += 1;
}
out += rendered.join(" | ");
if (part.opToNext) {
out += ` ${part.opToNext} `;
}
}
if (segIndex !== params.segments.length) {
return { ok: false, reason: "segment count mismatch" };
}
return { ok: true, command: out };
}
/**
* Splits a command string by chain operators (&&, ||, ;) while respecting quotes.
* Returns null when no chain is present or when the chain is malformed.
*/
export function splitCommandChain(command: string): string[] | null {
const parts: string[] = [];
let buf = "";
let inSingle = false;
let inDouble = false;
let escaped = false;
let foundChain = false;
let invalidChain = false;
const pushPart = () => {
const trimmed = buf.trim();
if (trimmed) {
parts.push(trimmed);
buf = "";
return true;
}
buf = "";
return false;
};
for (let i = 0; i < command.length; i += 1) {
const ch = command[i];
const next = command[i + 1];
if (escaped) {
buf += ch;
escaped = false;
continue;
}
if (!inSingle && !inDouble && ch === "\\") {
escaped = true;
buf += ch;
continue;
}
if (inSingle) {
if (ch === "'") {
inSingle = false;
}
buf += ch;
continue;
}
if (inDouble) {
if (ch === "\\" && isDoubleQuoteEscape(next)) {
buf += ch;
buf += next;
i += 1;
continue;
}
if (ch === '"') {
inDouble = false;
}
buf += ch;
continue;
}
if (ch === "'") {
inSingle = true;
buf += ch;
continue;
}
if (ch === '"') {
inDouble = true;
buf += ch;
continue;
}
if (ch === "&" && command[i + 1] === "&") {
if (!pushPart()) {
invalidChain = true;
}
i += 1;
foundChain = true;
continue;
}
if (ch === "|" && command[i + 1] === "|") {
if (!pushPart()) {
invalidChain = true;
}
i += 1;
foundChain = true;
continue;
}
if (ch === ";") {
if (!pushPart()) {
invalidChain = true;
}
foundChain = true;
continue;
}
buf += ch;
}
const pushedFinal = pushPart();
if (!foundChain) {
const parts = splitCommandChainWithOperators(command);
if (!parts) {
return null;
}
if (invalidChain || !pushedFinal) {
return null;
}
return parts.length > 0 ? parts : null;
return parts.map((p) => p.part);
}
export function analyzeShellCommand(params: {

35
src/infra/exec-approvals.test.ts
View File

@@ -5,7 +5,7 @@ import { describe, expect, it, vi } from "vitest";
import {
analyzeArgvCommand,
analyzeShellCommand,
buildSafeShellCommand,
buildSafeBinsShellCommand,
evaluateExecAllowlist,
evaluateShellAllowlist,
isSafeBinUsage,
@@ -80,21 +80,30 @@ describe("exec approvals allowlist matching", () => {
});
describe("exec approvals safe shell command builder", () => {
it("single-quotes argv tokens while preserving pipes/chaining", () => {
it("quotes only safeBins segments (leaves other segments untouched)", () => {
if (process.platform === "win32") {
return;
}
const res = buildSafeShellCommand({
command: 'head $FOO | grep * && echo "a\'b" ; wc -l',
const analysis = analyzeShellCommand({
command: "rg foo src/*.ts | head -n 5 && echo ok",
cwd: "/tmp",
env: { PATH: "/usr/bin:/bin" },
platform: process.platform,
});
expect(analysis.ok).toBe(true);
const res = buildSafeBinsShellCommand({
command: "rg foo src/*.ts | head -n 5 && echo ok",
segments: analysis.segments,
segmentSatisfiedBy: [null, "safeBins", null],
platform: process.platform,
});
expect(res.ok).toBe(true);
expect(res.command).toContain("'$FOO'");
expect(res.command).toContain("'*'");
expect(res.command).toContain("&&");
expect(res.command).toContain(";");
expect(res.command).toContain("|");
expect(res.command).toContain("'a'\"'\"'b'");
// Preserve non-safeBins segment raw (glob stays unquoted)
expect(res.command).toContain("rg foo src/*.ts");
// SafeBins segment is fully quoted
expect(res.command).toContain("'head' '-n' '5'");
});
});
@@ -346,6 +355,9 @@ describe("exec approvals shell allowlist (chained commands)", () => {
describe("exec approvals safe bins", () => {
it("allows safe bins with non-path args", () => {
if (process.platform === "win32") {
return;
}
const dir = makeTempDir();
const binDir = path.join(dir, "bin");
fs.mkdirSync(binDir, { recursive: true });
@@ -370,6 +382,9 @@ describe("exec approvals safe bins", () => {
});
it("blocks safe bins with file args", () => {
if (process.platform === "win32") {
return;
}
const dir = makeTempDir();
const binDir = path.join(dir, "bin");
fs.mkdirSync(binDir, { recursive: true });