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

Why Sec-FedCM-CSRF and not Sec-Fetch-Mode #320

Open
martinthomson opened this issue Jul 24, 2022 · 73 comments
Open

Why Sec-FedCM-CSRF and not Sec-Fetch-Mode #320

martinthomson opened this issue Jul 24, 2022 · 73 comments
Labels
compatibility risk Issues that may lead to backwards compatibility problems editorial needs resolution tpac2023

Comments

@martinthomson
Copy link

Sec-Fetch-Mode seems purpose-built for this sort of thing. Adding another header field doesn't really help a lot.

(A server will naturally ignore either a new Sec-Fetch-Mode value or the Sec-FedCM-CSRF thing. The value of the former is that it will compress better and it reuses an existing mechanism.

@npm1
Copy link
Collaborator

npm1 commented Aug 4, 2022

Is the proposal to add a new mode for Sec-Fetch-Mode, which would be added to all browser requests within the FedCM API? That seems reasonable to me.

@cbiesinger
Copy link
Collaborator

Hmm yeah looking at https://fetch.spec.whatwg.org/#concept-request-mode it seems like adding a fedcm method might be best. @annevk do you have any thoughts on that?

@domenic
Copy link

domenic commented Aug 11, 2022

I'm not sure about this. The fetch mode concept is currently used to figure out major modes of the processing model for fetch, in terms of what results in network errors, what kind of security properties are required, how credentials are processed, whether early hints are respected, whether responses are opaque, etc.

I can't find anywhere in the spec that actually invokes fetch in a rigorous way, e.g. says what request input is used to the fetch algorithm (this seems very bad!!). But I am hoping that the fetch that this spec does is just a normal "cors"-mode fetch. So inventing a new mode doesn't seem right, in that case.

@npm1
Copy link
Collaborator

npm1 commented Aug 12, 2022

Hmm thanks for the context. @martinthomson based on the above I suspect we should keep the new header? WDYT?

@bvandersloot-mozilla
Copy link
Collaborator

I can't find anywhere in the spec that actually invokes fetch in a rigorous way,

This was one of Anne's complaints as well. I think that it started to get defined in sections like check the root manifest, but isn't quite finished.

This spec doesn't actually do a normal "cors"-mode fetch- it has two types of request that are more specific. One is credential-less and referrer-less and another is credentialed.

One way forward that makes sense to me is making two new Sec-Fetch-Mode values, "fedcm-credentialless" and "fedcm-credentialed", which each specify the correct behavior within the constraints of Fetch. This also removes the need for the Sec-FedCM-CSRF header.

@domenic
Copy link

domenic commented Aug 15, 2022

I hope neither of those proposed fetches bypass CORS, and I hope even more ardently that neither of them rise to the level of behavioral differences that would require an entirely new fetch mode!!

Instead, it would make most sense if they were normal, "cors"-mode fetches, which set the referrer policy and credentials mode fields of the request appropriately.

@bvandersloot-mozilla
Copy link
Collaborator

Instead, it would make most sense if they were normal, "cors"-mode fetches, which set the referrer policy and credentials mode fields of the request appropriately.

Okay, that would work. Then we do still need the Sec-FedCM-CSRF header.

@martinthomson
Copy link
Author

Looking at this more closely, I don't think that the header is needed.

A fetch with the "cors" mode ensures that the content of a resource is not readable to a random site that invokes fetch() unless the (IDP) site deliberately enables access. To ensure that only the browser reads the response, the site needs to do exactly nothing. The site can use fetch headers to perform checks, and some amount of defense-in-depth is sensible here1, but the extra check is redundant.

For requests that mutate state (logout, token acquisition), we can insist upon a preflight by explicitly setting the use-CORS-preflight flag. But that doesn't get us what is needed. As far as I can tell, this is where things get a little sketchy because sites can set a referrer policy that prevents the inclusion of an Origin2. So a missing Origin is not a strong signal that the request comes from the UA.

An alternative is to use the CSRF token defense used by most sites: include a value in an earlier request that proves that the request is genuine.

In all cases, this only requires that the endpoint that the browser interacts have a URL with sufficient high entropy as to be sufficient hard to guess by a random site that invokes fetch3. I might even claim that this provides better defense than a passive signal like Sec-FedCM-CSRF because it doesn't require an additional check, though I guess that you could build an endpoint that doesn't pay any attention to a high-entropy URL component.

Footnotes

  1. You probably want to add X-Content-Type-Options: nosniff to the response. Then check Origin and Sec-Fetch-Dest and so forth.

  2. I can't see why we would allow script to make a cross-site request without Origin, so I might be missing something: the fetch spec is, as always, inscrutable.

  3. The simplest way to implement something like this is to have a per-user URL, with the URL encoding the identity of the user, a nonce, and an authentication tag; see also https://www.w3.org/TR/capability-urls/. Add a key identifier or version number and you can use the same symmetric key for many users.

@cbiesinger
Copy link
Collaborator

How would you provide an endpoint with a high-entropy URL, given that sites can just read it using a server-side proxying script?

@domenic since all of these fetches are browser-mediated and also we don't expose info to the website unless the user gives permission (by selecting an account), we do bypass CORS

@cbiesinger
Copy link
Collaborator

Also, in practice, even if we did use CORS, I'm sure IDPs will have to give access to everyone anyway since there's lots of IDPs and I doubt they want to run their CORS checks against the list every time.

@domenic
Copy link

domenic commented Aug 17, 2022

Bypassing CORS is VERY BAD and MUST NOT BE DONE for new features.

@martinthomson
Copy link
Author

I don't want this to seem facetious, but we bypass CORS for DoH. That said, I agree with @domenic in this case: this can use CORS.

@cbiesinger
Copy link
Collaborator

Well, that is a very strong statement. We can probably support CORS, but why do you say that not using it is so bad in this case?

@cbiesinger
Copy link
Collaborator

Also, should we require CORS even for the uncredentialed requests?

@cbiesinger
Copy link
Collaborator

To be honest, requiring CORS would make security worse here IMO because then the accounts endpoint becomes world-readable from websites instead of only to the request mediated by the UA.

@martinthomson
Copy link
Author

requiring CORS [...] the accounts endpoint becomes world-readable

With CORS, the resource decides what is and isn't readable. By default (except under some narrow conditions), content is not readable by any site. The browser can see it, but it won't pass that information on unless the site explicitly permits it.

@yi-gu
Copy link
Collaborator

yi-gu commented Aug 19, 2022

We don't want the IDP to know about the RP when fetching the accounts so at least CORS cannot be used there. In addition, since the "*" wildcard cannot be specified when responding to a credentialed requests request, the token request cannot use CORS, right? Otherwise the IDP must specify ALL the RP origins.

More importantly, there are 3 special things about FedCM API:

  1. the credentialed response is not visible to the RP's process with site isolation
  2. the URLs are not controlled by the RP (potential attacker), but are described in the IDP's manifest. i.e. they are fully controlled by the potential victim.
  3. it's true that the UI is triggered by a script running on the RP, however the UI is closer to the IDP than to the RP w.r.t. source and access (especially before user granting permission). e.g. when fetching the user picture from the IDP to display on the UI, the CSP img-src directive on the RP should not be applied because it's never exposed / used on the RP.

Therefore I think the current "Sec-" header without CORS solution makes more sense in this case.

@domenic
Copy link

domenic commented Aug 19, 2022

I'd encourage you to involve the security teams at your browsers if you are thinking of introducing another hole in the same origin model by bypassing CORS. We've previously gotten consensus across the web community that such violations are not acceptable, and I'm disappointed to see people trying to bypass it yet again. I don't really have the energy to continue litigating these issues, so I am just going to hope that such features get blocked in per-browser security review and cross-browser TAG review, in accordance with previous resolutions.

@bvandersloot-mozilla
Copy link
Collaborator

We don't want the IDP to know about the RP when fetching the accounts so at least CORS cannot be used there. In addition, since the "*" wildcard cannot be specified when responding to a credentialed requests request, the token request cannot use CORS, right? Otherwise the IDP must specify ALL the RP origins.

CORS preflight requests are sent with Referer-Policy "same-origin" which seems acceptable.
The Origin header will only be sent if the response tainting gets set to "cors", which I think we can manage to avoid for our non-credentialed requests.
Also, Access-Control-Allow-Origin may be taken from the Origin request header.

I think it is entirely possible for this request to be CORS-protected, and it makes sense to use it since the result does end up in the RP's content process.

Scrolling back, I think I agree with Martin that we don't even need the Sec-FedCM-CSRF header. We already have a high-entropy component in the request to fetch a token (nonce), so all of that discussion of high-entropy URL components is moot. What is the property we are defending by making sure the endpoints are only hit by the browser API? I can't come up with a good answer, but I admit I haven't gone to the scrollback of the original inclusion.

@bvandersloot-mozilla
Copy link
Collaborator

bvandersloot-mozilla commented Aug 22, 2022

CORS preflight requests are sent with Referer-Policy "same-origin" which seems acceptable. The Origin header will only be sent if the response tainting gets set to "cors", which I think we can manage to avoid for our non-credentialed requests.

Looking more closely, I don't think this is actually true. It looks like our non-credentialed requests would hit this fallthrough case. Unless there is a mechanism for suppressing the Origin header in CORS that I am missing, this is bad news.

@annevk
Copy link

annevk commented Aug 23, 2022

-Mode indeed seems wrongish, but what about -Dest? This does seem like it might warrant a new destination type.

@bvandersloot-mozilla
Copy link
Collaborator

It looks like our non-credentialed requests would hit this fallthrough case. Unless there is a mechanism for suppressing the Origin header in CORS that I am missing, this is bad news.

Continuing the conversation with myself. I think this is fine if we perform the uncredentialed fetches with a de-priveleged client: one whose origin is opaque. This would set the Origin header to null for those requests and be fetch-spec compatible. We have the idea of a Null security principal in Gecko that is useful for de-privileging.

-Mode indeed seems wrongish, but what about -Dest? This does seem like it might warrant a new destination type.

I like -Dest the most of the proposed options (-Dest, -Mode, and Sec-FedCM-CSRF). This also adds the ability for a developer to only allow "credential" requests and not arbitrary fetches using their CSP.

@domfarolino
Copy link

Drive-by:

I think this is fine if we perform the uncredentialed fetches with a de-priveleged client: one whose origin is opaque. This would set the Origin header to null for those requests and be fetch-spec compatible.

Out of curiosity, what would be the need for nulling out the Origin for the uncredentialed request? IIUC, this is the request with which we're specifically OK with exposing the RP to the IDP (via the client id), right? So is there a harm in keeping the origin too? Does that expose any more information?

@npm1
Copy link
Collaborator

npm1 commented Aug 23, 2022

I agree that there should not be a problem with the uncredentialed requests here, and I think they can use regular fetch. The complicated one is the credentialed one, which is one where we might need fetch expert help. I don't think doing a regular CORS fetch (with Origin being the RP) would work. As Christian pointed out, that would give the RPs the power to do those fetches outside of FedCM and read the accounts information.

@bvandersloot-mozilla
Copy link
Collaborator

Drive-by:

I think this is fine if we perform the uncredentialed fetches with a de-priveleged client: one whose origin is opaque. This would set the Origin header to null for those requests and be fetch-spec compatible.

Out of curiosity, what would be the need for nulling out the Origin for the uncredentialed request? IIUC, this is the request with which we're specifically OK with exposing the RP to the IDP (via the client id), right? So is there a harm in keeping the origin too? Does that expose any more information?

I was specifically talking about the requests for the manifests, which should not expose the RP to the IDP.

@bvandersloot-mozilla
Copy link
Collaborator

I agree that there should not be a problem with the uncredentialed requests here, and I think they can use regular fetch. The complicated one is the credentialed one, which is one where we might need fetch expert help. I don't think doing a regular CORS fetch (with Origin being the RP) would work. As Christian pointed out, that would give the RPs the power to do those fetches outside of FedCM and read the accounts information.

@npm1: That makes a lot of sense. Thanks for the clarification. If we add a new Sec-fetch-dest value of "credential", the IDP can validate that header in the request with an identical guarantee that the client is not initiating the request except through this API. And it has the added benefit of integrating into the Content Security Policy so the RP can make restrictive policy decisions as well.

@annevk
Copy link

annevk commented Aug 24, 2022

The same-origin policy does not care about whether a request is credentialed. (CORS does, but that's almost entirely about the level of opt-in we require.) I'm not sure why you all are saying that is significant? Note in particular that IP-address-authenticated servers are still a thing, to my knowledge.

@johannhof
Copy link
Member

The point was raised in #320 (comment) and I think it's central to this discussion. As to my knowledge, CORS in order to work requires the server to have knowledge of the request Origin. A central privacy guarantee in FedCM for at least one of the credentialed requests is that it happens without any association to the originating site (i.e. with a no-referrer policy).

@annevk
Copy link

annevk commented Aug 24, 2022

Not necessarily, if the origin is an opaque origin, you could respond with `null`, even with credentialed fetches. Normally you wouldn't really do that, but perhaps for a dedicated endpoint it's okay.

@npm1
Copy link
Collaborator

npm1 commented Nov 3, 2022

That sounds right to me. Regarding preflights, I believe we concluded that FedCM fetches specifically do not require preflights.

@cbiesinger
Copy link
Collaborator

With regards to the new mode, we should make sure that the spec will let us send first-party cookies while also sending a third-party Origin header (fyi @bvandersloot-mozilla )

@bvandersloot-mozilla
Copy link
Collaborator

I've created a first draft here: whatwg/fetch#1533.

noamr added a commit to noamr/fetch that referenced this issue Jan 2, 2023
This intends to be a summary of informative notes in
the #Requests section, into a concise "starter" guide
to populating a request, which is usually the tricky
bit of using Fetch.

Requested by authors of other specs, see for example:
w3ctag/design-principles#238
fedidcg/FedCM#320
noamr added a commit to noamr/fetch that referenced this issue Jan 2, 2023
This intends to be a summary of informative notes in
the #Requests section, into a concise "starter" guide
to populating a request, which is usually the tricky
bit of using Fetch.

Requested by authors of other specs, see for example:
w3ctag/design-principles#238
fedidcg/FedCM#320
noamr added a commit to noamr/fetch that referenced this issue Jan 4, 2023
This intends to be a summary of informative notes in
the #Requests section, into a concise "starter" guide
to populating a request, which is usually the tricky
bit of using Fetch.

Requested by authors of other specs, see for example:
w3ctag/design-principles#238
fedidcg/FedCM#320
noamr added a commit to noamr/fetch that referenced this issue Jan 18, 2023
This intends to be a summary of informative notes in
the #Requests section, into a concise "starter" guide
to populating a request, which is usually the tricky
bit of using Fetch.

Requested by authors of other specs, see for example:
w3ctag/design-principles#238
fedidcg/FedCM#320
@samuelgoto samuelgoto removed the agenda+ Regular CG meeting agenda items label Mar 29, 2023
@npm1
Copy link
Collaborator

npm1 commented Mar 12, 2024

An update based on the breakout that happened today. We aligned on @domfarolino's proposal regarding how to treat the accounts endpoint. It will be treated as same-origin, with a null client, and using only SameSite=None cookies. We also briefly discussed the suggestion to add a new header to our CORS request and aligned against it, see the CORS issue #428.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 12, 2024
See fedidcg/FedCM#320 (comment)

Bug: 329145816
Change-Id: I6408255a01118cd5ac4d0d0263a34051796dc301
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 13, 2024
See fedidcg/FedCM#320 (comment)

Bug: 329145816
Change-Id: I6408255a01118cd5ac4d0d0263a34051796dc301
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 13, 2024
See fedidcg/FedCM#320 (comment)

Bug: 329145816
Change-Id: I6408255a01118cd5ac4d0d0263a34051796dc301
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 14, 2024
See fedidcg/FedCM#320 (comment)

Bug: 329145816
Change-Id: I6408255a01118cd5ac4d0d0263a34051796dc301
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 15, 2024
See fedidcg/FedCM#320 (comment)

This is behind the off-by-default "FedCmSameSiteNone" feature.

Bug: 329145816
Change-Id: I6408255a01118cd5ac4d0d0263a34051796dc301
aarongable pushed a commit to chromium/chromium that referenced this issue Mar 15, 2024
See fedidcg/FedCM#320 (comment)

This is behind the off-by-default "FedCmSameSiteNone" feature.

Bug: 329145816
Change-Id: I6408255a01118cd5ac4d0d0263a34051796dc301
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5366009
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Nicolás Peña <npm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1273426}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 15, 2024
See fedidcg/FedCM#320 (comment)

This is behind the off-by-default "FedCmSameSiteNone" feature.

Bug: 329145816
Change-Id: I6408255a01118cd5ac4d0d0263a34051796dc301
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5366009
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Nicolás Peña <npm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1273426}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 15, 2024
See fedidcg/FedCM#320 (comment)

