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

Default password comparison should use constant-time string comparison #82

Closed
brendanlong opened this issue Apr 9, 2019 · 1 comment · Fixed by #83
Closed

Default password comparison should use constant-time string comparison #82

brendanlong opened this issue Apr 9, 2019 · 1 comment · Fixed by #83
Assignees
Labels

Comments

@brendanlong
Copy link
Contributor

The default for comparing passwords is client_password == stored_password:

https://github.com/miguelgrinberg/Flask-HTTPAuth/blob/master/flask_httpauth.py#L146

This is a problem because Python compares strings character-by-character, so a password closer to the correct password takes longer to check. You might expect that this doesn't matter, but with thousands of requests and some statistics, an attacker can guess the password much faster than they should be able to:

https://codahale.com/a-lesson-in-timing-attacks/

There are two options for working around this (not including "don't use plaintext passwords"):

  • Use a constant-time string comparison like hmac.compare_digest. This seems to require Python 3.3 but you could also just vendor this function.
  • Compare hashes. It's kind of silly to hash two plaintext passwords, but this is the simplest way to avoid timing attacks in this comparison. For this purpose, the hash function doesn't really matter very much and you just want it to be fast (MD5 would be fine, but SHA1 will probably get you fewer people complaining about things they don't understand)
@miguelgrinberg
Copy link
Owner

I agree. This is on the oldest password checking approach, which I hope very few people use at this point, but still, no reason to not eliminate the risk of a timing attack. Would you like to submit a PR with the change?

@miguelgrinberg miguelgrinberg self-assigned this Apr 9, 2019
brendanlong added a commit to brendanlong/Flask-HTTPAuth that referenced this issue Apr 10, 2019
This uses constant time string comparisons everywhere that looked plausibly like it
might matter:

 - Plaintext password comparison
 - Digest nonce comparison
 - Digest hash comparison

Fixes miguelgrinberg#82
brendanlong added a commit to brendanlong/Flask-HTTPAuth that referenced this issue Apr 13, 2019
This uses constant time string comparisons everywhere that looked plausibly like it
might matter:

 - Plaintext password comparison
 - Digest nonce comparison
 - Digest hash comparison

Fixes miguelgrinberg#82
brendanlong added a commit to brendanlong/Flask-HTTPAuth that referenced this issue Apr 29, 2019
This uses constant time string comparisons everywhere that looked plausibly like it
might matter:

 - Plaintext password comparison
 - Digest nonce comparison
 - Digest hash comparison

Fixes miguelgrinberg#82
brendanlong added a commit to brendanlong/Flask-HTTPAuth that referenced this issue Apr 29, 2019
This uses constant time string comparisons everywhere that looked plausibly like it
might matter:

 - Plaintext password comparison
 - Digest nonce comparison
 - Digest hash comparison

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

Successfully merging a pull request may close this issue.

2 participants