Skip to content

Commit 37ad965

Browse files
wfurtwfurt
authored
avoid allocating collection for intermediate certificates (#68188)
* avoid allocating collection for intermediate certificates * fix NRE for custom trust * feedback from review
1 parent 6c7ce85 commit 37ad965

File tree

10 files changed

+60
-120
lines changed

10 files changed

+60
-120
lines changed

‎src/libraries/Common/src/Interop/Windows/SChannel/UnmanagedCertificateContext.IntPtr.cs‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,11 @@ namespace System.Net
1010
{
1111
internal static partial class UnmanagedCertificateContext
1212
{
13-
internal static unsafe X509Certificate2Collection GetRemoteCertificatesFromStoreContext(IntPtr certContext)
13+
internal static unsafe void GetRemoteCertificatesFromStoreContext(IntPtr certContext, X509Certificate2Collection result)
1414
{
15-
X509Certificate2Collection result = new X509Certificate2Collection();
16-
1715
if (certContext == IntPtr.Zero)
1816
{
19-
return result;
17+
return;
2018
}
2119

2220
Interop.Crypt32.CERT_CONTEXT context = *(Interop.Crypt32.CERT_CONTEXT*)certContext;
@@ -46,8 +44,6 @@ internal static unsafe X509Certificate2Collection GetRemoteCertificatesFromStore
4644
last = next;
4745
}
4846
}
49-
50-
return result;
5147
}
5248
}
5349
}

‎src/libraries/Common/src/Interop/Windows/SChannel/UnmanagedCertificateContext.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,14 @@ namespace System.Net
1010
{
1111
internal static partial class UnmanagedCertificateContext
1212
{
13-
internal static X509Certificate2Collection GetRemoteCertificatesFromStoreContext(SafeFreeCertContext certContext)
13+
internal static void GetRemoteCertificatesFromStoreContext(SafeFreeCertContext certContext, X509Certificate2Collection collection)
1414
{
1515
if (certContext.IsInvalid)
1616
{
17-
return new X509Certificate2Collection();
17+
return;
1818
}
1919

20-
return GetRemoteCertificatesFromStoreContext(certContext.DangerousGetHandle());
20+
GetRemoteCertificatesFromStoreContext(certContext.DangerousGetHandle(), collection);
2121
}
2222
}
2323
}

‎src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -274,9 +274,9 @@ private static void OnRequestSendingRequest(WinHttpRequestState state)
274274
throw WinHttpException.CreateExceptionUsingError(lastError, "WINHTTP_CALLBACK_STATUS_SENDING_REQUEST/WinHttpQueryOption");
275275
}
276276

277-
// Get any additional certificates sent from the remote server during the TLS/SSL handshake.
278-
X509Certificate2Collection remoteCertificateStore =
279-
UnmanagedCertificateContext.GetRemoteCertificatesFromStoreContext(certHandle);
277+
// Get any additional certificates sent from the remote server during the TLS/SSL handshake.
278+
X509Certificate2Collection remoteCertificateStore = new X509Certificate2Collection();
279+
UnmanagedCertificateContext.GetRemoteCertificatesFromStoreContext(certHandle, remoteCertificateStore);
280280

281281
// Create a managed wrapper around the certificate handle. Since this results in duplicating
282282
// the handle, we will close the original handle after creating the wrapper.

‎src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Android.cs‎

