Skip to content

Cap namespace declarations per element in NamespaceResolver::push#972

Open
qifan-sailboat wants to merge 1 commit into
tafia:masterfrom
qifan-sailboat:fix/970-nsreader-xmlns-bindings-cap
Open

Cap namespace declarations per element in NamespaceResolver::push#972
qifan-sailboat wants to merge 1 commit into
tafia:masterfrom
qifan-sailboat:fix/970-nsreader-xmlns-bindings-cap

Conversation

@qifan-sailboat

Copy link
Copy Markdown

Fixes #970.

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 (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:

  • new constant 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);
  • new error variant NamespaceError::TooManyDeclarations(limit);
  • push counts xmlns/xmlns:* declarations and returns the error instead of allocating past the limit;
  • getter/setter NamespaceResolver::{max_declarations_per_element, set_max_declarations_per_element} (usize::MAX disables the limit);
  • NsReader::resolver_mut() so callers can reach the setter.

Adding a NamespaceError variant is technically a breaking change for callers that exhaustively match on it; happy to gate this on the next minor if preferred.

A regression test is included in name::namespaces.

`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 dralley left a comment

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.

Reasonable given that we have a similar limit in place around recursive entity resolution

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 75.92593% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.39%. Comparing base (e00ae5c) to head (a80ee87).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/name.rs 80.39% 10 Missing ⚠️
src/reader/ns_reader.rs 0.00% 3 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
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     
Flag Coverage Δ
unittests 57.39% <75.92%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Mingun Mingun left a comment

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.

The only one small nit about link in Changelog.md. I can made that change myself when I go home. Otherwise everything is good.

Comment thread Changelog.md
via `NamespaceResolver::set_max_declarations_per_element` (use `usize::MAX`
to disable).

[#970]: https://github.com/tafia/quick-xml/issues/970

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.

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NamespaceResolver::push — unbounded per-xmlns heap allocation inside NsReader, before the event is returned → OOM on untrusted XML

4 participants