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: CompareHashAndPassword only reads the first 72 bytes of the password input. #36546

Open
ncw opened this issue Jan 14, 2020 · 3 comments

Comments

@ncw
Copy link
Contributor

@ncw ncw commented Jan 14, 2020

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

Tested with golang/x/crypto: golang/crypto@61a8779

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

@gopherbot gopherbot added this to the Unreleased milestone Jan 14, 2020
@cagedmantis

This comment has been minimized.

Copy link
Contributor

@cagedmantis cagedmantis commented Jan 14, 2020

@vaibhav2ghadge

This comment has been minimized.

Copy link

@vaibhav2ghadge vaibhav2ghadge commented Jan 16, 2020

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

@ALTree

This comment has been minimized.

Copy link
Member

@ALTree ALTree commented Jan 16, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.