Commit: CSRF vulnerability, injecting state in session affect other gem that need to use the "state" param. #31

Closed
thomasjoyce opened this Issue Feb 14, 2013 · 5 comments

5 participants

@thomasjoyce

This change will affect folks that want to use "state" param but could not.
80d75bf

Particular for omniauth-stripe-connect gem.

The commit make the assumption that state param will always be a SecureRandom.hex(24)

but not true.

Reverting back to this code below may be advice.
Let me know if i'm wrong :- )

if options.authorize_params[:state].to_s.empty?
options.authorize_params[:state] = SecureRandom.hex(24)

end

@jackbit

This is serious issue that need fixed. Almost omniauth oauth2 authentication are broken with CSRF issues. The issue is similar like this: mkdynamic/omniauth-facebook#73
Badly it is not only happened on facebook, but happened to all omniauth-xxxx gems which use version 1.1.0 of omniauth-oauth2.

Those issue could be handle by adding provider_ignores_state: true on omniauth configuration but i am afraid that the attacker can use weakness like this article:
http://homakov.blogspot.com/2013/02/cross-origin-madness-or-your-frames-are.html

@cmer

+1

@tmilewski tmilewski closed this Sep 4, 2013
@tmilewski
INTRIDEA Inc. member

If you need to persist data between the request and callback phases please use a session or the like. By allowing state to be set, we open ourselves up to cases where omniauth-oauth2 may be secure but, unbeknownst to a user, a provider gem may not.

There are many ways to handle the requested use-cases, in both this ticket and others, where we don't need to sacrifice security.

@guilhermesimoes

I'll just add that OmniAuth is already kind enough to persist in the session any extra params you may send in your request.

So instead of passing data with

request_url?state=data

and getting it with

data = params[:state]

You can pass it with

request_url?data=data

and get it with

data = request.env['omniauth.params']['data']

And I think you can even omit the request and get the data with:

data = env['omniauth.params']['data']

Maybe it would be worth adding this to the README?

@tmilewski
INTRIDEA Inc. member

@GuilhermeSimoes This would definitely be worth adding to the README.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment