crypto/ecdsa: P521 ecdsa.Verify panics with malformed message

[guidovranken]

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

go version go1.20.5 linux/amd64

Does this issue reproduce with the latest release?

Yes, but only the 1.20 branch, not 1.19

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

Linux x64

What did you do?

https://go.dev/play/p/CCW4-OX4nMQ

What did you expect to see?

No panic

What did you see instead?

panic: ecdsa: internal error: truncated hash is too long

goroutine 1 [running]:
crypto/ecdsa.hashToNat[...](0xc00010604a?, 0xc0000605a0?, {0xc000051ebe?, 0xc000060660?, 0x1b1c1c4138914079?})
/usr/local/go-faketime/src/crypto/ecdsa/ecdsa.go:397 +0x167
crypto/ecdsa.verifyNISTEC[...](0xc00001e180, 0xc000051f50, {0xc000051ebe, 0x42, 0x42}, {0xc000106000, 0x0?, 0x0?})
/usr/local/go-faketime/src/crypto/ecdsa/ecdsa.go:511 +0x389
crypto/ecdsa.VerifyASN1(0xc000051f50, {0xc000051ebe, 0x42, 0x42}, {0xc000106000, 0x8b, 0xa0})
/usr/local/go-faketime/src/crypto/ecdsa/ecdsa.go:482 +0x1aa
crypto/ecdsa.Verify(0x40b75e?, {0xc000051ebe, 0x42, 0x42}, 0x0?, 0xc000051f10)
/usr/local/go-faketime/src/crypto/ecdsa/ecdsa_legacy.go:126 +0x10b
main.main()
/tmp/sandbox2369747498/prog.go:26 +0x22b

Found on OSS-Fuzz

[seankhliao]

cc @golang/security

[rolandshoemaker]

I think I'm generally of the opinion that this is one of the few cases where a panic is reasonable, given that the Verify and VerifyASN1 methods do not have error returns.

An incorrect length hash is indicative of something being very wrong, and typically should be something a developer can fix themselves (rather than simply consuming malformed attacker controlled inputs), so panicking and alerting the developer to this, rather than simply silently returning false, feels like the correct approach.

At most I think we should maybe document that the hash needs to be the right length?

[rolandshoemaker]

@FiloSottile points out that SEC 1, Section 4.1.3 and Section 4.1.4 do specify silent truncation:

5.2. Set E = H if dlog2 ne ≥ 8(hashlen), and set E equal to the leftmost dlog2 ne bits of H if dlog2 ne < 8(hashlen).

This seems like this will just silently cause breakage, but 🤷, the specification is the boss I guess.

[gopherbot]

Change https://go.dev/cl/502478 mentions this issue: crypto/ecdsa: properly truncate P-521 hashes

[FiloSottile]

@gopherbot please open a Go 1.20 backport.

I think this doesn't have security impact because ECDSA hashes are not attacker-controlled and no one has a 528 hash laying around, but it's a spec deviation leading to a panic, so might as well fix it in Go 1.21 and backport it.

[gopherbot]

Backport issue(s) opened: #60744 (for 1.20), #60745 (for 1.21).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

[guidovranken]

Here's another one found by OSS-Fuzz where ecdsa.Verify doesn't panic but incorrectly returns false in Go 1.20 and the dev branch, but correctly returns true in 1.19.

https://go.dev/play/p/iXeINrfKQqz?v=goprev

[FiloSottile]

Here's another one found by OSS-Fuzz where ecdsa.Verify doesn't panic but incorrectly returns false in Go 1.20 and the dev branch, but correctly returns true in 1.19.

https://go.dev/play/p/iXeINrfKQqz?v=goprev

Also fixed by CL 502478 (which is waiting for a Googler +1).

[gopherbot]

Change https://go.dev/cl/502915 mentions this issue: [release-branch.go1.20] crypto/ecdsa: properly truncate P-521 hashes