]> Cypherpunks repositories - gostls13.git/commitdiff
crypto/x509: keep RSA CRT values in ParsePKCS1PrivateKey
authorFilippo Valsorda <filippo@golang.org>
Fri, 29 Nov 2024 14:38:48 +0000 (15:38 +0100)
committerGopher Robot <gobot@golang.org>
Sat, 30 Nov 2024 01:49:35 +0000 (01:49 +0000)
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>
doc/godebug.md
doc/next/6-stdlib/99-minor/crypto/x509/69799.md
src/crypto/internal/fips140/rsa/rsa.go
src/crypto/rsa/rsa.go
src/crypto/x509/pkcs1.go
src/crypto/x509/pkcs8.go
src/crypto/x509/x509_test.go
src/internal/godebugs/table.go
src/runtime/metrics/doc.go

index 15918bdd59c0e3f79c7ac89f80c4820189b0e793..1b5674f2cd0b5bd01e54524f4d53a153adc101f7 100644 (file)
@@ -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
index 6eb000b3605e637324b305146e024711bbd64b92..2197168365200882d8bb37ad0253b5db987e0d91 100644 (file)
@@ -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.
index 59951f838b9147dadca68256d0a3e5ac0c4153ea..8803599f02a81126a279a39dc58a31c21aa6b849 100644 (file)
@@ -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
index 38fbfce9a3fd1f9028af46a99c367389f5899ecc..e4591fd5e289f09fc81f077c917914e13b18459a 100644 (file)
@@ -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 {
index 7929867ac6d2e722762288e7c24d61400993fba3..ca23358c8c4610c88d787d95e7cfde92c05492fc 100644 (file)
@@ -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
        }
 
index 6268c367575025fff96781d5722ece209cb80a24..d0ab573ff33236328031eb52312a21be3269fd6f 100644 (file)
@@ -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 {
index 1a714e7b62cf4775f68f53f4a7d2eae1792fb849..941ea572e69f26e2eea745045b53128ea85c6425 100644 (file)
@@ -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) {
index 852afaabce4d7d6ff11c3edc9d8d9b8b5117409f..9c48a923f032c35c918dfe50d598856fdb39b603 100644 (file)
@@ -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"},
index 34a1b01fe4bcffe7b3bca92f28211028f59db12f..563ddf4c9594055985d3c18972a64672e6704f06 100644 (file)
@@ -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=...