Skip to content
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

x/crypto/bcrypt: document handling of zero bytes #36016

Open
moyishizhe opened this issue Dec 6, 2019 · 7 comments
Open

x/crypto/bcrypt: document handling of zero bytes #36016

moyishizhe opened this issue Dec 6, 2019 · 7 comments
Labels
Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@moyishizhe
Copy link

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"

@bradfitz bradfitz changed the title bcrypt.CompareHashAndPassword is error in this case. x/crypto/bcrypt: CompareHashAndPassword is error in this case Dec 6, 2019
@gopherbot gopherbot added this to the Unreleased milestone Dec 6, 2019
@cagedmantis cagedmantis changed the title x/crypto/bcrypt: CompareHashAndPassword is error in this case x/crypto/bcrypt: function CompareHashAndPassword has in error in this case Dec 6, 2019
@magical
Copy link
Contributor

magical commented Dec 6, 2019

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 cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 6, 2019
@cagedmantis
Copy link
Contributor

@FiloSottile @katiehockman

@FiloSottile FiloSottile added Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 6, 2019
@FiloSottile
Copy link
Contributor

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.

@FiloSottile FiloSottile changed the title x/crypto/bcrypt: function CompareHashAndPassword has in error in this case x/crypto/bcrypt: document handling of zero bytes Dec 9, 2019
@howjmay
Copy link
Contributor

howjmay commented Feb 2, 2020

Could I try to pick this issue?

@agnivade
Copy link
Contributor

agnivade commented Feb 3, 2020

Go for it.

@howjmay
Copy link
Contributor

howjmay commented Feb 15, 2020

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

@gopherbot
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants