Closed Bug 1658869 (CVE-2022-45410) Opened 4 years ago Closed 2 years ago

SameSite policy bypassed with Service Worker FetchEvent

Categories

(Core :: DOM: Service Workers, defect, P2)

defect

Tracking

()

VERIFIED FIXED
107 Branch
Tracking Status
firefox-esr91 --- wontfix
firefox-esr102 107+ verified
firefox98 --- wontfix
firefox99 --- wontfix
firefox100 --- wontfix
firefox105 --- wontfix
firefox106 --- wontfix
firefox107 + verified
firefox108 --- verified

People

(Reporter: kiding, Assigned: edenchuang)

References

()

Details

(Keywords: sec-moderate, Whiteboard: [reporter-external] [client-bounty-form] [verif?][secdom:spec][disclosure at DEFCON 30, to be held in Aug 11-14][post-critsmash-triage][adv-main107+][adv-esr102.5+])

Attachments

(4 files)

Attached image x.jpg

It seems Firefox ignores SameSite policy by sending protected cookies when: a request is intercepted by a service worker FetchEvent, then fired from within its context. As developers willingly opt to implement FetchEvent mainly for caching purposes, this behavior undermines CSRF protection expected from setting up SameSite attribute.

I've set up a simple demo page at https://prong-glory-novel.glitch.me. The site responds with Set-Cookie the "None" "Lax" "Strict" cookies, then echos the Cookie header value in each response. Without a service worker registered, "cross-site page with a link back here" works as expected; "Lax" sent only for GET method, "Strict" not sent for neither GET nor POST. Once the worker registered however, the page will always show all three cookie values sent in the request, indicating SameSite policy is not being enforced.

rfc6265bis 5.2.2.2. states two different scenarios for service workers: 1. Requests which simply pass through a Service Worker, 2. Requests which are initiated by the Service Worker itself. It seems Firefox handles the above case in the latter way, presumably because there is a direct call to fetch() is involved. However, I believe they should be handled in the former way; The worker cannot manipulate the Request object in any way, and all it can do with the object is to hand it over to the network or drop it. In my opinion, it is natural to think the request is actually originated from a top-navigation document, so it should be treated like so.

Tested with 81.0a1 (2020-08-12) (64-bit) on macOS 10.15.6. Cross-reported to Chromium as Chrome exhibits the same behavior. Safari only sends "None" which I suppose is a different problem in itself.

Once thing Firefox-specific pecularity is that Developer Tools says it's sending the right cookies while it is absolutely not. See the attachment.


김동성 ᛫ Dongsung Kim ᛫ https://kidi.ng

Flags: sec-bounty?

Anne, do you know who is the right person to look into this?

Group: firefox-core-security → dom-core-security
Type: task → defect
Component: Security → DOM: Networking
Flags: needinfo?(annevk)
Product: Firefox → Core
Severity: -- → S3
Component: DOM: Networking → DOM: Service Workers
Flags: needinfo?(ytausky)
Flags: needinfo?(bugmail)
Flags: needinfo?(annevk)
Priority: -- → P2
Flags: sec-bounty? → sec-bounty+

Jens, could you please ask asuth for progress on this?

Flags: needinfo?(jstutte)

Given other priorities, we won't be able to get to this immediately. keeping :asuth' and :ytausky's needinfos, though.

Flags: needinfo?(jstutte)
Flags: needinfo?(bugmail)

(didn't mean to clear my needinfo, was trying to add someone as a CC and didn't realize the side-effect was happening)

Flags: needinfo?(bugmail)

For some reason I never saw this bug, but what Chrome and Firefox do is correct per the specification. When fetch() is invoked the service worker takes ownership of the request. This is indeed rather terrible and is discussed at https://github.com/httpwg/http-extensions/issues/1288, but there's no clear way that this can work well. Service workers defeat CSRF. We might have a discussion about this at the next W3C TPAC.

Reporter, could you please share the Chromium bug number? Or copy annevankesteren@gmail.com there? It would be useful to know what they are thinking with regards to this.

Status: UNCONFIRMED → NEW
Ever confirmed: true

Standards-wise we figured out a new model whereby the origin of a request is sometimes preserved: https://github.com/whatwg/fetch/pull/1345. (The URL list is preserved as well to not undo any tainting of the origin.)

That means this is now a security bug that we can and should fix.

Flags: needinfo?(ytausky) → needinfo?(jstutte)
Assignee: nobody → echuang
Flags: needinfo?(jstutte)

Chromium Team moved the bug forward and disclosed with a CVE attached.

