diff --git a/Changelog.md b/Changelog.md index 0b56fe07..fb25c121 100644 --- a/Changelog.md +++ b/Changelog.md @@ -16,6 +16,9 @@ ### New Features +- [#978]: Add `Deserializer::recursion_limit` to bound the recursion depth + of the serde `Deserializer` (defaults to 128, matching `serde_json`). + ### Bug Fixes - [#977]: `NamespaceResolver::push` (and hence every `NsReader` `Start`/`Empty` @@ -24,10 +27,21 @@ `u16` depth counter. Previously the unguarded `nesting_level += 1` panicked under `overflow-checks` builds and silently wrapped in release, corrupting namespace-scope bookkeeping on deeply nested untrusted input. +- [#978]: The serde `Deserializer` now returns the new + `DeError::TooDeeplyNested` instead of overflowing the native call stack + and aborting the process when deserializing a struct, a `Vec`-shaped + sequence of structs, or an enum variant nested deeper than the configured + recursion limit. Previously none of `deserialize_struct`, `deserialize_seq`, + or `deserialize_enum` bounded the recursion depth of their respective + re-entrant `MapAccess`/`SeqAccess`/`EnumAccess`/`VariantAccess` + implementations, so a maliciously deeply nested (but otherwise + well-formed) untrusted XML document could crash the whole process in a + way that could not be caught as a `Result::Err`. ### Misc Changes [#977]: https://github.com/tafia/quick-xml/issues/977 +[#978]: https://github.com/tafia/quick-xml/issues/978 ## 0.41.0 -- 2026-06-29 diff --git a/src/de/map.rs b/src/de/map.rs index 0a73e0ae..85b62c95 100644 --- a/src/de/map.rs +++ b/src/de/map.rs @@ -5,7 +5,7 @@ use crate::{ de::resolver::EntityResolver, de::simple_type::SimpleTypeDeserializer, de::text::TextDeserializer, - de::{DeEvent, Deserializer, XmlRead, TEXT_KEY, VALUE_KEY}, + de::{DeEvent, DepthGuard, Deserializer, XmlRead, TEXT_KEY, VALUE_KEY}, errors::serialize::DeError, errors::Error, events::attributes::IterState, @@ -173,7 +173,13 @@ where { /// Tag -- owner of attributes start: BytesStart<'de>, - de: &'d mut Deserializer<'de, R, E>, + /// A guard that both grants access to the deserializer and bounds the + /// recursion depth for as long as this element's map/struct scope is + /// alive (dropping it, e.g. when this `ElementMapAccess` is dropped, + /// releases one level of the recursion-depth budget). See + /// [`Deserializer::recursion_limit`] and + /// . + de: DepthGuard<'d, 'de, R, E>, /// State of the iterator over attributes. Contains the next position in the /// inner `start` slice, from which next attribute should be parsed. iter: IterState, @@ -200,13 +206,21 @@ where R: XmlRead<'de>, E: EntityResolver, { - /// Create a new ElementMapAccess + /// Create a new ElementMapAccess. + /// + /// Returns [`DeError::TooDeeplyNested`] instead of constructing the + /// accessor if doing so would exceed the deserializer's configured + /// recursion-depth limit -- every struct/map scope opened here can, via + /// a recursive user type, re-enter this constructor, and each level adds + /// unbounded native call-stack frames otherwise (see + /// ). pub fn new( de: &'d mut Deserializer<'de, R, E>, start: BytesStart<'de>, fields: &'static [&'static str], - ) -> Self { - Self { + ) -> Result { + let de = de.enter_depth()?; + Ok(Self { de, iter: IterState::new(start.name().as_ref().len(), false), start, @@ -214,7 +228,7 @@ where fields, has_value_field: fields.contains(&VALUE_KEY), has_text_field: fields.contains(&TEXT_KEY), - } + }) } /// Determines if subtree started with the specified event should be skipped. @@ -800,7 +814,9 @@ where V: Visitor<'de>, { match self.map.de.next()? { - DeEvent::Start(e) => visitor.visit_map(ElementMapAccess::new(self.map.de, e, fields)), + DeEvent::Start(e) => { + visitor.visit_map(ElementMapAccess::new(&mut *self.map.de, e, fields)?) + } DeEvent::Text(e) => { SimpleTypeDeserializer::from_text_content(e).deserialize_struct("", fields, visitor) } @@ -997,7 +1013,7 @@ where DeEvent::Start(start) => seed .deserialize(ElementDeserializer { start, - de: self.map.de, + de: &mut *self.map.de, }) .map(Some), // SAFETY: we just checked that the next event is Start @@ -1143,7 +1159,7 @@ where where V: Visitor<'de>, { - visitor.visit_map(ElementMapAccess::new(self.de, self.start, fields)) + visitor.visit_map(ElementMapAccess::new(self.de, self.start, fields)?) } fn deserialize_enum( diff --git a/src/de/mod.rs b/src/de/mod.rs index a64b10b7..7c39de71 100644 --- a/src/de/mod.rs +++ b/src/de/mod.rs @@ -2128,7 +2128,7 @@ use std::io::BufRead; use std::mem::replace; #[cfg(feature = "overlapped-lists")] use std::num::NonZeroUsize; -use std::ops::{Deref, Range}; +use std::ops::{Deref, DerefMut, Range}; /// Data represented by a text node or a CDATA node. XML markup is not expected pub(crate) const TEXT_KEY: &str = "$text"; @@ -2574,6 +2574,63 @@ where /// Buffer to store attribute name as a field name exposed to serde consumers key_buf: String, + + /// Current recursion depth of nested structs, sequences-of-structs and + /// enum variants. Every level of such nesting adds native call-stack + /// frames that are not otherwise bounded (see [`Self::max_depth`] and + /// ). + depth: usize, + /// Maximum allowed value of [`Self::depth`] before [`DeError::TooDeeplyNested`] + /// is returned. Configurable via [`Self::recursion_limit`]. + max_depth: usize, +} + +/// Default value of [`Deserializer::max_depth`], matching `serde_json`'s +/// default recursion limit. +const DEFAULT_RECURSION_LIMIT: usize = 128; + +/// RAII guard that decrements [`Deserializer::depth`] when dropped. Ensures +/// the recursion-depth counter stays balanced across early returns (`?`) and +/// panics unwinding through the guarded recursive call, not just the +/// successful-return path. +struct DepthGuard<'a, 'de, R, E> +where + R: XmlRead<'de>, + E: EntityResolver, +{ + de: &'a mut Deserializer<'de, R, E>, +} + +impl<'a, 'de, R, E> Drop for DepthGuard<'a, 'de, R, E> +where + R: XmlRead<'de>, + E: EntityResolver, +{ + fn drop(&mut self) { + self.de.depth -= 1; + } +} + +impl<'a, 'de, R, E> Deref for DepthGuard<'a, 'de, R, E> +where + R: XmlRead<'de>, + E: EntityResolver, +{ + type Target = Deserializer<'de, R, E>; + + fn deref(&self) -> &Self::Target { + self.de + } +} + +impl<'a, 'de, R, E> DerefMut for DepthGuard<'a, 'de, R, E> +where + R: XmlRead<'de>, + E: EntityResolver, +{ + fn deref_mut(&mut self) -> &mut Self::Target { + self.de + } } impl<'de, R, E> Deserializer<'de, R, E> @@ -2602,7 +2659,26 @@ where peek: None, key_buf: String::new(), + + depth: 0, + max_depth: DEFAULT_RECURSION_LIMIT, + } + } + + /// Checks and increments the recursion-depth counter, returning a + /// [`DepthGuard`] that decrements it again when dropped. Returns + /// [`DeError::TooDeeplyNested`] (without incrementing) if the configured + /// [`Self::max_depth`] (see [`Self::recursion_limit`]) would be exceeded. + /// + /// Must be called once per level of struct/sequence/enum nesting that + /// re-enters the deserializer, so that sibling (as opposed to nested) + /// elements do not spuriously accumulate depth. + fn enter_depth(&mut self) -> Result, DeError> { + if self.depth >= self.max_depth { + return Err(DeError::TooDeeplyNested(self.max_depth)); } + self.depth += 1; + Ok(DepthGuard { de: self }) } /// Returns `true` if all events was consumed. @@ -2716,6 +2792,46 @@ where self } + /// Set the maximum recursion depth allowed while deserializing nested + /// structs, sequences of structs (`Vec` where `T` is a struct), and + /// enum variants. + /// + /// Each level of such nesting re-enters the deserializer and adds native + /// call-stack frames that are not otherwise bounded. Deserializing a + /// maliciously deeply nested untrusted XML document can overflow the + /// native stack, which aborts the process -- this is a [DoS] that cannot + /// be caught as a [`Result::Err`], unlike ordinary parse errors. This + /// limit turns that abort into a clean [`DeError::TooDeeplyNested`]. + /// + /// Defaults to 128, matching `serde_json`'s default recursion limit. + /// + /// # Examples + /// + /// ``` + /// use quick_xml::de::Deserializer; + /// use quick_xml::DeError; + /// use serde::Deserialize; + /// + /// #[derive(Deserialize)] + /// struct Node { + /// #[serde(default, rename = "Node")] + /// node: Vec, + /// } + /// + /// let xml = "".repeat(1000) + &"".repeat(1000); + /// let mut de = Deserializer::from_str(&xml); + /// de.recursion_limit(100); + /// + /// let result = Node::deserialize(&mut de); + /// assert!(matches!(result, Err(DeError::TooDeeplyNested(100)))); + /// ``` + /// + /// [DoS]: https://en.wikipedia.org/wiki/Denial-of-service_attack + pub fn recursion_limit(&mut self, limit: usize) -> &mut Self { + self.max_depth = limit; + self + } + #[cfg(feature = "overlapped-lists")] fn peek(&mut self) -> Result<&DeEvent<'de>, DeError> { if self.read.is_empty() { @@ -3230,7 +3346,7 @@ where // When document is pretty-printed there could be whitespaces before the root element self.skip_whitespaces()?; match self.next()? { - DeEvent::Start(e) => visitor.visit_map(ElementMapAccess::new(self, e, fields)), + DeEvent::Start(e) => visitor.visit_map(ElementMapAccess::new(self, e, fields)?), // SAFETY: The reader is guaranteed that we don't have unmatched tags // If we here, then our deserializer has a bug DeEvent::End(e) => unreachable!("{:?}", e), @@ -3305,7 +3421,13 @@ where // which represents the enum variant // Checked by `top_level::list_of_enum` test in serde-de-seq self.skip_whitespaces()?; - visitor.visit_enum(var::EnumAccess::new(self)) + // Bounds recursion depth: a newtype variant wrapping the same enum + // type (`enum E { Wrap(Box) }`) re-enters this method through + // `VariantAccess::newtype_variant_seed`, adding native call-stack + // frames per level of XML nesting with no other bound. + // See https://github.com/tafia/quick-xml/issues/978 + let mut guard = self.enter_depth()?; + visitor.visit_enum(var::EnumAccess::new(&mut *guard)) } fn deserialize_seq(self, visitor: V) -> Result diff --git a/src/de/var.rs b/src/de/var.rs index e64e29f8..b5544129 100644 --- a/src/de/var.rs +++ b/src/de/var.rs @@ -132,7 +132,7 @@ where V: Visitor<'de>, { match self.de.next()? { - DeEvent::Start(e) => visitor.visit_map(ElementMapAccess::new(self.de, e, fields)), + DeEvent::Start(e) => visitor.visit_map(ElementMapAccess::new(self.de, e, fields)?), DeEvent::Text(e) => { SimpleTypeDeserializer::from_text_content(e).deserialize_struct("", fields, visitor) } diff --git a/src/errors.rs b/src/errors.rs index ad851da0..9d72a488 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -349,6 +349,18 @@ pub mod serialize { /// exceeded. The limit was provided as an argument #[cfg(feature = "overlapped-lists")] TooManyEvents(NonZeroUsize), + /// The document nests structs, sequences-of-structs, or enum variants + /// more deeply than the configured recursion limit allows. + /// + /// Each level of such nesting adds native call-stack frames while + /// deserializing (through `MapAccess`/`EnumAccess`/`VariantAccess` + /// re-entering the deserializer), which are not otherwise bounded. + /// Without this limit, a sufficiently deeply nested untrusted document + /// can overflow the native stack, aborting the process in a way that + /// cannot be caught as a [`Result::Err`]. The contained value is the + /// limit that was exceeded; see + /// [`Deserializer::recursion_limit`](crate::de::Deserializer::recursion_limit). + TooDeeplyNested(usize), } impl fmt::Display for DeError { @@ -365,6 +377,11 @@ pub mod serialize { Self::UnexpectedEof => f.write_str("unexpected `Event::Eof`"), #[cfg(feature = "overlapped-lists")] Self::TooManyEvents(s) => write!(f, "deserializer buffered {} events, limit exceeded", s), + Self::TooDeeplyNested(limit) => write!( + f, + "input document is nested too deeply, recursion limit ({}) exceeded", + limit + ), } } } diff --git a/tests/serde-issues.rs b/tests/serde-issues.rs index 8f962d62..e7c5cbaa 100644 --- a/tests/serde-issues.rs +++ b/tests/serde-issues.rs @@ -867,3 +867,156 @@ fn issue928() { }, ); } + +/// Regression tests for https://github.com/tafia/quick-xml/issues/978. +/// +/// The serde `Deserializer` had no recursion-depth cap on any of its +/// internal recursive re-entry points, so a sufficiently deeply nested +/// (but otherwise well-formed) XML document would overflow the native +/// call stack and abort the whole process -- a DoS that, unlike an +/// ordinary parse error, cannot be caught as a `Result::Err`. +/// +/// Variant analysis (see the issue comments) found three independent +/// re-entry points sharing this root cause: +/// 1. the struct/map cycle described in the issue body itself -- a +/// `$value` field holding a `Vec` of the same struct +/// (`deserialize_struct` -> `ElementMapAccess` -> `next_value_seed`); +/// 2. `deserialize_seq` for an ordinary (non-`$value`) `Vec` field +/// (`MapValueSeqAccess::next_element_seed` -> `ElementDeserializer::deserialize_struct`); +/// 3. enum `newtype_variant` re-entry +/// (`VariantAccess::newtype_variant_seed` re-entering `Deserializer::deserialize_enum`). +/// +/// All three are exercised here with a small `recursion_limit()` so the +/// test stays fast; the default limit (128, matching `serde_json`) is +/// checked separately in `default_limit_is_128`. +mod issue978 { + use super::*; + use quick_xml::de::Deserializer as XmlDeserializer; + use quick_xml::DeError; + + /// Cycle 1: a struct whose `$value` field is a `Vec` of itself -- the + /// "struct-variant" shape that #819 says works correctly at shallow + /// depth, but which (before this fix) had no depth cap at all. + #[test] + fn cycle1_struct_value_vec_self() { + #[derive(Debug, Deserialize)] + #[allow(dead_code)] + struct S { + #[serde(default, rename = "$value")] + v: Vec, + } + + // Within the limit: deserializes successfully, same as before this fix. + let xml = "".repeat(5) + &"".repeat(5); + let mut de = XmlDeserializer::from_str(&xml); + de.recursion_limit(8); + assert!(S::deserialize(&mut de).is_ok()); + + // Past the limit: a clean, catchable error -- before this fix, a + // sufficiently large depth here aborted the process instead + // (confirmed on master with depth 20_000). + let xml = "".repeat(50) + &"".repeat(50); + let mut de = XmlDeserializer::from_str(&xml); + de.recursion_limit(8); + assert!(matches!( + S::deserialize(&mut de), + Err(DeError::TooDeeplyNested(8)) + )); + } + + /// Cycle 2 (from the variant-analysis comment): a plain (non-`$value`) + /// `Vec` field, which recurses through `deserialize_seq` and + /// `ElementDeserializer::deserialize_struct` -- a different code path + /// than cycle 1, so it needs its own guard. + #[test] + fn cycle2_deserialize_seq_vec_self() { + #[derive(Debug, Deserialize)] + #[allow(dead_code)] + struct Node { + #[serde(default, rename = "Node")] + node: Vec, + } + + let xml = "".repeat(5) + &"".repeat(5); + let mut de = XmlDeserializer::from_str(&xml); + de.recursion_limit(8); + assert!(Node::deserialize(&mut de).is_ok()); + + // Before this fix, a sufficiently large depth here aborted the + // process instead (confirmed on master with depth 10_000). + let xml = "".repeat(50) + &"".repeat(50); + let mut de = XmlDeserializer::from_str(&xml); + de.recursion_limit(8); + assert!(matches!( + Node::deserialize(&mut de), + Err(DeError::TooDeeplyNested(8)) + )); + } + + /// Cycle 3 (from the variant-analysis comment): an enum newtype variant + /// wrapping itself. + /// + /// Note this shares its root cause with the already-filed #819: because + /// `EnumAccess::variant_seed` only *peeks* the `Start` event and + /// `VariantAccess::newtype_variant_seed` re-enters `deserialize_enum` + /// without consuming it either, the reader position never advances + /// across the recursive call. That means this shape does not actually + /// need *deep* nesting to blow the stack: it fails to terminate at + /// *any* depth, including a single `` pair (independently + /// confirmed on master: still overflows with `RUST_MIN_STACK=1073741824`, + /// i.e. a 1 GiB stack). So this specific cycle is not a distinct, + /// attacker-controlled-depth DoS the way cycles 1 and 2 are -- it is + /// #819 exhibited through a bigger document. The guard added for this + /// issue still converts that non-termination from an uncatchable abort + /// into a clean, catchable error, which is why it is kept and tested + /// here. + #[test] + fn cycle3_enum_newtype_variant_self() { + #[derive(Debug, Deserialize)] + #[allow(dead_code)] + enum E { + Leaf(bool), + Wrap(Box), + } + + // A shallow, non-recursive variant still deserializes normally. + let xml = "true"; + let mut de = XmlDeserializer::from_str(xml); + de.recursion_limit(8); + assert!(E::deserialize(&mut de).is_ok()); + + // A *single* level of the self-referential newtype variant already + // never terminates (see doc comment above) -- before this fix, this + // aborted the process even with a 1 GiB stack. + let xml = ""; + let mut de = XmlDeserializer::from_str(xml); + de.recursion_limit(8); + assert!(matches!( + E::deserialize(&mut de), + Err(DeError::TooDeeplyNested(8)) + )); + } + + /// The default recursion limit (128, matching `serde_json`'s default) + /// applies even without calling `recursion_limit()` explicitly. + #[test] + fn default_limit_is_128() { + #[derive(Debug, Deserialize)] + #[allow(dead_code)] + struct Node { + #[serde(default, rename = "Node")] + node: Vec, + } + + let xml = "".repeat(128) + &"".repeat(128); + let mut de = XmlDeserializer::from_str(&xml); + assert!(Node::deserialize(&mut de).is_ok()); + + let xml = "".repeat(1000) + &"".repeat(1000); + let mut de = XmlDeserializer::from_str(&xml); + assert!(matches!( + Node::deserialize(&mut de), + Err(DeError::TooDeeplyNested(128)) + )); + } +}