From 2f078962a0aa2d4692d04f6485d4a1aa6327c85e Mon Sep 17 00:00:00 2001 From: pkibiego Date: Sun, 11 Oct 2026 03:39:32 +0300 Subject: [PATCH] xlsx: bump quick-xml to 0.41 for the namespace-declaration DoS (RUSTSEC-2026-0195) The ODS and XLSB readers parse untrusted package metadata and content through `quick_xml::NsReader`. In 0.38.4 that is RUSTSEC-2026-0195 (CVSS 7.5): the namespace resolver pushes a binding for every `xmlns` / `xmlns:*` declaration on a start tag, with no cap on how many there are, and `NsReader::read_event` reaches that path - not only `read_resolved_event`. None of our caps bound it. MAX_PART (512 MiB), MAX_TOTAL (1 GiB), MAX_ENTRIES, MAX_CELLS, MAX_TEXT and MAX_DEPTH bound part size, entry count, cells, text and nesting, never declarations per tag. Measured with `gridcraft-cli info` on crafted .ods files, before this change: 754 B, 0 declarations -> 8.6 MB, exit 0 24 KB, 1,000,000 -> 52.6 MB, exit 0 187 KB, 8,000,000 -> 367.8 MB, exit 0 770 KB, 33,000,000 -> 1.46 GB, exit 0 About 45 bytes of resident memory per declaration, roughly 1,900x the file size, and no error at all. At MAX_PART a ~1 MB file reaches ~1.9 GB, and every part is held in memory while the package is read. Both readers share the path, so a downloaded .ods or .xlsb was enough. Nothing panicked, so `Session::execute` never saw it. 0.41.0 caps declarations per start tag at 256 (DEFAULT_MAX_DECLARATIONS_PER_ELEMENT, raisable through NamespaceResolver::set_max_declarations_per_element) and returns `NamespaceError::TooManyDeclarations` beyond it. The same fixtures after: 754 B -> 8.6 MB, ok 24 KB -> 19 MB, error 187 KB -> 101.8 MB, error 770 KB -> 397 MB, error with "start tag declares more than 256 namespace bindings". The residual peak is the part itself: MAX_PART still lets one part decompress to 256 MiB, the existing intended bound. API changes for 0.41 (0.x bumps may break; all four are in the xlsx crate): - `resolve_element` and `resolve_attribute` moved off `NsReader` on to the resolver: `reader.resolver_mut().resolve_element(..)`. `attr` and `tag` in ods.rs take `&mut NsReader` for that. - `Attribute::unescape_value` is deprecated in favour of `normalized_value`, which also applies the attribute-value normalization the XML specification requires. - `BytesText::xml_content` now takes the document's XML version. This also clears RUSTSEC-2026-0194, the quadratic duplicate-attribute check. xml.rs already passed `with_checks(false)`, but ods.rs and xlsb.rs used the checked iterator, so they were on the quadratic path as well. Regression test: `ods_tests::ods_namespace_declaration_bomb_is_rejected` asserts that a tag with thousands of declarations is an error and that a document with a handful still reads. It fails on 0.38.4 (the tag is accepted) and passes on 0.41.0. Verified: `cargo xtask ci`; the full xlsx suite (80 tests) plus the malformed-input tests; and the four measurements above, before and after, with no panics either way. --- Cargo.lock | 13 ++----------- Cargo.toml | 2 +- crates/xlsx/src/ods.rs | 23 ++++++++++++----------- crates/xlsx/src/tests/ods_tests.rs | 16 ++++++++++++++++ crates/xlsx/src/xlsb.rs | 7 ++++--- crates/xlsx/src/xml.rs | 5 +++-- 6 files changed, 38 insertions(+), 28 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f772f83..16e2221 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1584,7 +1584,7 @@ dependencies = [ "gridcraft-core", "gridcraft-formula", "gridcraft-model", - "quick-xml 0.38.4", + "quick-xml", "thiserror 2.0.21", "zip", ] @@ -2739,15 +2739,6 @@ version = "2.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a993555f31e5a609f617c12db6250dedcac1b0a85076912c436e6fc9b2c8e6a3" -[[package]] -name = "quick-xml" -version = "0.38.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b66c2058c55a409d601666cffe35f04333cf1013010882cec174a7467cd4e21c" -dependencies = [ - "memchr", -] - [[package]] name = "quick-xml" version = "0.41.0" @@ -3725,7 +3716,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "338e30461b3a2b67d70eb30a6d89f8e0c93a833e07d2ae89085cd070c4a00ac0" dependencies = [ "proc-macro2", - "quick-xml 0.41.0", + "quick-xml", "quote", ] diff --git a/Cargo.toml b/Cargo.toml index d7fad0f..d826390 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ serde_json = { version = "1", features = ["float_roundtrip"] } log = "0.4" rayon = "1" regex = { version = "1", default-features = false, features = ["std", "unicode", "perf"] } -quick-xml = "0.38" +quick-xml = "0.41" zip = { version = "3", default-features = false, features = ["deflate"] } image = { version = "0.25", default-features = false, features = ["png", "jpeg"] } diff --git a/crates/xlsx/src/ods.rs b/crates/xlsx/src/ods.rs index 4f63fd1..8415ce3 100644 --- a/crates/xlsx/src/ods.rs +++ b/crates/xlsx/src/ods.rs @@ -10,6 +10,7 @@ use gridcraft_core::date::{days_in_month, serial_from_ymd}; use gridcraft_core::{CellRef, DateSystem, MAX_COLS, MAX_ROWS, RangeRef, Value}; use gridcraft_model::{Cell, NumFmt, Sheet, StyleId, Workbook}; use quick_xml::NsReader; +use quick_xml::XmlVersion; use quick_xml::events::{BytesStart, Event}; use quick_xml::name::ResolveResult; @@ -67,13 +68,13 @@ fn is_ns(ns: ResolveResult<'_>, expected: &[u8]) -> bool { matches!(ns, ResolveResult::Bound(n) if n.as_ref() == expected) } -fn attr(reader: &NsReader<&[u8]>, e: &BytesStart<'_>, ns: &[u8], name: &[u8]) -> Result, IoError> { +fn attr(reader: &mut NsReader<&[u8]>, e: &BytesStart<'_>, ns: &[u8], name: &[u8]) -> Result, IoError> { let mut found = None; for a in e.attributes() { let a = a.map_err(|e| IoError::Xml(e.to_string()))?; - let (resolved, local) = reader.resolve_attribute(a.key); + let (resolved, local) = reader.resolver_mut().resolve_attribute(a.key); if is_ns(resolved, ns) && local.as_ref() == name { - let value = a.unescape_value().map_err(|e| IoError::Xml(e.to_string()))?; + let value = a.normalized_value(XmlVersion::Explicit1_0).map_err(|e| IoError::Xml(e.to_string()))?; if value.len() > MAX_CELL_TEXT { return Err(IoError::TooLarge("ODS attribute exceeds 1 MiB".into())); } @@ -106,7 +107,7 @@ fn check_manifest(bytes: &[u8]) -> Result<(), IoError> { let event = reader.read_event().map_err(xml_error)?; match &event { Event::Start(e) | Event::Empty(e) => { - let (ns, name) = reader.resolve_element(e.name()); + let (ns, name) = reader.resolver_mut().resolve_element(e.name()); if is_ns(ns.clone(), MANIFEST) && name.as_ref() == b"encryption-data" { return Err(IoError::Format("encrypted ODS files are not supported".into())); } @@ -153,8 +154,8 @@ enum Tag { Other, } -fn tag(reader: &NsReader<&[u8]>, e: &BytesStart<'_>) -> Tag { - let (ns, name) = reader.resolve_element(e.name()); +fn tag(reader: &mut NsReader<&[u8]>, e: &BytesStart<'_>) -> Tag { + let (ns, name) = reader.resolver_mut().resolve_element(e.name()); let local = name.as_ref(); if is_ns(ns.clone(), OFFICE) { return match local { @@ -253,23 +254,23 @@ impl Import { return Err(xml_error("XML nested too deeply")); } if self.stack.is_empty() { - if root_seen || tag(&reader, &e) != Tag::Document { + if root_seen || tag(&mut reader, &e) != Tag::Document { return Err(xml_error("expected one office:document-content root")); } root_seen = true; } - self.start(&reader, &e)?; + self.start(&mut reader, &e)?; } Event::Empty(e) => { if self.stack.is_empty() { return Err(xml_error("expected office:document-content with a spreadsheet")); } - self.start(&reader, &e)?; + self.start(&mut reader, &e)?; self.end()?; } Event::End(_) => self.end()?, Event::Text(t) => { - let text = t.xml_content().map_err(xml_error)?; + let text = t.xml_content(XmlVersion::Explicit1_0).map_err(xml_error)?; if self.stack.is_empty() && !text.trim().is_empty() { return Err(xml_error("text outside the document")); } @@ -301,7 +302,7 @@ impl Import { Ok(()) } - fn start(&mut self, reader: &NsReader<&[u8]>, e: &BytesStart<'_>) -> Result<(), IoError> { + fn start(&mut self, reader: &mut NsReader<&[u8]>, e: &BytesStart<'_>) -> Result<(), IoError> { let t = tag(reader, e); let depth = self.stack.len(); match t { diff --git a/crates/xlsx/src/tests/ods_tests.rs b/crates/xlsx/src/tests/ods_tests.rs index c0f2e0e..47c2586 100644 --- a/crates/xlsx/src/tests/ods_tests.rs +++ b/crates/xlsx/src/tests/ods_tests.rs @@ -23,6 +23,22 @@ fn text(s: &str) -> Value { Value::Text(s.into()) } +/// A start tag with thousands of `xmlns:` declarations is refused instead of allocated +/// (RUSTSEC-2026-0195: the resolver kept a binding per declaration, with no cap). +#[test] +fn ods_namespace_declaration_bomb_is_rejected() { + let bomb = |n: usize| -> String { (0..n).map(|i| format!(" xmlns:p{i}=\"urn:p{i}\"")).collect() }; + let doc = |decls: &str| { + format!( + r#"1"# + ) + }; + let build = |decls: &str| make_zip(&[("mimetype", MIME), ("META-INF/manifest.xml", MANIFEST), ("content.xml", doc(decls).as_bytes())]); + let ok = read_ods(&build(&bomb(8))); + assert!(ok.is_ok(), "a handful of namespaces still reads: {:?}", ok.err()); + assert!(read_ods(&build(&bomb(10_000))).is_err(), "a tag with thousands of declarations is refused"); +} + #[test] fn ods_typed_values_and_formula_caches_are_constants_and_survive_xlsx() { let bytes = row(r#" diff --git a/crates/xlsx/src/xlsb.rs b/crates/xlsx/src/xlsb.rs index 897a40b..3c84547 100644 --- a/crates/xlsx/src/xlsb.rs +++ b/crates/xlsx/src/xlsb.rs @@ -7,6 +7,7 @@ use std::sync::Arc; use gridcraft_core::{CellError, CellRef, DateSystem, MAX_COLS, MAX_ROWS, RangeRef, Value}; use gridcraft_model::{Cell, Sheet, Visibility, Workbook}; use quick_xml::NsReader; +use quick_xml::XmlVersion; use quick_xml::events::Event; use quick_xml::name::ResolveResult; @@ -160,7 +161,7 @@ fn metadata(bytes: &[u8], ns: &[u8], root_name: &[u8]) -> Result, IoE let event = reader.read_event().map_err(|e| IoError::Xml(e.to_string()))?; match &event { Event::Start(e) | Event::Empty(e) => { - let (namespace, local) = reader.resolve_element(e.name()); + let (namespace, local) = reader.resolver_mut().resolve_element(e.name()); let valid_ns = matches!(namespace, ResolveResult::Bound(n) if n.as_ref() == ns); if depth == 0 { if seen || !valid_ns || local.as_ref() != root_name { @@ -174,7 +175,7 @@ fn metadata(bytes: &[u8], ns: &[u8], root_name: &[u8]) -> Result, IoE if !a.key.as_ref().contains(&b':') { attrs.push(( String::from_utf8_lossy(a.key.as_ref()).into_owned(), - a.unescape_value().map_err(|e| IoError::Xml(e.to_string()))?.into_owned(), + a.normalized_value(XmlVersion::Explicit1_0).map_err(|e| IoError::Xml(e.to_string()))?.into_owned(), )); } } @@ -188,7 +189,7 @@ fn metadata(bytes: &[u8], ns: &[u8], root_name: &[u8]) -> Result, IoE } } Event::End(_) => depth = depth.checked_sub(1).ok_or_else(|| invalid("unexpected metadata end"))?, - Event::Text(t) if depth == 0 && !t.xml_content().map_err(|e| IoError::Xml(e.to_string()))?.trim().is_empty() => { + Event::Text(t) if depth == 0 && !t.xml_content(XmlVersion::Explicit1_0).map_err(|e| IoError::Xml(e.to_string()))?.trim().is_empty() => { return Err(invalid("text outside package metadata")); } Event::DocType(_) => return Err(invalid("DOCTYPE in package metadata is unsupported")), diff --git a/crates/xlsx/src/xml.rs b/crates/xlsx/src/xml.rs index efa5cdc..9e03f5f 100644 --- a/crates/xlsx/src/xml.rs +++ b/crates/xlsx/src/xml.rs @@ -2,6 +2,7 @@ //! shared strings) so large parts never need a full tree, plus escaping helpers for writing. use quick_xml::Reader; +use quick_xml::XmlVersion; use quick_xml::events::{BytesStart, Event}; use crate::IoError; @@ -124,7 +125,7 @@ fn start_el(e: &BytesStart<'_>) -> El { if key == b"xmlns" || key.starts_with(b"xmlns:") { continue; } - let v = match a.unescape_value() { + let v = match a.normalized_value(XmlVersion::Explicit1_0) { Ok(v) => v.into_owned(), Err(_) => String::from_utf8_lossy(&a.value).into_owned(), }; @@ -203,7 +204,7 @@ pub fn parse_streaming(xml: &[u8], split: &[&str], cb: &mut dyn FnMut(El) -> Res } Event::Text(t) => { if let Some(top) = stack.last_mut() { - let s = t.xml_content().map_err(|e| IoError::Xml(e.to_string()))?; + let s = t.xml_content(XmlVersion::Explicit1_0).map_err(|e| IoError::Xml(e.to_string()))?; top.text.push_str(&s); } }