From 6da4c30cae91c32e8cd5932533d32151c2d5b4b4 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 22:26:21 -0500 Subject: [PATCH] fix(svg): refuse a document compressed twice and charge a refused embedded one (#1924) One gzip layer is unpacked within the import budget, but usvg gunzips what it is handed again with no limit, so a doubly compressed document or embedded image expanded outside the budget. A compressed embedded image that failed its budget also left the shared allowance untouched, so every sibling image could repeat the work; a refusal now spends what was left. --- crates/svg/src/import/budget.rs | 15 +++++++++++++- crates/svg/src/import/budget_tests.rs | 28 +++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/crates/svg/src/import/budget.rs b/crates/svg/src/import/budget.rs index 75a5e6731..7de080af4 100644 --- a/crates/svg/src/import/budget.rs +++ b/crates/svg/src/import/budget.rs @@ -44,6 +44,11 @@ fn inflate(data: &[u8], limit: usize) -> Result, String> { if out.len() > limit { return Err(format!("compressed SVG expands past {} MiB", limit >> 20)); } + // usvg gunzips what it is handed, with no limit: one layer is unpacked here, within the + // budget, and what comes out must be the document itself. + if gzipped(&out) { + return Err("compressed SVG holds another compressed file, not a document".into()); + } Ok(out) } @@ -72,7 +77,15 @@ fn embedded(used: &Used, limits: &Limits, data: &[u8], opts: &Options) -> Option return None; } let left = limits.embedded.saturating_sub(used.bytes.load(Ordering::Relaxed)); - let plain = inflate(data, left).ok()?; + let plain = match inflate(data, left) { + Ok(plain) => plain, + // What a refused document unpacked before it was refused was work too: it uses up what is + // left, so the next image can't repeat it. + Err(_) => { + used.bytes.fetch_add(left, Ordering::Relaxed); + return None; + } + }; used.bytes.fetch_add(plain.len(), Ordering::Relaxed); if super::too_deep(&plain) { return None; diff --git a/crates/svg/src/import/budget_tests.rs b/crates/svg/src/import/budget_tests.rs index d3dcd2dea..d8740f901 100644 --- a/crates/svg/src/import/budget_tests.rs +++ b/crates/svg/src/import/budget_tests.rs @@ -111,3 +111,31 @@ fn linked_files_and_other_mime_types_stay_unresolved() { ); assert_eq!(layers(data.as_bytes(), Limits::default()).unwrap(), layers(svg("").as_bytes(), Limits::default()).unwrap()); } + +#[test] +fn a_document_compressed_twice_is_refused_not_unpacked_again() { + // One gzip layer is unpacked within the budget; usvg would unpack a second one without any. + let twice = gzip(&gzip(svg(&" ".repeat(1 << 20)).as_bytes())); + let e = layers(&twice, Limits { document: 4 << 20, ..Limits::default() }).unwrap_err(); + assert!(e.contains("another compressed file"), "{e}"); + // Embedded, the same is dropped. + let inner = svg(r##""##); + let data = svg(&format!( + r##""##, + base64::engine::general_purpose::STANDARD.encode(gzip(&gzip(inner.as_bytes()))) + )) + .into_bytes(); + assert_eq!(layers(&data, Limits::default()).unwrap(), layers(svg("").as_bytes(), Limits::default()).unwrap()); +} + +#[test] +fn a_refused_embedded_bomb_uses_up_the_budget_for_the_images_after_it() { + let bomb = gzip(svg(&" ".repeat(1 << 20)).as_bytes()); + let inner = svg(r##""##); + let image = |href: String| format!(r##""##); + let bomb_uri = format!("data:image/svg+xml;base64,{}", base64::engine::general_purpose::STANDARD.encode(&bomb)); + let data = svg(&format!("{}{}", image(bomb_uri).repeat(8), image(uri(inner.as_bytes())))).into_bytes(); + let limits = Limits { embedded: 4096, ..Limits::default() }; + let empty = layers(svg("").as_bytes(), limits).unwrap(); + assert_eq!(layers(&data, limits).unwrap(), empty, "the first bomb spends the allowance; nothing after it unpacks"); +}