Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix test_pkey_rsa.rb in FIPS. #790

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

junaruga
Copy link
Member

@junaruga junaruga commented Aug 16, 2024

This is a draft PR to fix the test_pkey_rsa.rb in FIPS. The 1st commit is a temporary workaround to fix another test failure that is same with the #789 .

@junaruga
Copy link
Member Author

junaruga commented Aug 16, 2024

@rhenium I have a question. I am trying to fix the test_sign_verify_pss in the test/openssl/test_pkey_rsa.rb.

I had to change the RSA bits from 1024 to 2048 to support FIPS. And I need to adjust the value of the salt_length: option. I still don't understand how to use the option after I debugged. Could you tell me how to adjust it? Thanks.

-    key = Fixtures.pkey("rsa1024")
+    key = Fixtures.pkey("rsa2048")

openssl-master case (non-FIPS),

https://github.com/ruby/openssl/actions/runs/10422253508/job/28866328411?pr=790#step:10:637

1) Failure: test_sign_verify_pss(OpenSSL::TestPKeyRSA)
/home/runner/work/openssl/openssl/test/openssl/test_pkey_rsa.rb:213:in `test_sign_verify_pss'
     210:       key.verify_pss("SHA256", signature, data, salt_length: 20, mgf1_hash: "SHA1")
     211: 
     212:     signature = key.sign_pss("SHA256", data, salt_length: :max, mgf1_hash: "SHA1")
  => 213:     assert_equal true,
     214:       key.verify_pss("SHA256", signature, data, salt_length: 94, mgf1_hash: "SHA1")
     215:     assert_equal true,
     216:       key.verify_pss("SHA256", signature, data, salt_length: :auto, mgf1_hash: "SHA1")
<true> expected but was
<false>

openssl-master with fips provider

https://github.com/ruby/openssl/actions/runs/10422253508/job/28866333229?pr=790#step:11:218

9) Error: test_sign_verify_pss(OpenSSL::TestPKeyRSA): OpenSSL::PKey::RSAError: invalid salt length
/home/runner/work/openssl/openssl/test/openssl/test_pkey_rsa.rb:212:in `sign_pss'
/home/runner/work/openssl/openssl/test/openssl/test_pkey_rsa.rb:212:in `test_sign_verify_pss'
     209:     assert_equal false,
     210:       key.verify_pss("SHA256", signature, data, salt_length: 20, mgf1_hash: "SHA1")
     211: 
  => 212:     signature = key.sign_pss("SHA256", data, salt_length: :max, mgf1_hash: "SHA1")
     213:     assert_equal true,
     214:       key.verify_pss("SHA256", signature, data, salt_length: 94, mgf1_hash: "SHA1")
     215:     assert_equal true,

@rhenium
Copy link
Member

rhenium commented Aug 16, 2024

The test case is for the salt length chosen with salt_length: :max, which depends on the key size. When using a 2048-bit key with SHA-256, it will be 256 - 1 - 1 - 32 = 222 octets.

Please see the EMSA-PSS-ENCODE definition in https://datatracker.ietf.org/doc/html/rfc8017: sLen (salt length) must be equal to or lower than emLen - 1 - 1 - hLen, where emLen is the modulus size in octets and hLen is the hash function's output size in octets.

test/openssl/test_pkey_rsa.rb Outdated Show resolved Hide resolved
test/openssl/test_pkey_rsa.rb Outdated Show resolved Hide resolved
test/openssl/test_pkey_rsa.rb Outdated Show resolved Hide resolved
@junaruga junaruga force-pushed the wip/fips-test-pkey-rsa branch from 67180ac to 20459f7 Compare August 19, 2024 10:05
@junaruga
Copy link
Member Author

This is a draft PR to fix the test_pkey_rsa.rb in FIPS. The 1st commit is a temporary workaround to fix another test failure that is same with the #789 .

I rebased this PR on the latest master branch, as the PR #789 was marged.

@junaruga
Copy link
Member Author

junaruga commented Aug 19, 2024

The test case is for the salt length chosen with salt_length: :max, which depends on the key size. When using a 2048-bit key with SHA-256, it will be 256 - 1 - 1 - 32 = 222 octets.

Please see the EMSA-PSS-ENCODE definition in https://datatracker.ietf.org/doc/html/rfc8017: sLen (salt length) must be equal to or lower than emLen - 1 - 1 - hLen, where emLen is the modulus size in octets and hLen is the hash function's output size in octets.

Thanks for your info with the reference of the document. I was able to fix the test_sign_verify_pss in non-FIPS cases by the following modification. The CI result is here.

diff --git a/test/openssl/test_pkey_rsa.rb b/test/openssl/test_pkey_rsa.rb
index 2230a47..15f5672 100644
--- a/test/openssl/test_pkey_rsa.rb
+++ b/test/openssl/test_pkey_rsa.rb
@@ -211,12 +211,12 @@ class OpenSSL::TestPKeyRSA < OpenSSL::PKeyTestCase

     signature = key.sign_pss("SHA256", data, salt_length: :max, mgf1_hash: "SHA1")
     assert_equal true,
-      key.verify_pss("SHA256", signature, data, salt_length: 94, mgf1_hash: "SHA1")
+      key.verify_pss("SHA256", signature, data, salt_length: 222, mgf1_hash: "SHA1")
     assert_equal true,
       key.verify_pss("SHA256", signature, data, salt_length: :auto, mgf1_hash: "SHA1")

     assert_raise(OpenSSL::PKey::RSAError) {
-      key.sign_pss("SHA256", data, salt_length: 95, mgf1_hash: "SHA1")
+      key.sign_pss("SHA256", data, salt_length: 223, mgf1_hash: "SHA1")
     }
   end

I still don't understand the calculation for the salt length (sLen) is like that.

sLen <= emLen - 1 - 1 - hLen = 256 - 1 - 1 - 32 = 222

According to the reference of the document.

https://datatracker.ietf.org/doc/html/rfc8017#section-9.1.1

Steps:
...
3. If emLen < hLen + sLen + 2, output "encoding error" and stop.

That means if sLen > emLen - 2 - hLen, output "encoding error" and stop.

What I don't understand are below. Could you explain more about these things?

  • Does emLen: 256 comes from the used SHA256?
  • Why is hLen 32?

And how about the following error in FIPS cases?

9) Error: test_sign_verify_pss(OpenSSL::TestPKeyRSA): OpenSSL::PKey::RSAError: invalid salt length
/home/runner/work/openssl/openssl/test/openssl/test_pkey_rsa.rb:212:in `sign_pss'
/home/runner/work/openssl/openssl/test/openssl/test_pkey_rsa.rb:212:in `test_sign_verify_pss'

@junaruga
Copy link
Member Author

And how about the following error in FIPS cases?

9) Error: test_sign_verify_pss(OpenSSL::TestPKeyRSA): OpenSSL::PKey::RSAError: invalid salt length
/home/runner/work/openssl/openssl/test/openssl/test_pkey_rsa.rb:212:in `sign_pss'
/home/runner/work/openssl/openssl/test/openssl/test_pkey_rsa.rb:212:in `test_sign_verify_pss'

I was able to debug this issue by the following minimal script with gdb.

$ OPENSSL_CONF=$HOME/.local/openssl-3.4.0-dev-fips-debug-d550d2aae5/ssl/openssl_fips.cnf \
  gdb --args ruby -I./lib -ropenssl -e '
    key = OpenSSL::PKey.read(File.read("test/openssl/fixtures/pkey/rsa2048.pem"))
    signature = key.sign_pss("SHA256", "Sign me!", salt_length: :max, mgf1_hash: "SHA1")
'

At the following part, the approved is 0 (= non-approved), because the saltlen (222) is larger than mdsize (32). But I am not sure how to fix the test signature = key.sign_pss("SHA256", data, salt_length: :max, mgf1_hash: "SHA1") (test/openssl/test_pkey_rsa.rb:212). Maybe we need to change the SHA1 to larger one such as SHA256?

https://github.com/openssl/openssl/blob/d550d2aae531c6fa2e10b1a30d2acdf373663889/providers/implementations/signature/rsa_sig.c#L580-L597

(gdb) f
#0  rsa_pss_saltlen_check_passed (ctx=0x68f9b0,
    algoname=0x7fffcde03673 "RSA Sign", saltlen=222)
    at providers/implementations/signature/rsa_sig.c:589
589	    if (!approved) {

(gdb) p saltlen
$15 = 222
(gdb) p mdsize
$16 = 32
(gdb) p approved
$17 = 0

(gdb) bt
#0  rsa_pss_saltlen_check_passed (ctx=0x68f9b0, algoname=0x7fffcde03673 "RSA Sign", saltlen=222) at providers/implementations/signature/rsa_sig.c:589
#1  0x00007fffcdc55936 in rsa_sign (vprsactx=0x68f9b0, sig=0x7fffe9c8e4d8 "", siglen=0x7fffffffc420, sigsize=256,
    tbs=0x7fffffffc300 "I\240N\f\335d\245\2007\262\020ei\nK\377\232\354\003\032\374r#\vO{%\b\vE\372i@\303\377\377\377\177", tbslen=32) at providers/implementations/signature/rsa_sig.c:734
#2  0x00007fffcdc56608 in rsa_digest_sign_final (vprsactx=0x68f9b0, sig=0x7fffe9c8e4d8 "", siglen=0x7fffffffc420, sigsize=256) at providers/implementations/signature/rsa_sig.c:1041
#3  0x00007fffce22cb5a in EVP_DigestSignFinal (ctx=0x69f020, sigret=0x7fffe9c8e4d8 "", siglen=0x7fffffffc420) at crypto/evp/m_sigver.c:520
#4  0x00007fffd06d61c1 in ossl_rsa_sign_pss (argc=3, argv=0x7fffe9cff058, self=140736690994560) at ../../../../ext/openssl/ossl_pkey_rsa.c:391

@rhenium
Copy link
Member

rhenium commented Aug 19, 2024

That means if sLen > emLen - 2 - hLen, output "encoding error" and stop.

What I don't understand are below. Could you explain more about these things?

  • Does emLen: 256 comes from the used SHA256?

No, it comes from the RSA key size. emLen = \ceil(emBits/8), where emBits is the RSA key's modulus size (the max size of data it can sign) in bits.

  • Why is hLen 32?

SHA-256's output is 32 octets long.


https://github.com/openssl/openssl/blob/d550d2aae531c6fa2e10b1a30d2acdf373663889/providers/implementations/signature/rsa_sig.c#L580-L597

According to the comment and https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-5.pdf, the NIST standard introduces additional constraints to RFC 8017. It sounds like that salt_length: :max tests need to be skipped in the FIPS mode.

@junaruga
Copy link
Member Author

That means if sLen > emLen - 2 - hLen, output "encoding error" and stop.
What I don't understand are below. Could you explain more about these things?

  • Does emLen: 256 comes from the used SHA256?

No, it comes from the RSA key size. emLen = \ceil(emBits/8), where emBits is the RSA key's modulus size (the max size of data it can sign) in bits.

  • Why is hLen 32?

SHA-256's output is 32 octets long.

All right! So, the calculation of the salt_length is like this.

sLen <= emLen (octat) - 2 - hLen (octet) = 2048 / 8 - 2 - 256 / 8 = 222

https://github.com/openssl/openssl/blob/d550d2aae531c6fa2e10b1a30d2acdf373663889/providers/implementations/signature/rsa_sig.c#L580-L597

According to the comment and https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-5.pdf, the NIST standard introduces additional constraints to RFC 8017. It sounds like that salt_length: :max tests need to be skipped in the FIPS mode.

All right. I couldn't find the written part in the https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.186-5.pdf. However, I will skip the assertion in FIPS with adding a comment.

@junaruga junaruga force-pushed the wip/fips-test-pkey-rsa branch 2 times, most recently from 8aad46c to 895ef23 Compare August 20, 2024 14:54
@junaruga junaruga changed the title WIP: Fix test_pkey_rsa.rb in FIPS. Fix test_pkey_rsa.rb in FIPS. Aug 20, 2024
@junaruga junaruga marked this pull request as ready for review August 20, 2024 15:00
@junaruga
Copy link
Member Author

@rhenium I fixed all the tests in test_pkey_rsa.rb for FIPS. Please view. Thank you.

test/openssl/test_pkey_rsa.rb Outdated Show resolved Hide resolved
test/openssl/test_pkey_rsa.rb Outdated Show resolved Hide resolved
test/openssl/test_pkey_rsa.rb Outdated Show resolved Hide resolved
test/openssl/test_pkey_rsa.rb Outdated Show resolved Hide resolved
test/openssl/test_pkey_rsa.rb Outdated Show resolved Hide resolved
test/openssl/test_pkey_rsa.rb Outdated Show resolved Hide resolved
test/openssl/test_pkey_rsa.rb Outdated Show resolved Hide resolved
test/openssl/test_pkey_rsa.rb Outdated Show resolved Hide resolved
@junaruga junaruga force-pushed the wip/fips-test-pkey-rsa branch from 895ef23 to d00ce20 Compare August 26, 2024 18:40
@junaruga
Copy link
Member Author

@rhenium I fixed all the items that you mentioned, except for the one thing whether we can drop the key2 tests in test_new and test_s_generate. Please review again. Thank you.

@junaruga junaruga force-pushed the wip/fips-test-pkey-rsa branch from d00ce20 to cb85a7e Compare August 28, 2024 15:22
@junaruga
Copy link
Member Author

I fixed all the items that you mentioned!

* test_sign_verify
  I created the signature text (`signature_encoded.txt`), that is used as a
  text to create the `signature0` in the `test_sign_verify` by the following
  steps with the `openssl` CLI on FIPS module.
  ```
  $ OPENSSL_DIR="${HOME}/.local/openssl-3.4.0-dev-fips-debug-3c6e114959"
  $ export OPENSSL_CONF="${OPENSSL_DIR}/ssl/openssl_fips.cnf"

  $ echo -n "Sign me!" > data.txt
  $ "${OPENSSL_DIR}/bin/openssl" dgst -sha256 -sign test/openssl/fixtures/pkey/rsa2048.pem data.txt > signature.txt
  $ cat signature.txt | base64 > signature_encoded.txt
  ```
@junaruga junaruga force-pushed the wip/fips-test-pkey-rsa branch from cb85a7e to 091f3eb Compare September 3, 2024 19:49
@rhenium rhenium merged commit ade5076 into ruby:master Sep 5, 2024
56 checks passed
@rhenium
Copy link
Member

rhenium commented Sep 5, 2024

Sorry for the delay - it looks good to me. Thank you!

@junaruga junaruga deleted the wip/fips-test-pkey-rsa branch September 5, 2024 08:53
@junaruga
Copy link
Member Author

junaruga commented Sep 5, 2024

Thank you for reviewing my PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants