fix(security): cap bsdtar extraction size to prevent decompression bomb DoS [DEVA11Y-484] #25
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -170,6 +170,12 @@ private struct BrowserStackCLIDownloader { | |
|
|
||
| private var fileManager: FileManager { .default } | ||
|
|
||
| // Decompression-bomb guards (DEVA11Y-484). The CLI binary is a few tens of MB; these | ||
| // ceilings leave generous headroom while bounding a malicious archive's footprint. | ||
| private static let maxCompressedBytes: Int64 = 100 * 1024 * 1024 // 100 MB on the wire | ||
| private static let maxDecompressedBytes: Int64 = 200 * 1024 * 1024 // 200 MB on disk | ||
| private static let maxArchiveEntries = 10_000 | ||
|
|
||
| func ensureArtifact() async throws -> BrowserStackCLIArtifact { | ||
| if let overrideURL { | ||
| let info = try await resolveOverrideArtifact(from: overrideURL) | ||
|
|
@@ -447,13 +453,25 @@ private struct BrowserStackCLIDownloader { | |
| let errorPipe = Pipe() | ||
| process.standardError = errorPipe | ||
|
|
||
| let limitState: ExtractionLimitState | ||
| do { | ||
| try process.run() | ||
| // Decompressed-size/entry guard (DEVA11Y-484); see the EXTRACTION GUARD block below. | ||
| limitState = startExtractionWatchdog(on: process, directory: directory, maxBytes: Self.maxDecompressedBytes, maxEntries: Self.maxArchiveEntries) | ||
| process.waitUntilExit() | ||
| } catch { | ||
| throw PluginError("Failed to launch bsdtar: \(error.localizedDescription)") | ||
| } | ||
|
|
||
| // Catch a bomb that completed within a single watchdog poll interval (fast disk). | ||
| if !limitState.exceeded, let reason = footprintExceeded(at: directory, maxBytes: Self.maxDecompressedBytes, maxEntries: Self.maxArchiveEntries) { | ||
| limitState.markExceeded(reason) | ||
| } | ||
| if limitState.exceeded { | ||
| try? fileManager.removeItem(at: directory) | ||
| forwardExit(code: 1, message: "BrowserStack CLI archive rejected: \(limitState.reason). Aborting to prevent disk exhaustion.") | ||
| } | ||
|
|
||
| if process.terminationReason != .exit || process.terminationStatus != 0 { | ||
| // Fall back to copying the file directly if it's already an executable. | ||
| let message = String(data: errorPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" | ||
|
|
@@ -582,8 +600,33 @@ private struct BrowserStackCLIDownloader { | |
|
|
||
| let (tempURL, response) = try await URLSession.shared.download(from: url) | ||
| if let httpResponse = response as? HTTPURLResponse, !(200..<300).contains(httpResponse.statusCode) { | ||
| try? fileManager.removeItem(at: tempURL) | ||
| throw PluginError("Failed to download BrowserStack CLI (HTTP \(httpResponse.statusCode)).") | ||
| } | ||
|
|
||
| // Compressed-size cap (DEVA11Y-484 review). Without it a multi-GB *compressed* | ||
| // payload from an attacker-controlled URL (BROWSERSTACK_A11Y_CLI_DOWNLOAD_URL) is | ||
| // checksummed and handed to the extraction guard, which only ever bounds the | ||
| // *decompressed* footprint — so the archive itself is an unbounded surface. | ||
| // | ||
| // LIMITATION, stated plainly: URLSession.download(from:) has no byte-level hook, so | ||
| // these checks reject the archive *after* the transfer rather than aborting it | ||
| // mid-stream. They therefore prevent an oversized archive from being verified, | ||
| // extracted, published or executed, but they do NOT bound peak temporary disk during | ||
| // the transfer itself. Bounding that needs a URLSessionDownloadDelegate that cancels | ||
| // in didWriteData — deliberately left as a separate change (DEVA11Y-761) rather than | ||
| // rewriting this shared download path here. The shell launchers do abort pre-transfer, | ||
| // via curl --max-filesize. | ||
| if response.expectedContentLength > Self.maxCompressedBytes { | ||
| try? fileManager.removeItem(at: tempURL) | ||
| throw PluginError("BrowserStack CLI archive declares \(response.expectedContentLength) bytes, above the \(Self.maxCompressedBytes)-byte limit; refusing to download it.") | ||
| } | ||
| let downloadedBytes = (try? fileManager.attributesOfItem(atPath: tempURL.path)[.size] as? Int64) ?? nil | ||
| if let downloadedBytes, downloadedBytes > Self.maxCompressedBytes { | ||
| try? fileManager.removeItem(at: tempURL) | ||
| throw PluginError("BrowserStack CLI archive is \(downloadedBytes) bytes, above the \(Self.maxCompressedBytes)-byte limit; refusing to use it.") | ||
| } | ||
|
|
||
| if fileManager.fileExists(atPath: destination.path) { | ||
| try fileManager.removeItem(at: destination) | ||
| } | ||
|
|
@@ -616,8 +659,16 @@ private struct BrowserStackCLIDownloader { | |
| ) | ||
|
|
||
| var fallback: URL? | ||
| var scanned = 0 | ||
|
|
||
| while let element = enumerator?.nextObject() as? URL { | ||
| scanned += 1 | ||
| if scanned > Self.maxArchiveEntries { | ||
| // Bound enumeration so an archive packed with millions of entries can't turn | ||
| // locateExecutable into a CPU/IO drain (DEVA11Y-484). | ||
| throw PluginError("Extracted archive contains more than \(Self.maxArchiveEntries) entries; refusing to continue.") | ||
| } | ||
|
|
||
| var isDirectory: ObjCBool = false | ||
| guard fileManager.fileExists(atPath: element.path, isDirectory: &isDirectory), !isDirectory.boolValue else { | ||
| continue | ||
|
|
@@ -787,6 +838,114 @@ private let browserstackCLIPermissionDeniedExitCode: Int32 = 4 | |
|
|
||
| // MARK: - Error | ||
|
|
||
| // === DEVA11Y-484 EXTRACTION GUARD === | ||
| // | ||
| // Rationale: bsdtar writes decompressed bytes straight to disk, so bounding the | ||
| // archive's *compressed* size says nothing about how much it expands to — useless | ||
| // against a decompression bomb. Instead we poll the destination directory while | ||
| // bsdtar runs and terminate it if the decompressed footprint crosses a byte OR | ||
| // entry ceiling (the entry ceiling stops a "millions of tiny files" bomb that stays | ||
| // small on disk). | ||
| // | ||
| // 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). | ||
| // | ||
| // Applies 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. Windows' unzip path has no streaming guard. | ||
|
|
||
| /// Thread-safe flag shared between the extraction watchdog and the main flow. | ||
| private final class ExtractionLimitState { | ||
| private let lock = NSLock() | ||
| private var didExceed = false | ||
| private var why = "" | ||
|
|
||
| func markExceeded(_ reason: String) { | ||
| lock.lock() | ||
| if !didExceed { | ||
| didExceed = true | ||
| why = reason | ||
| } | ||
| lock.unlock() | ||
| } | ||
|
|
||
| var exceeded: Bool { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| return didExceed | ||
| } | ||
|
|
||
| var reason: String { | ||
| lock.lock() | ||
| defer { lock.unlock() } | ||
| return why | ||
| } | ||
| } | ||
|
|
||
| /// Total bytes and entry count of all regular files under `url`. | ||
| private func extractionFootprint(at url: URL) -> (bytes: Int64, entries: Int) { | ||
| let fm = FileManager.default | ||
| // `.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). | ||
| guard let enumerator = fm.enumerator(at: url, includingPropertiesForKeys: [.isRegularFileKey, .fileSizeKey]) else { | ||
|
Collaborator
There was a problem hiding this comment. P3 — two small issues in
Collaborator
Author
There was a problem hiding this comment. Both fixed in 2c5fba8. 1. Fail-open → fail-closed. You are right that 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:
So the shared // `.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 |
||
| // 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) | ||
| } | ||
| var total: Int64 = 0 | ||
| var count = 0 | ||
| for case let element as URL in enumerator { | ||
| count += 1 | ||
| let values = try? element.resourceValues(forKeys: [.isRegularFileKey, .fileSizeKey]) | ||
| if values?.isRegularFile == true, let size = values?.fileSize { | ||
| total += Int64(size) | ||
| } | ||
| } | ||
| return (total, count) | ||
| } | ||
|
|
||
| /// Returns a rejection reason if the footprint under `directory` exceeds either ceiling. | ||
| private func footprintExceeded(at directory: URL, maxBytes: Int64, maxEntries: Int) -> String? { | ||
| let footprint = extractionFootprint(at: directory) | ||
| if footprint.bytes > maxBytes { | ||
| return "decompressed size exceeds \(maxBytes / (1024 * 1024)) MB" | ||
| } | ||
| if footprint.entries > maxEntries { | ||
| return "archive contains more than \(maxEntries) entries" | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| /// Starts a background watchdog that terminates `process` (bsdtar) if the decompressed | ||
| /// footprint in `directory` exceeds the byte or entry ceiling. | ||
| /// | ||
| /// This is a SOFT ceiling: bsdtar can write up to one poll interval's worth of data past | ||
| /// the limit before it is killed, so peak disk use is roughly `maxBytes + (50 ms × disk | ||
| /// write rate)` — the poll interval below is 50 ms. The goal is to prevent disk | ||
| /// *exhaustion* by a multi-GB/TB bomb, not to enforce an exact byte count. | ||
| /// Callers MUST also run `footprintExceeded` once the process exits, to catch a fast bomb | ||
| /// that finished within a single poll interval. | ||
| private func startExtractionWatchdog(on process: Process, directory: URL, maxBytes: Int64, maxEntries: Int) -> ExtractionLimitState { | ||
| let state = ExtractionLimitState() | ||
| let watchdog = Thread { | ||
| while process.isRunning { | ||
| if let reason = footprintExceeded(at: directory, maxBytes: maxBytes, maxEntries: maxEntries) { | ||
| state.markExceeded(reason) | ||
| process.terminate() | ||
|
Collaborator
There was a problem hiding this comment. P3 — SIGTERM only, no escalation.
Collaborator
Author
There was a problem hiding this comment. 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 A bounded wait plus 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 |
||
| break | ||
| } | ||
| Thread.sleep(forTimeInterval: 0.05) | ||
|
Collaborator
There was a problem hiding this comment. P3 — poll-interval doc drift. This sleeps every 50 ms (
Collaborator
Author
There was a problem hiding this comment. 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: 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). |
||
| } | ||
| } | ||
| watchdog.start() | ||
| return state | ||
| } | ||
| // === END DEVA11Y-484 EXTRACTION GUARD === | ||
|
|
||
| private struct PluginError: Error, CustomStringConvertible { | ||
| let message: String | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| #!/usr/bin/env bash -il | ||
|
|
||
| GIT_ROOT=$(git rev-parse --show-toplevel 2>/dev/null) | ||
|
|
@@ -241,21 +241,68 @@ | |
| } | ||
|
|
||
| download_binary() { | ||
| local max_compressed=104857600 # 100 MB cap on the compressed download | ||
| local max_decompressed=209715200 # 200 MB cap on the decompressed binary | ||
|
|
||
| # --max-filesize aborts the transfer once the declared size is known to exceed the cap. | ||
| # Measured against this endpoint (which 302s to sdk-assets), curl bails with a non-zero | ||
| # exit and nothing written to disk. curl documents the flag as a no-op when the length is | ||
| # unknown (chunked responses), so the explicit size check below backstops that case — | ||
| # otherwise an attacker-controlled endpoint could exhaust the disk during download, before | ||
| # the checksum and the decompression guard ever run (DEVA11Y-484 review). | ||
| local resolved_url | ||
| resolved_url=$(curl -fR --max-filesize "$max_compressed" -z "$BINARY_ZIP_PATH" -L "https://api.browserstack.com/sdk/v1/download_cli?os=${OS}&os_arch=${ARCH}" -o "$BINARY_ZIP_PATH" -w '%{url_effective}') || { | ||
| echo "CLI download failed or exceeds the maximum allowed download size (100 MB)." >&2 | ||
| rm -f "$BINARY_ZIP_PATH" | ||
| return 1 | ||
| } | ||
|
|
||
| local compressed_size | ||
| compressed_size=$(wc -c < "$BINARY_ZIP_PATH" 2>/dev/null || echo 0) | ||
| if [[ $compressed_size -gt $max_compressed ]]; then | ||
| echo "BrowserStack CLI archive exceeds the maximum allowed download size (100 MB). Aborting." >&2 | ||
| rm -f "$BINARY_ZIP_PATH" | ||
| return 1 | ||
| fi | ||
|
|
||
| verify_binary_integrity "$BINARY_ZIP_PATH" "$resolved_url" || return $? | ||
|
|
||
| # Extract to a temp path and atomically publish it. `> "$BINARY_PATH"` truncates the | ||
| # destination before bsdtar is known to have succeeded, so a corrupt payload — the live | ||
| # case today, since no sidecars are published yet and verification fails open — would | ||
| # zero out a previously-good cached binary. Stage + mv keeps the cached binary intact | ||
| # unless a fresh, extractable payload is in hand (DEVA11Y-473/474 review). | ||
| # | ||
| # The decompression-bomb guard (DEVA11Y-484) sits on that same staged path: head -c stops | ||
| # bsdtar via SIGPIPE once the decompressed output reaches the cap, and pipefail surfaces | ||
| # that as a failure. Because the cap applies to ${BINARY_PATH}.tmp and publication is a | ||
| # later mv, a rejected bomb leaves any previously-cached binary untouched. | ||
| # Save and restore pipefail rather than clearing it: these scripts do not enable it | ||
| # globally today, but unconditionally turning it off would silently disable it for | ||
| # everything after download_binary if they ever do (DEVA11Y-484 review). | ||
| local pipefail_was_set=0 | ||
| case "$(set +o)" in *"-o pipefail"*) pipefail_was_set=1 ;; esac | ||
| set -o pipefail | ||
| bsdtar -xvf "$BINARY_ZIP_PATH" -O | head -c "$max_decompressed" > "${BINARY_PATH}.tmp" | ||
|
Collaborator
There was a problem hiding this comment. P3 — shell path lacks the Swift entry-count guard. In (Applies identically to
Collaborator
Author
There was a problem hiding this comment. Acknowledged as a real gap, and deliberately not fixed in this PR — flagging rather than silently skipping. Your analysis is right: in Why it is not in this commit: there is no cheap, correct mechanism in
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 ( Happy to take the "extract to a directory" approach as its own PR if you would rather not carry the gap. |
||
| local extract_status=$? | ||
| [[ $pipefail_was_set -eq 1 ]] || set +o pipefail | ||
|
|
||
| local extracted_size | ||
| extracted_size=$(wc -c < "${BINARY_PATH}.tmp" 2>/dev/null || echo 0) | ||
| if [[ $extract_status -ne 0 || $extracted_size -ge $max_decompressed ]]; then | ||
| echo "BrowserStack CLI download failed or exceeds the maximum allowed size (200 MB). Aborting." >&2 | ||
| rm -f "${BINARY_PATH}.tmp" | ||
| return 1 | ||
| fi | ||
|
|
||
| # Clean the staged file up on *any* failure below, not just the size rejection above, | ||
| # so a failed chmod/mv never leaves a stray ${BINARY_PATH}.tmp in the cache. | ||
| 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 | ||
| } | ||
|
|
||
| # Self-update is opt-in (DEVA11Y-475): it runs only via the explicit `self-update` | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| d55cd02006f37fe71a9686cdd6f3eab3c675f51eb17c48e79577dac539a998c1 cli.sh |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 98b5de10b4b77dc17d71f9adb1c34b7b7454ad9fead2c4700000de1160262103 cli.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2 — Windows extraction path is unguarded. This correctly notes the
unzip/Expand-Archivepath 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 -xwithout-Pneutralizes.., absolute paths, and symlink-through, keeping all writes inside the polled-Cdirectory). That's correct today but load-bearing and unasserted — a future-Pwould let writes escape the polled dir and the footprint poll would measure nothing. Worth a comment pinning the assumption.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theprepareArtifact-levelfootprintExceededbackstop positioned againststagingDirectorybeforepublishVersionDirectory— 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 reachingExpand-Archiveat all.libarchive containment — pinned. Good catch that it was load-bearing and unasserted. Now stated in the guard block: