-
Notifications
You must be signed in to change notification settings - Fork 73
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
Enforce CORS on the Identity Assertions endpoint #428
Comments
(I know there was a longer discussion regarding CORS in #320, but it seems to me that there we were mostly concerned with the requests which did not want to disclose the RP to the IdP, and that we did not take this into consideration.) |
I don't think that CORS would avoid the problem being noted in the red note. The problem is that the IDP could in theory just use the clientID to determine what the client is, and this clientID is provided via JavaScript, so the website could lie about it. So it is inevitable that they have to verify that the clientID being used matches what the user agent says is the RP. |
Yes. In my opinion, responding with the appropriate CORS headers means exactly that the server explicitly acknowledges that it performed the appropriate checks (which of course are specific to the business logic of the API, and in the case of FedCM boil down to checking that client_id and origin match). |
So the threat scenario here is where evil.com invokes FedCM with some trusted RP's public The absolute minimal CORS check that an IDP might perform would return ACAO headers to any trusted RP, independent of Separately, it seems odd that if |
I suppose it is true that the browser could add a step to enforce that the RP is not lying about the client ID. For instance, the user agent could send client_id and the Origin to the IDP and the IDP needs to reply with some confirmation that those are indeed a match. It should be coupled with some fetch, for instance the ID assertion fetch, to not introduce latency. However, I do not think CORS is the right header for this because it increases the chance that the IDP accidentally makes the id assertion endpoint readable via other JS APIs from all RPs, which would be much worse than the attack being described here. |
@antosart ping -- did you have thoughts on npm1's comment above? I guess we could add an |
Using CORS to check if the pair of client_id and origin in a client side fetch call is valid is common practice for credentialed js calls with IDPs. The IDP would return all elligible origins for a specific client ID using CORS (the above mentioned easy method would not be possible honestly at least here in the EU)
Assuming we are shifting and removing referer and use origin instead this is a must. In the end the IDP must have the client_id and the origin available (which is the key part here) and will check if the pair is as expected. |
That might be a fair point as the FedCM API Call is not a classical client side fetch from the RP itself |
Sorry I misread that - you mean the IDP should return the origin in its response? How does that ad value? |
It's not totally clear to me where the risk would be in that case. In order to receive an ID token, the RP would still have to send an authenticated request with the proper cookie. Basically this would just mean that the RP could perform the auth flow without using the FedCM API (again, assuming it can send the first-party IDP cookie). If the IDP want to open this API only to FedCM, it can still check the X- header. |
Also I want to make sure I fully understand the risk of using CORS here. IIUC, there is no risk of exposing the tokens to JS on any old site, right? (Because |
The concern is that they will echo back the origin in ACAO, and forget to check the sec-fetch-dest. this would mean that websites can bypass FedCM and the corresponding user confirmation and send a request to the token endpoint directly. |
If they naively echo back ACAO with any old origin, then the IDP isn't checking that the origin is a trusted RP, which the spec currently states is a requirement for security (see OP), so that threat (from enabling CORS) doesn't leave us any worse off than the current state of affairs. |
Fair enough, but my point is still that even if they do check that it's still possible to forget to check sec-fetch-dest, and suddenly the result becomes readable. I'm skeptical that using CORS adds any real security. |
It solves the problem in the OP, right? I'm not saying it has no drawbacks (script readability), but I think it does solve that specific problem. So let me just see if I'm accurately understanding the options here: Not using CORS:
Using CORS
Is this an accurate summary? |
I'm not convinced that either of these is true:
Why are you assuming especially the latter? |
I'm not. If I were, I guess I'd base that hunch off of the fact that it is a very standard and well-known security check to implement so it would be harder to miss than the FedCM-specific check that is required. But the argument for CORS (which I'm not necessarily making, I'm just trying to map the tradeoffs here) doesn't rely on them being more likely to do the CORS check. The argument is that if they don't do either of the checks, the former fails open while the latter fails closed. |
Well not quite... the former ensures that they echo the incoming origin onto ACAO and fails closed otherwise. It does nothing to ensure they do any checks in addition to that echoing. |
If they send back an ACAO header in any way, it implies they're doing the checks. If they do them wrong, that's on them, but at least it requires them doing some logic to implement the standard security check, and fails if that logic is entirely absent. Whereas if the "check" logic is entirely absent today it just fails open. Idk that seems like a win to me, but we might just disagree about that. I can't help but think that CORS is perfect to mitigate the scenario that @antosart raised in the OP. |
(Back from vacation) I think your summary is accurate, @domfarolino. Although a more comprehensive summary could have the third option, which is adding an ad-hoc check to ensure that the |
@npm1 I am not sure I understand what you mean by 'feel free to share this info outside of FedCM' If the IdP wants to provide an API only for FedCM, then I believe they should block all requests with non-matching |
What I mean is that having the CORS requirement could give the impression to developers that they can also obtain the credentials via other fetches which require CORS, like a JS |
I think that is a good point, I don't think there are any other such mechanisms. On the other hand, I am unsure about the sentence "fetches should not be allowed under regular mechanisms protected by CORS". Is this really a requirement? An IdP which would like to support vendors which do not implement FedCM might want those APIs to be reached via fetch, no? |
I'm not aware of a vendor which uses cross-origin fetches for that purpose (vs an <iframe> and same-origin fetch + postMessage). |
At the risk of not adding much information to this thread, I just wanted to +1 what @npm1 said that I think CORS has a semantics mismatch problem here: it is intended for "Cross Origin Resource Sharing" (right?), which isn't the intention here: this (the identity assertion endpoint) is an endpoint that is designed to be called by the browser for the browser. We specifically do not want to expose this endpoint across origins, e.g. unintentionally through |
I don't think there is a qualitative difference from other endpoints, and I don't think that we can give a precise meaning to "is designed to be called by the browser for the browser". As an example - but I am sure there are others - cross origin web fonts declared in a So, I don't really think that CORS is 1-1 related to fetch. I think CORS means: this resource can be shared with / used for a page with this origin. And I believe this is exactly what happens for FedCM. |
That's a fair example. I guess CORS is already somewhat overloaded in the sense that it is used to allow access via various web platform APIs. From that perspective, using it here may not be totally wrong. I still think having a specific check of client id vs origin in the FedCM fetches is a bit better, but I can see how security folks might not want to introduce another mechanism to perform the check when CORS can be argued to be workable here. Is that the reason you prefer CORS to a FedCM specific check? |
Not exactly sure who this is directed to, but that's kind of where I'm coming from, yes. It's the common security protocol that we all think about when adding new features etc, and I think having less unique ones makes it all the less likely that we'll miss something when making future changes. Curious to hear what @antosart thinks though. |
@domfarolino yes that's exactly my point. I believe that relying on well-established, well-understood security primitives makes it easier to reason about security properties, both for spec/features developers and for web developers. |
To answer the CSP question, we do a 'manual' check in step 3 here https://fedidcg.github.io/FedCM/#fetch-the-config-file (this for for the well-known file, the first fetch performed in FedCM), since there is no |
In OAuth2 Client Credential Grant the token returned by the IdP to the SP is used to call a back channel endpoint. That endpoint has client identity authorization. There can be many different clients at an origin, so the client's own credentials can be used. I don't know if PKCE has the client authentication included on the backchannel flow. |
One issue with enforcing CORS on the identity endpoint is that the fetch spec (and thus, Chrome's networking stack) does not really allow for what we want. With CORS, what we want is:
And as far as I can tell, this is not possible today with fetch. |
@cbiesinger I think you are right, but fetch also does not support doing a non-CORS request sending all cookies with a third-party |
@antosart my thinking was to have the caller (the fedcm spec) append an ('Origin', I am realizing this works in Chrome because since we do not set a request initiator, the networking stack does not set an origin header itself, so the origin header we set in the fedcm code remains. In short, you are correct :( |
That said @antosart -- for the no-cors case we have a plan in form of whatwg/fetch#1533; we have no plan for how to spec the CORS suggestion |
Let me step into this now that I'm back from a leave. This seems like a case where CORS should be a requirement to me. The ID token is being handed directly to the FedCM caller. IIRC, we talked about this particular request in the meeting where we developed "unsafe-no-cors" as an idea. I thought we settled on exactly the points @domfarolino hits here:
To answer that last question: not that I can see. I also assumed that these cookies would have to be set to SameSite=None in order to function
That was going to be worked on (wrapped up?) by the Chrome FedCM team during my leave- is there anything that has happened out of band there?
It's been a while but I think I assumed that IDPs would have to set their auth cookies as third-party to work in federated contexts in the first place. I take that this is not the case. Is whatwg/fetch#1637 your proposed solution to the conflict you pointed out here: #428 (comment) ? I need to refresh myself with how the Firefox implementation functions right now- The lack of resolving the CORS issues in the spec means we were flying a bit in the dark and it seems to be biting the spec back here. |
The current consensus is to use "unsafe-no-cors" (or whatever name) for the endpoints, and to also use CORS for the id assertion endpoint. The ID assertion endpoint still needs the forbidden header protection.
Hmm looks like you are right. I think our team got confused and at some point we thought that these cookies would be SameSite=Lax, but those are not sent in crosss-site iframe requests. So I don't see how the cookies could be Strict or Lax.
Apologies, that's on me. We got distracted by other work plus the request to use CORS on the ID assertion endpoint, and we did get a resolution on that one. Now that you are back, it may make more sense for you to keep pushing the PR, but I can also help if you want, let me know and we can sync to distribute the work. |
I am sorry that I have not made progress on that :(
The last paragraph of https://fedidcg.github.io/FedCM/#browser-api talks about this pretty explicitly. (noticed this while reviewing https://github.com/fedidcg/FedCM/pull/473/files#diff-40cc3a1ba233cc3ca7b6d5873260da9676f6ae20bb897b62f7871c80d0bda4e9R305) |
I think this may actually not be an issue, because https://fetch.spec.whatwg.org/#append-a-request-origin-header would just append the request's serialized origin, which would be correct in this situation (we set the origin correctly in https://fedidcg.github.io/FedCM/#fetch-identity-assertion) One thing I did notice talking with Ben and Nicolas now is that we need to access the internal response because the regular one is filtered due to https://fetch.spec.whatwg.org/#ref-for-concept-request-response-tainting%E2%91%A5 setting tainting mode to opaque for no-cors fetches. |
I don't actually think this will do the trick. At TPAC we established that ORB (Opaque-Response Blocking, in whatwg/fetch#1442, originally from https://github.com/annevk/orb) will block all cross-origin no-cors JSON responses. So per spec, even reaching into the internal response won't do anything; ORB blocks things before that stage. (It seems the Chromium implementation is not broken in this way right now because network stack perceives the request as same-origin, which is incorrect.) |
FWIW, I pointed out the ORB issue a year ago: #320 (comment). |
Ah thanks! In the TPAC meeting there were a lot of blank stares and "wait, what's ORB?!"s when it got brought up, so I just naively assumed we hadn't discussed it here before. |
FedCM and CORS integration has been discussed in w3c-fedid/FedCM#428, among other places, and various flavors of the discussion revealed confusion around both the kNoCors request mode in the implementation and spec, as well as the `network::ResourceRequest::request_initiator`, and how it relates to the Fetch Standard. Some folks didn't realize that `request_initiator`, when used by the web platform specifically, was equivalent to a Fetch request's origin [1], so this CL helps clear this up. [1]: https://fetch.spec.whatwg.org/#concept-request-origin Bug: 1383340 Change-Id: If090182beb8d82e87784d6b6094f278c7f9a820d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4873650 Reviewed-by: Kenichi Ishibashi <[email protected]> Commit-Queue: Dominic Farolino <[email protected]> Cr-Commit-Position: refs/heads/main@{#1199321}
Sorry for the delays, this is still something we intend to do in FedCM. After some TPAC meetings, I think we are unblocked to make the change. The ID assertion endpoint will use CORS and some fetch spec monkeypatch to pass the relevant cookies. We'll send a PSA to developers of this change and update the spec. |
A drive-by comment as I'm skimming through this. Please feel free to let me know if this was already discussed and I missed it.. (or if it's a bad idea :D) It seems like we want want IDPs to perform 2 checks:
While the latter check is only required in browsers with 3P cookies, and can be solved by the IDP validating |
Yeah, I do like that idea. It's pretty simple and consistent with other security checks. I think there are two things working against it though:
@annevk do you have thoughts on this? |
My worry with So I would be inclined to not offer this and rely on IDPs to handle the (If we had origin manifests you could imagine requiring a particular destination upfront for certain paths, but origin manifests are very tricky.) |
Some more thoughts from today's discussion:
|
That's a good argument in favor of steering developers towards active server-side filtering. Having a client-side destination-based filter may be defense in depth in that case, but I admit that given the above, I'm not sure the case for that is very strong. |
The spec says in a red note that
This seems a crucial check that the IdP must implement, as it would otherwise allow anyone the get the user's id token. Wouldn't it be safer to enforce CORS here (i.e.: the user agent throws an exception if the response does not include the correct
Access-Control-Allow-Origin
header)? CORS exist exactly for this usecase. And this would make it impossible for IdPs to forget to implement this check.The text was updated successfully, but these errors were encountered: