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

Timing attacks #238

Closed
danschultzer opened this issue Jul 17, 2019 · 6 comments · Fixed by #239
Closed

Timing attacks #238

danschultzer opened this issue Jul 17, 2019 · 6 comments · Fixed by #239
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@danschultzer
Copy link
Collaborator

This is prompted by danschultzer/ex_oauth2_provider#56.

I haven't thought of this before, but Pow is vulnerable to timing attacks. The response time should be equal for login whether the user exists or not. I'm not sure how big of a deal it is since the registration page already can expose whether a user exists or not. But even then it's probably good practice to prevent it, at least it would help developers who have deactivated registration.

The same vulnerability can exist for the reset password logic, but I believe the process in Pow is already async (and the response is always a success).

I'll review all the extensions and see if there is any other places that is vulnerable, and patch them up. I would appreciate any help going through the repo, as well as any discussion/feedback!

@danschultzer danschultzer added enhancement New feature or request help wanted Extra attention is needed labels Jul 17, 2019
@OvermindDL1
Copy link

Even a signup page shouldn't leak that an account already exists, what it should do is pretend it is successful but send an email saying that a signup attempt was attempted with that same email.

But yes, the timing should definitely be equal for whether a user exists or not, not just with a password hash but with the same DB calls taking about the same amount of time and all.

@volkerholloh
Copy link

volkerholloh commented Jul 18, 2019

Excellent!

Firstly, Dan, thank a lot for this great package! I as a beginner had no trouble implementing it to a hobby project. 👍

But not getting mail-bombed as the rightful owner, there should be just one email.

I wonder if getting the response time equal is a bit hard, no? How about adding a random sleep to an unsuccessful signup/login attempt? To make bruteforcing harder, the sleep time could grow exponentially when the call comes from the same ip or on the same email address/account. The rightful owner would not be harmed as he/she would reset the password after a few attempts.

@OvermindDL1
Copy link

But not getting mail-bombed as the rightful owner, there should be just one email.

Indeed, services that send email on someone that attempted an invalid signup usually only send like 1 email a week summary of it if they send anything at all.

@danschultzer
Copy link
Collaborator Author

Even a signup page shouldn't leak that an account already exists, what it should do is pretend it is successful but send an email saying that a signup attempt was attempted with that same email.

I don't believe leakage can be prevented if you permit registration. If a successful response is shown after registration, all the attacker has to do to confirm if an account already existed, is to use those credentials again when logging in. I think this can only be prevented if the user id is always an email, and email confirmation is required (this is outside the scope of the Pow core).

The current setup in Pow is to give proper feedback to the user with already taken user id from the ecto changeset. By default, Pow also automatically creates a session for the user upon registration. Since I don't see any way to prevent leakage 100% in Pow core, I don't think it'll be worth it to change the UX. Instead, I'll let it be up to the developer to adjust their Pow implementation if it better fits their requirements (it would be trivial to change the registration flow with custom controller). Maybe a guide detailing security considerations like this would be good to have in Pow.

I'll work to ensure that the response time is equal for all code paths in the sign in flow, since modifying that wouldn't change the UX.

I wonder if getting the response time equal is a bit hard, no? How about adding a random sleep to an unsuccessful signup/login attempt?

That's intuitive, but it won't work for timing attacks. The best way to handle this is to make the exact same calculations as with successful authentication. In this case it would be to hash and match the password, even if we don't care about the return value. In some cases, like reset password, it's best to handle it all in a background job and always return success response early.

@danschultzer
Copy link
Collaborator Author

I've opened #239, and it should prevent timing attacks on sign in. It's impossible to unit test timing attacks themselves so I'm testing the code paths with process messages.

@danschultzer
Copy link
Collaborator Author

I don't believe leakage can be prevented if you permit registration. If a successful response is shown after registration, all the attacker has to do to confirm if an account already existed, is to use those credentials again when logging in. I think this can only be prevented if the user id is always an email, and email confirmation is required (this is outside the scope of the Pow core).

I was revising this recently and thought some more about it. I think if PowEmailConfirmation is used, then it definitely should work as @OvermindDL1 wrote to prevent information leakage by default. PowResetPassword already has such prevention. I've opened #350 to resolve this so if PowEmailConfirmation is enabled, the default "e-mail confirmation required" message will be shown if the only error is that the email has been taken 🎉

I'm also updating all extensions so they will prevent info leak if PowEmailConfirmation has been enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants