Cap extraction size/count and strip setuid bits (#2) #5

Merged

stephen merged 1 commit from feat/2-extraction-caps-setuid into main

2026-06-17 18:26:57 +00:00

stephen commented

2026-06-17 18:21:30 +00:00

Owner

What changed
Entry validation already refused traversal, escaping links, and special files, but two gaps in safe extraction remained, so #2 stayed open. A decompression bomb could still expand into a destination-filling tree, and a setuid/setgid binary inside an artifact would be written through verbatim on a runner that extracts as root.

Cap extraction before any byte is written: assertSafeTar bounds the summed uncompressed size and the header count (defaults 10 GiB and 100000 entries), overridable via RASTER_ARTIFACTS_MAX_TOTAL_BYTES and RASTER_ARTIFACTS_MAX_ENTRIES (0 disables a cap). An archive over a cap is refused and nothing is written.
Strip setuid/setgid bits from extracted files and directories. assertSafeTar reports the entries carrying them; extractArchive clears the bits after extraction, since tar's own handling varies by version, umask, and privilege.

How it was tested

make lint clean (actionlint/yq not installed locally; skipped)
make test (offline suite) green, 26 tests
new behavior has a test: over-cap archive refused (by size and by count), a disabled cap lets a large archive through, and setuid/setgid bits are detected by assertSafeTar and cleared by stripSetid
e2e workflow green on a runner (local-only change to extraction; no backend change)

CHANGELOG updated? yes

Closes #2.

**What changed** Entry validation already refused traversal, escaping links, and special files, but two gaps in safe extraction remained, so #2 stayed open. A decompression bomb could still expand into a destination-filling tree, and a setuid/setgid binary inside an artifact would be written through verbatim on a runner that extracts as root. - Cap extraction before any byte is written: `assertSafeTar` bounds the summed uncompressed size and the header count (defaults 10 GiB and 100000 entries), overridable via `RASTER_ARTIFACTS_MAX_TOTAL_BYTES` and `RASTER_ARTIFACTS_MAX_ENTRIES` (0 disables a cap). An archive over a cap is refused and nothing is written. - Strip setuid/setgid bits from extracted files and directories. `assertSafeTar` reports the entries carrying them; `extractArchive` clears the bits after extraction, since tar's own handling varies by version, umask, and privilege. **How it was tested** - [x] `make lint` clean (actionlint/yq not installed locally; skipped) - [x] `make test` (offline suite) green, 26 tests - [x] new behavior has a test: over-cap archive refused (by size and by count), a disabled cap lets a large archive through, and setuid/setgid bits are detected by `assertSafeTar` and cleared by `stripSetid` - [ ] e2e workflow green on a runner (local-only change to extraction; no backend change) **CHANGELOG updated?** yes Closes #2.

stephen added 1 commit

2026-06-17 18:21:30 +00:00

Cap extraction size/count and strip setuid bits (#2 )

test / unit (pull_request) Successful in 5s

Details

test / e2e (pull_request) Successful in 4s

Details

77e7e22e28

Entry validation already refused traversal, escaping links, and special
files, but two gaps remained in safe extraction: a decompression bomb
could still expand into a destination-filling tree, and a setuid/setgid
binary in an artifact would be written through verbatim on a runner that
extracts as root.

assertSafeTar now bounds the summed uncompressed size and the header
count before any byte is written (defaults 10 GiB and 100000 entries,
overridable via RASTER_ARTIFACTS_MAX_TOTAL_BYTES and
RASTER_ARTIFACTS_MAX_ENTRIES; 0 disables a cap), refusing the whole
archive on overflow. It also reports the entries carrying setuid/setgid
bits, which extractArchive strips after extraction since tar's own
handling of those bits varies by version, umask, and privilege.

Tests cover an over-cap archive (by size and by count) being refused, a
disabled cap letting a large archive through, and setuid/setgid bits
being detected and cleared.

stephen merged commit 58d7775938 into main

2026-06-17 18:26:57 +00:00

stephen referenced this pull request from a commit

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

Merge pull request 'Cap extraction size/count and strip setuid bits (#2)' (#5) from feat/2-extraction-caps-setuid into main

stephen reviewed

2026-06-17 18:29:24 +00:00

stephen left a comment

VERDICT: APPROVE (posted as a comment; the review API blocks a formal approve from the PR-author identity this CI tooling runs as. Over to you for the non-author approve + merge.)

Reviewed feat/2-extraction-caps-setuid @ 77e7e22. CI green (unit + e2e). make test green locally (26 tests). Diff is confined to src/lib/artifact-tar.js + tests + CHANGELOG/README.

What I verified

Size/count caps refuse before any write to the destination. Confirmed with real tar-built archives, not just the test harness:
- Over-count: a 5-file archive with RASTER_ARTIFACTS_MAX_ENTRIES=3 is refused and the destination stays empty.
- Over-size / bomb ratio: a 1 MiB-declared payload compressing to ~1.1 KiB, with RASTER_ARTIFACTS_MAX_TOTAL_BYTES=64KiB, is refused (uncompressed size exceeds cap) with nothing written to dest.
- Benign gzip artifact still extracts and round-trips (nested + top-level files).
- The caps run inside assertSafeTar before tar -xf, and size is summed from each header's declared size, which is exactly what tar would write. 0/non-positive disables a cap (documented, tested).
setuid/setgid handling. Built a real 0o4755 file, archived and extracted: assertSafeTar flags it via setidPaths, stripSetid clears 0o6000 after extraction, ordinary perms (owner-exec) and contents are preserved. The strip is unconditional post-extraction (re-stat + chmod), so it also covers a root runner where tar would keep the bits. Operator precedence in parseMode(block) & SETID_BITS is correct (&& binds looser than &).
No regression to the merged work. Digest verification (#1) lives in download-impl.js (verifyDigest runs before tar.extractArchive) and is not touched by this diff; the extractArchive(archive, dest, method) public signature is unchanged (caps are an internal default param). The #2-core entry validation (absolute/../escaping link/special file) is preserved in validateEntry; only its return value changed (null vs resolved path), and the existing safety tests still pass.
Tests are real. Over-cap-by-size, over-cap-by-count, disabled-cap, setuid-stripped-on-extract, plus direct assertSafeTar (reports setid) and stripSetid (clears on disk) unit tests. The suite honestly notes that non-root tar drops setuid on its own and adds the direct strip-path test to cover the root case.

Non-blocking observation (follow-up, not for this PR): for compressed archives, extractArchive decompresses the whole object to a temp .tar in os.tmpdir() before the byte cap is evaluated, so a true high-ratio bomb is still fully written to /tmp (then refused before the destination). This is pre-existing (random-access validation needs a seekable tar) and still fails closed (a full /tmp aborts the pipeline). Worth a later issue to cap/stream the decompression itself, but out of scope here and it does not weaken the destination guarantee this PR makes.

LGTM. Not merging (non-author); over to you.

**VERDICT: APPROVE** (posted as a comment; the review API blocks a formal approve from the PR-author identity this CI tooling runs as. Over to you for the non-author approve + merge.) Reviewed `feat/2-extraction-caps-setuid` @ 77e7e22. CI green (unit + e2e). `make test` green locally (26 tests). Diff is confined to `src/lib/artifact-tar.js` + tests + CHANGELOG/README. **What I verified** 1. Size/count caps refuse before any write to the destination. Confirmed with real `tar`-built archives, not just the test harness: - Over-count: a 5-file archive with `RASTER_ARTIFACTS_MAX_ENTRIES=3` is refused and the destination stays empty. - Over-size / bomb ratio: a 1 MiB-declared payload compressing to ~1.1 KiB, with `RASTER_ARTIFACTS_MAX_TOTAL_BYTES=64KiB`, is refused (`uncompressed size exceeds cap`) with nothing written to dest. - Benign gzip artifact still extracts and round-trips (nested + top-level files). - The caps run inside `assertSafeTar` before `tar -xf`, and `size` is summed from each header's declared size, which is exactly what tar would write. `0`/non-positive disables a cap (documented, tested). 2. setuid/setgid handling. Built a real `0o4755` file, archived and extracted: `assertSafeTar` flags it via `setidPaths`, `stripSetid` clears `0o6000` after extraction, ordinary perms (owner-exec) and contents are preserved. The strip is unconditional post-extraction (re-stat + chmod), so it also covers a root runner where tar would keep the bits. Operator precedence in `parseMode(block) & SETID_BITS` is correct (`&&` binds looser than `&`). 3. No regression to the merged work. Digest verification (#1) lives in `download-impl.js` (`verifyDigest` runs before `tar.extractArchive`) and is not touched by this diff; the `extractArchive(archive, dest, method)` public signature is unchanged (caps are an internal default param). The #2-core entry validation (absolute/`..`/escaping link/special file) is preserved in `validateEntry`; only its return value changed (null vs resolved path), and the existing safety tests still pass. 4. Tests are real. Over-cap-by-size, over-cap-by-count, disabled-cap, setuid-stripped-on-extract, plus direct `assertSafeTar` (reports setid) and `stripSetid` (clears on disk) unit tests. The suite honestly notes that non-root tar drops setuid on its own and adds the direct strip-path test to cover the root case. **Non-blocking observation (follow-up, not for this PR):** for compressed archives, `extractArchive` decompresses the whole object to a temp `.tar` in `os.tmpdir()` before the byte cap is evaluated, so a true high-ratio bomb is still fully written to `/tmp` (then refused before the destination). This is pre-existing (random-access validation needs a seekable tar) and still fails closed (a full `/tmp` aborts the pipeline). Worth a later issue to cap/stream the decompression itself, but out of scope here and it does not weaken the destination guarantee this PR makes. LGTM. Not merging (non-author); over to you.

stephen referenced this pull request

2026-06-17 18:32:29 +00:00

Hardening: bound the decompress-to-temp size before the extraction caps run #6