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
34 changes: 31 additions & 3 deletions src/lib/sandbox-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -279,11 +279,39 @@ function auditExtractedSymlinks(dirPath: string, allowedRoots: string[]): string
const stat = lstatSync(fullPath);
if (stat.isSymbolicLink()) {
const linkTarget = readlinkSync(fullPath);
const resolvedTarget = path.resolve(path.dirname(fullPath), linkTarget);
const inAnyAllowedRoot = allowedRoots.some((root) => isWithinRoot(resolvedTarget, root));

// Resolve relative to the symlink's containing directory (standard).
const resolvedRelative = path.resolve(path.dirname(fullPath), linkTarget);

// For absolute symlinks that point into the canonical sandbox data
// directory (/sandbox/.openclaw-data/** or /sandbox/.hermes-data/**),
// also check whether the target falls within the extraction root when
// the leading /sandbox/ prefix is mapped onto the archive root. This
// mirrors how the symlink resolves once the backup is restored inside
// the sandbox container (where /sandbox/.openclaw-data/* exists).
//
// Only /sandbox/ prefixed targets receive this treatment so that
// symlinks pointing to arbitrary absolute paths (e.g. /etc/passwd)
// are still rejected. Fixes #2317.
const SANDBOX_DATA_PREFIXES = ["/sandbox/.openclaw-data/", "/sandbox/.hermes-data/"];
// Normalize the target first to collapse any .. traversal segments
// (e.g. /sandbox/.openclaw-data/../../etc/passwd → /etc/passwd).
// Only then check the prefix — this prevents a traversal bypass
// where a crafted target starts with an allowed prefix but escapes it.
const normalizedTarget = path.posix.normalize(linkTarget);
const resolvedInArchive =
path.isAbsolute(normalizedTarget) &&
SANDBOX_DATA_PREFIXES.some((p) => normalizedTarget.startsWith(p))
? path.resolve(dirPath, normalizedTarget.replace(/^\//, ""))
: null;

const inAnyAllowedRoot =
allowedRoots.some((root) => isWithinRoot(resolvedRelative, root)) ||
(resolvedInArchive !== null && isWithinRoot(resolvedInArchive, dirPath));
Comment on lines +296 to +310

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Prefix allowlist is effectively bypassed by generic /sandbox root allowance.

The new allowlist check can still be bypassed because inAnyAllowedRoot accepts absolute /sandbox/* via the generic allowedRoots branch. Also, the archive remap strips only /, not /sandbox/, which does not match the intended mapping semantics.

🔧 Proposed fix
@@
-          const SANDBOX_DATA_PREFIXES = ["/sandbox/.openclaw-data/", "/sandbox/.hermes-data/"];
+          const SANDBOX_DATA_PREFIXES = ["/sandbox/.openclaw-data/", "/sandbox/.hermes-data/"];
@@
-          const normalizedTarget = path.posix.normalize(linkTarget);
-          const resolvedInArchive =
-            path.isAbsolute(normalizedTarget) &&
-            SANDBOX_DATA_PREFIXES.some((p) => normalizedTarget.startsWith(p))
-              ? path.resolve(dirPath, normalizedTarget.replace(/^\//, ""))
-              : null;
+          const normalizedTarget = path.posix.normalize(linkTarget);
+          const isAbsoluteTarget = path.isAbsolute(normalizedTarget);
+          const resolvedInArchive =
+            isAbsoluteTarget && SANDBOX_DATA_PREFIXES.some((p) => normalizedTarget.startsWith(p))
+              ? path.resolve(dirPath, normalizedTarget.replace(/^\/sandbox\//, ""))
+              : null;
@@
-          const inAnyAllowedRoot =
-            allowedRoots.some((root) => isWithinRoot(resolvedRelative, root)) ||
+          const inAnyAllowedRoot =
+            (!isAbsoluteTarget && allowedRoots.some((root) => isWithinRoot(resolvedRelative, root))) ||
             (resolvedInArchive !== null && isWithinRoot(resolvedInArchive, dirPath));
@@
-  const symlinkViolations = auditExtractedSymlinks(targetDir, [targetDir, "/sandbox"]);
+  const symlinkViolations = auditExtractedSymlinks(targetDir, [targetDir]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/lib/sandbox-state.ts` around lines 296 - 310, The allowlist is bypassed
because the generic allowedRoots check can match absolute /sandbox/* paths
before the archive-specific mapping is applied; update the logic in the
SANDBOX_DATA_PREFIXES block so you first canonicalize and check for the archive
prefix (using normalizedTarget and SANDBOX_DATA_PREFIXES), then map only the
leading "/sandbox/" segment off normalizedTarget (e.g. replace(/^\/*sandbox\//,
"")) to produce resolvedInArchive, and ensure inAnyAllowedRoot uses isWithinRoot
against resolvedInArchive (when non-null) rather than relying on the
allowedRoots branch to accept raw /sandbox paths; also tighten the allowedRoots
check to operate on resolvedRelative only so generic roots cannot accidentally
accept sandbox-absolute targets.


Comment on lines +296 to +311

Copilot AI Apr 26, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolvedInArchive is derived from dirPath, so isWithinRoot(resolvedInArchive, dirPath) will always be true for any non-null value. That means any absolute symlink starting with one of the allowlisted prefixes is unconditionally accepted, even if the target contains traversal segments like /sandbox/.openclaw-data/../../etc/passwd (which actually resolves to /etc/passwd). Consider normalizing the target (e.g. with path.posix.normalize), stripping the intended /sandbox/ (or data-dir) prefix correctly, then resolving and validating containment so .. segments can’t bypass the audit.

Copilot uses AI. Check for mistakes.
if (!inAnyAllowedRoot) {
violations.push(
`symlink escape: ${fullPath} -> ${linkTarget} (resolves to ${resolvedTarget})`,
`symlink escape: ${fullPath} -> ${linkTarget} (resolves to ${resolvedRelative})`,
);
}
} else if (stat.isDirectory()) {
Expand Down
76 changes: 76 additions & 0 deletions test/security-sandbox-tar-traversal.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,82 @@ describe("Fix: safeTarExtract blocks malicious archives and extracts safe ones",
fs.rmSync(workDir, { recursive: true, force: true });
}
});

// Regression #2317: /sandbox/.openclaw-data/* symlinks are created by
// Dockerfile.base for the .openclaw / .openclaw-data split. When a backup
// is extracted on the host, these absolute targets don't exist on the host
// and were falsely rejected as escapes. The fix maps /sandbox/ paths onto
// the extraction root before checking, matching the sandbox-internal view.
it("regression #2317: allows known-safe /sandbox/.openclaw-data symlinks in backup archives", async () => {
const { safeTarExtract } = await loadSandboxState();
const workDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-2317-"));
try {
const targetDir = path.join(workDir, "backup");
fs.mkdirSync(targetDir, { recursive: true });

// Simulate the workspace/media symlink created by Dockerfile.base
const tar = buildTar([
{
path: "workspace/media",
type: "2",
linkTarget: "/sandbox/.openclaw-data/media",
},
]);

const result = safeTarExtract(tar, targetDir);
expect(result.success).toBe(true);
} finally {
fs.rmSync(workDir, { recursive: true, force: true });
}
});

it("regression #2317: still blocks absolute symlinks outside /sandbox/.openclaw-data", async () => {
const { safeTarExtract } = await loadSandboxState();
const workDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-2317-block-"));
try {
const targetDir = path.join(workDir, "backup");
fs.mkdirSync(targetDir, { recursive: true });

// /etc/passwd should still be rejected — not in /sandbox/.openclaw-data/
const tar = buildTar([
{
path: "evil-link",
type: "2",
linkTarget: "/etc/passwd",
},
]);

const result = safeTarExtract(tar, targetDir);
expect(result.success).toBe(false);
expect(result.error).toContain("symlink");
} finally {
fs.rmSync(workDir, { recursive: true, force: true });
}
});
Comment on lines +417 to +439

Copilot AI Apr 26, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These regression tests don’t cover the important edge case where a symlink target is under an allowlisted prefix but normalizes outside it (e.g. /sandbox/.openclaw-data/../../etc/passwd). Given the new prefix-based handling in auditExtractedSymlinks, adding a test for this case would help prevent a traversal bypass from being inadvertently allowed.

Copilot uses AI. Check for mistakes.

it("regression #2317: blocks path traversal within allowed prefix (/sandbox/.openclaw-data/../../etc/passwd)", async () => {
const { safeTarExtract } = await loadSandboxState();
const workDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-2317-traversal-"));
try {
const targetDir = path.join(workDir, "backup");
fs.mkdirSync(targetDir, { recursive: true });

// Crafted target starts with allowed prefix but traverses out of it
const tar = buildTar([
{
path: "evil-traversal",
type: "2",
linkTarget: "/sandbox/.openclaw-data/../../etc/passwd",
},
]);

const result = safeTarExtract(tar, targetDir);
expect(result.success).toBe(false);
expect(result.error).toContain("symlink");
} finally {
fs.rmSync(workDir, { recursive: true, force: true });
}
});
});

describe("Fix: rejectHardLinks blocks hard-link entries at validation time", () => {
Expand Down
Loading