Fix test_pkey_dh.rb in FIPS. #694

junaruga · 2023-11-07T20:42:41Z

This PR is to fix test/openssl/test_pkey_dh.rb in FIPS case. Initially I was thinking to fix other test/openssl/test_pkey*.rb files in this PR too. However, the modification is bigger than I thought. So, this PR is only for test/openssl/test_pkey_dh.rb to make us review easily.

Related issue: #692

Add dh2048_fips.pem file (DH 2048 bits) in FIPS case, because a logic to generate the pem file in FIPS module is different from the logic in non-FIPS default behavior.

Created the DH file with 2048 bits, because the following command failed to generate the DH 1024 bits pem file in FIPS.

$ OPENSSL_CONF=/home/jaruga/.local/openssl-3.3.0-dev-fips-debug-1aa08644ec/ssl/openssl_fips.cnf \
  /home/jaruga/.local/openssl-3.3.0-dev-fips-debug-1aa08644ec/bin/openssl dhparam -out dh1024_fips.pem 1024
Generating DH parameters, 1024 bit long safe prime
dhparam: Generating DH key parameters failed

The dh2048_fips.pem file was created by the following command.

$ OPENSSL_CONF=/home/jaruga/.local/openssl-3.3.0-dev-fips-debug-1aa08644ec/ssl/openssl_fips.cnf \
  /home/jaruga/.local/openssl-3.3.0-dev-fips-debug-1aa08644ec/bin/openssl dhparam -out dh2048_fips.pem 2048

junaruga · 2023-11-09T09:42:46Z

According to the https://www.openssl.org/source/ - OpenSSL 3.0.8 security policy pdf document page 25, the following part doesn't include 1024 bit for DH public key. I guess that's why the openssl dhparam -out dh1024_fips.pem 1024 failed in FIPS module.

The Module does not output intermediate key generation values. The Module supports the following Public Keys listed below in Table 10.

Table 10 – Public Keys
DH Public - DH (2048/3072/4096/6144/8192) public key agreement key

test/openssl/test_pkey_dh.rb

rhenium · 2023-11-10T17:53:59Z

test/openssl/test_pkey_dh.rb

- EOF
+ assert_same_dh dh_params, key
+
+ pem = Fixtures.pkey_str(KEY_NAME)


This was deliberately hard-coded so that we wouldn't accidentally modify the file content without noticing.

OK. I changed to use the hardcored key name instead of KEY_NAME now.

I'm sorry that the comment location was pretty confusing. I was referring to the deleted PEM-encoded string.

Other test cases may depend on the specific parameters/key[*]. Accidentally modifying the content of dh2048_ffdhe2048.pem could lead to multiple test cases failing. I think it's useful to have a very basic test case that can work as a sanity check against it.

[*] Actually, this isn't the case for test_pkey_dh.rb - in other words, DH is under-tested. We should have at least an assert_equal test against the result of DH#derive using known test vectors. An example in test_pkey_rsa.rb is test_sign_verify which depends on the specific key rsa1024.pem.

Ah all right. You were talking about the deleted PEM-encoded string. No worry. I just misundertood it. I will revert the change.

- pem = <<~EOF - -----BEGIN DH PARAMETERS----- - MIGHAoGBAKnKQ8MNK6nYZzLrrcuTsLxuiJGXoOO5gT+tljOTbHBuiktdMTITzIY0 - pFxIvjG05D7HoBZQfrR0c92NGWPkAiCkhQKB8JCbPVzwNLDy6DZ0pmofDKrEsYHG - AQjjxMXhwULlmuR/K+WwlaZPiLIBYalLAZQ7ZbOPeVkJ8ePao0eLAgEC - -----END DH PARAMETERS----- - EOF

Do you want to me to hard-code "dh2048_ffdhe2048" in each part in the test_pkey_dh.rb file? Or can I use the KEY_NAME?

KEY_NAME = "dh2048_ffdhe2048"

I think it's useful to have a very basic test case that can work as a sanity check against it.

I hope that we will add the basic test case in another PR after this PR will be merged, as I want to focus on this PR's subject to pass the test_pkey_dh.rb in FIPS.

Do you want to me to hard-code "dh2048_ffdhe2048" in each part in the test_pkey_dh.rb file? Or can I use the KEY_NAME?

KEY_NAME = "dh2048_ffdhe2048"

I will go ahead with the hard-coded "dh2048_ffdhe2048" in each part for now.

Thank you, it looks good to me!

