From 9f1b0c494395b191bada9bbe6c33f0f9c0b325bf Mon Sep 17 00:00:00 2001 From: Tomas Weinfurt Date: Mon, 24 Jun 2024 23:31:09 -0700 Subject: [PATCH 1/2] use also SslCertificateTrust when constructing CertificateContext (#103372) * use also SslCertificateTrust when constructing CertificateContext * 'build * feedback --- .../SslStreamCertificateContext.Android.cs | 1 + .../SslStreamCertificateContext.Linux.cs | 1 + .../SslStreamCertificateContext.OSX.cs | 1 + .../SslStreamCertificateContext.Windows.cs | 1 + .../Security/SslStreamCertificateContext.cs | 28 +++++++++++++++++-- .../SslStreamNetworkStreamTest.cs | 16 +++++++++-- .../System.Net.Security.Unit.Tests.csproj | 2 ++ 7 files changed, 45 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Android.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Android.cs index fec08b42818f32..4edbfaa2672ade 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Android.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Android.cs @@ -9,6 +9,7 @@ namespace System.Net.Security public partial class SslStreamCertificateContext { private const bool TrimRootCertificate = true; + private const bool ChainBuildNeedsTrustedRoot = true; private SslStreamCertificateContext(X509Certificate2 target, ReadOnlyCollection intermediates, SslCertificateTrust? trust) { 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 9c7902d7948b03..c7e60c10c61f4d 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 @@ -18,6 +18,7 @@ namespace System.Net.Security public partial class SslStreamCertificateContext { private const bool TrimRootCertificate = true; + private const bool ChainBuildNeedsTrustedRoot = false; internal readonly ConcurrentDictionary SslContexts; internal readonly SafeX509Handle CertificateHandle; internal readonly SafeEvpPKeyHandle KeyHandle; diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.OSX.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.OSX.cs index 9b79e28ceb213f..73feca46fef6ff 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.OSX.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.OSX.cs @@ -10,6 +10,7 @@ public partial class SslStreamCertificateContext { // No leaf, no root. private const bool TrimRootCertificate = true; + private const bool ChainBuildNeedsTrustedRoot = false; private SslStreamCertificateContext(X509Certificate2 target, ReadOnlyCollection intermediates, SslCertificateTrust? trust) { diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Windows.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Windows.cs index 33d8cd18e2d193..c13424c6fdb8a6 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Windows.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Windows.cs @@ -10,6 +10,7 @@ public partial class SslStreamCertificateContext { // No leaf, include root. private const bool TrimRootCertificate = false; + private const bool ChainBuildNeedsTrustedRoot = false; internal static SslStreamCertificateContext Create(X509Certificate2 target) { 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 dd148599f48329..2bcc96b4d9a62e 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 @@ -51,9 +51,19 @@ internal static SslStreamCertificateContext Create( { if (additionalCertificates != null) { - foreach (X509Certificate cert in additionalCertificates) + chain.ChainPolicy.ExtraStore.AddRange(additionalCertificates); + } + + if (trust != null) + { + chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust; + if (trust._store != null) + { + chain.ChainPolicy.CustomTrustStore.AddRange(trust._store.Certificates); + } + if (trust._trustList != null) { - chain.ChainPolicy.ExtraStore.Add(cert); + chain.ChainPolicy.CustomTrustStore.AddRange(trust._trustList); } } @@ -67,6 +77,20 @@ internal static SslStreamCertificateContext Create( NetEventSource.Error(null, $"Failed to build chain for {target.Subject}"); } + if (!chainStatus && ChainBuildNeedsTrustedRoot && additionalCertificates != null) + { + // Some platforms like Android may not be able to build the chain unless the chain root is trusted. + // We can try to rebuild the chain with making all extra certificates trused. + // We do not try to evaluate trust here, we jsut need to construct the chain so it should not matter. + chain.ChainPolicy.CustomTrustStore.AddRange(additionalCertificates); + chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust; + chainStatus = chain.Build(target); + if (!chainStatus && NetEventSource.Log.IsEnabled()) + { + NetEventSource.Error(null, $"Failed to build chain for {target.Subject} while trusting additional certificates"); + } + } + int count = chain.ChainElements.Count - 1; // Some platforms (e.g. Android) can't ignore all verification and will return zero diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs index 7e5ccca615aa24..bdc5bd57b82279 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs @@ -914,19 +914,29 @@ public async Task SslStream_ClientCertificate_SendsChain() } } - [Fact] + [Theory] + [InlineData(true)] + [InlineData(false)] [ActiveIssue("https://github.com/dotnet/runtime/issues/68206", TestPlatforms.Android)] - public async Task SslStream_ClientCertificateContext_SendsChain() + public async Task SslStream_ClientCertificateContext_SendsChain(bool useTrust) { (X509Certificate2 clientCertificate, X509Certificate2Collection clientChain) = TestHelper.GenerateCertificates(nameof(SslStream_ClientCertificateContext_SendsChain), serverCertificate: false); TestHelper.CleanupCertificates(nameof(SslStream_ClientCertificateContext_SendsChain)); + SslCertificateTrust? trust = null; + if (useTrust) + { + // This is simplification. We make all the intermediates trusted, + // normally just the root would go here. + trust = SslCertificateTrust.CreateForX509Collection(clientChain, false); + } + var clientOptions = new SslClientAuthenticationOptions() { TargetHost = "localhost", }; clientOptions.RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true; - clientOptions.ClientCertificateContext = SslStreamCertificateContext.Create(clientCertificate, clientChain); + clientOptions.ClientCertificateContext = SslStreamCertificateContext.Create(clientCertificate, useTrust ? null : clientChain, offline:true, trust); await SslStream_ClientSendsChain_Core(clientOptions, clientChain); diff --git a/src/libraries/System.Net.Security/tests/UnitTests/System.Net.Security.Unit.Tests.csproj b/src/libraries/System.Net.Security/tests/UnitTests/System.Net.Security.Unit.Tests.csproj index 53875c5618df1c..c66db5eea0d0c9 100644 --- a/src/libraries/System.Net.Security/tests/UnitTests/System.Net.Security.Unit.Tests.csproj +++ b/src/libraries/System.Net.Security/tests/UnitTests/System.Net.Security.Unit.Tests.csproj @@ -74,6 +74,8 @@ Link="ProductionCode\System\Net\Security\ReadWriteAdapter.cs" /> + Date: Fri, 28 Jun 2024 12:34:43 +0200 Subject: [PATCH 2/2] [Android] Fix SslStreamCertificateContext empty custom trust store exception (#104016) * Check if certificate collections are not empty before changing trust mode to custom root trust * Enable SslStream_ClientCertificateContext_SendsChain test on Android * Apply suggestions from reviews * Avoid unnecessary allocations --- .../System/Net/Security/SslStreamCertificateContext.cs | 9 +++++++-- .../tests/FunctionalTests/SslStreamNetworkStreamTest.cs | 1 - 2 files changed, 7 insertions(+), 3 deletions(-) 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 2bcc96b4d9a62e..b3540d646a1d25 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 @@ -56,15 +56,20 @@ internal static SslStreamCertificateContext Create( if (trust != null) { - chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust; if (trust._store != null) { chain.ChainPolicy.CustomTrustStore.AddRange(trust._store.Certificates); } + if (trust._trustList != null) { chain.ChainPolicy.CustomTrustStore.AddRange(trust._trustList); } + + if (chain.ChainPolicy.CustomTrustStore.Count > 0) + { + chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust; + } } chain.ChainPolicy.VerificationFlags = X509VerificationFlags.AllFlags; @@ -77,7 +82,7 @@ internal static SslStreamCertificateContext Create( NetEventSource.Error(null, $"Failed to build chain for {target.Subject}"); } - if (!chainStatus && ChainBuildNeedsTrustedRoot && additionalCertificates != null) + if (!chainStatus && ChainBuildNeedsTrustedRoot && additionalCertificates?.Count > 0) { // Some platforms like Android may not be able to build the chain unless the chain root is trusted. // We can try to rebuild the chain with making all extra certificates trused. diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs index bdc5bd57b82279..1efefaaca48f20 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamNetworkStreamTest.cs @@ -917,7 +917,6 @@ public async Task SslStream_ClientCertificate_SendsChain() [Theory] [InlineData(true)] [InlineData(false)] - [ActiveIssue("https://github.com/dotnet/runtime/issues/68206", TestPlatforms.Android)] public async Task SslStream_ClientCertificateContext_SendsChain(bool useTrust) { (X509Certificate2 clientCertificate, X509Certificate2Collection clientChain) = TestHelper.GenerateCertificates(nameof(SslStream_ClientCertificateContext_SendsChain), serverCertificate: false);