CheckTrustedIssuer: Fixes for invalid chains (#2665)

NickCraver · web-flow · commit baf2b1e818f0 · 2024-03-09T09:53:11.000-05:00
This issue was brought to my attention last night (thanks to Badrish Chandramouli): dotnet/dotnet-api-docs#6660 This changeset ensures that we do not honor self-signed certs or partial/broken chains as a result of `X509VerificationFlags.AllowUnknownCertificateAuthority` downstream and adds a few tests and utilities to generate test certificates (currently valid for ~9000 days). Instead we are checking that the certificate we're being told to trust is explicitly in the chain, given that the result of `.Build()` cannot be trusted for this case. This also resolves an issue where `TrustIssuer` could be called but we'd error when _no errors_ were detected (due to requiring chain errors in our validator), this means users couldn't temporarily trust a cert while getting it installed on the machine for instance and migrating between the 2 setups was difficult. This needs careful eyes, please scrutinize heavily. It's possible this breaks an existing user, but...it should be broken if so unless there's a case I'm not seeing.
diff --git a/docs/ReleaseNotes.md b/docs/ReleaseNotes.md
@@ -9,6 +9,8 @@ Current package versions:
 ## Unreleased
 
 - Add new `LoggingTunnel` API; see https://stackexchange.github.io/StackExchange.Redis/Logging ([#2660 by mgravell](https://github.com/StackExchange/StackExchange.Redis/pull/2660))
+- **Potentiallty Breaking**: Fix `CheckTrustedIssuer` certificate validation for broken chain scenarios ([#2665 by NickCraver](https://github.com/StackExchange/StackExchange.Redis/pull/2665))
+  - Users inadvertently trusting a remote cert with a broken chain could not be failing custom validation before this change. This is only in play if you are using `ConfigurationOptions.TrustIssuer` at all.
 
 ## 2.7.27
 
diff --git a/src/StackExchange.Redis/ConfigurationOptions.cs b/src/StackExchange.Redis/ConfigurationOptions.cs
@@ -6,6 +6,7 @@
 using System.Net.Security;
 using System.Net.Sockets;
 using System.Security.Authentication;
+using System.Security.Cryptography;
 using System.Security.Cryptography.X509Certificates;
 using System.Text;
 using System.Threading;
@@ -279,13 +280,13 @@ public bool CheckCertificateRevocation
         }
 
         /// <summary>
-        /// Create a certificate validation check that checks against the supplied issuer even if not known by the machine.
+        /// Create a certificate validation check that checks against the supplied issuer even when not known by the machine.
         /// </summary>
         /// <param name="issuerCertificatePath">The file system path to find the certificate at.</param>
         public void TrustIssuer(string issuerCertificatePath) => CertificateValidationCallback = TrustIssuerCallback(issuerCertificatePath);
 
         /// <summary>
-        /// Create a certificate validation check that checks against the supplied issuer even if not known by the machine.
+        /// Create a certificate validation check that checks against the supplied issuer even when not known by the machine.
         /// </summary>
         /// <param name="issuer">The issuer to trust.</param>
         public void TrustIssuer(X509Certificate2 issuer) => CertificateValidationCallback = TrustIssuerCallback(issuer);
@@ -296,24 +297,65 @@ private static RemoteCertificateValidationCallback TrustIssuerCallback(X509Certi
         {
             if (issuer == null) throw new ArgumentNullException(nameof(issuer));
 
-            return (object _, X509Certificate? certificate, X509Chain? __, SslPolicyErrors sslPolicyError)
-                => sslPolicyError == SslPolicyErrors.RemoteCertificateChainErrors
-                    && certificate is X509Certificate2 v2
-                    && CheckTrustedIssuer(v2, issuer);
+            return (object _, X509Certificate? certificate, X509Chain? certificateChain, SslPolicyErrors sslPolicyError) =>
+            {
+                // If we're already valid, there's nothing further to check
+                if (sslPolicyError == SslPolicyErrors.None)
+                {
+                    return true;
+                }
+                // If we're not valid due to chain errors - check against the trusted issuer
+                // Note that we're only proceeding at all here if the *only* issue is chain errors (not more flags in SslPolicyErrors)
+                return sslPolicyError == SslPolicyErrors.RemoteCertificateChainErrors
+                       && certificate is X509Certificate2 v2
+                       && CheckTrustedIssuer(v2, certificateChain, issuer);
+            };
         }
 
-        private static bool CheckTrustedIssuer(X509Certificate2 certificateToValidate, X509Certificate2 authority)
+        private static bool CheckTrustedIssuer(X509Certificate2 certificateToValidate, X509Chain? chainToValidate, X509Certificate2 authority)
         {
-            // reference: https://stackoverflow.com/questions/6497040/how-do-i-validate-that-a-certificate-was-created-by-a-particular-certification-a
-            X509Chain chain = new X509Chain();
+            // Reference:
+            // https://stackoverflow.com/questions/6497040/how-do-i-validate-that-a-certificate-was-created-by-a-particular-certification-a
+            // https://github.com/stewartadam/dotnet-x509-certificate-verification
+            using X509Chain chain = new X509Chain();
             chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
-            chain.ChainPolicy.RevocationFlag = X509RevocationFlag.ExcludeRoot;
             chain.ChainPolicy.VerificationFlags = X509VerificationFlags.AllowUnknownCertificateAuthority;
-            chain.ChainPolicy.VerificationTime = DateTime.Now;
+            chain.ChainPolicy.VerificationTime = chainToValidate?.ChainPolicy?.VerificationTime ?? DateTime.Now;
             chain.ChainPolicy.UrlRetrievalTimeout = new TimeSpan(0, 0, 0);
 
             chain.ChainPolicy.ExtraStore.Add(authority);
-            return chain.Build(certificateToValidate);
+            try
+            {
+                // This only verifies that the chain is valid, but with AllowUnknownCertificateAuthority could trust
+                // self-signed or partial chained vertificates
+                var chainIsVerified = chain.Build(certificateToValidate);
+                if (chainIsVerified)
+                {
+                    // Our method is "TrustIssuer", which means any intermediate cert we're being told to trust
+                    // is a valid thing to trust, up until it's a root CA
+                    bool found = false;
+                    byte[] authorityData = authority.RawData;
+                    foreach (var chainElement in chain.ChainElements)
+                    {
+#if NET8_0_OR_GREATER
+#error TODO: use RawDataMemory (needs testing)
+#endif
+                        using var chainCert = chainElement.Certificate;
+                        if (!found && chainCert.RawData.SequenceEqual(authorityData))
+                        {
+                            found = true;
+                        }
+                    }
+                    return found;
+                }
+            }
+            catch (CryptographicException)
+            {
+                // We specifically don't want to throw during validation here and would rather exit out gracefully
+            }
+
+            // If we didn't find the trusted issuer in the chain at all - we do not trust the result.
+            return false;
         }
 
         /// <summary>
diff --git a/tests/StackExchange.Redis.Tests/Certificates/CertValidationTests.cs b/tests/StackExchange.Redis.Tests/Certificates/CertValidationTests.cs
@@ -0,0 +1,58 @@
+﻿using System;
+using System.IO;
+using System.Net.Security;
+using System.Security.Cryptography.X509Certificates;
+using Xunit;
+using Xunit.Abstractions;
+
+namespace StackExchange.Redis.Tests;
+
+public class CertValidationTests : TestBase
+{
+    public CertValidationTests(ITestOutputHelper output) : base(output) { }
+
+    [Fact]
+    public void CheckIssuerValidity()
+    {
+        // The endpoint cert is the same here
+        var endpointCert = LoadCert(Path.Combine("Certificates", "device01.foo.com.pem"));
+
+        // Trusting CA explicitly
+        var callback = ConfigurationOptions.TrustIssuerCallback(Path.Combine("Certificates", "ca.foo.com.pem"));
+        Assert.True(callback(this, endpointCert, null, SslPolicyErrors.None));
+        Assert.True(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors));
+        Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNameMismatch));
+        Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNotAvailable));
+        Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors | SslPolicyErrors.RemoteCertificateNameMismatch));
+        Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors | SslPolicyErrors.RemoteCertificateNotAvailable));
+
+        // Trusting the remote endpoint cert directly
+        callback = ConfigurationOptions.TrustIssuerCallback(Path.Combine("Certificates", "device01.foo.com.pem"));
+        Assert.True(callback(this, endpointCert, null, SslPolicyErrors.None));
+        Assert.True(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors));
+        Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNameMismatch));
+        Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNotAvailable));
+        Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors | SslPolicyErrors.RemoteCertificateNameMismatch));
+        Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors | SslPolicyErrors.RemoteCertificateNotAvailable));
+
+        // Attempting to trust another CA (mismatch)
+        callback = ConfigurationOptions.TrustIssuerCallback(Path.Combine("Certificates", "ca2.foo.com.pem"));
+        Assert.True(callback(this, endpointCert, null, SslPolicyErrors.None));
+        Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors));
+        Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNameMismatch));
+        Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateNotAvailable));
+        Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors | SslPolicyErrors.RemoteCertificateNameMismatch));
+        Assert.False(callback(this, endpointCert, null, SslPolicyErrors.RemoteCertificateChainErrors | SslPolicyErrors.RemoteCertificateNotAvailable));
+    }
+
+    private static X509Certificate2 LoadCert(string certificatePath) => new X509Certificate2(File.ReadAllBytes(certificatePath));
+
+    [Fact]
+    public void CheckIssuerArgs()
+    {
+        Assert.ThrowsAny<Exception>(() => ConfigurationOptions.TrustIssuerCallback(""));
+
+        var opt = new ConfigurationOptions();
+        Assert.Throws<ArgumentNullException>(() => opt.TrustIssuer((X509Certificate2)null!));
+    }
+}
diff --git a/tests/StackExchange.Redis.Tests/Certificates/README.md b/tests/StackExchange.Redis.Tests/Certificates/README.md
@@ -0,0 +1,4 @@
+The certificates here are randomly generated for testing only.
+They are not valid and only used for test validation.
+
+Please do not file security issue noise - these have no impact being public at all.
diff --git a/tests/StackExchange.Redis.Tests/Certificates/ca.foo.com.pem b/tests/StackExchange.Redis.Tests/Certificates/ca.foo.com.pem
@@ -0,0 +1,20 @@
+-----BEGIN CERTIFICATE-----
+MIIDTTCCAjWgAwIBAgIUU9SR3QMGVO8yN/mr8SQJ1p/OgIAwDQYJKoZIhvcNAQEL
+BQAwNjETMBEGA1UEAwwKY2EuZm9vLmNvbTESMBAGA1UECgwJTXlEZXZpY2VzMQsw
+CQYDVQQGEwJVUzAeFw0yNDAzMDcxNDAxMzJaFw00ODEwMjcxNDAxMzJaMDYxEzAR
+BgNVBAMMCmNhLmZvby5jb20xEjAQBgNVBAoMCU15RGV2aWNlczELMAkGA1UEBhMC
+VVMwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCqk8FT5dHU335oSEuY
+RGeHOHmtxtr5Eoxe4pRHWcBKARzRYi+fPjP/aSWh75yYcmyQ5o5e2JQTZQRwSaLh
+q8lrsT7AIeZboATVxECyT3kZdIJkLgWbfyzwJQtrW+ccDx3gDRt0FKRt8Hd3foIf
+ULICgkiz3C5sihT589QWmcP4XhcRf3A1bt3rrFWJBO1jmKz0P7pijT14lkdW4sVL
+AdFhoNg/a042a7wq2i8PxrkhWpwmHkW9ErnbWG9pRjMme+GDeNfGdHslL5grzbzC
+4B4w3QP4opLUp29O9oO1DjaAuZ86JVdy3+glugMvj4f8NVCVlHxRM5Kn/3WgWIIM
+XBK7AgMBAAGjUzBRMB0GA1UdDgQWBBRmgj4urVgvTcPgJtyqyUHaFX0svjAfBgNV
+HSMEGDAWgBRmgj4urVgvTcPgJtyqyUHaFX0svjAPBgNVHRMBAf8EBTADAQH/MA0G
+CSqGSIb3DQEBCwUAA4IBAQB2DIGlKpCdluVHURfgA5zfwoOnhtOZm7lwC/zbNd5a
+wNmb6Vy29feN/+6/dv7MFTXXB5f0TkDGrGbAsKVLD5mNSfQhHC8sxwotheMYq6LS
+T1Pjv3Vxku1O7q6FQrslDWfSBxzwep2q8fDXwD4C7VgVRM2EGg/vVee2juuTCmMU
+Z1LwJrOkBczW6b3ZvUThFGOvZkuI138EuR2gqPHMQIiQcPyX1syT7yhJAyDQRYOG
+cHSRojNciYVotSTgyYguUJdU7vfRJ+MLfoZClzJgvNav8yUC+sSrb5RD5vQlvxzG
+KrJ8Hh+hpIFsmQKj5dcochKvLLd1Z748b2+FB0jtxraU
+-----END CERTIFICATE-----
diff --git a/tests/StackExchange.Redis.Tests/Certificates/ca2.foo.com.pem b/tests/StackExchange.Redis.Tests/Certificates/ca2.foo.com.pem
@@ -0,0 +1,20 @@
+-----BEGIN CERTIFICATE-----
+MIIDTzCCAjegAwIBAgIUYXv168vvB1NPg3PfoRzcNFEMaC8wDQYJKoZIhvcNAQEL
+BQAwNzEUMBIGA1UEAwwLY2EyLmZvby5jb20xEjAQBgNVBAoMCU15RGV2aWNlczEL
+MAkGA1UEBhMCVVMwHhcNMjQwMzA3MTQwMTMyWhcNNDgxMDI3MTQwMTMyWjA3MRQw
+EgYDVQQDDAtjYTIuZm9vLmNvbTESMBAGA1UECgwJTXlEZXZpY2VzMQswCQYDVQQG
+EwJVUzCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAOFDZ2sf8Ik/I3jD
+Mp4NGoo+kMY1BiRWjSKnaphfcosR2MAT/ROIVhnkzSeiQQByf34cqN/7xNkHaufr
+oVcMuuiyWERPoZjBqnfzLZ+93uxnyIU6DVDdNIcKcBQxyhHMfOigFhKTia6eWhrf
+zSaBhbkndaUsXdINBAJgSq3HDuk8bIw8MTZH0giorhIyyyqT/gjWEbzKx6Ww99qV
+MMcjFIvXEmD9AEaNilHD4TtwqZrZKZpnVBaQvWrCK3tCGBDyiFlUhAibchbt/JzV
+sK002TFfUbn41ygHmcrBVL7lSEsuT2W/PNuDOdWa6NQ2RVzYivs5jYbWV1cAvAJP
+HMWJkZ8CAwEAAaNTMFEwHQYDVR0OBBYEFA6ZeCMPgDEu+eIUoCxU/Q06ViyoMB8G
+A1UdIwQYMBaAFA6ZeCMPgDEu+eIUoCxU/Q06ViyoMA8GA1UdEwEB/wQFMAMBAf8w
+DQYJKoZIhvcNAQELBQADggEBAGOa/AD0JNPwvyDi9wbVU+Yktx3vfuVyOMbnUQSn
+nOyWd6d95rZwbeYyN908PjERQT3EMo8/O0eOpoG9I79vjbcD6LAIbxS9XdI8kK4+
+D4e/DX/R85KoWSprB+VRXGqsChY0Y+4x5x2q/IoCi6+tywhzjqIlaGDYrlc688HO
+/+4iR9L945gpg4NT1hLnCwDYcdZ5vjv4NfgXDbGPUcEheYnfz3cHE2mYxEG9KXta
+f8hSj/MNNv31BzNcj4XKcDqp4Ke3ow4lAZsPPlixOxxRaLnpsKZmEYYQcLI8KVNk
+gdAUOSPZgzRqAag0rvVfrpyvfvlu0D9xeiBLdhaJeZCq1/s=
+-----END CERTIFICATE-----
diff --git a/tests/StackExchange.Redis.Tests/Certificates/create_certificates.sh b/tests/StackExchange.Redis.Tests/Certificates/create_certificates.sh
@@ -0,0 +1,39 @@
+#!/bin/bash
+set -eu
+# Adapted from https://github.com/stewartadam/dotnet-x509-certificate-verification/blob/main/create_certificates.sh
+
+base_dir="certificates"
+
+create_ca() {
+  local CA_CN="$1"
+  local certificate_output="${base_dir}/${CA_CN}.pem"
+
+  openssl genrsa -out "${base_dir}/${CA_CN}.key.pem" 2048 # Generate private key
+  openssl req -x509 -new -nodes -key "${base_dir}/${CA_CN}.key.pem" -sha256 -days 9000 -out "${certificate_output}" -subj "/CN=${CA_CN}/O=MyDevices/C=US" # Generate root certificate
+
+  echo -e "\nCertificate for CA ${CA_CN} saved to ${certificate_output}\n\n"
+}
+
+create_leaf_cert_req() {
+  local DEVICE_CN="$1"
+
+  openssl genrsa -out "${base_dir}/${DEVICE_CN}.key.pem" 2048 # new private key
+  openssl req -new -key "${base_dir}/${DEVICE_CN}.key.pem" -out "${base_dir}/${DEVICE_CN}.csr.pem" -subj "/CN=${DEVICE_CN}/O=MyDevices/C=US" # generate signing request for the CA
+}
+
+sign_leaf_cert() {
+  local DEVICE_CN="$1"
+  local CA_CN="$2"
+  local certificate_output="${base_dir}/${DEVICE_CN}.pem"
+
+  openssl x509 -req -in "${base_dir}/${DEVICE_CN}.csr.pem" -CA ""${base_dir}/${CA_CN}.pem"" -CAkey "${base_dir}/${CA_CN}.key.pem" -set_serial 01 -out "${certificate_output}" -days 8999 -sha256 # sign the CSR
+
+  echo -e "\nCertificate for ${DEVICE_CN} saved to ${certificate_output}\n\n"
+}
+
+mkdir -p "${base_dir}"
+
+# Create one self-issued CA + signed cert
+create_ca "ca.foo.com"
+create_leaf_cert_req "device01.foo.com"
+sign_leaf_cert "device01.foo.com" "ca.foo.com"
diff --git a/tests/StackExchange.Redis.Tests/Certificates/device01.foo.com.pem b/tests/StackExchange.Redis.Tests/Certificates/device01.foo.com.pem
@@ -0,0 +1,18 @@
+-----BEGIN CERTIFICATE-----
+MIIC5jCCAc4CAQEwDQYJKoZIhvcNAQELBQAwNjETMBEGA1UEAwwKY2EuZm9vLmNv
+bTESMBAGA1UECgwJTXlEZXZpY2VzMQswCQYDVQQGEwJVUzAeFw0yNDAzMDcxNDAx
+MzJaFw00ODEwMjYxNDAxMzJaMDwxGTAXBgNVBAMMEGRldmljZTAxLmZvby5jb20x
+EjAQBgNVBAoMCU15RGV2aWNlczELMAkGA1UEBhMCVVMwggEiMA0GCSqGSIb3DQEB
+AQUAA4IBDwAwggEKAoIBAQDBb4Mv87+MFVGLIWArc0wV1GH4h7Ha+49K+JAi8rtk
+fpQACcu3OGq5TjUuxecOz5eXDwJj/vR1rvjP0DaCuIlx4SNXXqVKooWpCLb2g4Mr
+IIiFcBsiaJNmhFvd92bqHOyuXsUTjkJKaLmH6nUqVIXEA/Py+jpuSFRp9N475IGZ
+yUUdaQUx9Ur953FagLbPVeE5Vh+NEA8vnw+ZBCQRBHlRgvSJtCAR/oznXXPdHGGZ
+rMWeNjl+v1iP8fZMq4vvooW0/zTVgH8lE/HJXtpaWEVeGpnOqBsgvl12WTGL5dMU
+n81JiI3AdUyW0ieh/5yr+OFNa/HNqGLK1NvnCDPbBFpnAgMBAAEwDQYJKoZIhvcN
+AQELBQADggEBAEpIIJJ7q4/EbCJog29Os9l5unn7QJon4R5TGJQIxdqDGrhXG8QA
+HiBGl/lFhAp9tfKvQIj4aODzMgHmDpzZmH/yhjDlquJgB4JYDDjf9UhtwUUbRDp0
+rEc5VikLuTJ21hcusKALH5fgBjzplRNPH8P660FxWnv9gSWCMNaiFCCxTU91g4L3
+/qZPTl5nr1j6M/+zXbndS5qlF7GkU5Kv9xEmasZ0Z65Wao6Ufhw+nczLbiLErrxD
+zLtTfr6WYFqzXeiPGnjTueG+1cYDjngmj2fbtjPn4W67q8Z0M/ZMj9ikr881d3zP
+3dzUEaGexGqvA2MCapIQ2vCCMDF33ismQts=
+-----END CERTIFICATE-----
diff --git a/tests/StackExchange.Redis.Tests/StackExchange.Redis.Tests.csproj b/tests/StackExchange.Redis.Tests/StackExchange.Redis.Tests.csproj
@@ -13,6 +13,7 @@
     <None Update="*.json" CopyToOutputDirectory="Always" />
     <EmbeddedResource Include="*Config.json" />
     <None Update="redislabs_ca.pem" CopyToOutputDirectory="PreserveNewest" />
+    <None Update="Certificates\*.pem" CopyToOutputDirectory="PreserveNewest" />
   </ItemGroup>
 
   <ItemGroup>
