1802057 - (CVE-2023-5723) Firefox accepts 0x7F in cookie names (and others?)

Status: RESOLVED FIXED

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

Description

[Daniel Veditz [:dveditz]]

+++ This bug was initially created as a clone of Bug #1797235 +++

bug 1797235 fixed an issue where the values of cookies set via document.cookie were not checked for invalid control characters. I happened to notice in the patch that nearby we have a completely separate blacklist of characters for CookieCommons::CheckName(), and that this list is missing 0x7F which is clearly forbidden by the spec. This name check is used for both HTTP and non-HTTP (scripted) cookies, unlike bug 1797235 which was limited to non-HTTP cookies. As an example, the following is allowed in Firefox, and breaks sites like https://instagram.com. It would also be allowed via HTTP Set-Cookie: assuming there's no generic Necko code that would reject the whole header before calling the cookie-specific code (haven't tested).

  document.cookie = "a\x7Fa=test";

We might want to reject additional characters. If we've gotten far enough to think we have a cookie name, and yet there's a semi-colon (0x3B) in it that's a pretty impossible state. We should think hard about why we don't use the same set of illegal characters for name and value.

But at the very least we have to add 0x7F to the forbidden list.

Comment 1

[Daniel Veditz [:dveditz]]

An argument against combining the bad-lists used by the two checks is that we probably should follow the stricter section 4 rules for cookie names, which means also disallow 0x22 (dquote), 0x2C (comma), 0x3B (semi-colon), and 0x5C (backslash). We know we do want to allow 3/4 of those in the cookie value for compat reasons, but it's easy to imagine server code getting tripped up by those in the name and doing the wrong thing.

I poked a few servers and didn't see anything outright broken by those characters, but I would expect those to cause more subtle internal app logic errors. I had my hopes up for the backslash triggering obvious problems, but no dice. The dquote seemed to trigger some weird parsing as a "value" instead of a name, but I'm not sure if that was really on the server end or maybe on the Firefox side (maybe even just in the DevTools display code). The control characters triggered very obvious rejection from the front-line HTTP handling code which made it more "useful" as a DOS attack, but also meant we don't have to worry about them contributing to more subtle site-specific logic errors.

The way we parse cookie strings we don't normally have to worry about finding a 0x3B in the name or value since that's our primary delimiter. But there's a direct "make a cookie" API exposed to extensions with the right permissions. It's good we're checking for this character currently in the cookie-value badlist, but we definitely also need it in the cookie-name badlist, too. Less likely, the cookie storage might be corrupt or tampered with and a misplaced ';' might come from there, too.

Comment 2

[haxatron1]

If it helps, this is the relevant code for Chromium: https://chromium.googlesource.com/chromium/src/+/refs/heads/main/net/cookies/parsed_cookie.cc#469

And the HttpUtil::IsControlChar:
https://chromium.googlesource.com/chromium/src/+/refs/heads/main/net/http/http_util.h#133

Comment 3

[Ed Guloien [:edgul]]

Ok, it seems pretty clear that we will add 0x3B and 0x7F to the CookieCommons::CheckName illegalCharacters list.

I'm also seeing that 0x09 (htab) is currently only in the CheckName list. So if we were to combine the name and value illegal lists we would need to resolve this difference as well.

That being said it sounds like we are wanting to be more restrictive in the name-list with these chars: 0x22 (dquote), 0x2C (comma), and 0x5C (backslash).

Is there something I can do to assist in verifying the path forward or are we safe to also add the contentious characters to the name list (and continue maintaining two lists going forward), namely: 0x09 (htab), 0x22 (dquote), 0x2C (comma), and 0x5C (backslash)?

Comment 4

[Daniel Veditz [:dveditz]]

We shouldn't combine the lists: names and values have different needs.

• yes, add 0x7 and 0x3B to the illegal NAME set
• also add 0x3D (equals) as Chrome has. Like the semi-colon we shouldn't encounter it from cookie headers or document.cookie because parsing will get to them first, but we wouldn't want to store those if extensions or folks using dev tools tried to create themn directly using lower-level APIs. Or if, for example, we implement the Cookie Store API

We can defer a decision on dquote, comma, and backslash: continue to allow them to match Chrome and historical Firefox practice. We should write up Web Platform Tests, though (that all/most browsers fail?), to encourage the spec to match reality or browsers to agree to match the spec.

I think we can block HTAB in cookie values. A server that is strict about "no control characters in headers" will reject headers with HTAB no matter where they occur in the field, and chromium browsers are getting away with being stricter. I'm sure we (or maybe Netscape!) made an exception for HTAB in values at some point because someone was using it, but that must have been long ago.

Chrome's code is easier to comprehend because it's a few simple tests and it's easy to see how they differ. I suspect our big ugly sets were a performance optimization of using FindCharInSet() rather than looping over the string testing iscntrl(c) || c == ';'. If we stick with it I suppose we just need to make sure the comments are very clear. With the changes added to allow for disabling Unicode in bug 1797231 we might be looping over that illegal set for every character and the inlined table-based behavior of iscntrl() might end up being faster.

Or maybe this isn't a hot spot anymore and we shouldn't worry about it.

Comment 5

[Ed Guloien [:edgul]]

Attachment: Bug 1802057 - Block the following characters from use in the cookie name in the cookie string: 0x3B (semi-colon), 0x3D (equals), and 0x7F (del) r?dveditz,valentin

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5afb1b714b6
Block the following characters from use in the cookie name in the cookie string: 0x3B (semi-colon), 0x3D (equals), and 0x7F (del) r=dveditz,cookie-reviewers

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5afb1b714b6

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Do we need this on ESR115 too? Please nominate for uplift if so.

Comment 9

[Ed Guloien [:edgul]]

Probably not necessary, thanks for checking.

Comment 10

[Tyson Smith [:tsmith]]

Attachment: advisory.txt

Comment 11

[Tyson Smith [:tsmith]]

Attachment: advisory.txt

Comment 12

[Daniel Veditz [:dveditz]]

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter