fix(security): zip bomb denial of service in document extraction [MEDIUM]

[lycheepuppy]

Summary

• Fix zip bomb DoS where DOCX/PPTX/XLSX extraction read ZIP entries into memory with read_to_string() without decompressed size limits, allowing a small compressed file to expand to gigabytes and cause OOM
• Added bounded_read_zip_entry() helper with per-entry (50 MB) and total (100 MB) decompressed size limits
• Uses take() as defense-in-depth against archives that lie about declared entry sizes
• Applied bounded reads to all three extractors: extract_pptx, extract_xlsx, and extract_office_xml (DOCX)
• Added regression tests verifying bounded read behavior

Security Finding: Zip Bomb Denial of Service in Document Extraction

Severity: MEDIUM
Reported by: FailSafe Security Researcher
Component: src/document_extraction/extractors.rs — extract_pptx(), extract_xlsx(), extract_office_xml()

Description

The document extraction pipeline handles DOCX, PPTX, and XLSX files by opening them as ZIP archives and reading individual entries into memory with read_to_string(). While a 10 MB limit (MAX_DOCUMENT_SIZE) is enforced on the compressed input data, there was no limit on the decompressed size of individual files within the archive.

ZIP achieves compression ratios of 1000:1 or higher for repetitive data like XML:

Compressed Size	Compression Ratio	Decompressed Size	Passes 10 MB Check?
5 MB	100:1	500 MB	Yes
9 MB	500:1	4.5 GB	Yes
10 MB	1000:1	10 GB	Yes

The existing MAX_EXTRACTED_TEXT_LEN limit (100K chars) is applied after all entries are fully decompressed into memory, so the OOM occurs during read_to_string() before the trim executes.

Attack Vector

An attacker who can submit a document for extraction (via file upload, web fetch, or any integration that triggers document parsing) can crash the IronClaw process with an OOM. The attack requires only a crafted DOCX/PPTX/XLSX file under 10 MB compressed.

Impact

• Process crash affects all active sessions, conversations, and running jobs on the same instance
• The 10 MB compressed size limit is insufficient — zip bombs well within 10 MB can decompress to gigabytes
• The attack surface includes any code path that triggers document extraction

Fix

Added bounded_read_zip_entry() that provides defense-in-depth:

1. Declared size check — file.size() catches honest zip bombs before reading
2. Cumulative tracking — total decompressed bytes tracked across all entries
3. take() enforcer — caps actual bytes read even if declared size is wrong

Test plan

• Existing document extraction tests still pass
• New test bounded_read_rejects_oversized_declared_entry — verifies small entries succeed
• New test bounded_read_tracks_total_decompressed — verifies cumulative size tracking
• cargo clippy --all -- -D warnings — zero warnings

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces decompression size limits for ZIP entries within Office document extractors to protect against zip bomb attacks. It adds a bounded_read_zip_entry utility and updates the PPTX, XLSX, and XML extractors accordingly. A review comment identifies a potential security bypass where the cumulative size counter relies on untrusted ZIP header metadata and suggests using actual bytes read instead, while also recommending the use of structured error types for improved error reporting.

[gemini-code-assist]

The current implementation trusts the uncompressed size declared in the ZIP header to update the cumulative counter, which can be bypassed by a malicious archive. To prevent OOM conditions, the counter should be updated using the actual number of bytes read. Additionally, per repository rules, use specific error variants (e.g., TotalSizeLimitExceeded) instead of generic strings to provide semantically clear error messages and ensure accumulated metrics are propagated with the error.

    if *total_decompressed + entry_size > MAX_DECOMPRESSED_TOTAL {
        return Err(ExtractionError::TotalSizeLimitExceeded {
            limit: MAX_DECOMPRESSED_TOTAL,
            current: *total_decompressed,
        });
    }

    let mut xml = String::new();
    file.take(MAX_DECOMPRESSED_ENTRY)
        .read_to_string(&mut xml)
        .map_err(|e| ExtractionError::EntryReadFailed {
            name: file.name().to_string(),
            source: e,
        })?;

    *total_decompressed += xml.len() as u64;
    if *total_decompressed > MAX_DECOMPRESSED_TOTAL {
        return Err(ExtractionError::TotalSizeLimitExceeded {
            limit: MAX_DECOMPRESSED_TOTAL,
            current: *total_decompressed,
        });
    }
    Ok(xml)

References

1. Create specific error variants for different failure modes to provide semantically correct and clear error messages.
2. When an operation can fail, ensure that any accumulated metrics (like resource usage) are propagated out along with the error, not discarded.
3. When accumulating metrics from multiple events, ensure the logic correctly sums the values rather than overwriting them.

[serrrfirat]

High Severity

bounded_read_zip_entry() in src/document_extraction/extractors.rs enforces decompression limits using file.size(), which is only the archive metadata value.

A malformed ZIP can lie about that size. In that case the total budget accounting undercounts real decompressed bytes, and the take(MAX_DECOMPRESSED_ENTRY) reader can truncate oversized content instead of rejecting it. That weakens the zip-bomb boundary this PR is trying to establish.

Suggested fix: account for actual bytes read, and fail closed if the bounded reader hits the cap or if metadata is inconsistent with the stream.

---

Medium Severity

The new regression tests do not actually exercise the attack boundary they describe. bounded_read_rejects_oversized_declared_entry() builds a tiny valid entry, and bounded_read_tracks_total_decompressed() only asserts that total > 0.

Suggested fix: add fixtures that truly exceed the per-entry and total decompressed limits and assert rejection, not just successful extraction.

[lycheepuppy]

added fixes based on feedback

[serrrfirat]

Security Review: Zip Bomb DoS in Document Extraction

