diff --git a/src/lib/state/sandbox.ts b/src/lib/state/sandbox.ts index 1e28939284c..d95ff1cb49b 100644 --- a/src/lib/state/sandbox.ts +++ b/src/lib/state/sandbox.ts @@ -12,13 +12,17 @@ import { createHash } from "node:crypto"; import { chmodSync, + closeSync, existsSync, lstatSync, mkdirSync, + mkdtempSync, + openSync, readdirSync, readFileSync, readlinkSync, rmSync, + statSync, writeFileSync, } from "node:fs"; import os from "node:os"; @@ -56,7 +60,7 @@ import * as registry from "./registry.js"; import { isSshTransportFailure } from "./ssh-transport.js"; import { restoreStateFile } from "./state-file-restore.js"; import { nemoclawStateRoot } from "./state-root.js"; -import { runTarListing } from "./tar-listing.js"; +import { runTarListing, type TarArchiveSource } from "./tar-listing.js"; const HOME_DIR = path.resolve(process.env.HOME || os.homedir()); const REBUILD_BACKUPS_DIR = path.join(nemoclawStateRoot(HOME_DIR, GATEWAY_PORT), "rebuild-backups"); @@ -349,9 +353,12 @@ function rejectSymlinksOnPath(targetPath: string): void { * List tar entries and validate every path is within targetDir. * Rejects absolute paths, path traversal (..), and null bytes. */ -export function validateTarEntries(tarBuffer: Buffer, targetDir: string): TarValidationResult { +export function validateTarEntries( + tarArchive: TarArchiveSource, + targetDir: string, +): TarValidationResult { const entries: string[] = []; - const listingFailure = runTarListing(tarBuffer, ["-tf", "-"], "tar listing", (line) => { + const listingFailure = runTarListing(tarArchive, ["-tf", "-"], "tar listing", (line) => { entries.push(line); }); if (listingFailure) { @@ -472,9 +479,9 @@ function auditExtractedSymlinks(dirPath: string, allowedRoots: string[]): string * legitimate reason to contain them, and they can be used to reference * files outside the extraction root. */ -export function rejectHardLinks(tarBuffer: Buffer): string[] { +export function rejectHardLinks(tarArchive: TarArchiveSource): string[] { const violations: string[] = []; - const listingFailure = runTarListing(tarBuffer, ["-tvf", "-"], "tar verbose listing", (line) => { + const listingFailure = runTarListing(tarArchive, ["-tvf", "-"], "tar verbose listing", (line) => { // Both GNU tar and bsdtar prefix hard-link entries with 'h' in verbose mode // and include " link to " in the line. if (line.startsWith("h") || / link to /.test(line)) { @@ -490,9 +497,9 @@ export function rejectHardLinks(tarBuffer: Buffer): string[] { * SECURITY: Validate tar contents, extract with safety flags, then * audit for symlink escapes. Nukes the extraction on any violation. */ -export function safeTarExtract(tarBuffer: Buffer, targetDir: string): SafeExtractResult { +export function safeTarExtract(tarArchive: TarArchiveSource, targetDir: string): SafeExtractResult { // Phase 1a: Validate entry paths before extraction - const validation = validateTarEntries(tarBuffer, targetDir); + const validation = validateTarEntries(tarArchive, targetDir); if (!validation.safe) { return { success: false, @@ -501,7 +508,7 @@ export function safeTarExtract(tarBuffer: Buffer, targetDir: string): SafeExtrac } // Phase 1b: Reject hard links (not detectable via tar -tf, require verbose listing) - const hardLinkViolations = rejectHardLinks(tarBuffer); + const hardLinkViolations = rejectHardLinks(tarArchive); if (hardLinkViolations.length > 0) { return { success: false, @@ -510,11 +517,25 @@ export function safeTarExtract(tarBuffer: Buffer, targetDir: string): SafeExtrac } // Phase 2: Extract with --no-same-owner to prevent ownership manipulation - const extractResult = spawnSync("tar", ["-xf", "-", "--no-same-owner", "-C", targetDir], { - input: tarBuffer, - stdio: ["pipe", "pipe", "pipe"], - timeout: 60000, - }); + let archiveFd: number | null = null; + let extractResult: ReturnType; + try { + extractResult = Buffer.isBuffer(tarArchive) + ? spawnSync("tar", ["-xf", "-", "--no-same-owner", "-C", targetDir], { + input: tarArchive, + stdio: ["pipe", "pipe", "pipe"], + timeout: 60000, + }) + : (() => { + archiveFd = openSync(tarArchive.filePath, "r"); + return spawnSync("tar", ["-xf", "-", "--no-same-owner", "-C", targetDir], { + stdio: [archiveFd, "pipe", "pipe"], + timeout: 60000, + }); + })(); + } finally { + if (archiveFd !== null) closeSync(archiveFd); + } if (extractResult.status !== 0) { return { @@ -1146,13 +1167,42 @@ export function backupSandboxState(sandboxName: string, options: BackupOptions = // could create symlinks to exfiltrate config contents via backup. const tarCmd = `tar -cf - -C ${shellQuote(dir)} -- ${existingDirs.map(shellQuote).join(" ")}`; _log(`Downloading via SSH+tar: ${tarCmd}`); - const result = spawnSync("ssh", [...sshArgs(configFile, sandboxName), tarCmd], { - stdio: ["ignore", "pipe", "pipe"], - timeout: 120000, - maxBuffer: 256 * 1024 * 1024, - }); + let downloadedTarDir: string | undefined; + let downloadedTarPath: string; + let downloadedTarFd: number; + try { + downloadedTarDir = mkdtempSync(path.join(os.tmpdir(), "nemoclaw-state-download-")); + downloadedTarPath = path.join(downloadedTarDir, "archive.tar"); + downloadedTarFd = openSync(downloadedTarPath, "wx", 0o600); + } catch (error) { + if (downloadedTarDir) { + rmSync(downloadedTarDir, { recursive: true, force: true }); + } + const detail = error instanceof Error ? error.message : String(error); + _log(`FAILED: Could not create local backup archive staging file — ${detail}`); + return { + success: false, + manifest, + backedUpDirs, + failedDirs: [...existingDirs], + backedUpFiles, + failedFiles: stateFiles.map((f) => f.path), + error: `Failed to create backup archive file: ${detail}`, + }; + } + let result: ReturnType; + try { + result = spawnSync("ssh", [...sshArgs(configFile, sandboxName), tarCmd], { + stdio: ["ignore", downloadedTarFd, "pipe"], + timeout: 120000, + maxBuffer: 256 * 1024 * 1024, + }); + } finally { + closeSync(downloadedTarFd); + } + const downloadedBytes = statSync(downloadedTarPath).size; _log( - `SSH+tar download: exit=${result.status}, stdout=${result.stdout ? result.stdout.length + " bytes" : "null"}, stderr=${(result.stderr?.toString() || "").substring(0, 200)}`, + `SSH+tar download: exit=${result.status}, stdout=${downloadedBytes} bytes, stderr=${(result.stderr?.toString() || "").substring(0, 200)}`, ); if (isSshTransportFailure(result)) unreachable = true; @@ -1161,20 +1211,27 @@ export function backupSandboxState(sandboxName: string, options: BackupOptions = // Accept exit 0, 1, or 2 when stdout has data — extract what tar produced // and determine per-dir success from tar's reported read errors. const tarExitedWithData = - result.stdout && - result.stdout.length > 0 && + downloadedBytes > 0 && (result.status === 0 || result.status === 1 || result.status === 2); - if (result.status !== 0 && result.stdout && result.stdout.length > 0) { + if (result.status !== 0 && downloadedBytes > 0) { _log( - `tar exited ${result.status} but produced ${result.stdout.length} bytes — attempting partial extraction`, + `tar exited ${result.status} but produced ${downloadedBytes} bytes — attempting partial extraction`, ); } + let extractResult: SafeExtractResult | null = null; + try { + if (tarExitedWithData) { + // SECURITY: Validate tar entries, extract safely, audit symlinks. + extractResult = safeTarExtract({ filePath: downloadedTarPath }, backupPath); + } + } finally { + rmSync(downloadedTarDir, { recursive: true, force: true }); + } + if (tarExitedWithData) { - // SECURITY: Validate tar entries, extract safely, audit symlinks - const extractResult = safeTarExtract(result.stdout, backupPath); - if (extractResult.success) { + if (extractResult?.success) { const extractedDirs = new Set(existingBackupDirs(backupPath, existingDirs)); if (result.status === 0) { for (const d of existingDirs) { @@ -1213,7 +1270,7 @@ export function backupSandboxState(sandboxName: string, options: BackupOptions = } } } - } else { + } else if (extractResult) { _log(`SECURITY: tar extraction blocked: ${extractResult.error}`); failedDirs.push(...existingDirs); } diff --git a/src/lib/state/tar-listing.ts b/src/lib/state/tar-listing.ts index f25f5450bbb..a6d21882775 100644 --- a/src/lib/state/tar-listing.ts +++ b/src/lib/state/tar-listing.ts @@ -52,8 +52,10 @@ function tarExitStatus(result: ReturnType): number { return result.status ?? (result.error || result.signal ? 1 : 0); } +export type TarArchiveSource = Buffer | { filePath: string }; + export function runTarListing( - tarBuffer: Buffer, + tarArchive: TarArchiveSource, args: string[], failureLabel: string, onLine: (line: string) => void, @@ -61,17 +63,32 @@ export function runTarListing( const tempDir = mkdtempSync(path.join(os.tmpdir(), "nemoclaw-tar-listing-")); const listingPath = path.join(tempDir, "listing.txt"); let listingFd: number | null = null; + let archiveFd: number | null = null; try { listingFd = openSync(listingPath, "w"); - const result = spawnSync("tar", args, { - input: tarBuffer, - encoding: "utf-8", - stdio: ["pipe", listingFd, "pipe"], - timeout: 60000, - maxBuffer: TAR_LISTING_STDERR_MAX_BUFFER_BYTES, - }); + const result = Buffer.isBuffer(tarArchive) + ? spawnSync("tar", args, { + input: tarArchive, + encoding: "utf-8", + stdio: ["pipe", listingFd, "pipe"], + timeout: 60000, + maxBuffer: TAR_LISTING_STDERR_MAX_BUFFER_BYTES, + }) + : (() => { + archiveFd = openSync(tarArchive.filePath, "r"); + return spawnSync("tar", args, { + encoding: "utf-8", + stdio: [archiveFd, listingFd, "pipe"], + timeout: 60000, + maxBuffer: TAR_LISTING_STDERR_MAX_BUFFER_BYTES, + }); + })(); closeSync(listingFd); listingFd = null; + if (archiveFd !== null) { + closeSync(archiveFd); + archiveFd = null; + } const status = tarExitStatus(result); if (status !== 0) { @@ -88,6 +105,7 @@ export function runTarListing( return `${failureLabel} failed: ${error instanceof Error ? error.message : String(error)}`; } finally { if (listingFd !== null) closeSync(listingFd); + if (archiveFd !== null) closeSync(archiveFd); rmSync(tempDir, { recursive: true, force: true }); } } diff --git a/test/security-sandbox-tar-traversal.test.ts b/test/security-sandbox-tar-traversal.test.ts index 9f9f431932d..f8ba849b36e 100644 --- a/test/security-sandbox-tar-traversal.test.ts +++ b/test/security-sandbox-tar-traversal.test.ts @@ -259,6 +259,40 @@ describe("Fix: validateTarEntries rejects malicious tar entries", () => { }); describe("Fix: safeTarExtract blocks malicious archives and extracts safe ones", () => { + it.each([ + ["path traversal", [{ path: "../escape.txt", content: "attacker-payload" }], "path traversal"], + [ + "a hard link", + [{ path: "inside/link.json", type: "1", linkTarget: "../outside.json" }], + "hard link", + ], + [ + "an escaping symlink", + [{ path: "escape-link", type: "2", linkTarget: "../outside.txt" }], + "symlink", + ], + ])("rejects a file-backed archive containing %s", async (_case, entries, expectedError) => { + const { safeTarExtract } = await loadSandboxState(); + const workDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-file-backed-hostile-")); + try { + const archivePath = path.join(workDir, "archive.tar"); + const targetDir = path.join(workDir, "backup"); + fs.mkdirSync(targetDir); + fs.writeFileSync(archivePath, buildTar(entries), { mode: 0o600 }); + + const result = safeTarExtract({ filePath: archivePath }, targetDir); + + expect(result.success).toBe(false); + expect(result.error).toContain(expectedError); + expect(fs.readdirSync(targetDir)).toEqual([]); + expect(fs.existsSync(path.join(workDir, "escape.txt"))).toBe(false); + expect(fs.existsSync(path.join(workDir, "outside.json"))).toBe(false); + expect(fs.existsSync(path.join(workDir, "outside.txt"))).toBe(false); + } finally { + fs.rmSync(workDir, { recursive: true, force: true }); + } + }); + it("blocks archive with path traversal — no files written", async () => { const { safeTarExtract } = await loadSandboxState(); const workDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-safe-")); diff --git a/test/snapshot.test.ts b/test/snapshot.test.ts index 12fcec71749..1487b3b0150 100644 --- a/test/snapshot.test.ts +++ b/test/snapshot.test.ts @@ -6,6 +6,7 @@ // - listBackups computes virtual v versions by timestamp-ascending position // - findBackup resolves selectors (v, name, exact timestamp) import fs from "node:fs"; +import { syncBuiltinESMExports } from "node:module"; import os from "node:os"; import path from "node:path"; import { pathToFileURL } from "node:url"; @@ -79,6 +80,11 @@ beforeEach(() => { function writeExecutable(filePath: string, source: string): void { fs.writeFileSync(filePath, source, { mode: 0o755 }); } +function restoreEnv(name: string, value: string | undefined): void { + value === undefined + ? Reflect.deleteProperty(process.env, name) + : Reflect.set(process.env, name, value); +} function writeAgentRegistry( sandboxName: string, agent: string | null, @@ -458,11 +464,14 @@ describe("sandbox directory backup semantics", () => { const fixture = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-openclaw-empty-dirs-")); const oldPath = process.env.PATH; const oldOpenshell = process.env.NEMOCLAW_OPENSHELL_BIN; + const oldTmpdir = process.env.TMPDIR; try { const binDir = path.join(fixture, "bin"); const openclawDir = path.join(fixture, "sandbox-root", ".openclaw"); + const stagingRoot = path.join(fixture, "staging"); const existingDirs = ["agents", "extensions", "workspace", "skills", "hooks", "cron"]; fs.mkdirSync(binDir, { recursive: true }); + fs.mkdirSync(stagingRoot); for (const dirName of existingDirs) { fs.mkdirSync(path.join(openclawDir, dirName), { recursive: true }); } @@ -487,6 +496,20 @@ if (cmd.includes("find ")) { process.exit(0); } if (cmd.includes("tar -cf -")) { + const stagingDirs = fs.readdirSync(${JSON.stringify(stagingRoot)}); + const archivePaths = stagingDirs + .map((entry) => require("node:path").join(${JSON.stringify(stagingRoot)}, entry, "archive.tar")) + .filter((candidate) => fs.existsSync(candidate)); + const archivePath = archivePaths.length === 1 ? archivePaths[0] : ""; + if ( + !fs.fstatSync(1).isFile() || + !archivePath || + !fs.existsSync(archivePath) || + fs.statSync(archivePath).ino !== fs.fstatSync(1).ino + ) { + process.stderr.write("backup tar stdout must stream to a file\\n"); + process.exit(64); + } const r = spawnSync("tar", ["-cf", "-", "-C", ${JSON.stringify(openclawDir)}, ...existingDirs], { stdio: ["ignore", "pipe", "pipe"], }); @@ -503,6 +526,7 @@ process.exit(0); openclawImagePluginInstalls: [], }); process.env.NEMOCLAW_OPENSHELL_BIN = openshell; + process.env.TMPDIR = stagingRoot; process.env.PATH = `${binDir}${path.delimiter}${oldPath || ""}`; const backup = sandboxState.backupSandboxState("alpha"); @@ -512,12 +536,60 @@ process.exit(0); expect(backup.manifest?.backedUpDirs).toEqual(existingDirs); expect(backup.manifest?.reconcileOpenClawImagePluginProvenance).toBe(true); expect(backup.manifest?.openclawImagePluginInstalls).toEqual([]); + expect(fs.readdirSync(stagingRoot)).toEqual([]); } finally { - if (oldOpenshell === undefined) { - delete process.env.NEMOCLAW_OPENSHELL_BIN; - } else { - process.env.NEMOCLAW_OPENSHELL_BIN = oldOpenshell; - } + restoreEnv("NEMOCLAW_OPENSHELL_BIN", oldOpenshell); + restoreEnv("TMPDIR", oldTmpdir); + process.env.PATH = oldPath; + fs.rmSync(fixture, { recursive: true, force: true }); + } + }); + + it("returns a structured failure when the archive staging file cannot be created", () => { + const fixture = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-backup-staging-failure-")); + const oldPath = process.env.PATH; + const oldOpenshell = process.env.NEMOCLAW_OPENSHELL_BIN; + const oldTmpdir = process.env.TMPDIR; + const originalOpenSync = fs.openSync; + try { + const binDir = path.join(fixture, "bin"); + const stagingRoot = path.join(fixture, "staging"); + fs.mkdirSync(binDir); + fs.mkdirSync(stagingRoot); + const openshell = writeFakeOpenshell(binDir); + writeExecutable( + path.join(binDir, "ssh"), + `#!/usr/bin/env node +const cmd = process.argv[process.argv.length - 1] || ""; +if (cmd.includes("[ -d ")) process.stdout.write("workspace\\n"); +if (cmd.includes("openclaw.json") && cmd.includes("cat --")) process.exit(2); +process.exit(0); +`, + ); + writeOpenClawRegistry("alpha"); + process.env.NEMOCLAW_OPENSHELL_BIN = openshell; + process.env.TMPDIR = stagingRoot; + process.env.PATH = `${binDir}${path.delimiter}${oldPath || ""}`; + + fs.openSync = ((filePath, flags, mode) => { + if (String(filePath).endsWith(`${path.sep}archive.tar`)) { + const error = new Error("ENOSPC: no space left on device"); + Object.assign(error, { code: "ENOSPC" }); + throw error; + } + return originalOpenSync(filePath, flags, mode); + }) as typeof fs.openSync; + syncBuiltinESMExports(); + const backup = sandboxState.backupSandboxState("alpha"); + expect(backup.success).toBe(false); + expect(backup.failedDirs).toEqual(["workspace"]); + expect(backup.error).toMatch(/Failed to create backup archive file.*ENOSPC/); + expect(fs.readdirSync(stagingRoot)).toEqual([]); + } finally { + fs.openSync = originalOpenSync; + syncBuiltinESMExports(); + restoreEnv("NEMOCLAW_OPENSHELL_BIN", oldOpenshell); + restoreEnv("TMPDIR", oldTmpdir); process.env.PATH = oldPath; fs.rmSync(fixture, { recursive: true, force: true }); }