Skip to content

Don't leak information about the existence of accounts in SessionsController #1600

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

Merged

Conversation

moritzhoeppner
Copy link
Contributor

The SessionsController leaks information about the existence of accounts even if Devise's paranoid mode is activated. It leaks this information in the following ways:

  1. If the resource is not found, the password isn't hashed, resulting in a probably significantly lower response time (given that the cost factor is configured correctly).
  2. If an account is locked or not confirmed, the error messages say so.

The pull request fixes these issues by:

  1. Hashing the given password if paranoid mode is activated and the resource isn't found.
  2. Always responding with "bad credentials" if paranoid mode is activated and the login fails.

@moritzhoeppner moritzhoeppner force-pushed the paranoid-sessions-controller branch 2 times, most recently from 11091de to f6d9f84 Compare May 31, 2023 19:40
if @resource.respond_to?(:locked_at) && @resource.locked_at
render_create_error_account_locked
else
render_create_error_not_confirmed
end
else
# In order to avoid timing attacks in paranoid mode, we want the password hash to be
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you move this new code and explanation to another method so doesn't make bigger than it's?

Copy link
Contributor Author

@moritzhoeppner moritzhoeppner Jun 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for the review :)
I moved the code in a new method and also took the liberty to refactor the beginning of the create method a bit. Hope that's fine.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@moritzhoeppner moritzhoeppner force-pushed the paranoid-sessions-controller branch from 9f7a272 to 2ba1ef7 Compare June 5, 2023 19:03
@MaicolBen MaicolBen merged commit 207a246 into lynndylanhurley:master Jun 6, 2023
ThiagoAnunciacao pushed a commit to ThiagoAnunciacao/devise_token_auth that referenced this pull request Jul 6, 2023
…troller (lynndylanhurley#1600)

* Don't leak information about the existence of accounts in DeivseTokenAuth::SessionsController

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

Successfully merging this pull request may close these issues.

2 participants