Cap namespace declarations per element in NamespaceResolver::push#972
Cap namespace declarations per element in NamespaceResolver::push#972qifan-sailboat wants to merge 1 commit into
Conversation
`NsReader` calls `NamespaceResolver::push` for every `Start`/`Empty`
event *before* the event is returned to the caller. `push` previously
iterated all `xmlns` / `xmlns:*` attributes on the start tag and
allocated one `NamespaceBinding` (plus prefix/value bytes in `buffer`)
per declaration with no upper bound, so an `NsReader` consumer had no
opportunity to bound its memory exposure on untrusted input — a start
tag of M bytes drove ~3.3×M bytes of resolver heap that the caller
never saw. With several concurrent readers this is a process-fatal OOM.
Add a configurable per-element declaration limit (default 256, far
above any real-world dialect) and a new `NamespaceError::TooManyDeclarations`
variant. `push` now returns the error instead of allocating past the
limit. Expose the limit via
`NamespaceResolver::{max_declarations_per_element,set_max_declarations_per_element}`
and add `NsReader::resolver_mut()` so callers can raise or disable it.
Fixes tafia#970.
Co-Authored-By: Claude <noreply@anthropic.com>
dralley
left a comment
There was a problem hiding this comment.
Reasonable given that we have a similar limit in place around recursive entity resolution
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #972 +/- ##
==========================================
+ Coverage 57.31% 57.39% +0.08%
==========================================
Files 46 46
Lines 18197 18242 +45
==========================================
+ Hits 10429 10470 +41
- Misses 7768 7772 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Mingun
left a comment
There was a problem hiding this comment.
The only one small nit about link in Changelog.md. I can made that change myself when I go home. Otherwise everything is good.
| via `NamespaceResolver::set_max_declarations_per_element` (use `usize::MAX` | ||
| to disable). | ||
|
|
||
| [#970]: https://github.com/tafia/quick-xml/issues/970 |
There was a problem hiding this comment.
We put links at the end of version section (here you put it at the end of Bug Fixes section). Could you please move it below (keep the 2 blank lines before the next ## section)
Fixes #970.
NsReadercallsNamespaceResolver::pushfor everyStart/Emptyevent before the event is returned to the caller.pushpreviously iterated allxmlns/xmlns:*attributes on the start tag and allocated oneNamespaceBinding(plus prefix/value bytes inbuffer) per declaration with no upper bound, so anNsReaderconsumer had no opportunity to bound its memory exposure on untrusted input — a start tag of M bytes drove ~3.3×M bytes of resolver heap that the caller never saw. With several concurrent readers this is a process-fatal OOM (see #970 for measurements and the demonstrated downstream impact on NLnet Labs Routinator: 8 workers × ~360 MB → SIGKILL).This adds a configurable per-element declaration limit:
name::DEFAULT_MAX_DECLARATIONS_PER_ELEMENT = 256(orders of magnitude above any real-world dialect — XHTML/SVG/SOAP/RSS/RRDP all use single-digit counts — while bounding per-tag resolver heap to a few KB);NamespaceError::TooManyDeclarations(limit);pushcountsxmlns/xmlns:*declarations and returns the error instead of allocating past the limit;NamespaceResolver::{max_declarations_per_element, set_max_declarations_per_element}(usize::MAXdisables the limit);NsReader::resolver_mut()so callers can reach the setter.Adding a
NamespaceErrorvariant is technically a breaking change for callers that exhaustivelymatchon it; happy to gate this on the next minor if preferred.A regression test is included in
name::namespaces.