From 9e6f1721a06c3ec5c55f3a798e7de64fac177f56 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Thu, 16 Feb 2023 19:34:41 -0500 Subject: [PATCH] Fix disposing root X.509 certificate prematurely for OCSP stapling (#82116) In SslStreamCertificateContext, don't dispose of the root cert if it's about to be handed to the AddRootCert PAL call, which was the high-level cause of a segfault when handling certificate chains of length 2 in OCSP Stapling on Linux. This change additionally guards against disposed certificates in the OCSP Stapling retriever (disabling the feature instead of segfaulting), and adds tests to ensure that we don't regress 2-cert chains in the future. --- .../X509Certificates/CertificateAuthority.cs | 4 +- .../Security/SslStreamCertificateContext.Linux.cs | 14 ++- .../Net/Security/SslStreamCertificateContext.cs | 20 +++- .../CertificateValidationRemoteServer.cs | 130 ++++++++++++--------- 4 files changed, 103 insertions(+), 65 deletions(-) diff --git a/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/CertificateAuthority.cs b/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/CertificateAuthority.cs index 075ea2e..184d8a6 100644 --- a/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/CertificateAuthority.cs +++ b/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/CertificateAuthority.cs @@ -854,8 +854,8 @@ SingleResponse ::= SEQUENCE { rootAuthority = new CertificateAuthority( rootCert, rootDistributionViaHttp ? certUrl : null, - issuerRevocationViaCrl ? cdpUrl : null, - issuerRevocationViaOcsp ? ocspUrl : null); + issuerRevocationViaCrl || (endEntityRevocationViaCrl && intermediateAuthorityCount == 0) ? cdpUrl : null, + issuerRevocationViaOcsp || (endEntityRevocationViaOcsp && intermediateAuthorityCount == 0) ? ocspUrl : null); CertificateAuthority issuingAuthority = rootAuthority; intermediateAuthorities = new CertificateAuthority[intermediateAuthorityCount]; diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs index fdc9870..2057459 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs @@ -73,11 +73,12 @@ namespace System.Net.Security _staplingForbidden = noOcspFetch; } - partial void AddRootCertificate(X509Certificate2? rootCertificate) + partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool transferredOwnership) { if (IntermediateCertificates.Length == 0) { _ca = rootCertificate; + transferredOwnership = true; } else { @@ -197,6 +198,17 @@ namespace System.Net.Security IntPtr subject = Certificate.Handle; IntPtr issuer = caCert.Handle; + Debug.Assert(subject != 0); + Debug.Assert(issuer != 0); + + // This should not happen - but in the event that it does, we can't give null pointers when building the + // request, so skip stapling, and set it as forbidden so we don't bother looking for new stapled responses + // in the future. + if (subject == 0 || issuer == 0) + { + _staplingForbidden = true; + return null; + } using (SafeOcspRequestHandle ocspRequest = Interop.Crypto.X509BuildOcspRequest(subject, issuer)) { diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs index 9ea3971..5157942 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.cs @@ -96,8 +96,13 @@ namespace System.Net.Security // Dispose the copy of the target cert. chain.ChainElements[0].Certificate.Dispose(); - // Dispose the last cert, if we didn't include it. - for (int i = count + 1; i < chain.ChainElements.Count; i++) + // Dispose of the certificates that we do not need. If we are holding on to the root, + // don't dispose of it. + int stopDisposingChainPosition = root is null ? + chain.ChainElements.Count : + chain.ChainElements.Count - 1; + + for (int i = count + 1; i < stopDisposingChainPosition; i++) { chain.ChainElements[i].Certificate.Dispose(); } @@ -109,12 +114,19 @@ namespace System.Net.Security // On Linux, AddRootCertificate will start a background download of an OCSP response, // unless this context was built "offline", or this came from the internal Create(X509Certificate2) ctx.SetNoOcspFetch(offline || noOcspFetch); - ctx.AddRootCertificate(root); + + bool transferredOwnership = false; + ctx.AddRootCertificate(root, ref transferredOwnership); + + if (!transferredOwnership) + { + root?.Dispose(); + } return ctx; } - partial void AddRootCertificate(X509Certificate2? rootCertificate); + partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool transferredOwnership); partial void SetNoOcspFetch(bool noOcspFetch); internal SslStreamCertificateContext Duplicate() diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/CertificateValidationRemoteServer.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/CertificateValidationRemoteServer.cs index 1b0f91c..c77eece 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/CertificateValidationRemoteServer.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/CertificateValidationRemoteServer.cs @@ -106,9 +106,11 @@ namespace System.Net.Security.Tests [PlatformSpecific(TestPlatforms.Linux)] [ConditionalTheory] [OuterLoop("Subject to system load race conditions")] - [InlineData(false)] - [InlineData(true)] - public Task ConnectWithRevocation_StapledOcsp(bool offlineContext) + [InlineData(false, false)] + [InlineData(true, false)] + [InlineData(false, true)] + [InlineData(true, true)] + public Task ConnectWithRevocation_StapledOcsp(bool offlineContext, bool noIntermediates) { // Offline will only work if // a) the revocation has been checked recently enough that it is cached, or @@ -116,7 +118,7 @@ namespace System.Net.Security.Tests // // At high load, the server's background fetch might not have completed before // this test runs. - return ConnectWithRevocation_WithCallback_Core(X509RevocationMode.Offline, offlineContext); + return ConnectWithRevocation_WithCallback_Core(X509RevocationMode.Offline, offlineContext, noIntermediates); } [Fact] @@ -192,7 +194,8 @@ namespace System.Net.Security.Tests private async Task ConnectWithRevocation_WithCallback_Core( X509RevocationMode revocationMode, - bool? offlineContext = false) + bool? offlineContext = false, + bool noIntermediates = false) { string offlinePart = offlineContext.HasValue ? offlineContext.GetValueOrDefault().ToString().ToLower() : "null"; string serverName = $"{revocationMode.ToString().ToLower()}.{offlinePart}.server.example"; @@ -203,13 +206,15 @@ namespace System.Net.Security.Tests PkiOptions.EndEntityRevocationViaOcsp | PkiOptions.CrlEverywhere, out RevocationResponder responder, out CertificateAuthority rootAuthority, - out CertificateAuthority intermediateAuthority, + out CertificateAuthority[] intermediateAuthorities, out X509Certificate2 serverCert, + intermediateAuthorityCount: noIntermediates ? 0 : 1, subjectName: serverName, keySize: 2048, extensions: TestHelper.BuildTlsServerCertExtensions(serverName)); - X509Certificate2 issuerCert = intermediateAuthority.CloneIssuerCert(); + CertificateAuthority issuingAuthority = noIntermediates ? rootAuthority : intermediateAuthorities[0]; + X509Certificate2 issuerCert = issuingAuthority.CloneIssuerCert(); X509Certificate2 rootCert = rootAuthority.CloneIssuerCert(); SslClientAuthenticationOptions clientOpts = new SslClientAuthenticationOptions @@ -243,71 +248,80 @@ namespace System.Net.Security.Tests serverCert = temp; } - await using (clientStream) - await using (serverStream) - using (responder) - using (rootAuthority) - using (intermediateAuthority) - using (serverCert) - using (issuerCert) - using (rootCert) - await using (SslStream tlsClient = new SslStream(clientStream)) - await using (SslStream tlsServer = new SslStream(serverStream)) + try { - intermediateAuthority.Revoke(serverCert, serverCert.NotBefore); - - SslServerAuthenticationOptions serverOpts = new SslServerAuthenticationOptions(); - - if (offlineContext.HasValue) + await using (clientStream) + await using (serverStream) + using (responder) + using (rootAuthority) + using (serverCert) + using (issuerCert) + using (rootCert) + await using (SslStream tlsClient = new SslStream(clientStream)) + await using (SslStream tlsServer = new SslStream(serverStream)) { - serverOpts.ServerCertificateContext = SslStreamCertificateContext.Create( - serverCert, - new X509Certificate2Collection(issuerCert), - offlineContext.GetValueOrDefault()); + issuingAuthority.Revoke(serverCert, serverCert.NotBefore); - if (revocationMode == X509RevocationMode.Offline) + SslServerAuthenticationOptions serverOpts = new SslServerAuthenticationOptions(); + + if (offlineContext.HasValue) { - if (offlineContext.GetValueOrDefault(false)) - { - // Add a delay just to show we're not winning because of race conditions. - await Task.Delay(200); - } - else + serverOpts.ServerCertificateContext = SslStreamCertificateContext.Create( + serverCert, + new X509Certificate2Collection(issuerCert), + offlineContext.GetValueOrDefault()); + + if (revocationMode == X509RevocationMode.Offline) { - if (!OperatingSystem.IsLinux()) + if (offlineContext.GetValueOrDefault(false)) { - throw new InvalidOperationException( - "This test configuration uses reflection and is only defined for Linux."); + // Add a delay just to show we're not winning because of race conditions. + await Task.Delay(200); } - - FieldInfo pendingDownloadTaskField = typeof(SslStreamCertificateContext).GetField( - "_pendingDownload", - BindingFlags.Instance | BindingFlags.NonPublic); - - if (pendingDownloadTaskField is null) + else { - throw new InvalidOperationException("Cannot find the pending download field."); - } - - Task download = (Task)pendingDownloadTaskField.GetValue(serverOpts.ServerCertificateContext); - - // If it's null, it should mean it has already finished. If not, it might not have. - if (download is not null) - { - await download; + if (!OperatingSystem.IsLinux()) + { + throw new InvalidOperationException( + "This test configuration uses reflection and is only defined for Linux."); + } + + FieldInfo pendingDownloadTaskField = typeof(SslStreamCertificateContext).GetField( + "_pendingDownload", + BindingFlags.Instance | BindingFlags.NonPublic); + + if (pendingDownloadTaskField is null) + { + throw new InvalidOperationException("Cannot find the pending download field."); + } + + Task download = (Task)pendingDownloadTaskField.GetValue(serverOpts.ServerCertificateContext); + + // If it's null, it should mean it has already finished. If not, it might not have. + if (download is not null) + { + await download; + } } } } + else + { + serverOpts.ServerCertificate = serverCert; + } + + Task serverTask = tlsServer.AuthenticateAsServerAsync(serverOpts); + Task clientTask = tlsClient.AuthenticateAsClientAsync(clientOpts); + + await TestConfiguration.WhenAllOrAnyFailedWithTimeout(clientTask, serverTask); } - else + } + finally + { + foreach (CertificateAuthority intermediateAuthority in intermediateAuthorities) { - serverOpts.ServerCertificate = serverCert; + intermediateAuthority.Dispose(); } - - Task serverTask = tlsServer.AuthenticateAsServerAsync(serverOpts); - Task clientTask = tlsClient.AuthenticateAsClientAsync(clientOpts); - - await TestConfiguration.WhenAllOrAnyFailedWithTimeout(clientTask, serverTask); } static bool CertificateValidationCallback( -- 2.7.4