prevent remember_token timing attacks

[gingerlime]

• see migrating from devise without kicking all sessions out? #916
• similar to Prevent user enumeration by timing attacks #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]

just realized that this is being called from lib/clearance/rack_session.rb so we can actually remove the headers param which is no longer needed.

[gingerlime]

Happy to refine the PR, but first want to check the project is open to this change in principle. I think it's important from a security standpoint, and I did my best not to break backwards compatibility. We're still not using clearance in production, but I tested it quickly on my dev environment, and it seems to work with signed cookies as well as without them.

[eebs]

Thanks for the PR @gingerlime. I agree that the change would make Clearance more secure and would like to find a way to move forward.

My main concerns are 1. Backwards compatibility and 2. Ease of transition. I'm wondering if it's possible to use some sort of fall back mechanism rather than an explicit configuration option. Would it make sense to always write the signed cookie when storing the remember token, and when reading the token we first look for a signed cookie and fall back to the regular cookie? Would this allow us to ship the change and as folks used the app it would migrate their cookies?

After some time we could remove the fallback. This would cause anyone that was signed in via a remember token to be signed out if they had not visited the app. This assumes that applications would upgrade to the version that introduced the fallback and then upgrade to the version that removed the fallback and relied solely on the encrypted cookie. If an application were to upgrade directly to the non-fallback version, all their users would be logged out. I think we could address this with a version bump if we felt uncomfortable with potentially logging users out.

What are your thoughts on this? Are there side effects I haven't considered? Thanks!

[gingerlime]

Hi @eebs. Interesting. I like your thinking 👍

Yes, it's possible and even simplifies the code if we try the signed cookie and fall back to the plain one. It can also provide a smoother transition. This is similar to what we're planning to do when transitioning from devise to clearance :)

but ... (there's always a but)

1. It's still not secure until you completely switch to signed cookies. i.e. your system is still vulnerable to timing attacks during the transition
2. If you upgrade and then there's some (unrelated) problem, you cannot roll back to the previous version without kicking users out for mismatching tokens
3. How do you plan to let users control the steps of the transition? e.g. for us, we don't need the transition, and prefer to switch to signed cookies from day one (we're not running clearance yet), other users might decide that they don't want to take the risk and switch to signed cookies within X days/weeks... So I think you're still looking at some kind of config to switch modes?

I would definitely see the benefit of having an option to transition cookies from plain to signed. So it's definitely worth looking at what it means to offer this transition option. I mentioned the code becomes simpler, but the tests will need quite a bit of work probably. However, I imagine that you would still want some way to control which option you use via config, rather than solely rely on versions.

Apologies, it's getting a bit late over here, so I might not have considered everything. I'm also still not familiar with the clearance codebase. Happy to continue this discussion and finding the best way forward.

[eebs]

Those are all great points, thanks for raising them. Perhaps we can find some compromise where we are secure by default, but for anyone that wishes to keep the old behavior, they could enable the configuration option. I'd like to think about this a bit more and gather some additional feedback. Happy to keep the discussion going in the meantime. 🤗

[gingerlime]

Yes, my initial thinking was to have a config defaulting to false (for backwards compatibility) and eventually defaulting to true (with the appropriate breaking changes warnings), and perhaps later on maybe removing the insecure option completely, but well into the future...

I didn't consider the transition problem... especially odd since the transition from devise brought me to the whole signed cookie thing in the first place ;-)

I just tested quickly and it's fairly simple to "migrate" insecure cookies with a prepend_before_filter like you suggested, e.g.

class ApplicationController < ActionController::Base
  before_action :require_login
  ...
  prepend_before_action :migrate_token

  def migrate_token
    # if cookie is already signed, do nothing
    return if cookies.signed["remember_token"]

    # if old (insecure) cookie exists
    if cookies["remember_token"].present?
      # and if we can find a valid user with it
      if user = User.find_by(:remember_token => cookies["remember_token"])
        sign_in(user)  # this will set the token to signed, if the config is set to true
      end
    end
  end

So then either users are advised to create this own migration on their own, or it can also be implemented within clearance (with another flag? or using a 3-state flag? e.g. true, false, migrate or something?)

Just throwing some ideas out there.

[hound]

Style/BlockDelimiters: Avoid using {...} for multi-line blocks.

[gingerlime]

@eebs can this be configured in hound? I'm not sure where/how it's set or what configuration you use.

I personally think that with a DSL like rspec, it's more readable to use expect { something }.to eq(...) is much more readable than expect do something end.to eq(...)

Perhaps using Semantic or braces_for_chaining or adding expect to the list of BracesRequiredMethods ? see https://rubydoc.info/gems/rubocop/RuboCop/Cop/Style/BlockDelimiters

[eebs]

Thanks for the feedback @gingerlime. We'll certainly consider that in the future but for now, let's go with the existing style guide.

[gingerlime]

👍 of course it's your call. If you guys want to apply this style, then please go ahead. I'd rather keep my code contributions with what I personally consider to be readable, clear and concise.

[gingerlime]

@eebs I pushed a commit that introduces a 3-way configuration for signed_cookie:

• false (default, backwards compatible, insecure)
• :migrate (still insecure, but allows users to migrate their unsigned cookies over a period of time)
• true (forces signed cookies)

I think this gives users and the project the most flexibility. Thanks for pointing this out! and I hope it fits with your concept.

[gingerlime]

@eebs it's been a while and I'm curious to figure out where the wind blows on this one 🌬️ :)

[eebs]

Hey @gingerlime sorry for the lack of communication, I haven't had time to follow up on this. I'm going to ask for some feedback from some folks internally (and would also love feedback from the rest of the community if you have thoughts!) and will get back to you soon. Thanks for your patience!

[mike-burns]

The migration path taken here is excellent -- thank you for putting the effort into that. I think this is an overall improvement. LGTM.

[eebs]

Thanks for your patience @gingerlime. I've left a few questions/comments but this is looking great.

Would you mind addressing the style issues noted by Hound as well?

Thanks!

[eebs]

I'm curious what the difference with this new implementation. We used to use Rack::Utils Is there any meaningful difference here?

[gingerlime]

As far as I understand, Rack::Utils is probably much lower level. Rails might use it under the hood, or use some other way to add cookies to Rack. Since Clearance is geared towards Rails (exclusively?), then I think it makes sense to use the Rails' built-in helper for handling cookies rather than Rack::Utils... I could be wrong though, I'm not that familiar with Rails internals and didn't dig too deep into it.

[eebs]

Is there a way we could make this exception clearer? I'm concerned that if this were in a stack trace somewhere it wouldn't be clear to a user where it was coming from or how to fix it.

Would something like

"Clearance's signed_cookie configuration value is invalid. Valid values are true, false, or :migrate. Set this option via Clearance.configure in an initializer"

[gingerlime]

Sounds good to me.

[gingerlime]

@eebs Thanks for the feedback.

I made another change to remove the (now unnecessary) param to add_cookie_to_headers. I also applied your suggestions + fixed most of the styleguide issues. There were a couple that I personally don't feel comfortable with, but it's your project, so please go ahead and apply them as you see fit.

Btw, Travis is kinda dead as far as I can see? I've read on HN that they backed away from their promise to keep it free for open-source? perhaps you guys want to switch to github actions or something else?

[eebs]

Thanks for the updates @gingerlime, I haven't forgotten about this PR but I've been busy recently. I'll work to get it in line with the style guide and merged.

I would be open to moving to GitHub Actions but don't have a lot of experience setting that up yet.

[gingerlime]

@eebs I'm not that familiar with github actions either, but after a bit of reading I converted the travis.yml to github actions on #926 ... for some strange reason the rails 6.0 tests are failing, but I don't know why. I don't know how long travis stopped working, so perhaps the issue already existed before?

[gingerlime]

Ah, looks like travis already failed with rails 6.0 anyway... and I don't think it's in any way related to this PR.

[gingerlime]

Any updates on this one? can't say I'm super motivated to continue to contribute to this project considering how long it takes to get an security fix implemented.

We're running this branch live for a few weeks now with 1m+ registered users without any issues. Note however, we migrated from devise to clearance and went directly with the signed_cookie = true option.

[gnfisher]

Hello @gingerlime, sorry for the delay in getting back to you. Thank you very much for all your work on this! It looks great and ready to merge. 👍