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
111 changes: 84 additions & 27 deletions src/lib/state/sandbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)) {
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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<typeof spawnSync>;
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 {
Expand Down Expand Up @@ -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<typeof spawnSync>;
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;

Expand All @@ -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) {
Expand Down Expand Up @@ -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);
}
Expand Down
34 changes: 26 additions & 8 deletions src/lib/state/tar-listing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,26 +52,43 @@ function tarExitStatus(result: ReturnType<typeof spawnSync>): 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,
): string | null {
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) {
Expand All @@ -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 });
}
}
34 changes: 34 additions & 0 deletions test/security-sandbox-tar-traversal.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment thread
apurvvkumaria marked this conversation as resolved.
} 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-"));
Expand Down
Loading
Loading