Harden artifact extraction symlink checks #7

Merged

stephen merged 1 commit from feat/2-safe-extract into main

2026-06-18 17:47:58 +00:00

stephen commented

2026-06-18 16:50:16 +00:00

Owner

Why:

extraction validation must not trust an existing destination tree

Changes:

reject archive entries that would traverse a pre-existing destination symlink outside the target directory
reject hardlink targets reached through escaping destination symlinks
extract with no owner restoration
add malicious tar tests for both escape paths

Testing:

npm test
npm run check
git diff --check

Why: - extraction validation must not trust an existing destination tree Changes: - reject archive entries that would traverse a pre-existing destination symlink outside the target directory - reject hardlink targets reached through escaping destination symlinks - extract with no owner restoration - add malicious tar tests for both escape paths Testing: - npm test - npm run check - git diff --check

stephen added 1 commit

2026-06-18 16:50:16 +00:00

Harden artifact extraction symlink checks

test / unit (pull_request) Successful in 23s

Details

test / e2e (pull_request) Successful in 23s

Details

eaacc3895b

stephen commented

2026-06-18 17:07:44 +00:00

Author

Owner

APPROVE

Verified the extraction validator against the full threat model. All four classes are rejected before a single byte is written (validation walks the headers, then tar -xf runs only if clean), and normal extraction is preserved. Ran the suite locally: 28/28 green.

(a) ../ traversal — validateEntry resolves name against destReal and rejects anything failing isInside (artifact-tar.js:237-238). Covered by tests/artifact-tar.test.js:75.
(b) absolute-path entries — path.isAbsolute(name) rejected at :236; absolute link targets rejected at :249. Covered by :88.
(c) symlinks/hardlinks escaping dest — two distinct vectors, both closed:
- In-archive escaping link target: anchor-resolve + isInside at :250-252 (:96).
- Pre-existing destination symlink (the gap this PR closes): existingPathStaysInside (:205-227) walks each path component, lstats it, and if a component is a symlink whose realpathSync lands outside destReal, refuses the archive. It is applied to every entry's own path (:239, so it catches a plain file written through a planted linked -> /outside dir) and to link targets (:253). realpathSync fully resolves chains, so nested/indirect symlinks are caught too. Covered by :103 (file through escaping symlink) and :113 (hardlink through escaping symlink).
(d) special files — char/block device (3/4), fifo (6), and any unknown typeflag fall through to the else and are refused (:257-259). Fifo covered by :124.

Also correct: --no-same-owner on extract (:369) drops archive uid/gid restoration, which matters on a root runner. Fail-closed behavior is sound: nothing is written on any rejection (the no-partial-write assertions at :84-85 etc. confirm it). Normal extraction unaffected, since existingPathStaysInside short-circuits on ENOENT for a fresh/empty dest (:215); the happy-path tests at :58 and :130 pass.

Non-blocking nits (not gating):

fs.realpathSync(cur) (:219) sits outside the try/catch that maps ENOENT (which only wraps the lstatSync at :213). A dangling escaping symlink would surface a raw ENOENT instead of UnsafeArchiveError. Still fails closed (extraction aborts, nothing written), so it is cosmetic, but wrapping it would keep the error type consistent.
Only fifo (6) has a dedicated special-file test; device types 3/4 hit the identical branch but aren't asserted directly. Optional to add.
The validate-then-tar -xf design retains an inherent TOCTOU window (an attacker with concurrent write access to dest during extraction); out of scope here, and this PR narrows it by rejecting symlinks already present at validation time.

Clean approve. Not merging - over to you.

**APPROVE** Verified the extraction validator against the full threat model. All four classes are rejected before a single byte is written (validation walks the headers, then `tar -xf` runs only if clean), and normal extraction is preserved. Ran the suite locally: 28/28 green. - **(a) `../` traversal** — `validateEntry` resolves `name` against `destReal` and rejects anything failing `isInside` (`artifact-tar.js:237-238`). Covered by `tests/artifact-tar.test.js:75`. - **(b) absolute-path entries** — `path.isAbsolute(name)` rejected at `:236`; absolute link targets rejected at `:249`. Covered by `:88`. - **(c) symlinks/hardlinks escaping dest** — two distinct vectors, both closed: - *In-archive* escaping link target: anchor-resolve + `isInside` at `:250-252` (`:96`). - *Pre-existing destination symlink* (the gap this PR closes): `existingPathStaysInside` (`:205-227`) walks each path component, `lstat`s it, and if a component is a symlink whose `realpathSync` lands outside `destReal`, refuses the archive. It is applied to every entry's own path (`:239`, so it catches a plain file written *through* a planted `linked -> /outside` dir) and to link targets (`:253`). `realpathSync` fully resolves chains, so nested/indirect symlinks are caught too. Covered by `:103` (file through escaping symlink) and `:113` (hardlink through escaping symlink). - **(d) special files** — char/block device (`3`/`4`), fifo (`6`), and any unknown typeflag fall through to the `else` and are refused (`:257-259`). Fifo covered by `:124`. Also correct: `--no-same-owner` on extract (`:369`) drops archive uid/gid restoration, which matters on a root runner. Fail-closed behavior is sound: nothing is written on any rejection (the no-partial-write assertions at `:84-85` etc. confirm it). Normal extraction unaffected, since `existingPathStaysInside` short-circuits on `ENOENT` for a fresh/empty dest (`:215`); the happy-path tests at `:58` and `:130` pass. Non-blocking nits (not gating): 1. `fs.realpathSync(cur)` (`:219`) sits outside the `try/catch` that maps `ENOENT` (which only wraps the `lstatSync` at `:213`). A *dangling* escaping symlink would surface a raw `ENOENT` instead of `UnsafeArchiveError`. Still fails closed (extraction aborts, nothing written), so it is cosmetic, but wrapping it would keep the error type consistent. 2. Only fifo (`6`) has a dedicated special-file test; device types `3`/`4` hit the identical branch but aren't asserted directly. Optional to add. 3. The validate-then-`tar -xf` design retains an inherent TOCTOU window (an attacker with concurrent write access to `dest` during extraction); out of scope here, and this PR narrows it by rejecting symlinks already present at validation time. Clean approve. Not merging - over to you.

stephen merged commit b5bbd54b5d into main

2026-06-18 17:47:58 +00:00

stephen referenced this pull request from a commit

2026-06-18 17:47:59 +00:00

Merge pull request 'Harden artifact extraction symlink checks' (#7) from feat/2-safe-extract into main