Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
serrrfirat merged 11 commits into from
Apr 22, 2026
Merged

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

Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
652d25f
fix(security): add decompressed size limits to ZIP-based document ext…
Apr 7, 2026
4d1ca56
fix(security): track actual decompressed bytes, not ZIP header metadata
Apr 10, 2026
20f05ca
Merge remote-tracking branch 'origin/staging' into fix/zip-bomb-decom…
Apr 10, 2026
09d6612
Merge branch 'staging' into fix/zip-bomb-decompression-limit
lycheepuppy Apr 13, 2026
389b826
fix(security): use typed errors and pre-check cumulative budget in ZI…
Apr 15, 2026
4f21d36
Merge branch 'staging' into fix/zip-bomb-decompression-limit
lycheepuppy Apr 15, 2026
1829a10
fix(security): add per-entry truncation tests and configurable limits…
Apr 20, 2026
8bcb9a7
Merge branch 'staging' into fix/zip-bomb-decompression-limit
lycheepuppy Apr 20, 2026
14b05c1
Merge branch 'staging' into fix/zip-bomb-decompression-limit
lycheepuppy Apr 21, 2026
dc0de13
style: cargo fmt
Apr 21, 2026
bbee8c3
Merge branch 'staging' into fix/zip-bomb-decompression-limit
lycheepuppy Apr 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 152 additions & 8 deletions src/document_extraction/extractors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

use std::io::Read;

/// Maximum decompressed size for a single ZIP entry (50 MB).
const MAX_DECOMPRESSED_ENTRY: u64 = 50 * 1024 * 1024;
/// Maximum total decompressed size across all ZIP entries (100 MB).
const MAX_DECOMPRESSED_TOTAL: u64 = 100 * 1024 * 1024;

/// Extract text from document bytes based on MIME type and optional filename.
pub fn extract_text(data: &[u8], mime: &str, filename: Option<&str>) -> Result<String, String> {
let base_mime = mime.split(';').next().unwrap_or(mime).trim();
Expand Down Expand Up @@ -64,6 +69,58 @@ pub fn extract_text(data: &[u8], mime: &str, filename: Option<&str>) -> Result<S
}
}

/// Read a zip entry into a string with decompressed size limits.
///
/// Uses the declared header size as a fast pre-check, then tracks **actual
/// bytes read** for the cumulative budget (ZIP headers can lie about sizes).
/// The `take()` reader caps any single entry at `MAX_DECOMPRESSED_ENTRY`.
/// If the reader hits that cap exactly we fail closed — the entry was
/// truncated, meaning the real size exceeds the limit.
fn bounded_read_zip_entry(
file: &mut zip::read::ZipFile<'_>,
total_decompressed: &mut u64,
) -> Result<String, String> {
// Fast pre-check using declared header size (untrusted, but cheap reject).
let declared_size = file.size();
if declared_size > MAX_DECOMPRESSED_ENTRY {
return Err(format!(
"zip entry '{}' declared size {} exceeds per-entry limit {}",
file.name(),
declared_size,
MAX_DECOMPRESSED_ENTRY,
));
}

let entry_name = file.name().to_string();
let mut bounded = file.take(MAX_DECOMPRESSED_ENTRY);
let mut xml = String::new();
bounded
.read_to_string(&mut xml)
.map_err(|e| format!("failed to read zip entry '{}': {e}", entry_name))?;

let actual_size = xml.len() as u64;

// Fail closed: if we read exactly the cap, the entry was truncated and
// the real decompressed size exceeds the per-entry limit.
if actual_size >= MAX_DECOMPRESSED_ENTRY {
return Err(format!(
"zip entry '{}' actual decompressed size reached per-entry limit {}",
entry_name, MAX_DECOMPRESSED_ENTRY,
));
}

// Track cumulative budget using actual bytes, not header metadata.
*total_decompressed += actual_size;
if *total_decompressed > MAX_DECOMPRESSED_TOTAL {
return Err(format!(
"total decompressed size {} exceeds limit {}",
total_decompressed, MAX_DECOMPRESSED_TOTAL,
));
}

Ok(xml)
}

