-
Notifications
You must be signed in to change notification settings - Fork 42
Adhere to Same Origin Policy more strictly #213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
bvandersloot-mozilla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if the tentative WPT had more coverage of the cases Anne outlined here, although I admit I didn't update them either.
storage-access.bs
Outdated
|
|
||
| 1. If |request|'s [=request/client=] is null, return false. | ||
| 1. If |request|'s [=request/client=]'s [=environment/has storage access=] is false, return false. | ||
| 1. If |request|'s [=url/origin=] is not [=same origin=] with |request|'s [=request/url=]'s [=url/origin=], return false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this makes a storage access grant only apply to a single origin, rather than the entire site?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and no. This makes the storage access grant be initially "active" only for the origin of the document that called document.requestStorageAccess(), rather than the entire site. But if another origin needs to access cookies, it can use document.requestStorageAccess() itself (which will resolve without a user gesture or a prompt), or activate the permission a different way (e.g. Storage Access Headers).
The effect of this is to keep the privacy boundary the same (i.e. keyed on <site, site>), but restrict the security model to be origin-based rather than site-based. So, one origin's use of SAA doesn't make the other origins of the site vulnerable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me clarify: it makes document.requestStorageAccess() only apply to the origin of the document?
I'd be more comfortable with this if there were a positive signal from Apple around Storage Access Headers, because this further leans on that feature for subresources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should fix the cross-origin-same-site navigation fetch to not send unpartitioned cookies, however the subresource fetch inside of the document is the part that leans on Storage Access Headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me clarify: it makes document.requestStorageAccess() only apply to the origin of the document?
document.requestStorageAccess() will still request a permission that applies to the whole site, but only the document's origin is opted-in and receives cookie access as a result of the call. Other origins have to explicitly activate the permission by calling document.requestStorageAccess() themselves, if they also need cookie access.
if there were a positive signal from Apple around Storage Access Headers
FWIW, Anne wrote that SAH seems like a reasonable extension of Storage Access, and opened one issue about worker support (specifically for the Activate-Storage-Access: load header). He didn't mention any concerns about the Activate-Storage-Access: retry; ... part, which is what this would lean on. That's not a formal positive signal, though, to be fair.
I agree that we should fix the cross-origin-same-site navigation fetch to not send unpartitioned cookies, however the subresource fetch inside of the document is the part that leans on Storage Access Headers.
Are you saying that you'd expect SAA's Fetch integration to treat navigation fetches and subresource fetches differently? My assumption was that SAA's usage of Fetch would be agnostic to the request's destination, so that fetches of different kinds of resources get the same kind of CSRF protection (whether it's a navigation or not); i.e., whether cookies are included wouldn't depend on what exactly the page is fetching. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other origins have to explicitly activate the permission by calling document.requestStorageAccess() themselves, if they also need cookie access.
So they would need to create another iframe to do that (excepting SAH for subresources)?
Are you saying that you'd expect SAA's Fetch integration to treat navigation fetches and subresource fetches differently?
Yeah, that is the way I was thinking. In my mind you are fetching resources that are going into a different context than the current one, so it is natural to treat them as the destination context rather than the source context.
The way I'm thinking about this is like this. The storage access API breaks open the (sub)document to use its unpartitioned cookies. An iframe provider controls security by dictating what Origins can request the API. We permit the same-origin self-initiated navigations to persist the openness to allow document fetches out of convenience to developers without relaxing the security model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So they would need to create another iframe to do that (excepting SAH for subresources)?
That's right. The principle is that if an origin didn't opt into cross-site cookie access, then the browser doesn't send cookies to it. And the two ways of opting in are:
- Call
document.requestStorageAccess()in a document which is same-origin with the server endpoint that wants to receive cookies. - Use SAH.
Yeah, that is the way I was thinking. In my mind you are fetching resources that are going into a different context than the current one, so it is natural to treat them as the destination context rather than the source context.
I think that model might be okay from a privacy POV, but not from a web security POV. From a security POV, we need to consider the source of the fetch too.
IMO this is easiest to see if we start by thinking about cross-site fetches: we know that when a malicious site can send unfettered credentialed requests to another site, it can carry out CSRF attacks. Whether the fetch is a navigation, or just a subresource fetch, doesn't matter w.r.t. CSRF.
But the web security model is the Same-Origin Policy, so the web's security principals are origins, not sites. So we can modify the CSRF attack (s/site/origin/g): a malicious origin should not be able to send credentialed requests to another origin. (At least not without that origin's prior go-ahead -- CORS' ACAC header is prior art here.)
This PR aims to achieve that protection -- at least whenever the SAA is the mechanism by which cookies are accessible.
|
Re: the WPT test cases Anne listed, AFAIK the WPT framework only provides two distinct domains, so we'd have to add a new one if we wanted to write the tests that involve all of A, B, and C. The semantics in this PR don't make a distinction between navigations vs subresources vs non-subresource requests (i.e. this PR doesn't involve the request's destination or "navigation" at all), so I chose not to do all the navigation-based variations. The cases I wrote were:
Combined with the rest of the WPT test coverage we have, I think that's a good start. (We already have "A has a nested document B, which subresources B" and similar, I think.) |
johannhof
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks good to me, but I want to be mindful that this could be a big change in web-observable behavior for some embedded third parties. It would change how SAA inherits storage access today in Firefox and Safari, and also how developers got to try it out in our initial 1% experiment in Chrome.
From a Chrome perspective I think we'd like to try and ship this security improvement (pending approval by Blink API Owners etc.), especially given that the old behavior should be easy to restore with SAH. But I'd leave it to @bvandersloot-mozilla and @annevk whether they're comfortable merging this behavior into the spec.
An alternative would be to factor the backwards-incompatible change out of the PR and merge this PR without it.
|
I think it may make sense to hold off merging this until we are sure the change to only effect the requesting Origin it is something that we can ship for compat reasons. The other part of this is something we should probably do though. |
|
I've split this PR into two: #214 adds the request infrastructure so that we can define the current behavior in Chrome/Firefox, and this PR is now stacked on top of #214 and includes the behavior change ( I also incorporated @annevk's feedback about using a tri-state instead of a boolean. |
|
Since #214 has now been merged, I've rebased this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems good to try. We should be prepared to revert course on this if we run into issues however. We also want to exclude tests for this from Interop 2025.
(You can consider WebKit interested.)
bvandersloot-mozilla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto Anne, but s/Webkit/Gecko/
request integration with stricter Same Origin Policy adherence|
So, should we merge this PR, despite not having multiple implementers yet? |
|
The template in OP should be completed, but I don't see why we wouldn't merge it? To merge you need implementer interest, not concrete implementations. |
|
Ok, yeah, I agree, that makes sense. @cfredric can you complete the remaining tasks? Then I'm happy to merge this. |
|
Thanks all, I filed bugs and added links to them. |
privacycg/storage-access#213 has been merged into the spec, so this behavior is no longer tentative. Bug: 379030052 Change-Id: I03c1fb1881f4cf6003cec7506ef0d3691d2ff1a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6454445 Reviewed-by: Aaron Selya <selya@google.com> Auto-Submit: Chris Fredrickson <cfredric@chromium.org> Commit-Queue: Chris Fredrickson <cfredric@chromium.org> Commit-Queue: Aaron Selya <selya@google.com> Cr-Commit-Position: refs/heads/main@{#1446536}
privacycg/storage-access#213 has been merged into the spec, so this behavior is no longer tentative. Bug: 379030052 Change-Id: I03c1fb1881f4cf6003cec7506ef0d3691d2ff1a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6454445 Reviewed-by: Aaron Selya <selya@google.com> Auto-Submit: Chris Fredrickson <cfredric@chromium.org> Commit-Queue: Chris Fredrickson <cfredric@chromium.org> Commit-Queue: Aaron Selya <selya@google.com> Cr-Commit-Position: refs/heads/main@{#1446536}
privacycg/storage-access#213 has been merged into the spec, so this behavior is no longer tentative. Bug: 379030052 Change-Id: I03c1fb1881f4cf6003cec7506ef0d3691d2ff1a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6454445 Reviewed-by: Aaron Selya <selya@google.com> Auto-Submit: Chris Fredrickson <cfredric@chromium.org> Commit-Queue: Chris Fredrickson <cfredric@chromium.org> Commit-Queue: Aaron Selya <selya@google.com> Cr-Commit-Position: refs/heads/main@{#1446536}
…ive", a=testonly Automatic update from web-platform-tests [SAA] Rename SOP tests to remove "tentative" privacycg/storage-access#213 has been merged into the spec, so this behavior is no longer tentative. Bug: 379030052 Change-Id: I03c1fb1881f4cf6003cec7506ef0d3691d2ff1a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6454445 Reviewed-by: Aaron Selya <selya@google.com> Auto-Submit: Chris Fredrickson <cfredric@chromium.org> Commit-Queue: Chris Fredrickson <cfredric@chromium.org> Commit-Queue: Aaron Selya <selya@google.com> Cr-Commit-Position: refs/heads/main@{#1446536} -- wpt-commits: 84067a927d3b9d8430a0abbc91bdf1370e7084d8 wpt-pr: 51972
…ive", a=testonly Automatic update from web-platform-tests [SAA] Rename SOP tests to remove "tentative" privacycg/storage-access#213 has been merged into the spec, so this behavior is no longer tentative. Bug: 379030052 Change-Id: I03c1fb1881f4cf6003cec7506ef0d3691d2ff1a8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6454445 Reviewed-by: Aaron Selya <selya@google.com> Auto-Submit: Chris Fredrickson <cfredric@chromium.org> Commit-Queue: Chris Fredrickson <cfredric@chromium.org> Commit-Queue: Aaron Selya <selya@google.com> Cr-Commit-Position: refs/heads/main@{#1446536} -- wpt-commits: 84067a927d3b9d8430a0abbc91bdf1370e7084d8 wpt-pr: 51972
This ships https://chromestatus.com/feature/5169937372676096, which was approved in https://groups.google.com/a/chromium.org/g/blink-dev/c/jot2b6e0ewo/m/iDXIfzNJAgAJ Note that this feature improves security of the API (by adhering to the SOP), but does not change any privacy properties. Spec change: privacycg/storage-access#213 Fixed: 379030052 Change-Id: I7d8d97b1cffe131f01c279df03a7b11bd6616c1f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6281077 Commit-Queue: Maks Orlovich <morlovich@chromium.org> Auto-Submit: Chris Fredrickson <cfredric@chromium.org> Reviewed-by: Maks Orlovich <morlovich@chromium.org> Cr-Commit-Position: refs/heads/main@{#1498795}
This is meant to address the ambiguities pointed out in #210 by defining Storage Access's integration with Fetch more rigorously. This PR codifies the behavior I'd like to Storage Access implementations to align on (namely stricter adherence to the Same Origin Policy).
Preview | Diff