Use state to mitigate CSRF #612

Closed
homakov opened this Issue Jun 7, 2012 · 13 comments

Comments

Projects
None yet
4 participants
@homakov

homakov commented Jun 7, 2012

I have a question, why omniauth doesn't provide 'state' param to prevent CSRF? For example facebook supports it and returns it back to make sure you are who started auth process. There should be 2 params: code and state. Now it's only code. And it is vulnerable. Should I add one?

@homakov homakov closed this Jun 7, 2012

@homakov homakov reopened this Jul 3, 2012

@AlexanderPavlenko

This comment has been minimized.

Show comment
Hide comment
@AlexanderPavlenko

AlexanderPavlenko Jul 3, 2012

Someone, please get assigned, or we'll get hundred pull-requests at once :)

Someone, please get assigned, or we'll get hundred pull-requests at once :)

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Jul 4, 2012

@AlexanderPavlenko we need fix asap :)
omniauth should automatically generate random state, save it in user session and check equality on callback_phase
10 LOC + tests..

homakov commented Jul 4, 2012

@AlexanderPavlenko we need fix asap :)
omniauth should automatically generate random state, save it in user session and check equality on callback_phase
10 LOC + tests..

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Jul 4, 2012

it looks good solution. 3.times.map{ rand.to_s[2..-1] }.reduce(&:concat) strange random..

homakov commented Jul 4, 2012

it looks good solution. 3.times.map{ rand.to_s[2..-1] }.reduce(&:concat) strange random..

@AlexanderPavlenko

This comment has been minimized.

Show comment
Hide comment
@AlexanderPavlenko

AlexanderPavlenko Jul 4, 2012

@homakov

strange random

nice one, for me :)

About solution — the only thing I fear is some dumb providers that just wouldn't pass back that state param. But if it's rare and code really works (I didn't have a chance to test it with the real OAuth providers, that's why I asked for your help), we can create pull request for it.

@homakov

strange random

nice one, for me :)

About solution — the only thing I fear is some dumb providers that just wouldn't pass back that state param. But if it's rare and code really works (I didn't have a chance to test it with the real OAuth providers, that's why I asked for your help), we can create pull request for it.

@benatkin

This comment has been minimized.

Show comment
Hide comment
@benatkin

benatkin Jul 6, 2012

👍

To read more, go here and search for "state" (keep the double quotes).

http://tools.ietf.org/html/draft-ietf-oauth-v2-27

benatkin commented Jul 6, 2012

👍

To read more, go here and search for "state" (keep the double quotes).

http://tools.ietf.org/html/draft-ietf-oauth-v2-27

@kitop

This comment has been minimized.

Show comment
Hide comment
@kitop

kitop Jul 6, 2012

@AlexanderPavlenko Seems good. Tried it and works :)
Why not use ruby's built in SecureRandom instead of generating the random by hand? Something like SecureRandom.hex(24)? (just be sure to require 'securerandom')

kitop commented Jul 6, 2012

@AlexanderPavlenko Seems good. Tried it and works :)
Why not use ruby's built in SecureRandom instead of generating the random by hand? Something like SecureRandom.hex(24)? (just be sure to require 'securerandom')

@AlexanderPavlenko

This comment has been minimized.

Show comment
Hide comment
@kitop

This comment has been minimized.

Show comment
Hide comment
@kitop

kitop Jul 6, 2012

Excellent!
Hope this gets merged! Are you going to send a pull request?

kitop commented Jul 6, 2012

Excellent!
Hope this gets merged! Are you going to send a pull request?

@AlexanderPavlenko

This comment has been minimized.

Show comment
Hide comment
@AlexanderPavlenko

This comment has been minimized.

Show comment
Hide comment
@AlexanderPavlenko

AlexanderPavlenko Jul 6, 2012

@homakov aaand it's merged, so this issue can be closed

@homakov aaand it's merged, so this issue can be closed

@homakov

This comment has been minimized.

Show comment
Hide comment
@homakov

homakov Jul 6, 2012

good job guys

homakov commented Jul 6, 2012

good job guys

@homakov homakov closed this Jul 6, 2012

@kitop kitop referenced this issue in mkdynamic/omniauth-facebook Jul 6, 2012

Closed

Update omniauth-oauth2 dependency to 1.1.0 (security) #67

@AlexanderPavlenko AlexanderPavlenko referenced this issue in omniauth/omniauth-oauth2 Feb 22, 2013

Merged

Use "state" param to mitigate CSRF #18

@skalb skalb referenced this issue in attachmentsme/ruby-box Mar 6, 2014

Merged

Add authorize_url_with_state to allow for CSRF protection. #81

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