fn extract_pdf(data: &[u8]) -> Result<String, String> {
pdf_extract::extract_text_from_mem(data)
.map(|t| t.trim().to_string())
Expand Down Expand Up @@ -92,10 +149,10 @@ fn extract_pptx(data: &[u8]) -> Result<String, String> {
slide_names.sort();

let mut all_text = Vec::new();
let mut total_decompressed: u64 = 0;
for name in &slide_names {
if let Ok(mut file) = archive.by_name(name) {
let mut xml = String::new();
if file.read_to_string(&mut xml).is_ok() {
if let Ok(xml) = bounded_read_zip_entry(&mut file, &mut total_decompressed) {
let text = strip_xml_tags(&xml);
if !text.is_empty() {
all_text.push(text);
Expand All @@ -115,10 +172,11 @@ fn extract_xlsx(data: &[u8]) -> Result<String, String> {
let mut archive =
zip::ZipArchive::new(cursor).map_err(|e| format!("invalid XLSX archive: {e}"))?;

let mut total_decompressed: u64 = 0;

// Read shared strings (xl/sharedStrings.xml)
let shared_strings = if let Ok(mut file) = archive.by_name("xl/sharedStrings.xml") {
let mut xml = String::new();
file.read_to_string(&mut xml)
let xml = bounded_read_zip_entry(&mut file, &mut total_decompressed)
.map_err(|e| format!("failed to read shared strings: {e}"))?;
parse_xlsx_shared_strings(&xml)
} else {
Expand All @@ -140,8 +198,7 @@ fn extract_xlsx(data: &[u8]) -> Result<String, String> {
let mut all_text = Vec::new();
for name in &sheet_names {
if let Ok(mut file) = archive.by_name(name) {
let mut xml = String::new();
if file.read_to_string(&mut xml).is_ok() {
if let Ok(xml) = bounded_read_zip_entry(&mut file, &mut total_decompressed) {
let text = parse_xlsx_sheet(&xml, &shared_strings);
if !text.is_empty() {
all_text.push(text);
Expand Down Expand Up @@ -170,8 +227,8 @@ fn extract_office_xml(data: &[u8], content_path: &str) -> Result<String, String>
.by_name(content_path)
.map_err(|e| format!("content file not found in archive: {e}"))?;

let mut xml = String::new();
file.read_to_string(&mut xml)
let mut total_decompressed: u64 = 0;
let xml = bounded_read_zip_entry(&mut file, &mut total_decompressed)
.map_err(|e| format!("failed to read content: {e}"))?;

let text = strip_xml_tags(&xml);
Expand Down Expand Up @@ -512,4 +569,91 @@ mod tests {
let strings = parse_xlsx_shared_strings(xml);
assert_eq!(strings, vec!["Name", "Age"]);
}

/// Regression: bounded_read_zip_entry tracks actual bytes read (not header
/// metadata) and a small entry should succeed with correct accounting.
#[test]
fn bounded_read_tracks_actual_bytes() {
use std::io::{Cursor, Write};
let content = b"<root>hello</root>";
let buf = Vec::new();
let cursor = Cursor::new(buf);
let mut writer = zip::ZipWriter::new(cursor);
let options = zip::write::SimpleFileOptions::default()
.compression_method(zip::CompressionMethod::Stored);
writer.start_file("test.xml", options).unwrap();
writer.write_all(content).unwrap();
let cursor = writer.finish().unwrap();
let data = cursor.into_inner();

let read_cursor = Cursor::new(&data);
let mut archive = zip::ZipArchive::new(read_cursor).unwrap();
let mut total: u64 = 0;
let mut file = archive.by_index(0).unwrap();
let result = bounded_read_zip_entry(&mut file, &mut total);
assert!(result.is_ok(), "small entry should be readable");
// Total must reflect actual content length, not header-declared size.
assert_eq!(total, content.len() as u64);
}

/// Regression: total decompressed tracking must accumulate actual bytes
/// across entries and equal the sum of real content sizes.
#[test]
fn bounded_read_accumulates_actual_bytes_across_entries() {
use std::io::{Cursor, Write};
let content_a = b"<a>data</a>";
let content_b = b"<b>more data here</b>";
let buf = Vec::new();
let cursor = Cursor::new(buf);
let mut writer = zip::ZipWriter::new(cursor);
let options = zip::write::SimpleFileOptions::default()
.compression_method(zip::CompressionMethod::Stored);
writer.start_file("a.xml", options).unwrap();
writer.write_all(content_a).unwrap();
writer.start_file("b.xml", options).unwrap();
writer.write_all(content_b).unwrap();
let cursor = writer.finish().unwrap();
let data = cursor.into_inner();

let read_cursor = Cursor::new(&data);
let mut archive = zip::ZipArchive::new(read_cursor).unwrap();
let mut total: u64 = 0;
let mut f0 = archive.by_index(0).unwrap();
bounded_read_zip_entry(&mut f0, &mut total).unwrap();
drop(f0);
let mut f1 = archive.by_index(1).unwrap();
bounded_read_zip_entry(&mut f1, &mut total).unwrap();
let expected = (content_a.len() + content_b.len()) as u64;
assert_eq!(total, expected, "total must equal sum of actual content sizes");
}

/// Regression: bounded_read_zip_entry must reject when cumulative
/// decompressed bytes exceed MAX_DECOMPRESSED_TOTAL, even for small entries.
#[test]
fn bounded_read_rejects_when_total_budget_exhausted() {
use std::io::{Cursor, Write};
let buf = Vec::new();
let cursor = Cursor::new(buf);
let mut writer = zip::ZipWriter::new(cursor);
let options = zip::write::SimpleFileOptions::default()
.compression_method(zip::CompressionMethod::Stored);
writer.start_file("small.xml", options).unwrap();
writer.write_all(b"<x/>").unwrap();
let cursor = writer.finish().unwrap();
let data = cursor.into_inner();

let read_cursor = Cursor::new(&data);
let mut archive = zip::ZipArchive::new(read_cursor).unwrap();
// Pre-fill the budget to just below the limit so even a tiny entry
// pushes it over.
let mut total: u64 = MAX_DECOMPRESSED_TOTAL - 1;
let mut file = archive.by_index(0).unwrap();
let result = bounded_read_zip_entry(&mut file, &mut total);
assert!(result.is_err(), "should reject when total budget is exceeded");
let err = result.unwrap_err();
assert!(
err.contains("total decompressed size"),
"error should mention total limit, got: {err}"
);
}
}

@serrrfirat serrrfirat Apr 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading