Skip to content

Commit 901395a

Browse files
rzikmrzikm
authored
Properly dispose intermediate certificates in SslStream (#117667)
* Dispose intermediate certs from internally created SslStreamCertificateContext * Dispose intermediates received over the wire * Release specific platform resources on linux as well * Fix build * Dispose ChainElement certificates in SslStreamCertificateContext.Windows build. * Assert in tests that certificates are not disposed * Fix Duplicate * Check against ExtraStore and CustomTrastStore disposal * Fix crash
1 parent ac75c8d commit 901395a

File tree

7 files changed

+126
-30
lines changed

7 files changed

+126
-30
lines changed

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

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99

1010
namespace System.Net.Security
1111
{
12-
internal sealed class SslAuthenticationOptions
12+
internal sealed class SslAuthenticationOptions : IDisposable
1313
{
1414
private const string EnableOcspStaplingContextSwitchName = "System.Net.Security.EnableServerOcspStaplingFromOnlyCertificateOnLinux";
1515

@@ -153,7 +153,7 @@ internal void UpdateOptions(SslServerAuthenticationOptions sslServerAuthenticati
153153
bool ocspFetch = false;
154154
_ = AppContext.TryGetSwitch(EnableOcspStaplingContextSwitchName, out ocspFetch);
155155
// given cert is X509Certificate2 with key. We can use it directly.
156-
CertificateContext = SslStreamCertificateContext.Create(certificateWithKey, additionalCertificates: null, offline: false, trust: null, noOcspFetch: !ocspFetch);
156+
SetCertificateContextFromCert(certificateWithKey, !ocspFetch);
157157
}
158158
else
159159
{
@@ -165,7 +165,7 @@ internal void UpdateOptions(SslServerAuthenticationOptions sslServerAuthenticati
165165
throw new AuthenticationException(SR.net_ssl_io_no_server_cert);
166166
}
167167

168-
CertificateContext = SslStreamCertificateContext.Create(certificateWithKey);
168+
SetCertificateContextFromCert(certificateWithKey);
169169
}
170170
}
171171

@@ -195,13 +195,22 @@ private static SslProtocols FilterOutIncompatibleSslProtocols(SslProtocols proto
195195
return protocols;
196196
}
197197

198+
internal void SetCertificateContextFromCert(X509Certificate2 certificate, bool? noOcspFetch = null)
199+
{
200+
CertificateContext = SslStreamCertificateContext.Create(certificate, null, offline: false, null, noOcspFetch ?? true);
201+
OwnsCertificateContext = true;
202+
}
203+
198204
internal bool AllowRenegotiation { get; set; }
199205
internal string TargetHost { get; set; }
200206
internal X509CertificateCollection? ClientCertificates { get; set; }
201207
internal List<SslApplicationProtocol>? ApplicationProtocols { get; set; }
202208
internal bool IsServer { get; set; }
203209
internal bool IsClient => !IsServer;
204-
internal SslStreamCertificateContext? CertificateContext { get; set; }
210+
internal SslStreamCertificateContext? CertificateContext { get; private set; }
211+
// If true, the certificate context was created by the SslStream and
212+
// certificates inside should be disposed when no longer needed.
213+
internal bool OwnsCertificateContext { get; private set; }
205214
internal SslProtocols EnabledSslProtocols { get; set; }
206215
internal X509RevocationMode CertificateRevocationCheckMode { get; set; }
207216
internal EncryptionPolicy EncryptionPolicy { get; set; }
@@ -221,5 +230,17 @@ private static SslProtocols FilterOutIncompatibleSslProtocols(SslProtocols proto
221230
#if TARGET_ANDROID
222231
internal SslStream.JavaProxy? SslStreamProxy { get; set; }
223232
#endif
233+
234+
public void Dispose()
235+
{
236+
if (OwnsCertificateContext && CertificateContext != null)
237+
{
238+
CertificateContext.ReleaseResources();
239+
}
240+
241+
#if TARGET_ANDROID
242+
SslStreamProxy?.Dispose();
243+
#endif
244+
}
224245
}
225246
}

‎src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs‎

Lines changed: 27 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -176,9 +176,7 @@ internal void CloseContext()
176176
_securityContext?.Dispose();
177177
_credentialsHandle?.Dispose();
178178

179-
#if TARGET_ANDROID
180-
_sslAuthenticationOptions.SslStreamProxy?.Dispose();
181-
#endif
179+
_sslAuthenticationOptions.Dispose();
182180
}
183181

