From ba9e9670ef3f845d26c24551cb103f0fde99c7ed Mon Sep 17 00:00:00 2001 From: qmuntal Date: Wed, 4 Sep 2024 12:35:04 +0000 Subject: [PATCH 1/6] only allow omission of precomputed RSA values in OpenSSL 3.0 and 3.1 --- rsa.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rsa.go b/rsa.go index 564ccdcf..81e65d8d 100644 --- a/rsa.go +++ b/rsa.go @@ -399,7 +399,7 @@ func newRSAKey3(isPriv bool, n, e, d, p, q, dp, dq, qinv BigInt) (C.GO_EVP_PKEY_ // OpenSSL 3.0 and 3.1 required all the precomputed values if // P and Q are present. See: // https://github.com/openssl/openssl/pull/22334 - if p != nil && q != nil && dp != nil && dq != nil && qinv != nil { + if vMinor >= 2 || (p != nil && q != nil && dp != nil && dq != nil && qinv != nil) { precomputed := [...]bigIntParam{ {OSSL_PKEY_PARAM_RSA_FACTOR1, p}, {OSSL_PKEY_PARAM_RSA_FACTOR2, q}, {OSSL_PKEY_PARAM_RSA_EXPONENT1, dp}, {OSSL_PKEY_PARAM_RSA_EXPONENT2, dq}, {OSSL_PKEY_PARAM_RSA_COEFFICIENT1, qinv}, From a6930fa0fedce2dd75a0b08eb10906d7d09ee4ff Mon Sep 17 00:00:00 2001 From: qmuntal Date: Wed, 4 Sep 2024 13:21:14 +0000 Subject: [PATCH 2/6] check if rsa primes and precomputed values exists --- rsa.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/rsa.go b/rsa.go index 81e65d8d..bdf4b7b4 100644 --- a/rsa.go +++ b/rsa.go @@ -400,11 +400,16 @@ func newRSAKey3(isPriv bool, n, e, d, p, q, dp, dq, qinv BigInt) (C.GO_EVP_PKEY_ // P and Q are present. See: // https://github.com/openssl/openssl/pull/22334 if vMinor >= 2 || (p != nil && q != nil && dp != nil && dq != nil && qinv != nil) { - precomputed := [...]bigIntParam{ - {OSSL_PKEY_PARAM_RSA_FACTOR1, p}, {OSSL_PKEY_PARAM_RSA_FACTOR2, q}, - {OSSL_PKEY_PARAM_RSA_EXPONENT1, dp}, {OSSL_PKEY_PARAM_RSA_EXPONENT2, dq}, {OSSL_PKEY_PARAM_RSA_COEFFICIENT1, qinv}, + if p != nil && q != nil { + comps = append(comps, bigIntParam{OSSL_PKEY_PARAM_RSA_FACTOR1, p}, bigIntParam{OSSL_PKEY_PARAM_RSA_FACTOR2, q}) + } + if dp != nil && dq != nil && qinv != nil { + comps = append(comps, + bigIntParam{OSSL_PKEY_PARAM_RSA_EXPONENT1, dp}, + bigIntParam{OSSL_PKEY_PARAM_RSA_EXPONENT2, dq}, + bigIntParam{OSSL_PKEY_PARAM_RSA_COEFFICIENT1, qinv}, + ) } - comps = append(comps, precomputed[:]...) } for _, comp := range comps { From 058cd37581db092e75fa69e8108c22436881d84c Mon Sep 17 00:00:00 2001 From: qmuntal Date: Wed, 4 Sep 2024 13:37:26 +0000 Subject: [PATCH 3/6] make logic more clear --- rsa.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/rsa.go b/rsa.go index bdf4b7b4..aabcc123 100644 --- a/rsa.go +++ b/rsa.go @@ -396,14 +396,15 @@ func newRSAKey3(isPriv bool, n, e, d, p, q, dp, dq, qinv BigInt) (C.GO_EVP_PKEY_ } comps = append(comps, required[:]...) - // OpenSSL 3.0 and 3.1 required all the precomputed values if - // P and Q are present. See: - // https://github.com/openssl/openssl/pull/22334 - if vMinor >= 2 || (p != nil && q != nil && dp != nil && dq != nil && qinv != nil) { - if p != nil && q != nil { + if p != nil && q != nil { + allPrecomputedExists := dp != nil && dq != nil && qinv != nil + // OpenSSL 3.0 and 3.1 required all the precomputed values if + // P and Q are present. If they are not, we need to omit also P and Q. + // See https://github.com/openssl/openssl/pull/22334 + if vMinor >= 2 || allPrecomputedExists { comps = append(comps, bigIntParam{OSSL_PKEY_PARAM_RSA_FACTOR1, p}, bigIntParam{OSSL_PKEY_PARAM_RSA_FACTOR2, q}) } - if dp != nil && dq != nil && qinv != nil { + if allPrecomputedExists { comps = append(comps, bigIntParam{OSSL_PKEY_PARAM_RSA_EXPONENT1, dp}, bigIntParam{OSSL_PKEY_PARAM_RSA_EXPONENT2, dq}, From 21bd00caaa66e3c1645376f753b21de6d17e1240 Mon Sep 17 00:00:00 2001 From: Quim Muntal Date: Thu, 5 Sep 2024 09:44:02 +0200 Subject: [PATCH 4/6] Update rsa.go Co-authored-by: Davis Goodin --- rsa.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/rsa.go b/rsa.go index aabcc123..4e45b02d 100644 --- a/rsa.go +++ b/rsa.go @@ -398,9 +398,12 @@ func newRSAKey3(isPriv bool, n, e, d, p, q, dp, dq, qinv BigInt) (C.GO_EVP_PKEY_ if p != nil && q != nil { allPrecomputedExists := dp != nil && dq != nil && qinv != nil - // OpenSSL 3.0 and 3.1 required all the precomputed values if - // P and Q are present. If they are not, we need to omit also P and Q. - // See https://github.com/openssl/openssl/pull/22334 + // The precomputed values should only be passed if P and Q are present + // and every precomputed value is present. (If any precomputed value is + // missing, don't pass any of them.) + // + // In OpenSSL 3.0 and 3.1, we must also omit P and Q if any precomputed + // value is missing. See https://github.com/openssl/openssl/pull/22334 if vMinor >= 2 || allPrecomputedExists { comps = append(comps, bigIntParam{OSSL_PKEY_PARAM_RSA_FACTOR1, p}, bigIntParam{OSSL_PKEY_PARAM_RSA_FACTOR2, q}) } From d78675df6473a157e25fe2da5ea338e5f044192c Mon Sep 17 00:00:00 2001 From: qmuntal Date: Thu, 5 Sep 2024 08:07:01 +0000 Subject: [PATCH 5/6] add tests --- rsa_test.go | 89 +++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 77 insertions(+), 12 deletions(-) diff --git a/rsa_test.go b/rsa_test.go index 35de951a..1600f00e 100644 --- a/rsa_test.go +++ b/rsa_test.go @@ -13,11 +13,73 @@ import ( ) func TestRSAKeyGeneration(t *testing.T) { + for _, size := range []int{2048, 3072} { + size := size + t.Run(strconv.Itoa(size), func(t *testing.T) { + t.Parallel() + _, _, _, _, _, _, _, _, err := openssl.GenerateKeyRSA(size) + if err != nil { + t.Fatal(err) + } + }) + } +} + +func testRSAEncryptDecryptPKCS1(t *testing.T, priv *openssl.PrivateKeyRSA, pub *openssl.PublicKeyRSA) { + msg := []byte("hi!") + enc, err := openssl.EncryptRSAPKCS1(pub, msg) + if err != nil { + t.Fatalf("EncryptPKCS1v15: %v", err) + } + dec, err := openssl.DecryptRSAPKCS1(priv, enc) + if err != nil { + t.Fatalf("DecryptPKCS1v15: %v", err) + } + if !bytes.Equal(dec, msg) { + t.Fatalf("got:%x want:%x", dec, msg) + } +} + +func TestRSAEncryptDecryptPKCS1(t *testing.T) { for _, size := range []int{2048, 3072} { size := size t.Run(strconv.Itoa(size), func(t *testing.T) { t.Parallel() priv, pub := newRSAKey(t, size) + testRSAEncryptDecryptPKCS1(t, priv, pub) + }) + } +} + +func TestRSAEncryptDecryptPKCS1_MissingPrecomputedValues(t *testing.T) { + N, E, D, P, Q, Dp, Dq, Qinv, err := openssl.GenerateKeyRSA(2048) + if err != nil { + t.Fatalf("GenerateKeyRSA: %v", err) + } + tt := []struct { + name string + wantErr bool + fn func() (*openssl.PrivateKeyRSA, *openssl.PublicKeyRSA) + }{ + {"noDp", false, func() (*openssl.PrivateKeyRSA, *openssl.PublicKeyRSA) { + return newRSAKeyFromParams(t, N, E, D, P, Q, nil, Dq, Qinv) + }}, + {"noDq", false, func() (*openssl.PrivateKeyRSA, *openssl.PublicKeyRSA) { + return newRSAKeyFromParams(t, N, E, D, P, Q, Dp, nil, Qinv) + }}, + {"noQinv", false, func() (*openssl.PrivateKeyRSA, *openssl.PublicKeyRSA) { + return newRSAKeyFromParams(t, N, E, D, P, Q, Dp, Dq, nil) + }}, + {"none", false, func() (*openssl.PrivateKeyRSA, *openssl.PublicKeyRSA) { + return newRSAKeyFromParams(t, N, E, D, P, Q, nil, nil, nil) + }}, + } + for _, tt := range tt { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + priv, pub := tt.fn() + testRSAEncryptDecryptPKCS1(t, priv, pub) msg := []byte("hi!") enc, err := openssl.EncryptRSAPKCS1(pub, msg) if err != nil { @@ -34,7 +96,7 @@ func TestRSAKeyGeneration(t *testing.T) { } } -func TestEncryptDecryptOAEP(t *testing.T) { +func TestRSAEncryptDecryptOAEP(t *testing.T) { sha256 := openssl.NewSHA256() msg := []byte("hi!") label := []byte("ho!") @@ -57,7 +119,7 @@ func TestEncryptDecryptOAEP(t *testing.T) { } } -func TestEncryptDecryptOAEP_EmptyLabel(t *testing.T) { +func TestRSAEncryptDecryptOAEP_EmptyLabel(t *testing.T) { sha256 := openssl.NewSHA256() msg := []byte("hi!") label := []byte("") @@ -80,7 +142,7 @@ func TestEncryptDecryptOAEP_EmptyLabel(t *testing.T) { } } -func TestEncryptDecryptOAEP_WithMGF1Hash(t *testing.T) { +func TestRSAEncryptDecryptOAEP_WithMGF1Hash(t *testing.T) { if openssl.SymCryptProviderAvailable() { t.Skip("SymCrypt provider does not support MGF1 hash") } @@ -107,7 +169,7 @@ func TestEncryptDecryptOAEP_WithMGF1Hash(t *testing.T) { } } -func TestEncryptDecryptOAEP_WrongLabel(t *testing.T) { +func TestRSAEncryptDecryptOAEP_WrongLabel(t *testing.T) { sha256 := openssl.NewSHA256() msg := []byte("hi!") priv, pub := newRSAKey(t, 2048) @@ -124,7 +186,7 @@ func TestEncryptDecryptOAEP_WrongLabel(t *testing.T) { } } -func TestSignVerifyPKCS1v15(t *testing.T) { +func TestRSASignVerifyPKCS1v15(t *testing.T) { sha256 := openssl.NewSHA256() priv, pub := newRSAKey(t, 2048) msg := []byte("hi!") @@ -151,7 +213,7 @@ func TestSignVerifyPKCS1v15(t *testing.T) { } } -func TestSignVerifyPKCS1v15_Unhashed(t *testing.T) { +func TestRSASignVerifyPKCS1v15_Unhashed(t *testing.T) { if openssl.SymCryptProviderAvailable() { t.Skip("SymCrypt provider does not support unhashed PKCS1v15") } @@ -168,7 +230,7 @@ func TestSignVerifyPKCS1v15_Unhashed(t *testing.T) { } } -func TestSignVerifyPKCS1v15_Invalid(t *testing.T) { +func TestRSASignVerifyPKCS1v15_Invalid(t *testing.T) { sha256 := openssl.NewSHA256() msg := []byte("hi!") priv, pub := newRSAKey(t, 2048) @@ -184,7 +246,7 @@ func TestSignVerifyPKCS1v15_Invalid(t *testing.T) { } } -func TestSignVerifyRSAPSS(t *testing.T) { +func TestRSASignVerifyRSAPSS(t *testing.T) { // Test cases taken from // https://github.com/golang/go/blob/54182ff54a687272dd7632c3a963e036ce03cb7c/src/crypto/rsa/pss_test.go#L200. const keyBits = 2048 @@ -225,15 +287,18 @@ func newRSAKey(t *testing.T, size int) (*openssl.PrivateKeyRSA, *openssl.PublicK if err != nil { t.Fatalf("GenerateKeyRSA(%d): %v", size, err) } - // Exercise omission of precomputed value - Dp = nil + return newRSAKeyFromParams(t, N, E, D, P, Q, Dp, Dq, Qinv) +} + +func newRSAKeyFromParams(t *testing.T, N, E, D, P, Q, Dp, Dq, Qinv openssl.BigInt) (*openssl.PrivateKeyRSA, *openssl.PublicKeyRSA) { + t.Helper() priv, err := openssl.NewPrivateKeyRSA(N, E, D, P, Q, Dp, Dq, Qinv) if err != nil { - t.Fatalf("NewPrivateKeyRSA(%d): %v", size, err) + t.Fatalf("NewPrivateKeyRSA: %v", err) } pub, err := openssl.NewPublicKeyRSA(N, E) if err != nil { - t.Fatalf("NewPublicKeyRSA(%d): %v", size, err) + t.Fatalf("NewPublicKeyRSA: %v", err) } return priv, pub } From ad1a4bc2cf2470cf9e9656a9d23460fe9e1a2bf9 Mon Sep 17 00:00:00 2001 From: qmuntal Date: Tue, 10 Sep 2024 08:03:15 +0200 Subject: [PATCH 6/6] exhaustively tests messing precomputed values --- rsa_test.go | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/rsa_test.go b/rsa_test.go index 1600f00e..94775332 100644 --- a/rsa_test.go +++ b/rsa_test.go @@ -4,6 +4,7 @@ import ( "bytes" "crypto" "crypto/rsa" + "fmt" "math/big" "strconv" "testing" @@ -52,33 +53,40 @@ func TestRSAEncryptDecryptPKCS1(t *testing.T) { } func TestRSAEncryptDecryptPKCS1_MissingPrecomputedValues(t *testing.T) { - N, E, D, P, Q, Dp, Dq, Qinv, err := openssl.GenerateKeyRSA(2048) + n, e, d, p, q, dp, dq, qinv, err := openssl.GenerateKeyRSA(2048) if err != nil { t.Fatalf("GenerateKeyRSA: %v", err) } tt := []struct { - name string - wantErr bool - fn func() (*openssl.PrivateKeyRSA, *openssl.PublicKeyRSA) + withDp bool + withDq bool + withQinv bool }{ - {"noDp", false, func() (*openssl.PrivateKeyRSA, *openssl.PublicKeyRSA) { - return newRSAKeyFromParams(t, N, E, D, P, Q, nil, Dq, Qinv) - }}, - {"noDq", false, func() (*openssl.PrivateKeyRSA, *openssl.PublicKeyRSA) { - return newRSAKeyFromParams(t, N, E, D, P, Q, Dp, nil, Qinv) - }}, - {"noQinv", false, func() (*openssl.PrivateKeyRSA, *openssl.PublicKeyRSA) { - return newRSAKeyFromParams(t, N, E, D, P, Q, Dp, Dq, nil) - }}, - {"none", false, func() (*openssl.PrivateKeyRSA, *openssl.PublicKeyRSA) { - return newRSAKeyFromParams(t, N, E, D, P, Q, nil, nil, nil) - }}, + {true, true, false}, + {true, false, true}, + {false, true, true}, + {false, false, false}, + {false, false, true}, + {false, true, false}, + {true, false, false}, + {true, true, true}, } for _, tt := range tt { tt := tt - t.Run(tt.name, func(t *testing.T) { + t.Run(fmt.Sprintf("dp=%v,dq=%v,qinv=%v", tt.withDp, tt.withDq, tt.withQinv), func(t *testing.T) { t.Parallel() - priv, pub := tt.fn() + dp1, dq1, qinv1 := dp, dq, qinv + if !tt.withDp { + dp1 = nil + } + if !tt.withDq { + dq1 = nil + } + if !tt.withQinv { + qinv1 = nil + } + + priv, pub := newRSAKeyFromParams(t, n, e, d, p, q, dp1, dq1, qinv1) testRSAEncryptDecryptPKCS1(t, priv, pub) msg := []byte("hi!") enc, err := openssl.EncryptRSAPKCS1(pub, msg)