Closed Bug 1663836 Opened 2 years ago Closed 1 year ago

HTTP headers with spaces in the name in HTTP2 should send ERR_HTTP2_PROTOCOL_ERROR

Categories

(Core :: Networking: HTTP, defect, P3)

defect

Tracking

()

RESOLVED FIXED
93 Branch
Webcompat Priority ?
Tracking Status
firefox-esr91 --- fixed
firefox93 --- fixed

People

(Reporter: karlcow, Assigned: manuel)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file, 1 obsolete file)

So Eric Lawrence tweeted this.
https://twitter.com/ericlaw/status/1294043667154468865

Spaces in HTTP Response Header Names:
IE, Cloudflare: Sure, why not?
Chrome/Edge/Safari/Firefox/Edge Legacy (over HTTP/1.x): Pretend it's not there.
Chrome/Edge/Safari (over H2): ERR_HTTP2_PROTOCOL_ERROR
Firefox/Edge Legacy (over H2): Pretend it's not there.

Not sure if it's an issue or not but let's track it here if we need to modify it.

Severity: -- → S4
Priority: -- → P2
Whiteboard: [necko-triaged]

I took a peek at this.

We validate the header name to be a well-formed token here: https://searchfox.org/mozilla-central/rev/0dfbe5a699cc6c73cf8c14d1aa10ba10ef3ec8fa/netwerk/protocol/http/nsHttpHeaderArray.cpp#352

Spaces (ASCII 0x20) are considered invalid: https://searchfox.org/mozilla-central/rev/0dfbe5a699cc6c73cf8c14d1aa10ba10ef3ec8fa/netwerk/protocol/http/nsHttp.cpp#191

So we return NS_ERROR_FAILURE: https://searchfox.org/mozilla-central/rev/0dfbe5a699cc6c73cf8c14d1aa10ba10ef3ec8fa/netwerk/protocol/http/nsHttpHeaderArray.cpp#355

Which causes us to return early from ParseHeaderLine_locked, without setting any headers: https://searchfox.org/mozilla-central/rev/0dfbe5a699cc6c73cf8c14d1aa10ba10ef3ec8fa/netwerk/protocol/http/nsHttpResponseHead.cpp#595

Then we move on to the next line. This explains why the "malformed" header is ignored (which is what ericlaw meant by "pretend it's not there" - "it" is the entire header line, not just the space).

So I guess we need to introduce a new error code for HTTP2_PROTOCOL_ERROR (I couldn't find one already defined) and return it if parsing the header line failed.

Assumption: The code I found is the code that's used to parse headers for all http versions. (i.e. I'm looking in the right place)

Questions for which I still need to find answers:

  1. Is this protocol error supposed to be returned for any invalid token for the header name? Or just if it contains spaces? (need to check specs)
  2. What's the right way to special-case http2 in this code path and propagate the appropriate error correctly?

I found this: https://tools.ietf.org/html/rfc7540#section-8.1.2

The spec mandates that prohibited header fields[1] should cause us to bail out, but it also has language specifically about enforcing lowercase header field names.

That leads me to question my assumption that I'm looking in the right place, since there's no lowercase validation going on there. So either we're just not doing it, or we're doing it somewhere else.

[1] Header field name validation is specified in https://tools.ietf.org/html/rfc7230#section-3.2

We are parsing HTTP2 header in a different place.

The check for uppercase header field names is here:
https://searchfox.org/mozilla-central/rev/0dfbe5a699cc6c73cf8c14d1aa10ba10ef3ec8fa/netwerk/protocol/http/Http2Compression.cpp#521

you should add the check for this bug somewhere there as well.

Some notes:
HTTP2 uses HPack for header compression: https://tools.ietf.org/html/rfc7541

Note: I was able to confirm that trying to fetch a resource served over h2 completely fails on Chrome when one of the response headers' field names contains a space.

I did this by adding a custom path handler to moz-http2.js:

  else if(u.pathname === "/nhnt11") {
    res.setHeader("With Spaces", "Hello");
    res.setHeader("Without-Spaces", "World");
    res.writeHead(200);
    res.end("");
    return;
  }
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Attachment #9199892 - Attachment is obsolete: true
Assignee: nhnt11 → nobody
Status: ASSIGNED → NEW
Priority: P2 → P3
Assignee: nobody → mbucher
Pushed by mbucher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78dbbb4a72f4
Treat invalid HTTP response header names over HTTP2 as malformed r=necko-reviewers,dragana
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 93 Branch
Flags: in-testsuite+

Comment on attachment 9233298 [details]
Bug 1663836 - Treat invalid HTTP response header names over HTTP2 as malformed r=#necko

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Web compatibility RFC 7540 "Any request or response that contains a character not permitted in a header field value MUST be treated as malformed (Section 8.1.2.6)." (https://datatracker.ietf.org/doc/html/rfc7540#section-10.3)

  • User impact if declined: Non-standard-compliant Firefox behavior continues in ESR 91

  • Fix Landed on Version: 93

  • Risk to taking this patch: Low

  • Why is the change risky/not risky? (and alternatives if risky): - simple fix

  • same behavior as chrome, safari and IE
  • shouldn't cause any regressions
  • String or UUID changes made by this patch: none
Attachment #9233298 - Flags: approval-mozilla-esr91?

Comment on attachment 9233298 [details]
Bug 1663836 - Treat invalid HTTP response header names over HTTP2 as malformed r=#necko

Fixes an important webcompat issue, approved for 91.2esr.

Attachment #9233298 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
You need to log in before you can comment on or make changes to this bug.