Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion nemoclaw-blueprint/scripts/ws-proxy-fix.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
71 changes: 69 additions & 2 deletions nemoclaw/src/blueprint/snapshot.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, FsEntry>();
Expand All @@ -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", () => ({
Expand All @@ -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);
}),
Expand Down Expand Up @@ -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<string, "file" | "dir">();
const childTypes = new Map<string, "file" | "dir" | "symlink">();
for (const [k, v] of store) {
if (k.startsWith(prefix)) {
const rest = k.slice(prefix.length);
Expand All @@ -92,6 +119,7 @@ vi.mock("node:fs", async (importOriginal) => {
name,
isDirectory: () => type === "dir",
isFile: () => type === "file",
isSymbolicLink: () => type === "symlink",
}));
}
return [...childTypes.keys()].sort();
Expand Down Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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", () => {
Expand Down
74 changes: 66 additions & 8 deletions nemoclaw/src/blueprint/snapshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand All @@ -39,41 +41,91 @@ 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));
}
}
};
walk(dir);
return files;
return { files, symlinks };
}

export function createSnapshot(): string | null {
if (!existsSync(OPENCLAW_DIR)) {
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<string, unknown> = {
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;
Expand Down Expand Up @@ -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);
}
Expand Down
47 changes: 46 additions & 1 deletion src/lib/sandbox-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 });
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// 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,
Expand Down
Loading