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.

Sign up for GitHub

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

test/openssl/test_pkey.rb: Fix pending tests in FIPS case. #664

Merged
merged 1 commit into from
Aug 16, 2023
+26 −5
Merged

test/openssl/test_pkey.rb: Fix pending tests in FIPS case. #664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions test/openssl/test_pkey.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,7 @@ def test_hmac_sign_verify
end

def test_ed25519
# https://github.com/openssl/openssl/issues/20758
pend('Not supported on FIPS mode enabled') if OpenSSL.fips_mode
pend_on_openssl_issue_21493

# Test vector from RFC 8032 Section 7.1 TEST 2
priv_pem = <<~EOF
Expand All @@ -101,7 +100,13 @@ def test_ed25519
pub = OpenSSL::PKey.read(pub_pem)
rescue OpenSSL::PKey::PKeyError
# OpenSSL < 1.1.1
pend "Ed25519 is not implemented"
if !openssl?(1, 1, 1)
pend "Ed25519 is not implemented"
elsif OpenSSL.fips_mode && openssl?(3, 1, 0, 0)
# See OpenSSL providers/fips/fipsprov.c PROV_NAMES_ED25519 entries
# with FIPS_UNAPPROVED_PROPERTIES in OpenSSL 3.1+.
pend "Ed25519 is not approved in OpenSSL 3.1+ FIPS code"
end
end
assert_instance_of OpenSSL::PKey::PKey, priv
assert_instance_of OpenSSL::PKey::PKey, pub
Expand Down Expand Up @@ -143,7 +148,7 @@ def test_ed25519
end

def test_x25519
pend('Not supported on FIPS mode enabled') if OpenSSL.fips_mode
pend_on_openssl_issue_21493

# Test vector from RFC 7748 Section 6.1
alice_pem = <<~EOF
Expand Down Expand Up @@ -197,7 +202,7 @@ def raw_initialize
end

def test_compare?
pend('Not supported on FIPS mode enabled') if OpenSSL.fips_mode
pend_on_openssl_issue_21493
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why this is skipped. This test case doesn't seem to be using any FIPS-140 unapproved algorithms.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it. OpenSSL::PKey.read(der_encoded_string) failing is indeed the issue fixed by the OpenSSL change.

Copy link
Member Author

@junaruga junaruga Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really a good point.

When commenting out the pend_on_openssl_issue_21493, CI result is here.

diff --git a/test/openssl/test_pkey.rb b/test/openssl/test_pkey.rb
index da3ae5d..247dd96 100644
--- a/test/openssl/test_pkey.rb
+++ b/test/openssl/test_pkey.rb
@@ -82,7 +82,7 @@ class OpenSSL::TestPKey < OpenSSL::PKeyTestCase
   end
 
   def test_ed25519
-    pend_on_openssl_issue_21493
+    # pend_on_openssl_issue_21493
 
     # Test vector from RFC 8032 Section 7.1 TEST 2
     priv_pem = <<~EOF
@@ -148,7 +148,7 @@ class OpenSSL::TestPKey < OpenSSL::PKeyTestCase
   end
 
   def test_x25519
-    pend_on_openssl_issue_21493
+    # pend_on_openssl_issue_21493
 
     # Test vector from RFC 7748 Section 6.1
     alice_pem = <<~EOF
@@ -202,7 +202,7 @@ class OpenSSL::TestPKey < OpenSSL::PKeyTestCase
   end
 
   def test_compare?
-    pend_on_openssl_issue_21493
+    # pend_on_openssl_issue_21493
 
     key1 = Fixtures.pkey("rsa1024")
     key2 = Fixtures.pkey("rsa1024")

The result test_compare? with error looks quite different from the result test_x25519 (and test_ed25519 on openssl-3.1.10 fips case). Actually what I added the pend_on_openssl_issue_21493 was close to my mistake. I just missed why the test_compare? failed. But actually these test_ed25519, test_x25519 and test_compare? failures can be fixed on the same one commit openssl/openssl@2acb0d3. Note that PR's commits were merged as 3 commits to the openssl/openssl's master, openssl-3.1 and openssl-3.0 branches.

After I saw your comment here, I checked which commit actually fixed by running git bisect with test_pkey_test_compare.rb including only test_compare? test, and test_pkey_test_x25519.rb including only test_x25519 test.

I executed the git-bisect below with the script git-bisect-ruby-test.sh to find which commit fixes the test.

$ pwd
/home/jaruga/git/openssl2 # openssl/openssl repository, master branch

$ git bisect start --term-new=fixed --term-old=unfixed
$ git bisect fixed 4c50610bdadbcf7aa6bbd968df67b8874234677b
$ git bisect unfixed cb8e64131e7ce230a9268bdd7cc4664868ff0dc9

