Skip to content

Commit 0dbb92d

Browse files
steipetesteipete
committed
fix(security): harden tar archive extraction parity
1 parent 17ede52 commit 0dbb92d

File tree

4 files changed

+277
-80
lines changed

4 files changed

+277
-80
lines changed

‎src/agents/skills-install-download.ts‎

Lines changed: 132 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,14 @@
1+
import { createHash } from "node:crypto";
12
import fs from "node:fs";
23
import path from "node:path";
34
import { Readable } from "node:stream";
45
import { pipeline } from "node:stream/promises";
56
import type { ReadableStream as NodeReadableStream } from "node:stream/web";
7+
import { isWindowsDrivePath } from "../infra/archive-path.js";
68
import {
7-
isWindowsDrivePath,
8-
resolveArchiveOutputPath,
9-
stripArchivePath,
10-
validateArchiveEntryPath,
11-
} from "../infra/archive-path.js";
12-
import { extractArchive as extractArchiveSafe } from "../infra/archive.js";
9+
createTarEntrySafetyChecker,
10+
extractArchive as extractArchiveSafe,
11+
} from "../infra/archive.js";
1312
import { fetchWithSsrFGuard } from "../infra/net/fetch-guard.js";
1413
import { isWithinDir } from "../infra/path-safety.js";
1514
import { runCommandWithTimeout } from "../process/exec.js";
@@ -63,6 +62,101 @@ function resolveArchiveType(spec: SkillInstallSpec, filename: string): string |
6362
return undefined;
6463
}
6564

