fix: Enforce W3C Baggage limits on the extract path - #2298
fix: Enforce W3C Baggage limits on the extract path#2298serhiy-bzhezytskyy wants to merge 7 commits into
Conversation
The inject path (#encode) applies MAX_ENTRIES, MAX_ENTRY_LENGTH and MAX_TOTAL_LENGTH, but extract parsed the inbound header with none of them, decoding an unbounded baggage header in full — the class the Java runtime tracks as CVE-2026-45292. extract now mirrors the inject limits through a private helper: over-limit entries are dropped at the point the limit is reached and already-decoded entries are kept. Adds three extract-side tests mirroring the existing inject-side limit tests. Fixes open-telemetry#2163 Assisted-By: Claude Fable 5
|
|
|
notice this still reads full header into memory |
decode_entries already stopped after 180 entries, but the header was split eagerly first, so a large header materialised every entry before any limit applied. Walk it lazily and stop at the entry limit instead. On a 7.8 MB header with 500k entries that is 180 strings instead of 500,000. Whitespace is now stripped per entry rather than over the whole header, which keeps the parsing semantics and drops a copy of the header. Empty entries are skipped explicitly: without the global gsub a whitespace-only header yields one empty entry, which raised inside decode_entries and was swallowed by the rescue. Assisted-By: Claude Fable 5
The allocation-count test covered the bound only indirectly. Extract a 1,000-entry header and a 100,000-entry one and assert the second allocates less than twice the first, so the assertion is relative and does not depend on a Ruby version's absolute counts. Assisted-By: Claude Fable 5
|
Thanks, you're right. Fixed and covered it with two tests: extract now walks the header lazily and stops at 180 entries, so nothing past the 180th is scanned or copied. The tests assert the property, that allocations do not grow with the header length; the numbers I measured separately are 180 strings instead of 500,000 and 0.04 ms instead of 53 ms on a 7.8 MB header with 500k entries. |
| return context if header.nil? || header.empty? | ||
|
|
||
| entries = header.gsub(/\s/, '').split(',') | ||
| entries = header.each_line(',', chomp: true).lazy.take(MAX_ENTRIES) |
There was a problem hiding this comment.
I think this works really well when we have a header playing by the rules, with lots of different keys of different lengths. What if we have one giant header without a separator?
Is it worth comparing the header's bytesize with MAX_TOTAL_LENGTH before diving into the entries?
There was a problem hiding this comment.
Thanks, you're right on both.
Fixed the giant header — bounded the separator search instead of scanning the whole thing: 1 GB entry with no comma measured 1s+ before, ~0.0002s now. Also switched the length checks to bytesize (spec is byte-denominated).
Fixed JRuby too — GC.stat[:total_allocated_objects] is nil there, skipped those two tests on non-MRI engines.
|
Thanks for this PR, @serhiy-bzhezytskyy! One question on the PR itself. I also noticed some JRuby tests are failing related to this PR. |
each_line(',', chomp: true) has to scan a whole comma-less stretch of
the header to find where it ends, so one oversized entry with no ','
anywhere forced a full-length scan before its size could be checked —
1s+ for a 1 GB entry. Split on ',' via a bounded window instead: never
look more than 2x MAX_ENTRY_LENGTH ahead for the next separator, so an
arbitrarily long run without one is recognized (and dropped) in
constant time.
Switched the length checks from character count to bytesize while
touching this code, matching the spec's byte-denominated limits (the
existing checks used String#size, which undercounts for multibyte
content).
Also: the two GC.stat[:total_allocated_objects]-based tests fail on
JRuby (returns nil there); skip them on non-MRI engines.
JRuby 10.1.1.0 in CI has no benchmark to require. Process.clock_gettime does the same job and needs no require, matching how this repo's own tests already measure time (span_test.rb).
3 participants
The
baggageextract path decodes the inbound header without applying theMAX_ENTRIES(180),MAX_ENTRY_LENGTH(4096) andMAX_TOTAL_LENGTH(8192) limits that the inject path (#encode) enforces, so an unbounded inbound header is parsed in full. This is the class the Java runtime fixed as CVE-2026-45292 (unbounded memory/CPU via W3C Baggage parsing, fixed in opentelemetry-java 1.62.0). Fixes #2163.extractnow enforces the same three limits through a small private helper mirroring#encode: over-limit entries are dropped where the limit is reached, and the entries decoded before that are kept. Three extract-side tests mirror the existing inject-side limit tests; each fails onmainand passes with the change.Assisted-By: Claude Fable 5