diff --git a/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift b/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift index 34c74dc..ff3e5b3 100644 --- a/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift +++ b/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift @@ -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 { + // 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() + break + } + Thread.sleep(forTimeInterval: 0.05) + } + } + watchdog.start() + return state +} +// === END DEVA11Y-484 EXTRACTION GUARD === + private struct PluginError: Error, CustomStringConvertible { let message: String diff --git a/scripts/bash/cli.sh b/scripts/bash/cli.sh index 5912a57..197fe97 100644 --- a/scripts/bash/cli.sh +++ b/scripts/bash/cli.sh @@ -241,21 +241,68 @@ verify_binary_integrity() { } 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 -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." >&2 + 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). - bsdtar -xvf "$BINARY_ZIP_PATH" -O > "${BINARY_PATH}.tmp" \ - && chmod 0755 "${BINARY_PATH}.tmp" \ - && mv -f "${BINARY_PATH}.tmp" "$BINARY_PATH" \ - && strip_quarantine + # + # 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" + 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` diff --git a/scripts/bash/cli.sh.sha256 b/scripts/bash/cli.sh.sha256 index 089b5d5..325a1e8 100644 --- a/scripts/bash/cli.sh.sha256 +++ b/scripts/bash/cli.sh.sha256 @@ -1 +1 @@ -14b7e853e5cbd233aa402a6be434cd860ee7cc4037f0e653752d6867c99bb7f2 cli.sh +d55cd02006f37fe71a9686cdd6f3eab3c675f51eb17c48e79577dac539a998c1 cli.sh diff --git a/scripts/fish/cli.sh b/scripts/fish/cli.sh index 5ad931f..db4fc84 100644 --- a/scripts/fish/cli.sh +++ b/scripts/fish/cli.sh @@ -253,21 +253,68 @@ verify_binary_integrity() { } 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 -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." >&2 + 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). - bsdtar -xvf "$BINARY_ZIP_PATH" -O > "${BINARY_PATH}.tmp" \ - && chmod 0755 "${BINARY_PATH}.tmp" \ - && mv -f "${BINARY_PATH}.tmp" "$BINARY_PATH" \ - && strip_quarantine + # + # 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" + 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` diff --git a/scripts/fish/cli.sh.sha256 b/scripts/fish/cli.sh.sha256 index bad1cac..f08924e 100644 --- a/scripts/fish/cli.sh.sha256 +++ b/scripts/fish/cli.sh.sha256 @@ -1 +1 @@ -0d2ca5760c849521d4d5a74fc418c168f76ed129af4a6c1f763003b50a3a4f51 cli.sh +98b5de10b4b77dc17d71f9adb1c34b7b7454ad9fead2c4700000de1160262103 cli.sh diff --git a/scripts/zsh/cli.sh b/scripts/zsh/cli.sh index 1d554d8..3882705 100644 --- a/scripts/zsh/cli.sh +++ b/scripts/zsh/cli.sh @@ -252,21 +252,68 @@ verify_binary_integrity() { } 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 -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." >&2 + 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). - bsdtar -xvf "$BINARY_ZIP_PATH" -O > "${BINARY_PATH}.tmp" \ - && chmod 0755 "${BINARY_PATH}.tmp" \ - && mv -f "${BINARY_PATH}.tmp" "$BINARY_PATH" \ - && strip_quarantine + # + # 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" + 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` diff --git a/scripts/zsh/cli.sh.sha256 b/scripts/zsh/cli.sh.sha256 index c161ba4..2bbef54 100644 --- a/scripts/zsh/cli.sh.sha256 +++ b/scripts/zsh/cli.sh.sha256 @@ -1 +1 @@ -aeb2333296f5b2b25c48a420abd0e89834fc3bf1b2e73acf782f223041dc3edf cli.sh +af0b8acbc1c2cb9b275a14d0f38866e3357cb4fa8b319d13b6435e52f0abad35 cli.sh