Skip to content
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.

Sign up for GitHub

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

Fix null reference exception in SslStream.Protocol #113606

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+22 −23
Open

Fix null reference exception in SslStream.Protocol #113606

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e6df2eb
Fix possible null reference exception in SslStream.Protocol
rzikm Mar 17, 2025
518af21
Update src/libraries/System.Net.Security/src/System/Net/Security/SslS…
rzikm Mar 17, 2025
072f264
code review feedback
rzikm Mar 18, 2025
b943978
Fix build
rzikm Mar 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 22 additions & 23 deletions src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,40 +194,44 @@ internal void CloseContext()
// Protecting from X509Certificate2 derived classes.
X509Certificate2? certEx = MakeEx(certificate);

if (certEx != null)
if (certEx is null)
{
if (certEx.HasPrivateKey)
{
if (NetEventSource.Log.IsEnabled())
NetEventSource.Log.CertIsType2(instance);
return null;
}

return certEx;
}
if (certEx.HasPrivateKey)
{
if (NetEventSource.Log.IsEnabled())
NetEventSource.Log.CertIsType2(instance);

if (!object.ReferenceEquals(certificate, certEx))
{
certEx.Dispose();
}
return certEx;
}

string certHash = certEx!.Thumbprint;
Span<byte> certHash = stackalloc byte[SHA512.HashSizeInBytes];
bool ret = certEx.TryGetCertHash(HashAlgorithmName.SHA512, certHash, out int written);
Debug.Assert(ret && written == certHash.Length);

if (!object.ReferenceEquals(certificate, certEx))
{
certEx.Dispose();
}

// ELSE Try the MY user and machine stores for private key check.
// For server side mode MY machine store takes priority.
X509Certificate2? found =
FindCertWithPrivateKey(isServer) ??
FindCertWithPrivateKey(!isServer);
FindCertWithPrivateKey(isServer, certHash) ??
FindCertWithPrivateKey(!isServer, certHash);
if (found is not null)
{
return found;
}

X509Certificate2? FindCertWithPrivateKey(bool isServer)
X509Certificate2? FindCertWithPrivateKey(bool isServer, ReadOnlySpan<byte> certHash)
{
if (CertificateValidationPal.EnsureStoreOpened(isServer) is X509Store store)
{
X509Certificate2Collection certs = store.Certificates;
X509Certificate2Collection found = certs.Find(X509FindType.FindByThumbprint, certHash, false);
X509Certificate2Collection found = certs.FindByThumbprint(HashAlgorithmName.SHA512, certHash);
Copy link
Member

Choose a reason for hiding this comment

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

Note that FindByThumbprint has different lifetime semantics than Find. Find would return an up-refed certificate, independent from the store. FindByThumbprint does not.

Since you iterate through all of the certs on line 261-264, found will get disposed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am a bit worried that this did not cause any of the existing tests to fail.

X509Certificate2? cert = null;
try
{
Expand All @@ -247,19 +251,14 @@ internal void CloseContext()
}
finally
{
for (int i = 0; i < found.Count; i++)
for (int i = 0; i < certs.Count; i++)
{
X509Certificate2 toDispose = found[i];
X509Certificate2 toDispose = certs[i];
if (!ReferenceEquals(toDispose, cert))
{
toDispose.Dispose();
}
}

for (int i = 0; i < certs.Count; i++)
{
certs[i].Dispose();
}
}
}

Expand Down
Loading