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: hashes different than in other languages #26301

Closed
geekgonecrazy opened this issue Jul 9, 2018 · 12 comments
Closed

x/crypto/bcrypt: hashes different than in other languages #26301

geekgonecrazy opened this issue Jul 9, 2018 · 12 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@geekgonecrazy
Copy link

geekgonecrazy commented Jul 9, 2018

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

go version go1.10.3 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/aaron/Library/Caches/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/aaron/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.10.3/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.10.3/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
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/3s/c5bbqcxn7dlfcpjys4wytyvc0000gn/T/go-build618429129=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Get a bcrypt hash generated by x/crypto and then try to validate in node.js and python.

I verified node.js and python can validate hashes between each other. But neither can validate hashes generated by this package.

Code samples: https://gist.github.com/geekgonecrazy/3a61ee15f515022295eef57f0713b52b

I can see the crypto package only has bcrypt 2a but from what I can tell 2a and 2b should be compatible.

To rule this out generated 2a from node.js and python could validate. But still not the crypto package.

I'm stumped... Its unclear if a bug, or the results for what ever reason are completely incompatible with other bcrypt packages.

@gopherbot gopherbot added this to the Unreleased milestone Jul 9, 2018
@ianlancetaylor ianlancetaylor changed the title x/crypto bcrypt hashes uncomparible in other languages x/crypto/bcrypt: hashes different than in other languages Jul 9, 2018
@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 9, 2018
@ianlancetaylor
Copy link
Contributor

CC @FiloSottile

@meirf
Copy link
Contributor

meirf commented Jul 10, 2018

I'm not sure if there is any value to this, but I noticed that if I make up a sequence of letters/numbers as the password instead of the sha then I do get a match between go and python. (For example asdfasdfhjlkjh45lhjk4t34lkjh34kjh34lkjh34lkjhkjh3434343434kjlhlk, $2a$10$RaRugqQbVh/YQ/lAf8fTZ.lIWUrVal3/ncbOSpv9DvM1WvKkPkyjS.)

@aead
Copy link
Contributor

aead commented Jul 10, 2018

golang.org/x/crypto/bcrypt generates pw-hashes using bcrypt version 2a (see $2a$) while python and node.js seem to create them using bcrypt version 2b (see $2b$).
Refs:

I guess bcrypt should be updated to handle versions 2b, too. (See https://github.com/golang/crypto/blob/master/bcrypt/bcrypt.go#L136)
Further I'd recommend to switch to algorithms providing stronger security properties / guarantees:
Argon2 or scrypt

@geekgonecrazy
Copy link
Author

geekgonecrazy commented Jul 10, 2018

I guess bcrypt should be updated to handle versions 2b, too. (See https://github.com/golang/crypto/blob/master/bcrypt/bcrypt.go#L136)

From my understanding at the least python and node.js should still be able to compare and match with an older version.

In node.js specifically if you do something like:

var hash = bcrypt.hashSync(sha, bcrypt.genSaltSync(10, 'a'))
console.log('bcrypt:', hash);

This generates $2a$ which python seems to be able to compare, but x/crypto/bcrypt still can't.

Further I'd recommend to switch to algorithms providing stronger security properties / guarantees:
Argon2 or scrypt

@aead
I wish that were an option. But migrating an existing application with thousands of users to a new password hash would be a last resort. Specifically using sha256 + bcrypt to integrate with an existing meteor application. Definitely wouldn't argue on security between the two. No doubt

I'm not sure if there is any value to this, but I noticed that if I make up a sequence of letters/numbers as the password instead of the sha then I do get a match between go and python.

@meirf now that is kind of interesting. Sha256 is just letters and numbers as well.

@conradoplg
Copy link
Contributor

In Go, you're feeding bcrypt with the byte array representation of the hash.

In Python, you're feeding bcrypt with the hex string representation of the hash.

Change hexdigest() in the Python code to digest() and it will work.

@geekgonecrazy
Copy link
Author

For me python is added just to be more thorough and make sure it wasn't just a go->nodejs thing but an issue with the hashes generated by go.

It being a byte array in go causing the problem makes some sense.

But the fact that I can't use a hash generated by go in any other language still remains a problem. The key problem in my case.

@ghost
Copy link

ghost commented Jul 11, 2018

I can't use a hash generated by go in any other language

You're doing it wrong. I've just checked: node.js successfully compares hashes generated by Go, and vice versa. The same with python. The bcrypt modules are perfectly interoperable.

@geekgonecrazy
Copy link
Author

@opennota care to elaborate? What exactly am I doing incorrectly? I've tried so many different variations, I totally believe i'm missing something that would be more obvious to someone more experienced.

@ghost
Copy link

ghost commented Jul 11, 2018

@geekgonecrazy
Probably what @conradoplg said. Try

bcrypt.GenerateFromPassword([]byte(hex.EncodeToString(sha)), 10)

(See https://golang.org/pkg/encoding/hex/#EncodeToString)

@aead
Copy link
Contributor

aead commented Jul 11, 2018

I guess working-as-intenent

@FiloSottile
Copy link
Contributor

Correct, Sum in Go returns the raw hash (and %x converts it to hex when printing it), while hexdigest in Python and sha256 in Javascript return a hex encoding, so you are feeding different inputs to bcrypt.

@geekgonecrazy
Copy link
Author

Makes sense! @opennota thanks for the code snippet. I've confirmed it works. Learn something new every day.

Thanks guys, glad it wasn't actually a bug in the package! :)

@golang golang locked and limited conversation to collaborators Jul 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

7 participants