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

Jump to bottom

rsa_padding_add_PKCS1_OAEP_mgf1_with_libctx(): fix check of |md| #11869

Closed
wants to merge 1 commit into from
Closed

rsa_padding_add_PKCS1_OAEP_mgf1_with_libctx(): fix check of |md| #11869

wants to merge 1 commit into from

Conversation

levitte
Copy link
Member

@levitte levitte commented May 19, 2020

In the FIPS module, the code as written generate an unconditional
error.

Fixes #11865

@levitte
rsa_padding_add_PKCS1_OAEP_mgf1_with_libctx(): fix check of |md|
d2a418f 
In the FIPS module, the code as written generate an unconditional
error.

Fixes openssl#11865
@levitte levitte added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels May 19, 2020
@levitte levitte requested a review from mattcaswell May 19, 2020 10:54
@levitte levitte mentioned this pull request May 19, 2020
Closed
t8m
t8m approved these changes May 19, 2020
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

LGTM

@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels May 19, 2020
paulidale
paulidale approved these changes May 19, 2020
View reviewed changes
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

This is the form I was going to comment with but @mattcaswell pipped me to it :)

@mattcaswell
Copy link
Member

Why didn't our tests pick this issue up?

bernd-edlinger
bernd-edlinger approved these changes May 19, 2020
View reviewed changes
Copy link
Member

@bernd-edlinger bernd-edlinger left a comment

Choose a reason for hiding this comment

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

good catch

@levitte
Copy link
Member Author

levitte commented May 19, 2020

Why didn't our tests pick this issue up?

I have no idea. Please someone investigate

@bernd-edlinger
Copy link
Member

yeah I will look.

@bernd-edlinger
Copy link
Member

This is test test case for the rsa pading:

$ ./util/shlib_wrap.sh test/rsa_test
1..4
    # Subtest: test_rsa_pkcs1
    1..3
    ok 1 - iteration 1
    ok 2 - iteration 2
    ok 3 - iteration 3
ok 1 - test_rsa_pkcs1
    # Subtest: test_rsa_sslv23
    1..3
    ok 1 - iteration 1
    ok 2 - iteration 2
    ok 3 - iteration 3
ok 2 - test_rsa_sslv23
    # Subtest: test_rsa_oaep
    1..3
    ok 1 - iteration 1
    ok 2 - iteration 2
    ok 3 - iteration 3
ok 3 - test_rsa_oaep
    # Subtest: test_rsa_security_bit
    1..12
    ok 1 - iteration 1
    ok 2 - iteration 2
    ok 3 - iteration 3
    ok 4 - iteration 4
    ok 5 - iteration 5
    ok 6 - iteration 6
    ok 7 - iteration 7
    ok 8 - iteration 8
    ok 9 - iteration 9
    ok 10 - iteration 10
    ok 11 - iteration 11
    ok 12 - iteration 12
ok 4 - test_rsa_security_bit

note I even added a test for SSLV23_PADDING when I
reworked that constant time RSA padding,
I would say it should be impossible how this test succeeds.

@mattcaswell
Copy link
Member

I would say it should be impossible how this test succeeds.

My guess is its only testing the default provider not the FIPS provider (which is where this issue occurs).

@mattcaswell
Copy link
Member

My guess is its only testing the default provider not the FIPS provider (which is where this issue occurs).

Although I would have expected evp_test to test this padding - and that should cover both providers.

@bernd-edlinger
Copy link
Member

and this compiles, so FIPS_MODULE is never defined:

