Fix WinHttpHandler when building certificate chain (dotnet/corefx#23951)

* Fix WinHttpHandler when building certificate chain

Unlike SslStream, WinHttpHandler was not using all the certificates
obtained from the TLS/SSL handshake when building the chain. In
addition to the end-entity server certificate, there were other certs
available in the native cert handle that could be passed along
to the chain build operation.

As part of fixing this, I refactored the Common schannel and crypto
interop code so that it can be used in both SslStream and
WinHttpHandler.

I tested this manually because this cannot be easily tested in the CI
system. Added traces in WinHttpHandler to support this.

Fixes dotnet/corefx#23827
Fixes dotnet/corefx#4473

* Address PR feedback

Check for null IntPtr.

Commit migrated from https://github.com/dotnet/corefx/commit/dcc29528c9c4633e81b1937e7dd9fde7115a938c

diff --git a/src/libraries/Common/src/Interop/Windows/SChannel/UnmanagedCertificateContext.IntPtr.cs b/src/libraries/Common/src/Interop/Windows/SChannel/UnmanagedCertificateContext.IntPtr.cs
new file mode 100644 (file)
index 0000000..f3d53c9
--- /dev/null
+++ b/src/libraries/Common/src/Interop/Windows/SChannel/UnmanagedCertificateContext.IntPtr.cs
@@ -0,0 +1,51 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System.Diagnostics;
+using System.Net.Security;
+using System.Runtime.InteropServices;
+using System.Security.Cryptography.X509Certificates;
+
+namespace System.Net
+{
+    internal static unsafe partial class UnmanagedCertificateContext
+    {
+        internal static X509Certificate2Collection GetRemoteCertificatesFromStoreContext(IntPtr certContext)
+        {
+            X509Certificate2Collection result = new X509Certificate2Collection();
+
+            if (certContext == IntPtr.Zero)
+            {
+                return result;
+            }
+
+            Interop.Crypt32.CERT_CONTEXT context =
+                Marshal.PtrToStructure<Interop.Crypt32.CERT_CONTEXT>(certContext);
+
+            if (context.hCertStore != IntPtr.Zero)
+            {
+                Interop.Crypt32.CERT_CONTEXT* last = null;
+
+                while (true)
+                {
+                    Interop.Crypt32.CERT_CONTEXT* next =
+                        Interop.Crypt32.CertEnumCertificatesInStore(context.hCertStore, last);
+
+                    if (next == null)
+                    {
+                        break;
+                    }
+
+                    var cert = new X509Certificate2(new IntPtr(next));
+                    if (NetEventSource.IsEnabled) NetEventSource.Info(certContext, $"Adding remote certificate:{cert}");
+
+                    result.Add(cert);
+                    last = next;
+                }
+            }
+
+            return result;
+        }        
+    }
+}

diff --git a/src/libraries/Common/src/Interop/Windows/SChannel/UnmanagedCertificateContext.cs b/src/libraries/Common/src/Interop/Windows/SChannel/UnmanagedCertificateContext.cs
index 66c4a60..73b856f 100644 (file)
--- a/src/libraries/Common/src/Interop/Windows/SChannel/UnmanagedCertificateContext.cs
+++ b/src/libraries/Common/src/Interop/Windows/SChannel/UnmanagedCertificateContext.cs
@@ -9,43 +9,16 @@ using System.Security.Cryptography.X509Certificates;
 
 namespace System.Net
 {
-    internal static unsafe class UnmanagedCertificateContext
+    internal static unsafe partial class UnmanagedCertificateContext
     {
         internal static X509Certificate2Collection GetRemoteCertificatesFromStoreContext(SafeFreeCertContext certContext)
         {
-            X509Certificate2Collection result = new X509Certificate2Collection();
-
             if (certContext.IsInvalid)
             {
-                return result;
-            }
-
-            Interop.Crypt32.CERT_CONTEXT context =
-                Marshal.PtrToStructure<Interop.Crypt32.CERT_CONTEXT>(certContext.DangerousGetHandle());
-
-            if (context.hCertStore != IntPtr.Zero)
-            {
-                Interop.Crypt32.CERT_CONTEXT* last = null;
-
-                while (true)
-                {
-                    Interop.Crypt32.CERT_CONTEXT* next =
-                        Interop.Crypt32.CertEnumCertificatesInStore(context.hCertStore, last);
-
-                    if (next == null)
-                    {
-                        break;
-                    }
-
-                    var cert = new X509Certificate2(new IntPtr(next));
-                    if (NetEventSource.IsEnabled) NetEventSource.Info(certContext, $"Adding remote certificate:{cert}");
-
-                    result.Add(cert);
-                    last = next;
-                }
+                return new X509Certificate2Collection();
             }
 
-            return result;
+            return GetRemoteCertificatesFromStoreContext(certContext.DangerousGetHandle());
         }
     }
 }

diff --git a/src/libraries/System.Net.Http.WinHttpHandler/src/System.Net.Http.WinHttpHandler.csproj b/src/libraries/System.Net.Http.WinHttpHandler/src/System.Net.Http.WinHttpHandler.csproj
index 4c0e2e8..78b6c50 100644 (file)
--- a/src/libraries/System.Net.Http.WinHttpHandler/src/System.Net.Http.WinHttpHandler.csproj
+++ b/src/libraries/System.Net.Http.WinHttpHandler/src/System.Net.Http.WinHttpHandler.csproj
@@ -35,6 +35,9 @@
     <Compile Include="$(CommonPath)\System\Net\HttpVersionInternal.cs">
       <Link>Common\System\Net\HttpVersionInternal.cs</Link>
     </Compile>
+    <Compile Include="$(CommonPath)\System\Net\Logging\NetEventSource.Common.cs">
+      <Link>Common\System\Net\Logging\NetEventSource.Common.cs</Link>
+    </Compile>
   </ItemGroup>
   <ItemGroup>
     <Reference Include="System.Buffers" />

diff --git a/src/libraries/System.Net.Http.WinHttpHandler/src/System.Net.Http.WinHttpHandler.msbuild b/src/libraries/System.Net.Http.WinHttpHandler/src/System.Net.Http.WinHttpHandler.msbuild
index 2661bfc..eca0378 100644 (file)
--- a/src/libraries/System.Net.Http.WinHttpHandler/src/System.Net.Http.WinHttpHandler.msbuild
+++ b/src/libraries/System.Net.Http.WinHttpHandler/src/System.Net.Http.WinHttpHandler.msbuild
@@ -5,11 +5,13 @@
   </PropertyGroup>
   <ItemGroup>
     <CompileItem Include="$(CommonPath)\Interop\Windows\Interop.Libraries.cs" />
+    <CompileItem Include="$(CommonPath)\Interop\Windows\Crypt32\Interop.CertEnumCertificatesInStore.cs" />
     <CompileItem Include="$(CommonPath)\Interop\Windows\Crypt32\Interop.certificates_types.cs" />
     <CompileItem Include="$(CommonPath)\Interop\Windows\Crypt32\Interop.certificates.cs" />
     <CompileItem Include="$(CommonPath)\Interop\Windows\kernel32\Interop.FormatMessage.cs" />
     <CompileItem Include="$(CommonPath)\Interop\Windows\kernel32\Interop.GetModuleHandle.cs" />
     <CompileItem Include="$(CommonPath)\Interop\Windows\Interop.HRESULT_FROM_WIN32.cs" />
+    <CompileItem Include="$(CommonPath)\Interop\Windows\SChannel\UnmanagedCertificateContext.IntPtr.cs" />
     <CompileItem Include="$(CommonPath)\Interop\Windows\winhttp\Interop.SafeWinHttpHandle.cs" />
     <CompileItem Include="$(CommonPath)\Interop\Windows\winhttp\Interop.winhttp_types.cs" />
     <CompileItem Include="$(CommonPath)\Interop\Windows\winhttp\Interop.winhttp.cs" />
@@ -17,8 +19,8 @@
     <CompileItem Include="$(CommonPath)\System\StringExtensions.cs" />
     <CompileItem Include="$(CommonPath)\System\Net\HttpKnownHeaderNames.cs" />
     <CompileItem Include="$(CommonPath)\System\Net\HttpKnownHeaderNames.TryGetHeaderName.cs" />
-    <CompileItem Include="$(CommonPath)\System\Net\UriScheme.cs" />
     <CompileItem Include="$(CommonPath)\System\Net\SecurityProtocol.cs" />
+    <CompileItem Include="$(CommonPath)\System\Net\UriScheme.cs" />
     <CompileItem Include="$(CommonPath)\System\Net\Http\HttpHandlerDefaults.cs" />
     <CompileItem Include="$(CommonPath)\System\Net\Http\NoWriteNoSeekStreamContent.cs" />
     <CompileItem Include="$(CommonPath)\System\Net\Http\WinHttpException.cs" />

diff --git a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpCertificateHelper.cs b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpCertificateHelper.cs
index 6868120..ca315bc 100644 (file)
--- a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpCertificateHelper.cs
+++ b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpCertificateHelper.cs
@@ -16,6 +16,7 @@ namespace System.Net.Http
         // TODO: Issue #2165. Merge with similar code used in System.Net.Security move to Common/src//System/Net.
         public static void BuildChain(
             X509Certificate2 certificate,
+            X509Certificate2Collection remoteCertificateStore,
             string hostName,
             bool checkCertificateRevocationList,
             out X509Chain chain,
@@ -32,6 +33,19 @@ namespace System.Net.Http
             // Authenticate the remote party: (e.g. when operating in client mode, authenticate the server).
             chain.ChainPolicy.ApplicationPolicy.Add(s_serverAuthOid);
 
+            if (remoteCertificateStore.Count > 0)
+            {
+                if (WinHttpTraceHelper.IsTraceEnabled())
+                {
+                    foreach (X509Certificate cert in remoteCertificateStore)
+                    {
+                        WinHttpTraceHelper.Trace("WinHttpCertificateHelper.BuildChain: adding cert to ExtraStore: {0}", cert.Subject);
+                    }
+                }
+
+                chain.ChainPolicy.ExtraStore.AddRange(remoteCertificateStore);
+            }
+
             if (!chain.Build(certificate))
             {
                 sslPolicyErrors |= SslPolicyErrors.RemoteCertificateChainErrors;

diff --git a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs
index 636229a..30c64f9 100644 (file)
--- a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs
+++ b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestCallback.cs
@@ -258,7 +258,11 @@ namespace System.Net.Http
 
                     throw WinHttpException.CreateExceptionUsingError(lastError);
                 }
-                
+
+                // Get any additional certificates sent from the remote server during the TLS/SSL handshake.
+                X509Certificate2Collection remoteCertificateStore =
+                    UnmanagedCertificateContext.GetRemoteCertificatesFromStoreContext(certHandle);
+
                 // Create a managed wrapper around the certificate handle. Since this results in duplicating
                 // the handle, we will close the original handle after creating the wrapper.
                 var serverCertificate = new X509Certificate2(certHandle);
@@ -271,6 +275,7 @@ namespace System.Net.Http
                 {
                     WinHttpCertificateHelper.BuildChain(
                         serverCertificate,
+                        remoteCertificateStore,
                         state.RequestMessage.RequestUri.Host,
                         state.CheckCertificateRevocationList,
                         out chain,

diff --git a/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/System.Net.Http.WinHttpHandler.Unit.Tests.csproj b/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/System.Net.Http.WinHttpHandler.Unit.Tests.csproj
index 9892003..871e666 100644 (file)
--- a/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/System.Net.Http.WinHttpHandler.Unit.Tests.csproj
+++ b/src/libraries/System.Net.Http.WinHttpHandler/tests/UnitTests/System.Net.Http.WinHttpHandler.Unit.Tests.csproj
@@ -19,12 +19,18 @@
     <Compile Include="$(CommonPath)\Interop\Windows\Interop.Libraries.cs">
       <Link>Common\Interop\Windows\Interop.Libraries.cs</Link>
     </Compile>
+    <Compile Include="$(CommonPath)\Interop\Windows\Crypt32\Interop.CertEnumCertificatesInStore.cs">
+      <Link>Common\Interop\Windows\Crypt32\Interop.CertEnumCertificatesInStore.cs</Link>
+    </Compile>
     <Compile Include="$(CommonPath)\Interop\Windows\Crypt32\Interop.certificates_types.cs">
       <Link>Common\Interop\Windows\Crypt32\Interop.certificates_types.cs</Link>
     </Compile>
     <Compile Include="$(CommonPath)\Interop\Windows\Interop.HRESULT_FROM_WIN32.cs">
       <Link>Common\Interop\Windows\Interop.HRESULT_FROM_WIN32.cs</Link>
     </Compile>
+    <Compile Include="$(CommonPath)\Interop\Windows\SChannel\UnmanagedCertificateContext.IntPtr.cs">
+      <Link>Common\Interop\Windows\SChannel\UnmanagedCertificateContext.IntPtr.cs</Link>
+    </Compile>
     <Compile Include="$(CommonPath)\Interop\Windows\winhttp\Interop.SafeWinHttpHandle.cs">
       <Link>Common\Interop\Windows\winhttp\Interop.SafeWinHttpHandle.cs</Link>
     </Compile>
@@ -52,6 +58,9 @@
     <Compile Include="$(CommonPath)\System\Net\HttpVersionInternal.cs">
       <Link>Common\System\Net\HttpVersionInternal.cs</Link>
     </Compile>
+    <Compile Include="$(CommonPath)\System\Net\Logging\NetEventSource.Common.cs">
+      <Link>Common\System\Net\Logging\NetEventSource.Common.cs</Link>
+    </Compile>
     <Compile Include="$(CommonPath)\System\Net\UriScheme.cs">
       <Link>Common\System\Net\UriScheme.cs</Link>
     </Compile>

diff --git a/src/libraries/System.Net.Security/src/System.Net.Security.csproj b/src/libraries/System.Net.Security/src/System.Net.Security.csproj
index 9b3d8de..c8756c3 100644 (file)
--- a/src/libraries/System.Net.Security/src/System.Net.Security.csproj
+++ b/src/libraries/System.Net.Security/src/System.Net.Security.csproj
@@ -162,6 +162,9 @@
     <Compile Include="$(CommonPath)\Interop\Windows\SChannel\UnmanagedCertificateContext.cs">
       <Link>Common\Interop\Windows\SChannel\UnmanagedCertificateContext.cs</Link>
     </Compile>
+    <Compile Include="$(CommonPath)\Interop\Windows\SChannel\UnmanagedCertificateContext.IntPtr.cs">
+      <Link>Common\Interop\Windows\SChannel\UnmanagedCertificateContext.IntPtr.cs</Link>
+    </Compile>
     <Compile Include="$(CommonPath)\Interop\Windows\sspicli\SecPkgContext_Bindings.cs">
       <Link>Common\Interop\Windows\sspicli\SecPkgContext_Bindings.cs</Link>
     </Compile>