Skip to content

Don't store plaintext passwords #1040

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions client/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,9 @@ func (c *Conn) genAuthResponse(authData []byte) ([]byte, bool, error) {
// password hashing
switch c.authPluginName {
case mysql.AUTH_NATIVE_PASSWORD:
return mysql.CalcPassword(authData[:20], []byte(c.password)), false, nil
return mysql.CalcNativePassword(authData[:20], []byte(c.password)), false, nil
case mysql.AUTH_CACHING_SHA2_PASSWORD:
return mysql.CalcCachingSha2Password(authData, c.password), false, nil
return mysql.CalcCachingSha2Password(authData, []byte(c.password)), false, nil
case mysql.AUTH_CLEAR_PASSWORD:
return []byte(c.password), true, nil
case mysql.AUTH_SHA256_PASSWORD:
Expand Down
176 changes: 164 additions & 12 deletions mysql/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ import (
"crypto/sha1"
"crypto/sha256"
"crypto/sha512"
"crypto/subtle"
"encoding/binary"
"encoding/hex"
"fmt"
"io"
mrand "math/rand"
Expand All @@ -29,7 +31,7 @@ func Pstack() string {
return string(buf[0:n])
}

func CalcPassword(scramble, password []byte) []byte {
func CalcNativePassword(scramble, password []byte) []byte {
if len(password) == 0 {
return nil
}
Expand All @@ -39,35 +41,100 @@ func CalcPassword(scramble, password []byte) []byte {
crypt.Write(password)
stage1 := crypt.Sum(nil)

// scrambleHash = SHA1(scramble + SHA1(stage1Hash))
// inner Hash
// stage2Hash = SHA1(stage1Hash)
crypt.Reset()
crypt.Write(stage1)
hash := crypt.Sum(nil)
stage2 := crypt.Sum(nil)

// outer Hash
// scrambleHash = SHA1(scramble + stage2Hash)
crypt.Reset()
crypt.Write(scramble)
crypt.Write(hash)
scramble = crypt.Sum(nil)
crypt.Write(stage2)
scrambleHash := crypt.Sum(nil)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: seems stage2 is not used anymore, so we can pass-in stage2[:0] to reuse the array under the slice.


// token = scrambleHash XOR stage1Hash
for i := range scramble {
scramble[i] ^= stage1[i]
return Xor(scrambleHash, stage1)
}

func Xor(hash1 []byte, hash2 []byte) []byte {
for i := range hash1 {
hash1[i] ^= hash2[i]
}
return scramble
return hash1
}

// hash_stage1 = xor(reply, sha1(public_seed, hash_stage2))
func Stage1FromReply(scramble []byte, seed []byte, stage2 []byte) []byte {
crypt := sha1.New()
crypt.Write(seed)
crypt.Write(stage2)
seededHash := crypt.Sum(nil)

return Xor(scramble, seededHash)
}

// FROM vitess.io/vitess/go/mysql/auth_server.go
// DecodePasswordHex decodes the standard format used by MySQL
// for 4.1 style password hashes. It drops the optionally leading * before
// decoding the rest as a hex encoded string.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// FROM vitess.io/vitess/go/mysql/auth_server.go
// DecodePasswordHex decodes the standard format used by MySQL
// for 4.1 style password hashes. It drops the optionally leading * before
// decoding the rest as a hex encoded string.
// DecodePasswordHex decodes the standard format used by MySQL
// for 4.1 style password hashes. It drops the optionally leading * before
// decoding the rest as a hex encoded string.
// the implementation is also learned from vitess.io/vitess/go/mysql/auth_server.go

Golang's coding style suggests the function name DecodePasswordHex being put at the beginning.

func DecodePasswordHex(hexEncodedPassword string) ([]byte, error) {
if hexEncodedPassword[0] == '*' {
hexEncodedPassword = hexEncodedPassword[1:]
}
return hex.DecodeString(hexEncodedPassword)
}

// EncodePasswordHex encodes to the standard format used by MySQL
// adds the optionally leading * to the hashed password
func EncodePasswordHex(passwordHash []byte) string {
hexstr := strings.ToUpper(hex.EncodeToString(passwordHash))
return "*" + hexstr
}

// NativePasswordHash = sha1(sha1(password))
func NativePasswordHash(password []byte) []byte {
if len(password) == 0 {
return nil
}

// stage1Hash = SHA1(password)
crypt := sha1.New()
crypt.Write(password)
stage1 := crypt.Sum(nil)

// stage2Hash = SHA1(stage1Hash)
crypt.Reset()
crypt.Write(stage1)
return crypt.Sum(nil)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return crypt.Sum(nil)
return crypt.Sum(stage1[:0])

The argument nil will let crypt.Sum allocate a new array of the returned slice. I think using stage1[:0] will reuse the underlying array of stage1 to avoid that allocation.

}

func CompareNativePassword(reply []byte, stored []byte, seed []byte) bool {
if len(stored) == 0 {
return false
}

// hash_stage1 = xor(reply, sha1(public_seed, hash_stage2))
stage1 := Stage1FromReply(reply, seed, stored)
// andidate_hash2 = sha1(hash_stage1)
crypt := sha1.New()
crypt.Write(stage1)
stage2 := crypt.Sum(nil)

// check(candidate_hash2 == hash_stage2)
// use ConstantTimeCompare to mitigate timing based attacks
return subtle.ConstantTimeCompare(stage2, stored) == 1
Comment on lines +120 to +126
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
crypt := sha1.New()
crypt.Write(stage1)
stage2 := crypt.Sum(nil)
// check(candidate_hash2 == hash_stage2)
// use ConstantTimeCompare to mitigate timing based attacks
return subtle.ConstantTimeCompare(stage2, stored) == 1
stage2 := sha1.Sum(stage1)
// check(candidate_hash2 == hash_stage2)
// use ConstantTimeCompare to mitigate timing based attacks
return subtle.ConstantTimeCompare(stage2[:], stored) == 1

we can reuse the builtin functions.

}

// CalcCachingSha2Password: Hash password using MySQL 8+ method (SHA256)
func CalcCachingSha2Password(scramble []byte, password string) []byte {
func CalcCachingSha2Password(scramble []byte, password []byte) []byte {
if len(password) == 0 {
return nil
}

// XOR(SHA256(password), SHA256(SHA256(SHA256(password)), scramble))

crypt := sha256.New()
crypt.Write([]byte(password))
crypt.Write(password)
message1 := crypt.Sum(nil)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see that you just mimic the old code, so my comments like crypt.Sum(stage1[:0]) or using sha1.Sum should be applied to the old codes as well. This is a bit irrelevant to your PR, so you can ignore these comments and I'll try to fix them in another PR.


crypt.Reset()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 149 can also use your new Xor function

Expand Down Expand Up @@ -135,6 +202,91 @@ func EncryptPassword(password string, seed []byte, pub *rsa.PublicKey) ([]byte,
return rsa.EncryptOAEP(sha1v, rand.Reader, pub, plain, nil)
}

const (
SALT_LENGTH = 16
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: My IDE says "Use camel case instead of snake case". However this project has both style so you can ignore it.

ITERATION_MULTIPLIER = 1000
SHA256_PASSWORD_ITERATIONS = 5
)

// generateUserSalt generate salt of given length for sha256_password hash
func generateUserSalt(length int) ([]byte, error) {
// Generate a random salt of the given length
// Implement this function for your project
salt := make([]byte, length)
_, err := rand.Read(salt)
if err != nil {
return []byte(""), err
}

// Restrict to 7-bit to avoid multi-byte UTF-8
for i := range salt {
salt[i] = salt[i] &^ 128
for salt[i] == 36 || salt[i] == 0 { // '$' or NUL
newval := make([]byte, 1)
_, err := rand.Read(newval)
if err != nil {
return []byte(""), err
}
salt[i] = newval[0] &^ 128
}
}
return salt, nil
}

// hashCrypt256 salt and hash a password the given number of iterations
func hashCrypt256(source, salt string, iterations uint64) (string, error) {
actualIterations := iterations * ITERATION_MULTIPLIER
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this function, the const ITERATION_MULTIPLIER is directly used. But for generateUserSalt SALT_LENGTH is pass-in as an argument. I think generateUserSalt can also directly use SALT_LENGTH

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to match mysql server salt generation, but I'm wondering if it's used there to generate both the salt for the stored password hashes and for scramble generation. I see the scramble generation here is using something similar but not quite the same, perhaps the two can/should be combined?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get your point clearly.

perhaps the two can/should be combined?

Do you mean the "scramble generation" related functions are similar to "salt generation" functions, so you want to merge them? Can you give a simple explanation about which functions are affected?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was looking at RandomBuf in mysql/util.go:319 which I believe may be better suited to use crypto/rand. I think that mysql is avoiding the multi-byte UTF-8 to simplify client/server salt exchanges so it either makes sense to remove that in the salt generation or combine the two functions here and generate salts that suit both purposes.

hashInput := []byte(source + salt)
var hash [32]byte
for i := uint64(0); i < actualIterations; i++ {
h := sha256.New()
h.Write(hashInput)
hash = sha256.Sum256(h.Sum(nil))
hashInput = hash[:]
}

hashHex := hex.EncodeToString(hash[:])
digest := fmt.Sprintf("$%d$%s$%s", iterations, salt, hashHex)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In generateUserSalt I see some restrictions are added to the random bytes. Is this line the reason of it? I think it's OK to remove the restrictions to make the whole logic more simple. We can hex-encode the salt without the restrictions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might be right, I was trying to recreate what was being done in mysql-server, but it may not be necessary here

return digest, nil
}

// Check256HashingPassword compares a password to a hash for sha256_password
// rather than trying to recreate just the hash we recreate the full hash
// and use that for comparison
func Check256HashingPassword(pwhash []byte, password string) (bool, error) {
pwHashParts := bytes.Split(pwhash, []byte("$"))
if len(pwHashParts) != 4 {
return false, errors.New("failed to decode hash parts")
}

iterationsPart := pwHashParts[1]
if len(iterationsPart) == 0 {
return false, errors.New("iterations part is empty")
}

iterations, err := strconv.ParseUint(string(iterationsPart), 10, 64)
if err != nil {
return false, errors.New("failed to decode iterations")
}
salt := pwHashParts[2][:SALT_LENGTH]

newHash, err := hashCrypt256(password, string(salt), iterations)
if err != nil {
return false, err
}

return bytes.Equal(pwhash, []byte(newHash)), nil
}

// NewSha256PasswordHash creates a new password hash for sha256_password
func NewSha256PasswordHash(pwd string) (string, error) {
salt, err := generateUserSalt(SALT_LENGTH)
if err != nil {
return "", err
}
return hashCrypt256(pwd, string(salt), SHA256_PASSWORD_ITERATIONS)
}

func DecompressMariadbData(data []byte) ([]byte, error) {
// algorithm always 0=zlib
// algorithm := (data[pos] & 0x07) >> 4
Expand Down
Loading
Loading