diff --git a/crypto/rsa/rsa_oaep.c b/crypto/rsa/rsa_oaep.c
index 8ffde9f..fdfdf92 100644
--- a/crypto/rsa/rsa_oaep.c
+++ b/crypto/rsa/rsa_oaep.c
@@ -71,6 +71,7 @@ int rsa_padding_add_PKCS1_OAEP_mgf1_with_libctx(OPENSSL_CTX *libctx,
     if (md == NULL)
         md = EVP_sha1();
 #else
+ssss
         RSAerr(0, ERR_R_PASSED_NULL_PARAMETER);
         return 0;
 #endif

@bernd-edlinger
Copy link
Member

... Now I'm lost, the FIPS code is probably dead alltogrther,
and should be removed.

@levitte
Copy link
Member Author

levitte commented May 19, 2020

I have no idea what you're doing, @bernd-edlinger. When I add an "ssss" line like you fid, my compiler does complain!

@bernd-edlinger
Copy link
Member

hmm.
I have master e9e7b5d
here, and the code in question does compile perfectly for me.

diff --git a/crypto/rsa/rsa_oaep.c b/crypto/rsa/rsa_oaep.c
index 8ffde9f..05dc5d8 100644
--- a/crypto/rsa/rsa_oaep.c
+++ b/crypto/rsa/rsa_oaep.c
@@ -67,10 +67,12 @@ int rsa_padding_add_PKCS1_OAEP_mgf1_with_libctx(OPENSSL_CTX *libctx,
     unsigned char seedmask[EVP_MAX_MD_SIZE];
     int mdlen, dbmask_len = 0;
 
+asm("int3");
 #ifndef FIPS_MODULE
     if (md == NULL)
         md = EVP_sha1();
 #else
+ssss
         RSAerr(0, ERR_R_PASSED_NULL_PARAMETER);
         return 0;
 #endif


make
gcc  -I. -Iinclude -Iproviders/common/include -Iproviders/implementations/include -Icrypto/include  -DSTATIC_LEGACY -pthread -m64 -Wall -O0 -g -DOPENSSL_USE_NODELETE -DL_ENDIAN -DOPENSSLDIR="\"/usr/local/ssl\"" -DENGINESDIR="\"/usr/local/lib/engines-3\"" -DMODULESDIR="\"/usr/local/lib/ossl-modules\"" -DOPENSSL_BUILDING_OPENSSL  -MMD -MF crypto/rsa/libcrypto-lib-rsa_oaep.d.tmp -MT crypto/rsa/libcrypto-lib-rsa_oaep.o -c -o crypto/rsa/libcrypto-lib-rsa_oaep.o crypto/rsa/rsa_oaep.c

but the int3 makes the test/rsa_test fail:

$ test/rsa_test 
1..4
    # Subtest: test_rsa_pkcs1
    1..3
    ok 1 - iteration 1
    ok 2 - iteration 2
    ok 3 - iteration 3
ok 1 - test_rsa_pkcs1
    # Subtest: test_rsa_sslv23
    1..3
    ok 1 - iteration 1
    ok 2 - iteration 2
    ok 3 - iteration 3
ok 2 - test_rsa_sslv23
    # Subtest: test_rsa_oaep
    1..3
Trace/Breakpoint ausgelöst (Speicherabzug geschrieben)

I have configured with ./config -d no-asm no-pic no-dso

@levitte
Copy link
Member Author

levitte commented May 19, 2020

With no-pic, you don't get a FIPS module...

@richsalz
Copy link
Contributor

With no-pic, you don't get a FIPS module...

Perhaps Configure should output a message about that.

@bernd-edlinger
Copy link
Member

Yeah, if I want to debug something, I do usually try to build everthing statically,
debugging shared object is of course more fun....

@bernd-edlinger
Copy link
Member

OTOH, when there is not FIPS which test case should complain about that ?

@bernd-edlinger
Copy link
Member

03-test_fipsinstall.t .............. skipped: Test only supported in a fips build
that is probably the hint I overlooked :-)

@bernd-edlinger
Copy link
Member

but how can it be, that the no-pic version with int3 in the oaep code path
has only

15-test_rsa.t ...................... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/10 subtests 
	(less 2 skipped subtests: 7 okay)
80-test_cms.t ...................... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/7 subtests

but @mattcaswell you said that the evp test should also trigger?
why doesn't it trigger here?

@bernd-edlinger
Copy link
Member

Okay, now shared object: ./config -d

diff --git a/crypto/rsa/rsa_oaep.c b/crypto/rsa/rsa_oaep.c
index 8ffde9f..2a0e428 100644
--- a/crypto/rsa/rsa_oaep.c
+++ b/crypto/rsa/rsa_oaep.c
@@ -71,6 +71,7 @@ int rsa_padding_add_PKCS1_OAEP_mgf1_with_libctx(OPENSSL_CTX *libctx,
     if (md == NULL)
         md = EVP_sha1();
 #else
+asm("int3");
         RSAerr(0, ERR_R_PASSED_NULL_PARAMETER);
         return 0;
 #endif

@levitte yes, the ssss version does not compile but int3 does not cause any test failures.
so again the result is this is dead code.
the code in crypto/rsa/rsa_oaep.c is only used for cms and the deprecated RSA API,
and those are not available in FIPS.

That explains why this defect cant be found in our tests.

@bernd-edlinger
Copy link
Member

The same duplication I already saw with the AES code.
The new code is in the providers, but the old code is still
needed for the CMS and probably engines to use.
Nobody can maintain a code base that is designed that way.
Which alpha release will remove the duplication ?

@bernd-edlinger
Copy link
Member

Hmm, is it possible that the deprecated legacy AES API is
also not using the provider @slontis ?

@levitte
Copy link
Member Author

levitte commented May 19, 2020

It's possible that our OAEP tests aren't performed with the FIPS module. That doesn't make the code dead.

@bernd-edlinger
Copy link
Member

I wouldn't call it a good sign either.
How is this code supposed to be tested?

@mattcaswell
Copy link
Member

How is this code supposed to be tested?

There are a number of OAEP tests in test/recipes/30-test_evp_data/evppkey.txt. The EVP tests are supposed to be run by both the default provider and the FIPS provider, so I would have expected those tests to pick up this issue. I haven't investigated why it didn't.

@bernd-edlinger
Copy link
Member

Ah, the answer is easy:

grep crypt.*OAEP 30-test_evp_data/evppkey.txt
Decrypt=RSA-OAEP-1
Decrypt=RSA-OAEP-1
Decrypt=RSA-OAEP-1
Decrypt=RSA-OAEP-1
Decrypt=RSA-OAEP-1
Decrypt=RSA-OAEP-1
Decrypt=RSA-OAEP-2
Decrypt=RSA-OAEP-2
Decrypt=RSA-OAEP-2
Decrypt=RSA-OAEP-2
Decrypt=RSA-OAEP-2
Decrypt=RSA-OAEP-2
Decrypt=RSA-OAEP-3
Decrypt=RSA-OAEP-3
Decrypt=RSA-OAEP-3
Decrypt=RSA-OAEP-3
Decrypt=RSA-OAEP-3
Decrypt=RSA-OAEP-3
Decrypt=RSA-OAEP-4
Decrypt=RSA-OAEP-4
Decrypt=RSA-OAEP-4
Decrypt=RSA-OAEP-4
Decrypt=RSA-OAEP-4
Decrypt=RSA-OAEP-4
Decrypt=RSA-OAEP-5
Decrypt=RSA-OAEP-5
Decrypt=RSA-OAEP-5
Decrypt=RSA-OAEP-5
Decrypt=RSA-OAEP-5
Decrypt=RSA-OAEP-5
Decrypt=RSA-OAEP-6
Decrypt=RSA-OAEP-6
Decrypt=RSA-OAEP-6
Decrypt=RSA-OAEP-6
Decrypt=RSA-OAEP-6
Decrypt=RSA-OAEP-6
Decrypt=RSA-OAEP-7
Decrypt=RSA-OAEP-7
Decrypt=RSA-OAEP-7
Decrypt=RSA-OAEP-7
Decrypt=RSA-OAEP-7
Decrypt=RSA-OAEP-7
Decrypt=RSA-OAEP-8
Decrypt=RSA-OAEP-8
Decrypt=RSA-OAEP-8
Decrypt=RSA-OAEP-8
Decrypt=RSA-OAEP-8
Decrypt=RSA-OAEP-8
Decrypt=RSA-OAEP-9
Decrypt=RSA-OAEP-9
Decrypt=RSA-OAEP-9
Decrypt=RSA-OAEP-9
Decrypt=RSA-OAEP-9
Decrypt=RSA-OAEP-9
Decrypt=RSA-OAEP-10
Decrypt=RSA-OAEP-10
Decrypt=RSA-OAEP-10
Decrypt=RSA-OAEP-10
Decrypt=RSA-OAEP-10
Decrypt=RSA-OAEP-10

@bernd-edlinger
Copy link
Member

You can't have a known answer test when the encyption is randomized.

@mattcaswell
Copy link
Member

You can't have a known answer test when the encyption is randomized.

We should perhaps add a roundtrip (i.e. encrypt then decrypt) test in evp_extra_test for OAEP.

@levitte
Copy link
Member Author

levitte commented May 20, 2020

Could we add encryption stanzas for evp_test?

@mattcaswell
Copy link
Member

Could we add encryption stanzas for evp_test?

Well, I think the problem there is as @bernd-edlinger points out "You can't have a known answer test when the encyption is randomized."

@slontis
Copy link
Member

slontis commented May 20, 2020

"You can't have a known answer test when the encyption is randomized."

You can if you supply the entropy.

@mattcaswell
Copy link
Member

You can if you supply the entropy.

Do we have that capability in evp_test?

@slontis
Copy link
Member

slontis commented May 20, 2020

not yet :)

@bernd-edlinger
Copy link
Member

now, I am curious, are there FIPS test vectors for that?

@paulidale
Copy link
Contributor

Do we have that capability in evp_test?

We do at the moment but not in a nice manner. The DRBG tests implement something that is adequate.

The changes in #11682 will break what is currently done but the capability needs to be available and it is on the TODO list.

@slontis
Copy link
Member

slontis commented May 20, 2020

now, I am curious, are there FIPS test vectors for that?

I cant see any..
Interop tests with another toolkit would probably be better.
(Or debug another toolkit to grab its internal data).

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@levitte levitte added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels May 20, 2020
openssl-machine pushed a commit that referenced this pull request May 20, 2020
@levitte
rsa_padding_add_PKCS1_OAEP_mgf1_with_libctx(): fix check of |md|
e637d47 
In the FIPS module, the code as written generate an unconditional
error.

Fixes #11865

Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #11869)
@levitte
Copy link
Member Author

levitte commented May 20, 2020

Merged

e637d47 rsa_padding_add_PKCS1_OAEP_mgf1_with_libctx(): fix check of |md|

@levitte levitte closed this May 20, 2020
@levitte levitte deleted the fix-11865 branch May 20, 2020 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

@bernd-edlinger bernd-edlinger bernd-edlinger approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Openssl 3.0.0 alpha3 fips provider: OAEP encryption fails
8 participants
@levitte @mattcaswell @bernd-edlinger @richsalz @slontis @paulidale @openssl-machine @t8m