[URGENT] Fix sanitizer build #13661

levitte · 2020-12-10T18:23:13Z

DECODER EVP_PKEY: Don't store all the EVP_KEYMGMTs

OSSL_DECODER_CTX_new_by_EVP_PKEY() would keep copies of all the
EVP_KEYMGMTs it finds.
This turns out to be fragile in certain circumstances, so we switch to
fetch the appropriate EVP_KEYMGMT when it's time to construct an
EVP_PKEY from the decoded data instead. This has the added benefit
that we now actually use the property query string that was given by
the caller for these fetches.

Fixes #13503

MSBLOB & PVK: Make it possible to write EVP_PKEYs with provided internal key

So far, the MSBLOB and PVK writers could only handle EVP_PKEYs with
legacy internal keys.

Specially to be able to compile the loader_attic engine, we use the C
macro OPENSSL_NO_PROVIDER_CODE to avoid building the provider specific
things when we don't need them. The alternative is to suck half of
crypto/evp/ into loader_attic, and that's just not feasible.

Fixes #13503

DECODER: Adjust the library context of keys in our decoders

Because decoders are coupled with keymgmts from the same provider,
ours need to produce provider side keys the same way. Since our
keymgmts create key data with the provider library context, so must
our decoders.

We solve with functions to adjust the library context of decoded keys,
and use them.

CORE: Separate OSSL_PROVIDER activation from OSSL_PROVIDER reference

This introduces a separate activation counter, and the function
ossl_provider_deactivate() for provider deactivation.

Something to be noted is that if the reference count goes down to
zero, we don't care if the activation count is non-zero (i.e. someone
forgot to call ossl_provider_deactivate()). Since there are no more
references to the provider, it doesn't matter.
The important thing is that deactivation doesn't remove the provider
as long as there are references to it, for example because there are
live methods associated with that provider, but still makes the
provider unavailable to create new methods from.

Fixes #13503
Fixes #12157

EVP: Fix memory leak in EVP_PKEY_CTX_dup()

In most error cases, EVP_PKEY_CTX_dup() would only free the EVP_PKEY_CTX
without freeing the duplicated contents.

Fixes #13503

levitte · 2020-12-10T18:25:32Z

I'm getting issues with the tests on my laptop, like this:

        # INFO:  @ ../master/test/evp_test.c:3166
        # ../../../master/test/recipes/30-test_evp_data/evppkey_dsa.txt:269: Expected SIGNATURE_MISMATCH got DIGESTSIGNFINAL_ERROR
        # 804BC250547F0000:error:0300009C:digital envelope routines:inner_evp_generic_fetch:unsupported algorithm:../master/crypto/evp/evp_fetch.c:345:Global default library context, Algorithm (SHA512 : 150), Properties (<null>)
        # 804BC250547F0000:error:01800078:bignum routines:BN_generate_dsa_nonce:no suitable digest:../master/crypto/bn/bn_rand.c:279:
        # 804BC250547F0000:error:05080003:dsa routines:dsa_sign_setup:BN lib:../master/crypto/dsa/dsa_ossl.c:310:
        # 804BC250547F0000:error:05080003:dsa routines:dsa_do_sign_int:BN lib:../master/crypto/dsa/dsa_ossl.c:177:
        ...
        not ok 1 - ../../../master/test/recipes/30-test_evp_data/evppkey_dsa.txt
    not ok 1 - run_file_tests
../../util/wrap.pl ../../test/evp_test -config ../../../master/test/default-and-legacy.cnf ../../../master/test/recipes/30-test_evp_data/evppkey_dsa.txt => 1
not ok 21 - running evp_test -config ../../../master/test/default-and-legacy.cnf evppkey_dsa.txt
#   Failed test 'running evp_test -config ../../../master/test/default-and-legacy.cnf evppkey_dsa.txt'
#   at ../master/test/recipes/30-test_evp.t line 130.

This makes me think of the recent issues with seed stuff, but I thought @paulidale had fixed it and the fix was merged. I have no idead what's going on...

levitte · 2020-12-11T09:53:30Z

I have no idea what's going on...

I got a better idea now, and it has nothing to do with the seed stuff.

Our decoders must adjust the library context of the created keys to match what would come out of the KEYMGMT if it was generated.

levitte · 2020-12-11T11:04:54Z

Sanitizers still not happy... that looks like a leak elsewhere, though.

levitte · 2020-12-11T11:05:49Z

This is becoming a generic fixup PR... I'll change title and description accordingly

t8m · 2020-12-11T11:08:16Z

Sanitizers still not happy... that looks like a leak elsewhere, though.

But the leak or at least its appearance seems to be triggered by this PR.

levitte · 2020-12-11T11:09:44Z

But the leak or at least its appearance seems to be triggered by this PR.

Yeah, something's up, I agree.

levitte · 2020-12-11T11:36:45Z

Found 1 leak, I hope that's it.

levitte · 2020-12-11T11:53:41Z

I did a sanitizer run on my laptop, using the exact same setup (apart from no-asm) as the sanitizer build, and it passed. I do see that it didn't quite succeed on Github CI, there's a failure in test/evp_extra_test2.c, which I can't reproduce for the moment.

I'd be grateful if anyone could take a look at that last bit and help me figure it out

UPDATE: Maybe it's a no-asm problem?

mattcaswell · 2020-12-11T12:26:07Z

I'd be grateful if anyone could take a look at that last bit and help me figure it out

I can periodically reproduce it with OPENSSL_TEST_RAND_ORDER=0

mattcaswell · 2020-12-11T12:34:41Z

With this patch I can reliably reproduce this failure even in a build without sanitizers:

diff --git a/test/evp_extra_test2.c b/test/evp_extra_test2.c
index 9181061247..a78d5059fe 100644
--- a/test/evp_extra_test2.c
+++ b/test/evp_extra_test2.c
@@ -278,9 +278,9 @@ int setup_tests(void)
         return 0;
     }
 
+    ADD_TEST(test_d2i_PrivateKey_ex);
     ADD_TEST(test_alternative_default);
     ADD_ALL_TESTS(test_d2i_AutoPrivateKey_ex, OSSL_NELEM(keydata));
-    ADD_TEST(test_d2i_PrivateKey_ex);
 
     return 1;
 }

It looks to me like an instance of #12157

levitte · 2020-12-11T14:28:29Z

Thanks for the hint, now I can go in and see what can be done about it.

paulidale · 2020-12-12T07:04:08Z

There is no need for trial and error over the test order randomisation.

When running with OPENSSL_TEST_RAND_ORDER=0, a test failure will print "random seed = nnnnnnn".
Rerunning with OPENSSL_TEST_RAND_ORDER=nnnnnnn will execute the tests in the exact same order.
This should remain true even across platforms and operating systems.

I've created #13672 to make the randomisation failure text more obvious and to document how to use the env variable.

levitte · 2020-12-16T14:38:03Z

A new commit was added, which fixes #12157, and hopefully make the sanitizers happy.

("It works on my laptop!" -- famous last words)

levitte · 2020-12-16T14:39:46Z

This is urgent, so it would be nice to get a review soon.

t8m · 2020-12-16T15:12:38Z

It seems the old failures in the sanitizer are now fixed. There is a new one - leak in the provided test. However this is not a regression caused by this PR as the new failure is there on master already. I did not yet find out which merged PR caused it.

levitte · 2020-12-16T16:06:01Z

It seems the old failures in the sanitizer are now fixed. There is a new one - leak in the provided test. However this is not a regression caused by this PR as the new failure is there on master already. I did not yet find out which merged PR caused it.

I found that one and added a commit that I hope fixes the issue.

levitte · 2020-12-16T16:15:35Z

Now it works on my laptop.

levitte · 2020-12-16T16:18:18Z

Yay, sanitizer CI passes 😄

crypto/dh/dh_lib.c

t8m · 2020-12-17T08:22:08Z

Working on review but it will take me some time as the changes are quite non-trivial.

levitte · 2020-12-17T08:38:44Z

Working on review but it will take me some time as the changes are quite non-trivial.

I'd recommend reading them one commit at a time, that will at least make each change more concise.

Oh, and thank you!

levitte · 2020-12-17T08:39:58Z

I'd recommend reading them one commit at a time, that will at least make each change more concise.

With that in mind, should I perform a squash?

t8m · 2020-12-17T08:55:05Z

I'd recommend reading them one commit at a time, that will at least make each change more concise.

With that in mind, should I perform a squash?

Yes, please.

OSSL_DECODER_CTX_new_by_EVP_PKEY() would keep copies of all the EVP_KEYMGMTs it finds. This turns out to be fragile in certain circumstances, so we switch to fetch the appropriate EVP_KEYMGMT when it's time to construct an EVP_PKEY from the decoded data instead. This has the added benefit that we now actually use the property query string that was given by the caller for these fetches. Fixes openssl#13503

