mirror of
https://github.com/openclaw/openclaw.git
synced 2026-04-18 16:07:28 +00:00
sandbox: allow directory boundary checks for mkdirp
This commit is contained in:
committed by
Peter Steinberger
parent
4fc7ecf088
commit
687f5779d1
@@ -173,6 +173,31 @@ describe("sandbox fs bridge shell compatibility", () => {
|
||||
expect(mockedExecDockerRaw).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("allows mkdirp for existing in-boundary subdirectories", async () => {
|
||||
const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-fs-bridge-mkdirp-"));
|
||||
try {
|
||||
const workspaceDir = path.join(stateDir, "workspace");
|
||||
const nestedDir = path.join(workspaceDir, "memory", "kemik");
|
||||
await fs.mkdir(nestedDir, { recursive: true });
|
||||
|
||||
const bridge = createSandboxFsBridge({
|
||||
sandbox: createSandbox({
|
||||
workspaceDir,
|
||||
agentWorkspaceDir: workspaceDir,
|
||||
}),
|
||||
});
|
||||
|
||||
await expect(bridge.mkdirp({ filePath: "memory/kemik" })).resolves.toBeUndefined();
|
||||
|
||||
const mkdirCall = findCallByScriptFragment('mkdir -p -- "$1"');
|
||||
expect(mkdirCall).toBeDefined();
|
||||
const mkdirPath = mkdirCall ? getDockerPathArg(mkdirCall[0]) : "";
|
||||
expect(mkdirPath).toBe("/workspace/memory/kemik");
|
||||
} finally {
|
||||
await fs.rm(stateDir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it("rejects pre-existing host symlink escapes before docker exec", async () => {
|
||||
const stateDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-fs-bridge-"));
|
||||
const workspaceDir = path.join(stateDir, "workspace");
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import fs from "node:fs";
|
||||
import { openBoundaryFile } from "../../infra/boundary-file-read.js";
|
||||
import { PATH_ALIAS_POLICIES, type PathAliasPolicy } from "../../infra/path-alias-guards.js";
|
||||
import type { SafeOpenSyncAllowedType } from "../../infra/safe-open-sync.js";
|
||||
import { execDockerRaw, type ExecDockerRawResult } from "./docker.js";
|
||||
import {
|
||||
buildSandboxFsMounts,
|
||||
@@ -23,6 +24,7 @@ type PathSafetyOptions = {
|
||||
aliasPolicy?: PathAliasPolicy;
|
||||
requireWritable?: boolean;
|
||||
allowMissingTarget?: boolean;
|
||||
allowedTypes?: readonly SafeOpenSyncAllowedType[];
|
||||
};
|
||||
|
||||
export type SandboxResolvedPath = {
|
||||
@@ -132,7 +134,11 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
|
||||
async mkdirp(params: { filePath: string; cwd?: string; signal?: AbortSignal }): Promise<void> {
|
||||
const target = this.resolveResolvedPath(params);
|
||||
this.ensureWriteAccess(target, "create directories");
|
||||
await this.assertPathSafety(target, { action: "create directories", requireWritable: true });
|
||||
await this.assertPathSafety(target, {
|
||||
action: "create directories",
|
||||
requireWritable: true,
|
||||
allowedTypes: ["directory"],
|
||||
});
|
||||
await this.runCommand('set -eu; mkdir -p -- "$1"', {
|
||||
args: [target.containerPath],
|
||||
signal: params.signal,
|
||||
@@ -258,6 +264,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
|
||||
rootPath: lexicalMount.hostRoot,
|
||||
boundaryLabel: "sandbox mount root",
|
||||
aliasPolicy: options.aliasPolicy,
|
||||
allowedTypes: options.allowedTypes,
|
||||
});
|
||||
if (!guarded.ok) {
|
||||
if (guarded.reason !== "path" || options.allowMissingTarget === false) {
|
||||
|
||||
@@ -2,7 +2,11 @@ import fs from "node:fs";
|
||||
import path from "node:path";
|
||||
import { resolveBoundaryPath, resolveBoundaryPathSync } from "./boundary-path.js";
|
||||
import type { PathAliasPolicy } from "./path-alias-guards.js";
|
||||
import { openVerifiedFileSync, type SafeOpenSyncFailureReason } from "./safe-open-sync.js";
|
||||
import {
|
||||
openVerifiedFileSync,
|
||||
type SafeOpenSyncAllowedType,
|
||||
type SafeOpenSyncFailureReason,
|
||||
} from "./safe-open-sync.js";
|
||||
|
||||
type BoundaryReadFs = Pick<
|
||||
typeof fs,
|
||||
@@ -28,6 +32,7 @@ export type OpenBoundaryFileSyncParams = {
|
||||
rootRealPath?: string;
|
||||
maxBytes?: number;
|
||||
rejectHardlinks?: boolean;
|
||||
allowedTypes?: readonly SafeOpenSyncAllowedType[];
|
||||
skipLexicalRootCheck?: boolean;
|
||||
ioFs?: BoundaryReadFs;
|
||||
};
|
||||
@@ -74,6 +79,7 @@ export function openBoundaryFileSync(params: OpenBoundaryFileSyncParams): Bounda
|
||||
resolvedPath,
|
||||
rejectHardlinks: params.rejectHardlinks ?? true,
|
||||
maxBytes: params.maxBytes,
|
||||
allowedTypes: params.allowedTypes,
|
||||
ioFs,
|
||||
});
|
||||
if (!opened.ok) {
|
||||
|
||||
49
src/infra/safe-open-sync.test.ts
Normal file
49
src/infra/safe-open-sync.test.ts
Normal file
@@ -0,0 +1,49 @@
|
||||
import fs from "node:fs";
|
||||
import fsp from "node:fs/promises";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { openVerifiedFileSync } from "./safe-open-sync.js";
|
||||
|
||||
async function withTempDir<T>(prefix: string, run: (dir: string) => Promise<T>): Promise<T> {
|
||||
const dir = await fsp.mkdtemp(path.join(os.tmpdir(), prefix));
|
||||
try {
|
||||
return await run(dir);
|
||||
} finally {
|
||||
await fsp.rm(dir, { recursive: true, force: true });
|
||||
}
|
||||
}
|
||||
|
||||
describe("openVerifiedFileSync", () => {
|
||||
it("rejects directories by default", async () => {
|
||||
await withTempDir("openclaw-safe-open-", async (root) => {
|
||||
const targetDir = path.join(root, "nested");
|
||||
await fsp.mkdir(targetDir, { recursive: true });
|
||||
|
||||
const opened = openVerifiedFileSync({ filePath: targetDir });
|
||||
expect(opened.ok).toBe(false);
|
||||
if (!opened.ok) {
|
||||
expect(opened.reason).toBe("validation");
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
it("accepts directories when allowedTypes includes directory", async () => {
|
||||
await withTempDir("openclaw-safe-open-", async (root) => {
|
||||
const targetDir = path.join(root, "nested");
|
||||
await fsp.mkdir(targetDir, { recursive: true });
|
||||
|
||||
const opened = openVerifiedFileSync({
|
||||
filePath: targetDir,
|
||||
allowedTypes: ["directory"],
|
||||
rejectHardlinks: true,
|
||||
});
|
||||
expect(opened.ok).toBe(true);
|
||||
if (!opened.ok) {
|
||||
return;
|
||||
}
|
||||
expect(opened.stat.isDirectory()).toBe(true);
|
||||
fs.closeSync(opened.fd);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -7,6 +7,8 @@ export type SafeOpenSyncResult =
|
||||
| { ok: true; path: string; fd: number; stat: fs.Stats }
|
||||
| { ok: false; reason: SafeOpenSyncFailureReason; error?: unknown };
|
||||
|
||||
export type SafeOpenSyncAllowedType = "file" | "directory";
|
||||
|
||||
type SafeOpenSyncFs = Pick<
|
||||
typeof fs,
|
||||
"constants" | "lstatSync" | "realpathSync" | "openSync" | "fstatSync" | "closeSync"
|
||||
@@ -28,9 +30,11 @@ export function openVerifiedFileSync(params: {
|
||||
rejectPathSymlink?: boolean;
|
||||
rejectHardlinks?: boolean;
|
||||
maxBytes?: number;
|
||||
allowedTypes?: readonly SafeOpenSyncAllowedType[];
|
||||
ioFs?: SafeOpenSyncFs;
|
||||
}): SafeOpenSyncResult {
|
||||
const ioFs = params.ioFs ?? fs;
|
||||
const allowedTypes = params.allowedTypes ?? ["file"];
|
||||
const openReadFlags =
|
||||
ioFs.constants.O_RDONLY |
|
||||
(typeof ioFs.constants.O_NOFOLLOW === "number" ? ioFs.constants.O_NOFOLLOW : 0);
|
||||
@@ -45,25 +49,29 @@ export function openVerifiedFileSync(params: {
|
||||
|
||||
const realPath = params.resolvedPath ?? ioFs.realpathSync(params.filePath);
|
||||
const preOpenStat = ioFs.lstatSync(realPath);
|
||||
if (!preOpenStat.isFile()) {
|
||||
if (!isAllowedType(preOpenStat, allowedTypes)) {
|
||||
return { ok: false, reason: "validation" };
|
||||
}
|
||||
if (params.rejectHardlinks && preOpenStat.nlink > 1) {
|
||||
if (params.rejectHardlinks && preOpenStat.isFile() && preOpenStat.nlink > 1) {
|
||||
return { ok: false, reason: "validation" };
|
||||
}
|
||||
if (params.maxBytes !== undefined && preOpenStat.size > params.maxBytes) {
|
||||
if (
|
||||
params.maxBytes !== undefined &&
|
||||
preOpenStat.isFile() &&
|
||||
preOpenStat.size > params.maxBytes
|
||||
) {
|
||||
return { ok: false, reason: "validation" };
|
||||
}
|
||||
|
||||
fd = ioFs.openSync(realPath, openReadFlags);
|
||||
const openedStat = ioFs.fstatSync(fd);
|
||||
if (!openedStat.isFile()) {
|
||||
if (!isAllowedType(openedStat, allowedTypes)) {
|
||||
return { ok: false, reason: "validation" };
|
||||
}
|
||||
if (params.rejectHardlinks && openedStat.nlink > 1) {
|
||||
if (params.rejectHardlinks && openedStat.isFile() && openedStat.nlink > 1) {
|
||||
return { ok: false, reason: "validation" };
|
||||
}
|
||||
if (params.maxBytes !== undefined && openedStat.size > params.maxBytes) {
|
||||
if (params.maxBytes !== undefined && openedStat.isFile() && openedStat.size > params.maxBytes) {
|
||||
return { ok: false, reason: "validation" };
|
||||
}
|
||||
if (!sameFileIdentity(preOpenStat, openedStat)) {
|
||||
@@ -84,3 +92,12 @@ export function openVerifiedFileSync(params: {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
function isAllowedType(stat: fs.Stats, allowedTypes: readonly SafeOpenSyncAllowedType[]): boolean {
|
||||
return allowedTypes.some((allowedType) => {
|
||||
if (allowedType === "file") {
|
||||
return stat.isFile();
|
||||
}
|
||||
return stat.isDirectory();
|
||||
});
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user