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

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

Open
dickhardt opened this issue Jun 3, 2022 · 15 comments
Open

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

dickhardt opened this issue Jun 3, 2022 · 15 comments
Labels
compatibility risk Issues that may lead to backwards compatibility problems design needs resolution

Comments

@dickhardt
Copy link

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
Copy link
Collaborator

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
Copy link
Author

@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
Copy link
Collaborator

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?

@samuelgoto samuelgoto reopened this Jun 6, 2022
@dickhardt
Copy link
Author

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
Copy link
Collaborator

kenrb commented Jun 6, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

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

@samuelgoto
Copy link
Collaborator

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!

@samuelgoto samuelgoto reopened this Jun 9, 2022
@cbiesinger
Copy link
Collaborator

I haven't seen the PR but IMO:

  • IDPs should not need to check the header
  • For consistency and code simplicity I think it's perfectly fine to send the header on all requests, including the manifest request
@kdenhartog
Copy link
Contributor

kdenhartog commented Jul 14, 2022

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
Copy link
Collaborator

npm1 commented Jul 15, 2022

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
Copy link
Contributor

kdenhartog commented Jul 16, 2022

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
Copy link
Collaborator

npm1 commented Jul 18, 2022

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

@npm1 npm1 added the design label Jul 20, 2022
@samuelgoto samuelgoto added the compatibility risk Issues that may lead to backwards compatibility problems label Oct 3, 2022
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 design needs resolution
6 participants