…nal key So far, the MSBLOB and PVK writers could only handle EVP_PKEYs with legacy internal keys. Specially to be able to compile the loader_attic engine, we use the C macro OPENSSL_NO_PROVIDER_CODE to avoid building the provider specific things when we don't need them. The alternative is to suck half of crypto/evp/ into loader_attic, and that's just not feasible. Fixes openssl#13503

Because decoders are coupled with keymgmts from the same provider, ours need to produce provider side keys the same way. Since our keymgmts create key data with the provider library context, so must our decoders. We solve with functions to adjust the library context of decoded keys, and use them.

This introduces a separate activation counter, and the function ossl_provider_deactivate() for provider deactivation. Something to be noted is that if the reference count goes down to zero, we don't care if the activation count is non-zero (i.e. someone forgot to call ossl_provider_deactivate()). Since there are no more references to the provider, it doesn't matter. The important thing is that deactivation doesn't remove the provider as long as there are references to it, for example because there are live methods associated with that provider, but still makes the provider unavailable to create new methods from. Fixes openssl#13503 Fixes openssl#12157

In most error cases, EVP_PKEY_CTX_dup() would only free the EVP_PKEY_CTX without freeing the duplicated contents. Fixes openssl#13503

levitte · 2020-12-17T08:57:44Z

With that in mind, should I perform a squash?

Yes, please.

Done

t8m

Just some doc nits, otherwise LGTM.

doc/internal/man3/ossl_provider_new.pod

…erence

t8m · 2020-12-17T10:56:48Z

Approved. Setting ready-to-merge as this is urgent.

levitte · 2020-12-17T11:01:12Z

Setting ready-to-merge as this is urgent.

Thanks. Will merge now

OSSL_DECODER_CTX_new_by_EVP_PKEY() would keep copies of all the EVP_KEYMGMTs it finds. This turns out to be fragile in certain circumstances, so we switch to fetch the appropriate EVP_KEYMGMT when it's time to construct an EVP_PKEY from the decoded data instead. This has the added benefit that we now actually use the property query string that was given by the caller for these fetches. Fixes #13503 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #13661)

…nal key So far, the MSBLOB and PVK writers could only handle EVP_PKEYs with legacy internal keys. Specially to be able to compile the loader_attic engine, we use the C macro OPENSSL_NO_PROVIDER_CODE to avoid building the provider specific things when we don't need them. The alternative is to suck half of crypto/evp/ into loader_attic, and that's just not feasible. Fixes #13503 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #13661)

Because decoders are coupled with keymgmts from the same provider, ours need to produce provider side keys the same way. Since our keymgmts create key data with the provider library context, so must our decoders. We solve with functions to adjust the library context of decoded keys, and use them. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #13661)

This introduces a separate activation counter, and the function ossl_provider_deactivate() for provider deactivation. Something to be noted is that if the reference count goes down to zero, we don't care if the activation count is non-zero (i.e. someone forgot to call ossl_provider_deactivate()). Since there are no more references to the provider, it doesn't matter. The important thing is that deactivation doesn't remove the provider as long as there are references to it, for example because there are live methods associated with that provider, but still makes the provider unavailable to create new methods from. Fixes #13503 Fixes #12157 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #13661)

In most error cases, EVP_PKEY_CTX_dup() would only free the EVP_PKEY_CTX without freeing the duplicated contents. Fixes #13503 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #13661)

levitte · 2020-12-17T11:04:11Z

Merged

054cde1 DECODER EVP_PKEY: Don't store all the EVP_KEYMGMTs
e77c13f MSBLOB & PVK: Make it possible to write EVP_PKEYs with provided internal key
6963979 DECODER: Adjust the library context of keys in our decoders
390f9ba CORE: Separate OSSL_PROVIDER activation from OSSL_PROVIDER reference
74cd923 EVP: Fix memory leak in EVP_PKEY_CTX_dup()

levitte · 2020-12-17T11:04:54Z

Thank you, @t8m!

levitte added branch: master Applies to master branch approval: review pending This pull request needs review by a committer severity: urgent Fixes an urgent issue (exempt from 24h grace period) labels Dec 10, 2020

levitte added this to the 3.0.0 beta1 milestone Dec 10, 2020

levitte mentioned this pull request Dec 10, 2020

build fails in test_encoder_decoder with OPENSSL_TEST_RAND_ORDER=0 #13503

