-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Link peer's X509 stack handle to parent SSL safe handle #113124
Conversation
There was a problem hiding this 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);
src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs
Outdated
Show resolved
Hide resolved
…aphy.Native/Interop.Ssl.cs Co-authored-by: Kevin Jones <vcsjones@github.com>
There was a problem hiding this 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.
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. runtime/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs Lines 57 to 76 in 4a8a95f
I will try and see how feasible it is. |
/ba-g Test failures are unrelated |
/backport to release/8.0-staging |
/backport to release/9.0-staging |
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/14891682108 |
Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/14891686789 |
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.