Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Persist the 'state' parameter until a successful callback. #75

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jcoglan
Copy link

@jcoglan jcoglan commented Jul 2, 2015

The explanation of this change is in the commit message. The one question I have is whether my guess at the reason for always generating a new state value is correct; if you're avoiding using ||= to protect against another threat I haven't thought of, I'd like to know.

We discovered the following issue while using [omniauth-facebook][1]. We
are seeing an unexpectedly high number of CSRF violations (roughly 6%
of sign-ins) when users return from facebook.com.

Every time the user visits `/auth/facebook` and invokes the
`request_phase` method, a new `state` value is generated and put in the
session. If a user opens the site in two tabs and navigates to
`/auth/facebook` in each, then they have two copies of
https://www.facebook.com/dialog/oauth open, each with a different
`state` param. But, only one of these state params is in the session
under the `omniauth.state` key.

One of these pages will thus have a `state` param that does not match
the session. It is also possible to trigger this with concurrent or
duplicate requests, or by using the back/forward buttons, causing a race
condition in updating the session.

The page with the `state` value that "lost" the race will authorize
Facebook's permissions, but then fail when redirected to
`/auth/facebook/callback` due to the token mismatch.

I wondered why this gem always assigns a new `state` value on visiting
`/auth/:provider`, compared to how [rack-protection][2] and [Rails][3]
implement CSRF protection using a guarded assignment. My assumption in
this patch is that this is to avoid sending a value to one provider that
could be redeemed by another. If I send a state value to alice.com, then
someone working for alice.com could take that value and the account of
the user that authenticated there, then email the user a phishing
callback link for bob.com, including a `state` value that will work.

To avoid this, I have introduced a namespace into the session: each
`state` value is keyed by the class name of the particular provider, so
you can have a different state value per provider, but have them be
reusable. The value is still removed from the session on completion to
avoid replay attacks.

Keeping the `state` constant until the auth process completes means it's
safe to open multiple tabs and perform concurrent requests, or anything
else that might leave the session in an inconsistent or unpredictable
state.

[1]: https://github.com/mkdynamic/omniauth-facebook
[2]: https://github.com/sinatra/rack-protection/blob/v1.5.3/lib/rack/protection/authenticity_token.rb#L24
[3]: https://github.com/rails/rails/blob/v4.2.3/actionpack/lib/action_controller/metal/request_forgery_protection.rb#L316
@timherby
Copy link

I am also seeing this issue (going back to linkedin auth page after canceling, and submitting causes a CSRF error instead of processing properly because the session state is already deleted).

I'd love to see a fix like this merged in. @jcoglan did you end up deploying this solution to production? Did you run into any issues? Looks like the tests are failing just due to some syntax errors. Maybe we can get this merged in if they pass across all ruby versions. I'd be happy to help out getting this through.

end
session["omniauth.state"] = params[:state]
params
site = self.class.name
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would I add a dynamic attribute in the state params? Here is a use case: I have a multi-tenant app and in the google strategy, I have authenticate! method and first thing it does is switch the tenant. Since the tenant name should be dynamic, I want to get the tenant name dynamically from the state_params or from the request.env['omniauth.auth']. I am not sure if I am putting it correctly but I want the tenant name dynamically set somewhere in request params. If the request is coming from the host1.com, tenant_name=host1, host2.com, tenant_name=host2, something of this sort. Since the request is coming from the auth server, does it mean I need to intercept the request and add the tenant in the request params depending upon the origin?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants