diff --git a/SECURITY.md b/SECURITY.md index f4ccac868c..a0a43fac3e 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -39,6 +39,10 @@ Reports without reproduction steps, demonstrated impact, and remediation advice OpenClaw is a labor of love. There is no bug bounty program and no budget for paid reports. Please still disclose responsibly so we can fix issues quickly. The best way to help the project right now is by sending PRs. +## Maintainers: GHSA Updates via CLI + +When patching a GHSA via `gh api`, include `X-GitHub-Api-Version: 2022-11-28` (or newer). Without it, some fields (notably CVSS) may not persist even if the request returns 200. + ## Out of Scope - Public Internet Exposure diff --git a/docs/tools/exec-approvals.md b/docs/tools/exec-approvals.md index 8aea7a4895..1243675ec3 100644 --- a/docs/tools/exec-approvals.md +++ b/docs/tools/exec-approvals.md @@ -125,7 +125,8 @@ are treated as allowlisted on nodes (macOS node or headless node host). This use that can run in allowlist mode **without** explicit allowlist entries. Safe bins reject positional file args and path-like tokens, so they can only operate on the incoming stream. Safe bins also force argv tokens to be treated as **literal text** at execution time (no globbing -and no `$VARS` expansion), so patterns like `*` or `$HOME/...` cannot be used to smuggle file reads. +and no `$VARS` expansion) for stdin-only segments, so patterns like `*` or `$HOME/...` cannot be +used to smuggle file reads. Shell chaining and redirections are not auto-allowed in allowlist mode. Shell chaining (`&&`, `||`, `;`) is allowed when every top-level segment satisfies the allowlist diff --git a/src/agents/bash-tools.exec.ts b/src/agents/bash-tools.exec.ts index 9a99346ca7..fa83241851 100644 --- a/src/agents/bash-tools.exec.ts +++ b/src/agents/bash-tools.exec.ts @@ -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) { diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index 01a46e4df6..cb491937df 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -55,6 +55,12 @@ export function resolveSafeBins(entries?: string[] | null): Set { 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; 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, }; } diff --git a/src/infra/exec-approvals-analysis.ts b/src/infra/exec-approvals-analysis.ts index 4c1246791e..7ade7a769a 100644 --- a/src/infra/exec-approvals-analysis.ts +++ b/src/infra/exec-approvals-analysis.ts @@ -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: { diff --git a/src/infra/exec-approvals.test.ts b/src/infra/exec-approvals.test.ts index b6f6e9725e..917e12b923 100644 --- a/src/infra/exec-approvals.test.ts +++ b/src/infra/exec-approvals.test.ts @@ -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 });