Lines changed: 5 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -40,38 +40,18 @@ internal static SslPolicyErrors VerifyCertificateProperties(
4040
//
4141
// Extracts a remote certificate upon request.
4242
//
43-
internal static X509Certificate2? GetRemoteCertificate(SafeDeleteContext securityContext)
44-
{
45-
return GetRemoteCertificate(securityContext, null);
46-
}
47-
48-
internal static X509Certificate2? GetRemoteCertificate(
49-
SafeDeleteContext? securityContext,
50-
out X509Certificate2Collection? remoteCertificateStore)
51-
{
52-
if (securityContext == null)
53-
{
54-
remoteCertificateStore = null;
55-
return null;
56-
}
57-
58-
remoteCertificateStore = new X509Certificate2Collection();
59-
return GetRemoteCertificate(securityContext, remoteCertificateStore);
60-
}
6143

6244
private static X509Certificate2? GetRemoteCertificate(
6345
SafeDeleteContext securityContext,
64-
X509Certificate2Collection? remoteCertificateStore)
46+
bool retrieveChainCertificates,
47+
ref X509Chain? chain)
6548
{
66-
if (securityContext == null)
67-
return null;
68-
6949
SafeSslHandle sslContext = ((SafeDeleteSslContext)securityContext).SslContext;
7050
if (sslContext == null)
7151
return null;
7252

7353
X509Certificate2? cert = null;
74-
if (remoteCertificateStore == null)
54+
if (!retrieveChainCertificates)
7555
{
7656
// Constructing a new X509Certificate2 adds a global reference to the pointer, so we dispose this handle
7757
using (SafeX509Handle handle = Interop.AndroidCrypto.SSLStreamGetPeerCertificate(sslContext))
@@ -84,6 +64,7 @@ internal static SslPolicyErrors VerifyCertificateProperties(
8464
}
8565
else
8666
{
67+
chain ??= new X509Chain();
8768
IntPtr[]? ptrs = Interop.AndroidCrypto.SSLStreamGetPeerCertificates(sslContext);
8869
if (ptrs != null && ptrs.Length > 0)
8970
{
@@ -95,7 +76,7 @@ internal static SslPolicyErrors VerifyCertificateProperties(
9576
// Constructing a new X509Certificate2 adds a global reference to the pointer, so we dispose this handle
9677
using (var handle = new SafeX509Handle(ptr))
9778
{
98-
remoteCertificateStore.Add(new X509Certificate2(handle.DangerousGetHandle()));
79+
chain.ChainPolicy.ExtraStore.Add(new X509Certificate2(handle.DangerousGetHandle()));
9980
}
10081
}
10182

‎src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.OSX.cs‎

Lines changed: 6 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -47,31 +47,10 @@ internal static SslPolicyErrors VerifyCertificateProperties(
4747
return errors;
4848
}
4949

50-
//
51-
// Extracts a remote certificate upon request.
52-
//
53-
internal static X509Certificate2? GetRemoteCertificate(SafeDeleteContext securityContext)
54-
{
55-
return GetRemoteCertificate(securityContext, null);
56-
}
57-
58-
internal static X509Certificate2? GetRemoteCertificate(
59-
SafeDeleteContext? securityContext,
60-
out X509Certificate2Collection? remoteCertificateStore)
61-
{
62-
if (securityContext == null)
63-
{
64-
remoteCertificateStore = null;
65-
return null;
66-
}
67-
68-
remoteCertificateStore = new X509Certificate2Collection();
69-
return GetRemoteCertificate(securityContext, remoteCertificateStore);
70-
}
71-
7250
private static X509Certificate2? GetRemoteCertificate(
7351
SafeDeleteContext securityContext,
74-
X509Certificate2Collection? remoteCertificateStore)
52+
bool retrieveChainCertificates,
53+
ref X509Chain? chain)
7554
{
7655
if (securityContext == null)
7756
{
@@ -91,12 +70,14 @@ internal static SslPolicyErrors VerifyCertificateProperties(
9170
{
9271
long chainSize = Interop.AppleCrypto.X509ChainGetChainSize(chainHandle);
9372

94-
if (remoteCertificateStore != null)
73+
if (retrieveChainCertificates)
9574
{
75+
chain ??= new X509Chain();
76+
9677
for (int i = 0; i < chainSize; i++)
9778
{
9879
IntPtr certHandle = Interop.AppleCrypto.X509ChainGetCertificateAtIndex(chainHandle, i);
99-
remoteCertificateStore.Add(new X509Certificate2(certHandle));
80+
chain.ChainPolicy.ExtraStore.Add(new X509Certificate2(certHandle));
10081
}
10182
}
10283

‎src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs‎

Lines changed: 5 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -24,26 +24,7 @@ internal static SslPolicyErrors VerifyCertificateProperties(
2424
//
2525
// Extracts a remote certificate upon request.
2626
//
27-
internal static X509Certificate2? GetRemoteCertificate(SafeDeleteContext securityContext)
28-
{
29-
return GetRemoteCertificate(securityContext, null);
30-
}
31-
32-
internal static X509Certificate2? GetRemoteCertificate(
33-
SafeDeleteContext? securityContext,
34-
out X509Certificate2Collection? remoteCertificateStore)
35-
{
36-
if (securityContext == null)
37-
{
38-
remoteCertificateStore = null;
39-
return null;
40-
}
41-
42-
remoteCertificateStore = new X509Certificate2Collection();
43-
return GetRemoteCertificate(securityContext, remoteCertificateStore);
44-
}
45-
46-
private static X509Certificate2? GetRemoteCertificate(SafeDeleteContext? securityContext, X509Certificate2Collection? remoteCertificateStore)
27+
private static X509Certificate2? GetRemoteCertificate(SafeDeleteContext? securityContext, bool retrieveChainCertificates, ref X509Chain? chain)
4728
{
4829
bool gotReference = false;
4930

@@ -64,8 +45,10 @@ internal static SslPolicyErrors VerifyCertificateProperties(
6445
result = new X509Certificate2(remoteContext.DangerousGetHandle());
6546
}
6647

67-
if (remoteCertificateStore != null)
48+
if (retrieveChainCertificates)
6849
{
50+
chain ??= new X509Chain();
51+
6952
using (SafeSharedX509StackHandle chainStack =
7053
Interop.OpenSsl.GetPeerCertificateChain(((SafeDeleteSslContext)securityContext).SslContext))
7154
{
@@ -81,7 +64,7 @@ internal static SslPolicyErrors VerifyCertificateProperties(
8164
{
8265
// X509Certificate2(IntPtr) calls X509_dup, so the reference is appropriately tracked.
8366
X509Certificate2 chainCert = new X509Certificate2(certPtr);
84-
remoteCertificateStore.Add(chainCert);
67+
chain.ChainPolicy.ExtraStore.Add(chainCert);
8568
}
8669
}
8770
}

‎src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs‎

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -28,17 +28,9 @@ internal static SslPolicyErrors VerifyCertificateProperties(
2828
// Extracts a remote certificate upon request.
2929
//
3030

31-
internal static X509Certificate2? GetRemoteCertificate(SafeDeleteContext? securityContext) =>
32-
GetRemoteCertificate(securityContext, retrieveCollection: false, out _);
33-
34-
internal static X509Certificate2? GetRemoteCertificate(SafeDeleteContext? securityContext, out X509Certificate2Collection? remoteCertificateCollection) =>
35-
GetRemoteCertificate(securityContext, retrieveCollection: true, out remoteCertificateCollection);
36-
3731
private static X509Certificate2? GetRemoteCertificate(
38-
SafeDeleteContext? securityContext, bool retrieveCollection, out X509Certificate2Collection? remoteCertificateCollection)
32+
SafeDeleteContext? securityContext, bool retrieveChainCertificates, ref X509Chain? chain)
3933
{
40-
remoteCertificateCollection = null;
41-
4234
if (securityContext == null)
4335
{
4436
return null;
@@ -54,7 +46,7 @@ internal static SslPolicyErrors VerifyCertificateProperties(
5446
//
5547
// We can use retrieveCollection to distinguish between in-handshake and after-handshake calls, because
5648
// the collection is retrieved for cert validation purposes after the handshake completes.
57-
if (retrieveCollection) // handshake completed
49+
if (retrieveChainCertificates) // handshake completed
5850
{
5951
SSPIWrapper.QueryContextAttributes_SECPKG_ATTR_REMOTE_CERT_CONTEXT(GlobalSSPI.SSPISecureChannel, securityContext, out remoteContext);
6052
}
@@ -72,9 +64,11 @@ internal static SslPolicyErrors VerifyCertificateProperties(
7264
{
7365
if (remoteContext != null && !remoteContext.IsInvalid)
7466
{
75-
if (retrieveCollection)
67+
if (retrieveChainCertificates)
7668
{
77-
remoteCertificateCollection = UnmanagedCertificateContext.GetRemoteCertificatesFromStoreContext(remoteContext);
69+
chain ??= new X509Chain();
70+
71+
UnmanagedCertificateContext.GetRemoteCertificatesFromStoreContext(remoteContext, chain.ChainPolicy.ExtraStore);
7872
}
7973

8074
remoteContext.Dispose();

‎src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.cs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,11 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
using System.Diagnostics;
5+
using System.Net.Security;
56
using System.Security;
67
using System.Security.Cryptography;
78
using System.Security.Cryptography.X509Certificates;
9+
using System.Runtime.InteropServices;
810

911
namespace System.Net
1012
{
@@ -14,6 +16,13 @@ internal static partial class CertificateValidationPal
1416

1517
private static volatile X509Store? s_myCertStoreEx;
1618
private static volatile X509Store? s_myMachineCertStoreEx;
19+
private static X509Chain? s_chain;
20+
21+
internal static X509Certificate2? GetRemoteCertificate(SafeDeleteContext securityContext) =>
22+
GetRemoteCertificate(securityContext, retrieveChainCertificates: false, ref s_chain);
23+
24+
internal static X509Certificate2? GetRemoteCertificate(SafeDeleteContext securityContext, ref X509Chain? chain) =>
25+
GetRemoteCertificate(securityContext, retrieveChainCertificates: true, ref chain);
1726

1827
static partial void CheckSupportsStore(StoreLocation storeLocation, ref bool hasSupport);
1928

‎src/libraries/System.Net.Security/src/System/Net/Security/SecureChannel.cs‎

Lines changed: 6 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -993,12 +993,10 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot
993993
// We don't catch exceptions in this method, so it's safe for "accepted" be initialized with true.
994994
bool success = false;
995995
X509Chain? chain = null;
996-
X509Certificate2Collection? remoteCertificateStore = null;
997996

998997
try
999998
{
1000-
X509Certificate2? certificate = CertificateValidationPal.GetRemoteCertificate(_securityContext, out remoteCertificateStore);
1001-
999+
X509Certificate2? certificate = CertificateValidationPal.GetRemoteCertificate(_securityContext!, ref chain);
10021000
if (_remoteCertificate != null && certificate != null &&
10031001
certificate.RawDataMemory.Span.SequenceEqual(_remoteCertificate.RawDataMemory.Span))
10041002
{
@@ -1016,18 +1014,17 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot
10161014
}
10171015
else
10181016
{
1019-
chain = new X509Chain();
1017+
if (chain == null)
1018+
{
1019+
chain = new X509Chain();
1020+
}
1021+
10201022
chain.ChainPolicy.RevocationMode = _sslAuthenticationOptions.CertificateRevocationCheckMode;
10211023
chain.ChainPolicy.RevocationFlag = X509RevocationFlag.ExcludeRoot;
10221024

10231025
// Authenticate the remote party: (e.g. when operating in server mode, authenticate the client).
10241026
chain.ChainPolicy.ApplicationPolicy.Add(_sslAuthenticationOptions.IsServer ? s_clientAuthOid : s_serverAuthOid);
10251027

1026-
if (remoteCertificateStore != null)
1027-
{
1028-
chain.ChainPolicy.ExtraStore.AddRange(remoteCertificateStore);
1029-
}
1030-
10311028
if (trust != null)
10321029
{
10331030
chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust;
@@ -1103,15 +1100,6 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot
11031100

11041101
chain.Dispose();
11051102
}
1106-
1107-
if (remoteCertificateStore != null)
1108-
{
1109-
int certCount = remoteCertificateStore.Count;
1110-
for (int i = 0; i < certCount; i++)
1111-
{
1112-
remoteCertificateStore[i].Dispose();
1113-
}
1114-
}
11151103
}
11161104

11171105
return success;

‎src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Chain.cs‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -114,18 +114,26 @@ internal bool Build(X509Certificate2 certificate, bool throwOnException)
114114
if (certificate == null || certificate.Pal == null)
115115
throw new ArgumentException(SR.Cryptography_InvalidContextHandle, nameof(certificate));
116116

117-
if (_chainPolicy != null && _chainPolicy.CustomTrustStore != null)
117+
if (_chainPolicy != null)
118118
{
119-
if (_chainPolicy.TrustMode == X509ChainTrustMode.System && _chainPolicy.CustomTrustStore.Count > 0)
120-
throw new CryptographicException(SR.Cryptography_CustomTrustCertsInSystemMode);
121-
122-
foreach (X509Certificate2 customCertificate in _chainPolicy.CustomTrustStore)
119+
if (_chainPolicy._customTrustStore != null)
123120
{
124-
if (customCertificate == null || customCertificate.Handle == IntPtr.Zero)
121+
if (_chainPolicy.TrustMode == X509ChainTrustMode.System && _chainPolicy.CustomTrustStore.Count > 0)
122+
throw new CryptographicException(SR.Cryptography_CustomTrustCertsInSystemMode);
123+
124+
foreach (X509Certificate2 customCertificate in _chainPolicy.CustomTrustStore)
125125
{
126-
throw new CryptographicException(SR.Cryptography_InvalidTrustCertificate);
126+
if (customCertificate == null || customCertificate.Handle == IntPtr.Zero)
127+
{
128+
throw new CryptographicException(SR.Cryptography_InvalidTrustCertificate);
129+
}
127130
}
128131
}
132+
133+
if (_chainPolicy.TrustMode == X509ChainTrustMode.CustomRootTrust && _chainPolicy._customTrustStore == null)
134+
{
135+
_chainPolicy._customTrustStore = new X509Certificate2Collection();
136+
}
129137
}
130138

131139
Reset();

0 commit comments

Comments
 (0)