184182
//
@@ -583,11 +581,7 @@ internal bool AcquireClientCredentials(ref byte[]? thumbPrint, bool newCredentia
583581

584582
if (newCredentialsRequested)
585583
{
586-
if (selectedCert != null)
587-
{
588-
// build the cert context only if it was not provided by the user
589-
_sslAuthenticationOptions.CertificateContext ??= SslStreamCertificateContext.Create(selectedCert);
590-
}
584+
UpdateCertificateContext(selectedCert);
591585

592586
if (SslStreamPal.TryUpdateClintCertificate(_credentialsHandle, _securityContext, _sslAuthenticationOptions))
593587
{
@@ -645,31 +639,30 @@ internal bool AcquireClientCredentials(ref byte[]? thumbPrint, bool newCredentia
645639
NetEventSource.Log.UsingCachedCredential(this);
646640
_credentialsHandle = cachedCredentialHandle;
647641
cachedCred = true;
648-
if (selectedCert != null)
649-
{
650-
_sslAuthenticationOptions.CertificateContext ??= SslStreamCertificateContext.Create(selectedCert!);
651-
}
642+
UpdateCertificateContext(selectedCert);
652643
}
653644
else
654645
{
655-
if (selectedCert != null)
656-
{
657-
_sslAuthenticationOptions.CertificateContext ??= SslStreamCertificateContext.Create(selectedCert!);
658-
}
646+
UpdateCertificateContext(selectedCert);
659647

660648
_credentialsHandle = AcquireCredentialsHandle(_sslAuthenticationOptions, newCredentialsRequested);
661649
thumbPrint = guessedThumbPrint; // Delay until here in case something above threw.
662650
}
663651
}
664652
finally
665653
{
666-
if (selectedCert != null)
667-
{
668-
_sslAuthenticationOptions.CertificateContext ??= SslStreamCertificateContext.Create(selectedCert);
669-
}
654+
UpdateCertificateContext(selectedCert);
670655
}
671656

672657
return cachedCred;
658+
659+
void UpdateCertificateContext(X509Certificate2? cert)
660+
{
661+
if (cert != null && _sslAuthenticationOptions.CertificateContext == null)
662+
{
663+
_sslAuthenticationOptions.SetCertificateContextFromCert(cert);
664+
}
665+
}
673666
}
674667

675668
private static List<T> EnsureInitialized<T>(ref List<T>? list) => list ??= new List<T>();
@@ -743,7 +736,7 @@ private bool AcquireServerCredentials(ref byte[]? thumbPrint)
743736
}
744737

745738
Debug.Assert(localCertificate.Equals(selectedCert), "'selectedCert' does not match 'localCertificate'.");
746-
_sslAuthenticationOptions.CertificateContext = SslStreamCertificateContext.Create(selectedCert);
739+
_sslAuthenticationOptions.SetCertificateContextFromCert(selectedCert);
747740
}
748741

749742
Debug.Assert(_sslAuthenticationOptions.CertificateContext != null);
@@ -1054,6 +1047,13 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot
10541047
bool success = false;
10551048
X509Chain? chain = null;
10561049

1050+
// We need to note the number of certs in ExtraStore that were
1051+
// provided (by the user), we will add more from the received peer
1052+
// chain and we want to dispose only these after we perform the
1053+
// validation.
1054+
// TODO: this forces allocation of X509Certificate2Collection
1055+
int preexistingExtraCertsCount = _sslAuthenticationOptions.CertificateChainPolicy?.ExtraStore?.Count ?? 0;
1056+
10571057
try
10581058
{
10591059
X509Certificate2? certificate = CertificateValidationPal.GetRemoteCertificate(_securityContext, ref chain, _sslAuthenticationOptions.CertificateChainPolicy);
@@ -1164,6 +1164,12 @@ internal bool VerifyRemoteCertificate(RemoteCertificateValidationCallback? remot
11641164

11651165
if (chain != null)
11661166
{
1167+
// Dispose only the certificates that were added by GetRemoteCertificate
1168+
for (int i = preexistingExtraCertsCount; i < chain.ChainPolicy.ExtraStore.Count; i++)
1169+
{
1170+
chain.ChainPolicy.ExtraStore[i].Dispose();
1171+
}
1172+
11671173
int elementsCount = chain.ChainElements.Count;
11681174
for (int i = 0; i < elementsCount; i++)
11691175
{

‎src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -416,5 +416,19 @@ private static string MakeUrl(string baseUri, ArraySegment<char> encodedRequest)
416416

417417
return uriString;
418418
}
419+
420+
partial void ReleasePlatformSpecificResources()
421+
{
422+
Debug.Assert(_staplingForbidden, "Shouldn't release resources while OCSP stapling may be happening on the background.");
423+
424+
CertificateHandle.Dispose();
425+
KeyHandle.Dispose();
426+
_rootCertificate?.Dispose();
427+
428+
foreach (X509Certificate2 cert in _privateIntermediateCertificates)
429+
{
430+
cert.Dispose();
431+
}
432+
}
419433
}
420434
}

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,8 @@ private SslStreamCertificateContext(X509Certificate2 target, ReadOnlyCollection<
4242
count++;
4343
}
4444

45+
DisposeChainElements(chain);
46+
4547
// OS failed to build the chain but we have at least some intermediates.
4648
// We will try to add them to "Intermediate Certification Authorities" store.
4749
if (!osCanBuildChain)
@@ -92,6 +94,8 @@ private SslStreamCertificateContext(X509Certificate2 target, ReadOnlyCollection<
9294
}
9395
}
9496