This is behind the off-by-default "FedCmSameSiteNone" feature.

Bug: 329145816
Change-Id: I6408255a01118cd5ac4d0d0263a34051796dc301
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5366009
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Nicolás Peña <npm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1273426}
@bvandersloot-mozilla
Copy link
Collaborator

That seems like a reasonable state for this. Sorry I couldn't be at the breakout.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 21, 2024
…es for FedCM requests, a=testonly

Automatic update from web-platform-tests
[FedCM] Don't send SameSite=Strict cookies for FedCM requests

See fedidcg/FedCM#320 (comment)

This is behind the off-by-default "FedCmSameSiteNone" feature.

Bug: 329145816
Change-Id: I6408255a01118cd5ac4d0d0263a34051796dc301
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5366009
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Nicolás Peña <npm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1273426}

--

wpt-commits: 08c2f13fa0d6bf961ab2e80b0db0a958ef991ee9
wpt-pr: 45066
BruceDai pushed a commit to BruceDai/wpt that referenced this issue Mar 25, 2024
See fedidcg/FedCM#320 (comment)

This is behind the off-by-default "FedCmSameSiteNone" feature.

Bug: 329145816
Change-Id: I6408255a01118cd5ac4d0d0263a34051796dc301
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5366009
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Nicolás Peña <npm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1273426}
ErichDonGubler pushed a commit to ErichDonGubler/firefox that referenced this issue Mar 25, 2024
…es for FedCM requests, a=testonly

Automatic update from web-platform-tests
[FedCM] Don't send SameSite=Strict cookies for FedCM requests

See fedidcg/FedCM#320 (comment)

This is behind the off-by-default "FedCmSameSiteNone" feature.

Bug: 329145816
Change-Id: I6408255a01118cd5ac4d0d0263a34051796dc301
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5366009
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Nicolás Peña <npm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1273426}

--

wpt-commits: 08c2f13fa0d6bf961ab2e80b0db0a958ef991ee9
wpt-pr: 45066
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility risk Issues that may lead to backwards compatibility problems editorial needs resolution tpac2023