$ git bisect run ~/git/report-openssl-fips-base-decoding-corruption/git-bisect-ruby-test.sh
...
2acb0d363c0032b5b97c4f6596609f40bd7d842f is the first fixed commit
commit 2acb0d363c0032b5b97c4f6596609f40bd7d842f
Author: Tomas Mraz <tomas@openssl.org>
Date:   Fri Jul 21 17:40:31 2023 +0200

    When exporting/importing decoded keys do not use 0 as selection
    
    When decoding 0 as the selection means to decode anything
    you get.
     
    However when exporting and then importing the key data 0 as
    selection is not meaningful.
    So we set it to OSSL_KEYMGMT_SELECT_ALL to make the export/import
    function export/import everything that we have decoded.
    
    Fixes #21493
    
    Reviewed-by: Matt Caswell <matt@openssl.org>
    Reviewed-by: Paul Dale <pauli@openssl.org>
    Reviewed-by: Todd Short <todd.short@me.com>
    (Merged from https://github.com/openssl/openssl/pull/21519)

 crypto/encode_decode/decoder_pkey.c                         | 6 +++++-
 providers/implementations/encode_decode/decode_der2key.c    | 6 +++++-
 providers/implementations/encode_decode/decode_msblob2key.c | 6 +++++-
 providers/implementations/encode_decode/decode_pvk2key.c    | 6 +++++-
 4 files changed, 20 insertions(+), 4 deletions(-)
bisect found first bad commit

$ git bisect log
git bisect start '--term-new=fixed' '--term-old=unfixed'
# status: waiting for both good and bad commits
# fixed: [4c50610bdadbcf7aa6bbd968df67b8874234677b] endecode_test.c: Add tests for decoding with 0 selection
git bisect fixed 4c50610bdadbcf7aa6bbd968df67b8874234677b
# status: waiting for good commit(s), bad commit known
# unfixed: [cb8e64131e7ce230a9268bdd7cc4664868ff0dc9] no_autoload: make the no-autoload-config option work again.
git bisect unfixed cb8e64131e7ce230a9268bdd7cc4664868ff0dc9
# fixed: [2acb0d363c0032b5b97c4f6596609f40bd7d842f] When exporting/importing decoded keys do not use 0 as selection
git bisect fixed 2acb0d363c0032b5b97c4f6596609f40bd7d842f
# unfixed: [1ae4678cebaa13604c0f31bdf2c64cd28bdaf287] Avoid exporting bogus (empty) data if empty selection is used
git bisect unfixed 1ae4678cebaa13604c0f31bdf2c64cd28bdaf287
# first fixed commit: [2acb0d363c0032b5b97c4f6596609f40bd7d842f] When exporting/importing decoded keys do not use 0 as selection

And the openssl/openssl@2acb0d3 was the first fixed commit. I am not sure why this commit fixed the test_compare?. I have not debugged the case yet.

I will adjust or refactor the test/openssl/utils.rb#pend_on_openssl_issue_21493, and comment in the commit by this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After debugging this case, I can see that the OpenSSL::PKey.read(der_encoded_string) is failing. So, I am planning to apply a workaround conditionally to the c code ossl_pkey_read_generic, rather than pending tests. The workaround should be only executed in the affected OpenSSL versions with 2 or more providers (number of providers >= 2). Because this issue would happen if an encoding/decoding provider (e.g. "base") is different from the encryption provider (e.g. "fips").


key1 = Fixtures.pkey("rsa1024")
key2 = Fixtures.pkey("rsa1024")
Expand Down
16 changes: 16 additions & 0 deletions test/openssl/utils.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,22 @@ def libressl?(major = nil, minor = nil, fix = nil)
return false unless version
!major || (version.map(&:to_i) <=> [major, minor, fix]) >= 0
end

# OpenSSL 3: x25519 a decode from and then encode to a pem file corrupts the
# key if fips+base provider is used
# This issue happens in OpenSSL between 3.0,0 and 3.0.10 or between 3.1.0 and
# 3.1.2.
# https://github.com/openssl/openssl/issues/21493
# https://github.com/openssl/openssl/pull/21519
def pend_on_openssl_issue_21493
if OpenSSL.fips_mode &&
(
(openssl?(3, 0, 0, 0) && !openssl?(3, 0, 0, 11)) ||
(openssl?(3, 1, 0, 0) && !openssl?(3, 1, 0, 3))
)
pend('See <https://github.com/openssl/openssl/issues/21493>')
end
end
end

class OpenSSL::TestCase < Test::Unit::TestCase
Expand Down