X509Chain.Build should throw when an internal error occurs

Chain building can sometimes return nonsensical values when an internal error is encountered. This changes the behavior so that chain building will throw instead of returning nonsensical values. A compat switch is provided so apps can opt out of this behavior. Fixes CVE-2024-0057.
dotnet · Nov 22, 2023 · 72e5ae9 · 72e5ae9
1 parent 21b9057
commit 72e5ae9
Show file tree

Hide file tree

Showing 7 changed files with 188 additions and 18 deletions.
diff --git a/src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs b/src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs
@@ -397,6 +397,20 @@ public static void GreaterThanOrEqualTo<T>(T actual, T greaterThanOrEqualTo, str
                 throw new XunitException(AddOptionalUserMessage($"Expected: {actual} to be greater than or equal to {greaterThanOrEqualTo}", userMessage));
         }
 
+        /// <summary>
+        /// Validate that a given enum value has the expected flag set.
+        /// </summary>
+        /// <typeparam name="T">The enum type.</typeparam>
+        /// <param name="expected">The flag which should be present in <paramref name="actual"/>.</param>
+        /// <param name="actual">The value which should contain the flag <paramref name="expected"/>.</param>
+        public static void HasFlag<T>(T expected, T actual, string userMessage = null) where T : Enum
+        {
+            if (!actual.HasFlag(expected))
+            {
+                throw new XunitException(AddOptionalUserMessage($"Expected: Value {actual} (of enum type {typeof(T).FullName}) to have the flag {expected} set.", userMessage));
+            }
+        }
+
         // NOTE: Consider using SequenceEqual below instead, as it will give more useful information about what
         // the actual differences are, especially for large arrays/spans.
         /// <summary>

diff --git a/src/libraries/System.Security.Cryptography.Pkcs/tests/SignedCms/SignedCmsTests.netcoreapp.cs b/src/libraries/System.Security.Cryptography.Pkcs/tests/SignedCms/SignedCmsTests.netcoreapp.cs
@@ -399,7 +399,10 @@ public static void AddSigner_RSA_EphemeralKey()
                 {
                     ContentInfo content = new ContentInfo(new byte[] { 1, 2, 3 });
                     SignedCms cms = new SignedCms(content, false);
-                    CmsSigner signer = new CmsSigner(certWithEphemeralKey);
+                    CmsSigner signer = new CmsSigner(certWithEphemeralKey)
+                    {
+                        IncludeOption = X509IncludeOption.EndCertOnly
+                    };
                     cms.ComputeSignature(signer);
                 }
             }
@@ -429,7 +432,8 @@ public static void AddSigner_DSA_EphemeralKey()
                     SignedCms cms = new SignedCms(content, false);
                     CmsSigner signer = new CmsSigner(certWithEphemeralKey)
                     {
-                        DigestAlgorithm = new Oid(Oids.Sha1, Oids.Sha1)
+                        DigestAlgorithm = new Oid(Oids.Sha1, Oids.Sha1),
+                        IncludeOption = X509IncludeOption.EndCertOnly
                     };
                     cms.ComputeSignature(signer);
                 }
@@ -458,7 +462,10 @@ public static void AddSigner_ECDSA_EphemeralKey()
                 {
                     ContentInfo content = new ContentInfo(new byte[] { 1, 2, 3 });
                     SignedCms cms = new SignedCms(content, false);
-                    CmsSigner signer = new CmsSigner(certWithEphemeralKey);
+                    CmsSigner signer = new CmsSigner(certWithEphemeralKey)
+                    {
+                        IncludeOption = X509IncludeOption.EndCertOnly
+                    };
                     cms.ComputeSignature(signer);
                 }
             }

diff --git a/src/libraries/System.Security.Cryptography/src/Resources/Strings.resx b/src/libraries/System.Security.Cryptography/src/Resources/Strings.resx
@@ -834,4 +834,7 @@
   <data name="Cryptography_X509_PfxWithoutPassword_ProblemFound" xml:space="preserve">
     <value>There was a problem with the PKCS12 (PFX) without a supplied password. See https://go.microsoft.com/fwlink/?linkid=2233907 for more information.</value>
   </data>
+  <data name="Cryptography_X509_ChainBuildingFailed" xml:space="preserve">
+    <value>An unknown chain building error occurred.</value>
+  </data>
 </root>
diff --git a/...Cryptography/src/System/Security/Cryptography/X509Certificates/LocalAppContextSwitches.cs b/...Cryptography/src/System/Security/Cryptography/X509Certificates/LocalAppContextSwitches.cs
@@ -11,6 +11,8 @@ internal static partial class LocalAppContextSwitches
 
         internal static long Pkcs12UnspecifiedPasswordIterationLimit { get; } = InitializePkcs12UnspecifiedPasswordIterationLimit();
 
+        internal static bool X509ChainBuildThrowOnInternalError { get; } = InitializeX509ChainBuildThrowOnInternalError();
+
         private static long InitializePkcs12UnspecifiedPasswordIterationLimit()
         {
             object? data = AppContext.GetData("System.Security.Cryptography.Pkcs12UnspecifiedPasswordIterationLimit");
@@ -29,5 +31,12 @@ private static long InitializePkcs12UnspecifiedPasswordIterationLimit()
                 return DefaultPkcs12UnspecifiedPasswordIterationLimit;
             }
         }
+
+        private static bool InitializeX509ChainBuildThrowOnInternalError()
+        {
+            // n.b. the switch defaults to TRUE if it has not been explicitly set.
+            return AppContext.TryGetSwitch("System.Security.Cryptography.ThrowOnX509ChainBuildInternalError", out bool isEnabled)
+                ? isEnabled : true;
+        }
     }
 }
diff --git a/...stem.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Chain.cs b/...stem.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Chain.cs
@@ -137,26 +137,62 @@ internal bool Build(X509Certificate2 certificate, bool throwOnException)
                     chainPolicy.UrlRetrievalTimeout,
                     chainPolicy.DisableCertificateDownloads);
 
-                if (_pal == null)
-                    return false;
-
-                _chainElements = new X509ChainElementCollection(_pal.ChainElements!);
-
-                Exception? verificationException;
-                bool? verified = _pal.Verify(chainPolicy.VerificationFlags, out verificationException);
-                if (!verified.HasValue)
+                bool success = false;
+                if (_pal is not null)
                 {
-                    if (throwOnException)
-                    {
-                        throw verificationException!;
-                    }
-                    else
+                    _chainElements = new X509ChainElementCollection(_pal.ChainElements!);
+
+                    Exception? verificationException;
+                    bool? verified = _pal.Verify(chainPolicy.VerificationFlags, out verificationException);
+                    if (!verified.HasValue)
                     {
-                        verified = false;
+                        if (throwOnException)
+                        {
+                            throw verificationException!;
+                        }
+                        else
+                        {
+                            verified = false;
+                        }
                     }
+
+                    success = verified.Value;
+                }
+
+                // There are two reasons success might be false here.
+                //
+                // The most common reason is that we built the chain but the chain appears to run
+                // afoul of policy. This is represented by BuildChain returning a non-null object
+                // and storing potential policy violations in the chain structure. The public Build
+                // method returns false to the caller, and the caller can inspect the ChainStatus
+                // and ChainElements properties and evaluate the failure reason against app-level
+                // policies. If the caller does not care about these policy violations, they can
+                // choose to ignore them and to treat chain building as successful.
+                //
+                // The other type of failure is that BuildChain simply can't build the chain at all.
+                // Perhaps something within the certificate is not valid or is unsupported, or perhaps
+                // there's an internal failure within the OS layer we're invoking, etc. Whatever the
+                // reason, we're not meaningfully able to initialize the ChainStatus property, which
+                // means callers may observe a non-empty list of policy violations. Depending on the
+                // caller's logic, they might incorrectly interpret this as there being no policy
+                // violations at all, which means they might treat this condition as success.
+                //
+                // To avoid callers misinterpeting this latter condition as success, we'll throw an
+                // exception, which matches general .NET API behavior when a method cannot complete
+                // its objective. A compat switch is provided to normalize this back to a 'false'
+                // return value for callers who cannot handle an exception here. If throwOnException
+                // is false, it means the caller explicitly wants to suppress exceptions and normalize
+                // them to a false return value.
+
+                if (!success
+                    && throwOnException
+                    && _pal?.ChainStatus is not { Length: > 0 }
+                    && LocalAppContextSwitches.X509ChainBuildThrowOnInternalError)
+                {
+                    throw new CryptographicException(SR.Cryptography_X509_ChainBuildingFailed);
                 }
 
-                return verified.Value;
+                return success;
             }
         }
 

diff --git a/src/libraries/System.Security.Cryptography/tests/X509Certificates/ChainTests.cs b/src/libraries/System.Security.Cryptography/tests/X509Certificates/ChainTests.cs
@@ -1269,6 +1269,58 @@ public static void BuildChainForSelfSignedSha3Certificate()
             }
         }
 
+        [Fact]
+        public static void BuildChainForSelfSignedCertificate_WithSha256RsaSignature()
+        {
+            using (ChainHolder chainHolder = new ChainHolder())
+            using (X509Certificate2 cert = new X509Certificate2(TestData.SelfSignedCertSha256RsaBytes))
+            {
+                X509Chain chain = chainHolder.Chain;
+                chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
+                chain.ChainPolicy.VerificationTime = cert.NotBefore.AddHours(2);
+
+                // No custom root of trust store means that this self-signed cert will at
+                // minimum be marked UntrustedRoot.
+
+                Assert.False(chain.Build(cert));
+                AssertExtensions.HasFlag(X509ChainStatusFlags.UntrustedRoot, chain.AllStatusFlags());
+            }
+        }
+
+        [Fact]
+        public static void BuildChainForSelfSignedCertificate_WithUnknownOidSignature()
+        {
+            using (ChainHolder chainHolder = new ChainHolder())
+            using (X509Certificate2 cert = new X509Certificate2(TestData.SelfSignedCertDummyOidBytes))
+            {
+                X509Chain chain = chainHolder.Chain;
+                chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
+                chain.ChainPolicy.VerificationTime = cert.NotBefore.AddHours(2);
+
+                // This tests a self-signed cert whose signature block contains a garbage signing alg OID.
+                // Some platforms return NotSignatureValid to indicate that they cannot understand the
+                // signature block. Other platforms return PartialChain to indicate that they think the
+                // bad signature block might correspond to some unknown, untrusted signer. Yet other
+                // platforms simply fail the operation; e.g., Windows's CertGetCertificateChain API returns
+                // NTE_BAD_ALGID, which we bubble up as CryptographicException.
+
+                if (PlatformDetection.UsesAppleCrypto)
+                {
+                    Assert.False(chain.Build(cert));
+                    AssertExtensions.HasFlag(X509ChainStatusFlags.PartialChain, chain.AllStatusFlags());
+                }
+                else if (PlatformDetection.IsOpenSslSupported)
+                {
+                    Assert.False(chain.Build(cert));
+                    AssertExtensions.HasFlag(X509ChainStatusFlags.NotSignatureValid, chain.AllStatusFlags());
+                }
+                else
+                {
+                    Assert.ThrowsAny<CryptographicException>(() => chain.Build(cert));
+                }
+            }
+        }
+
         internal static X509ChainStatusFlags AllStatusFlags(this X509Chain chain)
         {
             return chain.ChainStatus.Aggregate(

diff --git a/src/libraries/System.Security.Cryptography/tests/X509Certificates/TestData.cs b/src/libraries/System.Security.Cryptography/tests/X509Certificates/TestData.cs
@@ -4224,5 +4224,54 @@ internal static DSAParameters GetDSA1024Params()
             "09463C6E50BCA36EB3F8BCB00D8A415D2D0DB5AE08303B301F300706052B0E03" +
             "021A0414A57105D833610A6D07EBFBE51E5486CD3F8BCE0D0414DB32290CC077" +
             "37E9D9446E37F104FA876C861C0102022710").HexToByteArray();
+
+        // Used for chain building tests (CN=Test chain building)
+        internal static readonly byte[] SelfSignedCertSha256RsaBytes = (
+            "308202BD308201A5A003020102020900EF79C10DFD657168300D06092A864886F70D0101" +
+            "0B0500301E311C301A060355040313135465737420636861696E206275696C64696E6730" +
+            "1E170D3231313031333231353735335A170D3232313031333231353735335A301E311C30" +
+            "1A060355040313135465737420636861696E206275696C64696E6730820122300D06092A" +
+            "864886F70D01010105000382010F003082010A0282010100E3B5BBF862313DEAA9172788" +
+            "278B26A3EAB61B9B0326F5CEA91B1A6C6DFD156836A2363BFAC5B0F4A78F4CFF5A11F35A" +
+            "831C6D7935D1DFD13DD81DA29AA0645CBA9F4D20BF991C625E6D61CF396C15914DEE41F6" +
+            "1190E97B52BFF7AE52B79FD0E2EEE3319EC23C30D27A52A2E8A963557B12BEC0664ADEF9" +
+            "3C520B587EC5DABFBC70980DB7473414B4B6BF982EA9AA0969F2A76AA085464AE78DFB2B" +
+            "F04BDE7192874679193119C2AABEC04D360F61925921660BF09A0489B7C53464F5FC35B8" +
+            "612F5B993D544475C20AC46CD350A34551FEA0ACBD138D8B72F79052BF0EB3BD794A426C" +
+            "0117CB77B4F311FFD1C628F8E438E5474509AD51FA035558771546310203010001300D06" +
+            "092A864886F70D01010B050003820101000A12CE2FC3DC854113D179725E9D9ADD013A42" +
+            "D66340CEA7A465D54EC357AD8FED1828862D8B5C32EB3D21FC8B26A7CFA9D9FB36F593CC" +
+            "6AD30C25C96E8100C3F07B1B51430245EE995864749C53B409260B4040705654710C236F" +
+            "D9B7DE3F3BE5E6E5047712C5E506419106A57C5290BB206A97F6A3FCC4B4C83E25C3FC6D" +
+            "2BAB03B941374086265EE08A90A8C72A63A4053044B9FA3ABD5ED5785CFDDB15A6A327BD" +
+            "C0CC2B115B9D33BD6E528E35670E5A6A8D9CF52199F8D693315C60D9ADAD54EF7FDCED36" +
+            "0C8C79E84D42AB5CB6355A70951B1ABF1F2B3FB8BEB7E3A8D6BA2293C0DB8C86B0BB060F" +
+            "0D6DB9939E88B998662A27F092634BBF21F58EEAAA").HexToByteArray();
+
+        // This is nearly identical to the cert in Pkcs7SelfSignedCertSha256RsaBytes,
+        // but we've replaced the OID (1.2.840.113549.1.1.11 sha256RSA) with a dummy OID
+        // 1.3.9999.1234.5678.1234. The cert should load properly into an X509Certificate2
+        // object but will cause chain building to fail.
+        internal static readonly byte[] SelfSignedCertDummyOidBytes = (
+            "308202BD308201A5A003020102020900EF79C10DFD657168300D06092A864886F70D0101" +
+            "0B0500301E311C301A060355040313135465737420636861696E206275696C64696E6730" +
+            "1E170D3231313031333231353735335A170D3232313031333231353735335A301E311C30" +
+            "1A060355040313135465737420636861696E206275696C64696E6730820122300D06092A" +
+            "864886F70D01010105000382010F003082010A0282010100E3B5BBF862313DEAA9172788" +
+            "278B26A3EAB61B9B0326F5CEA91B1A6C6DFD156836A2363BFAC5B0F4A78F4CFF5A11F35A" +
+            "831C6D7935D1DFD13DD81DA29AA0645CBA9F4D20BF991C625E6D61CF396C15914DEE41F6" +
+            "1190E97B52BFF7AE52B79FD0E2EEE3319EC23C30D27A52A2E8A963557B12BEC0664ADEF9" +
+            "3C520B587EC5DABFBC70980DB7473414B4B6BF982EA9AA0969F2A76AA085464AE78DFB2B" +
+            "F04BDE7192874679193119C2AABEC04D360F61925921660BF09A0489B7C53464F5FC35B8" +
+            "612F5B993D544475C20AC46CD350A34551FEA0ACBD138D8B72F79052BF0EB3BD794A426C" +
+            "0117CB77B4F311FFD1C628F8E438E5474509AD51FA035558771546310203010001300D06" +
+            "092BCE0F8952AC2E8952050003820101000A12CE2FC3DC854113D179725E9D9ADD013A42" +
+            "D66340CEA7A465D54EC357AD8FED1828862D8B5C32EB3D21FC8B26A7CFA9D9FB36F593CC" +
+            "6AD30C25C96E8100C3F07B1B51430245EE995864749C53B409260B4040705654710C236F" +
+            "D9B7DE3F3BE5E6E5047712C5E506419106A57C5290BB206A97F6A3FCC4B4C83E25C3FC6D" +
+            "2BAB03B941374086265EE08A90A8C72A63A4053044B9FA3ABD5ED5785CFDDB15A6A327BD" +
+            "C0CC2B115B9D33BD6E528E35670E5A6A8D9CF52199F8D693315C60D9ADAD54EF7FDCED36" +
+            "0C8C79E84D42AB5CB6355A70951B1ABF1F2B3FB8BEB7E3A8D6BA2293C0DB8C86B0BB060F" +
+            "0D6DB9939E88B998662A27F092634BBF21F58EEAAA").HexToByteArray();
     }
 }