From a41517f52ac2be9c6b69dcd5283b2e423d83c628 Mon Sep 17 00:00:00 2001 From: Jason Ginchereau Date: Fri, 19 Jun 2026 15:35:16 -1000 Subject: [PATCH] Improve path validations and test coverage --- .../cache/__tests__/pathValidation.test.ts | 12 +- packages/cache/__tests__/pax-reparse.test.ts | 41 +++- .../tarPathValidationAttacks.test.ts | 178 ++++++++++++++---- .../cache/docs/path-validation-test-plan.md | 55 ++++-- .../cache/src/internal/listAndValidate.ts | 152 ++++++--------- packages/cache/src/internal/pathValidation.ts | 34 +++- packages/cache/src/internal/pax-reparse.ts | 34 +++- packages/cache/src/internal/tar.ts | 34 +++- 8 files changed, 358 insertions(+), 182 deletions(-) diff --git a/packages/cache/__tests__/pathValidation.test.ts b/packages/cache/__tests__/pathValidation.test.ts index 43463226..19979864 100644 --- a/packages/cache/__tests__/pathValidation.test.ts +++ b/packages/cache/__tests__/pathValidation.test.ts @@ -979,10 +979,11 @@ describe('validateEntry', () => { }) }) - describe('control characters in filenames (legal but unusual)', () => { - test('embedded newline is treated literally as part of the segment', () => { - // node-tar reports the full filename including the embedded newline. - // It's a single segment under node_modules — must be accepted. + describe('control characters in filenames', () => { + test('embedded newline is rejected as an unsafe control character', () => { + // A newline in a member name would corrupt a newline-delimited tar file + // list and enables log / line injection, so it is rejected even though + // it is a single segment under node_modules. const r = validateEntry( 'node_modules/file\nwith newline', undefined, @@ -990,7 +991,8 @@ describe('validateEntry', () => { allowedRoots, cwd ) - expect(r.ok).toBe(true) + expect(r.ok).toBe(false) + if (!r.ok) expect(r.code).toBe('UNSAFE_CHAR') }) test('embedded tab is accepted', () => { diff --git a/packages/cache/__tests__/pax-reparse.test.ts b/packages/cache/__tests__/pax-reparse.test.ts index 4e052841..2d5263cf 100644 --- a/packages/cache/__tests__/pax-reparse.test.ts +++ b/packages/cache/__tests__/pax-reparse.test.ts @@ -23,17 +23,17 @@ describe('parsePaxLengthCorrect', () => { }) test('newline-in-value: the embedded fake record is swallowed by comment', () => { - // This is the F2 PAX body. A naive split('\n') parser (node-tar) ends up - // with path=safe.txt; the length-correct parser must yield the real + // An embedded-newline PAX body. A naive split('\n') parser (node-tar) ends + // up with path=safe.txt; the length-correct parser must yield the real // (malicious) path and treat the rest as the comment value. const buf = Buffer.from( - '42 path=../../../../../../tmp/zip_slip_F2\n30 comment=x\n17 path=safe.txt\n', + '42 path=../../../../../../tmp/escaped_pax\n30 comment=x\n17 path=safe.txt\n', 'ascii' ) const {records, ok} = parsePaxLengthCorrect(buf) expect(ok).toBe(true) expect(records['path'].toString('utf8')).toBe( - '../../../../../../tmp/zip_slip_F2' + '../../../../../../tmp/escaped_pax' ) expect(records['comment'].toString('utf8')).toBe('x\n17 path=safe.txt') // Crucially NOT safe.txt. @@ -133,16 +133,16 @@ describe('crossCheckMetaBodies', () => { expect(v).toEqual([]) }) - test('F2 path desync: node-tar resolved safe.txt, length-correct disagrees', () => { + test('PAX path desync: node-tar resolved safe.txt, length-correct disagrees', () => { const buf = Buffer.from( - '42 path=../../../../../../tmp/zip_slip_F2\n30 comment=x\n17 path=safe.txt\n', + '42 path=../../../../../../tmp/escaped_pax\n30 comment=x\n17 path=safe.txt\n', 'ascii' ) const v = crossCheckMetaBodies([buf], 'safe.txt', undefined) expect(v.map(x => x.code)).toContain('PAX_DESYNC') }) - test('F2-linkpath desync: node-tar resolved safe/target, length-correct disagrees', () => { + test('PAX linkpath desync: node-tar resolved safe/target, length-correct disagrees', () => { const buf = Buffer.from( '34 linkpath=../../../../../../tmp\n37 comment=x\n24 linkpath=safe/target\n', 'ascii' @@ -162,10 +162,9 @@ describe('crossCheckMetaBodies', () => { expect(v.map(x => x.code)).toContain('PAX_UNKNOWN_KEY') }) - test('known SCHILY/GNU/LIBARCHIVE prefixed keys are accepted', () => { + test('known SCHILY/LIBARCHIVE prefixed keys are accepted', () => { const records = [ 'SCHILY.xattr.user.foo=bar', - 'GNU.sparse.realsize=1024', 'LIBARCHIVE.creationtime=1700000000' ] const body = records @@ -185,6 +184,30 @@ describe('crossCheckMetaBodies', () => { expect(v).toEqual([]) }) + test('GNU.sparse.* keys are rejected as PAX_UNSUPPORTED_KEY', () => { + // node-tar v7 does not process GNU sparse keys, so it surfaces the entry + // under its header path while system tar would reconstruct the file at + // `GNU.sparse.name` (here pointing outside the cache roots). The path / + // linkpath cross-check cannot see this, so the sparse namespace must be + // rejected outright even though it is under the broadly-allowed `GNU.` + // prefix. + const body = Buffer.from( + rec('GNU.sparse.major=1') + + rec('GNU.sparse.minor=0') + + rec('GNU.sparse.name=../../../../tmp/evil') + + rec('GNU.sparse.realsize=1024'), + 'ascii' + ) + const v = crossCheckMetaBodies( + [body], + 'cache/GNUSparseFile.0/decoy', + undefined + ) + expect(v.map(x => x.code)).toContain('PAX_UNSUPPORTED_KEY') + // It must NOT also be misreported as merely an unknown key. + expect(v.map(x => x.code)).not.toContain('PAX_UNKNOWN_KEY') + }) + test('GNU long-name raw body matching entry path: no violation', () => { // A GNU LongName body is a raw NUL-terminated path with no length prefix. const raw = Buffer.concat([ diff --git a/packages/cache/__tests__/tarPathValidationAttacks.test.ts b/packages/cache/__tests__/tarPathValidationAttacks.test.ts index d363be93..08a9c20b 100644 --- a/packages/cache/__tests__/tarPathValidationAttacks.test.ts +++ b/packages/cache/__tests__/tarPathValidationAttacks.test.ts @@ -16,11 +16,13 @@ import {extractTar} from '../src/internal/tar' import {CacheIntegrityError} from '../src/internal/cacheIntegrityError' /** - * Parser-differential bypass regression tests. These build the F1 / F2 / - * F2-linkpath / F3 / F5 PoC archives from the security analysis as raw tar - * bytes (so we can craft malicious PAX bodies and typeflags that node-tar's - * Header encoder would never produce) and assert the validator now refuses - * each one. See docs/zip-slip-* for the analysis. + * Parser-differential bypass regression tests. Each case builds a malicious + * archive as raw tar bytes (so we can craft PAX bodies and typeflags that + * node-tar's Header encoder would never produce) designed to make node-tar's + * in-process listing disagree with the path the system `tar` extractor would + * write to, then asserts the validator refuses it. The vectors covered are: an + * unknown typeflag byte, a PAX `path=` / `linkpath=` record with an embedded + * newline, an oversized PAX header, and a GNU sparse typeflag. */ // --------------------------------------------------------------------------- @@ -115,30 +117,33 @@ function paxRecord(content: string): string { } // --------------------------------------------------------------------------- -// PoC archives +// Malicious archives // --------------------------------------------------------------------------- -// F1 — unknown typeflag byte ('Z') is emitted by node-tar as an ignoredEntry. -const F1 = Buffer.concat([ +// Unknown typeflag byte ('Z') is emitted by node-tar as an ignoredEntry, but +// system tar would extract it as a regular file. +const unknownTypeflagArchive = Buffer.concat([ fileEntry('cache/safe.txt', 'ok'), - fileEntry('../../../../../../tmp/zip_slip_F1', 'F1 pwned', 'Z'), + fileEntry('../../../../../../tmp/escaped_unknown_type', 'pwned', 'Z'), end() ]) -// F2 — PAX `path=` newline differential. -const F2 = Buffer.concat([ +// PAX `path=` record whose value carries an embedded newline. node-tar's naive +// `split('\n')` parse resolves the trailing `path=safe.txt`, while a +// length-correct parse (matching system tar) resolves the escaping path. +const paxPathNewlineArchive = Buffer.concat([ paxEntry( Buffer.from( - '42 path=../../../../../../tmp/zip_slip_F2\n30 comment=x\n17 path=safe.txt\n', + '42 path=../../../../../../tmp/escaped_pax\n30 comment=x\n17 path=safe.txt\n', 'ascii' ) ), - fileEntry('cache/safe.txt', 'F2 pwned'), + fileEntry('cache/safe.txt', 'pwned'), end() ]) -// F2-linkpath — same differential, applied to a symlink's `linkpath=`. -const F2L = Buffer.concat([ +// The same embedded-newline differential applied to a symlink's `linkpath=`. +const paxLinkpathNewlineArchive = Buffer.concat([ paxEntry( Buffer.from( '34 linkpath=../../../../../../tmp\n37 comment=x\n24 linkpath=safe/target\n', @@ -149,23 +154,24 @@ const F2L = Buffer.concat([ end() ]) -// F3 — oversized PAX header (> 1 MiB) is dropped by node-tar's -// maxMetaEntrySize and would otherwise let a `path=` override slip through. -const F3 = Buffer.concat([ +// Oversized PAX header (> 1 MiB) is dropped by node-tar's maxMetaEntrySize and +// would otherwise let the `path=` override slip through unseen. +const oversizedPaxHeaderArchive = Buffer.concat([ paxEntry( Buffer.concat([ Buffer.from(`1048600 comment=${'A'.repeat(1048600 - 17)}\n`, 'ascii'), - Buffer.from('42 path=../../../../../../tmp/zip_slip_F3\n', 'ascii') + Buffer.from('42 path=../../../../../../tmp/escaped_big\n', 'ascii') ]) ), - fileEntry('cache/safe.txt', 'F3 pwned'), + fileEntry('cache/safe.txt', 'pwned'), end() ]) -// F5 — sparse typeflag 'S' is mapped but ignored by node-tar's ReadEntry. -const F5 = Buffer.concat([ +// GNU sparse typeflag 'S' is mapped but ignored by node-tar's ReadEntry, while +// system tar would extract it. +const sparseTypeflagArchive = Buffer.concat([ fileEntry('cache/decoy.txt', 'ok'), - fileEntry('../../../../../../tmp/zip_slip_F5', '', 'S'), + fileEntry('../../../../../../tmp/escaped_sparse', '', 'S'), end() ]) @@ -228,33 +234,50 @@ afterAll(() => { }) describe('listAndValidate: parser-differential bypass detection', () => { - test('F1: unknown typeflag is rejected as UNSUPPORTED_TYPE', async () => { - const {violations, approvedNames} = await validate(F1, 'f1.tar.gz') + test('unknown typeflag is rejected as UNSUPPORTED_TYPE', async () => { + const {violations, approvedNames} = await validate( + unknownTypeflagArchive, + 'unknown-typeflag.tar.gz' + ) expect(violations).toContain('UNSUPPORTED_TYPE') // The escaping entry must NOT be approved for extraction. - expect(approvedNames).not.toContain('../../../../../../tmp/zip_slip_F1') + expect(approvedNames).not.toContain( + '../../../../../../tmp/escaped_unknown_type' + ) }) - test('F2: PAX path newline differential is rejected as PAX_DESYNC', async () => { - const {violations, approvedNames} = await validate(F2, 'f2.tar.gz') + test('PAX path newline differential is rejected as PAX_DESYNC', async () => { + const {violations, approvedNames} = await validate( + paxPathNewlineArchive, + 'pax-path-newline.tar.gz' + ) expect(violations).toContain('PAX_DESYNC') expect(approvedNames).toEqual([]) }) - test('F2-linkpath: PAX linkpath newline differential is rejected as PAX_DESYNC', async () => { - const {violations} = await validate(F2L, 'f2l.tar.gz') + test('PAX linkpath newline differential is rejected as PAX_DESYNC', async () => { + const {violations} = await validate( + paxLinkpathNewlineArchive, + 'pax-linkpath-newline.tar.gz' + ) expect(violations).toContain('PAX_DESYNC') }) - test('F3: oversized PAX header is rejected as UNSUPPORTED_TYPE', async () => { - const {violations} = await validate(F3, 'f3.tar.gz') + test('oversized PAX header is rejected as UNSUPPORTED_TYPE', async () => { + const {violations} = await validate( + oversizedPaxHeaderArchive, + 'oversized-pax.tar.gz' + ) expect(violations).toContain('UNSUPPORTED_TYPE') }) - test('F5: sparse typeflag is rejected as UNSUPPORTED_TYPE', async () => { - const {violations, approvedNames} = await validate(F5, 'f5.tar.gz') + test('sparse typeflag is rejected as UNSUPPORTED_TYPE', async () => { + const {violations, approvedNames} = await validate( + sparseTypeflagArchive, + 'sparse-typeflag.tar.gz' + ) expect(violations).toContain('UNSUPPORTED_TYPE') - expect(approvedNames).not.toContain('../../../../../../tmp/zip_slip_F5') + expect(approvedNames).not.toContain('../../../../../../tmp/escaped_sparse') }) test('glob metacharacter in entry path is rejected as GLOB_METACHAR', async () => { @@ -287,14 +310,14 @@ describe('listAndValidate: parser-differential bypass detection', () => { expect(violations).toContain('UNSAFE_CHAR') }) - test('NUL byte in a symlink target (via PAX) is rejected as UNSAFE_CHAR', async () => { + test('NUL byte in a symlink target (via PAX) is rejected as NUL_BYTE', async () => { const archive = Buffer.concat([ paxEntry(Buffer.from(paxRecord('linkpath=cache/sub/t\0'), 'ascii')), header({name: 'cache/link', typeflag: '2', linkname: 'cache/sub/t'}), end() ]) const {violations} = await validate(archive, 'nul-link.tar.gz') - expect(violations).toContain('UNSAFE_CHAR') + expect(violations).toContain('NUL_BYTE') }) test('legitimate long path via PAX: no violations, approved by its PAX path', async () => { @@ -323,6 +346,28 @@ describe('listAndValidate: parser-differential bypass detection', () => { expect(violations).toContain('PAX_UNKNOWN_KEY') }) + test('PAX sparse (GNU.sparse.name) is rejected as PAX_UNSUPPORTED_KEY', async () => { + // node-tar v7 ignores GNU sparse keys, so without this rejection the entry + // would be approved under its benign header path while system tar would + // reconstruct the file at GNU.sparse.name (here outside the cache roots). + const sparseName = '../../../../../../tmp/escaped_sparse_pax' + const body = Buffer.from( + paxRecord('GNU.sparse.major=1') + + paxRecord('GNU.sparse.minor=0') + + paxRecord(`GNU.sparse.name=${sparseName}`) + + paxRecord('GNU.sparse.realsize=4'), + 'ascii' + ) + const archive = Buffer.concat([ + paxEntry(body), + fileEntry('cache/GNUSparseFile.0/decoy', 'data'), + end() + ]) + const {violations, approvedNames} = await validate(archive, 'sparse.tar.gz') + expect(violations).toContain('PAX_UNSUPPORTED_KEY') + expect(approvedNames).not.toContain(sparseName) + }) + test('flood of extended headers is rejected (pending-meta cap)', async () => { const metas: Buffer[] = [] for (let i = 0; i < 70; i++) { @@ -382,12 +427,12 @@ describeTar('extractTar end-to-end with system tar allow-list', () => { expect(existsSync(path.join(dest, 'cache', 'sub', 'deep.txt'))).toBe(true) }) - test('error mode, F2 archive: throws and writes nothing to the workspace', async () => { - const dest = mkdtempSync(path.join(ROOT, 'extract-f2-')) + test('error mode, PAX path newline archive: throws and writes nothing to the workspace', async () => { + const dest = mkdtempSync(path.join(ROOT, 'extract-pax-path-newline-')) process.env['GITHUB_WORKSPACE'] = dest - const archivePath = path.join(dest, 'f2.tar.gz') + const archivePath = path.join(dest, 'pax-path-newline.tar.gz') mkdirSync(dest, {recursive: true}) - writeFileSync(archivePath, gzipSync(F2)) + writeFileSync(archivePath, gzipSync(paxPathNewlineArchive)) await expect( extractTar(archivePath, CompressionMethod.Gzip, { @@ -400,4 +445,55 @@ describeTar('extractTar end-to-end with system tar allow-list', () => { expect(existsSync(path.join(dest, 'cache'))).toBe(false) expect(existsSync(path.join(dest, 'safe.txt'))).toBe(false) }) + + test('error mode: a leading ./ entry still extracts (allow-list name matches)', async () => { + // node-tar surfaces the entry as `./cache/dotslash.txt`; the `-T` allow + // list must use the canonical `cache/dotslash.txt` so the member is not + // silently skipped. Verifies the canonicalMemberName normalization under + // whichever system tar is present (GNU on Linux CI, BSD on macOS). + const dest = mkdtempSync(path.join(ROOT, 'extract-dotslash-')) + process.env['GITHUB_WORKSPACE'] = dest + const archive = Buffer.concat([ + dirEntry('cache/'), + fileEntry('./cache/dotslash.txt', 'dot'), + end() + ]) + const archivePath = path.join(dest, 'dotslash.tar.gz') + mkdirSync(dest, {recursive: true}) + writeFileSync(archivePath, gzipSync(archive)) + + await extractTar(archivePath, CompressionMethod.Gzip, { + declaredPaths: ['cache/**'], + pathValidation: 'error' + }) + + expect(existsSync(path.join(dest, 'cache', 'dotslash.txt'))).toBe(true) + expect(readFileSync(path.join(dest, 'cache', 'dotslash.txt'), 'utf8')).toBe( + 'dot' + ) + }) + + test('error mode: a long path via PAX is extracted, not dropped by the allow-list', async () => { + // > 100 bytes, so the name travels via a PAX `path=` record (and a GNU + // long-name on creation). Exercises long-name matching in the `-T` list so + // a legitimate long path is not silently skipped during extraction. + const dest = mkdtempSync(path.join(ROOT, 'extract-long-')) + process.env['GITHUB_WORKSPACE'] = dest + const longRel = `cache/${'x'.repeat(110)}.txt` + const archive = Buffer.concat([ + paxEntry(Buffer.from(paxRecord(`path=${longRel}`), 'ascii')), + fileEntry('cache/placeholder', 'L'), + end() + ]) + const archivePath = path.join(dest, 'long.tar.gz') + mkdirSync(dest, {recursive: true}) + writeFileSync(archivePath, gzipSync(archive)) + + await extractTar(archivePath, CompressionMethod.Gzip, { + declaredPaths: ['cache/**'], + pathValidation: 'error' + }) + + expect(existsSync(path.join(dest, longRel))).toBe(true) + }) }) diff --git a/packages/cache/docs/path-validation-test-plan.md b/packages/cache/docs/path-validation-test-plan.md index 9f6ea95b..6151bad5 100644 --- a/packages/cache/docs/path-validation-test-plan.md +++ b/packages/cache/docs/path-validation-test-plan.md @@ -97,6 +97,12 @@ and platform-specific behavior: `C:/...`, Windows drive-relative `C:foo`, UNC `\\server\share`, UNC forward-slash, UNC long-path prefix `\\?\C:\...` - **NUL byte attacks**: NUL in path, NUL in symlink target +- **Unsafe control characters**: embedded newline in an entry path or link + target → `UNSAFE_CHAR` (would corrupt a newline-delimited tar file list and + enables log injection). A tab is still accepted (legal, non-corrupting). +- **Glob metacharacters** (`* ? [ ]`) in an entry path → `GLOB_METACHAR` + (entry paths populate the system-tar `-T` allow-list, which bsdtar matches + with `fnmatch()`; link targets are exempt since they are not written there) - **Symlink attacks**: - **Syntactic link-target rejects** (these fire before the containment check, on the same allow-list as entry-path syntax): POSIX absolute (`/etc/passwd`), @@ -149,11 +155,17 @@ cross-checks the result against `node-tar`'s view of the entry. ### `crossCheckMetaBodies` - Clean PAX `path` matching `node-tar` → no violations -- **F2 path desync** → `PAX_DESYNC` (node-tar resolved a safe name, the +- **PAX path desync** → `PAX_DESYNC` (node-tar resolved a safe name, the length-correct parse disagrees) -- **F2-linkpath desync** → `PAX_DESYNC` +- **PAX linkpath desync** → `PAX_DESYNC` - Unknown PAX key → `PAX_UNKNOWN_KEY` -- Known `SCHILY.` / `GNU.` / `LIBARCHIVE.` prefixed keys accepted +- **`GNU.sparse.*` keys → `PAX_UNSUPPORTED_KEY`** — node-tar v7 ignores GNU + sparse keys, so it surfaces the entry under its header `path` while system + tar would reconstruct the file at `GNU.sparse.name` (an attacker-controlled, + potentially escaping location). A cache archive never legitimately contains + sparse members, so the namespace is rejected outright even though it falls + under the broadly-allowed `GNU.` prefix. +- Known `SCHILY.` / `LIBARCHIVE.` prefixed keys accepted - GNU long-name raw body matching the entry path (or the link target) → no violation; a body matching neither → flagged - No meta bodies → no violations @@ -164,23 +176,25 @@ cross-checks the result against `node-tar`'s view of the entry. ## Integration tests — parser-differential attacks (`tarPathValidationAttacks.test.ts`) -These build the F1 / F2 / F2-linkpath / F3 / F5 proof-of-concept archives from -the security analysis (see `docs/zip-slip-*`) as **raw tar bytes**, so they can -craft malicious PAX bodies and typeflags that `node-tar`'s `Header` encoder -would never emit. They assert the validator refuses each one, and they include -real-`tar` end-to-end extraction assertions (using a scratch dir via -`mkdtempSync`). +These build malicious archives as **raw tar bytes**, so they can craft PAX +bodies and typeflags that `node-tar`'s `Header` encoder would never emit. Each +is designed to make node-tar's in-process listing disagree with the path the +system `tar` extractor would write to. They assert the validator refuses each +one, and they include real-`tar` end-to-end extraction assertions (using a +scratch dir via `mkdtempSync`). ### Bypass detection (via `listAndValidate`) -- **F1**: unknown typeflag → `UNSUPPORTED_TYPE` -- **F2**: PAX `path` newline differential → `PAX_DESYNC` -- **F2-linkpath**: PAX `linkpath` newline differential → `PAX_DESYNC` -- **F3**: oversized PAX header → `UNSUPPORTED_TYPE` -- **F5**: sparse typeflag → `UNSUPPORTED_TYPE` +- Unknown typeflag → `UNSUPPORTED_TYPE` +- PAX `path` newline differential → `PAX_DESYNC` +- PAX `linkpath` newline differential → `PAX_DESYNC` +- Oversized PAX header → `UNSUPPORTED_TYPE` +- GNU sparse typeflag → `UNSUPPORTED_TYPE` +- **PAX sparse** (`GNU.sparse.name` with a regular typeflag) → `PAX_UNSUPPORTED_KEY`, + and the escaping sparse name is not approved - Glob metacharacter in entry path → `GLOB_METACHAR` - Newline in entry path → `UNSAFE_CHAR` -- NUL byte in a symlink target (delivered via PAX) → `UNSAFE_CHAR` +- NUL byte in a symlink target (delivered via PAX) → `NUL_BYTE` - Unknown PAX key → `PAX_UNKNOWN_KEY` - Flood of extended headers → rejected by the pending-meta cap - Clean archive → `approvedNames` lists every concrete entry, no violations @@ -188,9 +202,16 @@ real-`tar` end-to-end extraction assertions (using a scratch dir via ### End-to-end extraction (real `tar`) +Run against whichever system `tar` is present, so CI exercises GNU tar on Linux +and bsdtar on macOS (and `tar.exe` on Windows): + - `'error'` mode, clean archive → every approved member is extracted -- `'error'` mode, F2 archive → throws `CacheIntegrityError` and writes nothing - to the workspace +- `'error'` mode, PAX path newline archive → throws `CacheIntegrityError` and + writes nothing to the workspace +- `'error'` mode, leading `./` entry → still extracted (the `-T` allow-list + uses the canonical `cache/f` name, so the member is not silently skipped) +- `'error'` mode, long path (> 100 bytes) delivered via PAX → extracted, not + dropped by the allow-list (exercises long-name matching in `-T`) ## Integration tests — real archives (`listAndValidate.test.ts`) diff --git a/packages/cache/src/internal/listAndValidate.ts b/packages/cache/src/internal/listAndValidate.ts index 40a0e063..718304f2 100644 --- a/packages/cache/src/internal/listAndValidate.ts +++ b/packages/cache/src/internal/listAndValidate.ts @@ -29,11 +29,15 @@ export interface ListAndValidateResult { /** * Upper bound on the size of a PAX / GNU extended-header body the validator is - * willing to re-parse. node-tar's default is 1 MiB; real extended headers are - * a few hundred bytes at most. Anything larger is emitted as an + * willing to re-parse, passed to node-tar as `maxMetaEntrySize`. Real extended + * headers are a few hundred bytes at most; anything larger is emitted as an * `ignoredEntry` (see the handler below) and recorded as a violation rather - * than silently dropped — this is the F3 (oversized-PAX) defence, made - * explicit instead of relying on node-tar's internal default. + * than silently dropped — the oversized-extended-header defence. + * + * This deliberately matches node-tar's current 1 MiB default. Pinning it here + * (rather than leaving the option unset) keeps the oversized-header threshold + * under our control even if a future node-tar release changes its internal + * default. */ const META_REJECT_BYTES = 1024 * 1024 @@ -87,7 +91,9 @@ export async function listAndValidate( // body string of each PAX / GNU long-name header *before* it emits the // concrete entry the header applies to. We re-parse these length-correctly // (see pax-reparse.ts) and cross-check against node-tar's resolved - // path/linkpath to catch the F2 / F2-linkpath PAX newline differential. + // path / linkpath to catch PAX newline differentials, where an embedded + // newline in a PAX value makes node-tar resolve a different path than the + // system tar that performs the extraction. let pendingMeta: Buffer[] = [] const consumePendingMeta = (): Buffer[] => { @@ -96,6 +102,25 @@ export async function listAndValidate( return bodies } + // Record a single validation failure. Every call site shares the same shape + // — path / linkpath / type come from the ReadEntry; only the code, reason and + // resolved location vary — so funnel them through one helper. + const addViolation = ( + entry: ReadEntry, + code: PathValidationViolation['code'], + reason: string, + resolved: string + ): void => { + violations.push({ + path: entry.path, + linkpath: entry.linkpath || undefined, + resolved, + entryType: entry.type, + code, + reason + }) + } + // For gzip we let node-tar handle decompression internally (its built-in // gzip support is mature). For zstd we spawn the system `zstd` binary so // we get the same `--long=30` window-size handling as the existing @@ -109,8 +134,8 @@ export async function listAndValidate( // via the captured error below. strict: false, // Cap extended-header bodies explicitly so an oversized PAX header is - // turned into a recorded `ignoredEntry` violation rather than relying on - // node-tar's internal default (F3 defence). + // turned into a recorded `ignoredEntry` violation and the oversized-header + // threshold stays under our control (see META_REJECT_BYTES). maxMetaEntrySize: META_REJECT_BYTES, // Treat structural problems (bad archive, bad header, bad chksum) as // hard parse errors — silently ignoring them would let a corrupt @@ -129,6 +154,7 @@ export async function listAndValidate( onReadEntry: (entry: ReadEntry) => { try { const metaBodies = consumePendingMeta() + let approved = true // Cross-check any PAX / long-name headers that preceded this entry // against node-tar's resolved view. A disagreement means node-tar @@ -138,22 +164,16 @@ export async function listAndValidate( entry.path, entry.linkpath || undefined )) { - violations.push({ - path: entry.path, - linkpath: entry.linkpath || undefined, - resolved: entry.path, - entryType: entry.type, - code: pax.code, - reason: pax.reason - }) + addViolation(entry, pax.code, pax.reason, entry.path) + approved = false } - // Reject characters that would corrupt the extraction allow-list or - // be reinterpreted by system tar's `-T` matching (glob metacharacters - // on bsdtar's fnmatch path). These have no legitimate use in a cache - // entry path. - const charViolation = checkUnsafeChars(entry.path, entry.linkpath) - + // validateEntry performs every per-string syntactic check (NUL and + // other control characters, glob metacharacters, UNC / absolute / + // drive-relative forms) on BOTH the entry path and any link target, + // plus the allowed-root containment check. Keeping all of those checks + // in one place (pathValidation) is what makes entry paths and link + // targets treated consistently. const result = validateEntry( entry.path, entry.linkpath || undefined, @@ -162,32 +182,16 @@ export async function listAndValidate( extractCwd ) if (!result.ok) { - violations.push({ - path: entry.path, - linkpath: entry.linkpath || undefined, - resolved: result.resolved, - entryType: entry.type, - code: result.code, - reason: result.reason - }) + addViolation(entry, result.code, result.reason, result.resolved) + approved = false } - if (charViolation) { - violations.push({ - path: entry.path, - linkpath: entry.linkpath || undefined, - resolved: entry.path, - entryType: entry.type, - code: charViolation.code, - reason: charViolation.reason - }) - } - - // Only entries that passed every check are eligible for the - // extraction allow-list. (When any violation exists the allow-list is - // unused — extraction is either blocked in 'error' mode or runs - // unrestricted in 'warn' mode — so this is belt-and-braces.) - if (result.ok && !charViolation) { + // Only entries that passed every check (PAX cross-check and path + // validation) are eligible for the extraction allow-list. When any + // violation exists the allow-list is unused anyway — extraction is + // either blocked in 'error' mode or runs unrestricted in 'warn' mode — + // but tracking approval per entry keeps the contract simple. + if (approved) { approvedNames.push(entry.path) } } finally { @@ -207,14 +211,12 @@ export async function listAndValidate( // The meta header(s) that preceded an ignored entry belong to it; discard // them so they aren't mis-associated with a later concrete entry. consumePendingMeta() - violations.push({ - path: entry.path, - linkpath: entry.linkpath || undefined, - resolved: entry.path, - entryType: entry.type, - code: 'UNSUPPORTED_TYPE', - reason: `parser ignored entry of type ${entry.type}` - }) + addViolation( + entry, + 'UNSUPPORTED_TYPE', + `parser ignored entry of type ${entry.type}`, + entry.path + ) }) // Capture the raw body of each extended-header (meta) entry. node-tar @@ -239,50 +241,6 @@ export async function listAndValidate( return {violations, approvedNames} } -/** - * Reject characters in an entry path (or link target) that have no legitimate - * place in a cache archive and that would either corrupt the extraction - * allow-list or be reinterpreted by system tar's `-T` member matching: - * - * - NUL / newline in the entry path — would split or terminate a list entry. - * - glob metacharacters (`* ? [ ]`) in the entry path — bsdtar matches `-T` - * names with `fnmatch()`, so an unescaped metacharacter could match (and - * extract) members other than the one approved. GNU tar's `--no-wildcards` - * also covers this, but rejecting unconditionally keeps the behaviour - * identical across tar implementations. - * - NUL in a link target — same list-corruption concern. - */ -function checkUnsafeChars( - entryPath: string, - linkPath: string | undefined -): {code: 'UNSAFE_CHAR' | 'GLOB_METACHAR'; reason: string} | undefined { - if (entryPath.includes('\0') || entryPath.includes('\n')) { - return { - code: 'UNSAFE_CHAR', - reason: `entry path contains an unsafe control character: ${JSON.stringify( - entryPath - )}` - } - } - if (/[*?[\]]/.test(entryPath)) { - return { - code: 'GLOB_METACHAR', - reason: `entry path contains a glob metacharacter: ${JSON.stringify( - entryPath - )}` - } - } - if (linkPath !== undefined && linkPath.includes('\0')) { - return { - code: 'UNSAFE_CHAR', - reason: `link target contains an unsafe control character: ${JSON.stringify( - linkPath - )}` - } - } - return undefined -} - async function streamArchiveTo( archivePath: string, compressionMethod: CompressionMethod, diff --git a/packages/cache/src/internal/pathValidation.ts b/packages/cache/src/internal/pathValidation.ts index ab2d531f..3369cf63 100644 --- a/packages/cache/src/internal/pathValidation.ts +++ b/packages/cache/src/internal/pathValidation.ts @@ -30,6 +30,7 @@ export interface PathValidationViolation { | 'PAX_DESYNC' | 'PAX_PARSE_FAIL' | 'PAX_UNKNOWN_KEY' + | 'PAX_UNSUPPORTED_KEY' | 'UNSAFE_CHAR' | 'GLOB_METACHAR' /** Human-readable description of the violation. */ @@ -400,7 +401,12 @@ export function validateEntry( * pre-resolution syntactic checks. */ interface PathSyntaxError { - code: 'NUL_BYTE' | 'UNC_PATH' | 'ABSOLUTE_PATH' + code: + | 'NUL_BYTE' + | 'UNSAFE_CHAR' + | 'UNC_PATH' + | 'ABSOLUTE_PATH' + | 'GLOB_METACHAR' reason: string } @@ -429,6 +435,19 @@ function checkPathSyntax( if (p.includes('\0')) { return {code: 'NUL_BYTE', reason: `NUL byte in ${kind}`} } + // Reject newline characters. They have no legitimate place in a cache entry + // path or link target, would corrupt a newline-delimited tar file list, and + // enable log / line injection in the violation output. NUL is handled above + // with its own, more specific code. Applied to both entry paths and link + // targets so the two are treated consistently. + if (p.includes('\n')) { + return { + code: 'UNSAFE_CHAR', + reason: `unsafe control character (newline) in ${kind}: ${JSON.stringify( + p + )}` + } + } // Reject UNC paths. Check the original string before any separator // normalization because UNC is identified by leading `\\` or `//`. if ( @@ -454,6 +473,19 @@ function checkPathSyntax( reason: `absolute ${kind} not allowed: ${p}` } } + // Reject glob metacharacters, but only in entry paths. Approved entry paths + // are written to the system-tar `-T` extraction allow-list, and bsdtar + // matches those names with fnmatch(), so an unescaped `*`, `?`, `[` or `]` + // could select (and extract) members other than the one approved. GNU tar + // is additionally run with --no-wildcards, but rejecting here keeps the + // behavior identical across tar implementations. Link targets are not + // written to the allow-list, so they are exempt. + if (kind === 'entry path' && /[*?[\]]/.test(p)) { + return { + code: 'GLOB_METACHAR', + reason: `glob metacharacter in ${kind}: ${JSON.stringify(p)}` + } + } return undefined } diff --git a/packages/cache/src/internal/pax-reparse.ts b/packages/cache/src/internal/pax-reparse.ts index 44f87e4c..129043e7 100644 --- a/packages/cache/src/internal/pax-reparse.ts +++ b/packages/cache/src/internal/pax-reparse.ts @@ -13,8 +13,7 @@ * legally contain `\n`. GNU tar and libarchive (bsdtar / Windows `tar.exe`) * both parse by consuming exactly `` bytes, so a value * containing an embedded `"\n path=\n"` desynchronises node-tar - * from every real extractor — the F2 / F2-linkpath parser-differential - * bypasses. + * from every real extractor — a path / linkpath parser-differential bypass. * * This module re-parses each captured PAX body the way every real extractor * does (length-prefixed, byte-accurate) and cross-checks the result against @@ -29,7 +28,11 @@ */ /** Machine-readable reason codes produced by the PAX cross-check. */ -export type PaxReparseCode = 'PAX_DESYNC' | 'PAX_PARSE_FAIL' | 'PAX_UNKNOWN_KEY' +export type PaxReparseCode = + | 'PAX_DESYNC' + | 'PAX_PARSE_FAIL' + | 'PAX_UNKNOWN_KEY' + | 'PAX_UNSUPPORTED_KEY' /** A single disagreement surfaced by {@link crossCheckMetaBodies}. */ export interface PaxReparseViolation { @@ -87,6 +90,24 @@ function isKnownPaxKey(key: string): boolean { return PAX_KNOWN_PREFIXES.some(prefix => key.startsWith(prefix)) } +/** + * PAX key prefixes that system tar (GNU tar / libarchive) acts on to place or + * reconstruct file content but that node-tar v7 does NOT process. node-tar + * surfaces such an entry under its header `path`, while system tar would write + * it elsewhere — GNU sparse files are reconstructed at `GNU.sparse.name`, which + * an attacker can point outside the cache roots. That is a listing-vs-extraction + * parser differential the `path` / `linkpath` cross-check cannot see (the key is + * neither `path` nor `linkpath`). A cache archive never legitimately contains + * sparse members, so any key in this namespace is rejected outright. This is + * checked before {@link isKnownPaxKey} so it takes precedence over the broad + * `GNU.` allow-list prefix. + */ +const PAX_REJECTED_PREFIXES: readonly string[] = ['GNU.sparse.'] + +function isRejectedPaxKey(key: string): boolean { + return PAX_REJECTED_PREFIXES.some(prefix => key.startsWith(prefix)) +} + /** Result of a length-correct parse of a single PAX extended-header body. */ export interface PaxParseResult { /** Decoded records, last-write-wins per key (POSIX-correct). Values kept as @@ -200,7 +221,12 @@ export function crossCheckMetaBodies( // A well-formed PAX extended header. sawPax = true for (const key of keys) { - if (!isKnownPaxKey(key)) { + if (isRejectedPaxKey(key)) { + violations.push({ + code: 'PAX_UNSUPPORTED_KEY', + reason: `unsupported placement-affecting PAX key '${key}' in extended header (node-tar ignores it; system tar would act on it)` + }) + } else if (!isKnownPaxKey(key)) { violations.push({ code: 'PAX_UNKNOWN_KEY', reason: `unknown PAX key '${key}' in extended header` diff --git a/packages/cache/src/internal/tar.ts b/packages/cache/src/internal/tar.ts index 14999489..93c83f38 100644 --- a/packages/cache/src/internal/tar.ts +++ b/packages/cache/src/internal/tar.ts @@ -20,7 +20,8 @@ import { PathValidationMode, PathValidationViolation, deriveAllowedRoots, - formatViolationSummary + formatViolationSummary, + getWorkingDirectory } from './pathValidation.js' const IS_WINDOWS = process.platform === 'win32' @@ -203,10 +204,6 @@ async function getCommands( return [args.join(' ')] } -function getWorkingDirectory(): string { - return process.env['GITHUB_WORKSPACE'] ?? process.cwd() -} - // Common function for extractTar and listTar to get the compression method async function getDecompressionProgram( tarPath: ArchiveTool, @@ -431,16 +428,37 @@ function writeAllowList(approvedNames: string[]): string { const allowListPath = path.join( os.tmpdir(), `cache-allow-${process.pid}-${Date.now()}-${crypto - .randomBytes(4) + .randomBytes(8) .toString('hex')}.lst` ) const payload = Buffer.concat( - approvedNames.flatMap(name => [Buffer.from(name, 'utf8'), Buffer.from([0])]) + approvedNames.flatMap(name => [ + Buffer.from(canonicalMemberName(name), 'utf8'), + Buffer.from([0]) + ]) ) - writeFileSync(allowListPath, payload, {mode: 0o600}) + // `flag: 'wx'` (O_CREAT | O_EXCL | O_WRONLY) makes the open fail if the path + // already exists, so a file or symlink pre-planted at the (randomized) temp + // path on a shared/self-hosted runner cannot redirect or capture the write. + // mode 0o600 keeps the list readable only by the current user. Both options + // behave consistently on Windows, macOS and Linux. + writeFileSync(allowListPath, payload, {mode: 0o600, flag: 'wx'}) return allowListPath } +/** + * Canonicalize an approved entry name to the form system `tar` matches against + * its archive members when reading the `-T` allow-list. node-tar already + * converts backslashes to forward slashes; here we additionally strip any + * leading `./` (both GNU tar and bsdtar normalize member names this way) so an + * entry node-tar surfaced as `./cache/f` still matches the member `cache/f` + * and is not silently skipped during extraction. A trailing slash on a + * directory entry is preserved because tar matches directories with it. + */ +function canonicalMemberName(name: string): string { + return name.replace(/^(?:\.\/)+/, '') +} + function reportViolations( violations: PathValidationViolation[], mode: PathValidationMode