1920444 - Reject \r %xd and \n %xa in Set-Cookie header. nsHttpHeaderArray should not merge headers with \n

Status: NEW

(Core :: Networking: Cookies, defect, P2)

Description

[Valentin Gosu [:valentin]]

An attempt to reject it was made in bug 1892748, but that caused a regression because we use \n to join multiple Set-Cookie headers together here

I'm wondering if that means joining header lines using \r\n instead of just \n.
This would of course require changes to the cookieParser but that's probably unavoidable.

We should also turn bug 1901325 comment 12 into a test for this bug to avoid creating another cookie incident.

---

Another thing to consider is whether the API for nsHttpHeaderArray should return both Array of strings, to get the original individual values of the headers, and the current one where headers are merged. The additional processing and merging seems to cause issues for some consumers such as bug 1915233.

Comment 1

[edgul]

Note that we already have Bug 1903400. Could you update the title and description of this bug accordingly?

Comment 2

[edgul]

Just noting the failing WPTs:
https://wpt.fyi/results/cookies/name/name-ctl.html?label=experimental&label=master&aligned
https://wpt.fyi/results/cookies/value/value-ctl.html?label=experimental&label=master&aligned

Comment 3

[Andrea Marchesini [:baku]]

Note that the header merging is just part of the problem. Imagine this scenario:

• The server response is the following: [...other headers...]\r\nSetCookie: a=\nb\r\n
• We split the headers by line: https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpTransaction.cpp#2179-2187
• The lines are: [ ..., SetCookie: a=, b, ... ]
• With the current implementation of nsHttpTransaction.cpp, there is no way we can process a cookie header with \n correctly.

Comment 4

[edgul]

Is the \n in a=\nb expected to be treated as 0x0A in this case?
Because if so, the server SHOULDN'T send a cookie value with that: https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis#section-4.1.1-2
and the client is not expected to parse it: https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis#section-5.6-7.1.1

Comment 5

[Andrea Marchesini [:baku]]

This is before the cookie parsing. This is HTTP. RFC9110:

https://www.rfc-editor.org/rfc/rfc9110#name-field-values
"Field values containing CR, LF, or NUL characters are invalid and dangerous, due to the varying ways that implementations might parse and interpret those characters; a recipient of CR, LF, or NUL within a field value MUST either reject the message or replace each of those characters with SP before further processing or forwarding of that message"

After a correct header parsing, if the input was: Set-Cookie: a\nb=c\r\n, the CookieParser receives a b=c. The final cookie will have name a b and value c.

Comment 6

[edgul]

So should we be looking at removing the WPT's for this, since the tests seem to be malformed?
Chrome is also failing, though Safari is somehow passing. I haven't yet found how they are parsing HTTP differently.

Comment 7

[Valentin Gosu [:valentin]]

I don't think the tests are malformed.
See https://issues.chromium.org/issues/40191620#comment36
and more specifically https://issues.chromium.org/issues/346430311

The problem is that browsers (at least in HTTP/1.1) treat a lone CR or LF as a header separator whereas RFC 9110 indicates that is malformed.
I assume Safari doesn't split headers by lone CR, which is why they are able to pass that test (because the CR ends up in the header) and it can be rejected.

Comment 8

[edgul]

and more specifically https://issues.chromium.org/issues/346430311

These comments suggest potential breakage. Is there any reason to believe we won't cause breakage if we implement something similar to Safari?

Comment 9

[Valentin Gosu [:valentin]]

https://issues.chromium.org/issues/346430311
Not sure about individual CRs, but I believe individual LFs are supported by all browsers, so removing support for them will likely result in a lot of breakage, though as more an more traffic is H2/H3 now-a-days, probably substantially less than there would have been 10 years ago.

As the comment says, all browsers allow individual LFs.
If we want to align with Safari we should add a pref that doesn't split on lone CRs.

We might explore not splitting headers on lone LFs in the future, but it is quite a risky endeavor, and not something Gecko should do on its own.

Comment 10

[edgul]

Removing from [next] until we can stomach the risk mentioned in Comment 9