fix(security): cap bsdtar extraction size to prevent decompression bomb DoS [DEVA11Y-484]

[maunilm]

What

Adds a decompressed-size and entry-count guard to the CLI download/extract path, so a decompression bomb cannot exhaust developer or CI-runner disk.

Fixes DEVA11Y-484 (F-015, CWE-400, umbrella APPSEC-415).

Scope

This PR is deliberately narrowed to DEVA11Y-484's stated Remediation. Work that was previously in this branch — the regression suite, its CI workflow, and the Windows Expand-Archive backstop — was removed and is tracked in DEVA11Y-761, preserved on branch chore/DEVA11Y-484-followup-extraction-guard-harness. See Known gaps below.

Changes

Swift plugin — Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift

• startExtractionWatchdog polls the extraction directory every 50 ms while bsdtar runs and terminate()s it once the decompressed footprint crosses maxDecompressedBytes (200 MB) or maxArchiveEntries (10,000). A soft ceiling by design: peak disk ≈ maxBytes + (50 ms × write rate).
• A post-exit footprintExceeded re-check catches a bomb that finishes inside one poll interval.
• locateExecutable throws past 10,000 entries, per the ticket's ask.
• maxCompressedBytes (100 MB) checked against response.expectedContentLength and the downloaded file's actual size.

Attached to extractLocalArchive, which since #37 (DEVA11Y-473/474) is the single non-Windows extraction path — the archive is downloaded to a file and checksum-verified first, then extracted. The old streaming curl | bsdtar path that #37 deleted is gone, so there is no separate remote guard.

Launchers — scripts/{bash,zsh,fish}/cli.sh

• bsdtar … -O | head -c 209715200 with pipefail, so the cap is enforced by SIGPIPE, plus an explicit size assertion as a backstop.
• curl --max-filesize 104857600 plus a post-download size check (curl documents the flag as a no-op when the length is unknown).
• The cap writes to ${BINARY_PATH}.tmp and publication stays a later mv, so a rejected payload cannot truncate a previously-good cached binary — preserving fix(cli): verify downloaded CLI binary integrity before exec (DEVA11Y-473/474) #37's protection.
• Failure returns non-zero; the call site remains download_binary || exit $?, so fix(cli): verify downloaded CLI binary integrity before exec (DEVA11Y-473/474) #37's distinct exit 2 for an integrity mismatch survives.

Verification

No automated suite ships with this PR (see Known gaps), so this was verified directly against the live download endpoint:

• 27/27 assertions across bash/zsh/fish: real download exits 0 through fix(cli): verify downloaded CLI binary integrity before exec (DEVA11Y-473/474) #37's integrity check (38,017,898 B archive → 69,391,104 B binary, perms 0755); .tmp cleaned up after publish; re-run byte-identical; corrupt payload rejected; and the previously-cached binary's sha256 is unchanged after a rejected payload.
• Guard fires: with a 1 MB cap the pipeline returns non-zero at exactly the cap; with the real 200 MB cap it is clean.
• Swift guard, compiled standalone and driven against the real archive: real 200 MB cap does not flag (termStatus=0, 69,391,104 B, 1 entry); a 5 MB cap flags and SIGTERMs bsdtar mid-stream (termStatus=15), bounding disk to 36 MB of 66 MB; maxEntries=0 flags on entry count.
• Compressed cap: with a 1 MB cap curl aborts non-zero with nothing written; the real 100 MB cap passes the 38 MB archive.
• pipefail save/restore verified in both directions.
• swiftc -typecheck -parse-as-library clean; bash -n clean on all three launchers; all six .sha256 sidecars verify; self-update's own comparison matches for all three.

Headroom (CLI v1.52.1): largest platform archive is 41 MB compressed (2.4× under the 100 MB cap) and ~75.6 MB decompressed (2.65× under the 200 MB cap). The caps are duplicated in four places — they must move together when the CLI outgrows them.

Known gaps — owned, not hidden

