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

@@ -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) {
+func (pub *PublicKey) ECDH() (*ecdh.PublicKey, error) {
-	c := curveToECDH(k.Curve)
+	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) {
+	k, err := pub.Bytes()
-		return nil, errors.New("ecdsa: invalid public key")
+	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) {
+func (priv *PrivateKey) ECDH() (*ecdh.PrivateKey, error) {
-	c := curveToECDH(k.Curve)
+	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
+	k, err := priv.Bytes()
-	if k.D.BitLen() > size*8 {
+	if err != nil {
-		return nil, errors.New("ecdsa: invalid private key")
+		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

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")
 		}

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

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:

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)
+		return ecdsa.ParseUncompressedPublicKey(namedCurve, data)
-		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
 	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")

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) {
+	privateKey, err := key.Bytes()
-		return nil, errors.New("invalid elliptic key public key")
+	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)
+	size := (curve.Params().N.BitLen() + 7) / 8
-	curveOrder := curve.Params().N
+	privateKey := make([]byte, size)
-	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)
 
 	// 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)
 }

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) {
+		publicKeyBytes, err = pub.Bytes()
-			return nil, pkix.AlgorithmIdentifier{}, errors.New("x509: invalid elliptic curve public key")
+		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)