1783536 - (CVE-2023-29547) document.cookie in an insecure origin process allows setting an insecure cookie in that process that has the same name as a secure one

Status: RESOLVED FIXED

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

Description

[Marco Squarcina]

Attachment: ffdesyncsecure.js

Steps to reproduce:

I am reporting a desynchronization issue between document.cookie and the actual content of the cookie jar in Firefox. This report follows https://bugzilla.mozilla.org/show_bug.cgi?id=1782561, but I think these are two separate issues. Similarly to the previous report, this inconsistency could introduce vulnerabilities in applications trusting the cookies read from document.cookie. In particular, this issue may allow same-site and network attackers to spoof the value of a cookie previously set as Secure.

1. Visit https://example.com
2. Set a secure cookie using the console, e.g., document.cookie = "test=secure; Secure"
3. In the same tab, visit http://example.com
4. Set an insecure cookie test to the value insecure: document.cookie = "test=insecure"
5. Force a refresh to http://example.com

A demo is available at https://ffdesyncsecure.minimal.blue/. Nodejs sources are attached to this report.

Actual results:

Step 4. sets the cookie test=insecure in document.cookie, altough a Secure cookie with the same name (test) already exists. Notice that the cookie test=insecure is not saved in the cookie jar by inspecting devtools Storage -> Cookies.
Step 5. as expected, the test cookie is not attached in the request to http://example.com. At the same time, document.cookie still shows test=insecure.

The current behavior causes a desynchronization between document.cookie and the actual cookie jar of the browser.

Expected results:

The document.cookie setter at step 4. should silently fail to set the cookie test=insecure due to the presence of a Secure cookie with the same name. Chromium-based browsers behave as expected, i.e., the cookie test=insecure does not populate document.cookie.

Comment 1

[Marco Squarcina]

For the record, non-secure cookies in this case should be discarded according to point 16 of the Cookies Storage Model (Sec. 5.5).

Comment 2

[Dragana Damjanovic [:dragana]]

Can you take a look?

Comment 3

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

The severity field is not set for this bug.
:valentin, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[edgul]

Replicated in Nightly 106.0a1 (2022-09-08).
Starting investigation.

Comment 5

[edgul]

Attachment: Bug 1783536 - Prevent document.cookie de-sync from cookie jar when setting secure cookies. r=dveditz,#necko-reviewers

Comment 6

[edgul]

Status: Writing missing test with mochi-browser test

Comment 7

[edgul]

Should have enough tests to proceed now. I have started investigation.

Comment 8

[edgul]

Continuing with investigation next week.

Comment 9

[edgul]

Temporarily backburnered for other P1's

Comment 10

[edgul]

Resuming investigation.

Comment 11

[edgul]

In progress: Trying to debug with RR on child process with mochi browser test that exhibits behavior.

Comment 12

[edgul]

Dan, I'm wanting to confirm two things here:

1. That the scheme is not relevant (at least temporarily) to document.cookie in the above steps to reproduce as per this comment . This appears to have been done for webcompat reasons but seems to contradict this bug which suggests that document.cookie should be schemeful and the most spec compliant with schemeful same site and incrementally better cookies. Is the former, schemeless, implementation of document.cookie still valid after the schemeful cookie rework that went into re-enabling network.cookie.sameSite.schemeful pref?

2. Also, I was a bit surprised to find that the when attempting to overwrite a secure cookie with an insecure one via https, that the overwrite was successful all the way to the cookie jar. Is this expected?

Comment 13

[edgul]

In reading the storage model spec a little more closely, it looks like my above question #2 is expected.
https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-10#section-5.5

If the cookie's secure-only-flag is false, and the scheme component of request-uri does not denote a "secure" protocol, 
then abort these steps and ignore the cookie entirely if the cookie store contains one or more cookies that meet all of 
the following criteria:
* Their name matches the name of the newly-created cookie.
* Their secure-only-flag is true.
* ...