65+
const TAR_VERBOSE_MONTHS = new Set([
66+
"Jan",
67+
"Feb",
68+
"Mar",
69+
"Apr",
70+
"May",
71+
"Jun",
72+
"Jul",
73+
"Aug",
74+
"Sep",
75+
"Oct",
76+
"Nov",
77+
"Dec",
78+
]);
79+
const ISO_DATE_PATTERN = /^\d{4}-\d{2}-\d{2}$/;
80+
81+
function mapTarVerboseTypeChar(typeChar: string): string {
82+
switch (typeChar) {
83+
case "l":
84+
return "SymbolicLink";
85+
case "h":
86+
return "Link";
87+
case "b":
88+
return "BlockDevice";
89+
case "c":
90+
return "CharacterDevice";
91+
case "p":
92+
return "FIFO";
93+
case "s":
94+
return "Socket";
95+
case "d":
96+
return "Directory";
97+
default:
98+
return "File";
99+
}
100+
}
101+
102+
function parseTarVerboseSize(line: string): number {
103+
const tokens = line.trim().split(/\s+/).filter(Boolean);
104+
if (tokens.length < 6) {
105+
throw new Error(`unable to parse tar verbose metadata: ${line}`);
106+
}
107+
108+
let dateIndex = tokens.findIndex((token) => TAR_VERBOSE_MONTHS.has(token));
109+
if (dateIndex > 0) {
110+
const size = Number.parseInt(tokens[dateIndex - 1] ?? "", 10);
111+
if (!Number.isFinite(size) || size < 0) {
112+
throw new Error(`unable to parse tar entry size: ${line}`);
113+
}
114+
return size;
115+
}
116+
117+
dateIndex = tokens.findIndex((token) => ISO_DATE_PATTERN.test(token));
118+
if (dateIndex > 0) {
119+
const size = Number.parseInt(tokens[dateIndex - 1] ?? "", 10);
120+
if (!Number.isFinite(size) || size < 0) {
121+
throw new Error(`unable to parse tar entry size: ${line}`);
122+
}
123+
return size;
124+
}
125+
126+
throw new Error(`unable to parse tar verbose metadata: ${line}`);
127+
}
128+
129+
function parseTarVerboseMetadata(stdout: string): Array<{ type: string; size: number }> {
130+
const lines = stdout
131+
.split("\n")
132+
.map((line) => line.trim())
133+
.filter(Boolean);
134+
return lines.map((line) => {
135+
const typeChar = line[0] ?? "";
136+
if (!typeChar) {
137+
throw new Error("unable to parse tar entry type");
138+
}
139+
return {
140+
type: mapTarVerboseTypeChar(typeChar),
141+
size: parseTarVerboseSize(line),
142+
};
143+
});
144+
}
145+
146+
async function hashFileSha256(filePath: string): Promise<string> {
147+
const hash = createHash("sha256");
148+
const stream = fs.createReadStream(filePath);
149+
return await new Promise<string>((resolve, reject) => {
150+
stream.on("data", (chunk) => {
151+
hash.update(chunk as Buffer);
152+
});
153+
stream.on("error", reject);
154+
stream.on("end", () => {
155+
resolve(hash.digest("hex"));
156+
});
157+
});
158+
}
159+
66160
async function downloadFile(
67161
url: string,
68162
destPath: string,
@@ -132,6 +226,8 @@ async function extractArchive(params: {
132226
return { stdout: "", stderr: "tar not found on PATH", code: null };
133227
}
134228

229+
const preflightHash = await hashFileSha256(archivePath);
230+
135231
// Preflight list to prevent zip-slip style traversal before extraction.
136232
const listResult = await runCommandWithTimeout(["tar", "tf", archivePath], { timeoutMs });
137233
if (listResult.code !== 0) {
@@ -154,34 +250,43 @@ async function extractArchive(params: {
154250
code: verboseResult.code,
155251
};
156252
}
157-
for (const line of verboseResult.stdout.split("\n")) {
158-
const trimmed = line.trim();
159-
if (!trimmed) {
160-
continue;
161-
}
162-
const typeChar = trimmed[0];
163-
if (typeChar === "l" || typeChar === "h" || trimmed.includes(" -> ")) {
253+
const metadata = parseTarVerboseMetadata(verboseResult.stdout);
254+
if (metadata.length !== entries.length) {
255+
return {
256+
stdout: verboseResult.stdout,
257+
stderr: `tar verbose/list entry count mismatch (${metadata.length} vs ${entries.length})`,
258+
code: 1,
259+
};
260+
}
261+
const checkTarEntrySafety = createTarEntrySafetyChecker({
262+
rootDir: targetDir,
263+
stripComponents: strip,
264+
escapeLabel: "targetDir",
265+
});
266+
for (let i = 0; i < entries.length; i += 1) {
267+
const entryPath = entries[i];
268+
const entryMeta = metadata[i];
269+
if (!entryPath || !entryMeta) {
164270
return {
165271
stdout: verboseResult.stdout,
166-
stderr: "tar archive contains link entries; refusing to extract for safety",
272+
stderr: "tar metadata parse failure",
167273
code: 1,
168274
};
169275
}
276+
checkTarEntrySafety({
277+
path: entryPath,
278+
type: entryMeta.type,
279+
size: entryMeta.size,
280+
});
170281
}
171282

172-
for (const entry of entries) {
173-
validateArchiveEntryPath(entry, { escapeLabel: "targetDir" });
174-
const relPath = stripArchivePath(entry, strip);
175-
if (!relPath) {
176-
continue;
177-
}
178-
validateArchiveEntryPath(relPath, { escapeLabel: "targetDir" });
179-
resolveArchiveOutputPath({
180-
rootDir: targetDir,
181-
relPath,
182-
originalPath: entry,
183-
escapeLabel: "targetDir",
184-
});
283+
const postPreflightHash = await hashFileSha256(archivePath);
284+
if (postPreflightHash !== preflightHash) {
285+
return {
286+
stdout: "",
287+
stderr: "tar archive changed during safety preflight; refusing to extract",
288+
code: 1,
289+
};
185290
}
186291

187292
const argv = ["tar", "xf", archivePath, "-C", targetDir];

‎src/agents/skills-install.download.test.ts‎

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -260,13 +260,35 @@ describe("installDownloadSpec extraction safety (tar.bz2)", () => {
260260
label: "rejects archives containing symlinks",
261261
name: "tbz2-symlink",
262262
url: "https://example.invalid/evil.tbz2",
263-
listOutput: "link\nlink/pwned.txt\n",
263+
listOutput: "link\n",
264264
verboseListOutput: "lrwxr-xr-x 0 0 0 0 Jan 1 00:00 link -> ../outside\n",
265265
extract: "reject" as const,
266266
expectedOk: false,
267267
expectedExtract: false,
268268
expectedStderrSubstring: "link",
269269
},
270+
{
271+
label: "rejects archives containing FIFO entries",
272+
name: "tbz2-fifo",
273+
url: "https://example.invalid/evil.tbz2",
274+
listOutput: "evil-fifo\n",
275+
verboseListOutput: "prw-r--r-- 0 0 0 0 Jan 1 00:00 evil-fifo\n",
276+
extract: "reject" as const,
277+
expectedOk: false,
278+
expectedExtract: false,
279+
expectedStderrSubstring: "link",
280+
},
281+
{
282+
label: "rejects oversized extracted entries",
283+
name: "tbz2-oversized",
284+
url: "https://example.invalid/oversized.tbz2",
285+
listOutput: "big.bin\n",
286+
verboseListOutput: "-rw-r--r-- 0 0 0 314572800 Jan 1 00:00 big.bin\n",
287+
extract: "reject" as const,
288+
expectedOk: false,
289+
expectedExtract: false,
290+
expectedStderrSubstring: "archive entry extracted size exceeds limit",
291+
},
270292
{
271293
label: "extracts safe archives with stripComponents",
272294
name: "tbz2-ok",
@@ -322,4 +344,44 @@ describe("installDownloadSpec extraction safety (tar.bz2)", () => {
322344
}
323345
}
324346
});
347+
348+
it("rejects tar.bz2 archives that change after preflight", async () => {
349+
const entry = buildEntry("tbz2-preflight-change");
350+
const targetDir = path.join(resolveSkillToolsRootDir(entry), "target");
351+
const commandCallCount = runCommandWithTimeoutMock.mock.calls.length;
352+
353+
mockArchiveResponse(new Uint8Array([1, 2, 3]));
354+
355+
runCommandWithTimeoutMock.mockImplementation(async (...argv: unknown[]) => {
356+
const cmd = (argv[0] ?? []) as string[];
357+
if (cmd[0] === "tar" && cmd[1] === "tf") {
358+
return runCommandResult({ stdout: "package/hello.txt\n" });
359+
}
360+
if (cmd[0] === "tar" && cmd[1] === "tvf") {
361+
const archivePath = String(cmd[2] ?? "");
362+
if (archivePath) {
363+
await fs.appendFile(archivePath, "mutated");
364+
}
365+
return runCommandResult({ stdout: "-rw-r--r-- 0 0 0 0 Jan 1 00:00 package/hello.txt\n" });
366+
}
367+
if (cmd[0] === "tar" && cmd[1] === "xf") {
368+
throw new Error("should not extract");
369+
}
370+
return runCommandResult();
371+
});
372+
373+
const result = await installDownloadSkill({
374+
name: "tbz2-preflight-change",
375+
url: "https://example.invalid/change.tbz2",
376+
archive: "tar.bz2",
377+
targetDir,
378+
});
379+
380+
expect(result.ok).toBe(false);
381+
expect(result.stderr).toContain("changed during safety preflight");
382+
const extractionAttempted = runCommandWithTimeoutMock.mock.calls
383+
.slice(commandCallCount)
384+
.some((call) => (call[0] as string[])[1] === "xf");
385+
expect(extractionAttempted).toBe(false);
386+
});
325387
});

‎src/infra/archive.test.ts‎

Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -176,23 +176,30 @@ describe("archive utils", () => {
176176
},
177177
);
178178

179-
it("rejects archives that exceed archive size budget", async () => {
180-
await withArchiveCase("zip", async ({ archivePath, extractDir }) => {
181-
const zip = new JSZip();
182-
zip.file("package/file.txt", "ok");
183-
await fs.writeFile(archivePath, await zip.generateAsync({ type: "nodebuffer" }));
184-
const stat = await fs.stat(archivePath);
185-
186-
await expect(
187-
extractArchive({
179+
it.each([{ ext: "zip" as const }, { ext: "tar" as const }])(
180+
"rejects $ext archives that exceed archive size budget",
181+
async ({ ext }) => {
182+
await withArchiveCase(ext, async ({ workDir, archivePath, extractDir }) => {
183+
await writePackageArchive({
184+
ext,
185+
workDir,
188186
archivePath,
189-
destDir: extractDir,
190-
timeoutMs: 5_000,
191-
limits: { maxArchiveBytes: Math.max(1, stat.size - 1) },
192-
}),
193-
).rejects.toThrow("archive size exceeds limit");
194-
});
195-
});
187+
fileName: "file.txt",
188+
content: "ok",
189+
});
190+
const stat = await fs.stat(archivePath);
191+
192+
await expect(
193+
extractArchive({
194+
archivePath,
195+
destDir: extractDir,
196+
timeoutMs: 5_000,
197+
limits: { maxArchiveBytes: Math.max(1, stat.size - 1) },
198+
}),
199+
).rejects.toThrow("archive size exceeds limit");
200+
});
201+
},
202+
);
196203

197204
it("fails resolvePackedRootDir when extract dir has multiple root dirs", async () => {
198205
const workDir = await makeTempDir("packed-root");

0 commit comments

Comments
 (0)