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

Status: RESOLVED FIXED

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

Description

[Karl Dubost💡 :karlcow]

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.

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

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?

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

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

Comment 3

[Dragana Damjanovic [:dragana]]

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

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

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;
  }

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1663836 - HTTP headers with spaces in the name in HTTP2 should send ERR_HTTP2_PROTOCOL_ERROR. r=#necko

Comment 6

[Karl Dubost💡 :karlcow]

Some info
https://github.com/httpwg/http2-spec/issues/815
https://github.com/httpwg/http2-spec/pull/866

Comment 7

[Manuel Bucher [:manuel]]

Attachment: Bug 1663836 - Treat invalid HTTP response header names over HTTP2 as malformed r=#necko

Relevant section in the spec: https://datatracker.ietf.org/doc/html/rfc7540#section-10.3

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78dbbb4a72f4
Treat invalid HTTP response header names over HTTP2 as malformed r=necko-reviewers,dragana

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/78dbbb4a72f4

Comment 10

[Manuel Bucher [:manuel]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/38b53a0a4a4e