Comment 14

[edgul]

Please let me know if there is some alternative solution that I am missing here. Even with option B it seems like the solution to the below problem is less secure than the affliction. Which further begs the question: Can we even adequately address this bug without this?

The Problem:
From what I can tell we aren't sending secure cookies to non-secure origin processes. So in the case where Tab 1 has https://example.com secure cookie and we attempt an overwrite on the same cookie name with an insecure cookie on Tab2 for http://example.com, we have a limitation with the current plumbing in that we can't know which secure cookies are in the jar.

Two options readily come to mind:
A. Send all cookies for the relevant base domain + OA (including the secure ones) from the cookie jar in the parent process to the matching child. But this seems like a step backwards in terms of security and cookie isolation, especially if we continue to move towards more schemeful separations.
B. Some kind of additional signalling from parent to relevant content processes that we are holding cookies of these name-host-path, without telling the content process what the values are.

Additionally:
Assuming we get this working. I'm concerned about the edge case behavior if we:

1. Tab 1: set insecure cookie "test=yes" on http://example.com
2. Tab 2: set secure cookie "test=overwrite; Secure" on https://example.com.
   Step 2 overwrite shouldn't be able to make it to Tab 1's process. I'm guessing we should clear the cookie in Tab 1 process so that we don't create another de-synchronization.

Comment 15

[Marco Squarcina]

Hi, it's difficult for me to weigh in on option A or B without access to the bug mentioned above and due to missing background on Firefox implementation details.

That said, the example cited in the previous message is the dual of the problem I reported. I confirm that it creates an inconsistency since document.cookie returns different entries from the content of the cookie jar. Indeed, the insecure cookie test=yes should be cleared from document.cookie after setting the secure cookie in Tab 2.

Best,
Marco

Comment 16

[edgul]

Thanks for clarifying Marco.

Not sure if clarification is needed here, but essentially the child process that will be responsible for blocking the cookie-set will need to somehow know the cookie exists (but for a privileged origin) in the jar. But the security concern with Option A is that if the child process is ever compromised, then that process will have easier access to potentially other origin and other process cookies.

Also,

Which further begs the question: Can we even adequately address this bug without this?

My comment above is a bit confused, I will modify it. But the bug in question is still relevant. If it is fixed, then Option A or B for this bug seem like something we definitely should NOT do, but by then another alternative may become available.

Hoping security team can shed some light here.

Comment 17

[Daniel Veditz [:dveditz]]

