Do not call RAND_get0_public from within the FIPS provider initialization

[t8m]

This is necessary to avoid leaks on exit when the FIPS provider is initialized first in other thread than the main one.

Fixes #14437

[paulidale]

Nice find.

[mattcaswell]

Hmmm. It feels wrong that we should need to do this. Does this indicate some underlying bug in the init code?

[paulidale]

It could be a bug with the init code, I'm not sure how much effort we need to put into solving it. I don't think that insisting that initialisation takes place single threaded is onerous. It ought to be documented.

We should make an effort to at least understand the problem. Has @t8m done enough on this front?

[t8m]

Hmmm. It feels wrong that we should need to do this. Does this indicate some underlying bug in the init code?

The leaked data are from the initialization of the FIPS provider invoked from the FIPS provider shared library constructor.

The leak does not happen if FIPS provider is not configured in config file.

So yeah, this PR might be papering over a real issue.

The code path is something like this:

1. new thread created
2. it loads default provider
3. this triggers implicit initialization of libcrypto loading the config file
4. this triggers loading the FIPS provider as it is configured to be loaded
5. this runs constructor of the libfips.so
6. this triggers POST routine which initializes a libctx for the FIPS provider and drbg in it

The data from 6 are leaked.

This does not happen if the step 1 is avoided - i.e., the initial load and implicit initialization is done in the main thread.

[t8m]

I'll try to look into it some more as I am curious why there is the difference between running this in the main thread and a created thread.

[richsalz]

Something shoudl probably be documented, perhaps in doc/man7/openssl-threads.pod. It's not uncommon for multi-threaded programs to require initialization in the main thread.

[t8m]

Marking as draft for now while investigating some more.

[openssl-machine]

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.

[t8m]

My summary of investigation after many debugging runs:

• The leaked data is just associated with missing deallocation of the public and primary drbgs in the FIPS context.
• It can be reproduced by explicitly loading the FIPS provider as well (if you delete the activate=1 line from fipsmodule.cnf and load "fips" provider by OSSL_PROVIDER_load in a thread).
• More interestingly there is even bigger memory leak triggered if I just run RAND_get0_public() in the thread workers.
• But this memory leak does not happen if I first do OSSL_PROVIDER_load/unload("default") in the thread workers.
• But it happens again if I do RAND_get0_public() without the OSSL_PROVIDER_load/unload("default") in the main thread
• But it does not happen if I do RAND_get0_public() without the OSSL_PROVIDER_load/unload("default") in the main thread and not do any RAND_get0_public() in the thread workers.

Nice...

[mattcaswell]

So, do you now understand this problem sufficiently well to fix it?

[richsalz]

So, do you now understand this problem sufficiently well to fix it?

Or, perhaps, what to tell app developers on how to minimize the memory leaks or not worry about them.

[t8m]

So, do you now understand this problem sufficiently well to fix it?

Not yet fully. Unfortunately the leak by RAND_get0_public() without OSSL_PROVIDER_load() is actually a different problem which I fixed in the pushed commit.

Doing the global init is definitely a wrong fix though so I've dropped that.

[t8m]

This fixes the leak problem. However I suppose the problem would reappear if we decided that we have to always run KATs. The reason for the leak if the RAND_get0_public is called there was that the prov->provctx on the libcrypto side is not yet initialized when the ossl_init_thread_start is called for the ossl_ctx_thread_stop handler on the libctx from within the provider.

It is possible there is some better way how to fix the underlying problem however this at least is not just papering over the issue as the RAND_get0_public() is really not needed there anymore and it solves the leak.

[paulidale]

Looks okay.

[slontis]

The only reason this line was still there was that it was causing errors without it in the tests at some point. It looks like this issue may have shifted now..

[t8m]

I'm marking this urgent as this fixes the annoying CI failures. However I'd like to ask @mattcaswell to review before I'll merge.

[paulidale]

Urgent is good.

[mattcaswell]

I am quite sceptical about the need for these OPENSSL_init_crypto calls in the bio, engine and store code. The OPENSSL_INIT_BASE_ONLY option does 3 things:

• Creates the init lock
• Runs OPENSSL_cpuid_setup()
• Initialises the thread system we use for cleaning up thread local resources

bio/engine/store don't care about the init lock. That's entirely internal to the init code. They don't have any assembler code so shouldn't care about cpuid setup, and they don't use any thread local storage.

The previous calls where 0 was passed as the option did precisely nothing at all. So I'd like to know if anything breaks if you remove those calls altogether.

The calls in the err and rand code do make sense because both of those sub-systems utilise thread local storage.

[t8m]

OK, I removed those as suggested. @paulidale @mattcaswell please re-review.

[mattcaswell]

LGTM. But lets see the CIs go through before merging to make sure the removals haven't had some unexpected side effect.

[mattcaswell]

LGTM. Subject to CIs passing.

[t8m]

Aargh, the non-caching acvp test failure is actually valid.

[t8m]

Fortunately this is just wrong expectation in acvp_test and is easily fixable.

[t8m]

Heh, so re-review needed again. @paulidale @mattcaswell

[t8m]

Merged to master. This hopefully fixed the remaining intermittent failures. Or at least the common ones.

[t8m]

Thank you for the reviews