`state` as parameter not protecting against CSRF attacks

[GPrimola]

Hi,

I was validating the authorization flow of an application which we use oidcc_plug and it came to me that this state on Oidcc.Plug.Authorize module:

1. shouldn't be a parameter
2. it isn't preventing CSRF attacks as for RFC 6749.

Explanation:

This diagram illustrates the problem on the flow with state as a parameter to generate the redirect url.

Desired:

The Oidcc.Plug.Authorize should generate a random value representing the state for each specific authorization request.

[maennchen]

I'm a bit backed up with work right now. I'll come back to this in the next few days.

[maennchen]

@GPrimola It is a common pattern to include stuff in the state like redirect URLs etc. This is why the implementation is storing a state_verifier in the session and double checks the hash in the callback. But looking back at it, I see how this is insufficient in two ways:

• The attacker can guess the state if it uses a predictable value
• You can reuse the same value across multiple sessions

So we should make this safer. But we shouldn't take the users state option away.

Let's prepend a random binary (fixed length) to the beginning of the state. Then we use the full hash in the state_verifier. In the callback we check the hash and strip the random binary from the beginning of the state.

In the future please refrain from opening issues and PRs like this. CSRF is a security related topic and should be disclosed according to our security policy. https://github.com/erlef/oidcc_plug?tab=security-ov-file

Luckily this isn't too bad, but I'll still create a CVE for it.

[GPrimola]

Thanks for the feedback! My bad not paying attention to the security issues reporting.

Anyway, I've just updated #73 with your suggestion!

Thank you! :)

[maennchen]

In the callback we check the hash and strip the random binary from the beginning of the state.

I see that we're not offering state back to the callback user. They so far most likely just read the query param directly.

To provide a way to get the state cleanly, we should add it somewhere into the con (minus the random value at the start). How about adding it to the result?
Unfortunately, the data is currently in a tuple, a map would probably be better.

[GPrimola]

I took a quick look at AuthorizationCallback.call/2 and the state is being passed on as a parameter in conn.
Since the state's value might come from outside, that is from the client application, I'd say better leave it as it is, without the authenticity part though.
For compatibility reason I wouldn't change the tuple to a map for that, I think better keep it as transparent as possible.

WDYT?

[maennchen]

That does not work. We need a way to set a custom state and read it again in a way that is safe. Leaving it up to the callback breaks that expectation.

Adding an element to a tuple is just as much a breaking change as changing it to a map.