1797235 - (CVE-2022-46876) Firefox accepts control characters in cookies via document.cookie, which breaks the majority of websites

Status: RESOLVED FIXED

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

Description

[April King [:April]]

Attachment: Screen Shot 2022-10-24 at 10.06.17 PM.png

This issue is related to bug 1797231, but is considerably more serious and in my conversations with Dan Veditz should probably get fixed relatively quickly.

As in bug 1797231, Firefox allows certain cookie values that, when present, will cause a permanent denial-of-service attack on anyone whose cookie store has been poisoned by their presence. Unlike bug 1797231 however, this is due to Firefox allowing cookies to be set via document.cookie that contain control characters in defiance of RFC 6265: \x00-08, \x0A-x1F, and \x7F (delete).

These cookies can be created with the following Javascript (changing it up to 127 for delete)

for (i=0; i<32; i++) {
  let paddedIndex = i.toString().padStart(3, '0') + '_' + '0x' + i.toString(16).padStart(2, '0');

  document.cookie=`cookie${paddedIndex}=-- ${String.fromCharCode(i)} --`;
}

Firefox will allow these cookies to be created despite the fact that it seems to have code related to not allowing them via Set-Cookie:
https://searchfox.org/mozilla-central/source/netwerk/cookie/CookieCommons.cpp

bool CookieCommons::CheckHttpValue(const CookieStruct& aCookieData) {
  // reject cookie if value contains an RFC 6265 disallowed character - see
  // https://bugzilla.mozilla.org/show_bug.cgi?id=1191423
  // NOTE: this is not the full set of characters disallowed by 6265 - notably
  // 0x09, 0x20, 0x22, 0x2C, 0x5C, and 0x7F are missing from this list. This is
  // for parity with Chrome. This only applies to cookies set via the Set-Cookie
  // header, as document.cookie is defined to be UTF-8. Hooray for
  // symmetry!</sarcasm>
  const char illegalCharacters[] = {
      0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x0A, 0x0B, 0x0C,
      0x0D, 0x0E, 0x0F, 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17,
      0x18, 0x19, 0x1A, 0x1B, 0x1C, 0x1D, 0x1E, 0x1F, 0x3B, 0x00};
  return aCookieData.value().FindCharInSet(illegalCharacters, 0) == -1;
}

When created (see screenshot for what the console looks like), these cookies will permanently disable almost every site I tested: Facebook, Netflix, Github, Wikipedia, etc. Further, every request sent to a server running PHP seems to generate a permanent 400 error. I would hazard a guess that a significant percentage of the entire web can be rendered inaccessible if this code is able to run via XSS or any other method of code execution (such as allowed by playground sites).

These characters were almost certainly never intended to be allowed to be created via document.cookie, but seem to have bypassed the checks on them. Note that, unlike with the other bug which -does- affect Chromium, this bug only seems to affect Firefox.

Comment 1

[Daniel Veditz [:dveditz]]

// [This check] only applies to cookies set via the Set-Cookie
// header, as document.cookie is defined to be UTF-8.

UTF-8 supports the full set of control characters 🤦🏻‍♀️

Comment 2

[Daniel Veditz [:dveditz]]

Skipping this check for document.cookie was done intentionally at the time because it broke tests, and at the time (2015) browsers were super inconsistent. Now it's just plain wrong and we should fix any tests that are still broken. See conversation at bug 1191423 comment 27 and following.

Naively, we could try removing the aFromHTTP check at the two places CheckHttpValue() is called.
https://searchfox.org/mozilla-central/search?q=CheckHttpValue&path=&case=false&regexp=false

Comment 3

[Tom Schuster]

I tried follow Chrome's implementation of SetCookie, but all the proxying makes it difficult to tell if I am in the right function. It seems like they have two variants and they want to add "extra" checks soon? https://bugs.chromium.org/p/chromium/issues/detail?id=1276520

