crypto/x509: panics on invalid curve instead of returning error #54288
Comments
in marshalPublicKey case *ecdsa.PublicKey: because elliptic.Marshal() was called before oidFromNamedCurve we would panic if the curve was invalid. By calling oidFromNamedCurve first we can return early with a meaningfull error Fixes golang#54288
|
Change https://go.dev/cl/421617 mentions this issue: |
|
Ah, yeah, functions with an error return value should definitely return an error, not panic. I'll do a pass of all the marshal-side paths, and see if there are other issues like this. @gopherbot please open a backport issue to Go 1.19. I don't think this is a security issue because the attacker can't control the curve of a certificate being marshaled, but panic'ing where we were returning an error is a regression and we should quash it. |
|
Backport issue(s) opened: #54295 (for 1.19). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases. |
seankhliao
added
the
NeedsFix
|
Change https://go.dev/cl/422298 mentions this issue: |
|
Ok. I had already opened #54290 along with this issue, anything else i should do on my end? |
|
@FnuGk no, we've got it, thank you for the report! https://go.dev/cl/422298 is a slightly broader fix that is now under review. |
|
Change https://go.dev/cl/425634 mentions this issue: |
…CDSA keys MarshalPKIXPublicKey, CreateCertificate, CreateCertificateRequest, MarshalECPrivateKey, and MarshalPKCS8PrivateKey started raising a panic when encoding an invalid ECDSA key in Go 1.19. Since they have an error return value, they should return an error instead. Updates #54288 Fixes #54295 Change-Id: Iba132cd2f890ece36bb7d0396eb9a9a77bdb81df Reviewed-on: https://go-review.googlesource.com/c/go/+/422298 Auto-Submit: Filippo Valsorda <filippo@golang.org> Reviewed-by: Roland Shoemaker <roland@golang.org> Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com> (cherry picked from commit f64f12f) Reviewed-on: https://go-review.googlesource.com/c/go/+/425634
MarshalPKIXPublicKey, CreateCertificate, CreateCertificateRequest, MarshalECPrivateKey, and MarshalPKCS8PrivateKey started raising a panic when encoding an invalid ECDSA key in Go 1.19. Since they have an error return value, they should return an error instead. Fixes golang#54288 Change-Id: Iba132cd2f890ece36bb7d0396eb9a9a77bdb81df Reviewed-on: https://go-review.googlesource.com/c/go/+/422298 Auto-Submit: Filippo Valsorda <filippo@golang.org> Reviewed-by: Roland Shoemaker <roland@golang.org> Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com>
…CDSA keys MarshalPKIXPublicKey, CreateCertificate, CreateCertificateRequest, MarshalECPrivateKey, and MarshalPKCS8PrivateKey started raising a panic when encoding an invalid ECDSA key in Go 1.19. Since they have an error return value, they should return an error instead. Updates golang#54288 Fixes golang#54295 Change-Id: Iba132cd2f890ece36bb7d0396eb9a9a77bdb81df Reviewed-on: https://go-review.googlesource.com/c/go/+/422298 Auto-Submit: Filippo Valsorda <filippo@golang.org> Reviewed-by: Roland Shoemaker <roland@golang.org> Run-TryBot: Filippo Valsorda <filippo@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: David Chase <drchase@google.com> (cherry picked from commit f64f12f) Reviewed-on: https://go-review.googlesource.com/c/go/+/425634
What version of Go are you using (
go version)?What did you do?
In go 1.19
crypto/ellipticwill now panic when operating on invalid curve points. This is great as it avoids undefined behavior, butcrypto/x509will now panic inmarshalPublicKeyin thecase *ecdsa.PublicKey:where it previosly would returnerrors.New("x509: unsupported elliptic curve")as it callselliptic.Marshal(pub.Curve, pub.X, pub.Y)before it checksoidFromNamedCurve(pub.Curve).So simply switching those two calls around should fix the issue.
Meanwhile clients have to manually check if their key satisfies
IsOnCurvebefore calling something likex509.MarshalPKIXPublicKeywhere in previous versions of go you could rely on an error being returned instead of a panic.The text was updated successfully, but these errors were encountered: