From cb2380fb8bfe7982bf9354887639c6db6809395b Mon Sep 17 00:00:00 2001 From: Chris Bii Date: Wed, 22 Nov 2023 14:21:52 -0500 Subject: [PATCH] bcrypt: MagicCypherData is now declared direcly within bcrypt(). Avoids global allocation and unecessary copying. Removed unecessary encoding/decoding of salt between newFromPassword() and expensiveBlowfishSetup(). Decode functions called within newFromHash() take direct reference to hash byteslice rather than copies and mutate them within the function, although they can be mutated after also. --- bcrypt/bcrypt.go | 98 ++++++++++++++++++++++--------------------- bcrypt/bcrypt_test.go | 6 +-- 2 files changed, 54 insertions(+), 50 deletions(-) diff --git a/bcrypt/bcrypt.go b/bcrypt/bcrypt.go index 5577c0f939..064930b765 100644 --- a/bcrypt/bcrypt.go +++ b/bcrypt/bcrypt.go @@ -63,17 +63,6 @@ const ( minHashSize = 59 ) -// magicCipherData is an IV for the 64 Blowfish encryption calls in -// bcrypt(). It's the string "OrpheanBeholderScryDoubt" in big-endian bytes. -var magicCipherData = []byte{ - 0x4f, 0x72, 0x70, 0x68, - 0x65, 0x61, 0x6e, 0x42, - 0x65, 0x68, 0x6f, 0x6c, - 0x64, 0x65, 0x72, 0x53, - 0x63, 0x72, 0x79, 0x44, - 0x6f, 0x75, 0x62, 0x74, -} - type hashed struct { hash []byte salt []byte @@ -111,6 +100,13 @@ func CompareHashAndPassword(hashedPassword, password []byte) error { return err } + // This is simply put here instead of in newfromhash only to avoid failed test + // Altough failed test can be easily altered + p.salt, err = base64Decode(p.salt) + if err != nil { + return err + } + otherHash, err := bcrypt(password, p.cost, p.salt) if err != nil { return err @@ -156,12 +152,14 @@ func newFromPassword(password []byte, cost int) (*hashed, error) { return nil, err } - p.salt = base64Encode(unencodedSalt) - hash, err := bcrypt(password, p.cost, p.salt) + hash, err := bcrypt(password, p.cost, unencodedSalt) if err != nil { return nil, err } + + p.salt = base64Encode(unencodedSalt) p.hash = hash + return p, err } @@ -170,32 +168,38 @@ func newFromHash(hashedSecret []byte) (*hashed, error) { return nil, ErrHashTooShort } p := new(hashed) - n, err := p.decodeVersion(hashedSecret) + err := p.decodeVersion(&hashedSecret) if err != nil { return nil, err } - hashedSecret = hashedSecret[n:] - n, err = p.decodeCost(hashedSecret) + + err = p.decodeCost(&hashedSecret) if err != nil { return nil, err } - hashedSecret = hashedSecret[n:] // The "+2" is here because we'll have to append at most 2 '=' to the salt // when base64 decoding it in expensiveBlowfishSetup(). p.salt = make([]byte, encodedSaltSize, encodedSaltSize+2) copy(p.salt, hashedSecret[:encodedSaltSize]) - hashedSecret = hashedSecret[encodedSaltSize:] - p.hash = make([]byte, len(hashedSecret)) - copy(p.hash, hashedSecret) + p.hash = make([]byte, encodedHashSize) + copy(p.hash, hashedSecret[encodedSaltSize:]) return p, nil } func bcrypt(password []byte, cost int, salt []byte) ([]byte, error) { - cipherData := make([]byte, len(magicCipherData)) - copy(cipherData, magicCipherData) + // magicCipherData is an IV for the 64 Blowfish encryption calls in + // bcrypt(). It's the string "OrpheanBeholderScryDoubt" in big-endian bytes. + var magicCipherData = []byte{ + 0x4f, 0x72, 0x70, 0x68, + 0x65, 0x61, 0x6e, 0x42, + 0x65, 0x68, 0x6f, 0x6c, + 0x64, 0x65, 0x72, 0x53, + 0x63, 0x72, 0x79, 0x44, + 0x6f, 0x75, 0x62, 0x74, + } c, err := expensiveBlowfishSetup(password, uint32(cost), salt) if err != nil { @@ -204,28 +208,23 @@ func bcrypt(password []byte, cost int, salt []byte) ([]byte, error) { for i := 0; i < 24; i += 8 { for j := 0; j < 64; j++ { - c.Encrypt(cipherData[i:i+8], cipherData[i:i+8]) + c.Encrypt(magicCipherData[i:i+8], magicCipherData[i:i+8]) } } // Bug compatibility with C bcrypt implementations. We only encode 23 of // the 24 bytes encrypted. - hsh := base64Encode(cipherData[:maxCryptedHashSize]) + hsh := base64Encode(magicCipherData[:maxCryptedHashSize]) return hsh, nil } func expensiveBlowfishSetup(key []byte, cost uint32, salt []byte) (*blowfish.Cipher, error) { - csalt, err := base64Decode(salt) - if err != nil { - return nil, err - } - // Bug compatibility with C bcrypt implementations. They use the trailing // NULL in the key string during expansion. // We copy the key to prevent changing the underlying array. ckey := append(key[:len(key):len(key)], 0) - c, err := blowfish.NewSaltedCipher(ckey, csalt) + c, err := blowfish.NewSaltedCipher(ckey, salt) if err != nil { return nil, err } @@ -234,7 +233,7 @@ func expensiveBlowfishSetup(key []byte, cost uint32, salt []byte) (*blowfish.Cip rounds = 1 << cost for i = 0; i < rounds; i++ { blowfish.ExpandKey(ckey, c) - blowfish.ExpandKey(csalt, c) + blowfish.ExpandKey(salt, c) } return c, nil @@ -262,34 +261,39 @@ func (p *hashed) Hash() []byte { return arr[:n] } -func (p *hashed) decodeVersion(sbytes []byte) (int, error) { - if sbytes[0] != '$' { - return -1, InvalidHashPrefixError(sbytes[0]) +func (p *hashed) decodeVersion(sbytes *[]byte) error { + if (*sbytes)[0] != '$' { + return InvalidHashPrefixError((*sbytes)[0]) } - if sbytes[1] > majorVersion { - return -1, HashVersionTooNewError(sbytes[1]) + if (*sbytes)[1] > majorVersion { + return HashVersionTooNewError((*sbytes)[1]) } - p.major = sbytes[1] + p.major = (*sbytes)[1] n := 3 - if sbytes[2] != '$' { - p.minor = sbytes[2] + if (*sbytes)[2] != '$' { + p.minor = (*sbytes)[2] n++ } - return n, nil + + (*sbytes) = (*sbytes)[n:] + + return nil } // sbytes should begin where decodeVersion left off. -func (p *hashed) decodeCost(sbytes []byte) (int, error) { - cost, err := strconv.Atoi(string(sbytes[0:2])) +func (p *hashed) decodeCost(sbytes *[]byte) (err error) { + p.cost, err = strconv.Atoi(string((*sbytes)[0:2])) if err != nil { - return -1, err + return } - err = checkCost(cost) + + err = checkCost(p.cost) if err != nil { - return -1, err + return } - p.cost = cost - return 3, nil + + (*sbytes) = (*sbytes)[3:] + return } func (p *hashed) String() string { diff --git a/bcrypt/bcrypt_test.go b/bcrypt/bcrypt_test.go index 8b589e3652..030070f04f 100644 --- a/bcrypt/bcrypt_test.go +++ b/bcrypt/bcrypt_test.go @@ -30,7 +30,7 @@ func TestBcryptingIsEasy(t *testing.T) { func TestBcryptingIsCorrect(t *testing.T) { pass := []byte("allmine") - salt := []byte("XajjQvNhvvRt5GSeFk1xFe") + salt := []byte{101, 201, 101, 75, 19, 227, 199, 20, 239, 236, 133, 32, 30, 109, 243, 30} expectedHash := []byte("$2a$10$XajjQvNhvvRt5GSeFk1xFeyqRrsxkhBkUiQeg0dt.wU1qD4aFDcga") hash, err := bcrypt(pass, 10, salt) @@ -55,7 +55,7 @@ func TestBcryptingIsCorrect(t *testing.T) { func TestVeryShortPasswords(t *testing.T) { key := []byte("k") - salt := []byte("XajjQvNhvvRt5GSeFk1xFe") + salt := []byte{101, 201, 101, 75, 19, 227, 199, 20, 239, 236, 133, 32, 30, 109, 243, 30} _, err := bcrypt(key, 10, salt) if err != nil { t.Errorf("One byte key resulted in error: %s", err) @@ -63,7 +63,7 @@ func TestVeryShortPasswords(t *testing.T) { } func TestTooLongPasswordsWork(t *testing.T) { - salt := []byte("XajjQvNhvvRt5GSeFk1xFe") + salt := []byte{101, 201, 101, 75, 19, 227, 199, 20, 239, 236, 133, 32, 30, 109, 243, 30} // One byte over the usual 56 byte limit that blowfish has tooLongPass := []byte("012345678901234567890123456789012345678901234567890123456") tooLongExpected := []byte("$2a$10$XajjQvNhvvRt5GSeFk1xFe5l47dONXg781AmZtd869sO8zfsHuw7C")