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

migrating from devise without kicking all sessions out? #916

Closed
gingerlime opened this issue Nov 26, 2020 · 5 comments
Closed

migrating from devise without kicking all sessions out? #916

gingerlime opened this issue Nov 26, 2020 · 5 comments

Comments

@gingerlime
Copy link
Contributor

Hey 👋 ! Thanks for creating clearance. It looks like a breath of fresh air!

We're currently using devise and hoping to transition to clearance. Ideally, we'd like to automatically recognize existing devise sessions and then "migrate" them to clearance on-the-fly, so users won't even notice that we switched.

I can handle the devise deciphering of cookies/sessions to find out if a user is logged-in, and then I guess we can use sign_in(user) in clearance to migrate. I have a couple of questions however, in case anyone has some experience with this, or maybe just ideas on the best way to do this:

  1. Where is the best place to add this kind of on-the-fly migration code? Do we need a middleware for it? or is there a way to inject it somewhere else early enough before the require_login kicks in? also the initial request after deploying the change will have no clearance cookies set yet...
  2. Is there a better/different way to transition from devise to clearance without logging everyone out?
  3. When digging into this I noticed that devise uses signed cookies for the remember_user_token, whereas clearance just uses a plaintext cookie. Is this secure enough? I imagine signing cookies can eliminate hitting the database / timing attacks via brute-forcing the remember token...

Would appreciate any thoughts / tips. Thank you!

gingerlime pushed a commit to gingerlime/clearance that referenced this issue Nov 29, 2020
* see thoughtbot#916
* similar to thoughtbot#909
* also see GHSA-hrqr-hxpp-chr3
  for an example of the type of attack that could be possible with an
  injectable cookie value
* Rails provides signed cookies https://api.rubyonrails.org/classes/ActionDispatch/Cookies.html
  since Rails 3 (??) which prevents tampering
* using a signed cookie instead of a plain one, means the attacker cannot
  forge the cookie value, and therefore cannot perform timing attacks
  to find a valid token
* another added value is that tampering with the cookie will
  not even hit the database
* added a configuration parameter `signed_cookie` so this is optional
  and defaults to false for backwards compatibility
  (however, for better security, it might be better to issue a breaking
   change and default to true)
* updated specs
gingerlime pushed a commit to gingerlime/clearance that referenced this issue Nov 29, 2020
* see thoughtbot#916
* similar to thoughtbot#909
* also see GHSA-hrqr-hxpp-chr3
  for an example of the type of attack that could be possible with an
  injectable cookie value
* Rails provides signed cookies https://api.rubyonrails.org/classes/ActionDispatch/Cookies.html
  since Rails 3 (??) which prevents tampering
* using a signed cookie instead of a plain one, means the attacker cannot
  forge the cookie value, and therefore cannot perform timing attacks
  to find a valid token
* another added value is that tampering with the cookie will
  not even hit the database
* added a configuration parameter `signed_cookie` so this is optional
  and defaults to false for backwards compatibility
  (however, for better security, it might be better to issue a breaking
   change and default to true)
* changed the add_cookies_to_headers method to use ActionDispatch /
  Rails' cookie-handling code to set the cookie
* updated specs
gingerlime pushed a commit to gingerlime/clearance that referenced this issue Nov 29, 2020
* see thoughtbot#916
* similar to thoughtbot#909
* also see GHSA-hrqr-hxpp-chr3
  for an example of the type of attack that could be possible with an
  injectable cookie value
* Rails provides signed cookies https://api.rubyonrails.org/classes/ActionDispatch/Cookies.html
  since Rails 3 (??) which prevents tampering
* using a signed cookie instead of a plain one, means the attacker cannot
  forge the cookie value, and therefore cannot perform timing attacks
  to find a valid token
* another added value is that tampering with the cookie will
  not even hit the database
* added a configuration parameter `signed_cookie` so this is optional
  and defaults to false for backwards compatibility
  (however, for better security, it might be better to issue a breaking
   change and default to true)
* changed the add_cookies_to_headers method to use ActionDispatch /
  Rails' cookie-handling code to set the cookie
* updated specs
@gingerlime
Copy link
Contributor Author

gingerlime commented Nov 30, 2020

Our current workaround looks something like this (added to config/initializers/clearance.rb)

...

module Clearance
  class Session
    # monkey-patch so we can import the `remember_user_token` from devise
    def current_user
      if remember_token.present?
        @current_user ||= user_from_remember_token(remember_token)
      elsif cookies.signed[:remember_user_token].present?
        @current_user ||= Clearance.configuration.user_model.find_by(:id => cookies.signed[:remember_user_token].first)
      end

      @current_user
    end
  end
end

@eebs
Copy link
Contributor

eebs commented Nov 30, 2020

Hi @gingerlime thanks for opening an issue. I haven't personally done such a migration but I've added some thoughts to your questions.

  1. Where is the best place to add this kind of on-the-fly migration code? Do we need a middleware for it? or is there a way to inject it somewhere else early enough before the require_login kicks in? also the initial request after deploying the change will have no clearance cookies set yet...

Could you use Rails' prepend_before_action to ensure your migration code is run before the require_login before_action? You could look for the Devise token and call sign_in in this before block so that the current_user would be set.

  1. Is there a better/different way to transition from Devise to clearance without logging everyone out?

I'm not sure, but your approach of finding the user via the Devise remember token and signing them in seems reasonable to me.

  1. When digging into this I noticed that devise uses signed cookies for the remember_user_token, whereas clearance just uses a plaintext cookie. Is this secure enough? I imagine signing cookies can eliminate hitting the database / timing attacks via brute-forcing the remember token...

Thanks for opening a PR for this issue. I like the idea of using a signed cookie for the remember token as an attacker would have to guess the token and know the secret_key_base. I'll follow up further in the PR.

I'd like to make sure I understand the timing attack concern correctly. The concern is that because Clearance calls Clearance.configuration.user_model.where(remember_token: token).first an attacker could try tokens such as a, then b, etc, and measure the time the database took to respond. By varying the last character of the token and measuring the response time an attacker could eventually arrive at a valid remember_token and thus gain access to an account.

The mitigation would be to use a signed cookie because without the secret_key_base an attacker would always produce invalid cookie values and as such no remember token would be returned. This eliminates the call to the DB avoiding the timing attack.

@gingerlime
Copy link
Contributor Author

Hey @eebs 👋 Thanks for getting back to me.

Could you use Rails' prepend_before_action to ensure your migration code is run before the require_login before_action? You could look for the Devise token and call sign_in in this before block so that the current_user would be set.

Yes, this seems like a great idea. I don't know why I didn't think of it first. I'll give it a try. If it works, then hopefully I can avoid monkey-patching the Session.

I'd like to make sure I understand the timing attack concern correctly. The concern is that because Clearance calls Clearance.configuration.user_model.where(remember_token: token).first an attacker could try tokens such as a, then b, etc, and measure the time the database took to respond. By varying the last character of the token and measuring the response time an attacker could eventually arrive at a valid remember_token and thus gain access to an account.

The mitigation would be to use a signed cookie because without the secret_key_base an attacker would always produce invalid cookie values and as such no remember token would be returned. This eliminates the call to the DB avoiding the timing attack.

Yes, precisely :) you captured it perfectly. it's not a super-simple attack to execute, but certainly possible for a determined attacker. Using a signed cookie eliminates hitting the database even for more casual tampering attempts.

@eebs
Copy link
Contributor

eebs commented Nov 30, 2020

Yes, this seems like a great idea. I don't know why I didn't think of it first. I'll give it a try. If it works, then hopefully I can avoid monkey-patching the Session.

Great, let me know how this goes!

Yes, precisely :) you captured it perfectly. it's not a super-simple attack to execute, but certainly possible for a determined attacker. Using a signed cookie eliminates hitting the database even for more casual tampering attempts.

Excellent, thanks. Let's move this conversation to the PR, I'll provide some thoughts shortly.

gnfisher pushed a commit that referenced this issue Mar 5, 2021
This commit introduces signed cookies into Clearance using the signed cookies
functionality provided by
[ActionDispatch](https://api.rubyonrails.org/classes/ActionDispatch/Cookies.html).
By using a signed cookie an attacker cannot forge the cookie value, and
therefore cannot perform timing attacks to find a valid token. See [this Rack
security
advisory](GHSA-hrqr-hxpp-chr3)
for an example of the type of attack that could be possible with an injectable
cookie value.

This change adds an optional configuration parameter `signed_cookie` which
defaults to false for backwards compatibility and does not use a signed cookie.
The other two options are `true` to use a signed cookie and `:migrate` which
converts unsigned cookies to signed ones and provides a safe transition path.
You can set this via `Clearance.configure` in an initializer:

```ruby
# ./config/initializers/clearance.rb

Clearance.configure do |config|
  # ...

  config.signed_cookie = :migrate

  # ...
end
```

This change also switched to using Rail's `ActionDispatch` for cookie handling
rather than `Rack::Utils`.

See related issues and pull requests:

* #916
* Similar to #909

Co-authored-by: Yoav Aner <[email protected]>
Co-authored-by: Eebs Kobeissi <[email protected]>
MottiniMauro pushed a commit that referenced this issue Mar 5, 2021
This commit introduces signed cookies into Clearance using the signed cookies
functionality provided by
[ActionDispatch](https://api.rubyonrails.org/classes/ActionDispatch/Cookies.html).
By using a signed cookie an attacker cannot forge the cookie value, and
therefore cannot perform timing attacks to find a valid token. See [this Rack
security
advisory](GHSA-hrqr-hxpp-chr3)
for an example of the type of attack that could be possible with an injectable
cookie value.

This change adds an optional configuration parameter `signed_cookie` which
defaults to false for backwards compatibility and does not use a signed cookie.
The other two options are `true` to use a signed cookie and `:migrate` which
converts unsigned cookies to signed ones and provides a safe transition path.
You can set this via `Clearance.configure` in an initializer:

```ruby
# ./config/initializers/clearance.rb

Clearance.configure do |config|
  # ...

  config.signed_cookie = :migrate

  # ...
end
```

This change also switched to using Rail's `ActionDispatch` for cookie handling
rather than `Rack::Utils`.

See related issues and pull requests:

* #916
* Similar to #909

Co-authored-by: Yoav Aner <[email protected]>
Co-authored-by: Eebs Kobeissi <[email protected]>
@dorianmariecom
Copy link
Contributor

I think this would vary too much on a case by case basis. But feel free to document such migration (pull requests with such documentation are welcome).

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

No branches or pull requests

3 participants