Closed Bug 1953521 Opened 9 months ago Closed 9 months ago

Storage access "opt-in" is not reset after a cross-site redirect

Categories

(Core :: Privacy: Anti-Tracking, defect)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox-esr115 --- unaffected
firefox-esr128 --- unaffected
firefox136 --- wontfix
firefox137 --- wontfix
firefox138 --- fixed

People

(Reporter: chris.p.fredrickson, Assigned: bvandersloot)

References

Details

(Keywords: reporter-external, sec-moderate, Whiteboard: [fixed by 1956218][adv-main138+])

Attachments

(1 file, 1 obsolete file)

Steps to reproduce:

This test case captures the reproduction steps: https://wpt.fyi/results/storage-access-api/requestStorageAccess-cross-site-fetch.sub.https.window.html?label=experimental&label=master&aligned

This is a security bug, as it allows a malicious site to use redirects to send credentialed requests to arbitrary endpoints on any site that has called the Storage Access API.

  1. Navigate to a page on https://a.com, which embeds an iframe from https://b.com.
  2. In the iframe, execute document.requestStorageAccess() and accept the permission prompt.
  3. In the iframe, send a fetch request to some endpoint on https://evil.com which returns a 302 to redirect back to https://b.com/deletemyaccount.

Actual results:

Firefox attaches unpartitioned cookies when following the redirect.

Expected results:

Firefox should not have attached unpartitioned cookies when following the redirect.

If you can do #3 isn't the bigger problem that b.com has a massive CSRF bug that could be exploited when it's not in a frame?

Tim: if this is a known failure in the WPT do we already have a bug on this?

Group: core-security → dom-core-security
Flags: needinfo?(tihuang)

Radaring this to some folks.

For my edification and with apologies for the dumb question, as someone not very familiar with WPT: what is this test in comment 0? In particular, the "view source" link for the main github branch points to https://github.com/web-platform-tests/wpt/blob/1708853580/storage-access-api/requestStorageAccess-cross-site-fetch.sub.https.window.html which is basically the equivalent of a 404 (and so is the "master branch" link). Is this a new test that hasn't yet been added? Where can I see what the test is doing?

(I understand that the steps would produce the same result but running them in the context of WPT would make it easier to debug what is happening here than setting up your own relevant dummy domains.)

Flags: needinfo?(chris.p.fredrickson)
Flags: needinfo?(bvandersloot)
Flags: needinfo?(bugmail)

See here. This is a new test added two days ago. We haven't had a bug for this.

Ben, I see we only propagate Storage Access if the navigation is self-initiated and same-origin (Bug 1835907). In the implementation, it only takes care of subFrame navigations but not fetch requests. So, we still preserve Storage Access even if a cross-origin redirect happens during a fetch request. I think we should clear the storage access flag in this case to prevent the security issue.

Flags: needinfo?(tihuang)

Some missing context here: this test was added as part of a larger effort by Chris to let the Storage Access API be used while still getting the CSRF mitigations you might expect from removing third-party cookies.

:Gijs: https://github.com/web-platform-tests/wpt/blob/1708853580/storage-access-api/requestStorageAccess-cross-site-fetch.sub.https.window.js

:Tim: I'm not sure if this code path is using the storage access flag on the loadinfo that is conditionally cleared in Bug 1835907. It may be that when gathering cookies for the redirect we are just looking at the window's state, seeing that it is unpartitioned and happily grabbing the unpartitioned cookies. I think that logic may need to have a test for cross-origin redirects. Investigating now.

Flags: needinfo?(bvandersloot)

Thanks for adding that context Ben!

Dan -- yeah, step 3 of the repro steps is a CSRF exploit that relies on a flaw in the iframed site itself. But the browser has an opportunity to make that exploit impossible by removing the request's ambient authority after the cross-site redirect. Doing so gets cookies ever-so-slightly closer to following the Same-Origin Policy, at least when the Storage Access API is the mechanism that allows cookie access. (See also https://github.com/privacycg/storage-access/pull/213, where I'm suggesting that the Storage Access API should never make cookies accessible to an origin that hasn't explicitly opted into that access. That would fully align the Storage Access API with the Same-Origin Policy, and would go a long way to protecting users from CSRF attacks.)

Flags: needinfo?(chris.p.fredrickson)
Flags: needinfo?(bugmail)

@timhuang: Following your suggestion, this diff does fix it. The only difference is that this binds the redirect taint to origin, rather than site. We would have to add that to HTTP Base channels, which would also probably be best done if we add same-site-equivalents to the origin checks for principals and in the ScriptSecurityManager that this bit's calculation leans on. Those also might just be nice to generally have.

diff --git a/toolkit/components/antitracking/AntiTrackingUtils.cpp b/toolkit/components/antitracking/AntiTrackingUtils.cpp
index 79eec1c201e71..84faec05daf47 100644
--- a/toolkit/components/antitracking/AntiTrackingUtils.cpp
+++ b/toolkit/components/antitracking/AntiTrackingUtils.cpp
@@ -573,6 +573,12 @@ AntiTrackingUtils::GetStoragePermissionStateInParent(nsIChannel* aChannel) {
     return nsILoadInfo::NoStoragePermission;
   }

+
+  RefPtr<net::HttpBaseChannel> httpBaseChannel = do_QueryObject(aChannel);
+  if (httpBaseChannel && httpBaseChannel->HasRedirectTaintedOrigin()) {
+    return nsILoadInfo::NoStoragePermission;
+  }
+
   if (policyType == ExtContentPolicy::TYPE_SUBDOCUMENT) {
     // For loads of framed documents, we only use storage access
     // if the load is the result of a same-origin, same-site-initiated
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: sec-moderate
Assignee: nobody → bvandersloot
Status: NEW → ASSIGNED

I filed Bug 1956218 and will be resolving it via that.

The above patch landed. Let's close this out.

Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED

What versions are affected by this bug? Do we need to fix this for ESR-115 or ESR-128?

Depends on: 1956218
Flags: needinfo?(bvandersloot)
Whiteboard: [fixed by 1956218

This corrects behavior introduced in 132, so I'd say prior to that is invalid.

Flags: needinfo?(bvandersloot)
Group: dom-core-security → core-security-release
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Attachment #9474098 - Attachment is obsolete: true
Whiteboard: [fixed by 1956218 → [fixed by 1956218][adv-main138+]
Attached file advisory.txt โ€”
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: