mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-11 20:13:43 +00:00
fix(security): harden root-scoped writes against symlink races
This commit is contained in:
@@ -104,6 +104,7 @@ Docs: https://docs.openclaw.ai
|
|||||||
|
|
||||||
- Security/Sandbox media reads: eliminate sandbox media TOCTOU symlink-retarget escapes by enforcing root-scoped boundary-safe reads at attachment/image load time and consolidating shared safe-read helpers across sandbox media callsites. This ships in the next npm release. Thanks @tdjackey for reporting.
|
- Security/Sandbox media reads: eliminate sandbox media TOCTOU symlink-retarget escapes by enforcing root-scoped boundary-safe reads at attachment/image load time and consolidating shared safe-read helpers across sandbox media callsites. This ships in the next npm release. Thanks @tdjackey for reporting.
|
||||||
- Security/Subagents sandbox inheritance: block sandboxed sessions from spawning cross-agent subagents that would run unsandboxed, preventing runtime sandbox downgrade via `sessions_spawn agentId`. Thanks @tdjackey for reporting.
|
- Security/Subagents sandbox inheritance: block sandboxed sessions from spawning cross-agent subagents that would run unsandboxed, preventing runtime sandbox downgrade via `sessions_spawn agentId`. Thanks @tdjackey for reporting.
|
||||||
|
- Security/Workspace safe writes: harden `writeFileWithinRoot` against symlink-retarget TOCTOU races by opening existing files without truncation, creating missing files with exclusive create, deferring truncation until post-open identity+boundary validation, and removing out-of-root create artifacts on blocked races; added regression tests for truncate/create race paths. This ships in the next npm release (`2026.3.1`). Thanks @tdjackey for reporting.
|
||||||
- Security/Node metadata policy: harden node platform classification against Unicode confusables and switch unknown platform defaults to a conservative allowlist that excludes `system.run`/`system.which` unless explicitly allowlisted, preventing metadata canonicalization drift from broadening node command permissions. Thanks @tdjackey for reporting.
|
- Security/Node metadata policy: harden node platform classification against Unicode confusables and switch unknown platform defaults to a conservative allowlist that excludes `system.run`/`system.which` unless explicitly allowlisted, preventing metadata canonicalization drift from broadening node command permissions. Thanks @tdjackey for reporting.
|
||||||
- Plugins/Discovery precedence: load bundled plugins before auto-discovered global extensions so bundled channel plugins win duplicate-ID resolution by default (explicit `plugins.load.paths` overrides remain highest precedence), with loader regression coverage. Landed from contributor PR #29710 by @Sid-Qin. Thanks @Sid-Qin.
|
- Plugins/Discovery precedence: load bundled plugins before auto-discovered global extensions so bundled channel plugins win duplicate-ID resolution by default (explicit `plugins.load.paths` overrides remain highest precedence), with loader regression coverage. Landed from contributor PR #29710 by @Sid-Qin. Thanks @Sid-Qin.
|
||||||
- Discord/Reconnect integrity: release Discord message listener lane immediately while preserving serialized handler execution, add HELLO-stall resume-first recovery with bounded fresh-identify fallback after repeated stalls, and extend lifecycle/listener regression coverage for forced reconnect scenarios. Landed from contributor PR #29508 by @cgdusek. Thanks @cgdusek.
|
- Discord/Reconnect integrity: release Discord message listener lane immediately while preserving serialized handler execution, add HELLO-stall resume-first recovery with bounded fresh-identify fallback after repeated stalls, and extend lifecycle/listener regression coverage for forced reconnect scenarios. Landed from contributor PR #29508 by @cgdusek. Thanks @cgdusek.
|
||||||
|
|||||||
@@ -217,6 +217,7 @@ beforeEach(() => {
|
|||||||
({
|
({
|
||||||
stat: async () => makeFileStat(),
|
stat: async () => makeFileStat(),
|
||||||
readFile: async () => Buffer.from(""),
|
readFile: async () => Buffer.from(""),
|
||||||
|
truncate: async () => {},
|
||||||
writeFile: async () => {},
|
writeFile: async () => {},
|
||||||
close: async () => {},
|
close: async () => {},
|
||||||
}) as unknown,
|
}) as unknown,
|
||||||
@@ -621,6 +622,7 @@ describe("agents.files.get/set symlink safety", () => {
|
|||||||
({
|
({
|
||||||
stat: async () => targetStat,
|
stat: async () => targetStat,
|
||||||
readFile: async () => Buffer.from("inside\n"),
|
readFile: async () => Buffer.from("inside\n"),
|
||||||
|
truncate: async () => {},
|
||||||
writeFile: async () => {},
|
writeFile: async () => {},
|
||||||
close: async () => {},
|
close: async () => {},
|
||||||
}) as unknown,
|
}) as unknown,
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
import fs from "node:fs/promises";
|
import fs from "node:fs/promises";
|
||||||
import path from "node:path";
|
import path from "node:path";
|
||||||
import { afterEach, describe, expect, it } from "vitest";
|
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||||
import { createTrackedTempDirs } from "../test-utils/tracked-temp-dirs.js";
|
import { createTrackedTempDirs } from "../test-utils/tracked-temp-dirs.js";
|
||||||
import {
|
import {
|
||||||
createRootScopedReadFile,
|
createRootScopedReadFile,
|
||||||
@@ -197,6 +197,87 @@ describe("fs-safe", () => {
|
|||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it.runIf(process.platform !== "win32")(
|
||||||
|
"does not truncate out-of-root file when symlink retarget races write open",
|
||||||
|
async () => {
|
||||||
|
const root = await tempDirs.make("openclaw-fs-safe-root-");
|
||||||
|
const inside = path.join(root, "inside");
|
||||||
|
const outside = await tempDirs.make("openclaw-fs-safe-outside-");
|
||||||
|
await fs.mkdir(inside, { recursive: true });
|
||||||
|
const insideTarget = path.join(inside, "target.txt");
|
||||||
|
const outsideTarget = path.join(outside, "target.txt");
|
||||||
|
await fs.writeFile(insideTarget, "inside");
|
||||||
|
await fs.writeFile(outsideTarget, "X".repeat(4096));
|
||||||
|
const slot = path.join(root, "slot");
|
||||||
|
await fs.symlink(inside, slot);
|
||||||
|
|
||||||
|
const realRealpath = fs.realpath.bind(fs);
|
||||||
|
let flipped = false;
|
||||||
|
const realpathSpy = vi.spyOn(fs, "realpath").mockImplementation(async (...args) => {
|
||||||
|
const [filePath] = args;
|
||||||
|
if (!flipped && String(filePath).endsWith(path.join("slot", "target.txt"))) {
|
||||||
|
flipped = true;
|
||||||
|
await fs.rm(slot, { recursive: true, force: true });
|
||||||
|
await fs.symlink(outside, slot);
|
||||||
|
}
|
||||||
|
return await realRealpath(...args);
|
||||||
|
});
|
||||||
|
try {
|
||||||
|
await expect(
|
||||||
|
writeFileWithinRoot({
|
||||||
|
rootDir: root,
|
||||||
|
relativePath: path.join("slot", "target.txt"),
|
||||||
|
data: "new-content",
|
||||||
|
mkdir: false,
|
||||||
|
}),
|
||||||
|
).rejects.toMatchObject({ code: "outside-workspace" });
|
||||||
|
} finally {
|
||||||
|
realpathSpy.mockRestore();
|
||||||
|
}
|
||||||
|
|
||||||
|
await expect(fs.readFile(outsideTarget, "utf8")).resolves.toBe("X".repeat(4096));
|
||||||
|
},
|
||||||
|
);
|
||||||
|
|
||||||
|
it.runIf(process.platform !== "win32")(
|
||||||
|
"cleans up created out-of-root file when symlink retarget races create path",
|
||||||
|
async () => {
|
||||||
|
const root = await tempDirs.make("openclaw-fs-safe-root-");
|
||||||
|
const inside = path.join(root, "inside");
|
||||||
|
const outside = await tempDirs.make("openclaw-fs-safe-outside-");
|
||||||
|
await fs.mkdir(inside, { recursive: true });
|
||||||
|
const outsideTarget = path.join(outside, "target.txt");
|
||||||
|
const slot = path.join(root, "slot");
|
||||||
|
await fs.symlink(inside, slot);
|
||||||
|
|
||||||
|
const realOpen = fs.open.bind(fs);
|
||||||
|
let flipped = false;
|
||||||
|
const openSpy = vi.spyOn(fs, "open").mockImplementation(async (...args) => {
|
||||||
|
const [filePath] = args;
|
||||||
|
if (!flipped && String(filePath).endsWith(path.join("slot", "target.txt"))) {
|
||||||
|
flipped = true;
|
||||||
|
await fs.rm(slot, { recursive: true, force: true });
|
||||||
|
await fs.symlink(outside, slot);
|
||||||
|
}
|
||||||
|
return await realOpen(...args);
|
||||||
|
});
|
||||||
|
try {
|
||||||
|
await expect(
|
||||||
|
writeFileWithinRoot({
|
||||||
|
rootDir: root,
|
||||||
|
relativePath: path.join("slot", "target.txt"),
|
||||||
|
data: "new-content",
|
||||||
|
mkdir: false,
|
||||||
|
}),
|
||||||
|
).rejects.toMatchObject({ code: "outside-workspace" });
|
||||||
|
} finally {
|
||||||
|
openSpy.mockRestore();
|
||||||
|
}
|
||||||
|
|
||||||
|
await expect(fs.stat(outsideTarget)).rejects.toMatchObject({ code: "ENOENT" });
|
||||||
|
},
|
||||||
|
);
|
||||||
|
|
||||||
it("returns not-found for missing files", async () => {
|
it("returns not-found for missing files", async () => {
|
||||||
const dir = await tempDirs.make("openclaw-fs-safe-");
|
const dir = await tempDirs.make("openclaw-fs-safe-");
|
||||||
const missing = path.join(dir, "missing.txt");
|
const missing = path.join(dir, "missing.txt");
|
||||||
|
|||||||
@@ -42,10 +42,12 @@ export type SafeLocalReadResult = {
|
|||||||
|
|
||||||
const SUPPORTS_NOFOLLOW = process.platform !== "win32" && "O_NOFOLLOW" in fsConstants;
|
const SUPPORTS_NOFOLLOW = process.platform !== "win32" && "O_NOFOLLOW" in fsConstants;
|
||||||
const OPEN_READ_FLAGS = fsConstants.O_RDONLY | (SUPPORTS_NOFOLLOW ? fsConstants.O_NOFOLLOW : 0);
|
const OPEN_READ_FLAGS = fsConstants.O_RDONLY | (SUPPORTS_NOFOLLOW ? fsConstants.O_NOFOLLOW : 0);
|
||||||
const OPEN_WRITE_FLAGS =
|
const OPEN_WRITE_EXISTING_FLAGS =
|
||||||
|
fsConstants.O_WRONLY | (SUPPORTS_NOFOLLOW ? fsConstants.O_NOFOLLOW : 0);
|
||||||
|
const OPEN_WRITE_CREATE_FLAGS =
|
||||||
fsConstants.O_WRONLY |
|
fsConstants.O_WRONLY |
|
||||||
fsConstants.O_CREAT |
|
fsConstants.O_CREAT |
|
||||||
fsConstants.O_TRUNC |
|
fsConstants.O_EXCL |
|
||||||
(SUPPORTS_NOFOLLOW ? fsConstants.O_NOFOLLOW : 0);
|
(SUPPORTS_NOFOLLOW ? fsConstants.O_NOFOLLOW : 0);
|
||||||
|
|
||||||
const ensureTrailingSep = (value: string) => (value.endsWith(path.sep) ? value : value + path.sep);
|
const ensureTrailingSep = (value: string) => (value.endsWith(path.sep) ? value : value + path.sep);
|
||||||
@@ -301,8 +303,17 @@ export async function writeFileWithinRoot(params: {
|
|||||||
}
|
}
|
||||||
|
|
||||||
let handle: FileHandle;
|
let handle: FileHandle;
|
||||||
|
let createdForWrite = false;
|
||||||
try {
|
try {
|
||||||
handle = await fs.open(ioPath, OPEN_WRITE_FLAGS, 0o600);
|
try {
|
||||||
|
handle = await fs.open(ioPath, OPEN_WRITE_EXISTING_FLAGS, 0o600);
|
||||||
|
} catch (err) {
|
||||||
|
if (!isNotFoundPathError(err)) {
|
||||||
|
throw err;
|
||||||
|
}
|
||||||
|
handle = await fs.open(ioPath, OPEN_WRITE_CREATE_FLAGS, 0o600);
|
||||||
|
createdForWrite = true;
|
||||||
|
}
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
if (isNotFoundPathError(err)) {
|
if (isNotFoundPathError(err)) {
|
||||||
throw new SafeOpenError("not-found", "file not found");
|
throw new SafeOpenError("not-found", "file not found");
|
||||||
@@ -313,6 +324,7 @@ export async function writeFileWithinRoot(params: {
|
|||||||
throw err;
|
throw err;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
let openedRealPath: string | null = null;
|
||||||
try {
|
try {
|
||||||
const [stat, lstat] = await Promise.all([handle.stat(), fs.lstat(ioPath)]);
|
const [stat, lstat] = await Promise.all([handle.stat(), fs.lstat(ioPath)]);
|
||||||
if (lstat.isSymbolicLink() || !stat.isFile()) {
|
if (lstat.isSymbolicLink() || !stat.isFile()) {
|
||||||
@@ -326,6 +338,7 @@ export async function writeFileWithinRoot(params: {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const realPath = await fs.realpath(ioPath);
|
const realPath = await fs.realpath(ioPath);
|
||||||
|
openedRealPath = realPath;
|
||||||
const realStat = await fs.stat(realPath);
|
const realStat = await fs.stat(realPath);
|
||||||
if (!sameFileIdentity(stat, realStat)) {
|
if (!sameFileIdentity(stat, realStat)) {
|
||||||
throw new SafeOpenError("path-mismatch", "path mismatch");
|
throw new SafeOpenError("path-mismatch", "path mismatch");
|
||||||
@@ -337,11 +350,21 @@ export async function writeFileWithinRoot(params: {
|
|||||||
throw new SafeOpenError("outside-workspace", "file is outside workspace root");
|
throw new SafeOpenError("outside-workspace", "file is outside workspace root");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Truncate only after boundary and identity checks complete. This avoids
|
||||||
|
// irreversible side effects if a symlink target changes before validation.
|
||||||
|
if (!createdForWrite) {
|
||||||
|
await handle.truncate(0);
|
||||||
|
}
|
||||||
if (typeof params.data === "string") {
|
if (typeof params.data === "string") {
|
||||||
await handle.writeFile(params.data, params.encoding ?? "utf8");
|
await handle.writeFile(params.data, params.encoding ?? "utf8");
|
||||||
} else {
|
} else {
|
||||||
await handle.writeFile(params.data);
|
await handle.writeFile(params.data);
|
||||||
}
|
}
|
||||||
|
} catch (err) {
|
||||||
|
if (createdForWrite && err instanceof SafeOpenError && openedRealPath) {
|
||||||
|
await fs.rm(openedRealPath, { force: true }).catch(() => {});
|
||||||
|
}
|
||||||
|
throw err;
|
||||||
} finally {
|
} finally {
|
||||||
await handle.close().catch(() => {});
|
await handle.close().catch(() => {});
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user