]> Cypherpunks repositories - gostls13.git/commitdiff
crypto/ecdsa: clean up ECDSA parsing and serialization paths
authorFilippo Valsorda <filippo@golang.org>
Mon, 24 Nov 2025 19:46:14 +0000 (20:46 +0100)
committerGopher Robot <gobot@golang.org>
Tue, 25 Nov 2025 18:29:31 +0000 (10:29 -0800)
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 <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
src/crypto/ecdsa/ecdsa.go
src/crypto/ecdsa/ecdsa_test.go
src/crypto/internal/fips140/ecdsa/ecdsa.go
src/crypto/tls/tls.go
src/crypto/x509/parser.go
src/crypto/x509/sec1.go
src/crypto/x509/x509.go

index 54fbecb5764aab3391283e24e4aecd461367b2c5..38f0b564ac98e0a6ecc1fcec9a8c7239ddbd29ed 100644 (file)
@@ -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
index 87e74f2a0e9d6a2464619347cc49b3e4d0145e96..9594dbd1cb6c84e4b50b675d9fc4f7b7d9422c4d 100644 (file)
@@ -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")
                }
index 81179de4f4e0017fb5d330b24883abfbaa8dfde5..1c00f55810a772e874f70f5b18c6922a89d656a7 100644 (file)
@@ -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
index a0fd8e835da7ae01ece0e996e8860d6df6490234..69f096870c9f489a64a389debacd04c97bf3f235 100644 (file)
@@ -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:
index 680dcee203a8284dff0a582cd043b274a1e301a7..e255f7d604a40a770dee09f7e84065509d9390b6 100644 (file)
@@ -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")
index 8e81bd412984bc304f81ac0bde980004a100bf7d..6212c2d4ff58ed95f6ca6d52ab18719aeba652cf 100644 (file)
@@ -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)
 }
index afd3d8673a7ac9c2bc7ff8b90cbd195d50836def..85e8fceedcb767fdde28e4bd9ed890471a9a866d 100644 (file)
@@ -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)