Closed Bug 1782561 (CVE-2023-4055) Opened 2 years ago Closed 11 months ago

document.cookie desynchronization after cookie jar overflow

Categories

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

Firefox 103
defect

Tracking

()

VERIFIED FIXED
117 Branch
Tracking Status
firefox-esr102 116+ verified
firefox-esr115 116+ verified
firefox115 --- wontfix
firefox116 + verified
firefox117 + verified

People

(Reporter: squarcina, Assigned: valentin)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, sec-low, Whiteboard: [to be presented August 9-11, 2023][necko-triaged][adv-main116+][adv-ESR115.1+][adv-ESR102.14+])

Attachments

(4 files)

Attached file 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).

Group: firefox-core-security → network-core-security
Component: Untriaged → Networking: Cookies
Product: Firefox → Core
See Also: → 1761314
Flags: needinfo?(dveditz)
Flags: needinfo?(edgul)
Severity: -- → S3
Flags: needinfo?(edgul)
Priority: -- → P2
Whiteboard: [necko-triaged]
Blocks: cookie

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

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.

Assignee: nobody → valentin.gosu
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: P2 → P1

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>&

(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 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
Attachment #9340464 - Flags: sec-approval?

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

Flags: needinfo?(dveditz) → needinfo?(edgul)

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

Attachment #9340463 - Flags: sec-approval? → sec-approval+

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

Approved to request uplift and land

Attachment #9340464 - Flags: sec-approval? → sec-approval+

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.

Flags: needinfo?(ghess)

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

Flags: needinfo?(edgul) → needinfo?(tom)

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

Flags: needinfo?(tom)

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
Attachment #9340464 - Flags: approval-mozilla-release?
Attachment #9340464 - Flags: approval-mozilla-beta?
Attachment #9340463 - Flags: approval-mozilla-release?
Attachment #9340463 - Flags: approval-mozilla-beta?

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

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.

Attachment #9340463 - Flags: approval-mozilla-esr115?
Attachment #9340464 - Flags: approval-mozilla-release? → approval-mozilla-esr115?
Attachment #9340463 - Flags: approval-mozilla-release? → approval-mozilla-esr102?
Attachment #9340464 - Flags: approval-mozilla-esr102?
Pushed by nerli@mozilla.com:
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
Group: network-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → 117 Branch
Flags: needinfo?(ghess)

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 on attachment 9340463 [details]
Bug 1782561 - Do not use getter_AddRefs to pass object to function r=#necko

Approved for 116.0b4

Attachment #9340463 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9340464 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

Attachment #9340463 - Flags: approval-mozilla-esr115?
Attachment #9340463 - Flags: approval-mozilla-esr115+
Attachment #9340463 - Flags: approval-mozilla-esr102?
Attachment #9340463 - Flags: approval-mozilla-esr102+
Attachment #9340464 - Flags: approval-mozilla-esr115?
Attachment #9340464 - Flags: approval-mozilla-esr115+
Attachment #9340464 - Flags: approval-mozilla-esr102?
Attachment #9340464 - Flags: approval-mozilla-esr102+

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!

(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.

Whiteboard: [necko-triaged] → [to be presented August 9-11, 2023][necko-triaged]
See Also: → 1845870

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.

Keywords: sec-low
Whiteboard: [to be presented August 9-11, 2023][necko-triaged] → [to be presented August 9-11, 2023][necko-triaged][adv-main116+][adv-ESR115.1+][adv-ESR102.14+]
Regressions: 1858366
Group: core-security-release
Alias: CVE-2023-4055
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: