expire_all_remember_me_on_sign_out does not work as expected and might be obsolete

JanBussieck commented 5 years ago

Current behavior

When I set expire_all_remember_me_on_sign_out = false in the devise config, but I am using a remember_token (instead of defaulting to the authenticatable_salt), then I will still be logged out on all devices.

Expected behavior

I will only be logged out on the device where I initiate the log out and still be remembered on other devises.

This happens since the remember_token is reset regardless of whether the expire_all_remember_me_on_sign_out is set or not: https://github.com/plataformatec/devise/blob/master/lib/devise/models/rememberable.rb#L60

This was previously addressed in this issue: https://github.com/plataformatec/devise/issues/3236

where @josevalim mentions:

This is the intended behaviour. If the user signs out in a machine, we should not keep any information that would allow the user to sign in again, as this would be a security issue.

Given that that is the case why enable this option at all? Especially considering that it doesn't work as expected. Thanks in advance and thanks for your work on this great gem.

tegon commented 5 years ago

Hi @JanBussieck, thanks for reporting this.

I've tested the following possible scenarios:

When `expire_all_remember_me_on_sign_out` is `true`:

with `authenticatable_salt`

Sign In in two browsers, log out of one
The other one should be logged out too

Works correctly 👍

with `remember_token`

Sign In in two browsers, log out of one
The other one should be logged out too

Works correctly 👍

When `expire_all_remember_me_on_sign_out` is `false`:

with `authenticatable_salt`

Sign In in two browsers, log out of one
The other one should still be logged in

Works correctly 👍

with `remember_token`

Sign In in two browsers, log out of one
The other one should still be logged in

It doesn't work 👎

I believe your scenario is the last one, right?

Unfortunately, I don't know the historical reasons why this approach was followed. After looking at the original pull request it seems like the scenario with remember_token was never mentioned. I think it might have got forgotten since the default is authenticable_salt.

Regarding the security concerns, I'm not sure what they were since the cookie is going to be signed by Rails using secret_key_base or similar. Maybe @diegorv can help us here. In any case, this already happens for the authenticable_salt case. Now, this case might be seen as more secure since authenticable_salt uses part of the encrypted_password column - which is the hashed password.

So, in conclusion, I think this is definitely an inconsistency in Devise since it works for one case but not for the other. And I think it's better to fix it - i.e. dot not clean the remember_token when expire_all_remember_me_on_sign_out is false. We can add documentation to advise users that they should implement some logic to generate unique remember_tokens that don't contain sensitive information about their users and I think that should be enough.

Thoughts?

JanBussieck commented 5 years ago

Hi @tegon, thanks for your response and the additional clarity you provided. That is exactly the problem I encountered.

As far as I can see, the advantage over using a remember_token over the authenticable_salt is that it is regenerated every time a new remember-me-session is created, which means that if a remember-me-cookie were to be hijacked it would only be usable for one session, whereas a remember-me-cookie containing the authenticatable_salt as token will remain valid for future sessions unless the user changes their password. I could imagine that the reasoning behind not resetting the remember_token is that it would require a user to log out on all devises in order to invalidate the current remember-me-session.

Since the remember-me-session is stored on disk rather than in memory (and thus offering more vectors of attack for cookie hijacking), there are some approaches that recommend regenerating the token every time the cookie is used to authenticate the user. While providing additional security, this would imply that a user could not stay logged in on several devices (or even browsers) at once. A potential way to tackle the problem might be to store remember-me-data (i.e. token and timestamps) in a separate table referencing the user and create a separate entry for every session.

I am not really sure, just spit balling ideas at this point.

ritaly commented 4 years ago

Hi @tegon 👋 I try to handle (maybe) related issue, but there's no information about expire_all_remember_me_on_sign_out in Rememberable doc.

Could be nice to add info to doc - the default value for expire_all_remember_me_on_sign_out is true (look like it should be true)

Then I don't get this scenario:

with `remember_token`

and `expire_all_remember_me_on_sign_out` is `true`

# config/initializers/devise.rb
...
config.timeout_in = 2.hours
config.remember_for = 1.day
config.expire_all_remember_me_on_sign_out = true

Sign In in two browsers, both remember_me checked
Log out of one
The other one should be expired/logged out too, BUT it isn't - remember_user_token is still valid. Should I add sth more to my initializers?

heartcombo / devise