Skip to content

Commit 1661588

Browse files
coadlercoadler
authored
fix: user passwords cleanup (coder#1202)
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 ```
1 parent e330dc1 commit 1661588

File tree

3 files changed

+137
-24
lines changed

3 files changed

+137
-24
lines changed

‎coderd/userpassword/hashing_bench_test.go

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
package userpassword_test
2+
3+
import (
4+
"crypto/sha256"
5+
"testing"
6+
7+
"github.com/coder/coder/cryptorand"
8+
"golang.org/x/crypto/bcrypt"
9+
"golang.org/x/crypto/pbkdf2"
10+
)
11+
12+
var (
13+
salt = []byte(must(cryptorand.String(16)))
14+
secret = []byte(must(cryptorand.String(24)))
15+
16+
resBcrypt []byte
17+
resPbkdf2 []byte
18+
)
19+
20+
func BenchmarkBcryptMinCost(b *testing.B) {
21+
var r []byte
22+
b.ReportAllocs()
23+
24+
for i := 0; i < b.N; i++ {
25+
r, _ = bcrypt.GenerateFromPassword(secret, bcrypt.MinCost)
26+
}
27+
28+
resBcrypt = r
29+
}
30+
31+
func BenchmarkPbkdf2MinCost(b *testing.B) {
32+
var r []byte
33+
b.ReportAllocs()
34+
35+
for i := 0; i < b.N; i++ {
36+
r = pbkdf2.Key(secret, salt, 1024, 64, sha256.New)
37+
}
38+
39+
resPbkdf2 = r
40+
}
41+
42+
func BenchmarkBcryptDefaultCost(b *testing.B) {
43+
var r []byte
44+
b.ReportAllocs()
45+
46+
for i := 0; i < b.N; i++ {
47+
r, _ = bcrypt.GenerateFromPassword(secret, bcrypt.DefaultCost)
48+
}
49+
50+
resBcrypt = r
51+
}
52+
53+
func BenchmarkPbkdf2(b *testing.B) {
54+
var r []byte
55+
b.ReportAllocs()
56+
57+
for i := 0; i < b.N; i++ {
58+
r = pbkdf2.Key(secret, salt, 65536, 64, sha256.New)
59+
}
60+
61+
resPbkdf2 = r
62+
}
63+
64+
func must(s string, err error) string {
65+
if err != nil {
66+
panic(err)
67+
}
68+
69+
return s
70+
}

‎coderd/userpassword/userpassword.go

Lines changed: 62 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,25 +6,67 @@ import (
66
"crypto/subtle"
77
"encoding/base64"
88
"fmt"
9+
"os"
910
"strconv"
1011
"strings"
1112

1213
"golang.org/x/crypto/pbkdf2"
14+
"golang.org/x/exp/slices"
1315
"golang.org/x/xerrors"
1416
)
1517

16-
const (
17-
// This is the length of our output hash.
18-
// bcrypt has a hash size of 59, so we rounded up to a power of 8.
18+
var (
19+
// The base64 encoder used when producing the string representation of
20+
// hashes.
21+
base64Encoding = base64.RawStdEncoding
22+
23+
// The number of iterations to use when generating the hash. This was chosen
24+
// to make it about as fast as bcrypt hashes. Increasing this causes hashes
25+
// to take longer to compute.
26+
defaultHashIter = 65535
27+
28+
// This is the length of our output hash. bcrypt has a hash size of up to
29+
// 60, so we rounded up to a power of 8.
1930
hashLength = 64
31+
2032
// The scheme to include in our hashed password.
2133
hashScheme = "pbkdf2-sha256"
34+
35+
// A salt size of 16 is the default in passlib. A minimum of 8 can be safely
36+
// used.
37+
defaultSaltSize = 16
38+
39+
// The simulated hash is used when trying to simulate password checks for
40+
// users that don't exist.
41+
simulatedHash, _ = Hash("hunter2")
2242
)
2343

24-
// Compare checks the equality of passwords from a hashed pbkdf2 string.
25-
// This uses pbkdf2 to ensure FIPS 140-2 compliance. See:
44+
// Make password hashing much faster in tests.
45+
func init() {
46+
args := os.Args[1:]
47+
48+
// Ensure this can never be enabled if running in server mode.
49+
if slices.Contains(args, "server") {
50+
return
51+
}
52+
53+
for _, flag := range args {
54+
if strings.HasPrefix(flag, "-test.") {
55+
defaultHashIter = 1
56+
return
57+
}
58+
}
59+
}
60+
61+
// Compare checks the equality of passwords from a hashed pbkdf2 string. This
62+
// uses pbkdf2 to ensure FIPS 140-2 compliance. See:
2663
// https://csrc.nist.gov/csrc/media/templates/cryptographic-module-validation-program/documents/security-policies/140sp2261.pdf
2764
func Compare(hashed string, password string) (bool, error) {
65+
// If the hased password provided is empty, simulate comparing a real hash.
66+
if hashed == "" {
67+
hashed = simulatedHash
68+
}
69+
2870
if len(hashed) < hashLength {
2971
return false, xerrors.Errorf("hash too short: %d", len(hashed))
3072
}
@@ -42,37 +84,40 @@ func Compare(hashed string, password string) (bool, error) {
4284
if err != nil {
4385
return false, xerrors.Errorf("parse iter from hash: %w", err)
4486
}
45-
salt, err := base64.RawStdEncoding.DecodeString(parts[3])
87+
salt, err := base64Encoding.DecodeString(parts[3])
4688
if err != nil {
4789
return false, xerrors.Errorf("decode salt: %w", err)
4890
}
4991

5092
if subtle.ConstantTimeCompare([]byte(hashWithSaltAndIter(password, salt, iter)), []byte(hashed)) != 1 {
5193
return false, nil
5294
}
95+
5396
return true, nil
5497
}
5598

5699
// Hash generates a hash using pbkdf2.
57100
// See the Compare() comment for rationale.
58101
func Hash(password string) (string, error) {
59-
// bcrypt uses a salt size of 16 bytes.
60-
salt := make([]byte, 16)
102+
salt := make([]byte, defaultSaltSize)
61103
_, err := rand.Read(salt)
62104
if err != nil {
63105
return "", xerrors.Errorf("read random bytes for salt: %w", err)
64106
}
65-
// The default hash iteration is 1024 for speed.
66-
// As this is increased, the password is hashed more.
67-
return hashWithSaltAndIter(password, salt, 1024), nil
107+
108+
return hashWithSaltAndIter(password, salt, defaultHashIter), nil
68109
}
69110

70111
// Produces a string representation of the hash.
71112
func hashWithSaltAndIter(password string, salt []byte, iter int) string {
72-
hash := pbkdf2.Key([]byte(password), salt, iter, hashLength, sha256.New)
73-
hash = []byte(base64.RawStdEncoding.EncodeToString(hash))
74-
salt = []byte(base64.RawStdEncoding.EncodeToString(salt))
75-
// This format is similar to bcrypt. See:
76-
// https://en.wikipedia.org/wiki/Bcrypt#Description
77-
return fmt.Sprintf("$%s$%d$%s$%s", hashScheme, iter, salt, hash)
113+
var (
114+
hash = pbkdf2.Key([]byte(password), salt, iter, hashLength, sha256.New)
115+
encHash = make([]byte, base64Encoding.EncodedLen(len(hash)))
116+
encSalt = make([]byte, base64Encoding.EncodedLen(len(salt)))
117+
)
118+
119+
base64Encoding.Encode(encHash, hash)
120+
base64Encoding.Encode(encSalt, salt)
121+
122+
return fmt.Sprintf("$%s$%d$%s$%s", hashScheme, iter, encSalt, encHash)
78123
}

‎coderd/users.go

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -461,21 +461,19 @@ func (api *api) postLogin(rw http.ResponseWriter, r *http.Request) {
461461
if !httpapi.Read(rw, r, &loginWithPassword) {
462462
return
463463
}
464+
464465
user, err := api.Database.GetUserByEmailOrUsername(r.Context(), database.GetUserByEmailOrUsernameParams{
465466
Email: loginWithPassword.Email,
466467
})
467-
if errors.Is(err, sql.ErrNoRows) {
468-
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
469-
Message: "invalid email or password",
470-
})
471-
return
472-
}
473-
if err != nil {
468+
if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
474469
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
475470
Message: fmt.Sprintf("get user: %s", err.Error()),
476471
})
477472
return
478473
}
474+
475+
// If the user doesn't exist, it will be a default struct.
476+
479477
equal, err := userpassword.Compare(string(user.HashedPassword), loginWithPassword.Password)
480478
if err != nil {
481479
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{

0 commit comments

Comments
 (0)