1. No automated regression coverage ships with this PR. The suite was descoped; nothing prevents a future refactor from silently dropping the caps. DEVA11Y-761 item 1, and the highest-value follow-up.
2. Windows Expand-Archive is unguarded. No download cap enforcement mid-stream, no watchdog, no entry ceiling on that branch. Unchanged from main, but a real gap. DEVA11Y-761 item 3.
3. The Swift compressed cap rejects after the transfer, not mid-stream. URLSession.download(from:) has no byte-level hook, so an oversized archive is stopped before checksum/extract/exec but peak temporary disk during transfer is not bounded. Needs a URLSessionDownloadDelegate cancelling in didWriteData. The launchers do abort during transfer. DEVA11Y-761.
4. The launchers have no entry-count equivalent to the plugin's maxArchiveEntries. In -O mode an archive of millions of empty entries streams ~0 bytes, so head -c never fires; disk stays bounded but bsdtar still parses every entry. Open for review discussion — no cheap mechanism in -O mode.
5. Linux launcher path untested. Verified on macOS/arm64 against the live endpoint; CI has no Linux launcher job.

Note on the ticket's threat model

DEVA11Y-484 states the download has "no TLS, per scope.md:65". The URL in code is https:// and the live endpoint serves HTTPS with a 302 to https://sdk-assets.browserstack.com, which weakens the stated MitM reachability behind the AV:N / CVSS 5.3 rating. The BROWSERSTACK_A11Y_CLI_DOWNLOAD_URL override remains a genuine vector, so the fix stands — but the premise as written is inaccurate.

Refs DEVA11Y-484, DEVA11Y-761, APPSEC-415.

[Crash0v3rrid3]

Multi-agent code review — decompression-bomb guard (DEVA11Y-484)

Reviewed across security/adversarial, Swift concurrency, and shell-correctness lenses plus first-party verification against the PR head. The guard that ships is correctly implemented — but the PR description overstates the implementation on two verified counts, and I'd hold merge until those are reconciled.

What's correct (verified — no action needed)

• Swift concurrency is sound: watchdog thread lifecycle has no race/leak, terminate()→waitUntilExit()→removeItem ordering is safe, ExtractionLimitState's NSLock is correct, and forwardExit is -> Never/exit (no fall-through).
• Shell logic is correct: the bomb is genuinely caught (head -c cap → SIGPIPE → 141 via pipefail, with -ge as a backstop), legit downloads pass, and local x=$? captures the pipeline status correctly.
• scripts/fish/cli.sh is #!/usr/bin/env bash -il (a bash script), so the guard syntax is intact in all three wrappers.
• Path traversal / symlink escape is blocked by libarchive defaults (bsdtar -x without -P) — writes stay inside the polled directory.

Blocking / high-priority

1. (P1) The described test suite and CI workflow do not exist in the PR. The description details scripts/test/, run_tests.sh, check_drift.sh, .github/workflows/extraction-guard-tests.yml, and "53/53 assertions green." At the PR head none of these exist — scripts/ contains only the wrappers, and the only workflows present are Semgrep.yml, spm-smoke-test.yml, and verify-selfupdate-checksums.yml. A security-critical guard would merge with no regression protection. Please commit the suite + CI, or remove the claims from the description.

2. (P2) No compressed-download size cap exists, despite the Summary claiming one. The Summary states "curl --max-filesize (100 MB) caps the compressed download," but:

