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

Don't hash invalid passwords twice #5655

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

moritzhoeppner
Copy link

@moritzhoeppner moritzhoeppner commented Nov 29, 2023

In order to avoid "unpermitted parameters" errors, the password parameter is permitted by default for the sign_in action (see #2440 and #2452). Now, Devise::SessionsController#new creates a new resource with the permitted parameters before cleaning up the password. This means that an invalid password is hashed twice: The first time - as it should be - when it is validated with Devise::Encryptor.compare (invoked by resource.valid_password?), and then a second time with Devise::Encryptor.digest (invoked by Devise::SessionsController#new). Something similar happens if the authentication key is invalid and Devise.paranoid is true.

I think this is problematic as the number of stretches is always a compromise between password security on the one hand and performance and the danger of generating DoS opportunities on the other hand. Fixing this would probably allow some applications to increase the number of stretches.

My solution is to simply remove the password from the return value of sign_in_params in Devise::SessionsController#new. I think the risk that this breaks something in the application is lower than the risk of changing sign_in_params itself.

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

Successfully merging this pull request may close these issues.

1 participant