Skip to content
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

environment/has storage access is not cleared during cross-site redirects #210

Open
cfredric opened this issue Nov 15, 2024 · 11 comments
Open

Comments

@cfredric
Copy link
Contributor

cfredric commented Nov 15, 2024

The spec defines a has storage access boolean on environment, which gets set to true after a successful call to document.requestStorageAccess(). That bool's purpose is to be read by the cookie store, when attaching cookies to an outgoing request.

However, the environment's has storage access boolean is a latch: once it's set to true, it's never reset to false (for a given environment).

This means that, at least as specified currently, an iframe (on site a.com, embedded in top) could call document.requestStorageAccess(), then make a fetch to a cross-site endpoint evil.com/foo. That evil.com endpoint could respond with a redirect back to a.com/deletemyaccount, and then the user agent's fetch of a.com/deletemyaccount would include cookies due to the environment's has storage access.

Clearly that's not intended; the cookie store ought to read a boolean that gets cleared after a cross-site redirect. I think this implies that request ought to have its own has storage access boolean (naming TBD), which can be cleared after such a redirect without affecting the environment's has storage access.


But there's another interesting question to consider: should that new request/has storage access boolean also be cleared after a same-site but cross-origin redirect?

On one hand, clearing it would be consistent with the Same Origin Policy. And if Storage Access Headers is implemented in all major browsers, then sites would have an easy way to opt back in to including cookies, post-redirect: just supply the Activate-Storage-Access: retry; ... response header to retry the request with cookies. But clearing the bool isn't totally backward compatible, it would require sites to adopt SAH if needed.

On the other hand, cookies don't adhere to the Same Origin Policy in the first place, so it is more consistent if all their semantics are based around registerable domains, without some pieces being origin-based. Preserving the bool (and therefore including cookies after the redirect) would also be consistent with the behavior the iframe would have seen if there had been no redirect: if the iframe directly fetched the cross-origin, same-site URL, then the request would implicitly include cookies due to the environment's has storage access. This was intended behavior, since the cookies' model was based around registered domains, rather than origins (and therefore, so is the storage-access permission scope).


This question is relevant to me because I'm working on the SAH spec (draft), and realized that it's stricter than the SAA spec in this respect; it clears its new boolean on cross-origin redirects. But it'd be good to align on some shared model and intentional behavior, so that the origin/site behavior doesn't vary based on which particular API surface was used.

@annevk
Copy link
Collaborator

annevk commented Nov 16, 2024

What do implementations do today?

@HeikoTheissen
Copy link

HeikoTheissen commented Nov 20, 2024

should that new request/has storage access boolean also be cleared after a same-site but cross-origin redirect?

The following scenario gives a reason why clearing it is preferable.

We have

  • a portal on a.com which embeds a cross-site iframe with
  • an application on app.b.com which starts (in the same iframe) a logon flow to
  • an identity provider (IdP) on idp.b.com, which had previously set an unpartitioned session cookie.

Storage access permission for b.com embedded in a.com had previously been granted.

In Chrome 133.0.6844.0

"win64-133.0.6844.0/chrome-win64/chrome.exe"
--incognito --disable-sync --no-first-run --disable-extensions --remote-debugging-port=0
--flag-switches-begin
  --test-third-party-cookie-phaseout
  --enable-features=StorageAccessHeaders
  --disable-features=
    TopLevelTpcdSupportSettings,
    TpcdHeuristicsGrants,
    TpcdMetadataGrants,
    TpcdSupportSettings
--flag-switches-end

we observe the following behavior:

Request Origin Sec-Fetch-Storage-Access document .hasStorageAccess()
Launch app app.b.com inactive false
App redirects* to IdP idp.b.com inactive false
IdP reloads after calling document.requestStorageAccess() idp.b.com active true
IdP receives its unpartitioned session cookie and redirects* to App app.b.com active1 false2
App makes fetch request app.b.com inactive3 false