In parsed_cookie.cc they have both IsValidCookieValueLegacy and IsValidCookieValue.

Comment 4

[April King [:April]]

I wrote about it here:

https://grayduck.mn/drafts/http-cookie-idiosyncrasies/
(please don't share)

It does a good job of walking through what Chrome does and doesn't allow. The Javascript above will also let you poke at it. But they seem to follow RFC 6265 (bis) section 5.6, aside from not allowing 0x09. In my testing, they do not allow 0x00-0x1F or 0x7F, but (commented below) I think they should allow 0x09 (htab).

Comment 5

[April King [:April]]

If it's helpful:

bool CookieCommons::CheckHttpValue(const CookieStruct& aCookieData) {
  // reject cookie if value contains an RFC 6265 disallowed character - see
  // https://bugzilla.mozilla.org/show_bug.cgi?id=1191423
  // NOTE: this is not the full set of characters disallowed by 6265 - notably
  // 0x09, 0x20, 0x22, 0x2C, 0x5C, and 0x7F are missing from this list. This is
  // for parity with Chrome. This only applies to cookies set via the Set-Cookie
  // header, as document.cookie is defined to be UTF-8. Hooray for
  // symmetry!</sarcasm>
  const char illegalCharacters[] = {
      0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x0A, 0x0B, 0x0C,
      0x0D, 0x0E, 0x0F, 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17,
      0x18, 0x19, 0x1A, 0x1B, 0x1C, 0x1D, 0x1E, 0x1F, 0x3B, 0x00};
  return aCookieData.value().FindCharInSet(illegalCharacters, 0) == -1;
}

Is mostly correct. It would need to have 0x7F added to it, as the comment is incorrect. It also incorrectly notes that Chromium allows 0x09 (htab), which is not true. That said, I think allowing 0x09 is correct (the RFC agrees), and Safari allows it as well. My testing also hasn't found anything that breaks with 0x09 -- which is not to say it isn't out there, but I haven't found it.

The code above would also need to apply to document.cookie, which it currently seems not to. Both Chromium and Safari do apply these restrictions when set via document.cookie.

Comment 6

[Daniel Veditz [:dveditz]]

This is something between a sec-low and a sec-moderate. The DoS aspect is annoying, but worst case might allow for header injection if the circumstances of a particular site line up correctly.

Comment 7

[Tom Schuster]

Attachment: Bug 1797235 - Implement WPT expectation. r?#necko-reviewers

Comment 8

[Tom Schuster]

I pushed the patch to try. We seem to now pass most of the existing web platform test cookies/value/value-ctl.html. With the exception of

• Cookie with %x0 in value is rejected. (I think we truncate at some point before checking the characters ...)
• Cookie with %xa in value is rejected. (I am pretty sure our parser doesn't consider 0xA to be part of the value, so we don't even check it)
• Cookie with %xd in value is rejected. (Same as above)
• Cookie with %x7f in value is rejected. (We should add this as April pointed out)

Comment 9

[Tom Schuster]

https://hg.mozilla.org/mozilla-central/rev/497d9623fd39321e3b50fccad859f0ccdbbafd24

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Hi Tom, is this ready for an ESR102 approval request? It grafts cleanly.

Comment 11

[Tom Schuster]

Comment on attachment 9300764 [details]
Bug 1797235 - Implement WPT expectation. r?#necko-reviewers

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Low complexity security fix.
• User impact if declined: It's possible to brick access to a lot of websites unrecoverable without deleting cookies.
• Fix Landed on Version: 108
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This follows Chrome's behavior.

Comment 12

[Dianna Smith [:diannaS]]

Comment on attachment 9300764 [details]
Bug 1797235 - Implement WPT expectation. r?#necko-reviewers

Approved for 102.6esr

Comment 13

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/80eeeeddbf7f

Comment 15

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 16

[Tom Ritter [:tjr]]

As there are unresolved spec issues and general related issues not yet resolved, we're going to hold this advisory for now.