x/crypto/bcrypt: document handling of zero bytes

[moyishizhe]

What version of Go are you using (go version)?

$ go version
go version go1.13 darwin/amd64

Does this issue reproduce with the latest release?

yes.

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/user/Library/Caches/go-build"
GOENV="/Users/user/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/user/.go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/opt/go/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/opt/go/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/p9/jzgqgm3j02148q3nc_1mqc780000gn/T/go-build020284917=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

bcrypt.CompareHashAndPassword is error for this case ( "fxIyAIbpWcmAtQ==", "$2y$10$xMl6FD2r7TEHtm0j0hNSeOp5hA3FNWiJjSe/qbgEwZot/sohdZpBi").

type TestCase struct {
	password         string
	salt             string
	expectedPassword string
}

var tests = []TestCase{
	{
		"11111111", "RDcSaT5tERV3vQ==",
		"$2y$10$zVntO2ZlOr5UiNEb0zoWIOD1mf.kKgtEL8g/U.DavCtlb2EStfSyu",
	},
	{
		"11111111", "fxIyAIbpWcmAtQ==",
		"$2y$10$xMl6FD2r7TEHtm0j0hNSeOp5hA3FNWiJjSe/qbgEwZot/sohdZpBi",
	},
}

func TestPassword(t *testing.T) {
	for _, test := range tests {
		salt1, _ := base64.StdEncoding.DecodeString(test.salt)
		result := pbkdf2.Key([]byte(test.password), salt1, 35000, 32, sha512.New)
		t.Log(result)
		t.Fatal(bcrypt.CompareHashAndPassword([]byte(test.expectedPassword), result))
	}
}

the playground link is https://play.golang.org/p/ZdHeD1h6xW-

fxIyAIbpWcmAtQ==, $2y$10$xMl6FD2r7TEHtm0j0hNSeOp5hA3FNWiJjSe/qbgEwZot/sohdZpBi

But in php this case is right.

$password = '11111111';
$salt = 'fxIyAIbpWcmAtQ==';
$pbkdf22hash = hash_pbkdf2('sha512', $password, base64_decode($salt), 35000, 64);
$p0 = base64_encode(hex2bin($pbkdf22hash));
$excepted_hash = '$2y$10$xMl6FD2r7TEHtm0j0hNSeOp5hA3FNWiJjSe/qbgEwZot/sohdZpBi';
print_r(password_verify(base64_decode($p0), $excepted_hash));

in php 5.6:

<?php
$p0 = "Q0zCWhgeEtFYkAANuzKuTOctZVbZjxy4Aa7Yox1cZ0M=";
$salt = base64_decode('eE1sNkZEMnI3VEVIdG0wajBoTlNlTw==');
echo $salt;
echo "\n";
$result = password_hash(base64_decode($p0), PASSWORD_BCRYPT, ['salt' => $salt]);
echo $result;

the salt in php at password_hash is right.

What did you expect to see?

bcrypt.CompareHashAndPassword return nil.

What did you see instead?

bcrypt.CompareHashAndPassword return "crypto/bcrypt: hashedPassword is not the hash of the given password"

[magical]

The C bcrypt library (which PHP uses) has a limitation: the password cannot contain any 0 bytes. If there is a 0 byte in the password, everything after it is ignored for the purposes of the calculating the hash. Go intentionally does not emulate this behaviour.

There are a couple workarounds. First, you can strip off everything following a 0 byte yourself before calling GenerateFromHash/CompareHashFromPassword. Doing this makes your test cases pass:

https://play.golang.org/p/Gv2ZyJoXnn0

result := pbkdf2.Key([]byte(test.password), salt1, 35000, 32, sha512.New)
if i := bytes.IndexByte(result, 0); i >= 0 {
	result = result[:i]
}
fmt.Println(bcrypt.CompareHashAndPassword([]byte(test.expectedPassword), result))

However, it's probably a bad idea to discard key material like that. If interoperability with the C library is a concern, a better solution would be to avoid passing binary data to bcrypt in the first place.

result := pbkdf2.Key([]byte(test.password), salt1, 35000, 32, sha512.New)
result = []byte(hex.EncodeToString(result))
fmt.Println(bcrypt.CompareHashAndPassword([]byte(test.expectedPassword), result))

[cagedmantis]

@FiloSottile @katiehockman

[FiloSottile]

As @magical said, silently discarding data from the input is dangerous and not a bahavior we want to replicate. For random inputs there is a 1/256 chance the first byte will be a zero, in which case the hash generated by the PHP library will always be the same, which is a security issue. You should probably encode the input.

We should mention this behavior in the docs, though.

[howjmay]

Could I try to pick this issue?

[agnivade]

Go for it.

[howjmay]

I am wondering where is the document that I should add to?

[gopherbot]

Change https://go.dev/cl/516916 mentions this issue: docs(bcrypt): Clarify behavior with null bytes in passwords