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

Confirmable's security model leads to a poor user experience #3640

Closed
vincentwoo opened this issue Jun 16, 2015 · 16 comments
Closed

Confirmable's security model leads to a poor user experience #3640

vincentwoo opened this issue Jun 16, 2015 · 16 comments

Comments

@vincentwoo
Copy link
Contributor

I'm not sure if this is by design, but the following happens to me every day:

  1. A user registers for my website.
  2. They are directed to a specially crafted page I added to help deal with this very problem that tells them to sit tight the email to confirm is coming ASAP.
  3. User gets antsy, and requests a second confirmation email anyway.
  4. The first email arrives, but its link no longer works because requesting the second confirmation email resets the token.
  5. User gets upset and emails me. I confirm their account for them.

I poked into the devise code to do something like "reuse the last token if it was generated in the last minute or five", but that's not feasible because Confirmable discards the original token, and only stores its digest.

Is there any interest in... not... doing that? The tokens are randomly generated anyway, so I'm not sure what value there is in obfuscating the original token in the database.

This default behavior also makes it possible for me to indefinitely lock a new user out of their account by just requesting a new confirmation email for them every X seconds.

@josevalim
Copy link
Contributor

@vincentwoo the reason we added it was for security... but I realized that, since we no longer log users in after confirming their accounts, we don't need it. So we can fix it for confirmable but keep in mind reset password will have the same issue (and we can't fix it).

@vincentwoo
Copy link
Contributor Author

@josevalim By adding "it", do you mean having their be a token/digest system for confirmation? What security vulnerability did you perceive there to be with logging in after confirmation that you'd need to have a digest for?

I log my users in after confirmation because its insane to force them to login again - anyone who can compromise a user's email can log into their account whenever they want.

@josevalim
Copy link
Contributor

@vincentwoo if you sign them in automatically then anyone that has access to the token can gain access to their account. Someone from your team, someone that had temporary access to someone else email and so on.

@vincentwoo
Copy link
Contributor Author

How is that alleviated in any way by using a confirmation digest instead of a straight token lookup?

@josevalim
Copy link
Contributor

It doesn't solve the case someone access it in the e-mail (the end token) but if someone gain access to the database, they won't be able to gain access to the accounts. Because the password and all tokens are encrypted too. This is a risk you can choose to take but we won't by default in Devise.

@vincentwoo
Copy link
Contributor Author

I suppose. This only really helps you if (1) the attacker only gets read-only access to the DB and (2) they're interested in compromising an account that is still unconfirmed.

But like you said, since Devise doesn't login by default, I think it would be a fantastic UX improvement to revert the digesting and making the emailing smarter.

@vincentwoo
Copy link
Contributor Author

Ping! Any desire to do this?

@josevalim
Copy link
Contributor

Definitely. We can do the easiest step which is to just stop encrypting confirmation tokens. Keep in mind though that we will need to support both kinds of tokens for a while. :) In the future, we can remove the token altogether.

@carlosantoniodasilva
Copy link
Member

I'm just not sure if it is worth going through all the trouble since it
won't matter at the end for the developer whether the confirmation token is
encrypted or not. At least this way it is consistent (not saying I am
completely opposed, just saying it is a lot of trouble for no apparent
gain).

Thanks!

Carlos Antonio da Silva - via celular
On Jun 23, 2015 7:11 AM, "José Valim" [email protected] wrote:

Definitely. We can do the easiest step which is to just stop encrypting
confirmation tokens. Keep in mind though that we will need to support both
kinds of tokens for a while. :) In the future, we can remove the token
altogether.


Reply to this email directly or view it on GitHub
#3640 (comment)
.

@vincentwoo
Copy link
Contributor Author

@carlosantoniodasilva I'm not sure what position you're advocating.

@josevalim
Copy link
Contributor

@carlosantoniodasilva the gain is that we can resend the token without generating a new one and without causing confusion to the user in case they ask the confirmation to be resent .

@carlosantoniodasilva
Copy link
Member

@vincentwoo I was trying to explain I'm not sure it's worth changing at this point, sorry about being uncler.

@josevalim this is a good point, however there was someone on the mailing list these days asking for the exact same thing - but for reset password. It seems a bit inconsistent making the two implementations different based on that, though.

Anyway, this is just my 2 cents, I'm ok with changing if you feel fine about it. ❤️

@josevalim
Copy link
Contributor

We can do this for reset password because it is insecure. It is fine for confirmation. Another approach is to just remove all tokens from the database and use encrypted tokens, this way we have generate as many tokens as we want and they would all be valid for a time period.

@carlosantoniodasilva
Copy link
Member

Yeah, I'm not saying we should do that for both, just saying I don't like much the inconsistency, but I could live with it 😅. This other approach seems good as well.

@vincentwoo
Copy link
Contributor Author

fixed in #3661

@jarmo
Copy link

jarmo commented Nov 24, 2015

Few years ago I wanted to implement "gradual engagament" with Devise on a project, which had been on-going for some time already. I spent more hours than expected because example code in Wiki didn't work. The reason was that at the beginning of project Devise did send confirmation tokens in plain text (e.g. mailer template was generated with older version of Devise), but the wiki page assumed that they are sent as digested.

Yesterday I wanted to implement "gradual engagement" again on a totally new project, but spent also few hours. Reason is this change here - now tokens are sent as plain text again, but wiki still assumes they are sent as digests. There's no easy way to update Wiki as well, because it already has solutions for gazillion different Devise/Rails versions (see here as well).

Also, current solution makes "gradual engagement" solution in the Wiki less secure, because solutions provided there will sign in after confirmation - in other words, if database leaks, then it is possible to log in unconfirmed users again. And it is a problem, because security is as good as it's weakest link.

Also, if someone has access to a (read-only) database, then having a way to log users in might make problems worse. The reason might be that usually applications have some functionality using 3rd party services, which add more into attacker's arsenal (what about payment-providers?).

I'd say that better approach would be to use HMAC (see example in here) solution to lose a need for columns in database altogether.

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

No branches or pull requests

4 participants