Skip to content

Commit 44c04ec

Browse files
coadlercoadler
committed
fix: user passwords cleanup
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
1 parent 8661f92 commit 44c04ec

File tree

4 files changed

+176
-27
lines changed

4 files changed

+176
-27
lines changed

‎coderd/userpassword/hashing_bench_test.go

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
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(cryptorand.MustString(16))
14+
secret = []byte(cryptorand.MustString(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+
}

‎coderd/userpassword/userpassword.go

Lines changed: 64 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,25 +6,69 @@ 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"
16+
17+
"github.com/coder/coder/cryptorand"
1418
)
1519

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.
20+
var (
21+
// The base64 encoder used when producing the string representation of
22+
// hashes.
23+
base64Encoder = base64.RawStdEncoding
24+
25+
// The number of iterations to use when generating the hash. This was chosen
26+
// to make it about as fast as bcrypt hashes. Increasing this causes hashes
27+
// to take longer to compute.
28+
defaultHashIter = 65535
29+
30+
// This is the length of our output hash. bcrypt has a hash size of up to
31+
// 60, so we rounded up to a power of 8.
1932
hashLength = 64
33+
2034
// The scheme to include in our hashed password.
2135
hashScheme = "pbkdf2-sha256"
36+
37+
// A salt size of 16 is the default in passlib. A minimum of 8 can be safely
38+
// used.
39+
saltSize = 16
40+
41+
// The simulated hash is used when trying to simulate password checks for
42+
// users that don't exist.
43+
simulatedHash, _ = Hash(cryptorand.MustString(10))
2244
)
2345

24-
// Compare checks the equality of passwords from a hashed pbkdf2 string.
25-
// This uses pbkdf2 to ensure FIPS 140-2 compliance. See:
46+
// Make password hashing much faster in tests.
47+
func init() {
48+
args := os.Args[1:]
49+
50+
// Ensure this can never be enabled if running in server mode.
51+
if slices.Contains(args, "server") {
52+
return
53+
}
54+
55+
for _, flag := range args {
56+
if strings.HasPrefix(flag, "-test.") {
57+
defaultHashIter = 1
58+
return
59+
}
60+
}
61+
}
62+
63+
// Compare checks the equality of passwords from a hashed pbkdf2 string. This
64+
// uses pbkdf2 to ensure FIPS 140-2 compliance. See:
2665
// https://csrc.nist.gov/csrc/media/templates/cryptographic-module-validation-program/documents/security-policies/140sp2261.pdf
2766
func Compare(hashed string, password string) (bool, error) {
67+
// If the hased password provided is empty, simulate comparing a real hash.
68+
if hashed == "" {
69+
hashed = simulatedHash
70+
}
71+
2872
if len(hashed) < hashLength {
2973
return false, xerrors.Errorf("hash too short: %d", len(hashed))
3074
}
@@ -42,37 +86,40 @@ func Compare(hashed string, password string) (bool, error) {
4286
if err != nil {
4387
return false, xerrors.Errorf("parse iter from hash: %w", err)
4488
}
45-
salt, err := base64.RawStdEncoding.DecodeString(parts[3])
89+
salt, err := base64Encoder.DecodeString(parts[3])
4690
if err != nil {
4791
return false, xerrors.Errorf("decode salt: %w", err)
4892
}
4993

5094
if subtle.ConstantTimeCompare([]byte(hashWithSaltAndIter(password, salt, iter)), []byte(hashed)) != 1 {
5195
return false, nil
5296
}
97+
5398
return true, nil
5499
}
55100

56101
// Hash generates a hash using pbkdf2.
57102
// See the Compare() comment for rationale.
58103
func Hash(password string) (string, error) {
59-
// bcrypt uses a salt size of 16 bytes.
60-
salt := make([]byte, 16)
104+
salt := make([]byte, saltSize)
61105
_, err := rand.Read(salt)
62106
if err != nil {
63107
return "", xerrors.Errorf("read random bytes for salt: %w", err)
64108
}
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
109+
110+
return hashWithSaltAndIter(password, salt, defaultHashIter), nil
68111
}
69112

70113
// Produces a string representation of the hash.
71114
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)
115+
var (
116+
hash = pbkdf2.Key([]byte(password), salt, iter, hashLength, sha256.New)
117+
encHash = make([]byte, base64Encoder.EncodedLen(len(hash)))
118+
encSalt = make([]byte, base64Encoder.EncodedLen(len(salt)))
119+
)
120+
121+
base64Encoder.Encode(encHash, hash)
122+
base64Encoder.Encode(encSalt, salt)
123+
124+
return fmt.Sprintf("$%s$%d$%s$%s", hashScheme, iter, encSalt, encHash)
78125
}

‎coderd/users.go

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -419,21 +419,19 @@ func (api *api) postLogin(rw http.ResponseWriter, r *http.Request) {
419419
if !httpapi.Read(rw, r, &loginWithPassword) {
420420
return
421421
}
422+
422423
user, err := api.Database.GetUserByEmailOrUsername(r.Context(), database.GetUserByEmailOrUsernameParams{
423424
Email: loginWithPassword.Email,
424425
})
425-
if errors.Is(err, sql.ErrNoRows) {
426-
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
427-
Message: "invalid email or password",
428-
})
429-
return
430-
}
431-
if err != nil {
426+
if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
432427
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
433428
Message: fmt.Sprintf("get user: %s", err.Error()),
434429
})
435430
return
436431
}
432+
433+
// If the user doesn't exist, it will be a default struct.
434+
437435
equal, err := userpassword.Compare(string(user.HashedPassword), loginWithPassword.Password)
438436
if err != nil {
439437
httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{

‎cryptorand/strings.go

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import (
44
"crypto/rand"
55
"encoding/binary"
66
"strings"
7+
8+
"golang.org/x/xerrors"
79
)
810

911
// Charsets
@@ -32,7 +34,7 @@ const (
3234
Human = "23456789abcdefghjkmnpqrstuvwxyz"
3335
)
3436

35-
// StringCharset generates a random string using the provided charset and size
37+
// StringCharset generates a random string using the provided charset and size.
3638
func StringCharset(charSetStr string, size int) (string, error) {
3739
charSet := []rune(charSetStr)
3840

@@ -67,18 +69,58 @@ func StringCharset(charSetStr string, size int) (string, error) {
6769
return buf.String(), nil
6870
}
6971

72+
// MustStringCharset generates a random string of the given length, using the
73+
// provided charset. It will panic if an error occurs.
74+
func MustStringCharset(charSet string, size int) string {
75+
s, err := StringCharset(charSet, size)
76+
must(err)
77+
return s
78+
}
79+
7080
// String returns a random string using Default.
7181
func String(size int) (string, error) {
7282
return StringCharset(Default, size)
7383
}
7484

85+
// MustString generates a random string of the given length, using
86+
// the Default charset. It will panic if an error occurs.
87+
func MustString(size int) string {
88+
s, err := String(size)
89+
must(err)
90+
return s
91+
}
92+
7593
// HexString returns a hexadecimal string of given length.
7694
func HexString(size int) (string, error) {
7795
return StringCharset(Hex, size)
7896
}
7997

80-
// Sha1String returns a 40-character hexadecimal string, which matches
81-
// the length of a SHA-1 hash (160 bits).
98+
// MustHexString generates a random hexadecimal string of the given
99+
// length. It will panic if an error occurs.
100+
func MustHexString(size int) string {
101+
s, err := HexString(size)
102+
must(err)
103+
return s
104+
}
105+
106+
// Sha1String returns a 40-character hexadecimal string, which matches the
107+
// length of a SHA-1 hash (160 bits).
82108
func Sha1String() (string, error) {
83109
return StringCharset(Hex, 40)
84110
}
111+
112+
// MustSha1String returns a 40-character hexadecimal string, which matches the
113+
// length of a SHA-1 hash (160 bits). It will panic if an error occurs.
114+
func MustSha1String() string {
115+
s, err := Sha1String()
116+
must(err)
117+
return s
118+
}
119+
120+
// must is a utility function that panics with the given error if
121+
// err is non-nil.
122+
func must(err error) {
123+
if err != nil {
124+
panic(xerrors.Errorf("crand: %w", err))
125+
}
126+
}

0 commit comments

Comments
 (0)