Enforce W3C Baggage size and entry-count caps in Parser#1975
Conversation
The W3C Baggage specification recommends a max of 8192 total bytes and
180 list-members. `Parser::parseInto` walked the entire
`explode(',', $header)` with no cap on header byte size, no cap on
entry count, and no per-entry length cap, allowing a single inbound
HTTP request with an oversized `baggage` header to consume linear
CPU/memory in PHP-FPM workers.
This change adds two private constants `MAX_BAGGAGE_BYTES` (8192) and
`MAX_BAGGAGE_ENTRIES` (180), rejects the whole header up-front when
`strlen($header) > MAX_BAGGAGE_BYTES`, and breaks the parse loop once
the entry cap is reached. Mirrors the inject-side caps already in the
go, dotnet, cpp, and (post-1.62.0) java SDKs.
Background context: GHSA-7f5h-mpj3-v3hf (closed advisory; treated as a
hardening bug per the project security policy).
Refs: https://www.w3.org/TR/baggage/#limits
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1975 +/- ##
============================================
- Coverage 67.73% 67.73% -0.01%
- Complexity 3044 3046 +2
============================================
Files 459 459
Lines 8883 8891 +8
============================================
+ Hits 6017 6022 +5
- Misses 2866 2869 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
I had a couple of non-blockers, adding here in case you want to fast-follow @tonghuaroot:
|
|
Thanks for the thoughtful review @bobstrecansky! I've opened a fast-follow to pick up your suggestions: #1976.
|
Follow-up to #1975. Pins the exact off-by-one behaviour of the W3C Baggage caps that the existing tests (1024 pairs / 500 pairs) only exercised well past the limits: - byte cap is inclusive: 8192 bytes accepted, 8193 bytes rejected - list-member cap: 180 entries accepted, 181st dropped Also documents that $entries counts only ACCEPTED pairs (incremented at the bottom of the loop), not raw comma-separated list-members, so a future change does not silently move the counter and alter semantics. Signed-off-by: tonghuaroot <[email protected]>
2 participants
Fixes #1974.
What
BaggagePropagator::extractwalked the entireexplode(',', $header)result with no cap on header byte size, no cap on entry count, and no
per-entry length cap. This change adds two private constants to
Parserand enforces them insideparseInto.Change
Parser::MAX_BAGGAGE_BYTES = 8192Parser::MAX_BAGGAGE_ENTRIES = 180parseIntoreturns early whenstrlen($header) > MAX_BAGGAGE_BYTES(cheap, prevents the per-entryallocation loop running at all).
parseIntobreaks onceMAX_BAGGAGE_ENTRIESaccepted entrieshave been processed.
Values are taken from the W3C Baggage spec
(
#limits) and match whatthe other SDKs already do:
Tests
Two regression tests added in
tests/Unit/API/Baggage/Propagation/ParserTest.php:test_parse_into_rejects_header_exceeding_w3c_byte_limitbuilds a~10 KiB header from 1024 pairs and asserts the builder is never
invoked.
test_parse_into_caps_entries_at_w3c_list_member_limitbuilds a500-pair header (under 8192 bytes) and asserts the builder receives
exactly 180
setcalls.vendor/bin/phpunit tests/Unit/API/Baggagepasses 63/63 locally onPHP 8.5.6.
Static analysis
vendor/bin/phpstan analyse src/API/Baggage/Propagation/Parser.phpno errors.
vendor-bin/rector/vendor/bin/rector process ... --dry-runnochanges.
vendor-bin/php-cs-fixer/vendor/bin/php-cs-fixer fix ... --dry-runno changes.
Context
Originally filed as a private security advisory
(GHSA-7f5h-mpj3-v3hf,
now closed). Per the project security policy the maintainers prefer
to handle this through the standard issue/PR workflow as a hardening
bug.
References