refactor(security): tighten sandbox bind validation

This commit is contained in:
Peter Steinberger
2026-02-16 03:19:38 +01:00
parent a74251d415
commit a7cbce1b3d
4 changed files with 54 additions and 44 deletions

View File

@@ -3,7 +3,7 @@ import { tmpdir } from "node:os";
import { join } from "node:path"; import { join } from "node:path";
import { describe, expect, it } from "vitest"; import { describe, expect, it } from "vitest";
import { import {
getBlockedBindReasonStringOnly, getBlockedBindReason,
validateBindMounts, validateBindMounts,
validateNetworkMode, validateNetworkMode,
validateSeccompProfile, validateSeccompProfile,
@@ -11,18 +11,17 @@ import {
validateSandboxSecurity, validateSandboxSecurity,
} from "./validate-sandbox-security.js"; } from "./validate-sandbox-security.js";
describe("getBlockedBindReasonStringOnly", () => { describe("getBlockedBindReason", () => {
it("blocks ancestor mounts that would expose the Docker socket", () => { it("blocks common Docker socket directories", () => {
expect(getBlockedBindReasonStringOnly("/run:/run")).toEqual( expect(getBlockedBindReason("/run:/run")).toEqual(expect.objectContaining({ kind: "targets" }));
expect.objectContaining({ kind: "covers" }), expect(getBlockedBindReason("/var/run:/var/run:ro")).toEqual(
); expect.objectContaining({ kind: "targets" }),
expect(getBlockedBindReasonStringOnly("/var/run:/var/run:ro")).toEqual(
expect.objectContaining({ kind: "covers" }),
);
expect(getBlockedBindReasonStringOnly("/var:/var")).toEqual(
expect.objectContaining({ kind: "covers" }),
); );
}); });
it("does not block /var by default", () => {
expect(getBlockedBindReason("/var:/var")).toBeNull();
});
}); });
describe("validateBindMounts", () => { describe("validateBindMounts", () => {
@@ -62,7 +61,7 @@ describe("validateBindMounts", () => {
it("blocks parent mounts that would expose the Docker socket", () => { it("blocks parent mounts that would expose the Docker socket", () => {
expect(() => validateBindMounts(["/run:/run"])).toThrow(/blocked path/); expect(() => validateBindMounts(["/run:/run"])).toThrow(/blocked path/);
expect(() => validateBindMounts(["/var/run:/var/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", () => { it("blocks paths with .. traversal to dangerous directories", () => {

View File

@@ -18,6 +18,10 @@ export const BLOCKED_HOST_PATHS = [
"/dev", "/dev",
"/root", "/root",
"/boot", "/boot",
// Directories that commonly contain (or alias) the Docker socket.
"/run",
"/var/run",
"/private/var/run",
"/var/run/docker.sock", "/var/run/docker.sock",
"/private/var/run/docker.sock", "/private/var/run/docker.sock",
"/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). * String-only blocked-path check (no filesystem I/O).
* Blocks: * Blocks:
* - binds that target blocked paths (equal or under) * - 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 * - 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); const sourceRaw = parseBindSourcePath(bind);
if (!sourceRaw.startsWith("/")) { if (!sourceRaw.startsWith("/")) {
return { kind: "non_absolute", sourcePath: sourceRaw }; return { kind: "non_absolute", sourcePath: sourceRaw };
} }
const normalized = normalizeHostPath(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) { for (const blocked of BLOCKED_HOST_PATHS) {
if (normalized === blocked || normalized.startsWith(blocked + "/")) { if (sourceNormalized === blocked || sourceNormalized.startsWith(blocked + "/")) {
return { kind: "targets", blockedPath: 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; return null;
@@ -131,7 +134,7 @@ export function validateBindMounts(binds: string[] | undefined): void {
} }
// Fast string-only check (covers .., //, ancestor/descendant logic). // Fast string-only check (covers .., //, ancestor/descendant logic).
const blocked = getBlockedBindReasonStringOnly(bind); const blocked = getBlockedBindReason(bind);
if (blocked) { if (blocked) {
throw formatBindBlockedError({ bind, reason: blocked }); throw formatBindBlockedError({ bind, reason: blocked });
} }
@@ -141,25 +144,9 @@ export function validateBindMounts(binds: string[] | undefined): void {
const sourceNormalized = normalizeHostPath(sourceRaw); const sourceNormalized = normalizeHostPath(sourceRaw);
const sourceReal = tryRealpathAbsolute(sourceNormalized); const sourceReal = tryRealpathAbsolute(sourceNormalized);
if (sourceReal !== sourceNormalized) { if (sourceReal !== sourceNormalized) {
for (const blockedPath of BLOCKED_HOST_PATHS) { const reason = getBlockedReasonForSourcePath(sourceReal);
if (sourceReal === blockedPath || sourceReal.startsWith(blockedPath + "/")) { if (reason) {
throw formatBindBlockedError({ throw formatBindBlockedError({ bind, reason });
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 },
});
}
} }
} }
} }

View File

@@ -126,6 +126,30 @@ export const SandboxDockerSchema = z
}) })
.strict() .strict()
.superRefine((data, ctx) => { .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") { if (data.network?.trim().toLowerCase() === "host") {
ctx.addIssue({ ctx.addIssue({
code: z.ZodIssueCode.custom, code: z.ZodIssueCode.custom,

View File

@@ -11,7 +11,7 @@ import {
resolveSandboxConfigForAgent, resolveSandboxConfigForAgent,
resolveSandboxToolPolicyForAgent, resolveSandboxToolPolicyForAgent,
} from "../agents/sandbox.js"; } 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 { resolveToolProfilePolicy } from "../agents/tool-policy.js";
import { resolveBrowserConfig } from "../browser/config.js"; import { resolveBrowserConfig } from "../browser/config.js";
import { formatCliCommand } from "../cli/command-format.js"; import { formatCliCommand } from "../cli/command-format.js";
@@ -616,7 +616,7 @@ export function collectSandboxDangerousConfigFindings(cfg: OpenClawConfig): Secu
if (typeof bind !== "string") { if (typeof bind !== "string") {
continue; continue;
} }
const blocked = getBlockedBindReasonStringOnly(bind); const blocked = getBlockedBindReason(bind);
if (!blocked) { if (!blocked) {
continue; continue;
} }