Use "state" param to mitigate CSRF

[AlexanderPavlenko]

omniauth/omniauth#612

[kitop]

+1

[mbleigh]

By including SecureRandom you're making the library incompatible with Ruby 1.8. Is there a simple way to refactor to be 1.8 compat? I'm fine EOL'ing 1.8 support but I'd rather not do so for a security fix.

[kitop]

IIRC SecureRandom is included on Ruby 1.8.7. Ruby 1.8.6 would be deprecated.
For 1.8.6, it could be added by requiring activesupport/secure_random, but it makes it a little overkill I guess.

[AlexanderPavlenko]

@mbleigh looks like 1.8.7 supports SecureRandom as well
http://www.rubyinside.com/ruby-187-released-912.html

securerandom is a Ruby 1.9 library that's now part of the Ruby 1.8 standard library too.

Also, I've tried it with ree-1.8.7-2011.03 (the only one old thing I still have installed) and it works

[mbleigh]

This is merged and has been released as omniauth-oauth2 1.1.0

[mkdynamic]

So, if folks decide to pass a custom state parameter they will forgo the CSRF protection? Unless they actually use it for that purpose (i.e. pass something that is unique and non guessable each time).

If that is true, maybe it would be better to add the CSRF token when the user passes a custom state value, then remove it on the callback. So it would be transparent to the user, but makes CSRF fully automated for providers that support the state param.

Thoughts?

[AlexanderPavlenko]

it would be transparent to the user

It may be not transparent for the user and instead of helping can just break something in his stack. Also, it looks like a babysitting. The one thing is when the state param is absence and we add it, and the other when it's present and we're trying to “improve” it. I'm not going to implement this.

[moll]

I'm a bit late to the party, but is the suggestion to use cookie/session vars for handling custom data and params during the oauth the only current way? The Facebook API docs don't seem to support other custom params besides 'state' (I'm guessing it's in the OAuth spec)

So, what ways do we have to send data back and forth per one authing? Architecturally using a session store for just per 1 auth trip data does not seem a good place.

[AlexanderPavlenko]

Architecturally using a session store for just per 1 auth trip data does not seem a good place.

If it's not a good place, then state is the worst place. Also, if we are speaking about Rails, it uses session store all the time. It doesn't matter if user is signed in or not — anyone who hits the app, gets the cookie with session id.
Well, if you really wish to expose your app's params to OAuth provider, you still can put any string in state.

[moll]

I think the session store is great if you've want to keep stuff that relates to the user and his session, but for info that specifically affects 1 roundtrip to the provider and back, it makes IMO a poor place. I'm not talking about sensitive info.

For the session store to work with specific roundtrips you'd have to make sure it gets cleaned up after the client returns (if he returns) from the provider and to make sure that one trip's data doesn't get mixed with other concurrent trips. E.g. if people press two "connect" buttons simultaneously. Or am I misimagining how to use the session store for this?

One simple concrete example would be tracking which authing happens in a popup window and which not, and based on that render different views.

But, you mention overwriting state should still work? I was using state with the omniauth-google-oauth2 gem and received invalid_credentials when overwriting it.

[AlexanderPavlenko]

@moll can you please post how do you set your state param? According to https://github.com/intridea/omniauth-oauth2/blob/master/lib/omniauth/strategies/oauth2.rb#L51 the only trouble that I can anticipate for now is, khm, string key in hash. Also, you can try to debug your code step-by-step.

[AlexanderPavlenko]

Or maybe options.authorize_params[:state] is a wrong place for this thing and it should be taken from options[:state]?

[krainboltgreene]

This has apparently broken my ability to use this gem. What's going on?