From 44c04ecd1ccfc68441dce83437fa4336776799e3 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Wed, 27 Apr 2022 16:37:12 -0500 Subject: [PATCH 1/3] 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 --- coderd/userpassword/hashing_bench_test.go | 62 +++++++++++++++++ coderd/userpassword/userpassword.go | 81 ++++++++++++++++++----- coderd/users.go | 12 ++-- cryptorand/strings.go | 48 +++++++++++++- 4 files changed, 176 insertions(+), 27 deletions(-) create mode 100644 coderd/userpassword/hashing_bench_test.go diff --git a/coderd/userpassword/hashing_bench_test.go b/coderd/userpassword/hashing_bench_test.go new file mode 100644 index 0000000000000..2f4da9af2b6f0 --- /dev/null +++ b/coderd/userpassword/hashing_bench_test.go @@ -0,0 +1,62 @@ +package userpassword_test + +import ( + "crypto/sha256" + "testing" + + "github.com/coder/coder/cryptorand" + "golang.org/x/crypto/bcrypt" + "golang.org/x/crypto/pbkdf2" +) + +var ( + salt = []byte(cryptorand.MustString(16)) + secret = []byte(cryptorand.MustString(24)) + + resBcrypt []byte + resPbkdf2 []byte +) + +func BenchmarkBcryptMinCost(b *testing.B) { + var r []byte + b.ReportAllocs() + + for i := 0; i < b.N; i++ { + r, _ = bcrypt.GenerateFromPassword(secret, bcrypt.MinCost) + } + + resBcrypt = r +} + +func BenchmarkPbkdf2MinCost(b *testing.B) { + var r []byte + b.ReportAllocs() + + for i := 0; i < b.N; i++ { + r = pbkdf2.Key(secret, salt, 1024, 64, sha256.New) + } + + resPbkdf2 = r +} + +func BenchmarkBcryptDefaultCost(b *testing.B) { + var r []byte + b.ReportAllocs() + + for i := 0; i < b.N; i++ { + r, _ = bcrypt.GenerateFromPassword(secret, bcrypt.DefaultCost) + } + + resBcrypt = r +} + +func BenchmarkPbkdf2(b *testing.B) { + var r []byte + b.ReportAllocs() + + for i := 0; i < b.N; i++ { + r = pbkdf2.Key(secret, salt, 65536, 64, sha256.New) + } + + resPbkdf2 = r +} diff --git a/coderd/userpassword/userpassword.go b/coderd/userpassword/userpassword.go index 44ec3b16be994..d734668e09fea 100644 --- a/coderd/userpassword/userpassword.go +++ b/coderd/userpassword/userpassword.go @@ -6,25 +6,69 @@ import ( "crypto/subtle" "encoding/base64" "fmt" + "os" "strconv" "strings" "golang.org/x/crypto/pbkdf2" + "golang.org/x/exp/slices" "golang.org/x/xerrors" + + "github.com/coder/coder/cryptorand" ) -const ( - // This is the length of our output hash. - // bcrypt has a hash size of 59, so we rounded up to a power of 8. +var ( + // The base64 encoder used when producing the string representation of + // hashes. + base64Encoder = base64.RawStdEncoding + + // The number of iterations to use when generating the hash. This was chosen + // to make it about as fast as bcrypt hashes. Increasing this causes hashes + // to take longer to compute. + defaultHashIter = 65535 + + // This is the length of our output hash. bcrypt has a hash size of up to + // 60, so we rounded up to a power of 8. hashLength = 64 + // The scheme to include in our hashed password. hashScheme = "pbkdf2-sha256" + + // A salt size of 16 is the default in passlib. A minimum of 8 can be safely + // used. + saltSize = 16 + + // The simulated hash is used when trying to simulate password checks for + // users that don't exist. + simulatedHash, _ = Hash(cryptorand.MustString(10)) ) -// Compare checks the equality of passwords from a hashed pbkdf2 string. -// This uses pbkdf2 to ensure FIPS 140-2 compliance. See: +// Make password hashing much faster in tests. +func init() { + args := os.Args[1:] + + // Ensure this can never be enabled if running in server mode. + if slices.Contains(args, "server") { + return + } + + for _, flag := range args { + if strings.HasPrefix(flag, "-test.") { + defaultHashIter = 1 + return + } + } +} + +// Compare checks the equality of passwords from a hashed pbkdf2 string. This +// uses pbkdf2 to ensure FIPS 140-2 compliance. See: // 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. + if hashed == "" { + hashed = simulatedHash + } + if len(hashed) < hashLength { return false, xerrors.Errorf("hash too short: %d", len(hashed)) } @@ -42,7 +86,7 @@ func Compare(hashed string, password string) (bool, error) { if err != nil { return false, xerrors.Errorf("parse iter from hash: %w", err) } - salt, err := base64.RawStdEncoding.DecodeString(parts[3]) + salt, err := base64Encoder.DecodeString(parts[3]) if err != nil { return false, xerrors.Errorf("decode salt: %w", err) } @@ -50,29 +94,32 @@ func Compare(hashed string, password string) (bool, error) { if subtle.ConstantTimeCompare([]byte(hashWithSaltAndIter(password, salt, iter)), []byte(hashed)) != 1 { return false, nil } + return true, nil } // Hash generates a hash using pbkdf2. // See the Compare() comment for rationale. func Hash(password string) (string, error) { - // bcrypt uses a salt size of 16 bytes. - salt := make([]byte, 16) + salt := make([]byte, saltSize) _, err := rand.Read(salt) if err != nil { return "", xerrors.Errorf("read random bytes for salt: %w", err) } - // The default hash iteration is 1024 for speed. - // As this is increased, the password is hashed more. - return hashWithSaltAndIter(password, salt, 1024), nil + + return hashWithSaltAndIter(password, salt, defaultHashIter), nil } // Produces a string representation of the hash. func hashWithSaltAndIter(password string, salt []byte, iter int) string { - hash := pbkdf2.Key([]byte(password), salt, iter, hashLength, sha256.New) - hash = []byte(base64.RawStdEncoding.EncodeToString(hash)) - salt = []byte(base64.RawStdEncoding.EncodeToString(salt)) - // This format is similar to bcrypt. See: - // https://en.wikipedia.org/wiki/Bcrypt#Description - return fmt.Sprintf("$%s$%d$%s$%s", hashScheme, iter, salt, hash) + var ( + hash = pbkdf2.Key([]byte(password), salt, iter, hashLength, sha256.New) + encHash = make([]byte, base64Encoder.EncodedLen(len(hash))) + encSalt = make([]byte, base64Encoder.EncodedLen(len(salt))) + ) + + base64Encoder.Encode(encHash, hash) + base64Encoder.Encode(encSalt, salt) + + return fmt.Sprintf("$%s$%d$%s$%s", hashScheme, iter, encSalt, encHash) } diff --git a/coderd/users.go b/coderd/users.go index a38af8ba12f63..ca8517aa8a6cc 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -419,21 +419,19 @@ func (api *api) postLogin(rw http.ResponseWriter, r *http.Request) { if !httpapi.Read(rw, r, &loginWithPassword) { return } + user, err := api.Database.GetUserByEmailOrUsername(r.Context(), database.GetUserByEmailOrUsernameParams{ Email: loginWithPassword.Email, }) - if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: "invalid email or password", - }) - return - } - if err != nil { + if err != nil && !xerrors.Is(err, sql.ErrNoRows) { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: fmt.Sprintf("get user: %s", err.Error()), }) return } + + // If the user doesn't exist, it will be a default struct. + equal, err := userpassword.Compare(string(user.HashedPassword), loginWithPassword.Password) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ diff --git a/cryptorand/strings.go b/cryptorand/strings.go index fd16ba0af7e0e..9a0cd1f3d12dc 100644 --- a/cryptorand/strings.go +++ b/cryptorand/strings.go @@ -4,6 +4,8 @@ import ( "crypto/rand" "encoding/binary" "strings" + + "golang.org/x/xerrors" ) // Charsets @@ -32,7 +34,7 @@ const ( Human = "23456789abcdefghjkmnpqrstuvwxyz" ) -// StringCharset generates a random string using the provided charset and size +// StringCharset generates a random string using the provided charset and size. func StringCharset(charSetStr string, size int) (string, error) { charSet := []rune(charSetStr) @@ -67,18 +69,58 @@ func StringCharset(charSetStr string, size int) (string, error) { return buf.String(), nil } +// MustStringCharset generates a random string of the given length, using the +// provided charset. It will panic if an error occurs. +func MustStringCharset(charSet string, size int) string { + s, err := StringCharset(charSet, size) + must(err) + return s +} + // String returns a random string using Default. func String(size int) (string, error) { return StringCharset(Default, size) } +// MustString generates a random string of the given length, using +// the Default charset. It will panic if an error occurs. +func MustString(size int) string { + s, err := String(size) + must(err) + return s +} + // HexString returns a hexadecimal string of given length. func HexString(size int) (string, error) { return StringCharset(Hex, size) } -// Sha1String returns a 40-character hexadecimal string, which matches -// the length of a SHA-1 hash (160 bits). +// MustHexString generates a random hexadecimal string of the given +// length. It will panic if an error occurs. +func MustHexString(size int) string { + s, err := HexString(size) + must(err) + return s +} + +// Sha1String returns a 40-character hexadecimal string, which matches the +// length of a SHA-1 hash (160 bits). func Sha1String() (string, error) { return StringCharset(Hex, 40) } + +// MustSha1String returns a 40-character hexadecimal string, which matches the +// length of a SHA-1 hash (160 bits). It will panic if an error occurs. +func MustSha1String() string { + s, err := Sha1String() + must(err) + return s +} + +// must is a utility function that panics with the given error if +// err is non-nil. +func must(err error) { + if err != nil { + panic(xerrors.Errorf("crand: %w", err)) + } +} From 458c5dc52a401c02ec7e4742d5b76aa55862d4a5 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Thu, 28 Apr 2022 12:56:17 -0500 Subject: [PATCH 2/3] fixup! fix: user passwords cleanup --- coderd/userpassword/hashing_bench_test.go | 12 +++++- coderd/userpassword/userpassword.go | 4 +- cryptorand/strings.go | 48 ++--------------------- 3 files changed, 14 insertions(+), 50 deletions(-) diff --git a/coderd/userpassword/hashing_bench_test.go b/coderd/userpassword/hashing_bench_test.go index 2f4da9af2b6f0..d2f1c8ae3cebe 100644 --- a/coderd/userpassword/hashing_bench_test.go +++ b/coderd/userpassword/hashing_bench_test.go @@ -10,8 +10,8 @@ import ( ) var ( - salt = []byte(cryptorand.MustString(16)) - secret = []byte(cryptorand.MustString(24)) + salt = []byte(must(cryptorand.String(16))) + secret = []byte(must(cryptorand.String(24))) resBcrypt []byte resPbkdf2 []byte @@ -60,3 +60,11 @@ func BenchmarkPbkdf2(b *testing.B) { resPbkdf2 = r } + +func must(s string, err error) string { + if err != nil { + panic(err) + } + + return s +} diff --git a/coderd/userpassword/userpassword.go b/coderd/userpassword/userpassword.go index d734668e09fea..3afd7d6e6a23c 100644 --- a/coderd/userpassword/userpassword.go +++ b/coderd/userpassword/userpassword.go @@ -13,8 +13,6 @@ import ( "golang.org/x/crypto/pbkdf2" "golang.org/x/exp/slices" "golang.org/x/xerrors" - - "github.com/coder/coder/cryptorand" ) var ( @@ -40,7 +38,7 @@ var ( // The simulated hash is used when trying to simulate password checks for // users that don't exist. - simulatedHash, _ = Hash(cryptorand.MustString(10)) + simulatedHash, _ = Hash("hunter2") ) // Make password hashing much faster in tests. diff --git a/cryptorand/strings.go b/cryptorand/strings.go index 9a0cd1f3d12dc..fd16ba0af7e0e 100644 --- a/cryptorand/strings.go +++ b/cryptorand/strings.go @@ -4,8 +4,6 @@ import ( "crypto/rand" "encoding/binary" "strings" - - "golang.org/x/xerrors" ) // Charsets @@ -34,7 +32,7 @@ const ( Human = "23456789abcdefghjkmnpqrstuvwxyz" ) -// StringCharset generates a random string using the provided charset and size. +// StringCharset generates a random string using the provided charset and size func StringCharset(charSetStr string, size int) (string, error) { charSet := []rune(charSetStr) @@ -69,58 +67,18 @@ func StringCharset(charSetStr string, size int) (string, error) { return buf.String(), nil } -// MustStringCharset generates a random string of the given length, using the -// provided charset. It will panic if an error occurs. -func MustStringCharset(charSet string, size int) string { - s, err := StringCharset(charSet, size) - must(err) - return s -} - // String returns a random string using Default. func String(size int) (string, error) { return StringCharset(Default, size) } -// MustString generates a random string of the given length, using -// the Default charset. It will panic if an error occurs. -func MustString(size int) string { - s, err := String(size) - must(err) - return s -} - // HexString returns a hexadecimal string of given length. func HexString(size int) (string, error) { return StringCharset(Hex, size) } -// MustHexString generates a random hexadecimal string of the given -// length. It will panic if an error occurs. -func MustHexString(size int) string { - s, err := HexString(size) - must(err) - return s -} - -// Sha1String returns a 40-character hexadecimal string, which matches the -// length of a SHA-1 hash (160 bits). +// Sha1String returns a 40-character hexadecimal string, which matches +// the length of a SHA-1 hash (160 bits). func Sha1String() (string, error) { return StringCharset(Hex, 40) } - -// MustSha1String returns a 40-character hexadecimal string, which matches the -// length of a SHA-1 hash (160 bits). It will panic if an error occurs. -func MustSha1String() string { - s, err := Sha1String() - must(err) - return s -} - -// must is a utility function that panics with the given error if -// err is non-nil. -func must(err error) { - if err != nil { - panic(xerrors.Errorf("crand: %w", err)) - } -} From 449947f8c46c303bc623b5f6e3c3fc945ccfafb4 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Thu, 28 Apr 2022 13:13:50 -0500 Subject: [PATCH 3/3] fixup! fix: user passwords cleanup --- coderd/userpassword/userpassword.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/coderd/userpassword/userpassword.go b/coderd/userpassword/userpassword.go index 3afd7d6e6a23c..e2c9d41a7763d 100644 --- a/coderd/userpassword/userpassword.go +++ b/coderd/userpassword/userpassword.go @@ -18,7 +18,7 @@ import ( var ( // The base64 encoder used when producing the string representation of // hashes. - base64Encoder = base64.RawStdEncoding + base64Encoding = base64.RawStdEncoding // The number of iterations to use when generating the hash. This was chosen // to make it about as fast as bcrypt hashes. Increasing this causes hashes @@ -34,7 +34,7 @@ var ( // A salt size of 16 is the default in passlib. A minimum of 8 can be safely // used. - saltSize = 16 + defaultSaltSize = 16 // The simulated hash is used when trying to simulate password checks for // users that don't exist. @@ -84,7 +84,7 @@ func Compare(hashed string, password string) (bool, error) { if err != nil { return false, xerrors.Errorf("parse iter from hash: %w", err) } - salt, err := base64Encoder.DecodeString(parts[3]) + salt, err := base64Encoding.DecodeString(parts[3]) if err != nil { return false, xerrors.Errorf("decode salt: %w", err) } @@ -99,7 +99,7 @@ func Compare(hashed string, password string) (bool, error) { // Hash generates a hash using pbkdf2. // See the Compare() comment for rationale. func Hash(password string) (string, error) { - salt := make([]byte, saltSize) + salt := make([]byte, defaultSaltSize) _, err := rand.Read(salt) if err != nil { return "", xerrors.Errorf("read random bytes for salt: %w", err) @@ -112,12 +112,12 @@ func Hash(password string) (string, error) { func hashWithSaltAndIter(password string, salt []byte, iter int) string { var ( hash = pbkdf2.Key([]byte(password), salt, iter, hashLength, sha256.New) - encHash = make([]byte, base64Encoder.EncodedLen(len(hash))) - encSalt = make([]byte, base64Encoder.EncodedLen(len(salt))) + encHash = make([]byte, base64Encoding.EncodedLen(len(hash))) + encSalt = make([]byte, base64Encoding.EncodedLen(len(salt))) ) - base64Encoder.Encode(encHash, hash) - base64Encoder.Encode(encSalt, salt) + base64Encoding.Encode(encHash, hash) + base64Encoding.Encode(encSalt, salt) return fmt.Sprintf("$%s$%d$%s$%s", hashScheme, iter, encSalt, encHash) }