x/crypto/bcrypt: CompareHashAndPassword only reads the first 72 bytes of the password input.

[ncw]

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

Tested with golang/x/crypto: https://github.com/golang/crypto/commit/61a87790db17894570dfb32dbaa0a4af9ce60cb4

and

go version go1.13.4 linux/amd64

Does this issue reproduce with the latest release?

It repros with the latest golang/x/crypto yes.

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

go env Output

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ncw/.cache/go-build"
GOENV="/home/ncw/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/ncw/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/go/go1.13"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/go/go1.13/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build519269826=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I've discovered that bcrypt.CompareHashAndPassword only reads the first 72 characters of the password input. You can try this for yourself with the program below.

This means that any characters after the 72 byte mark are ignored when comparing passwords.

I was expecting to receive an error from CompareHashAndPassword but instead it said the clearly non-matching passwords matched.

This behavior of bcrypt is fairly well known: https://security.stackexchange.com/questions/39849/does-bcrypt-have-a-maximum-password-length

However I found it surprising that the CompareHashAndPassword didn't give an error. The documentation doesn't mention any 72 character limit either.

See: https://play.golang.org/p/igM0KIsFhDD

package main

import (
    "fmt"

    "golang.org/x/crypto/bcrypt"
)

// check makes the hash of a password of length n then
// sees if CompareHashAndPassword gives the correct answer for a password of one bigger
func check(n int) {
    pw := make([]byte, n)
    for i := range pw {
        pw[i] = 'x'
    }
    pwHash, err := bcrypt.GenerateFromPassword(pw, bcrypt.MinCost)
    if err != nil {
        panic(err)
    }
    fmt.Printf("Made hash of password of length %d\npw=%q\n", len(pw), pw)
    pw = append(pw, 'A')
    fmt.Printf("Now checking against a password of length %d with the same start\npw=%q\n", len(pw), pw)
    err = bcrypt.CompareHashAndPassword(pwHash, pw)
    if err != nil {
        fmt.Printf("OK got error: %v\n", err)
    } else {
        fmt.Printf("FAILED was expecting error\n")
    }
    fmt.Println()
}

func main() {
    check(71)
    check(72)
}

What did you expect to see?

I expected to see either an error being returned after comparing a 73 byte password with a 72 byte password.

Or a note in the docs that only the first 72 bytes of the password are significant.

What did you see instead?

Made hash of password of length 71
pw="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
Now checking against a password of length 72 with the same start
pw="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxA"
OK got error: crypto/bcrypt: hashedPassword is not the hash of the given password

Made hash of password of length 72
pw="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
Now checking against a password of length 73 with the same start
pw="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxA"
FAILED was expecting error

Cc: @FiloSottile

[cagedmantis]

/cc @katiehockman

[vaibhav2ghadge]

how to contribute in crypto/bcrypt package im not able to find that package in go source @katiehockman

[ALTree]

@vaibhav2ghadge It's here: https://github.com/golang/crypto

[sixcolors]

it’s actually worse than you think. If using this for web that’s not a 72 char limit but a 72 byte limit. So you’re probably using utf8. Could be even fewer chars.

See https://security.stackexchange.com/questions/39849/does-bcrypt-have-a-maximum-password-length

This behavior is common to many libraries implementing bcrypt. Throwing an error would break many prod implementations. I would not recommend that it return a error.

If password truncation is a concern you could pre-hash with sha512 or similar (and salt and pepper that hash to avoid adding a Password Shucking vulnerability)

or

use a different hashing algorithm like argon2id.

[gopherbot]

Change https://go.dev/cl/450415 mentions this issue: bcrypt: reject passwords longer than 72 bytes

[FiloSottile]

This is a big enough footgun that I feel like it's worth starting to return an error. Silent truncation is never the expected behavior and I can even see it be a security issue if the first part is somehow low-entropy. We can make the impact limited by only returning an error from GenerateFromPassword, and maintaining compatibility in CompareHashAndPassword.

@sixcolors what production breakage do you expect?

[ncw]

This seems like a reasonable compromise to me. :+1:

[sixcolors]

This PR is a good solution that does not break existing hashes but stops longer passwords from being silently truncated.

Pre hashing can be done correctly but it can also be a minefield and is no longer a recommendation details

Bcrypt has its limitations but it’s tried and true and is probably compliant in most organizations.

good discussion on password length issue here https://stackoverflow.com/questions/24751279/password-max-length-with-bcrypt-blowfish

[sixcolors]

This is a big enough footgun that I feel like it's worth starting to return an error. Silent truncation is never the expected behavior and I can even see it be a security issue if the first part is somehow low-entropy. We can make the impact limited by only returning an error from GenerateFromPassword, and maintaining compatibility in CompareHashAndPassword.

@sixcolors what production breakage do you expect?

👍 solution is a good one

One issue I could see coming up is that error handling for password length may not be implemented and that silent truncation may be the expectation. This would change that behavior. However, I’m also not a fan of the silent truncation.

I think this should be accepted and merged.

Thanks!