* redirect via form submission

  1. Fetches include cookies (via SAA) across same-site redirects/navigations. Our app issues partitioned or unpartitioned session cookies based on the Sec-Fetch-Storage-Access header, active means unpartitioned session cookie.
  2. Storage access is inherited in new document only across same-origin navigations.
  3. Since the app has not called document.requestStorageAccess(), it makes its requests with Sec-Fetch-Storage-Access: inactive and without the unpartitioned session cookie (hence they fail).

Calling document.requestStorageAccess() in all our apps is infeasible (especially since it is needed only if app and IdP are same-site). The alternative of responding with Activate-Storage-Access would lead to extra sequential requests (privacycg/storage-access-headers#6).

Instead, we prefer a same-site cross-origin redirection like 1 above to be made with Sec-Fetch-Storage-Access: inactive so that the app issues a partitioned session cookie.

Addendum: We want the same behavior also if the redirects are HTTP-redirects (rather than triggered by form submission). The current behavior is:

Request Origin Sec-Fetch-Storage-Access document .hasStorageAccess() Leg
Launch app app.b.com inactive n/a pre-redirect
App redirects* to IdP idp.b.com inactive false post-redirect
IdP reloads after calling document.requestStorageAccess() idp.b.com active n/a pre-redirect
IdP receives its unpartitioned session cookie and redirects* to App app.b.com active1 false2 post-redirect
App makes fetch request app.b.com inactive3 false

* HTTP redirect

@annevk
Copy link
Collaborator

annevk commented Nov 20, 2024

@HeikoTheissen that seems completely off-topic. I'm going to mark it as such.

@johannhof
Copy link
Member

@annevk sorry, why is it off-topic? It describes the current behavior in Chrome and makes a case for a certain behavior based on their business logic. I'm going to unhide it for now but don't want to get into an edit war so happy to chat about it directly if you disagree :)

@cfredric
Copy link
Contributor Author

cfredric commented Dec 3, 2024

What do implementations do today?

I assume you're referring to the cross-origin (but same-site) redirect case, since maintaining the storage-access opt-in after a cross-site redirect is pretty clearly a CSRF vector.

For the cross-origin, same-site redirect case:

  • Chromium maintains storage access (i.e. sends cookies) post-redirect
  • Firefox (133.0) maintains storage access post-redirect
  • I don't have an Apple device, so I can't test Safari. @annevk do you know what Safari does?

IMO, we ought to make the browser follow the same origin policy here, i.e. stop implicitly maintaining storage access after the cross-origin redirect. SAH gives an easy way to re-opt-in to using a storage access permission grant, so this change would be a strict security win with no utility cost. Also as Heiko points out, web applications are already built around the assumption that different origins are isolated from each other, so IMO we should strive for consistency with that model by default (even if there's also an escape hatch via SAH).

@HeikoTheissen
Copy link

HeikoTheissen commented Dec 9, 2024

IMO, we ought to make the browser follow the same origin policy here, i.e. stop implicitly maintaining storage access after the cross-origin redirect.

I agree, but the same should be true for cross-origin navigation ("redirect triggered by form submission"), not just for HTTP redirect. Both can occur during a SAML logon flow:

  • HTTP redirect occurs with SAML HTTP Redirect Binding (SAML Binding spec, section 3.4).
  • Redirect via form submission occurs with SAML HTTP POST Binding (section 3.5).

The type of SAML binding should not affect the storage access behavior (the same argument was made for the ancestor chain bit).

@bvandersloot-mozilla
Copy link
Collaborator

It really should be cleared, per the spec and the WPT. I believe this is already the case.

spec: https://privacycg.github.io/storage-access/#navigation
relevant test: https://github.com/web-platform-tests/wpt/blob/ef413686d1/storage-access-api/requestStorageAccess-cross-origin-iframe-navigation.sub.https.window.js

An interesting question here is making sure that the document's request's access to cookies matches the resulting document's bit

@annevk
Copy link
Collaborator

annevk commented Dec 10, 2024

We should test these cases:

  • A with a nested document B. Where B navigates B to A2.
  • A with a nested document B. Where B navigates B to C.
  • A with a nested document B. Where B navigates B to C which redirects to B2.
  • A with a nested document B. Where B navigates B to B2 which redirects to A2.
  • A with a nested document B. Where B navigates B to B2 which redirects to C.
  • A with a nested document B. Where A navigates B to A2.
  • A with a nested document B. Where A navigates B to C.
  • A with a nested document B. Where A navigates B to C which redirects to B2.
  • A with a nested document B. Where A navigates B to B2 which redirects to A2.
  • A with a nested document B. Where A navigates B to B2 which redirects to C.

And then there's subresource cases:

  • A with a nested document B. Where B subresources A2.
  • A with a nested document B. Where B subresources C.
  • A with a nested document B. Where B subresources C which redirects to B2.
  • A with a nested document B. Where B subresources B2 which redirects to A2.
  • A with a nested document B. Where B subresources B2 which redirects to C.

In all of these cases different letters are different sites (we should also answer the site vs origin question, requiring some more tests), and the different numbers represent different resources within the same site.

@johannhof
Copy link
Member

(We looked at this in our editor's call)

Yeah, to add to the above - what happens to subsequent navigational requests in an iframe if a document called document.rSA() in that iframe? I think we should strive to always keep "credentials for document load" consistent with "storage access for subresources on that document", so that developers don't end up wrongly inferring availability of credentials in one place based on the other.

@bvandersloot-mozilla
Copy link
Collaborator

I've got a patch for the WPT to make sure we keep the document load credentials consistent with the subresource credentials. I'll look to get that landed. But Firefox does still pass that.

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Dec 11, 2024
…tials

Some of the tests discussed in privacycg/storage-access#210

Differential Revision: https://phabricator.services.mozilla.com/D231720

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1936355
gecko-commit: 8db61a9fffaff233a8ff6648033afd8f67a52f63
gecko-reviewers: anti-tracking-reviewers, timhuang
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Dec 11, 2024
…t fetch credentials - r=anti-tracking-reviewers,timhuang

Some of the tests discussed in privacycg/storage-access#210

Differential Revision: https://phabricator.services.mozilla.com/D231720
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Dec 12, 2024
…tials

Some of the tests discussed in privacycg/storage-access#210

Differential Revision: https://phabricator.services.mozilla.com/D231720

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1936355
gecko-commit: 8db61a9fffaff233a8ff6648033afd8f67a52f63
gecko-reviewers: anti-tracking-reviewers, timhuang
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Dec 12, 2024
…t fetch credentials - r=anti-tracking-reviewers,timhuang

Some of the tests discussed in privacycg/storage-access#210

Differential Revision: https://phabricator.services.mozilla.com/D231720
@cfredric
Copy link
Contributor Author

It really should be cleared, per the spec and the WPT. I believe this is already the case.

spec: https://privacycg.github.io/storage-access/#navigation

That section of the spec is a modification of how HTML handles navigations. We should also define how Fetch handles redirects for subresource requests, since that's hand-waved at the moment.

gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Dec 13, 2024
…t fetch credentials - r=anti-tracking-reviewers,timhuang

Some of the tests discussed in privacycg/storage-access#210

Differential Revision: https://phabricator.services.mozilla.com/D231720

UltraBlame original commit: 8db61a9fffaff233a8ff6648033afd8f67a52f63
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Dec 13, 2024
…t fetch credentials - r=anti-tracking-reviewers,timhuang

Some of the tests discussed in privacycg/storage-access#210

Differential Revision: https://phabricator.services.mozilla.com/D231720

UltraBlame original commit: 8db61a9fffaff233a8ff6648033afd8f67a52f63
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Dec 13, 2024
…t fetch credentials - r=anti-tracking-reviewers,timhuang

Some of the tests discussed in privacycg/storage-access#210

Differential Revision: https://phabricator.services.mozilla.com/D231720

UltraBlame original commit: 8db61a9fffaff233a8ff6648033afd8f67a52f63
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Dec 19, 2024
…t fetch credentials - r=anti-tracking-reviewers,timhuang

Some of the tests discussed in privacycg/storage-access#210

Differential Revision: https://phabricator.services.mozilla.com/D231720
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants