-
Notifications
You must be signed in to change notification settings - Fork 937
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1. Adds benchmarks comparing bcrypt and our pbkdf2 settings 1. Changes the pbkdf2 hash iterations back to 65k. 1024 is insecure 1. Gets rid of the short circuit when the user isn't found, preventing timing attacks which can reveal which emails exist on a deployment
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 !
// 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.
1. Adds benchmarks comparing bcrypt and our pbkdf2 settings 1. Changes the pbkdf2 hash iterations back to 65k. 1024 is insecure 1. 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: linux goarch: amd64 pkg: github.com/coder/coder/coderd/userpassword cpu: Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz BenchmarkBcryptMinCost-16 1651 702727 ns/op 5165 B/op 10 allocs/op BenchmarkPbkdf2MinCost-16 1669 714843 ns/op 804 B/op 10 allocs/op BenchmarkBcryptDefaultCost-16 27 42676316 ns/op 5246 B/op 10 allocs/op BenchmarkPbkdf2-16 26 45902236 ns/op 804 B/op 10 allocs/op PASS ok github.com/coder/coder/coderd/userpassword 5.036s ```
timing attacks which can reveal which emails exist on a deployment