crypto/rsa: check for post-Precompute changes in Validate

Updates #74115

Change-Id: I6a6a6964be55cff5131d99235f621b4ff2a93d2b
Reviewed-on: https://go-review.googlesource.com/c/go/+/687835
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Mark Freeman <markfreeman@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Daniel McCarney <daniel@binaryparadox.net>
Auto-Submit: Filippo Valsorda <filippo@golang.org>

doc/next/6-stdlib/99-minor/crypto/rsa/74115.md

@@ -0,0 +1,2 @@
+If [PrivateKey] fields are modified after calling [PrivateKey.Precompute],
+[PrivateKey.Validate] now fails.

src/crypto/rsa/equal_test.go

@@ -49,4 +49,10 @@ func TestEqual(t *testing.T) {
 	if private.Equal(other) {
 		t.Errorf("different private keys are Equal")
 	}
+
+	noPrecomp := *private
+	noPrecomp.Precomputed = rsa.PrecomputedValues{}
+	if !private.Equal(&noPrecomp) {
+		t.Errorf("private key with no precomputation is not equal to itself: %v", private)
+	}
 }

src/crypto/rsa/rsa.go

@@ -103,7 +103,10 @@ type OAEPOptions struct {
 	Label []byte
 }
 
-// A PrivateKey represents an RSA key
+// A PrivateKey represents an RSA key.
+//
+// Its fields must not be modified after calling [PrivateKey.Precompute], and
+// should not be used directly as big.Int values for cryptographic purposes.
 type PrivateKey struct {
 	PublicKey            // public part.
 	D         *big.Int   // private exponent
@@ -111,7 +114,7 @@ type PrivateKey struct {
 
 	// Precomputed contains precomputed values that speed up RSA operations,
 	// if available. It must be generated by calling PrivateKey.Precompute and
-	// must not be modified.
+	// must not be modified afterwards.
 	Precomputed PrecomputedValues
 }
 
@@ -228,21 +231,52 @@ type CRTValue struct {
 //
 // It runs faster on valid keys if run after [PrivateKey.Precompute].
 func (priv *PrivateKey) Validate() error {
-	// We can operate on keys based on d alone, but it isn't possible to encode
+	// We can operate on keys based on d alone, but they can't be encoded with
-	// with [crypto/x509.MarshalPKCS1PrivateKey], which unfortunately doesn't
+	// [crypto/x509.MarshalPKCS1PrivateKey], which unfortunately doesn't return
-	// return an error.
+	// an error, so we need to reject them here.
 	if len(priv.Primes) < 2 {
 		return errors.New("crypto/rsa: missing primes")
 	}
-	// If Precomputed.fips is set, then the key has been validated by
+	// If Precomputed.fips is set and consistent, then the key has been
-	// [rsa.NewPrivateKey] or [rsa.NewPrivateKeyWithoutCRT].
+	// validated by [rsa.NewPrivateKey] or [rsa.NewPrivateKeyWithoutCRT].
-	if priv.Precomputed.fips != nil {
+	if priv.precomputedIsConsistent() {
 		return nil
 	}
+	if priv.Precomputed.fips != nil {
+		return errors.New("crypto/rsa: precomputed values are inconsistent with the key")
+	}
 	_, err := priv.precompute()
 	return err
 }
 
+func (priv *PrivateKey) precomputedIsConsistent() bool {
+	if priv.Precomputed.fips == nil {
+		return false
+	}
+	N, e, d, P, Q, dP, dQ, qInv := priv.Precomputed.fips.Export()
+	if !bigIntEqualToBytes(priv.N, N) || priv.E != e || !bigIntEqualToBytes(priv.D, d) {
+		return false
+	}
+	if len(priv.Primes) != 2 {
+		return P == nil && Q == nil && dP == nil && dQ == nil && qInv == nil
+	}
+	return bigIntEqualToBytes(priv.Primes[0], P) &&
+		bigIntEqualToBytes(priv.Primes[1], Q) &&
+		bigIntEqualToBytes(priv.Precomputed.Dp, dP) &&
+		bigIntEqualToBytes(priv.Precomputed.Dq, dQ) &&
+		bigIntEqualToBytes(priv.Precomputed.Qinv, qInv)
+}
+
+// bigIntEqual reports whether a and b are equal, ignoring leading zero bytes in
+// b, and leaking only their bit length through timing side-channels.
+func bigIntEqualToBytes(a *big.Int, b []byte) bool {
+	if a == nil || a.BitLen() > len(b)*8 {
+		return false
+	}
+	buf := a.FillBytes(make([]byte, len(b)))
+	return subtle.ConstantTimeCompare(buf, b) == 1
+}
+
 // rsa1024min is a GODEBUG that re-enables weak RSA keys if set to "0".
 // See https://go.dev/issue/68762.
 var rsa1024min = godebug.New("rsa1024min")
@@ -505,14 +539,15 @@ var ErrVerification = errors.New("crypto/rsa: verification error")
 // Precompute performs some calculations that speed up private key operations
 // in the future. It is safe to run on non-validated private keys.
 func (priv *PrivateKey) Precompute() {
-	if priv.Precomputed.fips != nil {
+	if priv.precomputedIsConsistent() {
 		return
 	}
 
 	precomputed, err := priv.precompute()
 	if err != nil {
-		// We don't have a way to report errors, so just leave the key
+		// We don't have a way to report errors, so just leave Precomputed.fips
-		// unmodified. Validate will re-run precompute.
+		// nil. Validate will re-run precompute and report its error.
+		priv.Precomputed.fips = nil
 		return
 	}
 	priv.Precomputed = precomputed

src/crypto/rsa/rsa_test.go

@@ -23,6 +23,7 @@ import (
 	"io"
 	"math/big"
 	"os"
+	"slices"
 	"strings"
 	"testing"
 )
@@ -226,8 +227,9 @@ func TestEverything(t *testing.T) {
 }
 
 func testEverything(t *testing.T, priv *PrivateKey) {
-	if err := priv.Validate(); err != nil {
+	validateErr := priv.Validate()
-		t.Errorf("Validate() failed: %s", err)
+	if validateErr != nil && len(priv.Primes) >= 2 {
+		t.Errorf("Validate() failed: %s", validateErr)
 	}
 
 	msg := []byte("test")
@@ -373,19 +375,21 @@ func testEverything(t *testing.T, priv *PrivateKey) {
 		t.Errorf("DecryptPKCS1v15 accepted a long ciphertext")
 	}
 
-	der, err := x509.MarshalPKCS8PrivateKey(priv)
+	if validateErr == nil {
-	if err != nil {
+		der, err := x509.MarshalPKCS8PrivateKey(priv)
-		t.Errorf("MarshalPKCS8PrivateKey: %v", err)
+		if err != nil {
-	}
+			t.Errorf("MarshalPKCS8PrivateKey: %v", err)
-	key, err := x509.ParsePKCS8PrivateKey(der)
+		}
-	if err != nil {
+		key, err := x509.ParsePKCS8PrivateKey(der)
-		t.Errorf("ParsePKCS8PrivateKey: %v", err)
+		if err != nil {
-	}
+			t.Errorf("ParsePKCS8PrivateKey: %v", err)
-	if !key.(*PrivateKey).Equal(priv) {
+		}
-		t.Errorf("private key mismatch")
+		if !key.(*PrivateKey).Equal(priv) {
+			t.Errorf("private key mismatch")
+		}
 	}
 
-	der, err = x509.MarshalPKIXPublicKey(&priv.PublicKey)
+	der, err := x509.MarshalPKIXPublicKey(&priv.PublicKey)
 	if err != nil {
 		t.Errorf("MarshalPKIXPublicKey: %v", err)
 	}
@@ -517,6 +521,30 @@ fLVGuFoTVIu2bF0cWAjNNMg=
 
 var test2048Key = parseKey(test2048KeyPEM)
 
+var test2048KeyOnlyD = &PrivateKey{
+	PublicKey: test2048Key.PublicKey,
+	D:         test2048Key.D,
+}
+
+var test2048KeyWithoutPrecomputed = &PrivateKey{
+	PublicKey: test2048Key.PublicKey,
+	D:         test2048Key.D,
+	Primes:    test2048Key.Primes,
+}
+
+// test2048KeyWithPrecomputed is different from test2048Key because it includes
+// only the public precomputed values, and not the fips140/rsa.PrivateKey.
+var test2048KeyWithPrecomputed = &PrivateKey{
+	PublicKey: test2048Key.PublicKey,
+	D:         test2048Key.D,
+	Primes:    test2048Key.Primes,
+	Precomputed: PrecomputedValues{
+		Dp:   test2048Key.Precomputed.Dp,
+		Dq:   test2048Key.Precomputed.Dq,
+		Qinv: test2048Key.Precomputed.Qinv,
+	},
+}
+
 var test3072Key = parseKey(testingKey(`-----BEGIN TESTING KEY-----
 MIIG/gIBADANBgkqhkiG9w0BAQEFAASCBugwggbkAgEAAoIBgQDJrvevql7G07LM
 xQAwAA1Oo8qUAkWfmpgrpxIUZE1QTyMCDaspQJGBBR2+iStrzi2NnWvyBz3jJWFZ
@@ -700,31 +728,15 @@ func BenchmarkEncryptOAEP(b *testing.B) {
 func BenchmarkSignPKCS1v15(b *testing.B) {
 	b.Run("2048", func(b *testing.B) { benchmarkSignPKCS1v15(b, test2048Key) })
 	b.Run("2048/noprecomp/OnlyD", func(b *testing.B) {
-		benchmarkSignPKCS1v15(b, &PrivateKey{
+		benchmarkSignPKCS1v15(b, test2048KeyOnlyD)
-			PublicKey: test2048Key.PublicKey,
-			D:         test2048Key.D,
-		})
 	})
 	b.Run("2048/noprecomp/Primes", func(b *testing.B) {
-		benchmarkSignPKCS1v15(b, &PrivateKey{
+		benchmarkSignPKCS1v15(b, test2048KeyWithoutPrecomputed)
-			PublicKey: test2048Key.PublicKey,
-			D:         test2048Key.D,
-			Primes:    test2048Key.Primes,
-		})
 	})
 	// This is different from "2048" because it's only the public precomputed
 	// values, and not the crypto/internal/fips140/rsa.PrivateKey.
 	b.Run("2048/noprecomp/AllValues", func(b *testing.B) {
-		benchmarkSignPKCS1v15(b, &PrivateKey{
+		benchmarkSignPKCS1v15(b, test2048KeyWithPrecomputed)
-			PublicKey: test2048Key.PublicKey,
-			D:         test2048Key.D,
-			Primes:    test2048Key.Primes,
-			Precomputed: PrecomputedValues{
-				Dp:   test2048Key.Precomputed.Dp,
-				Dq:   test2048Key.Precomputed.Dq,
-				Qinv: test2048Key.Precomputed.Qinv,
-			},
-		})
 	})
 }
 
@@ -1188,3 +1200,120 @@ cHPGX1uUDRAU1xxtpVQ0OqXyEgqwz6y6hYRw
 	testEverything(t, k1)
 	testEverything(t, k2)
 }
+
+func TestNotPrecomputed(t *testing.T) {
+	t.Run("OnlyD", func(t *testing.T) {
+		testEverything(t, test2048KeyOnlyD)
+		k := *test2048KeyOnlyD
+		k.Precompute()
+		testEverything(t, &k)
+	})
+	t.Run("Primes", func(t *testing.T) {
+		testEverything(t, test2048KeyWithoutPrecomputed)
+		k := *test2048KeyWithoutPrecomputed
+		k.Precompute()
+		if k.Precomputed.Dp == nil || k.Precomputed.Dq == nil || k.Precomputed.Qinv == nil {
+			t.Error("Precomputed values should not be nil after Precompute()")
+		}
+		testEverything(t, &k)
+	})
+	t.Run("AllValues", func(t *testing.T) {
+		testEverything(t, test2048KeyWithPrecomputed)
+		k := *test2048KeyWithoutPrecomputed
+		k.Precompute()
+		if k.Precomputed.Dp == nil || k.Precomputed.Dq == nil || k.Precomputed.Qinv == nil {
+			t.Error("Precomputed values should not be nil after Precompute()")
+		}
+		testEverything(t, &k)
+	})
+}
+
+func TestModifiedPrivateKey(t *testing.T) {
+	if test512Key.Validate() != nil {
+		t.Fatal("test512Key should be valid")
+	}
+
+	t.Run("PublicKey mismatch", func(t *testing.T) {
+		testModifiedPrivateKey(t, func(k *PrivateKey) {
+			k.PublicKey = test512KeyTwo.PublicKey
+		})
+	})
+	t.Run("Precomputed mismatch", func(t *testing.T) {
+		testModifiedPrivateKey(t, func(k *PrivateKey) {
+			k.Precomputed = test512KeyTwo.Precomputed
+		})
+	})
+
+	t.Run("N+2", func(t *testing.T) {
+		testModifiedPrivateKey(t, func(k *PrivateKey) {
+			k.N = new(big.Int).Add(k.N, big.NewInt(2))
+		})
+	})
+	t.Run("N=0", func(t *testing.T) {
+		testModifiedPrivateKey(t, func(k *PrivateKey) {
+			k.N = new(big.Int)
+		})
+	})
+	t.Run("N is nil", func(t *testing.T) {
+		testModifiedPrivateKey(t, func(k *PrivateKey) {
+			k.N = nil
+		})
+	})
+
+	t.Run("P+2", func(t *testing.T) {
+		testModifiedPrivateKey(t, func(k *PrivateKey) {
+			k.Primes[0] = new(big.Int).Add(k.Primes[0], big.NewInt(2))
+		})
+	})
+	t.Run("P=0", func(t *testing.T) {
+		testModifiedPrivateKey(t, func(k *PrivateKey) {
+			k.Primes[0] = new(big.Int)
+		})
+	})
+	t.Run("P is nil", func(t *testing.T) {
+		testModifiedPrivateKey(t, func(k *PrivateKey) {
+			k.Primes[0] = nil
+		})
+	})
+
+	t.Run("Q+2", func(t *testing.T) {
+		testModifiedPrivateKey(t, func(k *PrivateKey) {
+			k.Primes[1] = new(big.Int).Add(k.Primes[1], big.NewInt(2))
+		})
+	})
+	t.Run("Q=0", func(t *testing.T) {
+		testModifiedPrivateKey(t, func(k *PrivateKey) {
+			k.Primes[1] = new(big.Int)
+		})
+	})
+	t.Run("Q is nil", func(t *testing.T) {
+		testModifiedPrivateKey(t, func(k *PrivateKey) {
+			k.Primes[1] = nil
+		})
+	})
+
+	t.Run("E+2", func(t *testing.T) {
+		testModifiedPrivateKey(t, func(k *PrivateKey) {
+			k.E += 2
+		})
+	})
+	t.Run("E=0", func(t *testing.T) {
+		testModifiedPrivateKey(t, func(k *PrivateKey) {
+			k.E = 0
+		})
+	})
+}
+
+func testModifiedPrivateKey(t *testing.T, f func(*PrivateKey)) {
+	k := new(PrivateKey)
+	*k = *test512Key
+	k.Primes = slices.Clone(k.Primes)
+	f(k)
+	if err := k.Validate(); err == nil {
+		t.Error("Validate should have failed")
+	}
+	k.Precompute()
+	if err := k.Validate(); err == nil {
+		t.Error("Validate should have failed after Precompute()")
+	}
+}