1782561 - (CVE-2023-4055) document.cookie desynchronization after cookie jar overflow

Status: VERIFIED FIXED

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

Description

[Marco Squarcina]

Attachment: ffdesync.js

Steps to reproduce:

We discovered that cookie jar overflows cause a desynchronization between the cookies listed by document.cookie and the actual cookie jar. We created a demo application to reproduce the inconsistencies. The demo is available at https://cookiedesync.minimal.blue/, full nodejs sources are attached to this report (execute with node ffdesync.js).

While creating this report we noticed a related bug at https://bugzilla.mozilla.org/show_bug.cgi?id=1761314. We decided to open a new issue and mark it as security-related due to the potential harm that this bug could cause: the reported issue allows same-origin and same-site attackers (i.e., an attacker controlling a related-domain of the target website who can set domain cookies) to fixate arbitrary cookies in document.cookie that will survive deletion attempts from the server, e.g., via the Clear-Site-Data HTTP header. This inconsistent state could introduce vulnerabilities in applications trusting the cookies read from document.cookie. Notice, for instance, that frontends often set custom HTTP headers using the values of specific cookies read via the document.cookie API. Examples are ASP.NET and Angular.

The issue can be manually reproduced following the steps below, please check our demo for the full set of tests.

1. Visit https://example.com/, set 181 cookies via document.cookie using the devetools Console:

   for(let i=0; i<181; i++) document.cookie = a${i}=_;

2. Trigger a a subsequent request (ctrl-shift-r), observe the cookies attached to the request via the devtools Network tab

3. In the devtools Console, read the cookies via document.cookie

4. Go to the devtools Storage tab in the browser inspector, delete all cookies

5. In the devtools Console, read the cookies via document.cookie

Actual results:

1. Cookies are set

2. Due to the overflow, only 151 cookies a30 ... a180 (included) are attached to the HTTP request. The Storage > Cookies panel is consistently showing only these 151 cookies.

3. document.cookie shows 181 cookies, from a0 ... a180, not reflecting the actual content of the cookie jar.

4. Cookies are deleted

5. document.cookie now shows only the first 30 cookies, a0 ... a29. Notice that HTTP requests at this point do not attach any cookie. Forced page reloads do not change the output of document.cookie

Expected results:

The output of document.cookie should be consistent with the state of the cookie jar (and, consequently, with the cookies attached to HTTP requests).

Comment 1

[Marco Squarcina]

Dear all,

We included a description of this bug in a paper on Web session integrity that will be presented at USENIX Security (August 9-11). We will also provide a Black Hat briefing on the same topic (August 9-10). As this issue has not yet been fixed, shall we request an embargo until August 9 or are you fine if the paper gets published earlier? We have until July 11 to ask for the embargo, so please let me know :)

Thanks!
Marco

Comment 2

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Hi Marco,

Sorry for the long delay in addressing this. I'd like to also thank you for a really great test case. It's been really useful in debugging this.

I think we'll want to request an embargo until August 9 to make sure the fix gets into all releases. Daniel can confirm.
I'll post the fix later today.

Comment 3

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Attachment: Bug 1782561 - Do not use getter_AddRefs to pass object to function r=#necko

When using getter_AddRefs it actually nulls out the pointer before
passing it to the function, so in this case a new list was being
created with every call to CreateOrUpdatePurgeList.
Instead, we can just pass a reference to the nsCOMPtr<nsIArray>&

Comment 4

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Attachment: Bug 1782561 - Make sure we can evict cookies while processing AddCookie r=#necko

Depends on D181743

Comment 5

[Marco Squarcina]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #2)

Hi Marco,

Sorry for the long delay in addressing this. I'd like to also thank you for a really great test case. It's been really useful in debugging this.

I think we'll want to request an embargo until August 9 to make sure the fix gets into all releases. Daniel can confirm.
I'll post the fix later today.

Hi Valentin,

No problem, I requested the embargo! Let me know when the fix lands in a nightly build, and I'll test it.

Comment 6

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Comment on attachment 9340464 [details]
Bug 1782561 - Make sure we can evict cookies while processing AddCookie r=#necko

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Easily. Requesting approval since bug does not have a sec rating.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: Bug 1450199
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: (applies to all branches)
• How likely is this patch to cause regressions; how much testing does it need?: I would say the likelihood of regressions is low to medium.
  There is definitely a possibility of unexpected interactions with other cookie code, but that's a low probability.
• Is Android affected?: Yes

Comment 7

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Ed, could you make sure this lands once it gets sec approval?

Comment 8

[Tom Ritter [:tjr] (OOTO until mid-August)]

Comment on attachment 9340463 [details]
Bug 1782561 - Do not use getter_AddRefs to pass object to function r=#necko

