]> Cypherpunks repositories - gostls13.git/commitdiff
crypto/rsa: check for post-Precompute changes in Validate
authorFilippo Valsorda <filippo@golang.org>
Thu, 10 Jul 2025 15:46:48 +0000 (17:46 +0200)
committerGopher Robot <gobot@golang.org>
Tue, 9 Sep 2025 19:31:46 +0000 (12:31 -0700)
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 [new file with mode: 0644]
src/crypto/rsa/equal_test.go
src/crypto/rsa/rsa.go
src/crypto/rsa/rsa_test.go

diff --git a/doc/next/6-stdlib/99-minor/crypto/rsa/74115.md b/doc/next/6-stdlib/99-minor/crypto/rsa/74115.md
new file mode 100644 (file)
index 0000000..0cda968
--- /dev/null
@@ -0,0 +1,2 @@
+If [PrivateKey] fields are modified after calling [PrivateKey.Precompute],
+[PrivateKey.Validate] now fails.
index cf86e6c02421442ed454f04c75f993c6612e8f69..435429c3d156359d17a46b45f9d87ad4ad043523 100644 (file)
@@ -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)
+       }
 }
index c557c3710af7102276e06b7986d27d9d4b5f45fa..b6b94a79bbca51c22ad02009f52626dca742804f 100644 (file)
@@ -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
-       // with [crypto/x509.MarshalPKCS1PrivateKey], which unfortunately doesn't
-       // return an error.
+       // We can operate on keys based on d alone, but they can't be encoded with
+       // [crypto/x509.MarshalPKCS1PrivateKey], which unfortunately doesn't return
+       // 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
-       // [rsa.NewPrivateKey] or [rsa.NewPrivateKeyWithoutCRT].
-       if priv.Precomputed.fips != nil {
+       // If Precomputed.fips is set and consistent, then the key has been
+       // validated by [rsa.NewPrivateKey] or [rsa.NewPrivateKeyWithoutCRT].
+       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
-               // unmodified. Validate will re-run precompute.
+               // We don't have a way to report errors, so just leave Precomputed.fips
+               // nil. Validate will re-run precompute and report its error.
+               priv.Precomputed.fips = nil
                return
        }
        priv.Precomputed = precomputed
index 2f54deae6897252e8d148d3d0b4769a62f6740e1..e919479a10258802c3c0ba3bbde250bb61127d91 100644 (file)
@@ -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 {
-               t.Errorf("Validate() failed: %s", err)
+       validateErr := priv.Validate()
+       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 err != nil {
-               t.Errorf("MarshalPKCS8PrivateKey: %v", err)
-       }
-       key, err := x509.ParsePKCS8PrivateKey(der)
-       if err != nil {
-               t.Errorf("ParsePKCS8PrivateKey: %v", err)
-       }
-       if !key.(*PrivateKey).Equal(priv) {
-               t.Errorf("private key mismatch")
+       if validateErr == nil {
+               der, err := x509.MarshalPKCS8PrivateKey(priv)
+               if err != nil {
+                       t.Errorf("MarshalPKCS8PrivateKey: %v", err)
+               }
+               key, err := x509.ParsePKCS8PrivateKey(der)
+               if err != nil {
+                       t.Errorf("ParsePKCS8PrivateKey: %v", err)
+               }
+               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{
-                       PublicKey: test2048Key.PublicKey,
-                       D:         test2048Key.D,
-               })
+               benchmarkSignPKCS1v15(b, test2048KeyOnlyD)
        })
        b.Run("2048/noprecomp/Primes", func(b *testing.B) {
-               benchmarkSignPKCS1v15(b, &PrivateKey{
-                       PublicKey: test2048Key.PublicKey,
-                       D:         test2048Key.D,
-                       Primes:    test2048Key.Primes,
-               })
+               benchmarkSignPKCS1v15(b, test2048KeyWithoutPrecomputed)
        })
        // 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{
-                       PublicKey: test2048Key.PublicKey,
-                       D:         test2048Key.D,
-                       Primes:    test2048Key.Primes,
-                       Precomputed: PrecomputedValues{
-                               Dp:   test2048Key.Precomputed.Dp,
-                               Dq:   test2048Key.Precomputed.Dq,
-                               Qinv: test2048Key.Precomputed.Qinv,
-                       },
-               })
+               benchmarkSignPKCS1v15(b, test2048KeyWithPrecomputed)
        })
 }
 
@@ -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()")
+       }
+}