fix(security): harden file installs and race-path tests

This commit is contained in:
Peter Steinberger 2026-03-02 19:29:17 +00:00
parent e1bc5cad25
commit dbbd41a2ed
5 changed files with 199 additions and 137 deletions

View File

@ -148,40 +148,37 @@ describe("archive utils", () => {
}); });
}); });
it.runIf(process.platform !== "win32")( it("does not clobber out-of-destination file when parent dir is symlink-rebound during zip extract", async () => {
"does not clobber out-of-destination file when parent dir is symlink-rebound during zip extract", await withArchiveCase("zip", async ({ workDir, archivePath, extractDir }) => {
async () => { const outsideDir = path.join(workDir, "outside");
await withArchiveCase("zip", async ({ workDir, archivePath, extractDir }) => { await fs.mkdir(outsideDir, { recursive: true });
const outsideDir = path.join(workDir, "outside"); const slotDir = path.join(extractDir, "slot");
await fs.mkdir(outsideDir, { recursive: true }); await fs.mkdir(slotDir, { recursive: true });
const slotDir = path.join(extractDir, "slot");
await fs.mkdir(slotDir, { recursive: true });
const outsideTarget = path.join(outsideDir, "target.txt"); const outsideTarget = path.join(outsideDir, "target.txt");
await fs.writeFile(outsideTarget, "SAFE"); await fs.writeFile(outsideTarget, "SAFE");
const zip = new JSZip(); const zip = new JSZip();
zip.file("slot/target.txt", "owned"); zip.file("slot/target.txt", "owned");
await fs.writeFile(archivePath, await zip.generateAsync({ type: "nodebuffer" })); await fs.writeFile(archivePath, await zip.generateAsync({ type: "nodebuffer" }));
await withRealpathSymlinkRebindRace({ await withRealpathSymlinkRebindRace({
shouldFlip: (realpathInput) => realpathInput === slotDir, shouldFlip: (realpathInput) => realpathInput === slotDir,
symlinkPath: slotDir, symlinkPath: slotDir,
symlinkTarget: outsideDir, symlinkTarget: outsideDir,
timing: "after-realpath", timing: "after-realpath",
run: async () => { run: async () => {
await expect( await expect(
extractArchive({ archivePath, destDir: extractDir, timeoutMs: 5_000 }), extractArchive({ archivePath, destDir: extractDir, timeoutMs: 5_000 }),
).rejects.toMatchObject({ ).rejects.toMatchObject({
code: "destination-symlink-traversal", code: "destination-symlink-traversal",
} satisfies Partial<ArchiveSecurityError>); } satisfies Partial<ArchiveSecurityError>);
}, },
});
await expect(fs.readFile(outsideTarget, "utf8")).resolves.toBe("SAFE");
}); });
},
); await expect(fs.readFile(outsideTarget, "utf8")).resolves.toBe("SAFE");
});
});
it("rejects tar path traversal (zip slip)", async () => { it("rejects tar path traversal (zip slip)", async () => {
await withArchiveCase("tar", async ({ workDir, archivePath, extractDir }) => { await withArchiveCase("tar", async ({ workDir, archivePath, extractDir }) => {

View File

@ -1,7 +1,10 @@
import fs from "node:fs/promises"; import fs from "node:fs/promises";
import path from "node:path"; import path from "node:path";
import { afterEach, describe, expect, it, vi } from "vitest"; import { afterEach, describe, expect, it, vi } from "vitest";
import { withRealpathSymlinkRebindRace } from "../test-utils/symlink-rebind-race.js"; import {
createRebindableDirectoryAlias,
withRealpathSymlinkRebindRace,
} from "../test-utils/symlink-rebind-race.js";
import { createTrackedTempDirs } from "../test-utils/tracked-temp-dirs.js"; import { createTrackedTempDirs } from "../test-utils/tracked-temp-dirs.js";
import { import {
copyFileWithinRoot, copyFileWithinRoot,
@ -269,100 +272,27 @@ describe("fs-safe", () => {
} }
}); });
it.runIf(process.platform !== "win32")( it("does not truncate out-of-root file when symlink retarget races write open", async () => {
"does not truncate out-of-root file when symlink retarget races write open", const root = await tempDirs.make("openclaw-fs-safe-root-");
async () => { const inside = path.join(root, "inside");
const root = await tempDirs.make("openclaw-fs-safe-root-"); const outside = await tempDirs.make("openclaw-fs-safe-outside-");
const inside = path.join(root, "inside"); await fs.mkdir(inside, { recursive: true });
const outside = await tempDirs.make("openclaw-fs-safe-outside-"); const insideTarget = path.join(inside, "target.txt");
await fs.mkdir(inside, { recursive: true }); const outsideTarget = path.join(outside, "target.txt");
const insideTarget = path.join(inside, "target.txt"); await fs.writeFile(insideTarget, "inside");
const outsideTarget = path.join(outside, "target.txt"); await fs.writeFile(outsideTarget, "X".repeat(4096));
await fs.writeFile(insideTarget, "inside"); const slot = path.join(root, "slot");
await fs.writeFile(outsideTarget, "X".repeat(4096)); await createRebindableDirectoryAlias({
const slot = path.join(root, "slot"); aliasPath: slot,
await fs.symlink(inside, slot); targetPath: inside,
});
await withRealpathSymlinkRebindRace({ await withRealpathSymlinkRebindRace({
shouldFlip: (realpathInput) => realpathInput.endsWith(path.join("slot", "target.txt")), shouldFlip: (realpathInput) => realpathInput.endsWith(path.join("slot", "target.txt")),
symlinkPath: slot, symlinkPath: slot,
symlinkTarget: outside, symlinkTarget: outside,
timing: "before-realpath", timing: "before-realpath",
run: async () => { run: async () => {
await expect(
writeFileWithinRoot({
rootDir: root,
relativePath: path.join("slot", "target.txt"),
data: "new-content",
mkdir: false,
}),
).rejects.toMatchObject({ code: "outside-workspace" });
},
});
await expect(fs.readFile(outsideTarget, "utf8")).resolves.toBe("X".repeat(4096));
},
);
it.runIf(process.platform !== "win32")(
"does not clobber out-of-root file when symlink retarget races write-from-path open",
async () => {
const root = await tempDirs.make("openclaw-fs-safe-root-");
const inside = path.join(root, "inside");
const outside = await tempDirs.make("openclaw-fs-safe-outside-");
const sourceDir = await tempDirs.make("openclaw-fs-safe-source-");
const sourcePath = path.join(sourceDir, "source.txt");
await fs.writeFile(sourcePath, "new-content");
await fs.mkdir(inside, { recursive: true });
const outsideTarget = path.join(outside, "target.txt");
await fs.writeFile(outsideTarget, "X".repeat(4096));
const slot = path.join(root, "slot");
await fs.symlink(inside, slot);
await withRealpathSymlinkRebindRace({
shouldFlip: (realpathInput) => realpathInput.endsWith(path.join("slot", "target.txt")),
symlinkPath: slot,
symlinkTarget: outside,
timing: "before-realpath",
run: async () => {
await expect(
writeFileFromPathWithinRoot({
rootDir: root,
relativePath: path.join("slot", "target.txt"),
sourcePath,
mkdir: false,
}),
).rejects.toMatchObject({ code: "outside-workspace" });
},
});
await expect(fs.readFile(outsideTarget, "utf8")).resolves.toBe("X".repeat(4096));
},
);
it.runIf(process.platform !== "win32")(
"cleans up created out-of-root file when symlink retarget races create path",
async () => {
const root = await tempDirs.make("openclaw-fs-safe-root-");
const inside = path.join(root, "inside");
const outside = await tempDirs.make("openclaw-fs-safe-outside-");
await fs.mkdir(inside, { recursive: true });
const outsideTarget = path.join(outside, "target.txt");
const slot = path.join(root, "slot");
await fs.symlink(inside, slot);
const realOpen = fs.open.bind(fs);
let flipped = false;
const openSpy = vi.spyOn(fs, "open").mockImplementation(async (...args) => {
const [filePath] = args;
if (!flipped && String(filePath).endsWith(path.join("slot", "target.txt"))) {
flipped = true;
await fs.rm(slot, { recursive: true, force: true });
await fs.symlink(outside, slot);
}
return await realOpen(...args);
});
try {
await expect( await expect(
writeFileWithinRoot({ writeFileWithinRoot({
rootDir: root, rootDir: root,
@ -371,13 +301,88 @@ describe("fs-safe", () => {
mkdir: false, mkdir: false,
}), }),
).rejects.toMatchObject({ code: "outside-workspace" }); ).rejects.toMatchObject({ code: "outside-workspace" });
} finally { },
openSpy.mockRestore(); });
}
await expect(fs.stat(outsideTarget)).rejects.toMatchObject({ code: "ENOENT" }); await expect(fs.readFile(outsideTarget, "utf8")).resolves.toBe("X".repeat(4096));
}, });
);
it("does not clobber out-of-root file when symlink retarget races write-from-path open", async () => {
const root = await tempDirs.make("openclaw-fs-safe-root-");
const inside = path.join(root, "inside");
const outside = await tempDirs.make("openclaw-fs-safe-outside-");
const sourceDir = await tempDirs.make("openclaw-fs-safe-source-");
const sourcePath = path.join(sourceDir, "source.txt");
await fs.writeFile(sourcePath, "new-content");
await fs.mkdir(inside, { recursive: true });
const outsideTarget = path.join(outside, "target.txt");
await fs.writeFile(outsideTarget, "X".repeat(4096));
const slot = path.join(root, "slot");
await createRebindableDirectoryAlias({
aliasPath: slot,
targetPath: inside,
});
await withRealpathSymlinkRebindRace({
shouldFlip: (realpathInput) => realpathInput.endsWith(path.join("slot", "target.txt")),
symlinkPath: slot,
symlinkTarget: outside,
timing: "before-realpath",
run: async () => {
await expect(
writeFileFromPathWithinRoot({
rootDir: root,
relativePath: path.join("slot", "target.txt"),
sourcePath,
mkdir: false,
}),
).rejects.toMatchObject({ code: "outside-workspace" });
},
});
await expect(fs.readFile(outsideTarget, "utf8")).resolves.toBe("X".repeat(4096));
});
it("cleans up created out-of-root file when symlink retarget races create path", async () => {
const root = await tempDirs.make("openclaw-fs-safe-root-");
const inside = path.join(root, "inside");
const outside = await tempDirs.make("openclaw-fs-safe-outside-");
await fs.mkdir(inside, { recursive: true });
const outsideTarget = path.join(outside, "target.txt");
const slot = path.join(root, "slot");
await createRebindableDirectoryAlias({
aliasPath: slot,
targetPath: inside,
});
const realOpen = fs.open.bind(fs);
let flipped = false;
const openSpy = vi.spyOn(fs, "open").mockImplementation(async (...args) => {
const [filePath] = args;
if (!flipped && String(filePath).endsWith(path.join("slot", "target.txt"))) {
flipped = true;
await createRebindableDirectoryAlias({
aliasPath: slot,
targetPath: outside,
});
}
return await realOpen(...args);
});
try {
await expect(
writeFileWithinRoot({
rootDir: root,
relativePath: path.join("slot", "target.txt"),
data: "new-content",
mkdir: false,
}),
).rejects.toMatchObject({ code: "outside-workspace" });
} finally {
openSpy.mockRestore();
}
await expect(fs.stat(outsideTarget)).rejects.toMatchObject({ code: "ENOENT" });
});
it("returns not-found for missing files", async () => { it("returns not-found for missing files", async () => {
const dir = await tempDirs.make("openclaw-fs-safe-"); const dir = await tempDirs.make("openclaw-fs-safe-");

View File

@ -20,6 +20,7 @@ vi.mock("../process/exec.js", () => ({
let installPluginFromArchive: typeof import("./install.js").installPluginFromArchive; let installPluginFromArchive: typeof import("./install.js").installPluginFromArchive;
let installPluginFromDir: typeof import("./install.js").installPluginFromDir; let installPluginFromDir: typeof import("./install.js").installPluginFromDir;
let installPluginFromNpmSpec: typeof import("./install.js").installPluginFromNpmSpec; let installPluginFromNpmSpec: typeof import("./install.js").installPluginFromNpmSpec;
let installPluginFromPath: typeof import("./install.js").installPluginFromPath;
let runCommandWithTimeout: typeof import("../process/exec.js").runCommandWithTimeout; let runCommandWithTimeout: typeof import("../process/exec.js").runCommandWithTimeout;
let suiteTempRoot = ""; let suiteTempRoot = "";
let tempDirCounter = 0; let tempDirCounter = 0;
@ -308,8 +309,12 @@ afterAll(() => {
}); });
beforeAll(async () => { beforeAll(async () => {
({ installPluginFromArchive, installPluginFromDir, installPluginFromNpmSpec } = ({
await import("./install.js")); installPluginFromArchive,
installPluginFromDir,
installPluginFromNpmSpec,
installPluginFromPath,
} = await import("./install.js"));
({ runCommandWithTimeout } = await import("../process/exec.js")); ({ runCommandWithTimeout } = await import("../process/exec.js"));
}); });
@ -598,6 +603,37 @@ describe("installPluginFromDir", () => {
}); });
}); });
describe("installPluginFromPath", () => {
it("blocks hardlink alias overwrites when installing a plain file plugin", async () => {
const baseDir = makeTempDir();
const extensionsDir = path.join(baseDir, "extensions");
const outsideDir = path.join(baseDir, "outside");
fs.mkdirSync(extensionsDir, { recursive: true });
fs.mkdirSync(outsideDir, { recursive: true });
const sourcePath = path.join(baseDir, "payload.js");
fs.writeFileSync(sourcePath, "console.log('SAFE');\n", "utf-8");
const victimPath = path.join(outsideDir, "victim.js");
fs.writeFileSync(victimPath, "ORIGINAL", "utf-8");
const targetPath = path.join(extensionsDir, "payload.js");
fs.linkSync(victimPath, targetPath);
const result = await installPluginFromPath({
path: sourcePath,
extensionsDir,
mode: "update",
});
expect(result.ok).toBe(false);
if (result.ok) {
return;
}
expect(result.error.toLowerCase()).toMatch(/hardlink|path alias escape/);
expect(fs.readFileSync(victimPath, "utf-8")).toBe("ORIGINAL");
});
});
describe("installPluginFromNpmSpec", () => { describe("installPluginFromNpmSpec", () => {
it("uses --ignore-scripts for npm pack and cleans up temp dir", async () => { it("uses --ignore-scripts for npm pack and cleans up temp dir", async () => {
const stateDir = makeTempDir(); const stateDir = makeTempDir();

View File

@ -2,6 +2,7 @@ import fs from "node:fs/promises";
import path from "node:path"; import path from "node:path";
import { MANIFEST_KEY } from "../compat/legacy-names.js"; import { MANIFEST_KEY } from "../compat/legacy-names.js";
import { fileExists, readJsonFile, resolveArchiveKind } from "../infra/archive.js"; import { fileExists, readJsonFile, resolveArchiveKind } from "../infra/archive.js";
import { writeFileFromPathWithinRoot } from "../infra/fs-safe.js";
import { resolveExistingInstallPath, withExtractedArchiveRoot } from "../infra/install-flow.js"; import { resolveExistingInstallPath, withExtractedArchiveRoot } from "../infra/install-flow.js";
import { import {
resolveInstallModeOptions, resolveInstallModeOptions,
@ -401,7 +402,15 @@ export async function installPluginFromFile(params: {
} }
logger.info?.(`Installing to ${targetFile}`); logger.info?.(`Installing to ${targetFile}`);
await fs.copyFile(filePath, targetFile); try {
await writeFileFromPathWithinRoot({
rootDir: extensionsDir,
relativePath: path.basename(targetFile),
sourcePath: filePath,
});
} catch (err) {
return { ok: false, error: String(err) };
}
return buildFileInstallResult(pluginId, targetFile); return buildFileInstallResult(pluginId, targetFile);
} }

View File

@ -1,6 +1,17 @@
import fs from "node:fs/promises"; import fs from "node:fs/promises";
import path from "node:path";
import { vi } from "vitest"; import { vi } from "vitest";
export async function createRebindableDirectoryAlias(params: {
aliasPath: string;
targetPath: string;
}): Promise<void> {
const aliasPath = path.resolve(params.aliasPath);
const targetPath = path.resolve(params.targetPath);
await fs.rm(aliasPath, { recursive: true, force: true });
await fs.symlink(targetPath, aliasPath, process.platform === "win32" ? "junction" : undefined);
}
export async function withRealpathSymlinkRebindRace<T>(params: { export async function withRealpathSymlinkRebindRace<T>(params: {
shouldFlip: (realpathInput: string) => boolean; shouldFlip: (realpathInput: string) => boolean;
symlinkPath: string; symlinkPath: string;
@ -17,13 +28,17 @@ export async function withRealpathSymlinkRebindRace<T>(params: {
if (!flipped && params.shouldFlip(filePath)) { if (!flipped && params.shouldFlip(filePath)) {
flipped = true; flipped = true;
if (params.timing !== "after-realpath") { if (params.timing !== "after-realpath") {
await fs.rm(params.symlinkPath, { recursive: true, force: true }); await createRebindableDirectoryAlias({
await fs.symlink(params.symlinkTarget, params.symlinkPath); aliasPath: params.symlinkPath,
targetPath: params.symlinkTarget,
});
return await realRealpath(...args); return await realRealpath(...args);
} }
const resolved = await realRealpath(...args); const resolved = await realRealpath(...args);
await fs.rm(params.symlinkPath, { recursive: true, force: true }); await createRebindableDirectoryAlias({
await fs.symlink(params.symlinkTarget, params.symlinkPath); aliasPath: params.symlinkPath,
targetPath: params.symlinkTarget,
});
return resolved; return resolved;
} }
return await realRealpath(...args); return await realRealpath(...args);