`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.

[GPrimola]

Sorry @maennchen but I'm not following you. I couldn't see why

We need a way to set a custom state and read it again in a way that is safe.

The solution, it seems to me, fills the CSRF protection gap in oidcc plug and is retro compatible.

The custom state (from UserAgent) is being set both on session cookies and in the IdP, when the client creates a PAR. The callback would just "sanitize" off the authenticity part of the state for transparency of the process.

I've adapted the former diagram to this new workflow for clarity.
I've also reviewed the RFC parameters also this section 10.12

[GPrimola]

@maennchen I don't get it. Since the state can be set by the user agent, why do you think it's a good idea to deliver it "structured", and not as a parameter (as it is in the beginning) down the plug chain?

Anyway, your idea is to change the return value of AuthorizationCallback.call/2 from {:ok, {token, userinfo}} to {:ok, %{token: token, userinfo: userinfo, state: state}}?

[maennchen]

Excuse the long wait @GPrimola. I was traveling a lot recently and unfortunately did not have as much time as I hoped for maintenance.

The problem is the following:

• Dev does authorize redirect with state=some_redirect_url
• Callback reads state and redirects there

After this change, we would add a random value to the state to make sure it's not predictable. That would lead to a nonsensical redirect.

So adding the random value to the state is fine, but we need to expose the user provided state somewhere besides the query param since that has been changed.

{:ok, %{token: token, userinfo: userinfo, state: state}}

Yes

[GPrimola]

No problem @maennchen I was off for a while as well! :)

The thing that I'm not on the same page as you is:

we need to expose the user provided state somewhere besides the query param

I don't think that way.
What I think is, since the state is something set by the user (i.e. the Oidcc.Plug.Authorize module accepts it), the whole process of "signing" it to be sure it's authentic should be transparent, therefore that initial state sent by the frontend application should be delivered to the "protected" action transparently.

Take the callback/2 action of the README for instance.

  def callback(%Plug.Conn{private: %{
    Oidcc.Plug.AuthorizationCallback => {:ok, {_token, userinfo}}}
  } = conn, params) do
    url =
      # I went real wild here to illustrate that `state` is user managable
      case Jason.decode(params["state"]) do
        {:ok, %{"last_page" => url}}
          url
        {:error, _} ->
          [[url]] = Regex.scan(~r/last_page_redirect/, params["state"])
          url
      end
    conn
    |> put_session("oidcc_claims", userinfo)
    |> redirect(external: url)
  end

I really don't see the state going in the result of the authorization, it just don't belong there, specially because it is not required (if the user don't send anything in the state, the state will be used just for authenticity purpose) and after removing the authenticity salt it will be just empty again.

  def callback(%Plug.Conn{private: %{
    Oidcc.Plug.AuthorizationCallback => {:ok, {_token, userinfo, state}}}
  } = conn, params) do
    
    conn
    |> put_session("oidcc_claims", userinfo)
    |> redirect(to: "/")
  end

[maennchen]

I have the feeling we're talking past each other.

Use Case:

• User is not logged in and tries to access /profile.
• User is redirected to /authorize?state=/profile
• Authorize Plug appends secret to state and redirects to login provider. So state now is something like [random]/profile.
• User is redirected back to /callback?state=[random]/profile by login provider.
• We check the state with the state_verifier.
• User action is called.
  • Old: action params["state"] contains [random]/profile. => Can't redirect here because of the random value
  • New: state is passed as part of private[Oidcc.Plug.AuthorizationCallback] with [random] stripped out. This we can use for redirect.
  • If state was not provided, state will only be [random]. In that case we can just set it to nil.

[GPrimola]

@maennchen sorry my pace, I've been very busy.

Anyway, I got you, and we were kind of talking the same thing, except I didn't want to break the current contract, not yet. I'd prefer having the CSRF verification done in the state first before introducing breaking changes. Nevertheless I'm adding the user-agent provided state as another conn.private key.

I've done the changes in the #73.