search

w3c-fedid / FedCM

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

FedCM spec: IdP should validate nonce to prevent CSRF? #582

Closed obfuscoder closed 4 months ago

obfuscoder commented 5 months ago

In the current draft of the FedCM spec (https://fedidcg.github.io/FedCM/), section 3.5. Identity assertion endpoint contains the following:

NOTE: An [IDP](https://fedidcg.github.io/FedCM/#idp) should validate the nonce, if present, to prevent CSRF-style attacks.

There is some confusion amongst implementers, what should be validated here?

The nonce is optional and it is the RP's responsibility to define unique nonce values when initiating the FedCM call and compare the returned nonce with the provided one.

cbiesinger commented 5 months ago

@bvandersloot-mozilla

samuelgoto commented 5 months ago

I'm thinking that this should be done as part of w3c-fedid/FedCM#536 in userland (e.g. an OAuth Profile layer), rather than in browserland in the FedCM spec per se.

@timcappalli, wdyt?

achimschloss commented 5 months ago

Agreed, if nonce was not already a top level parameter and removing it would be a breaking change it should be userland via https://github.com/fedidcg/FedCM/issues/556 - I guess it would be fair to just remove the note

aaronpk commented 5 months ago

the comment in the sample code in the spec says this:

// Assume there is a method returning a random number. Store the value in a variable which can // later be used to check against the value in the token returned.

This assumes the client understands how to parse the token returned, which I thought was not an assumption the FedCM spec wanted to make. I would recommend removing the nonce parameter entirely in favor of moving all of that kind of thing into a different layer as described in w3c-fedid/FedCM#536.

If there is anything FedCM needs to do at the FedCM API layer to mitigate this kind of injection/replay attacks, it should provide the other half of the validation story as well.

bc-pi commented 5 months ago

@aaronpk's perspective on layering https://github.com/fedidcg/FedCM/issues/582#issuecomment-2108243833 makes a lot of sense.

samuelgoto commented 5 months ago

I would recommend removing the nonce parameter entirely in favor of moving all of that kind of thing into a different layer as described in w3c-fedid/FedCM#536.

Yeah, nonce I think would be an easy (and, I think, only?) target once we introduce w3c-fedid/continuation#2.

cbiesinger commented 5 months ago

In principle I agree with moving nonce to params. However, we have shipped it since the initial version of FedCM and so I am not sure we can remove it now for backwards compatibility reasons.

obfuscoder commented 5 months ago

As changing API may be a bit tricky and needs more thinking through, could you at least remove the NOTE I mentioned as @achimschloss suggested?

samuelgoto commented 5 months ago

As changing API may be a bit tricky and needs more thinking through, could you at least remove the NOTE I mentioned as @achimschloss suggested?

Yeah, that sounds reasonable. Care to send a PR?

We'll get to this at some point, but if you could put together a PR, you may beat us to it.

npm1 commented 5 months ago

Should we close this issue now?

samuelgoto commented 4 months ago

Should we close this issue now?

Yeah, I think we can close this issue now that this PR has been merged:

https://github.com/fedidcg/FedCM/pull/583

I would recommend removing the nonce parameter entirely in favor of moving all of that kind of thing into a different layer as described in w3c-fedid/FedCM#536.

The only suggestion left here unaddressed is this, and I kicked off a separate issue to track it independently of this one:

https://github.com/fedidcg/FedCM/issues/616