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

crypto/md5: on 32-bit platforms, panic trying to Sum after a large amount of data hashed #29545

Closed
jblebrun opened this issue Jan 4, 2019 · 3 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@jblebrun
Copy link
Contributor

jblebrun commented Jan 4, 2019

(Note: this is the same problem as in #29517, but in the crypto/md5 package. Fix will be similar)

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

$ go version 1.11.1

Does this issue reproduce with the latest release?

Yes

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

go env
$ go env

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11.1/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11.1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
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/w6/9bjdntr539z1v138l_8mvmqshs5f5p/T/go-build219458045=/tmp/go-build -gno-record-gcc-switches -fno-common"

Note: the issue only occurs targeting architectures for which an int is 32 bits. I can easily trigger the problem by building with GOARCH=386.

What did you do?

Hash a large amount of data (length large than a platform int), save the hash using binary.Marshal, restore it using binary.Unmarshal, and then try to Sum it.

This play.golang.org link contains a simple example that triggers the issue:
https://play.golang.org/p/QWPkg05_3mx

When I run it there, the arch I see is amd64p32, so the issue occurs.

What did you expect to see?

I expect to be able to successfully get a checksum for the unmarshaled md5 without panic.

What did you see instead?

Upon calling h.Sum(nil), I get a panic similar to:

panic: d.nx != 0

goroutine 1 [running]:
crypto/md5.(*digest).checkSum(0x41c6f0, 0x60, 0x0, 0x0, 0x0, 0x0)
	/usr/local/go/src/crypto/md5/md5.go:189 +0x300
crypto/md5.(*digest).Sum(0x452000, 0x0, 0x0, 0x0, 0x0, 0x0, 0x60, 0x4441c0)
	/usr/local/go/src/crypto/md5/md5.go:166 +0x60
main.main()
	/tmp/sandbox438102748/main.go:23 +0x1e0

Possible resolution

Fix cast in unmarshal code (as in #29517)

@gopherbot
Copy link

Change https://golang.org/cl/156297 mentions this issue: crypto/md5: fix casting of d.nx in UnmarshalBinary

@katiehockman katiehockman added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 4, 2019
@katiehockman katiehockman added this to the Go1.13 milestone Jan 4, 2019
@josharian
Copy link
Contributor

You’ve found and fixed a few of these. Out of curiosity, how did you find them? Encountered a crash in your program? Visual inspection of source? Some kind of tool?

cc @dominikh in case there is a useful tool to be built here

@jblebrun
Copy link
Contributor Author

jblebrun commented Jan 8, 2019

@josharian Initially, I discovered the issue in crypto/sha256 from an actual crash on code that was running, so nothing too special there. After investigating that crash, I remembered that the hash Marshal/Unmarshal code was all added around the same time, so I decided to peek around the rest of the crypto directory for anything similar (I pretty much just grepped for d.nx) -- and discovered that the same casting issue existing for sha512, md5, and sha1 as well.

@golang golang locked and limited conversation to collaborators Jan 8, 2020
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

4 participants