w3c-fedid / FedCM

A privacy preserving identity exchange Web API
https://w3c-fedid.github.io/FedCM/
Other
371 stars 72 forks source link

Value of Sec-FedCM-CSRF header in discovery call? #267

Open dickhardt opened 2 years ago

dickhardt commented 2 years ago

Is the IdP required to check for the Sec-FedCM-CSRF header in the discovery call per 7.1?

I don't understand the value of the header given that the manifest is public

samuelgoto commented 2 years ago

Is the IdP required to check for the Sec-FedCM-CSRF header in the discovery call per 7.1?

The IdP is required to check for the presence of the header.

I don't understand the value of the header given that the manifest is public

This allows the IdP to protect all of their HTTP endpoints against XSRF (without actually generating XSRF tokens) by knowing that the HTTP requests are guaranteed to be generated by the user agent (only the UA can set Sec-* headers in an HTTP request) as opposed to the content area (e.g. JS).

https://fedidcg.github.io/FedCM/#Sec-FedCM-CSRF

Closing, feel free to re-open if this doesn't make sense to you.

dickhardt commented 2 years ago

@samuelgoto

Why would the IdP need to protect the FedCM metadata? I would expect that to be public data and readily available.

(I don't think I have permissions to re-open)

samuelgoto commented 2 years ago

Why would the IdP need to protect the FedCM metadata? I would expect that to be public data and readily available.

It protects all of the endpoints, including the ones that takes POSTs to generate the id tokens.

Makes sense?

dickhardt commented 2 years ago

No -- that does not make sense. For example the .well-known/openid-configuration endpoint for OIDC is not protected -- so it is strange that the equivalent in Fed-CM would be 'protected'. The response is static.

kenrb commented 2 years ago

It really isn't needed on the request for the manifest. An uncredentialed request to a public resource could come from anywhere, there is nothing an attacker would gain from sending it from another user's browser.

cbiesinger commented 2 years ago

I agree, and having the IDP check this header makes debugging harder.

In general, I'm not sure there's value in being prescriptive about whether the IDP should check this header

samuelgoto commented 2 years ago

No -- that does not make sense. For example the .well-known/openid-configuration endpoint for OIDC is not protected -- so it is strange that the equivalent in Fed-CM would be 'protected'. The response is static.

Ah, sure, is the origin question whether you should use this header specifically in the metadata fetch? If so, because it is static, much like the manifest files too, yeah, sure, it doesn't seem it would need to use that protection.

It was intended to address XSS in the unsafe endpoints, like the id_token_endpoint, for which you should use the headers.

Does that make sense?

samuelgoto commented 2 years ago

Is the IdP required to check for the Sec-FedCM-CSRF header in the discovery call per 7.1?

Was re-reading your comment now, and I see that you are referring to the example in the section 7.1, which I think you are right: it wouldn't be necessary.

I'll send a PR to clarify that in the spec.

dickhardt commented 2 years ago

Good to see we got on the same page ... finally! =)

samuelgoto commented 2 years ago

Good to see we got on the same page ... finally! =)

Neat! Will keep this open until I get my PR merged as a resolution to this issue! Thanks!

cbiesinger commented 2 years ago

I haven't seen the PR but IMO:

kdenhartog commented 2 years ago

I think there's a false assumption being made here with this header. Correct me if I'm wrong here because I've not done enough reading on the sec-* headers. Let's imagine a scenario where an attacker controls network access such as a public wifi network and they're maintaining an active MITM connection via a TLS middlebox. A practical scene where this would occur would be a target is transiting through an airport of a country which is looking to maliciously harvest credentials via their public airport wifi network.

The attacker could inject malicious JS into the RP's page during the initial response when the target visits a site that allows it to make the IDP requests via JS and redirect the requests at the network level to a proxy which is able to inject this header. In this case, the malicious JS could be used to harvest credentials for a variety of different origins visited while on the public airport wifi network in a way that subverts the intended purpose of this header and allows the malicious actor to harvest user credentials at scale for anyone who accesses their public network.

The one defense that is implemented today is that browsers notify users of these issues via the "This site uses HTTP it isn't secure" page that's displayed, but in many cases users will just allow access because they don't understand the network is being actively MITMed since it appears on every page rather than just a few. The secondary defense users have to prevent this issue is that they can use a VPN to secure themselves from the public wifi network, but it shouldn't be assumed that the user will know to use a VPN to protect themselves from this style of attack.

npm1 commented 2 years ago

The header protects against an attacker with JS control, not against an attacker with network control. To protect against the latter, I believe our API will not work with HTTP(no S). I do not see it in the spec so this sounds like something that should be explicitly called out :)

kdenhartog commented 2 years ago

That's fair to put that out of scope since the browser is fairly limited in the capabilities to control network level attacks. One thing that can be done to mitigate this attack is to switch from a bearer token model to a client bound token model, but that comes with tradeoffs in terms of complexity on the client side to manage PKI (might be able to build this on Webauthn?) and the tokens no longer would be able to be opaque for this to work. Would it be reasonable for me to open a separate issue for us to discuss that further first?

npm1 commented 2 years ago

I don't fully follow your suggestion but sure feel free to open a new issue for it!