diff --git a/nemoclaw-blueprint/scripts/ws-proxy-fix.js b/nemoclaw-blueprint/scripts/ws-proxy-fix.js index 1b9a9626750..9b1e1b5d348 100644 --- a/nemoclaw-blueprint/scripts/ws-proxy-fix.js +++ b/nemoclaw-blueprint/scripts/ws-proxy-fix.js @@ -176,7 +176,9 @@ const _PATCHED = Symbol.for("nemoclaw.wsProxyFix"); if (isDiscordWsUpgrade(host, opts.headers)) { // Guard: if isDiscordWsUpgrade matched but host resolved to // undefined, we cannot construct a CONNECT tunnel (no target). - // Fall through to the original https.request unchanged. + // Fall through to the original https.request unchanged. Before + // PR #2422 this path would have attempted the tunnel with an + // undefined host, which would fail in createTunnelAgent anyway. if (!host) { return callOriginalRequest(input, options, callback); } diff --git a/nemoclaw/src/blueprint/snapshot.test.ts b/nemoclaw/src/blueprint/snapshot.test.ts index b65e9e5222d..9079f816d7b 100644 --- a/nemoclaw/src/blueprint/snapshot.test.ts +++ b/nemoclaw/src/blueprint/snapshot.test.ts @@ -8,8 +8,9 @@ const SNAP = "/snap/20260323"; // ── In-memory filesystem ──────────────────────────────────────── interface FsEntry { - type: "file" | "dir"; + type: "file" | "dir" | "symlink"; content?: string; + target?: string; } const store = new Map(); @@ -22,6 +23,10 @@ function addDir(p: string): void { store.set(p, { type: "dir" }); } +function addSymlink(p: string, target: string): void { + store.set(p, { type: "symlink", target }); +} + const FAKE_HOME = "/fakehome"; vi.mock("node:os", () => ({ @@ -33,6 +38,28 @@ vi.mock("node:fs", async (importOriginal) => { return { ...original, existsSync: (p: string) => store.has(p), + lstatSync: (p: string) => { + const entry = store.get(p); + if (!entry) { + throw Object.assign(new Error(`ENOENT: no such file or directory, lstat '${p}'`), { + code: "ENOENT", + }); + } + return { + isSymbolicLink: () => entry.type === "symlink", + isDirectory: () => entry.type === "dir", + isFile: () => entry.type === "file", + }; + }, + readlinkSync: (p: string) => { + const entry = store.get(p); + if (entry?.type !== "symlink") { + throw Object.assign(new Error(`EINVAL: invalid argument, readlink '${p}'`), { + code: "EINVAL", + }); + } + return entry.target ?? ""; + }, mkdirSync: vi.fn((p: string) => { addDir(p); }), @@ -70,7 +97,7 @@ vi.mock("node:fs", async (importOriginal) => { }), readdirSync: (p: string, opts?: { withFileTypes?: boolean }) => { const prefix = p.endsWith("/") ? p : p + "/"; - const childTypes = new Map(); + const childTypes = new Map(); for (const [k, v] of store) { if (k.startsWith(prefix)) { const rest = k.slice(prefix.length); @@ -92,6 +119,7 @@ vi.mock("node:fs", async (importOriginal) => { name, isDirectory: () => type === "dir", isFile: () => type === "file", + isSymbolicLink: () => type === "symlink", })); } return [...childTypes.keys()].sort(); @@ -153,6 +181,37 @@ describe("snapshot", () => { expect(manifest.contents).toContain("openclaw.json"); expect(manifest.contents).toContain("hooks/demo/HOOK.md"); }); + + it("rejects when ~/.openclaw is a symlink", () => { + addSymlink(OPENCLAW_DIR, "/etc"); + + expect(() => createSnapshot()).toThrow(/symbolic link/); + }); + + it("rejects when an ancestor of ~/.nemoclaw is a symlink", () => { + addDir(OPENCLAW_DIR); + addSymlink(`${FAKE_HOME}/.nemoclaw`, "/attacker-controlled"); + + expect(() => createSnapshot()).toThrow(/symbolic link/); + }); + + it("records symlinks in manifest when present in tree", () => { + addDir(OPENCLAW_DIR); + addFile(`${OPENCLAW_DIR}/openclaw.json`, '{"version":"1"}'); + addSymlink(`${OPENCLAW_DIR}/evil`, "/etc/shadow"); + + const result = createSnapshot(); + expect(result).not.toBeNull(); + if (!result) throw new Error("createSnapshot returned null"); + + const manifestPath = `${result}/snapshot.json`; + const entry = store.get(manifestPath); + if (!entry?.content) throw new Error("manifest not written"); + const manifest = JSON.parse(entry.content); + expect(manifest.file_count).toBe(1); + expect(manifest.contents).toContain("openclaw.json"); + expect(manifest.symlinks).toContain("evil"); + }); }); describe("restoreIntoSandbox", () => { @@ -345,6 +404,14 @@ describe("snapshot", () => { const archived = [...store.keys()].find((k) => k.includes(".openclaw.nemoclaw-archived.")); expect(archived).toBeDefined(); }); + + it("returns false when ~/.openclaw is a symlink", () => { + addDir(`${SNAP}/openclaw`); + addFile(`${SNAP}/openclaw/openclaw.json`, '{"restored":true}'); + addSymlink(OPENCLAW_DIR, "/attacker-controlled"); + + expect(rollbackFromSnapshot(SNAP)).toBe(false); + }); }); describe("listSnapshots", () => { diff --git a/nemoclaw/src/blueprint/snapshot.ts b/nemoclaw/src/blueprint/snapshot.ts index 93095eddb0e..e92d4a1aa4d 100644 --- a/nemoclaw/src/blueprint/snapshot.ts +++ b/nemoclaw/src/blueprint/snapshot.ts @@ -15,15 +15,17 @@ import type { Dirent } from "node:fs"; import { cpSync, existsSync, + lstatSync, mkdirSync, readdirSync, readFileSync, + readlinkSync, renameSync, rmSync, writeFileSync, } from "node:fs"; import { homedir } from "node:os"; -import { join, relative } from "node:path"; +import { dirname, isAbsolute, join, relative, resolve } from "node:path"; import { execa } from "execa"; @@ -39,12 +41,49 @@ function compactTimestamp(): string { .replace(/\.\d+Z$/, "Z"); } -function collectFiles(dir: string): string[] { +/** + * Reject a path if it — or any ancestor up to $HOME — is a symlink. + * Prevents an attacker from planting a symlink at the target path to + * redirect reads or writes to an attacker-controlled directory. + * + * Mirrors the pattern from src/lib/config-io.ts (PR #2290). + */ +function rejectSymlinksOnPath(targetPath: string): void { + const resolvedHome = resolve(HOME); + const resolved = resolve(targetPath); + + const relToHome = relative(resolvedHome, resolved); + if (relToHome === "" || relToHome.startsWith("..") || isAbsolute(relToHome)) { + return; + } + + let current = resolved; + while (current !== resolvedHome && current !== dirname(current)) { + try { + const stat = lstatSync(current); + if (stat.isSymbolicLink()) { + const linkTarget = readlinkSync(current); + throw new Error( + `Refusing to operate on path: ${current} is a symbolic link ` + + `(target: ${linkTarget}). This may indicate a symlink attack.`, + ); + } + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error; + } + current = dirname(current); + } +} + +function collectFiles(dir: string): { files: string[]; symlinks: string[] } { const files: string[] = []; + const symlinks: string[] = []; const walk = (current: string): void => { for (const entry of readdirSync(current, { withFileTypes: true })) { const full = join(current, entry.name); - if (entry.isDirectory()) { + if (entry.isSymbolicLink()) { + symlinks.push(relative(dir, full)); + } else if (entry.isDirectory()) { walk(full); } else if (entry.isFile()) { files.push(relative(dir, full)); @@ -52,7 +91,7 @@ function collectFiles(dir: string): string[] { } }; walk(dir); - return files; + return { files, symlinks }; } export function createSnapshot(): string | null { @@ -60,20 +99,33 @@ export function createSnapshot(): string | null { return null; } + // SECURITY: Verify source path is not a symlink before copying. + // Without this check, an attacker who replaces ~/.openclaw with a symlink + // to an arbitrary directory (e.g. /etc) could cause cpSync to copy + // sensitive files into the snapshot. + rejectSymlinksOnPath(OPENCLAW_DIR); + const timestamp = compactTimestamp(); const snapshotDir = join(SNAPSHOTS_DIR, timestamp); + + // SECURITY: Verify snapshot destination ancestors are not symlinks. + rejectSymlinksOnPath(snapshotDir); + mkdirSync(snapshotDir, { recursive: true }); const dest = join(snapshotDir, "openclaw"); cpSync(OPENCLAW_DIR, dest, { recursive: true }); - const contents = collectFiles(dest); - const manifest = { + const { files, symlinks } = collectFiles(dest); + const manifest: Record = { timestamp, source: OPENCLAW_DIR, - file_count: contents.length, - contents, + file_count: files.length, + contents: files, }; + if (symlinks.length > 0) { + manifest.symlinks = symlinks; + } writeFileSync(join(snapshotDir, "snapshot.json"), JSON.stringify(manifest, null, 2)); return snapshotDir; @@ -176,6 +228,12 @@ export function rollbackFromSnapshot(snapshotDir: string): boolean { : null; try { + // SECURITY: Verify restore destination is not a symlink before writing. + // Without this check, an attacker who replaces ~/.openclaw with a symlink + // could redirect snapshot contents to an arbitrary directory. + // Inside the try/catch to preserve the boolean-return contract. + rejectSymlinksOnPath(OPENCLAW_DIR); + if (archivePath !== null) { moveSync(OPENCLAW_DIR, archivePath); } diff --git a/src/lib/sandbox-state.ts b/src/lib/sandbox-state.ts index 7c0a4153e10..69ed5a1ccbd 100644 --- a/src/lib/sandbox-state.ts +++ b/src/lib/sandbox-state.ts @@ -31,7 +31,8 @@ import { resolveOpenshell } from "./resolve-openshell.js"; import { captureOpenshellCommand } from "./openshell.js"; import { sanitizeConfigFile, isSensitiveFile } from "./credential-filter.js"; -const REBUILD_BACKUPS_DIR = path.join(process.env.HOME || "/tmp", ".nemoclaw", "rebuild-backups"); +const HOME_DIR = path.resolve(process.env.HOME || os.homedir()); +const REBUILD_BACKUPS_DIR = path.join(HOME_DIR, ".nemoclaw", "rebuild-backups"); const MANIFEST_VERSION = 1; @@ -170,6 +171,41 @@ function isWithinRoot(candidatePath: string, rootPath: string): boolean { return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative)); } +/** + * Reject a path if it — or any ancestor up to $HOME — is a symlink. + * Prevents an attacker from planting a symlink at the target path to + * redirect reads or writes to an attacker-controlled directory. + * + * Mirrors the pattern from config-io.ts (PR #2290) and + * nemoclaw/src/blueprint/snapshot.ts. + */ +function rejectSymlinksOnPath(targetPath: string): void { + const home = HOME_DIR; + const resolved = path.resolve(targetPath); + + const relToHome = path.relative(home, resolved); + if (relToHome === "" || relToHome.startsWith("..") || path.isAbsolute(relToHome)) { + return; + } + + let current = resolved; + while (current !== home && current !== path.dirname(current)) { + try { + const stat = lstatSync(current); + if (stat.isSymbolicLink()) { + const linkTarget = readlinkSync(current); + throw new Error( + `Refusing to operate on path: ${current} is a symbolic link ` + + `(target: ${linkTarget}). This may indicate a symlink attack.`, + ); + } + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error; + } + current = path.dirname(current); + } +} + /** * List tar entries and validate every path is within targetDir. * Rejects absolute paths, path traversal (..), and null bytes. @@ -528,7 +564,16 @@ export function backupSandboxState(sandboxName: string, options: BackupOptions = } const timestamp = new Date().toISOString().replace(/[:.]/g, "-"); const backupPath = path.join(REBUILD_BACKUPS_DIR, sandboxName, timestamp); + + // SECURITY: Verify backup destination ancestors are not symlinks. + // Without this check, an attacker who plants ~/.nemoclaw/rebuild-backups + // as a symlink could redirect snapshot content to an arbitrary directory. + rejectSymlinksOnPath(backupPath); + mkdirSync(backupPath, { recursive: true, mode: 0o700 }); + // Re-check after creation to narrow the TOCTOU race window — + // a symlink swapped in between the first check and mkdirSync is caught here. + rejectSymlinksOnPath(backupPath); // Capture applied policy presets from the registry so they can be // re-applied after rebuild. Presets live in the gateway policy engine,