• scripts/*/cli.sh — the download curl (curl -fR -z … -L … -o …) has no --max-filesize.
• BrowserStackAccessibilityLint.swift — download(...) uses URLSession.shared.download(from: url) with no Content-Length/byte limit.

In the fix's own threat model (MITM of the HTTPS endpoint, or an attacker-controlled HTTPS override URL), a multi-GB compressed payload exhausts disk during download — before checksum or extraction — bypassing the entire decompression guard. Please add --max-filesize to the curl download and a byte cap to the Swift download, or strike the claim.

3. (P2) Windows extraction path is unguarded — see the inline note; either guard it or track it as an explicit follow-up.

Lower priority

Inline P3 comments cover: shell entry-count asymmetry, set +o pipefail toggled unconditionally, .tmp cleanup on chmod/mv failure, poll-interval doc drift (50 ms vs 200 ms), SIGTERM-only kill, extractionFootprint fail-open + hidden-file inconsistency, missing private on the new decls, and the libarchive-containment assumption.

Verdict: Not ready — the core guard is correct, but the compressed-download cap (#2) and the test/CI suite (#1) are described but absent, and Windows is unguarded (#3). Land the missing pieces or correct the description and consciously accept the gaps.

_{🤖 Multi-agent review via Claude Code (compound-engineering). Posted as comments, not a formal request-changes.}

[Crash0v3rrid3]

P3 — shell path lacks the Swift entry-count guard. In -O mode an archive of millions of tiny/empty entries streams ~0 bytes to stdout, so head -c never fills and never SIGPIPEs bsdtar. Disk stays bounded (good), but bsdtar still parses every entry (CPU/time drain) and a near-empty bogus payload passes the size check and gets chmod+mv'd into the cache. The Swift path guards this with maxArchiveEntries = 10_000; the wrappers have no equivalent. Consider an entry ceiling or --max-time on extraction.

(Applies identically to scripts/zsh/cli.sh and scripts/fish/cli.sh.)

[maunilm]

Acknowledged as a real gap, and deliberately not fixed in this PR — flagging rather than silently skipping.

Your analysis is right: in -O mode an archive of millions of empty entries streams ~0 bytes to stdout, so head -c never fills and never SIGPIPEs bsdtar. Disk stays bounded, but bsdtar parses every entry and a near-empty payload passes the size check and gets published. The plugin's maxArchiveEntries = 10_000 has no wrapper equivalent.

Why it is not in this commit: there is no cheap, correct mechanism in -O mode. The options I considered:

• bsdtar -tf pre-pass to count entries — doubles archive parsing and is itself unbounded on a millions-of-entries archive, so it moves the CPU drain rather than removing it.
• --max-time on extraction — a wall-clock proxy for an entry count; flaky on slow CI runners and does not actually bound entries.
• Extract to a directory instead of -O so the footprint is measurable like the plugin's — the correct fix, but that is a real change to the wrapper's extraction model, and the wrappers are what self-update ships to every user from main. Not something I want to land in the same PR as the guard, untested on Linux.

So: tracked as a follow-up on DEVA11Y-761 with your reasoning quoted, and listed under Known gaps item 4 in the rewritten PR description so it is owned rather than invisible.

Worth noting the residual is narrower than it was: the compressed-download cap added in 2c5fba8 (curl --max-filesize + post-download size check) bounds how large such an archive can be in the first place, so the CPU drain is capped at parsing a ≤100 MB archive rather than an unbounded one. That does not close the gap, but it does bound it.

Happy to take the "extract to a directory" approach as its own PR if you would rather not carry the gap.

[Crash0v3rrid3]

P3 — set +o pipefail is toggled unconditionally. Neither wrapper sets pipefail globally today, so this is safe now, but it disables the option outright rather than restoring the prior state. If a global set -o pipefail is ever added to these scripts, this line will silently switch it off for everything after download_binary. Prefer save/restore, e.g. capture set +o | grep pipefail before and restore it after.

Also note: if chmod/mv fail on the happy path just below, ${BINARY_PATH}.tmp is left on disk — the rm -f cleanup only runs on the size-rejection branch. Minor, but inconsistent with the explicit cleanup above.

(Applies identically to scripts/zsh/cli.sh and scripts/fish/cli.sh.)

[maunilm]

Both fixed in 2c5fba8, in all three launchers.

pipefail — now saved and restored rather than cleared:

local pipefail_was_set=0
case "$(set +o)" in *"-o pipefail"*) pipefail_was_set=1 ;; esac
set -o pipefail
bsdtar … | head -c "$max_decompressed" > "${BINARY_PATH}.tmp"
local extract_status=$?
[[ $pipefail_was_set -eq 1 ]] || set +o pipefail

Verified both directions: with set -o pipefail in the caller it is still set after download_binary returns; with it off, it stays off.

.tmp on the happy path — good catch, the asymmetry was real. chmod/mv are now guarded with cleanup on failure instead of a bare && chain:

if ! { chmod 0755 "${BINARY_PATH}.tmp" && mv -f "${BINARY_PATH}.tmp" "$BINARY_PATH"; }; then
  echo "BrowserStack CLI: failed to publish the downloaded binary." >&2
  rm -f "${BINARY_PATH}.tmp"
  return 1
fi
strip_quarantine

[Crash0v3rrid3]

P3 — poll-interval doc drift. This sleeps every 50 ms (0.05), but the PR description and the overshoot math in the docstring above refer to a "200 ms poll interval" — off by 4×. Either bump this to 0.2 or correct the description/comment so the documented worst-case footprint (maxBytes + pollInterval × writeRate) matches reality.

[maunilm]

Fixed in 2c5fba8. You were right that it was off by 4x — and the drift was in the docstring rather than the code, so I corrected the docs to the real 50 ms rather than slowing the poll:

/// the limit before it is killed, so peak disk use is roughly `maxBytes + (50 ms x disk
/// write rate)` — the poll interval below is 50 ms.

Kept 50 ms because it is what the measurements in the description were actually taken at: against the 400 MB fixture the watchdog bounded peak disk to 58 MB, and re-verified on this head against the real 38 MB archive with a 5 MB cap it bounds to 36 MB of 66 MB. Widening to 200 ms would loosen that overshoot 4x for no benefit.

The PR description has also been rewritten (it was stale in several places — see the top-level reply).

[Crash0v3rrid3]

P3 — SIGTERM only, no escalation. terminate() sends SIGTERM once and the loop breaks. bsdtar doesn't trap SIGTERM so this is fine in practice, but if it's ever slow to die (blocked I/O), waitUntilExit() on the main thread blocks with no SIGKILL fallback. Low impact; consider a bounded wait + kill(pid, SIGKILL) escalation for robustness.

[maunilm]

Agreed on the analysis, and taking your own read that it is low impact — not changing it in this PR.

For the record on why: bsdtar does not trap SIGTERM, so in practice it dies immediately; the watchdog breaks straight after terminate() and the loop condition is while process.isRunning, so the thread exits cleanly with no leak. The theoretical hang needs bsdtar blocked in uninterruptible I/O, in which case waitUntilExit() on the main thread would stall with no SIGKILL fallback.

A bounded wait plus kill(pid, SIGKILL) escalation is the right hardening and I would rather add it with a test that actually exercises the escalation path than add an untested kill to a security fix. Noted on DEVA11Y-761 alongside the other deferred items.

Verified on the current head that the non-pathological path behaves: against the real 38 MB archive with a 5 MB cap the watchdog fires and bsdtar reports terminationStatus = 15 (SIGTERM), with disk bounded to 36 MB of the 66 MB it would otherwise have written.

[Crash0v3rrid3]

P3 — two small issues in extractionFootprint.

1. Fails open: if fm.enumerator(...) returns nil (directory transiently unreadable/missing), this returns (0, 0) → footprintExceeded returns nil → "not exceeded" for that poll. Low risk since the plugin created the dir, but a transient failure silently disables the guard for that tick.
2. Inconsistent "entry" definition: this enumerator omits .skipsHiddenFiles, while locateExecutable's enumerator (line ~646) passes options: [.skipsHiddenFiles]. The same 10 000 ceiling therefore counts hidden files here but not there. Align the two so "entries" means the same thing in both guards.

[maunilm]

Both fixed in 2c5fba8.

1. Fail-open → fail-closed. You are right that (0, 0) silently disabled the guard for that poll. It now fails closed:

guard let enumerator = fm.enumerator(at: url, includingPropertiesForKeys: [...]) else {
    // Fail CLOSED: a directory we just created being unreadable is not a "0 bytes"
    // result, and returning (0, 0) would silently disable the guard for that poll.
    return (Int64.max, Int.max)
}

A transient failure now trips the ceiling and aborts rather than waving the archive through. Failing closed is the right default for a guard, and the false-positive cost is an aborted download with a clear message.

2. Hidden-file inconsistency. Also real — but after looking at both call sites I kept the difference and documented it rather than aligning them, because they are measuring different things:

• extractionFootprint measures what bsdtar actually wrote — dotfiles included, since they consume disk and count toward a "millions of tiny files" bomb. Adding .skipsHiddenFiles would let an all-dotfiles archive slip the entry ceiling.
• locateExecutable is searching for a binary, so skipping hidden files is correct there.

So the shared 10_000 is deliberately counting different sets. Comment added at the enumerator making that explicit so the next reader does not "fix" it:

// `.skipsHiddenFiles` is deliberately NOT set, so the entry count here matches what
// bsdtar actually wrote — including dotfiles. locateExecutable skips hidden files
// because it is searching for a binary, not measuring a footprint; the two use the
// same ceiling but count deliberately different things (DEVA11Y-484 review).

Happy to split into two named constants if you would rather the shared 10_000 not imply the two are equivalent.

[Crash0v3rrid3]

P3 — hygiene: these new top-level declarations lack private. ExtractionLimitState, extractionFootprint, footprintExceeded, and startExtractionWatchdog are the only non-private helpers in the file — every other helper (isTruthy, packageCacheRoot, hardwareIdentifier, …) is private. Harmless in a single-file plugin target, but worth marking private for consistency.

[maunilm]

Fixed in 2c5fba8 — all four are now private:

• private final class ExtractionLimitState
• private func extractionFootprint(at:)
• private func footprintExceeded(at:maxBytes:maxEntries:)
• private func startExtractionWatchdog(on:directory:maxBytes:maxEntries:)

Agreed it was inconsistent with every other helper in the file. swiftc -typecheck -parse-as-library is clean after the change.

[Crash0v3rrid3]

P2 — Windows extraction path is unguarded. This correctly notes the unzip/Expand-Archive path has no streaming guard, but Windows is a supported target (#if os(Windows) branches, browserstack-cli.exe, PowerShell checksum). A zip bomb there fully exhausts disk with no download cap, no watchdog, and no entry ceiling. It's out of this PR's stated 4-surface scope, so either add a guard to the Windows path or track it as an explicit follow-up so the gap is owned rather than just commented.

Also, defense-in-depth note for the non-Windows path: containment depends on libarchive's default behavior (bsdtar -x without -P neutralizes .., absolute paths, and symlink-through, keeping all writes inside the polled -C directory). That's correct today but load-bearing and unasserted — a future -P would let writes escape the polled dir and the footprint poll would measure nothing. Worth a comment pinning the assumption.

[maunilm]

Two parts here.

Windows — now tracked, not merely commented. Agreed it is a real gap, and it is explicitly owned: DEVA11Y-761 item 3, with the implementation preserved on chore/DEVA11Y-484-followup-extraction-guard-harness. That branch carries the prepareArtifact-level footprintExceeded backstop positioned against stagingDirectory before publishVersionDirectory — which is where it belongs after #32 restructured extraction, so a rejected archive never becomes a visible version directory.

It came out of this PR when the PR was narrowed to DEVA11Y-484's stated Remediation, which scopes the bsdtar paths only. I noted on the ticket that "Windows has no bomb guard" probably deserves its own security ticket rather than sitting in a cleanup task — say the word and I will raise one.

One thing that does help Windows in the meantime: the compressed-download cap added in 2c5fba8 sits in the shared download(from:to:), so it applies on Windows too. It does not bound decompression, but it stops a multi-GB archive reaching Expand-Archive at all.

libarchive containment — pinned. Good catch that it was load-bearing and unasserted. Now stated in the guard block:

// Containment assumption (load-bearing): `bsdtar -x` WITHOUT `-P` neutralises `..`,
// absolute paths and symlink-through, so every write lands inside the `-C` directory we
// poll. Adding `-P` would let writes escape that directory and the footprint poll would
// measure nothing — do not add it (DEVA11Y-484 review).

[maunilm]

Thanks — this was a genuinely useful review, and one of the two blockers was my fault in a way worth naming explicitly.

Everything below is in 2c5fba8, plus a rewritten PR description. All 9 checks green.

Blocker 1 (P1) — test suite and CI "described but absent": you were right, and the cause was a stale description

The suite and workflow really were not there. The reason is not that they were forgotten — they were deliberately removed when this PR was narrowed to DEVA11Y-484's stated Remediation (the PR had grown to +1044/−7 across 18 files for an XS ticket, with only 216 lines of production code). I removed the code and failed to update the description, so it kept advertising a harness, a CI workflow and "53/53 assertions" that no longer existed. That is exactly the kind of claim a reviewer should not have to discover by diffing, and it wasted your time.

The description is now rewritten to match the shipped code, with an explicit Known gaps section that owns all five gaps rather than implying coverage that does not exist.

The suite itself is not lost: preserved verbatim on chore/DEVA11Y-484-followup-extraction-guard-harness and tracked as DEVA11Y-761 item 1, flagged there as the highest-value follow-up. Your point stands that a security-critical guard is merging without regression protection — that is a conscious, documented trade, not an oversight.

Blocker 2 (P2) — no compressed cap: half stale description, half a real hole. Fixed.

Same root cause for the wording — the cap had been descoped as "not in the ticket's Remediation" and the Summary still claimed it. But your threat-model argument is the substantive part and I think it is correct: without a wire cap, a multi-GB compressed payload exhausts disk before the checksum or the decompression guard ever run, which walks around the entire fix. Deferring it on a scoping technicality was the wrong call.

Reinstated in 2c5fba8:

• Launchers: curl --max-filesize 104857600, plus an explicit post-download size check because curl documents the flag as a no-op when the length is unknown (chunked). Verified against the live endpoint: with a 1 MB cap curl aborts non-zero with 0 bytes written; with the real 100 MB cap the 38 MB archive passes. (Observed exit is 56 rather than 63 — with -L following the 302 to sdk-assets it aborts during receive, not pre-transfer. The comment states the observed behaviour, not the idealised one.)
• Plugin: maxCompressedBytes = 100 MB, checked against both response.expectedContentLength and the downloaded file's actual size, temp file removed on rejection. Applies on Windows too, since it lives in the shared download(from:to:).

One limitation I want to state plainly rather than let the description imply otherwise: URLSession.download(from:) has no byte-level hook, so on the Swift path these checks reject the archive after the transfer. They stop an oversized archive being verified, extracted, published or executed, but they do not bound peak temporary disk during the transfer — which is precisely the scenario you described. Bounding that needs a URLSessionDownloadDelegate cancelling in didWriteData. I chose not to rewrite that shared download path inside a security fix I cannot exercise end-to-end without credentials; it is documented inline, in Known gaps item 3, and on DEVA11Y-761. The launchers do abort during transfer, so the primary installer path is covered.

Blocker 3 (P2) — Windows unguarded

Tracked as DEVA11Y-761 item 3 with the implementation preserved on the follow-up branch, per your "either guard it or track it as an explicit follow-up so the gap is owned". Detail in the inline thread. I also noted on the ticket that this probably warrants its own security ticket rather than living in a cleanup task — happy to raise one.

P3s

Fixed: private on all four new decls; poll-interval doc drift (docs corrected to the real 50 ms rather than slowing the poll); extractionFootprint now fails closed on a nil enumerator; the .skipsHiddenFiles asymmetry documented as deliberate with reasoning; pipefail saved/restored and verified both directions; .tmp cleaned up on chmod/mv failure; libarchive containment assumption pinned in the guard block.

Deferred with reasoning in-thread: the launcher entry-count gap (no cheap correct mechanism in -O mode — the right fix is extracting to a directory, which is too large a change to the wrapper model to smuggle in here) and SIGTERM escalation (which you flagged low-impact yourself).

Verification on this head

No automated suite ships, so this was verified directly against the live endpoint: 27/27 assertions across bash/zsh/fish — real download through #37's integrity check, .tmp cleanup, byte-identical re-run, corrupt payload rejected, and the previously-cached binary's sha256 unchanged after a rejected payload (confirming the cap on ${BINARY_PATH}.tmp preserves #37's anti-truncation protection rather than undoing it). Swift guard compiled standalone and driven against the real archive: real 200 MB cap does not flag; 5 MB cap SIGTERMs bsdtar mid-stream bounding disk to 36 MB of 66 MB; maxEntries=0 flags on entry count. swiftc -typecheck clean, bash -n clean, all six sidecars verify, self-update's own comparison matches.

Ready for another look when you have a moment.