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.

Sign up for GitHub

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

Jump to bottom

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

Closed
guidovranken opened this issue Jun 12, 2023 · 9 comments
Closed

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

guidovranken opened this issue Jun 12, 2023 · 9 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@guidovranken
Copy link

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 seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 12, 2023
@seankhliao
Copy link
Member

cc @golang/security

@rolandshoemaker
Copy link
Member

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
Copy link
Member

@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
Copy link

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

@FiloSottile
Copy link
Contributor

@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.

This was referenced Jun 12, 2023
Closed
Closed
@gopherbot
Copy link

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
Copy link
Author

guidovranken commented Jun 13, 2023 •
edited

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
Copy link
Contributor

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
Copy link

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

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 14, 2023
@dmitshur dmitshur added this to the Go1.21 milestone Jun 14, 2023
gopherbot pushed a commit that referenced this issue Jun 19, 2023
@FiloSottile @gopherbot
[release-branch.go1.20] crypto/ecdsa: properly truncate P-521 hashes
a7a48fa 
Before, if a hash was exactly 66 bytes long, we weren't truncating it
for use with P-521, because the byte length was not overflowing.
However, the bit length could still overflow.

Fixes #60744
Updates #60741

Change-Id: I37a0ee210add0eb566e6dc1c141e83e992983eb6
Reviewed-on: https://go-review.googlesource.com/c/go/+/502478
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
(cherry picked from commit 886fba5)
Reviewed-on: https://go-review.googlesource.com/c/go/+/502915
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@ripatel-fd ripatel-fd mentioned this issue Oct 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
6 participants
@FiloSottile @dmitshur @rolandshoemaker @guidovranken @gopherbot @seankhliao