Fix X509 reference leak in SslStream extra chain certificates on Unix - #132351
Open
glima wants to merge 1 commit into
Open
Fix X509 reference leak in SslStream extra chain certificates on Unix#132351glima wants to merge 1 commit into
glima wants to merge 1 commit into
Conversation
CryptoNative_SslAddExtraChainCert uses SSL_ctrl(ssl, SSL_CTRL_CHAIN_CERT, 1, x509), which is SSL_add1_chain_cert and takes its own reference. Its sibling CryptoNative_SslCtxAddExtraChainCert uses SSL_CTX_add_extra_chain_cert, which is add0 and takes ownership of the caller's reference. Both are driven by managed helpers that are character for character identical (Interop.Ssl.cs:312 and Interop.SslCtx.cs:47): they up-ref with Crypto.X509UpRef and then call SetHandleAsInvalid to transfer ownership. That is the add0 contract, so against add1 two references are taken and neither is handed back. SetHandleAsInvalid also disarms the SafeHandle, so no finalizer reclaims it: the native X509, its X509_PUBKEY and the EVP_PKEY cached inside it survive until process exit, invisible to the GC and to a managed heap dump. Fixes dotnet#132350
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes the OpenSSL shim for adding “extra chain” certificates to an SSL* so it uses add0 ownership semantics (OpenSSL takes ownership of the caller’s reference) instead of add1 semantics (OpenSSL up-refs). This aligns the native helper with the existing managed call pattern (X509UpRef + SetHandleAsInvalid) and prevents a per-connection X509 reference leak on Unix when intermediates are added per SSL handle.
Changes:
- Switch
CryptoNative_SslAddExtraChainCertfromSSL_CTRL_CHAIN_CERTwithlarg=1tolarg=0. - Add an in-code comment documenting the intended ownership contract and why
larg=0is required.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #132350.
CryptoNative_SslAddExtraChainCertandCryptoNative_SslCtxAddExtraChainCerthave opposite ownership semantics, but the two managed helpers that drive them are character-for-character identical:CryptoNative_SslCtxAddExtraChainCertSSL_CTX_add_extra_chain_certCryptoNative_SslAddExtraChainCertSSL_ctrl(ssl, SSL_CTRL_CHAIN_CERT, **1**, x509)X509UpRef+SetHandleAsInvalidis the add0 contract. Against add1 two references are taken and neither is returned, so oneX509leaks per intermediate perSSLhandle.SetHandleAsInvalidalso disarms theSafeHandle, so nothing is finalized either — the nativeX509, itsX509_PUBKEYand theEVP_PKEYcached inside it live until process exit, invisible to the GC and todotnet-gcdump.This makes the SSL shim add0, matching its
SSL_CTXsibling and the contract the caller already implements.CryptoNative_SslAddExtraChainCerthas exactly one caller,Interop.Ssl.AddExtraChainCertificates, so the change is self-consistent.CryptoNative_SslCtxAddExtraChainCertis genuinely add0 and is untouched.Verification
Because the change compiles to a single immediate, I validated it by patching that one byte in a shipped
libSystem.Security.Cryptography.Native.OpenSsl.so(mov $0x1,%edx->mov $0x0,%edx) so the two runtimes under test differ by exactly one byte and nothing else. Each arm resolved through its own privateDOTNET_ROOT, verified from/proc/<pid>/maps.Counting every
X509by pointer (create at refcount 1,X509_up_ref+1,X509_free-1, entry dropped at 0) over 800 client connections presenting a client certificate with one intermediate, .NET 10.0.10 / OpenSSL 3.5.7:On the shared-context arm nothing accumulates because the same native certificate is reused, but the surplus reference is still directly visible:
X509_up_refon the intermediate over 800 connectionsX509UpRef+ssl_cert_add1_chain_cert)A self-contained repro that needs no tracing or binary patching is in the issue.
Not done
SslStreamtest that presents a certificate with intermediates over N connections and asserts the native certificate count does not grow, but that needs a hook the test suite does not have today. Happy to add one if you can point me at the right pattern.Origin
Found while diagnosing an OOM in the infrastructure behind an Azure PostgreSQL product: ~0.59 leaked certificates/s, ~14 MiB/h of native growth, linear across 19 hours, in a container with a flat ~9 MB managed heap. A forced gen2
gcdumpshowed 10 liveX509Certificate2against ~15,300 outstanding native certificates, which is what made it hard to attribute.