Do you want to me to hard-code "dh2048_ffdhe2048" in each part in the test_pkey_dh.rb file? Or can I use the KEY_NAME?

I didn't have an opinion on this. Either style seems good as long as we're consistent.

junaruga · 2023-11-14T16:44:38Z

I rebased this PR fixing by the review and updating the commit message. Now I see the CI failures in OpenSSL 1.1.0 and 1.0.0 and LibreSSL 3.4, 3.3, 3.2 and 3.1. I am trying to debug the issue by installing OpenSSL 1.1.0 on my local. Please let me know if you have an idea of how to fix it. Thank you.

https://github.com/ruby/openssl/actions/runs/6866516958/job/18672914263?pr=694#step:12:638

  3) Failure: test_params_ok?(OpenSSL::TestPKeyDH)
/home/runner/work/openssl/openssl/test/openssl/test_pkey_dh.rb:106:in `test_params_ok?'
     103:       OpenSSL::ASN1::Integer(dh0.p),
     104:       OpenSSL::ASN1::Integer(dh0.g)
     105:     ]))
  => 106:     assert_equal(true, dh1.params_ok?)
     107: 
     108:     dh2 = OpenSSL::PKey::DH.new(OpenSSL::ASN1::Sequence([
     109:       OpenSSL::ASN1::Integer(dh0.p + 1),
<true> expected but was
<false>

rhenium · 2023-11-14T21:15:52Z

After a bit of git blame, I found that OpenSSL 1.1.1d changed the behavior of DH_check(): openssl/openssl#9435

Then LibreSSL 3.5.0 imported the patch: libressl/openbsd@c537641

According to the original PR openssl/openssl#9435, the main motivation actually was to let RFC 7919 groups pass DH_check().

I feel it's safe to simply skip the test case on older versions of OpenSSL based on the version number, considering that they are all EOL now.

junaruga · 2023-11-15T18:42:34Z

Thanks for debugging the issue.

I debugged with the following reproducing scripts. The test.rb is for dh2048_ffdhe2048.pem.

$ cat test.rb
str = <<EOF
-----BEGIN DH PARAMETERS-----
MIIBCAKCAQEA//////////+t+FRYortKmq/cViAnPTzx2LnFg84tNpWp4TZBFGQz
+8yTnc4kmz75fS/jY2MMddj2gbICrsRhetPfHtXV/WVhJDP1H18GbtCFY2VVPe0a
87VXE15/V8k1mE8McODmi3fipona8+/och3xWKE2rec1MKzKT0g6eXq8CrGCsyT7
YdEIqUuyyOP7uWrat2DX9GgdT0Kj3jlN9K5W7edjcrsZCwenyO4KbXCeAvzhzffi
7MA0BM0oNC9hkXL+nOmFg/+OTxIy7vKBg8P+OxtMb61zO7X8vC7CIAXFjvGDfRaD
ssbzSibBsu/6iGtCOGEoXJf//////////wIBAg==
-----END DH PARAMETERS-----
EOF

dh0 = OpenSSL::PKey.read(str)
seq = OpenSSL::ASN1::Sequence([
  OpenSSL::ASN1::Integer(dh0.p),
  OpenSSL::ASN1::Integer(dh0.g)
])
dh1 = OpenSSL::PKey::DH.new(seq)
# It shluld be true.
p dh1.params_ok?

The script prints false with the OpenSSL 1.1.0l (the latest OpenSSL 1.0.0 version).

$ ruby -I./lib -ropenssl test.rb
false

First, I compared how the test.rb is executed with OpenSSL 3.3.0-dev (1aa08644ec), OpenSSL 1.1.1w (the latest OpenSSL 1.1.1), and OpenSSL 1.1.0l.

The process is different in the following part.

openssl/ext/openssl/ossl_pkey_dh.c

Lines 330 to 346 in fac5cac

 #ifdef HAVE_EVP_PKEY_CHECK 

 EVP_PKEY *pkey; 

 EVP_PKEY_CTX *pctx; 

 GetPKey(self, pkey); 

 pctx = EVP_PKEY_CTX_new(pkey, /* engine */NULL); 

 if (!pctx) 

 ossl_raise(eDHError, "EVP_PKEY_CTX_new"); 

 ret = EVP_PKEY_param_check(pctx); 

 EVP_PKEY_CTX_free(pctx); 

 #else 

 DH *dh; 

 int codes; 

 GetDH(self, dh); 

 ret = DH_check(dh, &codes) == 1 && codes == 0; 

 #endif

In the case of the OpenSSL 3.3.0-dev, OpenSSL 1.1.1w where the test.rb prints true, it executes the block of the #ifdef HAVE_EVP_PKEY_CHECK (line 331), then executes the ret = EVP_PKEY_param_check(pctx); returning 1.

In the case of the OpenSSL 1.1.0l where the test.rb prints false, it executes the block of the #else (line 345), then executes DH_check(dh, &codes). After that DH_check(dh, &codes) returns 1, but the codes is 8.

(gdb) f
#0  ossl_dh_check_params (self=140737117116760)
    at ../../../../ext/openssl/ossl_pkey_dh.c:348
348	    if (ret == 1)
(gdb) p DH_check(dh, &codes)
$18 = 1
(gdb) p codes
$19 = 8

I prepared the test_dh1024.rb for dh1024.pem.

$ cat test_dh1024.rb
str = <<EOF
-----BEGIN DH PARAMETERS-----
MIGHAoGBAKnKQ8MNK6nYZzLrrcuTsLxuiJGXoOO5gT+tljOTbHBuiktdMTITzIY0
pFxIvjG05D7HoBZQfrR0c92NGWPkAiCkhQKB8JCbPVzwNLDy6DZ0pmofDKrEsYHG
AQjjxMXhwULlmuR/K+WwlaZPiLIBYalLAZQ7ZbOPeVkJ8ePao0eLAgEC
-----END DH PARAMETERS-----
EOF

dh0 = OpenSSL::PKey.read(str)
seq = OpenSSL::ASN1::Sequence([
  OpenSSL::ASN1::Integer(dh0.p),
  OpenSSL::ASN1::Integer(dh0.g)
])
dh1 = OpenSSL::PKey::DH.new(seq)
# It shluld be true.
p dh1.params_ok?

The script prints true with the OpenSSL 1.1.0l.

$ ruby -I./lib -ropenssl -rdebug test_dh1024.rb
true

So, I checked what proces makes the difference of the case between test.rb and test_dh1024.rb with OpenSSL 1.1.0l.

The process making a result was the following part.

https://github.com/openssl/openssl/blob/OpenSSL_1_1_0l/crypto/dh/dh_check.c#L110

With the tesr.rb, the l = BN_mod_word(dh->p, 24); returns 23. and DH_NOT_SUITABLE_GENERATOR (8) is set to the *ret.

(gdb) l
106	        if (dh->j && BN_cmp(dh->j, t1))
107	            *ret |= DH_CHECK_INVALID_J_VALUE;
108	
109	    } else if (BN_is_word(dh->g, DH_GENERATOR_2)) {
110	        l = BN_mod_word(dh->p, 24);
111	        if (l == (BN_ULONG)-1)
112	            goto err;
113	        if (l != 11)
114	            *ret |= DH_NOT_SUITABLE_GENERATOR;
115	    } else if (BN_is_word(dh->g, DH_GENERATOR_5)) {
(gdb) f
#0  DH_check (dh=0x884a30, ret=0x7fffffffc7a8) at crypto/dh/dh_check.c:111
111	        if (l == (BN_ULONG)-1)
(gdb) p l
$23 = 23
...
(gdb) n
114	            *ret |= DH_NOT_SUITABLE_GENERATOR;
(gdb) n
124	    r = BN_is_prime_ex(dh->p, BN_prime_checks, ctx, NULL);
(gdb) p *ret
$24 = 8
(gdb) p DH_NOT_SUITABLE_GENERATOR
$25 = 8

With the test_dh1024.rb, the l = BN_mod_word(dh->p, 24); returns 11. This is okay case.

(gdb) p l
$12 = 11
...
(gdb) f
#0  DH_check (dh=0xa273b0, ret=0x7fffffffc798) at crypto/dh/dh_check.c:140
140	    if (ctx != NULL) {
(gdb) p ok
$14 = 1

So, I thought the dh1.params_ok? returns true if the HAVE_EVP_PKEY_CHECK is defined, and EVP_PKEY_param_check(pctx) is called. But the dh1.params_ok? returns false if the HAVE_EVP_PKEY_CHECK is not defined, and DH_check(dh, &codes) is called.

This result supports your investigation? Is the DH_check called in the EVP_PKEY_param_check(pctx)?

junaruga · 2023-11-16T16:45:12Z

This result supports your investigation? Is the DH_check called in the EVP_PKEY_param_check(pctx)?

OK. I confirmed that the DH_check is called in the EVP_PKEY_param_check(pctx) with the OpenSSL 3.3.0-dev (1aa08644ec), openssl/openssl@1aa0864 . Below is the backtrace.

https://github.com/openssl/openssl/blob/1aa08644ecd4005c0f55276b2e8dabd8a2a758f0/crypto/dh/dh_check.c#L115

$ gdb --args ruby -I./lib -ropenssl -rdebug test.rb
(gdb) b ossl_dh_check_params
...
(gdb) f
#0  DH_check_ex (dh=0x59cb50) at crypto/dh/dh_check.c:115
115	    if (!DH_check(dh, &errflags))
(gdb) bt
#0  DH_check_ex (dh=0x59cb50) at crypto/dh/dh_check.c:115
#1  0x00007fffce665d42 in dh_validate (keydata=0x59cb50, selection=132, checktype=0)
    at providers/implementations/keymgmt/dh_kmgmt.c:429
#2  0x00007fffce442aa2 in evp_keymgmt_validate (keymgmt=0x649d10, keydata=0x59cb50,
    selection=132, checktype=0) at crypto/evp/keymgmt_meth.c:452
#3  0x00007fffce44f1fd in try_provided_check (ctx=0xc380f0, selection=132,
    checktype=0) at crypto/evp/pmeth_check.c:44
#4  0x00007fffce44f409 in evp_pkey_param_check_combined (ctx=0xc380f0, checktype=0)
    at crypto/evp/pmeth_check.c:101
#5  0x00007fffce44f4f4 in EVP_PKEY_param_check (ctx=0xc380f0)
    at crypto/evp/pmeth_check.c:128
#6  0x00007fffceb511b0 in ossl_dh_check_params (self=140737117124440)
    at ../../../../ext/openssl/ossl_pkey_dh.c:338
...

junaruga · 2023-11-16T16:48:38Z

After a bit of git blame, I found that OpenSSL 1.1.1d changed the behavior of DH_check(): openssl/openssl#9435

Perhaps, are you talking about git bisect rather than git blame in your sentence above?

junaruga · 2023-11-16T17:40:11Z

OK. I skipped the test in OpenSSL 1.1.1c or early versions.

I see one test is failing randomly in test/openssl/test_ocsp.rb on macos-latest trufeeruby below for this PR. So, I opened the issue #695.

https://github.com/ruby/openssl/actions/runs/6894315128/job/18755841590?pr=694

We use dh2048_ffdhe2048.pem file (DH 2048 bits) instead of dh1024.pem file in both non-FIPS and FIPS cases. Because the following command fails to generate the pem file with 1024 bits. And the OpenSSL FIPS 140-2 security policy document explains the DH public keys are allowed from 2048 bits.[1] ``` $ OPENSSL_CONF=/home/jaruga/.local/openssl-3.3.0-dev-fips-debug-1aa08644ec/ssl/openssl_fips.cnf \ /home/jaruga/.local/openssl-3.3.0-dev-fips-debug-1aa08644ec/bin/openssl \ dhparam -out dh1024.pem 1024 Generating DH parameters, 1024 bit long safe prime dhparam: Generating DH key parameters failed ``` The dh2048_ffdhe2048.pem file was created by the following command with the OpenSSL FIPS configuration file. The logic to generate the DH pem file is different between non-FIPS and FIPS cases. In FIPS, it seems that the command always returns the text defined as ffdhe2048 in the FFDHE groups in RFC 7919 unlike non-FIPS.[2] As the generated pem file is a normal and valid PKCS#3-style group parameter, we use the file for the non-FIPS case too. ``` $ OPENSSL_CONF=/home/jaruga/.local/openssl-3.3.0-dev-fips-debug-1aa08644ec/ssl/openssl_fips.cnf \ /home/jaruga/.local/openssl-3.3.0-dev-fips-debug-1aa08644ec/bin/openssl \ dhparam -out dh2048_ffdhe2048.pem 2048 ``` Note that the hard-coded PEM-encoded string in the `test_DHparams` is intentional to avoid modifying the content unintentionally. * [1] https://www.openssl.org/source/ - OpenSSL 3.0.8 FIPS 140-2 security policy document page 25, Table 10 – Public Keys - DH Public - DH (2048/3072/4096/6144/8192) public key agreement key * [2] RFC7919 - Appendix A.1: ffdhe2048 https://www.rfc-editor.org/rfc/rfc7919#appendix-A.1

junaruga · 2023-11-16T20:07:45Z

Now this PR is ready for reviewing again. After rebasing the failure above was gone. Could you review it?

rhenium · 2023-11-25T10:12:07Z

OK. I confirmed that the DH_check is called in the EVP_PKEY_param_check(pctx) with the OpenSSL 3.3.0-dev (1aa08644ec),

Yes, this was my observation too.

rhenium · 2023-11-25T10:13:43Z

Sorry for taking a long time to get back on this. The change looks good to me! Thanks.

junaruga · 2023-11-27T07:30:42Z

Okay. Thank you for reviewing!

junaruga force-pushed the wip/fips-test-pkey-dh branch from d9ed13e to f8113e4 Compare November 8, 2023 09:57

rhenium reviewed Nov 10, 2023

View reviewed changes

junaruga mentioned this pull request Nov 13, 2023

Support OpenSSL 3 FIPS #603

Open

junaruga force-pushed the wip/fips-test-pkey-dh branch from f8113e4 to 857943a Compare November 14, 2023 16:24

junaruga force-pushed the wip/fips-test-pkey-dh branch from 857943a to fb61ba3 Compare November 15, 2023 18:52

junaruga force-pushed the wip/fips-test-pkey-dh branch from fb61ba3 to f90b9b5 Compare November 16, 2023 17:28

junaruga mentioned this pull request Nov 16, 2023

macos-latest truffeeruby-head: test_basic_response_response_operations failing #695

Closed

junaruga force-pushed the wip/fips-test-pkey-dh branch from f90b9b5 to 6a4ff26 Compare November 16, 2023 18:21

rhenium merged commit 1fa9fc5 into ruby:master Nov 25, 2023
43 checks passed

junaruga deleted the wip/fips-test-pkey-dh branch November 27, 2023 07:30

kenhys mentioned this pull request Feb 28, 2024

FIPS support in Fluentd fluent/fluentd#3121

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix test_pkey_dh.rb in FIPS. #694

Fix test_pkey_dh.rb in FIPS. #694

junaruga commented Nov 7, 2023 •

edited

Loading

junaruga commented Nov 9, 2023

rhenium Nov 10, 2023

junaruga Nov 14, 2023

rhenium Nov 14, 2023

junaruga Nov 15, 2023 •

edited

Loading

junaruga Nov 15, 2023

junaruga Nov 15, 2023

rhenium Nov 25, 2023

junaruga commented Nov 14, 2023 •

edited

Loading

rhenium commented Nov 14, 2023

junaruga commented Nov 15, 2023 •

edited

Loading

junaruga commented Nov 16, 2023

junaruga commented Nov 16, 2023

junaruga commented Nov 16, 2023 •

edited

Loading

junaruga commented Nov 16, 2023 •

edited

Loading

rhenium commented Nov 25, 2023

rhenium commented Nov 25, 2023

junaruga commented Nov 27, 2023

Fix test_pkey_dh.rb in FIPS. #694

Fix test_pkey_dh.rb in FIPS. #694

Conversation

junaruga commented Nov 7, 2023 • edited Loading

junaruga commented Nov 9, 2023

rhenium Nov 10, 2023

Choose a reason for hiding this comment

junaruga Nov 14, 2023

Choose a reason for hiding this comment

rhenium Nov 14, 2023

Choose a reason for hiding this comment

junaruga Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

junaruga Nov 15, 2023

Choose a reason for hiding this comment

junaruga Nov 15, 2023

Choose a reason for hiding this comment

rhenium Nov 25, 2023

Choose a reason for hiding this comment

junaruga commented Nov 14, 2023 • edited Loading

rhenium commented Nov 14, 2023

junaruga commented Nov 15, 2023 • edited Loading

junaruga commented Nov 16, 2023

junaruga commented Nov 16, 2023

junaruga commented Nov 16, 2023 • edited Loading

junaruga commented Nov 16, 2023 • edited Loading

rhenium commented Nov 25, 2023

rhenium commented Nov 25, 2023

junaruga commented Nov 27, 2023

junaruga commented Nov 7, 2023 •

edited

Loading

junaruga Nov 15, 2023 •

edited

Loading

junaruga commented Nov 14, 2023 •

edited

Loading

junaruga commented Nov 15, 2023 •

edited

Loading

junaruga commented Nov 16, 2023 •

edited

Loading

junaruga commented Nov 16, 2023 •

edited

Loading