aaronrenner / phx_gen_auth

An authentication system generator for Phoenix 1.5 applications.
772 stars 55 forks source link

Account Enumeration Vulnerability #98

Closed samu closed 3 years ago

samu commented 3 years ago

The UserRegistrationController is vulnerable to the account enumeration vulnerability, as describe in this OWASP article. If we try to create a user with an email which already exists, we will see the respective validation message.

As described in the article, this behavior might be a design choice, and i understand if this generator wants to leave it open to the developer to close this vulnerability. However, i don't understand why in the ResetPasswordController an impartial error message is returned (see here).

Would you agree that, for consistency reasons, it would be better if these controllers stuck to a specific strategy?

Would you consider closing the account enumeration vulnerability in the UserRegistrationController by simply returning a success message, if all the validations, except the email uniqueness constraint, succeeded?

josevalim commented 3 years ago

The guides outline this: https://github.com/aaronrenner/phx_gen_auth/blob/a6be7e5f0f5e244b0de0561b256b4774549d2a1b/guides/overview.md#enumeration-attacks

The reason why we do it for almost everything is because it is much easier for you, as the user, to close two holes than all of them. After all, it is most likely you will customize the signup and update workflows anyway, while the other ones are more likely to be used as is. :)

samu commented 3 years ago

I'm not sure if i fully understand the reasoning here. Wouldn't it be easier to add the extra logic of filtering out the email uniqueness constraint validation error and returning success if that's the only error, and if somebody doesn't like that behaviour, they can simply remove the relevant code? Removing the code is easy - however, being aware of this vulnerability is hard, so in my opinion it would be worth to consider adding it.

The guides say this:

If your application is really sensitive to enumeration attacks, you need to implement your own registration workflow, which tends to be very different from the workflow for most applications

Again, i'm not sure if i fully understand this reasoning. Why is it very different? All we have to do is: not show the error, and return success. Maybe i'm missing something.

josevalim commented 3 years ago

Well, if the user tries to sign up and the user is already on the system, what do you do? Maybe you say, well, log the user in. But you can only do that if the password is the same. If the password is not the same, you can't log them in, and by simply not logging someone in as part of what is seemingly a valid sign up, you would leak they are in the system. So the only solution is to never sign everyone up, but this can be a very poor user experience for most applications. You will most likely want to customize it so users are not lost along the way.

samu commented 3 years ago

Right, i see. Since we want to sign in the user right after sign-up, we have no other choice than telling him if it was successful or not. Thanks for the explanation.