Skip to content

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

Merged
rzikm merged 3 commits into
dotnet:mainfrom
rzikm:109689-dotnet-OpenSSL-double-free-with-Ubuntu-2404
Mar 18, 2025
Merged

Link peer's X509 stack handle to parent SSL safe handle#113124
rzikm merged 3 commits into
dotnet:mainfrom
rzikm:109689-dotnet-OpenSSL-double-free-with-Ubuntu-2404

Conversation

@rzikm

@rzikmrzikm commented Mar 4, 2025

Copy link
Copy Markdown
Member

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.

CopilotAI review requested due to automatic review settings March 4, 2025 13:30

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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

FileDescription
src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.csRefactors 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>

@wfurtwfurt left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
MemberAuthor

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(SafeSharedX509StackHandlechainStack=
Interop.OpenSsl.GetPeerCertificateChain((SafeSslHandle)securityContext))
{
if(!chainStack.IsInvalid)
{
intcount=Interop.Crypto.GetX509StackFieldCount(chainStack);
for(inti=0;i<count;i++)
{
IntPtrcertPtr=Interop.Crypto.GetX509StackField(chainStack,i);
if(certPtr!=IntPtr.Zero)
{
// X509Certificate2(IntPtr) calls X509_dup, so the reference is appropriately tracked.
X509Certificate2chainCert=newX509Certificate2(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
Copy Markdown
MemberAuthor

/ba-g Test failures are unrelated

@rzikm
rzikm merged commit 0d3a436 into dotnet:mainMar 18, 2025
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 18, 2025
@rzikm

rzikm commented May 7, 2025

Copy link
Copy Markdown
MemberAuthor

/backport to release/8.0-staging

@rzikm

rzikm commented May 7, 2025

Copy link
Copy Markdown
MemberAuthor

/backport to release/9.0-staging

@github-actionsgithub-actionsBot unlocked this conversation May 7, 2025
@github-actions

Copy link
Copy Markdown
Contributor

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

@github-actions

Copy link
Copy Markdown
Contributor

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

@github-actionsgithub-actionsBot locked as resolved and limited conversation to collaborators May 7, 2025
Sign up for freeto 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

4 participants

@rzikm@vcsjones@wfurt