From a7cbce1b3de2db87300c5c11f4be2ab3a2d29623 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 16 Feb 2026 03:19:38 +0100 Subject: [PATCH] refactor(security): tighten sandbox bind validation --- .../sandbox/validate-sandbox-security.test.ts | 23 +++++---- .../sandbox/validate-sandbox-security.ts | 47 +++++++------------ src/config/zod-schema.agent-runtime.ts | 24 ++++++++++ src/security/audit-extra.sync.ts | 4 +- 4 files changed, 54 insertions(+), 44 deletions(-) diff --git a/src/agents/sandbox/validate-sandbox-security.test.ts b/src/agents/sandbox/validate-sandbox-security.test.ts index 91668c74e..ada189dd0 100644 --- a/src/agents/sandbox/validate-sandbox-security.test.ts +++ b/src/agents/sandbox/validate-sandbox-security.test.ts @@ -3,7 +3,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { describe, expect, it } from "vitest"; import { - getBlockedBindReasonStringOnly, + getBlockedBindReason, validateBindMounts, validateNetworkMode, validateSeccompProfile, @@ -11,18 +11,17 @@ import { validateSandboxSecurity, } from "./validate-sandbox-security.js"; -describe("getBlockedBindReasonStringOnly", () => { - it("blocks ancestor mounts that would expose the Docker socket", () => { - expect(getBlockedBindReasonStringOnly("/run:/run")).toEqual( - expect.objectContaining({ kind: "covers" }), - ); - expect(getBlockedBindReasonStringOnly("/var/run:/var/run:ro")).toEqual( - expect.objectContaining({ kind: "covers" }), - ); - expect(getBlockedBindReasonStringOnly("/var:/var")).toEqual( - expect.objectContaining({ kind: "covers" }), +describe("getBlockedBindReason", () => { + it("blocks common Docker socket directories", () => { + expect(getBlockedBindReason("/run:/run")).toEqual(expect.objectContaining({ kind: "targets" })); + expect(getBlockedBindReason("/var/run:/var/run:ro")).toEqual( + expect.objectContaining({ kind: "targets" }), ); }); + + it("does not block /var by default", () => { + expect(getBlockedBindReason("/var:/var")).toBeNull(); + }); }); describe("validateBindMounts", () => { @@ -62,7 +61,7 @@ describe("validateBindMounts", () => { it("blocks parent mounts that would expose the Docker socket", () => { expect(() => validateBindMounts(["/run:/run"])).toThrow(/blocked path/); expect(() => validateBindMounts(["/var/run:/var/run"])).toThrow(/blocked path/); - expect(() => validateBindMounts(["/var:/var"])).toThrow(/blocked path/); + expect(() => validateBindMounts(["/var:/var"])).not.toThrow(); }); it("blocks paths with .. traversal to dangerous directories", () => { diff --git a/src/agents/sandbox/validate-sandbox-security.ts b/src/agents/sandbox/validate-sandbox-security.ts index 57498e544..2ed84e9c9 100644 --- a/src/agents/sandbox/validate-sandbox-security.ts +++ b/src/agents/sandbox/validate-sandbox-security.ts @@ -18,6 +18,10 @@ export const BLOCKED_HOST_PATHS = [ "/dev", "/root", "/boot", + // Directories that commonly contain (or alias) the Docker socket. + "/run", + "/var/run", + "/private/var/run", "/var/run/docker.sock", "/private/var/run/docker.sock", "/run/docker.sock", @@ -58,28 +62,27 @@ export function normalizeHostPath(raw: string): string { * String-only blocked-path check (no filesystem I/O). * Blocks: * - binds that target blocked paths (equal or under) - * - binds that cover blocked paths (ancestor mounts like /run or /var) + * - binds that cover the system root (mounting "/" is never safe) * - non-absolute source paths (relative / volume names) because they are hard to validate safely */ -export function getBlockedBindReasonStringOnly(bind: string): BlockedBindReason | null { +export function getBlockedBindReason(bind: string): BlockedBindReason | null { const sourceRaw = parseBindSourcePath(bind); if (!sourceRaw.startsWith("/")) { return { kind: "non_absolute", sourcePath: sourceRaw }; } const normalized = normalizeHostPath(sourceRaw); + return getBlockedReasonForSourcePath(normalized); +} +export function getBlockedReasonForSourcePath(sourceNormalized: string): BlockedBindReason | null { + if (sourceNormalized === "/") { + return { kind: "covers", blockedPath: "/" }; + } for (const blocked of BLOCKED_HOST_PATHS) { - if (normalized === blocked || normalized.startsWith(blocked + "/")) { + if (sourceNormalized === blocked || sourceNormalized.startsWith(blocked + "/")) { return { kind: "targets", blockedPath: blocked }; } - // Ancestor mounts: mounting /run exposes /run/docker.sock. - if (normalized === "/") { - return { kind: "covers", blockedPath: blocked }; - } - if (blocked.startsWith(normalized + "/")) { - return { kind: "covers", blockedPath: blocked }; - } } return null; @@ -131,7 +134,7 @@ export function validateBindMounts(binds: string[] | undefined): void { } // Fast string-only check (covers .., //, ancestor/descendant logic). - const blocked = getBlockedBindReasonStringOnly(bind); + const blocked = getBlockedBindReason(bind); if (blocked) { throw formatBindBlockedError({ bind, reason: blocked }); } @@ -141,25 +144,9 @@ export function validateBindMounts(binds: string[] | undefined): void { const sourceNormalized = normalizeHostPath(sourceRaw); const sourceReal = tryRealpathAbsolute(sourceNormalized); if (sourceReal !== sourceNormalized) { - for (const blockedPath of BLOCKED_HOST_PATHS) { - if (sourceReal === blockedPath || sourceReal.startsWith(blockedPath + "/")) { - throw formatBindBlockedError({ - bind, - reason: { kind: "targets", blockedPath }, - }); - } - if (sourceReal === "/") { - throw formatBindBlockedError({ - bind, - reason: { kind: "covers", blockedPath }, - }); - } - if (blockedPath.startsWith(sourceReal + "/")) { - throw formatBindBlockedError({ - bind, - reason: { kind: "covers", blockedPath }, - }); - } + const reason = getBlockedReasonForSourcePath(sourceReal); + if (reason) { + throw formatBindBlockedError({ bind, reason }); } } } diff --git a/src/config/zod-schema.agent-runtime.ts b/src/config/zod-schema.agent-runtime.ts index e84bb38d7..1d2702acd 100644 --- a/src/config/zod-schema.agent-runtime.ts +++ b/src/config/zod-schema.agent-runtime.ts @@ -126,6 +126,30 @@ export const SandboxDockerSchema = z }) .strict() .superRefine((data, ctx) => { + if (data.binds) { + for (let i = 0; i < data.binds.length; i += 1) { + const bind = data.binds[i]?.trim() ?? ""; + if (!bind) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["binds", i], + message: "Sandbox security: bind mount entry must be a non-empty string.", + }); + continue; + } + const firstColon = bind.indexOf(":"); + const source = (firstColon <= 0 ? bind : bind.slice(0, firstColon)).trim(); + if (!source.startsWith("/")) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ["binds", i], + message: + `Sandbox security: bind mount "${bind}" uses a non-absolute source path "${source}". ` + + "Only absolute POSIX paths are supported for sandbox binds.", + }); + } + } + } if (data.network?.trim().toLowerCase() === "host") { ctx.addIssue({ code: z.ZodIssueCode.custom, diff --git a/src/security/audit-extra.sync.ts b/src/security/audit-extra.sync.ts index ea347ee2e..411920a9b 100644 --- a/src/security/audit-extra.sync.ts +++ b/src/security/audit-extra.sync.ts @@ -11,7 +11,7 @@ import { resolveSandboxConfigForAgent, resolveSandboxToolPolicyForAgent, } from "../agents/sandbox.js"; -import { getBlockedBindReasonStringOnly } from "../agents/sandbox/validate-sandbox-security.js"; +import { getBlockedBindReason } from "../agents/sandbox/validate-sandbox-security.js"; import { resolveToolProfilePolicy } from "../agents/tool-policy.js"; import { resolveBrowserConfig } from "../browser/config.js"; import { formatCliCommand } from "../cli/command-format.js"; @@ -616,7 +616,7 @@ export function collectSandboxDangerousConfigFindings(cfg: OpenClawConfig): Secu if (typeof bind !== "string") { continue; } - const blocked = getBlockedBindReasonStringOnly(bind); + const blocked = getBlockedBindReason(bind); if (!blocked) { continue; }