Overall: Request Changes — The core fix is solid and well-designed. The layered defense in bounded_read_zip_entry() is the right approach. Main gap is test coverage for the most critical defense path.

What works well

• Layered defense: declared-size pre-check → take() cap (50MB) → fail-closed truncation detection → actual-bytes cumulative tracking (100MB)
• Correctly fixed after reviewer feedback — cumulative tracking now uses xml.len() (actual bytes read), not header-declared size
• Applied consistently across all three extractors (PPTX, XLSX, DOCX)
• Fail-closed design: truncation = rejection, not silent data loss
• Clean, minimal diff with good doc comments

Must fix

1. No test for the per-entry size limit / fail-closed truncation path
The most critical defense layer — where take() truncates an oversized entry and the fail-closed check fires (actual_size >= MAX_DECOMPRESSED_ENTRY) — is completely untested. This is the path that actually stops a zip bomb.

Suggestion: Refactor bounded_read_zip_entry to accept limit parameters (with a public wrapper that passes the defaults). Then tests can use small limits (e.g., 100 bytes) to exercise the truncation and fail-closed paths without creating 50MB fixtures:

fn bounded_read_zip_entry_with_limits(
    file: &mut ZipFile<'_>,
    total: &mut u64,
    max_entry: u64,
    max_total: u64,
) -> Result<String, String> { ... }

// Public API uses defaults
fn bounded_read_zip_entry(file: &mut ZipFile<'_>, total: &mut u64) -> Result<String, String> {
    bounded_read_zip_entry_with_limits(file, total, MAX_DECOMPRESSED_ENTRY, MAX_DECOMPRESSED_TOTAL)
}

Should fix

2. Add a caller-level integration test
No test drives extract_pptx, extract_xlsx, or extract_office_xml end-to-end with a crafted archive. Per the project rule: "Test through the caller, not just the helper." A test that creates a small PPTX-like ZIP with an oversized ppt/slides/slide1.xml entry and verifies the extractor rejects it would be high-value.

3. Structured error types
Error messages use format!() strings rather than structured error variants. The project convention (CLAUDE.md) is thiserror for error types. Consider an ExtractionError enum with EntryTooLarge, TotalSizeLimitExceeded, EntryReadFailed variants. This was also flagged by the Gemini reviewer.

Minor / Nice to have

• Add a test with a deflated (compressed) ZIP entry to confirm take() operates on decompressed bytes (all current tests use CompressionMethod::Stored)
• Consider capping the number of ZIP entries processed as additional hardening (currently unbounded, though the cumulative budget provides a natural cap)
• The extract_office_xml function initializes cumulative tracking but only reads one entry — not a bug, just unnecessary overhead. A comment would clarify the intent.

[serrrfirat]

Paranoid Security Review — NEEDS CHANGES

PR: #2093 — Zip bomb DoS in document extraction
Reviewer context: Automated deep security review

High

No test for per-entry truncation path: The most critical defense — where take() truncates a decompressing entry and the actual_size >= MAX_DECOMPRESSED_ENTRY check rejects it — has zero test coverage. This is the path that stops a real zip bomb (where the declared header size is a lie). Existing tests only exercise small entries and the cumulative budget.

Fix: Refactor into bounded_read_zip_entry_with_limits(file, total, max_entry, max_total) and test with small limits (e.g., 100 bytes) to exercise the fail-closed truncation detection without needing 50 MB fixtures.

Missing caller-level test: No test drives extract_pptx, extract_xlsx, or extract_office_xml with a crafted oversized archive. Per the project's "Test Through the Caller, Not Just the Helper" rule, this is a required coverage gap.

Medium

No cap on ZIP entry count (entry count bomb): A malicious archive with millions of zero-byte entries consumes CPU and memory for metadata parsing without triggering the byte budget. Recommend adding const MAX_ZIP_ENTRIES: usize = 10_000 with an early bail.

read_to_string allocates up to 50 MB in one shot: Multiple concurrent extractions could allocate 200 MB+ simultaneously since limits are per-archive, not per-process.

Low

• String error types instead of structured thiserror errors (project convention).
• All tests use CompressionMethod::Stored — no test with actual deflate compression.

Summary

The core defense mechanism is well-designed (layered: pre-check → take() → fail-closed → actual-bytes tracking). The TOCTOU concern is explicitly addressed. The main blocker is test coverage for the actual zip bomb defense path and caller-level integration tests.

[serrrfirat]

Medium Severity — Test Coverage

Missing XLSX caller-level rejection test.

DOCX has extract_docx_rejects_oversized_entry and PPTX has extract_pptx_rejects_oversized_slide, but there's no equivalent for XLSX. XLSX has a more complex two-phase path: the shared strings read propagates errors with ?, while the sheets loop swallows errors with continue. A caller-level test for extract_xlsx with an oversized sheet (or oversized xl/sharedStrings.xml) would verify both paths and complete the caller-level coverage.

Suggestion — add something like:

#[test]
fn extract_xlsx_rejects_oversized_shared_strings() {
    // Build a zip with xl/sharedStrings.xml > 50 MB
    // Assert extract_xlsx returns Err
}

#[test]
fn extract_xlsx_rejects_oversized_sheet() {
    // Build a zip with xl/worksheets/sheet1.xml > 50 MB,
    // valid xl/sharedStrings.xml
    // Assert extract_xlsx returns Err (no valid sheets → "no text found")
}

Non-blocking — the fix itself is solid.

[serrrfirat]

Solid security fix. Defense-in-depth approach with pre-check + take() + cumulative tracking is well designed. Tests are thorough. One non-blocking comment about adding XLSX caller-level tests to match DOCX/PPTX coverage.