From: Filippo Valsorda <filippo@golang.org>
Date: Fri, 29 Nov 2024 14:38:48 +0000 (+0100)
Subject: crypto/x509: keep RSA CRT values in ParsePKCS1PrivateKey
X-Git-Tag: go1.24rc1~55
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=c5c4f3dd5f5e5a6a27fe53dc57eaf6acf414a4bc;p=gostls13.git

crypto/x509: keep RSA CRT values in ParsePKCS1PrivateKey

Turns out that recomputing them (and qInv in particular) in constant
time is expensive, so let's not throw them away when they are available.
They are much faster to check, so we now do that on precompute.

Also, thanks to the opaque crypto/internal/fips140/rsa.PrivateKey type,
we now have some assurance that the values we use are always ones we
checked.

Recovers most of the performance loss since CL 630516 in the happy path.
Also, since now we always use the CRT, if necessary by running a
throwaway Precompute, which is now cheap if PrecomputedValues is filled
out, we effectively fixed the JSON round-trip slowdown (#59695).

goos: darwin
goarch: arm64
pkg: crypto/rsa
cpu: Apple M2
                            │ 3b42687c56  │          f017604bc6-dirty           │
                            │   sec/op    │   sec/op     vs base                │
ParsePKCS8PrivateKey/2048-8   26.76µ ± 1%   65.99µ ± 1%  +146.64% (p=0.002 n=6)

Fixes #59695
Updates #69799
For #69536

Change-Id: I507f8c5a32e69ab28990a3bf78959836b9b08cc9
Reviewed-on: https://go-review.googlesource.com/c/go/+/632478
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
---

diff --git a/doc/godebug.md b/doc/godebug.md
index 15918bdd59..1b5674f2cd 100644
--- a/doc/godebug.md
+++ b/doc/godebug.md
@@ -208,6 +208,11 @@ X25519MLKEM768 by default. The default can be reverted using the
 [`tlsmlkem` setting](/pkg/crypto/tls/#Config.CurvePreferences).
 Go 1.24 also removed X25519Kyber768Draft00 and the Go 1.23 `tlskyber` setting.
 
+Go 1.24 made [`ParsePKCS1PrivateKey`](/pkg/crypto/x509/#ParsePKCS1PrivateKey)
+use and validate the CRT parameters in the encoded private key. This behavior
+can be controlled with the `x509rsacrt` setting. Using `x509rsacrt=0` restores
+the Go 1.23 behavior.
+
 ### Go 1.23
 
 Go 1.23 changed the channels created by package time to be unbuffered
diff --git a/doc/next/6-stdlib/99-minor/crypto/x509/69799.md b/doc/next/6-stdlib/99-minor/crypto/x509/69799.md
index 6eb000b360..2197168365 100644
--- a/doc/next/6-stdlib/99-minor/crypto/x509/69799.md
+++ b/doc/next/6-stdlib/99-minor/crypto/x509/69799.md
@@ -1,3 +1,7 @@
 [MarshalPKCS8PrivateKey] now returns an error instead of marshaling an invalid
 RSA key. ([MarshalPKCS1PrivateKey] doesn't have an error return, and its behavior
 when provided invalid keys continues to be undefined.)
+
+[ParsePKCS1PrivateKey] and [ParsePKCS8PrivateKey] now use and validate the
+encoded CRT values, so might reject invalid keys that were previously accepted.
+Use `GODEBUG=x509rsacrt=0` to revert to recomputing them.
diff --git a/src/crypto/internal/fips140/rsa/rsa.go b/src/crypto/internal/fips140/rsa/rsa.go
index 59951f838b..8803599f02 100644
--- a/src/crypto/internal/fips140/rsa/rsa.go
+++ b/src/crypto/internal/fips140/rsa/rsa.go
@@ -104,6 +104,43 @@ func newPrivateKey(n *bigmod.Modulus, e int, d *bigmod.Nat, p, q *bigmod.Modulus
 	return pk, nil
 }
 
+// NewPrivateKeyWithPrecomputation creates a new RSA private key from the given
+// parameters, which include precomputed CRT values.
+func NewPrivateKeyWithPrecomputation(N []byte, e int, d, P, Q, dP, dQ, qInv []byte) (*PrivateKey, error) {
+	n, err := bigmod.NewModulus(N)
+	if err != nil {
+		return nil, err
+	}
+	p, err := bigmod.NewModulus(P)
+	if err != nil {
+		return nil, err
+	}
+	q, err := bigmod.NewModulus(Q)
+	if err != nil {
+		return nil, err
+	}
+	dN, err := bigmod.NewNat().SetBytes(d, n)
+	if err != nil {
+		return nil, err
+	}
+	qInvNat, err := bigmod.NewNat().SetBytes(qInv, p)
+	if err != nil {
+		return nil, err
+	}
+
+	pk := &PrivateKey{
+		pub: PublicKey{
+			N: n, E: e,
+		},
+		d: dN, p: p, q: q,
+		dP: dP, dQ: dQ, qInv: qInvNat,
+	}
+	if err := checkPrivateKey(pk); err != nil {
+		return nil, err
+	}
+	return pk, nil
+}
+
 // NewPrivateKeyWithoutCRT creates a new RSA private key from the given parameters.
 //
 // This is meant for deprecated multi-prime keys, and is not FIPS 140 compliant.
@@ -157,37 +194,64 @@ func checkPrivateKey(priv *PrivateKey) error {
 	}
 
 	N := priv.pub.N
-	Π := bigmod.NewNat().ExpandFor(N)
-	for _, prime := range []*bigmod.Modulus{priv.p, priv.q} {
-		p := prime.Nat().ExpandFor(N)
-		if p.IsZero() == 1 || p.IsOne() == 1 {
-			return errors.New("crypto/rsa: invalid prime")
-		}
-		Π.Mul(p, N)
+	p := priv.p
+	q := priv.q
 
-		// Check that de ≡ 1 mod p-1, for each prime.
-		// This implies that e is coprime to each p-1 as e has a multiplicative
-		// inverse. Therefore e is coprime to lcm(p-1,q-1,r-1,...) =
-		// exponent(ℤ/nℤ). It also implies that a^de ≡ a mod p as a^(p-1) ≡ 1
-		// mod p. Thus a^de ≡ a mod n for all a coprime to n, as required.
-
-		pMinus1, err := bigmod.NewModulus(p.SubOne(N).Bytes(N))
-		if err != nil {
-			return errors.New("crypto/rsa: invalid prime")
-		}
+	// Check that pq ≡ 1 mod N (and that pN < N and q < N).
+	pN := bigmod.NewNat().ExpandFor(N)
+	if _, err := pN.SetBytes(p.Nat().Bytes(p), N); err != nil {
+		return errors.New("crypto/rsa: invalid prime")
+	}
+	qN := bigmod.NewNat().ExpandFor(N)
+	if _, err := qN.SetBytes(q.Nat().Bytes(q), N); err != nil {
+		return errors.New("crypto/rsa: invalid prime")
+	}
+	if pN.Mul(qN, N).IsZero() != 1 {
+		return errors.New("crypto/rsa: p * q != n")
+	}
 
-		e := bigmod.NewNat().SetUint(uint(priv.pub.E)).ExpandFor(pMinus1)
+	// Check that de ≡ 1 mod p-1, and de ≡ 1 mod q-1.
+	//
+	// This implies that e is coprime to each p-1 as e has a multiplicative
+	// inverse. Therefore e is coprime to lcm(p-1,q-1,r-1,...) = exponent(ℤ/nℤ).
+	// It also implies that a^de ≡ a mod p as a^(p-1) ≡ 1 mod p. Thus a^de ≡ a
+	// mod n for all a coprime to n, as required.
+	//
+	// This checks dP, dQ, and e. We don't check d because it is not actually
+	// used in the RSA private key operation.
+	pMinus1, err := bigmod.NewModulus(p.Nat().SubOne(p).Bytes(p))
+	if err != nil {
+		return errors.New("crypto/rsa: invalid prime")
+	}
+	dP, err := bigmod.NewNat().SetBytes(priv.dP, pMinus1)
+	if err != nil {
+		return errors.New("crypto/rsa: invalid CRT exponent")
+	}
+	de := bigmod.NewNat()
+	de.SetUint(uint(priv.pub.E)).ExpandFor(pMinus1)
+	de.Mul(dP, pMinus1)
+	if de.IsOne() != 1 {
+		return errors.New("crypto/rsa: invalid CRT exponent")
+	}
 
-		de := bigmod.NewNat()
-		de.Mod(priv.d, pMinus1)
-		de.Mul(e, pMinus1)
-		if de.IsOne() != 1 {
-			return errors.New("crypto/rsa: invalid exponents")
-		}
+	qMinus1, err := bigmod.NewModulus(q.Nat().SubOne(q).Bytes(q))
+	if err != nil {
+		return errors.New("crypto/rsa: invalid prime")
+	}
+	dQ, err := bigmod.NewNat().SetBytes(priv.dQ, qMinus1)
+	if err != nil {
+		return errors.New("crypto/rsa: invalid CRT exponent")
+	}
+	de.SetUint(uint(priv.pub.E)).ExpandFor(qMinus1)
+	de.Mul(dQ, qMinus1)
+	if de.IsOne() != 1 {
+		return errors.New("crypto/rsa: invalid CRT exponent")
 	}
-	// Check that Πprimes == n.
-	if Π.IsZero() != 1 {
-		return errors.New("crypto/rsa: invalid modulus")
+
+	// Check that qInv * q ≡ 1 mod p.
+	one := q.Nat().Mul(priv.qInv, p)
+	if one.IsOne() != 1 {
+		return errors.New("crypto/rsa: invalid CRT coefficient")
 	}
 
 	return nil
diff --git a/src/crypto/rsa/rsa.go b/src/crypto/rsa/rsa.go
index 38fbfce9a3..e4591fd5e2 100644
--- a/src/crypto/rsa/rsa.go
+++ b/src/crypto/rsa/rsa.go
@@ -519,6 +519,20 @@ func (priv *PrivateKey) precompute() (PrecomputedValues, error) {
 		return precomputed, errors.New("crypto/rsa: prime Q is nil")
 	}
 
+	// If the CRT values are already set, use them.
+	if priv.Precomputed.Dp != nil && priv.Precomputed.Dq != nil && priv.Precomputed.Qinv != nil {
+		k, err := rsa.NewPrivateKeyWithPrecomputation(priv.N.Bytes(), priv.E, priv.D.Bytes(),
+			priv.Primes[0].Bytes(), priv.Primes[1].Bytes(),
+			priv.Precomputed.Dp.Bytes(), priv.Precomputed.Dq.Bytes(), priv.Precomputed.Qinv.Bytes())
+		if err != nil {
+			return precomputed, err
+		}
+		precomputed = priv.Precomputed
+		precomputed.fips = k
+		precomputed.CRTValues = make([]CRTValue, 0)
+		return precomputed, nil
+	}
+
 	k, err := rsa.NewPrivateKey(priv.N.Bytes(), priv.E, priv.D.Bytes(),
 		priv.Primes[0].Bytes(), priv.Primes[1].Bytes())
 	if err != nil {
diff --git a/src/crypto/x509/pkcs1.go b/src/crypto/x509/pkcs1.go
index 7929867ac6..ca23358c8c 100644
--- a/src/crypto/x509/pkcs1.go
+++ b/src/crypto/x509/pkcs1.go
@@ -8,6 +8,7 @@ import (
 	"crypto/rsa"
 	"encoding/asn1"
 	"errors"
+	"internal/godebug"
 	"math/big"
 )
 
@@ -19,10 +20,9 @@ type pkcs1PrivateKey struct {
 	D       *big.Int
 	P       *big.Int
 	Q       *big.Int
-	// We ignore these values, if present, because rsa will calculate them.
-	Dp   *big.Int `asn1:"optional"`
-	Dq   *big.Int `asn1:"optional"`
-	Qinv *big.Int `asn1:"optional"`
+	Dp      *big.Int `asn1:"optional"`
+	Dq      *big.Int `asn1:"optional"`
+	Qinv    *big.Int `asn1:"optional"`
 
 	AdditionalPrimes []pkcs1AdditionalRSAPrime `asn1:"optional,omitempty"`
 }
@@ -41,9 +41,16 @@ type pkcs1PublicKey struct {
 	E int
 }
 
+// x509rsacrt, if zero, makes ParsePKCS1PrivateKey ignore and recompute invalid
+// CRT values in the RSA private key.
+var x509rsacrt = godebug.New("x509rsacrt")
+
 // ParsePKCS1PrivateKey parses an [RSA] private key in PKCS #1, ASN.1 DER form.
 //
 // This kind of key is commonly encoded in PEM blocks of type "RSA PRIVATE KEY".
+//
+// Before Go 1.24, the CRT parameters were ignored and recomputed. To restore
+// the old behavior, use the GODEBUG=x509rsacrt=0 environment variable.
 func ParsePKCS1PrivateKey(der []byte) (*rsa.PrivateKey, error) {
 	var priv pkcs1PrivateKey
 	rest, err := asn1.Unmarshal(der, &priv)
@@ -64,7 +71,8 @@ func ParsePKCS1PrivateKey(der []byte) (*rsa.PrivateKey, error) {
 		return nil, errors.New("x509: unsupported private key version")
 	}
 
-	if priv.N.Sign() <= 0 || priv.D.Sign() <= 0 || priv.P.Sign() <= 0 || priv.Q.Sign() <= 0 {
+	if priv.N.Sign() <= 0 || priv.D.Sign() <= 0 || priv.P.Sign() <= 0 || priv.Q.Sign() <= 0 ||
+		priv.Dp.Sign() <= 0 || priv.Dq.Sign() <= 0 || priv.Qinv.Sign() <= 0 {
 		return nil, errors.New("x509: private key contains zero or negative value")
 	}
 
@@ -78,6 +86,9 @@ func ParsePKCS1PrivateKey(der []byte) (*rsa.PrivateKey, error) {
 	key.Primes = make([]*big.Int, 2+len(priv.AdditionalPrimes))
 	key.Primes[0] = priv.P
 	key.Primes[1] = priv.Q
+	key.Precomputed.Dp = priv.Dp
+	key.Precomputed.Dq = priv.Dq
+	key.Precomputed.Qinv = priv.Qinv
 	for i, a := range priv.AdditionalPrimes {
 		if a.Prime.Sign() <= 0 {
 			return nil, errors.New("x509: private key contains zero or negative prime")
@@ -89,6 +100,19 @@ func ParsePKCS1PrivateKey(der []byte) (*rsa.PrivateKey, error) {
 
 	key.Precompute()
 	if err := key.Validate(); err != nil {
+		// If x509rsacrt=0 is set, try dropping the CRT values and
+		// rerunning precomputation and key validation.
+		if x509rsacrt.Value() == "0" {
+			key.Precomputed.Dp = nil
+			key.Precomputed.Dq = nil
+			key.Precomputed.Qinv = nil
+			key.Precompute()
+			if err := key.Validate(); err == nil {
+				x509rsacrt.IncNonDefault()
+				return key, nil
+			}
+		}
+
 		return nil, err
 	}
 
diff --git a/src/crypto/x509/pkcs8.go b/src/crypto/x509/pkcs8.go
index 6268c36757..d0ab573ff3 100644
--- a/src/crypto/x509/pkcs8.go
+++ b/src/crypto/x509/pkcs8.go
@@ -32,6 +32,9 @@ type pkcs8 struct {
 // in the future.
 //
 // This kind of key is commonly encoded in PEM blocks of type "PRIVATE KEY".
+//
+// Before Go 1.24, the CRT parameters of RSA keys were ignored and recomputed.
+// To restore the old behavior, use the GODEBUG=x509rsacrt=0 environment variable.
 func ParsePKCS8PrivateKey(der []byte) (key any, err error) {
 	var privKey pkcs8
 	if _, err := asn1.Unmarshal(der, &privKey); err != nil {
diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 1a714e7b62..941ea572e6 100644
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -253,6 +253,61 @@ func TestMarshalRSAPrivateKey(t *testing.T) {
 		priv.Primes[2].Cmp(priv2.Primes[2]) != 0 {
 		t.Errorf("wrong priv:\ngot  %+v\nwant %+v", priv2, priv)
 	}
+
+	if priv.Precomputed.Dp == nil {
+		t.Fatalf("Precomputed.Dp is nil")
+	}
+}
+
+func TestMarshalRSAPrivateKeyInvalid(t *testing.T) {
+	block, _ := pem.Decode([]byte(strings.ReplaceAll(
+		`-----BEGIN RSA TESTING KEY-----
+MIIEowIBAAKCAQEAsPnoGUOnrpiSqt4XynxA+HRP7S+BSObI6qJ7fQAVSPtRkqso
+tWxQYLEYzNEx5ZSHTGypibVsJylvCfuToDTfMul8b/CZjP2Ob0LdpYrNH6l5hvFE
+89FU1nZQF15oVLOpUgA7wGiHuEVawrGfey92UE68mOyUVXGweJIVDdxqdMoPvNNU
+l86BU02vlBiESxOuox+dWmuVV7vfYZ79Toh/LUK43YvJh+rhv4nKuF7iHjVjBd9s
+B6iDjj70HFldzOQ9r8SRI+9NirupPTkF5AKNe6kUhKJ1luB7S27ZkvB3tSTT3P59
+3VVJvnzOjaA1z6Cz+4+eRvcysqhrRgFlwI9TEwIDAQABAoIBAEEYiyDP29vCzx/+
+dS3LqnI5BjUuJhXUnc6AWX/PCgVAO+8A+gZRgvct7PtZb0sM6P9ZcLrweomlGezI
+FrL0/6xQaa8bBr/ve/a8155OgcjFo6fZEw3Dz7ra5fbSiPmu4/b/kvrg+Br1l77J
+aun6uUAs1f5B9wW+vbR7tzbT/mxaUeDiBzKpe15GwcvbJtdIVMa2YErtRjc1/5B2
+BGVXyvlJv0SIlcIEMsHgnAFOp1ZgQ08aDzvilLq8XVMOahAhP1O2A3X8hKdXPyrx
+IVWE9bS9ptTo+eF6eNl+d7htpKGEZHUxinoQpWEBTv+iOoHsVunkEJ3vjLP3lyI/
+fY0NQ1ECgYEA3RBXAjgvIys2gfU3keImF8e/TprLge1I2vbWmV2j6rZCg5r/AS0u
+pii5CvJ5/T5vfJPNgPBy8B/yRDs+6PJO1GmnlhOkG9JAIPkv0RBZvR0PMBtbp6nT
+Y3yo1lwamBVBfY6rc0sLTzosZh2aGoLzrHNMQFMGaauORzBFpY5lU50CgYEAzPHl
+u5DI6Xgep1vr8QvCUuEesCOgJg8Yh1UqVoY/SmQh6MYAv1I9bLGwrb3WW/7kqIoD
+fj0aQV5buVZI2loMomtU9KY5SFIsPV+JuUpy7/+VE01ZQM5FdY8wiYCQiVZYju9X
+Wz5LxMNoz+gT7pwlLCsC4N+R8aoBk404aF1gum8CgYAJ7VTq7Zj4TFV7Soa/T1eE
+k9y8a+kdoYk3BASpCHJ29M5R2KEA7YV9wrBklHTz8VzSTFTbKHEQ5W5csAhoL5Fo
+qoHzFFi3Qx7MHESQb9qHyolHEMNx6QdsHUn7rlEnaTTyrXh3ifQtD6C0yTmFXUIS
+CW9wKApOrnyKJ9nI0HcuZQKBgQCMtoV6e9VGX4AEfpuHvAAnMYQFgeBiYTkBKltQ
+XwozhH63uMMomUmtSG87Sz1TmrXadjAhy8gsG6I0pWaN7QgBuFnzQ/HOkwTm+qKw
+AsrZt4zeXNwsH7QXHEJCFnCmqw9QzEoZTrNtHJHpNboBuVnYcoueZEJrP8OnUG3r
+UjmopwKBgAqB2KYYMUqAOvYcBnEfLDmyZv9BTVNHbR2lKkMYqv5LlvDaBxVfilE0
+2riO4p6BaAdvzXjKeRrGNEKoHNBpOSfYCOM16NjL8hIZB1CaV3WbT5oY+jp7Mzd5
+7d56RZOE+ERK2uz/7JX9VSsM/LbH9pJibd4e8mikDS9ntciqOH/3
+-----END RSA TESTING KEY-----`, "TESTING KEY", "PRIVATE KEY")))
+	testRSA2048, _ := ParsePKCS1PrivateKey(block.Bytes)
+
+	broken := *testRSA2048
+	broken.Precomputed.Dp = new(big.Int).SetUint64(42)
+
+	parsed, err := ParsePKCS1PrivateKey(MarshalPKCS1PrivateKey(&broken))
+	if err == nil {
+		t.Errorf("expected error, got success")
+	}
+
+	t.Setenv("GODEBUG", "x509rsacrt=0")
+
+	parsed, err = ParsePKCS1PrivateKey(MarshalPKCS1PrivateKey(&broken))
+	if err != nil {
+		t.Fatalf("expected success, got error: %v", err)
+	}
+	// Dp should have been recomputed.
+	if parsed.Precomputed.Dp.Cmp(testRSA2048.Precomputed.Dp) != 0 {
+		t.Errorf("Dp recomputation failed: got %v, want %v", parsed.Precomputed.Dp, testRSA2048.Precomputed.Dp)
+	}
 }
 
 func TestMarshalRSAPublicKey(t *testing.T) {
diff --git a/src/internal/godebugs/table.go b/src/internal/godebugs/table.go
index 852afaabce..9c48a923f0 100644
--- a/src/internal/godebugs/table.go
+++ b/src/internal/godebugs/table.go
@@ -62,6 +62,7 @@ var All = []Info{
 	{Name: "winsymlink", Package: "os", Changed: 22, Old: "0"},
 	{Name: "x509keypairleaf", Package: "crypto/tls", Changed: 23, Old: "0"},
 	{Name: "x509negativeserial", Package: "crypto/x509", Changed: 23, Old: "1"},
+	{Name: "x509rsacrt", Package: "crypto/x509", Changed: 24, Old: "0"},
 	{Name: "x509usefallbackroots", Package: "crypto/x509"},
 	{Name: "x509usepolicies", Package: "crypto/x509", Changed: 24, Old: "0"},
 	{Name: "zipinsecurepath", Package: "archive/zip"},
diff --git a/src/runtime/metrics/doc.go b/src/runtime/metrics/doc.go
index 34a1b01fe4..563ddf4c95 100644
--- a/src/runtime/metrics/doc.go
+++ b/src/runtime/metrics/doc.go
@@ -362,6 +362,10 @@ Below is the full list of supported metrics, ordered lexicographically.
 		package due to a non-default GODEBUG=x509negativeserial=...
 		setting.
 
+	/godebug/non-default-behavior/x509rsacrt:events
+		The number of non-default behaviors executed by the crypto/x509
+		package due to a non-default GODEBUG=x509rsacrt=... setting.
+
 	/godebug/non-default-behavior/x509usefallbackroots:events
 		The number of non-default behaviors executed by the crypto/x509
 		package due to a non-default GODEBUG=x509usefallbackroots=...