From 6e8379ea7a2a89b5dfe5a19a8aa5dd609c4e4480 Mon Sep 17 00:00:00 2001 From: Jason Ginchereau Date: Fri, 19 Jun 2026 09:17:41 -1000 Subject: [PATCH] Implement PAX header validation and path safety checks in tar extraction - Added a new test suite for parser-differential bypass detection in tar archives. - Introduced length-correct PAX extended-header re-parsing to catch discrepancies between node-tar and system tar. - Enhanced the listAndValidate function to return approved names alongside violations for better extraction control. - Implemented checks for unsafe characters and glob metacharacters in entry paths. - Updated the tar extraction logic to utilize an allow-list for approved entries when path validation is in error mode. - Added utility functions for writing temporary allow-list files for system tar extraction. --- .../cache/__tests__/listAndValidate.test.ts | 12 +- packages/cache/__tests__/pax-reparse.test.ts | 255 +++++++++++ .../cache/__tests__/tarPathValidation.test.ts | 148 +++++-- .../tarPathValidationAttacks.test.ts | 403 ++++++++++++++++++ .../cache/src/internal/listAndValidate.ts | 183 +++++++- packages/cache/src/internal/pathValidation.ts | 5 + packages/cache/src/internal/pax-reparse.ts | 271 ++++++++++++ packages/cache/src/internal/tar.ts | 102 ++++- 8 files changed, 1332 insertions(+), 47 deletions(-) create mode 100644 packages/cache/__tests__/pax-reparse.test.ts create mode 100644 packages/cache/__tests__/tarPathValidationAttacks.test.ts create mode 100644 packages/cache/src/internal/pax-reparse.ts diff --git a/packages/cache/__tests__/listAndValidate.test.ts b/packages/cache/__tests__/listAndValidate.test.ts index 743ad637..dcdda54a 100644 --- a/packages/cache/__tests__/listAndValidate.test.ts +++ b/packages/cache/__tests__/listAndValidate.test.ts @@ -5,7 +5,17 @@ import * as zlib from 'zlib' import {execSync} from 'child_process' import {Header} from 'tar' import {CompressionMethod} from '../src/internal/constants' -import {listAndValidate} from '../src/internal/listAndValidate' +import {listAndValidate as listAndValidateImpl} from '../src/internal/listAndValidate' + +/** + * Thin wrapper returning only the violations array, so the many existing + * assertions below can keep treating the result as a flat list. New tests that + * need the `approvedNames` allow-list call `listAndValidateImpl` directly. + */ +const listAndValidate = async ( + ...args: Parameters +): Promise>['violations']> => + (await listAndValidateImpl(...args)).violations /** * Real-archive integration tests for listAndValidate. These build small tar diff --git a/packages/cache/__tests__/pax-reparse.test.ts b/packages/cache/__tests__/pax-reparse.test.ts new file mode 100644 index 00000000..4e052841 --- /dev/null +++ b/packages/cache/__tests__/pax-reparse.test.ts @@ -0,0 +1,255 @@ +import { + parsePaxLengthCorrect, + crossCheckMetaBodies, + PAX_KNOWN_KEYS +} from '../src/internal/pax-reparse' + +/** Build a length-correct PAX record string for `=` content. */ +function rec(content: string): string { + const base = 1 + Buffer.byteLength(content) + 1 // space + content + LF + let len = base + String(base).length + if (String(len).length !== String(base).length) { + len = base + String(len).length + } + return `${len} ${content}\n` +} + +describe('parsePaxLengthCorrect', () => { + test('single well-formed record', () => { + const buf = Buffer.from('17 path=safe.txt\n', 'ascii') + const {records, ok} = parsePaxLengthCorrect(buf) + expect(ok).toBe(true) + expect(records['path'].toString('utf8')).toBe('safe.txt') + }) + + 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 + // (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', + 'ascii' + ) + const {records, ok} = parsePaxLengthCorrect(buf) + expect(ok).toBe(true) + expect(records['path'].toString('utf8')).toBe( + '../../../../../../tmp/zip_slip_F2' + ) + expect(records['comment'].toString('utf8')).toBe('x\n17 path=safe.txt') + // Crucially NOT safe.txt. + expect(records['path'].toString('utf8')).not.toBe('safe.txt') + }) + + test('empty buffer parses to empty record set', () => { + const {records, ok} = parsePaxLengthCorrect(Buffer.alloc(0)) + expect(ok).toBe(true) + expect(Object.keys(records)).toHaveLength(0) + }) + + test('truncated record: not ok', () => { + // length says 42 but the buffer is shorter + const buf = Buffer.from('42 path=too-short\n', 'ascii') + const {ok} = parsePaxLengthCorrect(buf) + expect(ok).toBe(false) + }) + + test('non-numeric length prefix: not ok', () => { + const buf = Buffer.from('xx path=foo\n', 'ascii') + const {ok} = parsePaxLengthCorrect(buf) + expect(ok).toBe(false) + }) + + test('missing trailing newline: not ok', () => { + // "16 path=safe.txt" is 16 bytes but the last byte is 't', not '\n' + const buf = Buffer.from('16 path=safe.txt', 'ascii') + const {ok} = parsePaxLengthCorrect(buf) + expect(ok).toBe(false) + }) + + test('length spans past buffer end: not ok', () => { + const buf = Buffer.from('99 path=x\n', 'ascii') + const {ok} = parsePaxLengthCorrect(buf) + expect(ok).toBe(false) + }) + + test('record without "=" : not ok', () => { + const buf = Buffer.from('10 nokeyval\n', 'ascii') + const {ok} = parsePaxLengthCorrect(buf) + expect(ok).toBe(false) + }) + + test('record with correct length+LF but no "=" : not ok', () => { + // "5 ab\n" is exactly 5 bytes and ends in LF, but has no '='. This + // exercises the missing-separator branch (distinct from a length/LF + // mismatch). + const buf = Buffer.from('5 ab\n', 'ascii') + const {ok} = parsePaxLengthCorrect(buf) + expect(ok).toBe(false) + }) + + test('value may itself contain "=" — only the first one is the separator', () => { + const buf = Buffer.from(rec('comment=a=b=c'), 'ascii') + const {records, ok} = parsePaxLengthCorrect(buf) + expect(ok).toBe(true) + expect(records['comment'].toString('utf8')).toBe('a=b=c') + }) + + test('zero-length prefix is rejected', () => { + const buf = Buffer.from('0 path=x\n', 'ascii') + const {ok} = parsePaxLengthCorrect(buf) + expect(ok).toBe(false) + }) + + test('high-bit value bytes are preserved as raw Buffer', () => { + const value = Buffer.from([0xc3, 0x28, 0xff]) // invalid utf8 on purpose + const inner = Buffer.concat([Buffer.from('path=', 'ascii'), value]) + // record = " " + inner + "\n" + const base = 1 + inner.length + 1 + let len = base + String(base).length + if (String(len).length !== String(base).length) + len = base + String(len).length + const buf = Buffer.concat([ + Buffer.from(`${len} `, 'ascii'), + inner, + Buffer.from('\n', 'ascii') + ]) + const {records, ok} = parsePaxLengthCorrect(buf) + expect(ok).toBe(true) + expect(Buffer.compare(records['path'], value)).toBe(0) + }) + + test('last write wins for repeated keys', () => { + const buf = Buffer.from('14 path=a.txt\n14 path=b.txt\n', 'ascii') + const {records, ok} = parsePaxLengthCorrect(buf) + expect(ok).toBe(true) + expect(records['path'].toString('utf8')).toBe('b.txt') + }) +}) + +describe('crossCheckMetaBodies', () => { + test('clean PAX path matching node-tar: no violations', () => { + const buf = Buffer.from('17 path=safe.txt\n', 'ascii') + const v = crossCheckMetaBodies([buf], 'safe.txt', undefined) + expect(v).toEqual([]) + }) + + test('F2 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', + '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', () => { + const buf = Buffer.from( + '34 linkpath=../../../../../../tmp\n37 comment=x\n24 linkpath=safe/target\n', + 'ascii' + ) + const v = crossCheckMetaBodies([buf], 'cache/link', 'safe/target') + expect(v.map(x => x.code)).toContain('PAX_DESYNC') + }) + + test('unknown PAX key is rejected', () => { + const content = 'EVIL.placement=1' + const base = 1 + content.length + 1 + let len = base + String(base).length + if (String(len).length !== String(base).length) + len = base + String(len).length + const buf = Buffer.from(`${len} ${content}\n`, 'ascii') + const v = crossCheckMetaBodies([buf], 'cache/x', undefined) + expect(v.map(x => x.code)).toContain('PAX_UNKNOWN_KEY') + }) + + test('known SCHILY/GNU/LIBARCHIVE prefixed keys are accepted', () => { + const records = [ + 'SCHILY.xattr.user.foo=bar', + 'GNU.sparse.realsize=1024', + 'LIBARCHIVE.creationtime=1700000000' + ] + const body = records + .map(content => { + const base = 1 + content.length + 1 + let len = base + String(base).length + if (String(len).length !== String(base).length) + len = base + String(len).length + return `${len} ${content}\n` + }) + .join('') + const v = crossCheckMetaBodies( + [Buffer.from(body, 'ascii')], + 'cache/x', + undefined + ) + expect(v).toEqual([]) + }) + + 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([ + Buffer.from('cache/a/very/long/name.txt', 'ascii'), + Buffer.from([0, 0]) + ]) + const v = crossCheckMetaBodies( + [raw], + 'cache/a/very/long/name.txt', + undefined + ) + expect(v).toEqual([]) + }) + + test('long-name body that matches neither path nor linkpath is flagged', () => { + const raw = Buffer.from('something-unaccountable', 'ascii') + const v = crossCheckMetaBodies([raw], 'cache/x', undefined) + expect(v.map(x => x.code)).toContain('PAX_PARSE_FAIL') + }) + + test('no meta bodies: no violations', () => { + expect(crossCheckMetaBodies([], 'cache/x', undefined)).toEqual([]) + }) + + test('PAX setting both path and linkpath in agreement: no violations', () => { + const body = Buffer.from( + rec('path=cache/link') + rec('linkpath=cache/target'), + 'ascii' + ) + const v = crossCheckMetaBodies([body], 'cache/link', 'cache/target') + expect(v).toEqual([]) + }) + + test('directory path with trailing slash compares equal (no false desync)', () => { + // node-tar may resolve a directory entry without the trailing slash that + // the PAX record carries; normalizeForCompare must treat them as equal. + const body = Buffer.from(rec('path=cache/dir/'), 'ascii') + const v = crossCheckMetaBodies([body], 'cache/dir', undefined) + expect(v).toEqual([]) + }) + + test('multiple PAX bodies merge with last-write-wins before comparison', () => { + const bodies = [ + Buffer.from(rec('path=cache/first'), 'ascii'), + Buffer.from(rec('path=cache/second'), 'ascii') + ] + // node-tar's resolved path is the last write; agreement => no violation. + expect(crossCheckMetaBodies(bodies, 'cache/second', undefined)).toEqual([]) + // Disagreement with the merged (last) value => desync. + expect( + crossCheckMetaBodies(bodies, 'cache/first', undefined).map(x => x.code) + ).toContain('PAX_DESYNC') + }) + + test('GNU long-name raw body matching the link target (not the path)', () => { + const raw = Buffer.concat([ + Buffer.from('cache/sub/target', 'ascii'), + Buffer.from([0]) + ]) + const v = crossCheckMetaBodies([raw], 'cache/link', 'cache/sub/target') + expect(v).toEqual([]) + }) + + test('PAX_KNOWN_KEYS includes path and linkpath', () => { + expect(PAX_KNOWN_KEYS.has('path')).toBe(true) + expect(PAX_KNOWN_KEYS.has('linkpath')).toBe(true) + }) +}) diff --git a/packages/cache/__tests__/tarPathValidation.test.ts b/packages/cache/__tests__/tarPathValidation.test.ts index 3fad47d2..32bdb678 100644 --- a/packages/cache/__tests__/tarPathValidation.test.ts +++ b/packages/cache/__tests__/tarPathValidation.test.ts @@ -1,6 +1,7 @@ import * as exec from '@actions/exec' import * as core from '@actions/core' import * as io from '@actions/io' +import * as fs from 'fs' import * as path from 'path' import {CompressionMethod} from '../src/internal/constants' import * as tar from '../src/internal/tar' @@ -82,23 +83,26 @@ describe('extractTar path validation integration', () => { }) test('violations present: exactly one warning, debug per violation, extraction still proceeds', async () => { - jest.spyOn(listAndValidate, 'listAndValidate').mockResolvedValue([ - { - path: '../escape.txt', - resolved: '', - entryType: 'File', - code: 'OUTSIDE_ROOTS', - reason: 'escapes allowed roots' - }, - { - path: 'cache/link', - linkpath: '/etc/passwd', - resolved: '', - entryType: 'SymbolicLink', - code: 'LINK_OUTSIDE_ROOTS', - reason: 'symlink target outside allowed roots' - } - ]) + jest.spyOn(listAndValidate, 'listAndValidate').mockResolvedValue({ + approvedNames: [], + violations: [ + { + path: '../escape.txt', + resolved: '', + entryType: 'File', + code: 'OUTSIDE_ROOTS', + reason: 'escapes allowed roots' + }, + { + path: 'cache/link', + linkpath: '/etc/passwd', + resolved: '', + entryType: 'SymbolicLink', + code: 'LINK_OUTSIDE_ROOTS', + reason: 'symlink target outside allowed roots' + } + ] + }) const warnSpy = jest.spyOn(core, 'warning').mockImplementation() const debugSpy = jest.spyOn(core, 'debug').mockImplementation() const execMock = jest.spyOn(exec, 'exec').mockResolvedValue(0) @@ -120,15 +124,18 @@ describe('extractTar path validation integration', () => { }) test('single violation: warning text uses singular wording', async () => { - jest.spyOn(listAndValidate, 'listAndValidate').mockResolvedValue([ - { - path: '../boom.txt', - resolved: '', - entryType: 'File', - code: 'OUTSIDE_ROOTS', - reason: 'escapes' - } - ]) + jest.spyOn(listAndValidate, 'listAndValidate').mockResolvedValue({ + approvedNames: [], + violations: [ + { + path: '../boom.txt', + resolved: '', + entryType: 'File', + code: 'OUTSIDE_ROOTS', + reason: 'escapes' + } + ] + }) const warnSpy = jest.spyOn(core, 'warning').mockImplementation() jest.spyOn(core, 'debug').mockImplementation() jest.spyOn(exec, 'exec').mockResolvedValue(0) @@ -144,15 +151,18 @@ describe('extractTar path validation integration', () => { describe("mode 'error'", () => { test('violations present: throws CacheIntegrityError, system tar NEVER invoked', async () => { - jest.spyOn(listAndValidate, 'listAndValidate').mockResolvedValue([ - { - path: '../etc/passwd', - resolved: '', - entryType: 'File', - code: 'OUTSIDE_ROOTS', - reason: 'escapes allowed roots' - } - ]) + jest.spyOn(listAndValidate, 'listAndValidate').mockResolvedValue({ + approvedNames: [], + violations: [ + { + path: '../etc/passwd', + resolved: '', + entryType: 'File', + code: 'OUTSIDE_ROOTS', + reason: 'escapes allowed roots' + } + ] + }) jest.spyOn(core, 'warning').mockImplementation() jest.spyOn(core, 'debug').mockImplementation() const execMock = jest.spyOn(exec, 'exec').mockResolvedValue(0) @@ -183,7 +193,7 @@ describe('extractTar path validation integration', () => { ] jest .spyOn(listAndValidate, 'listAndValidate') - .mockResolvedValue(violations) + .mockResolvedValue({violations, approvedNames: []}) jest.spyOn(core, 'warning').mockImplementation() jest.spyOn(core, 'debug').mockImplementation() @@ -340,4 +350,70 @@ describe('extractTar path validation integration', () => { expect(listMock.mock.calls[0][1]).toBe(CompressionMethod.ZstdWithoutLong) }) }) + + describe("mode 'error' extraction allow-list", () => { + test('clean archive: extraction restricted to approved members via --null/-T', async () => { + jest.spyOn(listAndValidate, 'listAndValidate').mockResolvedValue({ + violations: [], + approvedNames: ['cache/a.txt', 'cache/b.txt'] + }) + jest.spyOn(io, 'mkdirP').mockResolvedValue() + + let command = '' + let listPath: string | undefined + let listContents: Buffer | undefined + jest.spyOn(exec, 'exec').mockImplementation(async cmd => { + command = cmd + const m = /-T "([^"]+)"/.exec(command) + if (m) { + listPath = m[1] + listContents = fs.readFileSync(listPath) + } + return 0 + }) + + await tar.extractTar(archive, CompressionMethod.Gzip, { + declaredPaths: ['cache/**'], + pathValidation: 'error' + }) + + // Allow-list flags are present and `--null` precedes `-T`. + expect(command).toContain('--null') + expect(command).toContain('--no-recursion') + expect(command).toMatch(/--null .*-T "[^"]+"/) + // GNU tar (the tool detected under the io.which mock on non-Windows) + // additionally receives the wildcard-hardening flags. + if (process.platform !== 'win32') { + expect(command).toContain('--no-wildcards') + expect(command).toContain('--anchored') + } + // The list is NUL-separated and contains exactly the approved names. + expect(listContents?.toString('utf8')).toBe('cache/a.txt\0cache/b.txt\0') + // The temporary allow-list file is cleaned up after extraction. + expect(listPath).toBeDefined() + expect(fs.existsSync(listPath as string)).toBe(false) + }) + + test('warn mode does not restrict extraction (no -T) even when clean', async () => { + jest.spyOn(listAndValidate, 'listAndValidate').mockResolvedValue({ + violations: [], + approvedNames: ['cache/a.txt'] + }) + jest.spyOn(io, 'mkdirP').mockResolvedValue() + + let command = '' + jest.spyOn(exec, 'exec').mockImplementation(async cmd => { + command = cmd + return 0 + }) + + await tar.extractTar(archive, CompressionMethod.Gzip, { + declaredPaths: ['cache/**'], + pathValidation: 'warn' + }) + + expect(command).not.toContain('-T "') + expect(command).not.toContain('--no-recursion') + }) + }) }) diff --git a/packages/cache/__tests__/tarPathValidationAttacks.test.ts b/packages/cache/__tests__/tarPathValidationAttacks.test.ts new file mode 100644 index 00000000..d363be93 --- /dev/null +++ b/packages/cache/__tests__/tarPathValidationAttacks.test.ts @@ -0,0 +1,403 @@ +import { + mkdtempSync, + writeFileSync, + rmSync, + mkdirSync, + existsSync, + readFileSync +} from 'fs' +import * as os from 'os' +import * as path from 'path' +import {gzipSync} from 'zlib' +import {execSync} from 'child_process' +import {CompressionMethod} from '../src/internal/constants' +import {listAndValidate} from '../src/internal/listAndValidate' +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. + */ + +// --------------------------------------------------------------------------- +// Raw tar construction +// --------------------------------------------------------------------------- + +const BLOCK = 512 + +function octal(n: number, len: number): Buffer { + return Buffer.from(`${n.toString(8).padStart(len - 1, '0')}\0`, 'ascii') +} + +function put( + buf: Buffer, + offset: number, + data: string | Buffer, + max: number +): void { + const b = Buffer.isBuffer(data) ? data : Buffer.from(data, 'ascii') + b.copy(buf, offset, 0, Math.min(b.length, max)) +} + +function header(opts: { + name?: string + mode?: number + size?: number + typeflag?: string + linkname?: string +}): Buffer { + const { + name = '', + mode = 0o644, + size = 0, + typeflag = '0', + linkname = '' + } = opts + const h = Buffer.alloc(BLOCK) + put(h, 0, name, 100) + octal(mode, 8).copy(h, 100) + octal(0, 8).copy(h, 108) + octal(0, 8).copy(h, 116) + octal(size, 12).copy(h, 124) + octal(0, 12).copy(h, 136) + h.fill(0x20, 148, 156) // chksum field = spaces while summing + put(h, 156, typeflag, 1) + put(h, 157, linkname, 100) + put(h, 257, 'ustar\0', 6) + put(h, 263, '00', 2) + let sum = 0 + for (let i = 0; i < BLOCK; i++) sum += h[i] + put(h, 148, `${sum.toString(8).padStart(6, '0')}\0 `, 8) + return h +} + +function pad(buf: Buffer): Buffer { + const p = (BLOCK - (buf.length % BLOCK)) % BLOCK + return p > 0 ? Buffer.concat([buf, Buffer.alloc(p)]) : buf +} + +function fileEntry(name: string, contents: string, typeflag = '0'): Buffer { + const body = Buffer.from(contents, 'ascii') + return Buffer.concat([header({name, size: body.length, typeflag}), pad(body)]) +} + +function dirEntry(name: string): Buffer { + return header({ + name: name.endsWith('/') ? name : `${name}/`, + mode: 0o755, + typeflag: '5' + }) +} + +function paxEntry(body: Buffer): Buffer { + return Buffer.concat([ + header({name: 'PaxHeader', size: body.length, typeflag: 'x'}), + pad(body) + ]) +} + +function end(): Buffer { + return Buffer.alloc(BLOCK * 2) +} + +/** Build a single length-correct PAX record (`" =\n"`). */ +function paxRecord(content: string): string { + const base = 1 + Buffer.byteLength(content) + 1 + let len = base + String(base).length + if (String(len).length !== String(base).length) { + len = base + String(len).length + } + return `${len} ${content}\n` +} + +// --------------------------------------------------------------------------- +// PoC archives +// --------------------------------------------------------------------------- + +// F1 — unknown typeflag byte ('Z') is emitted by node-tar as an ignoredEntry. +const F1 = Buffer.concat([ + fileEntry('cache/safe.txt', 'ok'), + fileEntry('../../../../../../tmp/zip_slip_F1', 'F1 pwned', 'Z'), + end() +]) + +// F2 — PAX `path=` newline differential. +const F2 = Buffer.concat([ + paxEntry( + Buffer.from( + '42 path=../../../../../../tmp/zip_slip_F2\n30 comment=x\n17 path=safe.txt\n', + 'ascii' + ) + ), + fileEntry('cache/safe.txt', 'F2 pwned'), + end() +]) + +// F2-linkpath — same differential, applied to a symlink's `linkpath=`. +const F2L = Buffer.concat([ + paxEntry( + Buffer.from( + '34 linkpath=../../../../../../tmp\n37 comment=x\n24 linkpath=safe/target\n', + 'ascii' + ) + ), + header({name: 'cache/link', typeflag: '2', linkname: 'safe/target'}), + 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([ + paxEntry( + Buffer.concat([ + Buffer.from(`1048600 comment=${'A'.repeat(1048600 - 17)}\n`, 'ascii'), + Buffer.from('42 path=../../../../../../tmp/zip_slip_F3\n', 'ascii') + ]) + ), + fileEntry('cache/safe.txt', 'F3 pwned'), + end() +]) + +// F5 — sparse typeflag 'S' is mapped but ignored by node-tar's ReadEntry. +const F5 = Buffer.concat([ + fileEntry('cache/decoy.txt', 'ok'), + fileEntry('../../../../../../tmp/zip_slip_F5', '', 'S'), + end() +]) + +// --------------------------------------------------------------------------- +// Test harness +// --------------------------------------------------------------------------- + +const ROOT = mkdtempSync(path.join(os.tmpdir(), 'cache-attacks-')) + +function workspace(): string { + return path.join(ROOT, 'workspace') +} + +function writeGz(name: string, archive: Buffer): string { + mkdirSync(ROOT, {recursive: true}) + const p = path.join(ROOT, name) + writeFileSync(p, gzipSync(archive)) + return p +} + +async function validate( + archive: Buffer, + name: string +): Promise<{violations: string[]; approvedNames: string[]}> { + const p = writeGz(name, archive) + const result = await listAndValidate( + p, + CompressionMethod.Gzip, + [path.join(workspace(), 'cache')], + workspace() + ) + return { + violations: result.violations.map(v => v.code), + approvedNames: result.approvedNames + } +} + +const TAR_AVAILABLE = ((): boolean => { + try { + execSync(process.platform === 'win32' ? 'where tar' : 'which tar', { + stdio: 'ignore' + }) + return true + } catch { + return false + } +})() +const describeTar = TAR_AVAILABLE ? describe : describe.skip + +beforeAll(() => { + mkdirSync(workspace(), {recursive: true}) +}) + +afterAll(() => { + try { + rmSync(ROOT, {recursive: true, force: true}) + } catch { + // best-effort + } +}) + +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') + expect(violations).toContain('UNSUPPORTED_TYPE') + // The escaping entry must NOT be approved for extraction. + expect(approvedNames).not.toContain('../../../../../../tmp/zip_slip_F1') + }) + + test('F2: PAX path newline differential is rejected as PAX_DESYNC', async () => { + const {violations, approvedNames} = await validate(F2, 'f2.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') + expect(violations).toContain('PAX_DESYNC') + }) + + test('F3: oversized PAX header is rejected as UNSUPPORTED_TYPE', async () => { + const {violations} = await validate(F3, 'f3.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') + expect(violations).toContain('UNSUPPORTED_TYPE') + expect(approvedNames).not.toContain('../../../../../../tmp/zip_slip_F5') + }) + + test('glob metacharacter in entry path is rejected as GLOB_METACHAR', async () => { + const archive = Buffer.concat([fileEntry('cache/[id].js', 'x'), end()]) + const {violations} = await validate(archive, 'glob.tar.gz') + expect(violations).toContain('GLOB_METACHAR') + }) + + test('clean archive: approvedNames lists every concrete entry, no violations', async () => { + const archive = Buffer.concat([ + dirEntry('cache/'), + fileEntry('cache/file.txt', 'hi'), + dirEntry('cache/sub/'), + fileEntry('cache/sub/deep.txt', 'deep'), + end() + ]) + const {violations, approvedNames} = await validate(archive, 'clean.tar.gz') + expect(violations).toEqual([]) + expect(approvedNames).toEqual([ + 'cache/', + 'cache/file.txt', + 'cache/sub/', + 'cache/sub/deep.txt' + ]) + }) + + test('newline in entry path is rejected as UNSAFE_CHAR', async () => { + const archive = Buffer.concat([fileEntry('cache/a\nb.txt', 'x'), end()]) + const {violations} = await validate(archive, 'newline.tar.gz') + expect(violations).toContain('UNSAFE_CHAR') + }) + + test('NUL byte in a symlink target (via PAX) is rejected as UNSAFE_CHAR', 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') + }) + + test('legitimate long path via PAX: no violations, approved by its PAX path', async () => { + const longName = `cache/${'d/'.repeat(60)}file.txt` + const archive = Buffer.concat([ + paxEntry(Buffer.from(paxRecord(`path=${longName}`), 'ascii')), + // ustar name is a short placeholder; the PAX `path` overrides it. + fileEntry('cache/placeholder', 'x'), + end() + ]) + const {violations, approvedNames} = await validate( + archive, + 'longpath.tar.gz' + ) + expect(violations).toEqual([]) + expect(approvedNames).toContain(longName) + }) + + test('unknown PAX key is rejected as PAX_UNKNOWN_KEY', async () => { + const archive = Buffer.concat([ + paxEntry(Buffer.from(paxRecord('EVIL.placement=1'), 'ascii')), + fileEntry('cache/x', 'y'), + end() + ]) + const {violations} = await validate(archive, 'unknown-key.tar.gz') + expect(violations).toContain('PAX_UNKNOWN_KEY') + }) + + test('flood of extended headers is rejected (pending-meta cap)', async () => { + const metas: Buffer[] = [] + for (let i = 0; i < 70; i++) { + metas.push(paxEntry(Buffer.from(paxRecord('comment=x'), 'ascii'))) + } + const archive = Buffer.concat([...metas, fileEntry('cache/x', 'y'), end()]) + const p = writeGz('meta-flood.tar.gz', archive) + await expect( + listAndValidate( + p, + CompressionMethod.Gzip, + [path.join(workspace(), 'cache')], + workspace() + ) + ).rejects.toThrow() + }) +}) + +describeTar('extractTar end-to-end with system tar allow-list', () => { + let savedWorkspace: string | undefined + + beforeEach(() => { + savedWorkspace = process.env['GITHUB_WORKSPACE'] + }) + + afterEach(() => { + if (savedWorkspace === undefined) { + delete process.env['GITHUB_WORKSPACE'] + } else { + process.env['GITHUB_WORKSPACE'] = savedWorkspace + } + }) + + test('error mode, clean archive: every approved member is extracted', async () => { + const dest = mkdtempSync(path.join(ROOT, 'extract-clean-')) + process.env['GITHUB_WORKSPACE'] = dest + const archive = Buffer.concat([ + dirEntry('cache/'), + fileEntry('cache/file.txt', 'hello'), + dirEntry('cache/sub/'), + fileEntry('cache/sub/deep.txt', 'deep'), + end() + ]) + const archivePath = path.join(dest, 'clean.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', 'file.txt'))).toBe(true) + expect(readFileSync(path.join(dest, 'cache', 'file.txt'), 'utf8')).toBe( + 'hello' + ) + 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-')) + process.env['GITHUB_WORKSPACE'] = dest + const archivePath = path.join(dest, 'f2.tar.gz') + mkdirSync(dest, {recursive: true}) + writeFileSync(archivePath, gzipSync(F2)) + + await expect( + extractTar(archivePath, CompressionMethod.Gzip, { + declaredPaths: ['cache/**'], + pathValidation: 'error' + }) + ).rejects.toThrow(CacheIntegrityError) + + // No member was extracted anywhere under the workspace. + expect(existsSync(path.join(dest, 'cache'))).toBe(false) + expect(existsSync(path.join(dest, 'safe.txt'))).toBe(false) + }) +}) diff --git a/packages/cache/src/internal/listAndValidate.ts b/packages/cache/src/internal/listAndValidate.ts index da3b9ecc..40a0e063 100644 --- a/packages/cache/src/internal/listAndValidate.ts +++ b/packages/cache/src/internal/listAndValidate.ts @@ -9,12 +9,48 @@ import { prepareAllowedRoots, validateEntry } from './pathValidation.js' +import {crossCheckMetaBodies} from './pax-reparse.js' + +/** + * Result of streaming and validating a tar archive. + */ +export interface ListAndValidateResult { + /** Entries that failed validation. Empty iff the archive is clean. */ + violations: PathValidationViolation[] + /** + * The `entry.path` of every entry that passed validation, in archive order. + * Used to build the NUL-separated allow-list (`-T`) handed to system `tar` + * so extraction is restricted to members the validator approved — by the + * exact name node-tar derived from the same bytes. Only meaningful when + * `violations` is empty (otherwise extraction is blocked or unrestricted). + */ + approvedNames: string[] +} + +/** + * 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 + * `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. + */ +const META_REJECT_BYTES = 1024 * 1024 + +/** + * Maximum number of pending extended-header (meta) bodies to retain while + * waiting for the concrete entry they apply to. A legitimate entry is + * preceded by at most a handful of meta headers; an archive that streams an + * unbounded run of meta headers with no concrete entry is malformed and must + * not be allowed to grow memory without bound. + */ +const MAX_PENDING_META = 64 /** * Stream the entries of a (possibly compressed) tar archive and validate each * against the allowed roots. Does NOT extract any files — entries are read * for header inspection only. Returns the list of violations (empty if the - * archive is clean). + * archive is clean) and the names approved for extraction. * * Throws an Error if the archive cannot be parsed (corrupt header, * decompression failure, truncated stream, etc.). The caller is responsible @@ -25,8 +61,9 @@ export async function listAndValidate( compressionMethod: CompressionMethod, allowedRoots: string[], extractCwd: string -): Promise { +): Promise { const violations: PathValidationViolation[] = [] + const approvedNames: string[] = [] // Precompute the normalized / case-folded form of each allowed root once, // so the per-entry containment check is a handful of string compares @@ -45,6 +82,20 @@ export async function listAndValidate( firstParseError = new Error(`tar parse error (${code}): ${message}`) } + // Raw bodies of the extended-header (meta) entries seen since the last + // concrete entry. node-tar emits a `'meta'` event carrying the decoded + // 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. + let pendingMeta: Buffer[] = [] + + const consumePendingMeta = (): Buffer[] => { + const bodies = pendingMeta + pendingMeta = [] + return bodies + } + // 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 @@ -57,6 +108,10 @@ export async function listAndValidate( // headers) don't abort parsing. Real corruption is surfaced explicitly // 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). + maxMetaEntrySize: META_REJECT_BYTES, // Treat structural problems (bad archive, bad header, bad chksum) as // hard parse errors — silently ignoring them would let a corrupt // archive sail through validation. We DO NOT throw on softer warnings @@ -73,6 +128,32 @@ export async function listAndValidate( }, onReadEntry: (entry: ReadEntry) => { try { + const metaBodies = consumePendingMeta() + + // Cross-check any PAX / long-name headers that preceded this entry + // against node-tar's resolved view. A disagreement means node-tar + // mis-parsed the header relative to what GNU/BSD tar will extract. + for (const pax of crossCheckMetaBodies( + metaBodies, + 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 + }) + } + + // 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) + const result = validateEntry( entry.path, entry.linkpath || undefined, @@ -90,6 +171,25 @@ export async function listAndValidate( reason: result.reason }) } + + 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) { + approvedNames.push(entry.path) + } } finally { // Drain the entry so the parser advances. Without this the stream // stalls waiting for the consumer to read the entry body. @@ -98,12 +198,89 @@ export async function listAndValidate( } }) + // Entries node-tar refuses to classify (unknown typeflag bytes, mapped + // typeflags it doesn't extract, and oversized meta headers) are emitted on + // `'ignoredEntry'` rather than `'entry'`, so they never reach onReadEntry. + // A cache archive should never legitimately contain one, and system tar may + // still extract them — so we fail closed and record each as a violation. + parser.on('ignoredEntry', (entry: ReadEntry) => { + // 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}` + }) + }) + + // Capture the raw body of each extended-header (meta) entry. node-tar + // decodes the body to a string before emitting it; we re-encode to bytes so + // the length-correct PAX re-parser operates on the same view node-tar used. + parser.on('meta', (metaBody: string) => { + if (pendingMeta.length >= MAX_PENDING_META) { + recordParseError( + 'TAR_ENTRY_INVALID', + 'too many consecutive extended headers' + ) + return + } + pendingMeta.push(Buffer.from(metaBody, 'utf8')) + }) + await streamArchiveTo(archivePath, compressionMethod, parser) if (firstParseError) { throw firstParseError } - return violations + 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( diff --git a/packages/cache/src/internal/pathValidation.ts b/packages/cache/src/internal/pathValidation.ts index e268de43..ab2d531f 100644 --- a/packages/cache/src/internal/pathValidation.ts +++ b/packages/cache/src/internal/pathValidation.ts @@ -27,6 +27,11 @@ export interface PathValidationViolation { | 'OUTSIDE_ROOTS' | 'LINK_OUTSIDE_ROOTS' | 'UNSUPPORTED_TYPE' + | 'PAX_DESYNC' + | 'PAX_PARSE_FAIL' + | 'PAX_UNKNOWN_KEY' + | 'UNSAFE_CHAR' + | 'GLOB_METACHAR' /** Human-readable description of the violation. */ reason: string } diff --git a/packages/cache/src/internal/pax-reparse.ts b/packages/cache/src/internal/pax-reparse.ts new file mode 100644 index 00000000..44f87e4c --- /dev/null +++ b/packages/cache/src/internal/pax-reparse.ts @@ -0,0 +1,271 @@ +/** + * Length-correct PAX extended-header re-parser. + * + * node-tar v7 parses PAX extended-header bodies with a naive `split('\n')` + * (see `pax.js`'s `parseKV`, whose own source carries an `XXX Values with \n + * in them will fail this` comment). A PAX record is actually a length-prefixed + * sequence: + * + * " =\n" + * + * where `` is the total byte length of the record including + * the digits, the space, the `=`, the value, and the trailing `\n`. Values may + * 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. + * + * This module re-parses each captured PAX body the way every real extractor + * does (length-prefixed, byte-accurate) and cross-checks the result against + * node-tar's view of the entry. Any disagreement on `path` or `linkpath` is a + * `PAX_DESYNC` violation; a body node-tar treated as PAX that this parser + * cannot fully account for is a `PAX_PARSE_FAIL`; and any PAX key not on the + * known allow-list is a `PAX_UNKNOWN_KEY` (fail-closed against future + * placement-affecting extensions). + * + * Everything here is pure (no I/O, no node-tar dependency) so it can be unit + * tested in isolation. + */ + +/** Machine-readable reason codes produced by the PAX cross-check. */ +export type PaxReparseCode = 'PAX_DESYNC' | 'PAX_PARSE_FAIL' | 'PAX_UNKNOWN_KEY' + +/** A single disagreement surfaced by {@link crossCheckMetaBodies}. */ +export interface PaxReparseViolation { + code: PaxReparseCode + reason: string +} + +/** + * PAX keys that are known-legitimate and do not influence where extracted + * bytes land (or, for `path` / `linkpath`, are explicitly cross-checked). + * Anything outside this set — and the prefix set below — is rejected so a + * future placement-affecting PAX extension cannot silently slip past the + * validator. + * + * Sourced from POSIX.1-2017 §3.4.3 plus the GNU tar (`xheader.c`) and + * libarchive (`archive_read_format_tar.c`) extension vocabularies. + */ +export const PAX_KNOWN_KEYS: ReadonlySet = new Set([ + // POSIX.1-2017 §3.4.3 + 'atime', + 'ctime', + 'mtime', + 'charset', + 'comment', + 'gid', + 'gname', + 'hdrcharset', + 'linkpath', + 'path', + 'size', + 'uid', + 'uname', + // SCHILY / star vocabulary that node-tar itself emits and consumes + 'dev', + 'ino', + 'nlink', + 'mode' +]) + +/** + * Dotted-namespace PAX key prefixes that are known-legitimate. Matched as + * prefixes so e.g. `SCHILY.xattr.user.foo`, `GNU.sparse.realsize`, and + * `LIBARCHIVE.creationtime` are all accepted without enumerating every + * possible suffix. + */ +export const PAX_KNOWN_PREFIXES: readonly string[] = [ + 'SCHILY.', + 'GNU.', + 'LIBARCHIVE.', + 'POSIX.' +] + +function isKnownPaxKey(key: string): boolean { + if (PAX_KNOWN_KEYS.has(key)) return true + return PAX_KNOWN_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 + * raw bytes so non-ASCII / high-bit data is preserved until comparison. */ + records: Record + /** True iff every byte of the body was accounted for by length-prefixed + * records. False signals a body this parser could not fully account for. */ + ok: boolean +} + +const DIGIT_0 = 0x30 +const DIGIT_9 = 0x39 +const SPACE = 0x20 +const EQUALS = 0x3d +const LF = 0x0a + +/** + * Parse a PAX extended-header body using strict, byte-accurate, + * length-prefixed records — the same algorithm GNU tar and libarchive use. + * + * Returns `{ok: false}` (with whatever records were parsed up to the failure) + * the moment a record is not fully length-accountable: a non-digit length, a + * length that overruns the buffer, a missing trailing `\n`, or a record with + * no `=`. + */ +export function parsePaxLengthCorrect(buf: Buffer): PaxParseResult { + const records: Record = {} + let i = 0 + while (i < buf.length) { + // — a run of ASCII digits terminated by a single space. + let j = i + while (j < buf.length && buf[j] >= DIGIT_0 && buf[j] <= DIGIT_9) j++ + if (j === i || j >= buf.length || buf[j] !== SPACE) { + return {records, ok: false} + } + const len = parseInt(buf.subarray(i, j).toString('ascii'), 10) + // Length must be positive and the whole record must fit in the buffer. + if (!Number.isFinite(len) || len <= 0 || i + len > buf.length) { + return {records, ok: false} + } + // The record must end with a newline at exactly the length boundary. + if (buf[i + len - 1] !== LF) { + return {records, ok: false} + } + // " =\n" — the first '=' after the space separates + // key from value. The value runs to just before the trailing newline and + // may itself contain '=' and '\n'. + const eq = buf.indexOf(EQUALS, j + 1) + if (eq < 0 || eq >= i + len - 1) { + return {records, ok: false} + } + const key = buf.subarray(j + 1, eq).toString('utf8') + const value = buf.subarray(eq + 1, i + len - 1) // raw bytes, no trailing LF + records[key] = value // last write wins + i += len + } + return {records, ok: i === buf.length} +} + +/** + * Normalise a path for cross-parser comparison: collapse `\` to `/` (matching + * node-tar's `normalizeWindowsPath` on Windows; a no-op for typical POSIX + * paths) and drop a single trailing `/` so directory entries compare equal + * regardless of how each side renders them. Pure string manipulation. + */ +function normalizeForCompare(p: string): string { + let s = p.replace(/\\/g, '/') + if (s.length > 1 && s.endsWith('/')) { + s = s.slice(0, -1) + } + return s +} + +function nulTrim(buf: Buffer): Buffer { + const nul = buf.indexOf(0) + return nul === -1 ? buf : buf.subarray(0, nul) +} + +/** + * Cross-check the captured extended-header (meta) bodies that preceded a + * concrete entry against node-tar's resolved view of that entry. + * + * `metaBodies` are the raw bodies of every `ExtendedHeader` / + * `GlobalExtendedHeader` / GNU `LongName` / `LongLink` meta entry emitted + * since the previous concrete entry, in order. `entryPath` / `entryLinkpath` + * are node-tar's final (post-PAX, post-long-name) values for the entry. + * + * The check is sound without ever invoking the system `tar`: this re-parser + * computes the same `path` / `linkpath` every real extractor would, so a + * disagreement with node-tar's value is exactly the signal that node-tar + * mis-parsed the header. + */ +export function crossCheckMetaBodies( + metaBodies: Buffer[], + entryPath: string, + entryLinkpath: string | undefined +): PaxReparseViolation[] { + const violations: PaxReparseViolation[] = [] + let sawPax = false + // Merged length-correct view of path / linkpath across all PAX bodies + // (last write wins), matching POSIX precedence. + let lcPath: Buffer | undefined + let lcLinkpath: Buffer | undefined + + for (const body of metaBodies) { + if (body.length === 0) continue + const parsed = parsePaxLengthCorrect(body) + const keys = Object.keys(parsed.records) + + if (parsed.ok && keys.length > 0) { + // A well-formed PAX extended header. + sawPax = true + for (const key of keys) { + if (!isKnownPaxKey(key)) { + violations.push({ + code: 'PAX_UNKNOWN_KEY', + reason: `unknown PAX key '${key}' in extended header` + }) + } + } + if (parsed.records['path'] !== undefined) { + lcPath = parsed.records['path'] + } + if (parsed.records['linkpath'] !== undefined) { + lcLinkpath = parsed.records['linkpath'] + } + } else { + // Not a length-accountable PAX body. This is either a GNU `LongName` / + // `LongLink` body (raw NUL-terminated path, no length prefix) or a + // malformed PAX header crafted to desync node-tar. A legitimate GNU + // long-name body's NUL-trimmed bytes must equal exactly the path or + // linkpath node-tar resolved for the entry; anything else is a parser + // disagreement we refuse to extract. + const raw = nulTrim(body).toString('utf8') + const rawCmp = normalizeForCompare(raw) + const matchesPath = rawCmp === normalizeForCompare(entryPath) + const matchesLink = + entryLinkpath !== undefined && + rawCmp === normalizeForCompare(entryLinkpath) + if (!matchesPath && !matchesLink) { + violations.push({ + code: 'PAX_PARSE_FAIL', + reason: + 'extended header body is not length-accountable and does not ' + + "match the entry's resolved path or linkpath" + }) + } + } + } + + if (sawPax) { + if (lcPath !== undefined) { + const lc = normalizeForCompare(lcPath.toString('utf8')) + if (lc !== normalizeForCompare(entryPath)) { + violations.push({ + code: 'PAX_DESYNC', + reason: `PAX path disagreement: node-tar resolved ${JSON.stringify( + entryPath + )} but length-correct parse yields ${JSON.stringify( + lcPath.toString('utf8') + )}` + }) + } + } + if (lcLinkpath !== undefined) { + const lc = normalizeForCompare(lcLinkpath.toString('utf8')) + const ntLink = entryLinkpath ?? '' + if (lc !== normalizeForCompare(ntLink)) { + violations.push({ + code: 'PAX_DESYNC', + reason: `PAX linkpath disagreement: node-tar resolved ${JSON.stringify( + entryLinkpath + )} but length-correct parse yields ${JSON.stringify( + lcLinkpath.toString('utf8') + )}` + }) + } + } + } + + return violations +} diff --git a/packages/cache/src/internal/tar.ts b/packages/cache/src/internal/tar.ts index 33cd29f1..14999489 100644 --- a/packages/cache/src/internal/tar.ts +++ b/packages/cache/src/internal/tar.ts @@ -1,7 +1,9 @@ import {exec} from '@actions/exec' import * as core from '@actions/core' import * as io from '@actions/io' -import {existsSync, writeFileSync} from 'fs' +import {existsSync, writeFileSync, unlinkSync} from 'fs' +import * as os from 'os' +import * as crypto from 'crypto' import * as path from 'path' import * as utils from './cacheUtils.js' import {ArchiveTool} from './contracts.js' @@ -64,7 +66,8 @@ async function getTarArgs( tarPath: ArchiveTool, compressionMethod: CompressionMethod, type: string, - archivePath = '' + archivePath = '', + allowListPath = '' ): Promise { const args = [`"${tarPath.path}"`] const cacheFileName = utils.getCacheFileName(compressionMethod) @@ -106,6 +109,34 @@ async function getTarArgs( '-C', workingDirectory.replace(new RegExp(`\\${path.sep}`, 'g'), '/') ) + // When an extraction allow-list is supplied (pathValidation: 'error' + // with a clean archive), restrict extraction to exactly the members the + // validator approved, matched by the names node-tar derived from the + // same bytes. This makes the extraction parser's view of any + // path-channel parser differential a no-op: a member system tar would + // place at a different path than node-tar simply isn't on the list. + // + // `--null` MUST precede `-T` on GNU tar (otherwise the list is read + // newline-delimited with stray NULs). `--no-recursion` stops an + // approved directory from implicitly pulling in unapproved children. + // The `--no-wildcards*` / `--anchored` flags are GNU-only defence in + // depth; bsdtar lacks them but glob metacharacters are already rejected + // during validation, so its `fnmatch()`-based `-T` matching degrades to + // exact-name matching. + if (allowListPath) { + args.push('--null', '--no-recursion') + if (tarPath.type === ArchiveToolType.GNU) { + args.push( + '--no-wildcards', + '--no-wildcards-match-slash', + '--anchored' + ) + } + args.push( + '-T', + `"${allowListPath.replace(new RegExp(`\\${path.sep}`, 'g'), '/')}"` + ) + } break case 'list': args.push( @@ -137,7 +168,8 @@ async function getTarArgs( async function getCommands( compressionMethod: CompressionMethod, type: string, - archivePath = '' + archivePath = '', + allowListPath = '' ): Promise { let args @@ -146,7 +178,8 @@ async function getCommands( tarPath, compressionMethod, type, - archivePath + archivePath, + allowListPath ) const compressionArgs = type !== 'create' @@ -290,6 +323,11 @@ export async function extractTar( const workingDirectory = getWorkingDirectory() const pathValidation: PathValidationMode = options?.pathValidation ?? 'off' + // Names approved for extraction by the validator. When pathValidation is + // 'error' and the archive is clean, system tar is restricted to exactly + // these members via a NUL-separated `-T` allow-list (see below). + let approvedNames: string[] | undefined + // Run path validation BEFORE creating the extraction directory or invoking // system tar. In 'error' mode, a CacheIntegrityError thrown here means no // bytes are ever written to the workspace. In 'warn' mode, violations are @@ -306,12 +344,14 @@ export async function extractTar( } let violations: PathValidationViolation[] | undefined try { - violations = await listAndValidate( + const result = await listAndValidate( archivePath, compressionMethod, allowedRoots, workingDirectory ) + violations = result.violations + approvedNames = result.approvedNames } catch (error) { // Parse / decompression failure encountered while validating. The // validator's tar parser is stricter than the system `tar` that @@ -344,13 +384,61 @@ export async function extractTar( violations ) } + // In 'warn' mode a violation means we must NOT restrict extraction to + // the (possibly incomplete) approved list — fall back to extracting + // everything, matching legacy behavior. + approvedNames = undefined } } // Create directory to extract tar into await io.mkdirP(workingDirectory) - const commands = await getCommands(compressionMethod, 'extract', archivePath) - await execCommands(commands) + + // In 'error' mode with a clean archive, write the approved member names to a + // NUL-separated allow-list and restrict system tar to exactly those members. + // This closes path-channel parser differentials: a member tar would extract + // to an escaped path is not on the list, so it is never extracted. + let allowListPath = '' + if (pathValidation === 'error' && approvedNames !== undefined) { + allowListPath = writeAllowList(approvedNames) + } + + try { + const commands = await getCommands( + compressionMethod, + 'extract', + archivePath, + allowListPath + ) + await execCommands(commands) + } finally { + if (allowListPath) { + try { + unlinkSync(allowListPath) + } catch { + // best-effort cleanup of the temporary allow-list file + } + } + } +} + +/** + * Write the approved member names to a temporary NUL-separated file suitable + * for `tar --null -T`. Returns the absolute path to the written file. The + * caller is responsible for unlinking it. + */ +function writeAllowList(approvedNames: string[]): string { + const allowListPath = path.join( + os.tmpdir(), + `cache-allow-${process.pid}-${Date.now()}-${crypto + .randomBytes(4) + .toString('hex')}.lst` + ) + const payload = Buffer.concat( + approvedNames.flatMap(name => [Buffer.from(name, 'utf8'), Buffer.from([0])]) + ) + writeFileSync(allowListPath, payload, {mode: 0o600}) + return allowListPath } function reportViolations(