achedeuzot / ueberauth_auth0

Auth0 OAuth2 strategy for Überauth.
https://hexdocs.pm/ueberauth_auth0
MIT License
71 stars 46 forks source link

CSRF error #168

Closed jorbs closed 3 years ago

jorbs commented 3 years ago

I am getting the error below when trying to login into my application using an incognito browser:

%Ueberauth.Failure{
  errors: [
    %Ueberauth.Failure.Error{
      message: "Cross-Site Request Forgery attack",
      message_key: :csrf_attack
    }
  ],
  provider: :auth0,
  strategy: Ueberauth.Strategy.Auth0
}

The used versions are:

"ueberauth": {:hex, :ueberauth, "0.7.0"...
"ueberauth_auth0": {:hex, :ueberauth_auth0, "0.8.1"...

How ueberauth_auth0 deals with CSRF?

Also, I noticed that ueberauth implements the config ignores_csrf_attack. If I set it true in my ueberauth_auth0 fork the login works fine.

EddieWhi commented 3 years ago

Looks like csrf protection was added in ueberauth 0.7.0 PR with recognition that strategies will need to change and guidence for updating (or ignores_csrf_attack will need to be set to true to keep the old behavior). Rolling back to 0.6.3 seems to work.

jmhossler commented 3 years ago

I raised the PR above ^ to deal with this. The solution there is basically to remove the option to specify state and to just, by default, use the Ueberauth generated state param. I hope this helps fix the issue and allows us to continue to upgrade ueberauth!

jmhossler commented 3 years ago

In case you are interested more about the background behind this issue:

The error mentioned above was caused the latest update to ueberauth (mentioned by @EddieWhi) where they built in support for CSRF protection. They do this by automatically generating a state token on the connection, and comparing the state given on the callback to the state param that was generated on the initial request. This was added in this CSRF PR.

The current way to provide a state param to this auth0 plugin relies on configuration -- which means the state param would be static. Even if we set ignore_csrf_attack to true, we're still not really protected. I could be wrong here and misunderstanding elixir config -- I'm relatively new to elixir -- but taking the Ueberauth approach of generating state tokens on a per-user basis seems to be a lot more secure than having a static value that can be discovered.

Without the above change, we either have to disable CSRF protection (which, for auth, would be a very bad thing IMO) or use the state param provided by ueberauth to fill in the state param we send to auth0.

Because this deals with security during authentication, I feel like this is an important update to ensure our apps aren't vulnerable to CSRF attacks. It would be nice if we could get some maintainer eyes on it soon.

@achedeuzot is there anything I can do to help get this seen more quickly?

jorbs commented 3 years ago

@jmhossler currently I use the state parameter to store some user state before the login happens (e.g. the page he was at before the login was requested). With this PR, will I have to change my implementation to store that info in a different place (cookies for example) or can I keep using the state param?

jmhossler commented 3 years ago

Ah, I didn't realize that was a use case. I'm guessing you are doing something like https://auth0.com/docs/protocols/state-parameters#use-the-stored-url-to-redirect-users? As a curiosity, how are you passing in these custom state values in to the request?

I mentioned one way to keep allowing the user to pass in the state param would be to make a separate plugin that has the ueberauth csrf validation off that is basically the same but allows the user to specify and manage the state themselves. Do you think that would work?

The problem is the Ueberauth stuff is very opinionated. Either you have their CSRF protection turned on and you have to use their custom state value, or you turn it off -- and that is decided when you call 'use'. I can split it up and restore the original implementation and just disable the ueberauth check if that is the case.

With what I have implemented now, you would have to change how you handle that redirect logic, and it seems like your use-case is perfectly acceptable and secure as well so I don't want to break that.

jorbs commented 3 years ago

@jmhossler

Ah, I didn't realize that was a use case. I'm guessing you are doing something like https://auth0.com/docs/protocols/state-parameters#use-the-stored-url-to-redirect-users? As a curiosity, how are you passing in these custom state values in to the request?

Yes, I am doing like the documentation. I encode my custom state with base64 and then decode it after a successful login.

I mentioned one way to keep allowing the user to pass in the state param would be to make a separate plugin that has the ueberauth csrf validation off that is basically the same but allows the user to specify and manage the state themselves. Do you think that would work?

That works, but the effort to build the plugin doesn't worth. I mean... changing the implementation to use cookies is fine.

achedeuzot commented 3 years ago

Thank you @jmhossler for your pull-request with the fix, I've published the v1.0.0 package to hex 🎉

I preferred to bump the major version so it's well visible that the library has a breaking change with the state parameter now being used by ueberauth.

Thanks also to @jorbs for the issue and @EddieWhi for the discussion.

Cheers 🍻