Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

CSRF on namespace authentications #2640

Closed
markedmondson opened this Issue · 11 comments

4 participants

@markedmondson

The change to clean up the CSRF token (747751a#diff-c5a6f3b45e837867908223a66b012977) on a sign_in causes issues when using Devise across scopes.

devise_for :users,
path: "/users",

devise_for :admin,
path: "/admin",

The issue being that if you are logged in through an authenticated /admin devise session, then additionally login through a /users the existing CSRF token is cleared (https://github.com/plataformatec/devise/blob/747751a20f50aa8814dcd3eb9a3648f00ab6a707/lib/devise/hooks/csrf_cleaner.rb) and submitting a form in the /admin scope causes the entire session to be dropped.

Does anyone have a preferred solution for dealing with this, it seems it may be necessary to disable the Devise.clean_up_csrf_token_on_authentication and implement a namespaced CSRF session store.

@avit avit referenced this issue from a commit in avit/devise
@avit avit Failing test for #2640 21d7a96
@avit

I've submitted an updated test case that demonstrates the error following these steps:

  • User signs in
  • User visits a page that gives her a CSRF token (e.g. a form page)
  • User opens a second browser window, where she signs in as an admin
  • User is rejected from posting the form in the first window, and loses the session

@josevalim, do you have any direction for a solution? Should the form_authenticity_token method store individual "_csrf_token" keys for different scopes? Or, do not reset the token on login if there exists a session for another scope?

@josevalim
Owner

Thanks @avit. I am not sure about how to proceed on this one. Not resetting the token leaves it vulnerable and there is no way we can reliably check which scope the user is in so we have scoped csrf tokens. I mean, we can for pages that requires authentication, but not all of them do.

@avit

@josevalim I was able to check which scope the user has just signed into through warden, and allow setting separate tokens for each scope. Using this, I think we can clear the "root" CSRF token (unauthenticated), and then just the one for the newly-authenticated session. This way, signing in leaves other authenticated scopes alone with their own CSRF token.

Does this work? One change for users is that trying to read session[:csrf_token] manually will fetch the wrong one. (The form_authenticity_token method should be used instead.)

Do you think we still need to clear the "root" CSRF token, or would this change allow it to just be treated as a separate, unauthenticated "scope"? (i.e. it doesn't overlap with the authenticated scopes anymore, so should it matter to clear it?)

@josevalim
Owner

Does this work?

You tell me. :) Can you please give it a go a couple days and let us know how it works in practice?

One change for users is that trying to read session[:csrf_token] manually will fetch the wrong one. (The form_authenticity_token method should be used instead.)

Indeed. This is a small change though, we shouldn't worry about it.

Do you think we still need to clear the "root" CSRF token, or would this change allow it to just be treated as a separate, unauthenticated "scope"? (i.e. it doesn't overlap with the authenticated scopes anymore, so should it matter to clear it?)

Both approaches will likely work fine. Let's continue cleaning just in case.

@josevalim
Owner

@avit and @markedmondson I am not sure how to fix this issue. The proposed solution in #2646 still leaves it vulnerable to attacks. The previous solution on #2642 seems to be a better deal, but have you battle tested it in production? :)

@avit

I posted on that previous PR:

I think this approach is a dead-end: the authenticated user might be posting to an unauthenticated page: there's no way to tell which scope's csrf token should be used.

If we have separate CSRF tokens for each scope, it would be impossible to post anything between scopes, even if you are properly authenticated in both. We would have to do something like:

  • Prefix the scope name into the token so we know which one it is when it is posted, e.g. admin-a1b2c3d4e5
  • Ensure comparisons are done against the correct token in session after splitting to get the scope prefix.

At first glance it looks clumsy, but I can look again to see if it's workable. What do you think about prefixing the scope name like that?

@josevalim
Owner

Prefixing the token seems like a good idea, let's give it a try? :)

@homakov

User signs in
User visits a page that gives her a CSRF token (e.g. a form page)
User opens a second browser window, where she signs in as an admin
User is rejected from posting the form in the first window, and loses the session

Nothing to fix here. Since you're logged in and received new session cookie you can't trust CSRF token you previously had so you have to refresh it. Maximum devise can do is to not destroy session but just reload the page.

By fixing this you introduce vulnerability back!

@avit

This "invalid" CSRF token situation also happens with javascript posts, like auto-complete fields, which makes it difficult to just reload the page correctly in every situation. I hope we can find a solution that doesn't require that.

@homakov

Well auto complete should use GET but i know what you mean. It's not hard to extend jquery_ujs and make page reload seeing new token. Every page laoded before login is potentionally malicious because uses old CSRF token.

@josevalim
Owner

Fixed this on latest master. Rails 4 is migrating to null session and exceptions in the next versions and it will at least allow the user to simply retry instead of discarding the whole session.

@josevalim josevalim closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.