Fix X509Chain build error reporting on Windows (dotnet/corefx#36150)

This issue started out with a difference in behavior between .NET Core 2.0
and .NET Core 2.1. An HTTP request to a TLS website worked in 2.0 and failed
in 2.1. The website used a certificate that chained to an untrusted root. But
the certificate itself was placed in the Windows Local Machine/TrustedPeople cert
store.

It was first thought this was a difference between how WinHttpHandler (WinHTTP)
and SocketsHttpHandler worked. But it turned out to be a problem with SslStream.

The problem can also be observed on current .NET Core 3.0 (master) switching between
WinHttpHandler (disable SocketsHttpHandler via AppContext) and the default
SocketsHttpHandler.

In both cases, the X509Chain built has an error in its X509ChainStatus array. The
single error is 'PartialChain'. The X509Chain.Build() method will try to build a chain
using the applicable policies set into the X509Chain before building. This includes
implied operating system policy such as trusting certificates that are in the
TrustedPeople cert store.  X509Chain.Build() returns a boolean indicating if the
chain was successfully built. There might still be entries in the X509ChainStatus
array indicating "soft failures" that were ignored based on policy.

We have some duplicative/similar code in WinHttpHandler and SslStream for building
an X509Chain object. The WinHttpHandler code and the Unix version of SslStream were
both correctly using the bool return value of X509Chain.Build() in determining whether
or not to return a final SslPolicyErrors.RemoteCertificateChainErrors. But the Windows
version of SslStream was not using that boolean. Instead, it was actually checking the
X509ChainStatus array.

This PR fixes the logic to be consistent and only check the result of X509Chain.Build().

I tested this change with CI tests as well as putting the certificate into the TrustedPeople
cert store (and also removing it).

Fixes dotnet/corefx#29563

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

diff --git a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs
index 5ace8dc..aa46f2d 100644 (file)
--- a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs
+++ b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Windows.cs
@@ -24,7 +24,8 @@ namespace System.Net
         {
             SslPolicyErrors sslPolicyErrors = SslPolicyErrors.None;
 
-            if (!chain.Build(remoteCertificate)       // Build failed on handle or on policy.
+            bool chainBuildResult = chain.Build(remoteCertificate);
+            if (!chainBuildResult       // Build failed on handle or on policy.
                 && chain.SafeHandle.DangerousGetHandle() == IntPtr.Zero)   // Build failed to generate a valid handle.
             {
                 throw new CryptographicException(Marshal.GetLastWin32Error());
@@ -69,8 +70,7 @@ namespace System.Net
                 }
             }
 
-            X509ChainStatus[] chainStatusArray = chain.ChainStatus;
-            if (chainStatusArray != null && chainStatusArray.Length != 0)
+            if (!chainBuildResult)
             {
                 sslPolicyErrors |= SslPolicyErrors.RemoteCertificateChainErrors;
             }