Keep OSSL_LIB_CTX handles alive forever#129435
Conversation
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security |
There was a problem hiding this comment.
Pull request overview
This PR changes the OpenSSL provider-key implementation to cache OSSL_LIB_CTX / provider state for the lifetime of the process, simplifying “extra handle” lifetime management and re-enabling the OpenSSL named key test coverage that was previously disabled due to suspected invalid memory access.
Changes:
- Introduces a per-provider cache of
OSSL_LIB_CTX+OSSL_PROVIDERin the native OpenSSL PAL and stops freeing these contexts during normal key destruction. - Simplifies the EVP_PKEY native interop surface (
EvpPkeyDestroy,UpRefEvpPkey) by removing the extra-handle refcounting scheme. - Removes
ActiveIssueskips from OpenSSL named-key crypto and SslStream functional tests.
Show a summary per file
| File | Description |
|---|---|
| src/native/libs/System.Security.Cryptography.Native/pal_ssl.c | Updates EVP_PKEY destroy call to the simplified signature. |
| src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.h | Removes EvpPKeyExtraHandle definition and updates function signatures. |
| src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.c | Adds a global provider/libctx cache and removes extra-handle refcounting. |
| src/libraries/System.Security.Cryptography/tests/OpenSslNamedKeysTests.manual.cs | Re-enables named key tests by removing ActiveIssue attributes. |
| src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SafeEvpPKeyHandle.OpenSsl.cs | Stops passing ExtraHandle into destroy; clarifies ExtraHandle semantics. |
| src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamOpenSslNamedKeysTests.manual.cs | Re-enables SslStream TPM/provider tests by removing ActiveIssue attributes. |
| src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EvpPkey.cs | Updates P/Invoke signatures and adjusts failure cleanup accordingly. |
Copilot's findings
- Files reviewed: 7/7 changed files
- Comments generated: 1
This updates the LoadKeyFromProvider to treat extraHandle as an inout. When it points to NULL, that means create a new "extra" handle, in this case an OSSL_LIB_CTX. When it doesn't point to NULL, that means use this existing value. This keeps the number of P/Invokes the same, as well as keeping extra handle opaque - the managed side does not know that it is an OSSL_LIB_CTX.
bartonjs
left a comment
There was a problem hiding this comment.
Dictionary is overkill, but easy to work with, so... eh, it's fine.
|
No findings. Great job! 🎉 🔍 Reviewed by nitpicker |
|
/ba-g timeout on unrelated platform. The platform that timed out is Windows NAOT, this change only affects Unix. |
Freeing an OSSL_LIB_CTX is a somewhat complicated task. It registers per-thread callbacks to mutate thread-local state. Other OpenSSL APIs, like random number generation, also have thread-local state. In the case of the random number generator, it registered a thread data destructor to clean up random state when the thread is shutting down. This, in turn, sees that the OSSL_LIB_CTX is associated with that thread.
If the OSSL_LIB_CTX is freed before the thread has shut down, the thread dtor will access invalid memory.
OpenSSL requires you to call
OPENSSL_thread_stop_exon all threads before callingOSSL_LIB_CTX_freeto unhook the context from the thread.This is generally not feasible for .NET.
As a solution, we are going to not free the context, and re-use it for each provider. Every provider loaded with
SafeEvpPKeyHandle.OpenKeyFromProviderwill create a context and load that provider into it. The context and provider will remain alive for the duration of the process.The number of providers expected to be used in any application is generally expected to be low - likely two or three.
The provider and context are now cached, and kept in a heap-backed buffer. There is no resizing strategy or pre-allocated buffer. Opening a new provider results in re-sizing the array. This strategy is likely fine because there will rarely ever be more than a few providers, as already noted. The resizing is managed by a mutex to synchronize access to the array. We could use something like
pthread_rwlock_tif we wanted to increase complexity. Given that contention on this mutex would only happen fromOpenKeyFromProvider, I expect it to be rare.Since the context and provider are now alive permanently, the "extra handle" bookkeeping gets simpler. We no longer need to pass it to destroy, and we no longer need to ref-count it. In fact we don't even need a struct at all anymore, the OSSL_LIB_CTX is the extra handle now.
This reverts Add ActiveIssue for all named key tests to mitigate invalid memory access #129367 since the data dependency is resolved.
Fixes #129339