Storage access "opt-in" is not reset after a cross-site redirect
Categories
(Core :: Privacy: Anti-Tracking, defect)
Tracking
()
| 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)
|
342 bytes,
text/plain
|
Details |
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.
- Navigate to a page on https://a.com, which embeds an iframe from https://b.com.
- In the iframe, execute
document.requestStorageAccess()and accept the permission prompt. - 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.
Comment 1•9 months ago
|
||
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?
Comment 2•9 months ago
|
||
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.)
Comment 3•9 months ago
|
||
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.
| Assignee | ||
Comment 4•9 months ago
|
||
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.
: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.
| Reporter | ||
Comment 5•9 months ago
|
||
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.)
Updated•9 months ago
|
| Assignee | ||
Comment 6•9 months ago
|
||
@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
Updated•9 months ago
|
Updated•9 months ago
|
| Assignee | ||
Comment 7•9 months ago
|
||
Updated•9 months ago
|
| Assignee | ||
Comment 8•9 months ago
•
|
||
I filed Bug 1956218 and will be resolving it via that.
| Assignee | ||
Comment 9•9 months ago
|
||
The above patch landed. Let's close this out.
Comment 10•9 months ago
|
||
What versions are affected by this bug? Do we need to fix this for ESR-115 or ESR-128?
| Assignee | ||
Comment 11•9 months ago
|
||
This corrects behavior introduced in 132, so I'd say prior to that is invalid.
Updated•9 months ago
|
Updated•8 months ago
|
Updated•8 months ago
|
Updated•8 months ago
|
Updated•8 months ago
|
Comment 12•8 months ago
|
||
Updated•3 months ago
|
Description
•