- Notifications
You must be signed in to change notification settings - Fork928
fix: user passwords cleanup#1202
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
1. Adds benchmarks comparing bcrypt and our pbkdf2 settings1. Changes the pbkdf2 hash iterations back to 65k. 1024 is insecure1. Gets rid of the short circuit when the user isn't found, preventing timing attacks which can reveal which emails exist on a deployment
codecovbot commentedApr 27, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #1202 +/- ##==========================================- Coverage 66.27% 65.35% -0.92%========================================== Files 265 261 -4 Lines 16681 16750 +69 Branches 157 157 ==========================================- Hits 11055 10947 -108- Misses 4484 4658 +174- Partials 1142 1145 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Great job improving this. I didn't realize 1024 was insecure, but I'm sure happy you did. 🌮 to you@coadler !
Uh oh!
There was an error while loading.Please reload this page.
| // https://csrc.nist.gov/csrc/media/templates/cryptographic-module-validation-program/documents/security-policies/140sp2261.pdf | ||
| func Compare(hashed string, password string) (bool, error) { | ||
| // If the hased password provided is empty, simulate comparing a real hash. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this so attackers can't use a timing attack to check if the user has no password set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's for users that don't exist, so that there's no difference in response times based on if the user exists or not.
Uh oh!
There was an error while loading.Please reload this page.
1. Adds benchmarks comparing bcrypt and our pbkdf2 settings1. Changes the pbkdf2 hash iterations back to 65k. 1024 is insecure1. Gets rid of the short circuit when the user isn't found, preventing timing attacks which can reveal which emails exist on a deployment```$ go test -bench .goos: linuxgoarch: amd64pkg: github.com/coder/coder/coderd/userpasswordcpu: Intel(R) Core(TM) i9-9900K CPU @ 3.60GHzBenchmarkBcryptMinCost-16 1651 702727 ns/op 5165 B/op 10 allocs/opBenchmarkPbkdf2MinCost-16 1669 714843 ns/op 804 B/op 10 allocs/opBenchmarkBcryptDefaultCost-16 27 42676316 ns/op 5246 B/op 10 allocs/opBenchmarkPbkdf2-16 26 45902236 ns/op 804 B/op 10 allocs/opPASSok github.com/coder/coder/coderd/userpassword5.036s```
Uh oh!
There was an error while loading.Please reload this page.
timing attacks which can reveal which emails exist on a deployment