From: Filippo Valsorda Date: Thu, 10 Jul 2025 15:46:48 +0000 (+0200) Subject: crypto/rsa: check for post-Precompute changes in Validate X-Git-Tag: go1.26rc1~928 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=5d9d0513dc;p=gostls13.git 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 Reviewed-by: Mark Freeman Reviewed-by: Michael Pratt Reviewed-by: Daniel McCarney Auto-Submit: Filippo Valsorda --- 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 index 0000000000..0cda968663 --- /dev/null +++ b/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. diff --git a/src/crypto/rsa/equal_test.go b/src/crypto/rsa/equal_test.go index cf86e6c024..435429c3d1 100644 --- a/src/crypto/rsa/equal_test.go +++ b/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) + } } diff --git a/src/crypto/rsa/rsa.go b/src/crypto/rsa/rsa.go index c557c3710a..b6b94a79bb 100644 --- a/src/crypto/rsa/rsa.go +++ b/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 - // 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 diff --git a/src/crypto/rsa/rsa_test.go b/src/crypto/rsa/rsa_test.go index 2f54deae68..e919479a10 100644 --- a/src/crypto/rsa/rsa_test.go +++ b/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 { - 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()") + } +}