From 95cde2659401d839bce92bcf69a68511968ddd7f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 11:05:29 +0000 Subject: [PATCH] fix(tar-extract): bound pax parsing and enforce strict tar framing (PHARN-18) A 192 KB archive whose pax global header inflated to ~128 MB cost ~13 s CPU and >1 GB RSS to parse (OOM under a 512 MB heap, leaving the temp clone). Framing was lenient: a zero block mid-archive silently dropped the rest, a missing end marker, trailing garbage and a missing ustar magic were accepted, and `a//b` / `a/./b` segments were left to safeJoin. The extractor now refuses a g/x payload over 64 KiB before parsing it, requires only zeros after the end marker, refuses a missing marker and a non-ustar header, and refuses empty/"." segments below the root. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01TvcuVhk8hTeDskp5pAJhnc --- .dev/features/tar-strict-framing/GRILL.md | 29 ++++++ .dev/features/tar-strict-framing/PLAN.md | 56 ++++++++++++ .../features/tar-strict-framing/REGRESSION.md | 30 +++++++ .dev/features/tar-strict-framing/REVIEW.md | 43 +++++++++ .dev/features/tar-strict-framing/SHIP.md | 17 ++++ .dev/features/tar-strict-framing/VERIFY.md | 27 ++++++ .../tar-strict-framing/regression-report.json | 20 +++++ .../tar-strict-framing/verify-report.json | 17 ++++ .pharn/writes-scope.json | 4 +- src/lib/tar-extract.ts | 52 ++++++++++- tests/tar-extract.test.ts | 88 +++++++++++++++++++ 11 files changed, 380 insertions(+), 3 deletions(-) create mode 100644 .dev/features/tar-strict-framing/GRILL.md create mode 100644 .dev/features/tar-strict-framing/PLAN.md create mode 100644 .dev/features/tar-strict-framing/REGRESSION.md create mode 100644 .dev/features/tar-strict-framing/REVIEW.md create mode 100644 .dev/features/tar-strict-framing/SHIP.md create mode 100644 .dev/features/tar-strict-framing/VERIFY.md create mode 100644 .dev/features/tar-strict-framing/regression-report.json create mode 100644 .dev/features/tar-strict-framing/verify-report.json diff --git a/.dev/features/tar-strict-framing/GRILL.md b/.dev/features/tar-strict-framing/GRILL.md new file mode 100644 index 0000000..f90d32c --- /dev/null +++ b/.dev/features/tar-strict-framing/GRILL.md @@ -0,0 +1,29 @@ +# GRILL — tar-strict-framing + +Plan: `.dev/features/tar-strict-framing/PLAN.md` · spec-hash `bca940a5…d729d3c4e` matches live +`ARCHITECTURE.md`. Registered grillers: `{"registered":0,"grillers":[]}` → inline axes only. + +## Findings + +```yaml +- type: FINDING + rule_id: 'P5' + severity: important + file: '.dev/features/tar-strict-framing/PLAN.md:6' + problem: 'The all-zero tail check must be linear and allocation-free (a byte loop or a single indexOf of a non-zero byte), or a 128 MiB zero tail becomes its own cost.' + evidence: '`requires every remaining byte to be zero`' +- type: FINDING + rule_id: 'P5' + severity: minor + file: '.dev/features/tar-strict-framing/PLAN.md:8' + problem: 'Accept both `ustar\0` (POSIX, what git archive writes) and `ustar ` (old GNU) — the check is the 5-byte `ustar` prefix — so a GNU-written fixture or mirror is not refused over the variant.' + evidence: '`refuses a header without the ustar magic`' +- type: FINDING + rule_id: 'P1' + severity: minor + file: '.dev/features/tar-strict-framing/PLAN.md:24' + problem: 'The oversized-pax eval must fail on the base source by BEHAVIOUR it can observe cheaply (accepted vs refused with the new message), not by timing — a wall-clock assertion is flaky in CI.' + evidence: '`an oversized g payload refused fast (bounded time, before parsing)`' +``` + +ADVISORY VERDICT: 3 concerns raised (0 blocking-severity, 3 advisory) — all folded into the build. diff --git a/.dev/features/tar-strict-framing/PLAN.md b/.dev/features/tar-strict-framing/PLAN.md new file mode 100644 index 0000000..3de88f6 --- /dev/null +++ b/.dev/features/tar-strict-framing/PLAN.md @@ -0,0 +1,56 @@ +# PLAN — tar-strict-framing (PHARN-18: bounded pax parsing and strict tar framing) + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: the extractor (1) refuses a pax `g`/`x` header whose payload exceeds a fixed cap (64 KiB) + BEFORE parsing its records; (2) after the first all-zero block requires every remaining byte to be zero + (a zero block mid-archive no longer silently truncates the tree; garbage after the end marker is + refused); (3) refuses an archive that ends without an end-of-archive marker; (4) refuses a header + without the `ustar` magic; (5) refuses empty (`a//b`) and `.` path segments explicitly, instead of + relying on `safeJoin`'s backstop. +- layer(s): the CLI itself (`src/lib/tar-extract.ts`) +- constitution_refs: [P1, P2, P5] + +## Discovery — verified this run (P6) + +Review repro: a 192,449-byte archive whose global header expands to ~128 MB of tiny records costs +12.99 s CPU and 1,115 MiB RSS in `readPaxKeywords`; with `--max-old-space-size=512` the process aborts +(exit 134) and leaves the clone in the temp dir. Framing: `if (isZeroBlock(header)) break;` stops at the +first zero block (entries after it are silently dropped); a missing end marker, trailing garbage and a +missing `ustar` magic are all accepted; `resolveEntryPath` rejects `..`/absolute/root but not `''`/`.` +segments below the root. Every codeload archive (git archive) has `ustar\0` magic, a pax global header +of one `comment=` record (~52 bytes) and zero-padded end blocks, so none of these rules touches it. + +## Files + +- `src/lib/tar-extract.ts` — `MAX_PAX_BYTES` gate; end-marker + all-zero tail; missing-marker refusal; + `ustar` magic check; `''`/`.` segment refusal — layer CLI/lib +- `tests/tar-extract.test.ts` — an oversized `g` payload refused fast (bounded time, before parsing); + zero block mid-archive followed by an entry → refused; trailing garbage → refused; no end marker → + refused; missing magic → refused; `a//b` and `a/./b` → refused; the codeload-shaped fixture still + extracts + +## Contracts satisfied + +- `THREAT-MODEL.md` §2 "post-decompression cap bounds the compression bomb" — now also bounds the cost of + parsing pax records. + +## Evals to write (P1) + +- listed above; each fails (is accepted, or is slow) on the base source. + +## Guarantee audit (P0) + +- "a pax payload parse is O(64 KiB)" → floor: size check before `readPaxKeywords`. +- "no entry after the end marker is ever skipped silently" → floor: all-zero tail check. + +## Trust audit (P2) + +- Only narrows what an untrusted archive may contain. + +## Determinism audit (P5) + +- Pure byte checks. + +## Open questions (HALT) + +- none diff --git a/.dev/features/tar-strict-framing/REGRESSION.md b/.dev/features/tar-strict-framing/REGRESSION.md new file mode 100644 index 0000000..f99546b --- /dev/null +++ b/.dev/features/tar-strict-framing/REGRESSION.md @@ -0,0 +1,30 @@ +# REGRESSION — tar-strict-framing + +The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment. + +## Base and partition + +- **base:** `00105215205cbdb994e8bea6d80eca4891d07865` (`origin/main` at build time; the build is an uncommitted working tree on top of it). +- **inside** (each declared in `PLAN.md` `## Files`): `src/lib/tar-extract.ts`, `tests/tar-extract.test.ts`. +- **scope partition:** `check-regress.mjs scope` exited **0**, `escaped: []`. `.pharn/` (hook scratch) and this + feature's own stage artifacts are not build output. +- **outside gates:** the stdlib `*.test.mjs` / `*.test.cjs` files + whole-repo `validate`; 0 committed eval pairs. +- **style-gate skip:** `inside` touches no shared style config, so `lint` / `format:check` / `lint:md` are absent from both maps. + +## Per-gate exit codes + +| gate | base | head | flipped? | +| ---------- | ---- | ---- | -------- | +| `tests` | 0 | 0 | no | +| `validate` | 0 | 0 | no | + +- `regressions[]`: **empty** +- `pre_existing[]`: **empty** + +## Verdict + +**REGRESSIONS: none — no deterministically-detectable breakage outside the feature.** +(`regression-report.json` `.verdict` = `no-regressions`.) + +Residual (P0/P7): this catches exactly what its suite catches. The vitest suite exercising `src/**` is +owned by `/pharn-dev-build`'s floor and `/pharn-dev-verify`. This certifies the comparison, never the increment. diff --git a/.dev/features/tar-strict-framing/REVIEW.md b/.dev/features/tar-strict-framing/REVIEW.md new file mode 100644 index 0000000..628fd2d --- /dev/null +++ b/.dev/features/tar-strict-framing/REVIEW.md @@ -0,0 +1,43 @@ +# REVIEW — tar-strict-framing + +Floor first: `node .dev/floor/validate.mjs .` → exit 0 (GREEN). Everything below is **advisory**. + +## Floor-gate findings (blocking) + +None. + +- **L-floor (P0):** a `g`/`x` payload over 64 KiB is refused on its size field before `readPaxKeywords` + runs; after the first zero block every byte must be zero (a linear byte loop, no allocation — grill + #1); a walk that runs out without a zero block is refused; the header must start `ustar` (both POSIX + and old-GNU variants — grill #2); `''`/`.` segments below the root are refused. +- **L-eval (P1):** 7 cases were accepted by the base source; the pax case asserts the refusal message, + not a timing (grill #3). The codeload-shaped fixture still extracts. +- **L-trust (P2):** only narrows what the untrusted archive may contain. +- **L-axis (P3):** contained in `tar-extract.ts`. + +## Advisory findings + +```yaml +- type: FINDING + rule_id: 'P4' + severity: minor + file: 'THREAT-MODEL.md' + problem: 'THREAT-MODEL.md §2 (the extractor rejection list) should gain these framing rules and the pax payload cap; it is write-protected, so a maintainer adds them.' + evidence: 'tar archive has a pax header of N bytes, over the 65536-byte limit' +- type: FINDING + rule_id: 'P5' + severity: minor + file: 'src/lib/tar-extract.ts' + problem: 'The decompressed archive itself is still held in memory up to the 128 MiB cap; that bound is unchanged (and documented) — only the pax PARSE cost is new here.' + evidence: 'gunzipSync(archive, { maxOutputLength: limits.maxTotalBytes })' +- type: FINDING + rule_id: 'P4' + severity: minor + file: 'CHANGELOG.md:8' + problem: "No CHANGELOG `[Unreleased]` entry (not in the plan's `## Files`)." + evidence: '## [Unreleased]' +``` + +## Verdict + +**GREEN** — 0 floor-gate findings, 3 advisory findings. No lesson proposed for canon. diff --git a/.dev/features/tar-strict-framing/SHIP.md b/.dev/features/tar-strict-framing/SHIP.md new file mode 100644 index 0000000..d56a47f --- /dev/null +++ b/.dev/features/tar-strict-framing/SHIP.md @@ -0,0 +1,17 @@ +# SHIP — tar-strict-framing + +Stages run, in order: `/pharn-dev-plan` → GATE 1 (human: **Approve as written**) → `/pharn-dev-grill` → +`/pharn-dev-build` → `/pharn-dev-regress` → `/pharn-dev-verify` → `/pharn-dev-review` → GATE 2. + +| stage | structural verdict (verbatim) | +| -------------------- | ------------------------------------------------------ | +| `/pharn-dev-build` | `node .dev/floor/validate.mjs .` exit `0` | +| `/pharn-dev-regress` | `regression-report.json` `.verdict` = `no-regressions` | +| `/pharn-dev-verify` | `verify-report.json` `.verdict` = `PASS` | + +- Review: [`REVIEW.md`](REVIEW.md) · Grill (advisory): [`GRILL.md`](GRILL.md) +- Run ended at **GATE 2**. The human's standing instruction for this batch: open a PR and merge it only if + its CI checks are green. + +chain ran; the named floor verdicts are as shown — this is NOT a judgment that the increment is good or +wise; that is the human's call at the post-review gate. diff --git a/.dev/features/tar-strict-framing/VERIFY.md b/.dev/features/tar-strict-framing/VERIFY.md new file mode 100644 index 0000000..65219b9 --- /dev/null +++ b/.dev/features/tar-strict-framing/VERIFY.md @@ -0,0 +1,27 @@ +# VERIFY — tar-strict-framing + +## FLOOR layer (owns the verdict) + +Gates run over the whole repo with the feature present, as a non-root user on node 22 with the session +proxy variables unset (as root with the proxy set, 5 pre-existing tests in `init.test.ts` / +`update.test.ts` fail for environmental reasons, identically at the baseline). + +| gate | exit | +| -------------- | ---- | +| `format:check` | 0 | +| `lint` | 0 | +| `lint:md` | 0 | +| `test` | 0 | +| `typecheck` | 0 | +| `validate` | 0 | + +No `structural:*` gate — the increment ships no eval-actual pair. + +**VERDICT: PASS** (`.dev/floor/check-verify.mjs`, `failing_gates: []`). + +## ADVISORY layer + +`node .dev/floor/count-verifiers.mjs .` → `{"registered":0,"verifiers":[]}` — floor gates only. + +Residual (P0/P7): "verified" means the named gates passed — not that the feature is correct in any sense +the suite does not encode. diff --git a/.dev/features/tar-strict-framing/regression-report.json b/.dev/features/tar-strict-framing/regression-report.json new file mode 100644 index 0000000..c38f4a7 --- /dev/null +++ b/.dev/features/tar-strict-framing/regression-report.json @@ -0,0 +1,20 @@ +{ + "base": "00105215205cbdb994e8bea6d80eca4891d07865", + "inside": [ + "src/lib/tar-extract.ts", + "tests/tar-extract.test.ts" + ], + "outside_gates": { + "tests": { + "base": 0, + "head": 0 + }, + "validate": { + "base": 0, + "head": 0 + } + }, + "regressions": [], + "pre_existing": [], + "verdict": "no-regressions" +} diff --git a/.dev/features/tar-strict-framing/verify-report.json b/.dev/features/tar-strict-framing/verify-report.json new file mode 100644 index 0000000..64c51d9 --- /dev/null +++ b/.dev/features/tar-strict-framing/verify-report.json @@ -0,0 +1,17 @@ +{ + "feature": "tar-strict-framing", + "gates": { + "format:check": 0, + "lint": 0, + "lint:md": 0, + "test": 0, + "typecheck": 0, + "validate": 0 + }, + "verdict": "PASS", + "failing_gates": [], + "verifiers": { + "registered": 0, + "findings": [] + } +} diff --git a/.pharn/writes-scope.json b/.pharn/writes-scope.json index dba58e5..892bdfe 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/terminal-safe-text/SHIP.md" + ".dev/features/tar-strict-framing/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-09-24T10:59:45.280Z" + "set_at": "2026-09-24T11:05:27.882Z" } diff --git a/src/lib/tar-extract.ts b/src/lib/tar-extract.ts index a1b4421..38f9f28 100644 --- a/src/lib/tar-extract.ts +++ b/src/lib/tar-extract.ts @@ -77,6 +77,7 @@ const LEN_SIZE = 12; const OFF_CHKSUM = 148; const LEN_CHKSUM = 8; const OFF_TYPEFLAG = 156; +const OFF_MAGIC = 257; const OFF_PREFIX = 345; const LEN_PREFIX = 155; @@ -221,6 +222,20 @@ function describeTypeflag(typeflag: string): string { : `'${typeflag}'`; } +/** The largest pax (`g`/`x`) payload whose records are parsed at all. */ +const MAX_PAX_BYTES = 64 * 1024; + +/** Refuse any non-zero byte from `from` to the end (after the end marker). */ +function assertZeroTail(tar: Buffer, from: number): void { + for (let i = from; i < tar.length; i += 1) { + if (tar[i] !== 0) { + throw new TarExtractError( + 'tar archive has data after its end-of-archive marker.', + ); + } + } +} + /** True when the block is 512 NUL bytes — the end-of-archive marker. */ function isZeroBlock(block: Buffer): boolean { for (const byte of block) if (byte !== 0) return false; @@ -292,6 +307,13 @@ function resolveEntryPath( ); } const rest = segments.slice(1); + // Below the root, every segment must name something: `a//b` and `a/./b` are + // refused here rather than left to safeJoin's lexical backstop. + if (rest.some((segment) => segment === '' || segment === '.')) { + throw new TarExtractError( + `tar entry has an empty or "." path segment: ${fullPath}`, + ); + } if (rest.length === 0) { // The root directory entry itself: nothing to write, not a failure. if (isDirectory) return { rel: null, root }; @@ -337,12 +359,25 @@ export function extractTar( while (offset + BLOCK <= tar.length) { const header = tar.subarray(offset, offset + BLOCK); - if (isZeroBlock(header)) break; + // The end-of-archive marker. Everything after it must be zero padding: a + // zero block followed by more entries used to end the walk there and drop + // the rest of the tree in silence, and trailing garbage was accepted. + if (isZeroBlock(header)) { + assertZeroTail(tar, offset + BLOCK); + return; + } if (!checksumOk(header)) { throw new TarExtractError( 'tar header failed its checksum — the archive is corrupt or truncated.', ); } + // Every header git archive writes is ustar (`ustar\0`); old GNU writes + // `ustar ` — the 5-byte prefix admits both and nothing else. + if (header.toString('latin1', OFF_MAGIC, OFF_MAGIC + 5) !== 'ustar') { + throw new TarExtractError( + 'tar header is not a ustar header (missing "ustar" magic).', + ); + } const size = readOctal(header, OFF_SIZE, LEN_SIZE); const dataStart = offset + BLOCK; @@ -355,6 +390,16 @@ export function extractTar( const next = dataStart + Math.ceil(size / BLOCK) * BLOCK; const typeflag = String.fromCharCode(header[OFF_TYPEFLAG]!); + // A pax header's records are parsed (below) only when its payload is small: + // git archive writes one `comment=` record (~52 bytes), while a payload + // inflated to the decompression cap cost seconds of CPU and a gigabyte of + // memory to parse. Refused on the size field alone, before any parse. + if ((typeflag === 'x' || typeflag === 'g') && size > MAX_PAX_BYTES) { + throw new TarExtractError( + `tar archive has a pax header of ${size} bytes, over the ${MAX_PAX_BYTES}-byte limit pharn accepts.`, + ); + } + // REJECT — a per-file pax extended header. The decision is the typeflag // ALONE; the payload is read only afterwards, to name what was seen. (The // ustar size field above is still read first, so an 'x' whose size field is @@ -438,4 +483,9 @@ export function extractTar( offset = next; } + // The walk ran out of bytes without meeting a zero block: the archive was cut + // short (every real tar ends with at least one). + throw new TarExtractError( + 'tar archive has no end-of-archive marker — truncated.', + ); } diff --git a/tests/tar-extract.test.ts b/tests/tar-extract.test.ts index 43b5118..842f292 100644 --- a/tests/tar-extract.test.ts +++ b/tests/tar-extract.test.ts @@ -590,6 +590,94 @@ describe('extractTar', () => { ); }); + // PHARN-18: strict framing and bounded pax parsing. Each case was ACCEPTED + // by the base source (or, for the pax payload, parsed at full size). + const refusal = (archive: Buffer): string => { + try { + extractTar(archive, tmp.path(), LIMITS); + } catch (err) { + expect(err).toBeInstanceOf(TarExtractError); + return (err as Error).message; + } + return 'accepted'; + }; + + it('refuses a pax global header over the payload cap, before parsing it', () => { + // Well-formed records, so the base source parsed every one and accepted. + const record = paxRecord('comment', 'x'.repeat(100)); + const big = record.repeat(Math.ceil((64 * 1024 + 1) / record.length)); + expect( + refusal( + tar([ + { name: 'pax_global_header', type: 'g', data: big }, + { name: 'pharn-oss-abc1234/', type: '5' }, + ]), + ), + ).toMatch(/pax header of \d+ bytes, over the 65536-byte limit/); + }); + + it('refuses entries hidden after a zero block mid-archive', () => { + const archive = Buffer.concat([ + tar([GLOBAL_HEADER, { name: 'pharn-oss-abc1234/', type: '5' }]).subarray( + 0, + -BLOCK, // keep ONE zero block, then keep going + ), + tar([{ name: 'pharn-oss-abc1234/hidden.md', data: 'x' }]), + ]); + expect(refusal(archive)).toMatch(/data after its end-of-archive marker/); + }); + + it('refuses trailing garbage after the end marker', () => { + const archive = Buffer.concat([githubArchive([]), Buffer.from('garbage')]); + expect(refusal(archive)).toMatch(/data after its end-of-archive marker/); + }); + + it('refuses an archive with no end-of-archive marker', () => { + const archive = githubArchive([ + { name: 'pharn-oss-abc1234/a.md', data: 'x' }, + ]).subarray(0, -2 * BLOCK); + expect(refusal(archive)).toMatch(/no end-of-archive marker/); + }); + + it('refuses a header without the ustar magic', () => { + const archive = githubArchive([ + { name: 'pharn-oss-abc1234/a.md', data: 'x' }, + ]); + // Blank the magic of the third header and re-seal its checksum, so the + // magic check — not the checksum — is what refuses it. + const at = 2 * BLOCK; + archive.fill(0, at + 257, at + 263); + archive.write(' ', at + 148, 8, 'latin1'); + let sum = 0; + for (const byte of archive.subarray(at, at + BLOCK)) sum += byte; + archive.write( + sum.toString(8).padStart(6, '0') + '\0 ', + at + 148, + 8, + 'latin1', + ); + expect(refusal(archive)).toMatch(/missing "ustar" magic/); + }); + + it.each([['pharn-oss-abc1234/a//b.md'], ['pharn-oss-abc1234/a/./b.md']])( + 'refuses the empty or "." segment in %s', + (name) => { + expect(refusal(githubArchive([{ name, data: 'x' }]))).toMatch( + /empty or "\." path segment/, + ); + }, + ); + + it('still extracts the codeload shape (one small global header, zero tail)', () => { + expect( + refusal( + githubArchive([ + { name: 'pharn-oss-abc1234/SKILLS_VERSION', data: '1\n' }, + ]), + ), + ).toBe('accepted'); + }); + it('enforces the entry-count cap', () => { const many = Array.from({ length: 5 }, (_, i) => ({ name: `pharn-oss-abc1234/f${i}.txt`,