Skip to content

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

Merged
merged 3 commits into from
Apr 28, 2022
Merged

fix: user passwords cleanup #1202

merged 3 commits into from
Apr 28, 2022

Conversation

coadler
Copy link
Contributor

@coadler coadler commented Apr 27, 2022

  1. Adds benchmarks comparing bcrypt and our pbkdf2 settings
  2. Changes the pbkdf2 hash iterations back to 65k. 1024 is insecure
  3. 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

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
@coadler coadler self-assigned this Apr 27, 2022
@codecov
Copy link

codecov bot commented Apr 27, 2022

Codecov Report

Merging #1202 (449947f) into main (8661f92) will decrease coverage by 0.91%.
The diff coverage is 82.35%.

@@            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     
Flag Coverage Δ
unittest-go-macos-latest 53.02% <82.35%> (-0.54%) ⬇️
unittest-go-postgres- 64.54% <82.35%> (-0.97%) ⬇️
unittest-go-ubuntu-latest 55.50% <82.35%> (-0.64%) ⬇️
unittest-go-windows-2022 ?
unittest-js 66.50% <ø> (ø)
Impacted Files Coverage Δ
coderd/users.go 60.86% <40.00%> (-2.41%) ⬇️
coderd/userpassword/userpassword.go 70.00% <89.65%> (+6.58%) ⬆️
cli/configssh.go 61.19% <0.00%> (-6.72%) ⬇️
cli/cliui/agent.go 77.19% <0.00%> (-5.27%) ⬇️
pty/ptytest/ptytest.go 86.95% <0.00%> (-4.35%) ⬇️
peerbroker/proxy.go 58.72% <0.00%> (-3.49%) ⬇️
cli/templateinit.go 58.62% <0.00%> (-3.45%) ⬇️
agent/agent.go 63.12% <0.00%> (-3.20%) ⬇️
provisioner/echo/serve.go 56.80% <0.00%> (-2.41%) ⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8661f92...449947f. Read the comment docs.

@coadler coadler requested a review from kylecarbs April 27, 2022 22:45
Copy link
Member

@kylecarbs kylecarbs left a 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.
Copy link
Member

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?

Copy link
Contributor Author

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.

@coadler coadler enabled auto-merge (squash) April 28, 2022 18:18
@coadler coadler merged commit 1661588 into main Apr 28, 2022
@coadler coadler deleted the colin/password-cleanup branch April 28, 2022 18:22
@misskniss misskniss added this to the V2 Beta milestone May 15, 2022
kylecarbs pushed a commit that referenced this pull request Jun 10, 2022
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
```
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.

3 participants