Approved to request uplift and land

Comment 9

[Tom Ritter [:tjr] (OOTO until mid-August)]

Comment on attachment 9340464 [details]
Bug 1782561 - Make sure we can evict cookies while processing AddCookie r=#necko

Approved to request uplift and land

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The bug is marked as tracked for firefox116 (beta) and tracked for firefox117 (nightly). However, the bug still has low severity.

:ghess, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 11

[Ed Guloien [:edgul]]

With Valentin out of office, I'm not certain if we want to be trying uplift for this. Tom, what do you think?

Comment 12

[Tom Ritter [:tjr] (OOTO until mid-August)]

I would strongly lean towards uplifting it, given that it's going to get broader attention due to being presented.

Comment 13

[Ed Guloien [:edgul]]

Comment on attachment 9340464 [details]
Bug 1782561 - Make sure we can evict cookies while processing AddCookie r=#necko

Beta/Release Uplift Approval Request

• User impact if declined: Users will continue to experience possible eviction desynchronizations when we are processing new cookies.
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Should only change content process batch deletion when we are processing cookies
• String changes made/needed:
• Is Android affected?: Yes

Comment 14

[Ed Guloien [:edgul]]

I assume we are okay to uplift to release. Please let me know if it is too soon for that.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9340463 [details]
Bug 1782561 - Do not use getter_AddRefs to pass object to function r=#necko

As far as I know, the plan is for this to ride Fx116 to release, not attempt a mid-cycle dot release uplift which would require a lot more overhead. Adding the ESR approval requests, though, since we are going to need to get it there too for 102.14esr & 115.1esr shipping alongside Fx116.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/05793789fabc
Do not use getter_AddRefs to pass object to function r=necko-reviewers,cookie-reviewers,jesup,edgul
https://hg.mozilla.org/mozilla-central/rev/75070c5d3bb6
Make sure we can evict cookies while processing AddCookie r=necko-reviewers,cookie-reviewers,jesup,edgul

Comment 17

[Marco Squarcina]

Hi all,

I tested FF nightly 117.0a1 (2023-07-10), and it passed all my tests on desynchronization issues, resulting equivalent to Chromium's behavior.

Thanks for addressing the problem!

Best,
Marco

Comment 18

[Dianna Smith [:diannaS] - on Leave, back on August 9th]

Comment on attachment 9340463 [details]
Bug 1782561 - Do not use getter_AddRefs to pass object to function r=#necko

Approved for 116.0b4

Comment 19

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/147afc745aee
https://hg.mozilla.org/releases/mozilla-beta/rev/dd425af55b5f

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9340463 [details]
Bug 1782561 - Do not use getter_AddRefs to pass object to function r=#necko

Approved for 115.1esr and 102.14esr.

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/888192f3f6be
https://hg.mozilla.org/releases/mozilla-esr115/rev/b06899762843

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr102/rev/3b9ef0cfced1
https://hg.mozilla.org/releases/mozilla-esr102/rev/11c4f109f4d0

Comment 23

[Negritas Sergiu, Desktop QA]

Hello!
I have managed to reproduce the issue with firefox 105.0a1(2022-08-01) on Ubuntu 22.04. I can confirm that the issue is fixed with firefox 117.0a1(2023-07-16), 115.0.2esr, 102.14.0esr, 116.0b6 on Windows 10, Ubuntu 22.04 and MacOS 12. Updating the flags and status of this bug accordingly.

Have a nice day!

Comment 24

[Daniel Veditz [:dveditz]]

(In reply to Marco Squarcina from comment #17)

I tested FF nightly .... [It is] equivalent to Chromium's behavior.

Interestingly not quite: after the cookie purge Chrome has 150 cookies and we have 151 as you mentioned in comment 0. The difference doesn't really matter since the quota is just a recommendation from the spec, but I know we intended 150 (kCookieQuotaPerHost = 150;) so we're off by one. That's unrelated to this issue so I'll file a separate bug.

Comment 25

[Daniel Veditz [:dveditz]]

Seeing the paper might have been enlightening, but I can't think of scenarios where this would turn into a serious security bug for a site. The site's client-side code may be confused about what cookies exist, but the cookies won't be sent to the host. If you add a cookie it will get propagated out to all the content processes so this can only be abused to remove cookies from network requests while making them appear un-removed in a particular content process. It's widely recommended that sensitive cookies like session/authentication cookies be sent HttpOnly, so those won't appear in document.cookie to confuse anyone.

Most of the time if the site is misbehaving because of this you can load the site in a new tab and close the corrupted one to get rid of the corrupted content process.

Comment 26

[Jesse Schwartzentruber (:truber)]

Attachment: advisory.txt