Closed

levitte changed the title ~~DECODER EVP_PKEY: Don't store all the EVP_KEYMGMTs + PVK/MSBLOB handling of provided EVP_PKEYs~~ [URGENT] Fix 13503 Dec 11, 2020

levitte changed the title ~~[URGENT] Fix 13503~~ [URGENT] Fix sanitizer build Dec 11, 2020

levitte force-pushed the fix-13503 branch 2 times, most recently from ec054ce to bfbe5bf Compare December 16, 2020 14:36

levitte mentioned this pull request Dec 16, 2020

Fix a crash with multi-threaded applications using the FIPS module #13660

Closed

paulidale reviewed Dec 16, 2020

View reviewed changes

crypto/dh/dh_lib.c Outdated Show resolved Hide resolved

levitte mentioned this pull request Dec 17, 2020

Fix d2i_AutoPrivateKey_ex so that is uses the new decoder (and produces non legacy keys). #13591

Closed

2 tasks

levitte added 5 commits December 17, 2020 09:57

EVP: Fix memory leak in EVP_PKEY_CTX_dup()

af49f51

In most error cases, EVP_PKEY_CTX_dup() would only free the EVP_PKEY_CTX without freeing the duplicated contents. Fixes openssl#13503

levitte force-pushed the fix-13503 branch from 7b1f795 to af49f51 Compare December 17, 2020 08:57

t8m requested changes Dec 17, 2020

View reviewed changes

doc/internal/man3/ossl_provider_new.pod Outdated Show resolved Hide resolved

doc/internal/man3/ossl_provider_new.pod Outdated Show resolved Hide resolved

doc/internal/man3/ossl_provider_new.pod Outdated Show resolved Hide resolved

fixup! CORE: Separate OSSL_PROVIDER activation from OSSL_PROVIDER ref…

4f22699

…erence

t8m approved these changes Dec 17, 2020

View reviewed changes

t8m added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: review pending This pull request needs review by a committer labels Dec 17, 2020

levitte closed this Dec 17, 2020

levitte deleted the fix-13503 branch December 17, 2020 11:04

levitte mentioned this pull request Dec 17, 2020

Deprecate DSA harder (cont. from #13187) #13638

Closed

Uh oh!

[URGENT] Fix sanitizer build #13661

[URGENT] Fix sanitizer build #13661

Uh oh!

Conversation

levitte commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DECODER EVP_PKEY: Don't store all the EVP_KEYMGMTs

MSBLOB & PVK: Make it possible to write EVP_PKEYs with provided internal key

DECODER: Adjust the library context of keys in our decoders

CORE: Separate OSSL_PROVIDER activation from OSSL_PROVIDER reference

EVP: Fix memory leak in EVP_PKEY_CTX_dup()

Uh oh!

levitte commented Dec 10, 2020

Uh oh!

levitte commented Dec 11, 2020

Uh oh!

levitte commented Dec 11, 2020

Uh oh!

levitte commented Dec 11, 2020

Uh oh!

t8m commented Dec 11, 2020

Uh oh!

levitte commented Dec 11, 2020

Uh oh!

levitte commented Dec 11, 2020

Uh oh!

levitte commented Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattcaswell commented Dec 11, 2020

Uh oh!

mattcaswell commented Dec 11, 2020

Uh oh!

levitte commented Dec 11, 2020

Uh oh!

paulidale commented Dec 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Dec 16, 2020

Uh oh!

levitte commented Dec 16, 2020

Uh oh!

t8m commented Dec 16, 2020

Uh oh!

levitte commented Dec 16, 2020

Uh oh!

levitte commented Dec 16, 2020

Uh oh!

levitte commented Dec 16, 2020

Uh oh!

Uh oh!

t8m commented Dec 17, 2020

Uh oh!

levitte commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Dec 17, 2020

Uh oh!

t8m commented Dec 17, 2020

Uh oh!

levitte commented Dec 17, 2020

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t8m commented Dec 17, 2020

Uh oh!

levitte commented Dec 17, 2020

Uh oh!

levitte commented Dec 17, 2020

Uh oh!

levitte commented Dec 17, 2020

Uh oh!

Reviewers

Assignees

Labels

levitte commented Dec 10, 2020 •

edited

Loading

levitte commented Dec 11, 2020 •

edited

Loading

paulidale commented Dec 12, 2020 •

edited

Loading

levitte commented Dec 17, 2020 •

edited

Loading