[Bugzilla usability request: instead of making markdown links to bugs ("this bug", "this comment", "bug mentioned above") please use plain text mentions and let Bugzilla auto-linkify them ("bug 1756213", "bug 1748693 comment 13"). Auto links will reveal the status and bug title in the hover text which can be a time-saver compared to having to open all the links to figure out what they are. If you don't have access to a bug you lose out on the title, but at least you still get a little info about the status.]

the most spec compliant with schemeful same site and incrementally better cookies.

Those proposals are obsolete—please do not use them as references. They are probably very similar to what's in the latest rfc6265bis drafts, but there could be subtle changes that end up making a big difference somewhere.

I'm lowering the security severity of this bug, and maybe it's enough of an edge-case it doesn't need to be hidden. This bug doesn't allow the (global) setting of an insecure cookie. The global cookie jar continues to reflect the correct behavior. This bug only affects what is visible to document.cookie in that one child process. To reduce the harm of spectre-style attacks, the fission project has led to each child process having only a subset of cookies relevant to the origin rather than direct access to all cookies. When you've got multiple copies of the same data they are inevitably at risk of being out of sync.

At some point any cookies created using document.cookie have to be communicated up to the real cookie jar in the parent, and at that point adding the cookie fails. Why isn't the child process told about this failure so it can clean up document.cookie?

To your original questions in comment 12:

• as you found, point 2 is fine: a site that can set a secure cookie is allowed to delete it, or replace it with a non-secure cookie
• in question 1 you're confusing two different uses of scheme--as indeed did the implementation that had to be fixed in bug 1748693. The "schemeful" in "schemeful samesite" refers to the schemes of the originating browser context and the destination of a request. The cookies themselves have no scheme.

Your analysis in comment 14 seems spot on.

Option A would be the easiest, at the cost of reintroducing Spectre worries for secure cookies. That seems worse than this bug since the secure site is trying to do the right thing, and this bug only lets an insecure site confuse itself.

Option B is essentially what CookieServiceParent.cpp does with httponly cookies: bug 1440462 changed things to blank out the value but continue sending the name and other metadata so the child process could know not to override those from document.cookie. You could do something like that, but as you pointed out in comment 14 the CookieService::GetCookiesForURI() has already stripped those out. Maybe you could add yet another argument to that routine that lets it know we want a slightly expanded set of cookies than the ones we want to send along with a network request, and then the list gets re-filtered by CookieServiceChild::GetCookieStringFromDocument()

Option C might be that when the cookies set in the child process are shipped to the parent, return an error code so the child process can delete the cookie it just added via document.cookie. It'd be tricky though, because we send these as a batch and there's currently no way to signal a specific failed cookie. Maybe we could generate a "cookie was deleted" update message back to the child for each attempt to shadow a secure cookie?

Or even Option D: live with this bug. As long as the cookie isn't set in the real cookie store and isn't sent with network requests it's mostly self-inflicted confusion for the affected site. Of course it could also be set by a MITM of the insecure site, or a malicious sibling domain (in the same session, since the cookie will go away when the process does). so it would be nice to fix.

But [bug 1788109] is still relevant. If it is fixed, then Option A or B for this bug seem like something we definitely should NOT do, but by then another alternative may become available.

I don't think bug 1788109 is relevant. We currently try to limit what cookies are sent to child processes to prevent spectre-like attacks, which don't require compromising the child process. The problem bug 1788109 foresees assumes an already-compromised child process. Your options represent the same level of leak either way.

I do agree that Option A is something we should not do: it's a worse risk than the problem described in this bug.

If we could make secure cookies work similarly to httponly cookies then Option B would be reasonable.

Comment 18

[edgul]

I agree that Option B looks like the way to go here.

• The parameter expansion of GetCookiesForURI appears necessary since I wouldn't expect the parent-context CookieService::GetCookieStringFromHttp to change behavior.
• Checking for secure cookies in an insecure origin will be easy within GetCookiesForURI.
• Filtering in SetCookieStringFromDocument will be similar.
• Wiping cookie values before broadcast to content processes will align with http-only cookies, namely in functions: RemoveCookie(), AddCookie(), RemoveBatchDeletedCookies() and SerializeCookieList()
• Looks like filtering should already work in the GetCookieStringFromDocument() for CookieServiceChild and CookieService so that's nice.

Further, since we can probably trust a server to not intentionally try to overwrite it's own cookies I would expect the behavior of the following functions to remain the same:
CookieService::SetCookieStringFromHttp -> only used to put cookie into jar from response header in parent context
CookieServiceChild::GetCookieStringFromHttp -> not implemented
CookieServiceChild::SetCookieStringFromHttp -> only used to add cookie from response header to child and notify parent to store cookie in jar

Comment 19

[edgul]

Made some minor adjustments to attachment 9295018 [details]. Dan, can you have a look?

Treeherder: https://treeherder.mozilla.org/jobs?repo=try&revision=38aba67f90777dd8912907b6e37f27162db723be

Comment 20

[edgul]

Comment on attachment 9295018 [details]
Bug 1783536 - Prevent document.cookie de-sync from cookie jar when setting secure cookies. r=dveditz,#necko-reviewers

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not very easily
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?:
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Should port pretty easily
• How likely is this patch to cause regressions; how much testing does it need?: Low likelihood. Testing has already been added.
• Is Android affected?: Unknown

Comment 21

[edgul]

Does not need sec-approval as it is sec-low.

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Prevent document.cookie de-sync from cookie jar when setting secure cookies. r=dveditz,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/ba89445b47f17de3b38fac4c88bce3be79ca36e0
https://hg.mozilla.org/mozilla-central/rev/ba89445b47f1

Comment 23

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

The patch landed in nightly and beta is affected.
:edgul, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox111 to wontfix.

For more information, please visit auto_nag documentation.

Comment 24

[edgul]

Maybe not. Dan what do you think?

Comment 25

[Marco Squarcina]

I tried to reproduce the issue against Firefox 112.0a1 (2023-02-22) (64-bit) and I confirm that the problem I reported and its dual (https://bugzilla.mozilla.org/show_bug.cgi?id=1783536#c14) are gone. However, I noticed an inconsistency in the Storage Inspector:

1. Go to http://example.com, execute document.cookie = 'test=insecure'
2. New tab, go to https://example.com, execute document.cookie = 'test=secure; Secure'
3. Go back to the first tab where http://example.com is open. Check Storage > Cookies in the inspector
4. I would expect to see nothing, instead the Secure cookie is shown. Refreshing the page, or visiting http://example.com in a new tab does not prune the entry from the cookies listed by the Storage Inspector. Still, no cookies are attached to requests and document.cookie is empty.

Additionally, I tested the other desync issue caused by a cookie jar overflow, see https://bugzilla.mozilla.org/show_bug.cgi?id=1782561. Unfortunately, this desynchronization is not addressed by the patch landed in FF nightly, so the 2 issues seem to be unrelated.

Comment 26

[edgul]

Thanks for catching this Marco, I'm seeing the same thing.
I'll take a look at how we interface with dev tools on this.

Comment 27

[Marco Squarcina]

I have just realized that the Storage Inspector shows Secure cookies irrespective of the scheme of the current page. Contrary to what I initially thought, this discrepancy has nothing to do with the desynchronization issue discussed here, sorry.

I always assumed that cookies listed in the Storage Inspector are scoped to the current origin + path, similar to Chrome. I guess I was wrong. Out of curiosity, I tried to access Secure cookies from a non-secure origin using WebDriver with the code below.

#!/usr/bin/env python3

import time
from selenium import webdriver

driver = webdriver.Firefox()

driver.get("https://example.com")
driver.execute_script("document.cookie = 'test=secure; Secure; SameSite=Strict'")
time.sleep(1)
print(f'[S] Cookies on https://example.com: {driver.get_cookies()}')

driver.get("http://example.com/")
time.sleep(1)
print(f'[I] Cookies on http://example.com: {driver.get_cookies()}')

driver.close()

Again, to my surprise, the Secure cookie is printed twice on Firefox, whereas on Chrome, the Secure cookie is returned only on https://example.com.

I explicitly added the SameSite attribute to level any differences between the two browsers. According to rfc6265bis, http://example.com should be considered a different site wrt https://example.com. So WebDriver is showing a cookie scoped for a different site here. This looks like a fun CTF challenge, but maybe there are real-world security implications affecting pipelines using WebDriver + FF?

I also had a quick look at the WebDriver spec and found out that all associated cookies to a document must meet the requirements specified by the algorithm in rfc6265, Sec 5.4 (step 1) . Despite referencing the old standard from 2011, the first step mentions that Secure cookies must be discarded on non-secure documents. This is somehow related to https://github.com/mozilla/geckodriver/issues/1840 and bug#1690739.

If you think this is relevant (personally I do), shall we keep this issue closed, and I'll open a new one?

Comment 28

[edgul]

Thanks for looking at how Storage Inspector accesses the cookies.
Yeah, I think we can keep this closed and open a new one.

Comment 29

[June Wilde (she/her) [:jewilde]]

Attachment: advisory.txt