fix(header): reject double quote in HEADER_CHARS_H2 - #876
Open
choudhryfrompak wants to merge 1 commit into
Open
choudhryfrompak wants to merge 1 commit into
choudhryfrompak wants to merge 1 commit into
Conversation
PR hyperium#716 removed `"` (0x22) from HEADER_CHARS so HeaderName::from_bytes (the HTTP/1.1 path) correctly rejects it per the tchar grammar in RFC 9110 5.6.2. It missed the sibling HEADER_CHARS_H2 table, which is used by HeaderName::from_lowercase and from_static and still accepts the byte. from_lowercase is the path HTTP/2 and HTTP/3 implementations use to build header names directly from on-the-wire bytes during decode (e.g. h2's HPACK decoder calls it with no other charset check), so a remote peer could get a literal '"' embedded in a header name that the HTTP/1.1 path would have rejected outright. Add a regression test and align HEADER_CHARS_H2 with HEADER_CHARS at that index; the two tables are otherwise intentionally identical except for case folding (from_bytes lower-cases ASCII letters, from_lowercase rejects uppercase).
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This finishes what #716 started. That PR removed
"(0x22) from HEADER_CHARS soHeaderName::from_bytesrejects a double quote in a header name, per the tchar grammar in RFC 9110 5.6.2. It only touched HEADER_CHARS though, and missed the sibling table, HEADER_CHARS_H2, which still maps 0x22 to itself instead of 0.HEADER_CHARS_H2 is what
HeaderName::from_lowercaseandHeaderName::from_staticuse.from_lowercaseis the one that matters here: it's the path HTTP/2 and HTTP/3 stacks use to build a HeaderName straight from on-the-wire bytes during decode, with no other charset check in front of it. Concretely, h2's HPACK decoder calls it inHeader::new(src/hpack/header.rs) on raw header-name bytes pulled off the wire. So right now a remote HTTP/2 peer can get a literal"into a header name that the HTTP/1.1 path (from_bytes) would have rejected outright. It's not a crash or memory-safety bug, just a case where the two decode paths disagree on what's a valid header name, which is exactly the kind of inconsistency that leads to request smuggling / splitting style bugs further down a proxy chain when one hop validates differently than another.I checked HEADER_CHARS_H2 against HEADER_CHARS byte by byte to see if there was more divergence than the one documented index. There isn't — the only other differences are the 26 uppercase ASCII letters, which is intentional: from_bytes lower-cases uppercase input, from_lowercase rejects it (the doc example on from_lowercase already shows this). So 0x22 was the one real bug.
Fix is the same one-line table edit #716 made, just on the other table, plus a regression test on from_lowercase since #716 didn't add one anywhere (that's arguably how this got missed in the first place).
Tested:
cargo testpasses (fmt and clippy clean on my end, modulo a pre-existing clippy warning on current stable that's unrelated to this change and present on master too). I also pulled a local h2 checkout, pointed its http dependency at this patched crate, and ran h2's hpack tests and its full lib test suite — both pass clean, so nothing in h2 depends on the old accept-quote behavior.