97+
DisposeChainElements(chain);
98+
9599
if (!osCanBuildChain)
96100
{
97101
// Add also root to Intermediate CA store so OS can complete building chain.
@@ -123,6 +127,15 @@ static void TryAddToStore(X509Store store, X509Certificate2 certificate)
123127
}
124128
}
125129
}
130+
131+
static void DisposeChainElements(X509Chain chain)
132+
{
133+
int elementsCount = chain.ChainElements.Count;
134+
for (int i = 0; i < elementsCount; i++)
135+
{
136+
chain.ChainElements[i].Certificate.Dispose();
137+
}
138+
}
126139
}
127140
}
128141
}

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

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,29 @@ internal static SslStreamCertificateContext Create(
169169

170170
internal SslStreamCertificateContext Duplicate()
171171
{
172-
return new SslStreamCertificateContext(new X509Certificate2(TargetCertificate), IntermediateCertificates, Trust);
172+
// Create will internally clone any certificates that it will
173+
// retain, so we don't have to duplicate any of the instances here
174+
X509Certificate2Collection intermediates = new X509Certificate2Collection();
175+
foreach (X509Certificate2 cert in IntermediateCertificates)
176+
{
177+
intermediates.Add(cert);
178+
}
179+
180+
return Create(new X509Certificate2(TargetCertificate), intermediates, trust: Trust);
181+
}
182+
183+
internal void ReleaseResources()
184+
{
185+
// TargetCertificate is owned by the user, but we have created the cert context
186+
// which looked up intermediate certificates and only we have reference to them.
187+
foreach (X509Certificate2 cert in IntermediateCertificates)
188+
{
189+
cert.Dispose();
190+
}
191+
192+
ReleasePlatformSpecificResources();
173193
}
194+
195+
partial void ReleasePlatformSpecificResources();
174196
}
175197
}

‎src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,10 @@ public async Task SslStream_RequireClientCert_IsMutuallyAuthenticated_ReturnsTru
114114
Assert.False(server.IsMutuallyAuthenticated, "server.IsMutuallyAuthenticated");
115115
}
116116
}
117+
118+
// Assert that the certificates are not being disposed
119+
Assert.NotEqual(_clientCertificate.Handle, IntPtr.Zero);
120+
Assert.NotEqual(_serverCertificate.Handle, IntPtr.Zero);
117121
}
118122

119123
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindows7))]
@@ -152,7 +156,8 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout(
152156
// mutual authentication should only be set if server required client cert
153157
Assert.Equal(expectMutualAuthentication, server.IsMutuallyAuthenticated);
154158
Assert.Equal(expectMutualAuthentication, client.IsMutuallyAuthenticated);
155-
};
159+
}
160+
;
156161
}
157162
}
158163

@@ -268,7 +273,8 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout(
268273
{
269274
Assert.Null(server.RemoteCertificate);
270275
}
271-
};
276+
}
277+
;
272278
}
273279
}
274280

@@ -322,7 +328,8 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout(
322328
{
323329
Assert.Null(server.RemoteCertificate);
324330
}
325-
};
331+
}
332+
;
326333
}
327334
}
328335

@@ -380,7 +387,8 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout(
380387
{
381388
Assert.Null(server.RemoteCertificate);
382389
}
383-
};
390+
}
391+
;
384392
}
385393
}
386394

‎src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -807,6 +807,18 @@ public async Task SslStream_ServerUntrustedCaWithCustomTrust_OK(bool usePartialC
807807

808808
await TestConfiguration.WhenAllOrAnyFailedWithTimeout(t1, t2);
809809
}
810+
811+
// the ownership of the ExtraStore cert is not transferred to the
812+
// SslStream, so we need to verify that the certs are still valid.
813+
foreach (X509Certificate2 cert in clientOptions.CertificateChainPolicy.ExtraStore)
814+
{
815+
Assert.NotEqual(cert.Handle, IntPtr.Zero);
816+
}
817+
818+
foreach (X509Certificate2 cert in clientOptions.CertificateChainPolicy.CustomTrustStore)
819+
{
820+
Assert.NotEqual(cert.Handle, IntPtr.Zero);
821+
}
810822
}
811823

812824
private async Task SslStream_ClientSendsChain_Core(SslClientAuthenticationOptions clientOptions, X509Certificate2Collection clientChain)

0 commit comments

Comments
 (0)