Skip to content

Link peer's X509 stack handle to parent SSL safe handle #113124

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.

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

Merged

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Mar 4, 2025

Fixes #109689.

Fixes rare data race between handshake and SslStream.Dispose() where we would try to access already freed STACKOF(X509) and crash.

I also reviewed other interop usages to make sure there are no similar bugs lurking elsewhere on SslStream codebase.

Tested locally by inserting artificial delays at relevant places.

@Copilot Copilot AI review requested due to automatic review settings March 4, 2025 13:30
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR addresses a race condition during SSL/TLS handshakes by restructuring how the peer certificate chain is retrieved.

  • Renames the original SslGetPeerCertChain method to a private implementation (SslGetPeerCertChain_private).
  • Introduces a new public wrapper for SslGetPeerCertChain that uses SafeInteriorHandle.OpenInteriorHandle.

Reviewed Changes

File Description
src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs Refactors peer certificate retrieval to link the parent SSL safe handle using a new wrapper method

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs:126

  • [nitpick] Consider renaming 'SslGetPeerCertChain_private' to a more conventional name such as 'SslGetPeerCertChainCore' to clarify its role as the internal implementation detail.
internal static partial SafeSharedX509StackHandle SslGetPeerCertChain_private(SafeSslHandle ssl);

…aphy.Native/Interop.Ssl.cs

Co-authored-by: Kevin Jones <vcsjones@github.com>
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM.
Do we have confidence that we do not leak the handle anyhow?
And I'm wondering if we could add some test that would for example connect and dispose in loop. While not perfect and 100% reliable we would at least have some coverage for such scenarios e.g. disposing in middle of some operation.

@rzikm
Copy link
Member Author

rzikm commented Mar 13, 2025

Do we have confidence that we do not leak the handle anyhow?

The usage in question puts the handle to a using block, the handle lives only while we get the entire cert chain from the SSL instance.

using (SafeSharedX509StackHandle chainStack =
Interop.OpenSsl.GetPeerCertificateChain((SafeSslHandle)securityContext))
{
if (!chainStack.IsInvalid)
{
int count = Interop.Crypto.GetX509StackFieldCount(chainStack);
for (int i = 0; i < count; i++)
{
IntPtr certPtr = Interop.Crypto.GetX509StackField(chainStack, i);
if (certPtr != IntPtr.Zero)
{
// X509Certificate2(IntPtr) calls X509_dup, so the reference is appropriately tracked.
X509Certificate2 chainCert = new X509Certificate2(certPtr);
chain.ChainPolicy.ExtraStore.Add(chainCert);
}
}
}
}

And I'm wondering if we could add some test that would for example connect and dispose in loop. While not perfect and 100% reliable we would at least have some coverage for such scenarios e.g. disposing in middle of some operation.

I will try and see how feasible it is.

@rzikm
Copy link
Member Author

rzikm commented Mar 17, 2025

/ba-g Test failures are unrelated

@rzikm rzikm merged commit 0d3a436 into dotnet:main Mar 18, 2025
80 of 84 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2025
@rzikm
Copy link
Member Author

rzikm commented May 7, 2025

/backport to release/8.0-staging

@rzikm
Copy link
Member Author

rzikm commented May 7, 2025

/backport to release/9.0-staging

@github-actions github-actions bot unlocked this conversation May 7, 2025
Copy link
Contributor

github-actions bot commented May 7, 2025

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/14891682108

Copy link
Contributor

github-actions bot commented May 7, 2025

Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/14891686789

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dotnet OpenSSL double free with Ubuntu 24.04
3 participants