Fix null reference exception in SslStream.Protocol #113606
Conversation
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a null reference exception in SslStream.Protocol by ensuring that an invalid certificate conversion is handled correctly and improves the certificate lookup by switching from the thumbprint string to a SHA512-based hash.
- Return early from the method when the certificate conversion fails (null certificate).
- Update the certificate hash retrieval to use GetCertHash with SHA512.
- Adjust the certificate store lookup to use the new FindByThumbprint API with the SHA512 hash.
Comments suppressed due to low confidence (2)
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs:197
- With the updated null check returning early, please ensure there are unit tests covering cases where certificate conversion fails, so that the early return behavior is validated.
if (certEx is null)
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs:210
- Verify that using GetCertHash with SHA512 produces the correct hash format needed for the FindByThumbprint lookup to avoid mismatches with certificate store entries.
byte[] certHash = certEx!.GetCertHash(HashAlgorithmName.SHA512);
src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs
Outdated
Show resolved
Hide resolved
…tream.Protocol.cs Co-authored-by: Marie Píchová <[email protected]>
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| } | ||
|
|
||
| string certHash = certEx!.Thumbprint; | ||
| byte[] certHash = certEx.GetCertHash(HashAlgorithmName.SHA512); |
There was a problem hiding this comment.
If I remember well the Thumbprint was cached inside X509 e.g. calling it again would not allocate new array. I'm wondering we can use stack here to avoid allocation...?
There was a problem hiding this comment.
f I remember well the Thumbprint was cached inside X509 e.g. calling it again would not allocate new array.
Thumbprint allocates a string every time it is accessed.
I'm wondering we can use stack here to avoid allocation.
Something like
Span<byte> thumbprint = stackalloc byte[SHA512.HashSizeInBytes];
bool ret = certEx.TryGetCertHash(HashAlgorithmName.SHA512, thumbprint, out int written);
Debug.Assert(ret && written == SHA512.HashSizeInBytes);
| @@ -227,7 +229,7 @@ internal void CloseContext() | |||
| 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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I am a bit worried that this did not cause any of the existing tests to fail.
|
/ba-g wasm build failure is unrelated. |
Fixes #110804.
While at it, I also mad sure we are using SHA512 of the certificate during lookup as we do in other parts of the area.