Is it possible to reserve a new CVE number that can track this bug? I reckon Firefox Developer Tools' "pecularity" makes this a bit different from Chromium's.

(In reply to kiding from comment #9)

Chromium Team moved the bug forward and disclosed with a CVE attached.

Can you link to the chromium bug? ( https://github.com/whatwg/fetch/pull/1345 doesn't appear to list one)

Is it possible to reserve a new CVE number that can track this bug? I reckon Firefox Developer Tools' "pecularity" makes this a bit different from Chromium's.

I believe in our processes, CVEs normally get assigned when the associated bug is fixed; passing this question to Dan to confirm.

Flags: needinfo?(kiding)
Flags: needinfo?(dveditz)

(In reply to :Gijs (he/him) from comment #10)

Can you link to the chromium bug? ( https://github.com/whatwg/fetch/pull/1345 doesn't appear to list one)

It’s the same one mentioned in comment #7: https://bugs.chromium.org/p/chromium/issues/detail?id=1115847.

Flags: needinfo?(kiding)

I believe in our processes, CVEs normally get assigned when the associated bug is fixed; passing this question to Dan to confirm.

Yes, we assign them when registering the vulnerability info with MITRE. And since they publish what we send them, we don't do that until bugs are fixed and we're in the release end-game.

Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][secdom:spec]

The reporter is letting us know that this is part of a talk that will be presented at DEFCON30, i.e., we are expecting this to be public by mid August.
AFAIU, this is still pending a resolution in the specification though?

Whiteboard: [reporter-external] [client-bounty-form] [verif?][secdom:spec] → [reporter-external] [client-bounty-form] [verif?][secdom:spec][disclosure at DEFCON 30, to be held in Aug 11-14]

(In reply to Frederik Braun [:freddy] from comment #14)

The reporter is letting us know that this is part of a talk that will be presented at DEFCON30, i.e., we are expecting this to be public by mid August.
AFAIU, this is still pending a resolution in the specification though?

The chromium ticket is fixed, and the fetch spec ticket is also closed. Eden, are you still working on the patch?

Flags: needinfo?(echuang)

Eden is on a leave. Andrew, can you help here?

Assignee: echuang → bugmail
Status: NEW → ASSIGNED
Flags: needinfo?(echuang)
Flags: needinfo?(bugmail)
Assignee: bugmail → echuang
Attachment #9275861 - Attachment description: Bug 1658869 - Saving FetchEvent.request's origianl principal and ContentPolicyType for SameSite cookie checking. r=#dom-worker-reviewers → Bug 1658869 - Propagate the InterceptedHttpChannel information to fetch's channel casued by FetchEvent.request. r=#dom-worker-reviewers

Propagate the InterceptedHttpChannel information to fetch's channel casued by FetchEvent.request. r=dom-worker-reviewers,dragana,jesup
https://hg.mozilla.org/integration/autoland/rev/6bca2bb22edf29579ee3d8754ba0a954fb0b7ffc
https://hg.mozilla.org/mozilla-central/rev/6bca2bb22edf

Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 107 Branch

Pretty late in the cycle to uplift to Beta for 106 at this point. Let's aim to get it on ESR next cycle though. Eden, can you please attach a rebased patch for ESR102 when you get a chance and request approval? Thanks!

Comment on attachment 9275861 [details]
Bug 1658869 - Propagate the InterceptedHttpChannel information to fetch's channel casued by FetchEvent.request. r=#dom-worker-reviewers

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Third-party pages could get first-party same-site cookies through ServiceWorker.
  • User impact if declined: When fetch() through the FetchEvent.request or NavigationPreload, the same-site cookie policy would not work correctly. Same-site cookies would leak to the third-party pages.
  • Fix Landed on Version: 107
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The patch adds some new members of InternalRequest and loadInfo. It doesn't change the logic for the old code.
Flags: needinfo?(echuang)
Attachment #9275861 - Flags: approval-mozilla-esr102?
Flags: qe-verify+
Whiteboard: [reporter-external] [client-bounty-form] [verif?][secdom:spec][disclosure at DEFCON 30, to be held in Aug 11-14] → [reporter-external] [client-bounty-form] [verif?][secdom:spec][disclosure at DEFCON 30, to be held in Aug 11-14][post-critsmash-triage]]
QA Whiteboard: [qa-triaged]
Attached image 106 vs 107.png

Tried reproducing the issue in order to be able to verify it (tried on two different systems) by following the info and test site provided in Comment 0.

From what I tried so far, I couldn't see any difference between 106.0 and latest 107.0a1 (with fix), I'll attach a screenshot. Am I missing something? Can you eventually provide some exact STR, and what should we be paying attention to in order to know that now it behaves correctly? Thank you!

Flags: needinfo?(echuang)

Comment on attachment 9275861 [details]
Bug 1658869 - Propagate the InterceptedHttpChannel information to fetch's channel casued by FetchEvent.request. r=#dom-worker-reviewers

This needs a rebased patch for ESR.

Attachment #9275861 - Flags: approval-mozilla-esr102?

Catalin, I had to change some of the demo page for the presentation, sorry! Here is how to reproduce the bug.

  1. Visit https://prong-glory-novel.glitch.me
  2. Open the caret, click the “register” button.
  3. Visit https://goofy-voracious-spatula.glitch.me
  4. Click the “Navigate using GET” link.
  5. The page should say: Your browser sent: cookie: none-cookie=1; lax-cookie=2
  6. Repeat 3.
  7. Click the “Navigate using POST” link.
  8. The page should say: Your browser sent: cookie: none-cookie=1

I did my own testing that confirms it works as expected on 108.0a1, and does not work on 106.0beta.

Thanks Kiding, that did the trick. It shows as expected following your STR. Tests were performed on Firefox 108.0a1 (2022-10-17). Tests were performed on macOS 13, Ubuntu 22.04 and WIndows 11.

Flags: needinfo?(echuang)

(In reply to kiding from comment #23)

Catalin, I had to change some of the demo page for the presentation, sorry! Here is how to reproduce the bug.

  1. Visit https://prong-glory-novel.glitch.me
  2. Open the caret, click the “register” button.
  3. Visit https://goofy-voracious-spatula.glitch.me
  4. Click the “Navigate using GET” link.
  5. The page should say: Your browser sent: cookie: none-cookie=1; lax-cookie=2
  6. Repeat 3.
  7. Click the “Navigate using POST” link.
  8. The page should say: Your browser sent: cookie: none-cookie=1

We should consider turning this into an automated test.

Eden, please write a test (here or in a follow-up bug) according to the steps above.

Flags: needinfo?(echuang)
Flags: in-testsuite?

(In reply to Frederik Braun [:freddy] from comment #26)

Eden, please write a test (here or in a follow-up bug) according to the steps above.

testing/web-platform/tests/service-workers/service-worker/same-site-cookies.https.html covers the situation, so that is the reason why I did not write a test for it.

Flags: needinfo?(echuang)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #22)

Comment on attachment 9275861 [details]
Bug 1658869 - Propagate the InterceptedHttpChannel information to fetch's channel casued by FetchEvent.request. r=#dom-worker-reviewers

This needs a rebased patch for ESR.

I did the rebase, probably need rebase again.

(In reply to Eden Chuang[:edenchuang] from comment #28)

I did the rebase, probably need rebase again.

Sorry, I missed that it got updated in Phab. For the future, it's less confusing to submit a new revision for the rebase rather than overwriting the one that already landed elsewhere.

Attachment #9275861 - Flags: approval-mozilla-esr102?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #29)

(In reply to Eden Chuang[:edenchuang] from comment #28)

I did the rebase, probably need rebase again.

Sorry, I missed that it got updated in Phab. For the future, it's less confusing to submit a new revision for the rebase rather than overwriting the one that already landed elsewhere.

Sorry for the confusion. Will submit a new revision next time.

Comment on attachment 9275861 [details]
Bug 1658869 - Propagate the InterceptedHttpChannel information to fetch's channel casued by FetchEvent.request. r=#dom-worker-reviewers

Approved for 102.5esr.

Attachment #9275861 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+

Verified the fix as well on Firefox ESR102.5.0 (treeherder build). Tests were performed on macOS 13, Ubuntu 22.04 and Windows 11.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [reporter-external] [client-bounty-form] [verif?][secdom:spec][disclosure at DEFCON 30, to be held in Aug 11-14][post-critsmash-triage]] → [reporter-external] [client-bounty-form] [verif?][secdom:spec][disclosure at DEFCON 30, to be held in Aug 11-14][post-critsmash-triage][adv-main107+]
Attached file advisory.txt
Whiteboard: [reporter-external] [client-bounty-form] [verif?][secdom:spec][disclosure at DEFCON 30, to be held in Aug 11-14][post-critsmash-triage][adv-main107+] → [reporter-external] [client-bounty-form] [verif?][secdom:spec][disclosure at DEFCON 30, to be held in Aug 11-14][post-critsmash-triage][adv-main107+][adv-esr102.5+]
Alias: CVE-2022-45410
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: