From a5ebc6b67c1e397ab74abadf20a7f290cf28949e Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Mon, 24 Nov 2025 20:46:14 +0100 Subject: [PATCH] crypto/ecdsa: clean up ECDSA parsing and serialization paths Check for invalid encodings and keys more systematically in ParseRawPrivateKey/PrivateKey.Bytes, ParseUncompressedPublicKey/PublicKey.Bytes, and fips140/ecdsa.NewPrivateKey/NewPublicKey. Also, use these functions throughout the codebase. This should not change any observable behavior, because there were multiple layers of checks and every path would hit at least one. Change-Id: I6a6a46566c95de871a5a37996835a0e51495f1d8 Reviewed-on: https://go-review.googlesource.com/c/go/+/724000 LUCI-TryBot-Result: Go LUCI Auto-Submit: Filippo Valsorda Reviewed-by: Roland Shoemaker Reviewed-by: Cherry Mui --- src/crypto/ecdsa/ecdsa.go | 62 +++++++++++----------- src/crypto/ecdsa/ecdsa_test.go | 3 +- src/crypto/internal/fips140/ecdsa/ecdsa.go | 14 +++++ src/crypto/tls/tls.go | 7 ++- src/crypto/x509/parser.go | 22 +++----- src/crypto/x509/sec1.go | 30 +++++------ src/crypto/x509/x509.go | 6 +-- 7 files changed, 72 insertions(+), 72 deletions(-) diff --git a/src/crypto/ecdsa/ecdsa.go b/src/crypto/ecdsa/ecdsa.go index 54fbecb576..38f0b564ac 100644 --- a/src/crypto/ecdsa/ecdsa.go +++ b/src/crypto/ecdsa/ecdsa.go @@ -60,15 +60,16 @@ type PublicKey struct { // ECDH returns k as a [ecdh.PublicKey]. It returns an error if the key is // invalid according to the definition of [ecdh.Curve.NewPublicKey], or if the // Curve is not supported by crypto/ecdh. -func (k *PublicKey) ECDH() (*ecdh.PublicKey, error) { - c := curveToECDH(k.Curve) +func (pub *PublicKey) ECDH() (*ecdh.PublicKey, error) { + c := curveToECDH(pub.Curve) if c == nil { return nil, errors.New("ecdsa: unsupported curve by crypto/ecdh") } - if !k.Curve.IsOnCurve(k.X, k.Y) { - return nil, errors.New("ecdsa: invalid public key") + k, err := pub.Bytes() + if err != nil { + return nil, err } - return c.NewPublicKey(elliptic.Marshal(k.Curve, k.X, k.Y)) + return c.NewPublicKey(k) } // Equal reports whether pub and x have the same value. @@ -181,16 +182,16 @@ type PrivateKey struct { // ECDH returns k as a [ecdh.PrivateKey]. It returns an error if the key is // invalid according to the definition of [ecdh.Curve.NewPrivateKey], or if the // Curve is not supported by [crypto/ecdh]. -func (k *PrivateKey) ECDH() (*ecdh.PrivateKey, error) { - c := curveToECDH(k.Curve) +func (priv *PrivateKey) ECDH() (*ecdh.PrivateKey, error) { + c := curveToECDH(priv.Curve) if c == nil { return nil, errors.New("ecdsa: unsupported curve by crypto/ecdh") } - size := (k.Curve.Params().N.BitLen() + 7) / 8 - if k.D.BitLen() > size*8 { - return nil, errors.New("ecdsa: invalid private key") + k, err := priv.Bytes() + if err != nil { + return nil, err } - return c.NewPrivateKey(k.D.FillBytes(make([]byte, size))) + return c.NewPrivateKey(k) } func curveToECDH(c elliptic.Curve) ecdh.Curve { @@ -367,10 +368,6 @@ func generateFIPS[P ecdsa.Point[P]](curve elliptic.Curve, c *ecdsa.Curve[P], ran return privateKeyFromFIPS(curve, privateKey) } -// errNoAsm is returned by signAsm and verifyAsm when the assembly -// implementation is not available. -var errNoAsm = errors.New("no assembly implementation available") - // SignASN1 signs a hash (which should be the result of hashing a larger message) // using the private key, priv. If the hash is longer than the bit-length of the // private key's curve order, the hash will be truncated to that length. It @@ -576,27 +573,30 @@ func privateKeyToFIPS[P ecdsa.Point[P]](c *ecdsa.Curve[P], priv *PrivateKey) (*e if err != nil { return nil, err } + + // Reject values that would not get correctly encoded. + if priv.D.BitLen() > priv.Curve.Params().N.BitLen() { + return nil, errors.New("ecdsa: private key scalar too large") + } + if priv.D.Sign() <= 0 { + return nil, errors.New("ecdsa: private key scalar is zero or negative") + } + + size := (priv.Curve.Params().N.BitLen() + 7) / 8 + const maxScalarSize = 66 // enough for a P-521 private key + if size > maxScalarSize { + return nil, errors.New("ecdsa: internal error: curve size too large") + } + D := priv.D.FillBytes(make([]byte, size, maxScalarSize)) + return privateKeyCache.Get(priv, func() (*ecdsa.PrivateKey, error) { - return ecdsa.NewPrivateKey(c, priv.D.Bytes(), Q) + return ecdsa.NewPrivateKey(c, D, Q) }, func(k *ecdsa.PrivateKey) bool { return subtle.ConstantTimeCompare(k.PublicKey().Bytes(), Q) == 1 && - leftPadBytesEqual(k.Bytes(), priv.D.Bytes()) + subtle.ConstantTimeCompare(k.Bytes(), D) == 1 }) } -func leftPadBytesEqual(a, b []byte) bool { - if len(a) < len(b) { - a, b = b, a - } - if len(a) > len(b) { - x := make([]byte, 0, 66 /* enough for a P-521 private key */) - x = append(x, make([]byte, len(a)-len(b))...) - x = append(x, b...) - b = x - } - return subtle.ConstantTimeCompare(a, b) == 1 -} - // pointFromAffine is used to convert the PublicKey to a nistec SetBytes input. func pointFromAffine(curve elliptic.Curve, x, y *big.Int) ([]byte, error) { bitSize := curve.Params().BitSize @@ -607,7 +607,7 @@ func pointFromAffine(curve elliptic.Curve, x, y *big.Int) ([]byte, error) { if x.BitLen() > bitSize || y.BitLen() > bitSize { return nil, errors.New("overflowing coordinate") } - // Encode the coordinates and let SetBytes reject invalid points. + // Encode the coordinates and let [ecdsa.NewPublicKey] reject invalid points. byteLen := (bitSize + 7) / 8 buf := make([]byte, 1+2*byteLen) buf[0] = 4 // uncompressed point diff --git a/src/crypto/ecdsa/ecdsa_test.go b/src/crypto/ecdsa/ecdsa_test.go index 87e74f2a0e..9594dbd1cb 100644 --- a/src/crypto/ecdsa/ecdsa_test.go +++ b/src/crypto/ecdsa/ecdsa_test.go @@ -694,7 +694,8 @@ func testInvalidPrivateKeys(t *testing.T, curve elliptic.Curve) { t.Errorf("ParseRawPrivateKey accepted short key") } - b = append(b, make([]byte, (curve.Params().BitSize+7)/8)...) + b = make([]byte, (curve.Params().BitSize+7)/8) + b = append(b, []byte{1, 2, 3}...) if _, err := ParseRawPrivateKey(curve, b); err == nil { t.Errorf("ParseRawPrivateKey accepted long key") } diff --git a/src/crypto/internal/fips140/ecdsa/ecdsa.go b/src/crypto/internal/fips140/ecdsa/ecdsa.go index 81179de4f4..1c00f55810 100644 --- a/src/crypto/internal/fips140/ecdsa/ecdsa.go +++ b/src/crypto/internal/fips140/ecdsa/ecdsa.go @@ -156,24 +156,38 @@ var p521Order = []byte{0x01, 0xff, 0x3b, 0xb5, 0xc9, 0xb8, 0x89, 0x9c, 0x47, 0xae, 0xbb, 0x6f, 0xb7, 0x1e, 0x91, 0x38, 0x64, 0x09} +// NewPrivateKey creates a new ECDSA private key from the given D and Q byte +// slices. D must be the fixed-length big-endian encoding of the private scalar, +// and Q must be the compressed or uncompressed encoding of the public point. func NewPrivateKey[P Point[P]](c *Curve[P], D, Q []byte) (*PrivateKey, error) { fips140.RecordApproved() pub, err := NewPublicKey(c, Q) if err != nil { return nil, err } + if len(D) != c.N.Size() { + return nil, errors.New("ecdsa: invalid private key length") + } d, err := bigmod.NewNat().SetBytes(D, c.N) if err != nil { return nil, err } + if d.IsZero() == 1 { + return nil, errors.New("ecdsa: private key is zero") + } priv := &PrivateKey{pub: *pub, d: d.Bytes(c.N)} return priv, nil } +// NewPublicKey creates a new ECDSA public key from the given Q byte slice. +// Q must be the compressed or uncompressed encoding of the public point. func NewPublicKey[P Point[P]](c *Curve[P], Q []byte) (*PublicKey, error) { // SetBytes checks that Q is a valid point on the curve, and that its // coordinates are reduced modulo p, fulfilling the requirements of SP // 800-89, Section 5.3.2. + if len(Q) < 1 || Q[0] == 0 { + return nil, errors.New("ecdsa: invalid public key encoding") + } _, err := c.newPoint().SetBytes(Q) if err != nil { return nil, err diff --git a/src/crypto/tls/tls.go b/src/crypto/tls/tls.go index a0fd8e835d..69f096870c 100644 --- a/src/crypto/tls/tls.go +++ b/src/crypto/tls/tls.go @@ -24,7 +24,6 @@ package tls // https://www.imperialviolet.org/2013/02/04/luckythirteen.html. import ( - "bytes" "context" "crypto" "crypto/ecdsa" @@ -335,7 +334,7 @@ func X509KeyPair(certPEMBlock, keyPEMBlock []byte) (Certificate, error) { if !ok { return fail(errors.New("tls: private key type does not match public key type")) } - if pub.N.Cmp(priv.N) != 0 { + if !priv.PublicKey.Equal(pub) { return fail(errors.New("tls: private key does not match public key")) } case *ecdsa.PublicKey: @@ -343,7 +342,7 @@ func X509KeyPair(certPEMBlock, keyPEMBlock []byte) (Certificate, error) { if !ok { return fail(errors.New("tls: private key type does not match public key type")) } - if pub.X.Cmp(priv.X) != 0 || pub.Y.Cmp(priv.Y) != 0 { + if !priv.PublicKey.Equal(pub) { return fail(errors.New("tls: private key does not match public key")) } case ed25519.PublicKey: @@ -351,7 +350,7 @@ func X509KeyPair(certPEMBlock, keyPEMBlock []byte) (Certificate, error) { if !ok { return fail(errors.New("tls: private key type does not match public key type")) } - if !bytes.Equal(priv.Public().(ed25519.PublicKey), pub) { + if !priv.Public().(ed25519.PublicKey).Equal(pub) { return fail(errors.New("tls: private key does not match public key")) } default: diff --git a/src/crypto/x509/parser.go b/src/crypto/x509/parser.go index 680dcee203..e255f7d604 100644 --- a/src/crypto/x509/parser.go +++ b/src/crypto/x509/parser.go @@ -10,7 +10,6 @@ import ( "crypto/ecdh" "crypto/ecdsa" "crypto/ed25519" - "crypto/elliptic" "crypto/rsa" "crypto/x509/pkix" "encoding/asn1" @@ -250,7 +249,7 @@ func parseExtension(der cryptobyte.String) (pkix.Extension, error) { func parsePublicKey(keyData *publicKeyInfo) (any, error) { oid := keyData.Algorithm.Algorithm params := keyData.Algorithm.Parameters - der := cryptobyte.String(keyData.PublicKey.RightAlign()) + data := keyData.PublicKey.RightAlign() switch { case oid.Equal(oidPublicKeyRSA): // RSA public keys must have a NULL in the parameters. @@ -259,6 +258,7 @@ func parsePublicKey(keyData *publicKeyInfo) (any, error) { return nil, errors.New("x509: RSA key missing NULL parameters") } + der := cryptobyte.String(data) p := &pkcs1PublicKey{N: new(big.Int)} if !der.ReadASN1(&der, cryptobyte_asn1.SEQUENCE) { return nil, errors.New("x509: invalid RSA public key") @@ -292,34 +292,26 @@ func parsePublicKey(keyData *publicKeyInfo) (any, error) { if namedCurve == nil { return nil, errors.New("x509: unsupported elliptic curve") } - x, y := elliptic.Unmarshal(namedCurve, der) - if x == nil { - return nil, errors.New("x509: failed to unmarshal elliptic curve point") - } - pub := &ecdsa.PublicKey{ - Curve: namedCurve, - X: x, - Y: y, - } - return pub, nil + return ecdsa.ParseUncompressedPublicKey(namedCurve, data) case oid.Equal(oidPublicKeyEd25519): // RFC 8410, Section 3 // > For all of the OIDs, the parameters MUST be absent. if len(params.FullBytes) != 0 { return nil, errors.New("x509: Ed25519 key encoded with illegal parameters") } - if len(der) != ed25519.PublicKeySize { + if len(data) != ed25519.PublicKeySize { return nil, errors.New("x509: wrong Ed25519 public key size") } - return ed25519.PublicKey(der), nil + return ed25519.PublicKey(data), nil case oid.Equal(oidPublicKeyX25519): // RFC 8410, Section 3 // > For all of the OIDs, the parameters MUST be absent. if len(params.FullBytes) != 0 { return nil, errors.New("x509: X25519 key encoded with illegal parameters") } - return ecdh.X25519().NewPublicKey(der) + return ecdh.X25519().NewPublicKey(data) case oid.Equal(oidPublicKeyDSA): + der := cryptobyte.String(data) y := new(big.Int) if !der.ReadASN1Integer(y) { return nil, errors.New("x509: invalid DSA public key") diff --git a/src/crypto/x509/sec1.go b/src/crypto/x509/sec1.go index 8e81bd4129..6212c2d4ff 100644 --- a/src/crypto/x509/sec1.go +++ b/src/crypto/x509/sec1.go @@ -11,7 +11,6 @@ import ( "encoding/asn1" "errors" "fmt" - "math/big" ) const ecPrivKeyVersion = 1 @@ -55,15 +54,19 @@ func MarshalECPrivateKey(key *ecdsa.PrivateKey) ([]byte, error) { // marshalECPrivateKeyWithOID marshals an EC private key into ASN.1, DER format and // sets the curve ID to the given OID, or omits it if OID is nil. func marshalECPrivateKeyWithOID(key *ecdsa.PrivateKey, oid asn1.ObjectIdentifier) ([]byte, error) { - if !key.Curve.IsOnCurve(key.X, key.Y) { - return nil, errors.New("invalid elliptic key public key") + privateKey, err := key.Bytes() + if err != nil { + return nil, err + } + publicKey, err := key.PublicKey.Bytes() + if err != nil { + return nil, err } - privateKey := make([]byte, (key.Curve.Params().N.BitLen()+7)/8) return asn1.Marshal(ecPrivateKey{ Version: 1, - PrivateKey: key.D.FillBytes(privateKey), + PrivateKey: privateKey, NamedCurveOID: oid, - PublicKey: asn1.BitString{Bytes: elliptic.Marshal(key.Curve, key.X, key.Y)}, + PublicKey: asn1.BitString{Bytes: publicKey}, }) } @@ -106,16 +109,8 @@ func parseECPrivateKey(namedCurveOID *asn1.ObjectIdentifier, der []byte) (key *e return nil, errors.New("x509: unknown elliptic curve") } - k := new(big.Int).SetBytes(privKey.PrivateKey) - curveOrder := curve.Params().N - if k.Cmp(curveOrder) >= 0 { - return nil, errors.New("x509: invalid elliptic curve private key value") - } - priv := new(ecdsa.PrivateKey) - priv.Curve = curve - priv.D = k - - privateKey := make([]byte, (curveOrder.BitLen()+7)/8) + size := (curve.Params().N.BitLen() + 7) / 8 + privateKey := make([]byte, size) // Some private keys have leading zero padding. This is invalid // according to [SEC1], but this code will ignore it. @@ -130,7 +125,6 @@ func parseECPrivateKey(namedCurveOID *asn1.ObjectIdentifier, der []byte) (key *e // according to [SEC1] but since OpenSSL used to do this, we ignore // this too. copy(privateKey[len(privateKey)-len(privKey.PrivateKey):], privKey.PrivateKey) - priv.X, priv.Y = curve.ScalarBaseMult(privateKey) - return priv, nil + return ecdsa.ParseRawPrivateKey(curve, privateKey) } diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go index afd3d8673a..85e8fceedc 100644 --- a/src/crypto/x509/x509.go +++ b/src/crypto/x509/x509.go @@ -101,10 +101,10 @@ func marshalPublicKey(pub any) (publicKeyBytes []byte, publicKeyAlgorithm pkix.A if !ok { return nil, pkix.AlgorithmIdentifier{}, errors.New("x509: unsupported elliptic curve") } - if !pub.Curve.IsOnCurve(pub.X, pub.Y) { - return nil, pkix.AlgorithmIdentifier{}, errors.New("x509: invalid elliptic curve public key") + publicKeyBytes, err = pub.Bytes() + if err != nil { + return nil, pkix.AlgorithmIdentifier{}, err } - publicKeyBytes = elliptic.Marshal(pub.Curve, pub.X, pub.Y) publicKeyAlgorithm.Algorithm = oidPublicKeyECDSA var paramBytes []byte paramBytes, err = asn1.Marshal(oid) -- 2.52.0