Fix null reference exception in SslStream.Protocol #113606
Conversation
ghost
added
the
|
/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);
| { | ||
| 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. |
|
Since the change to SHA-512 requires using a different API for potential private key lookups for client certificate selection (in case the client certificate instance does not have private key attached, we check the cert store and try to look it up based on a cert fingerprint), I wanted to do some perf measurements to make sure we don't significantly regress that scenario. When the private key is not attached, we iterate over all certs in the store on the C# side and manually calculate the SHA-512 hash, as opposed to passing SHA-1 to a Windows API which performs the search for us, thus there is regression in terms of memory allocated. But there does not seem to be measurable regression on time per handshake. The (I believe) more usual case where the X509Certificate2 instance contains the private key as well, is without regression Thus I think it is safe to proceed with the change. |
4 participants
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.