From 72e5ae975785990e904372573c93dd661279f662 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Wed, 22 Nov 2023 22:29:35 +0000 Subject: [PATCH] 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. --- .../TestUtilities/System/AssertExtensions.cs | 14 ++++ .../SignedCms/SignedCmsTests.netcoreapp.cs | 13 +++- .../src/Resources/Strings.resx | 3 + .../LocalAppContextSwitches.cs | 9 +++ .../X509Certificates/X509Chain.cs | 66 ++++++++++++++----- .../tests/X509Certificates/ChainTests.cs | 52 +++++++++++++++ .../tests/X509Certificates/TestData.cs | 49 ++++++++++++++ 7 files changed, 188 insertions(+), 18 deletions(-) diff --git a/src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs b/src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs index 230e4cb4276a..f2a9fa6333ca 100644 --- 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 Contains(string value, string substring) throw new XunitException(AddOptionalUserMessage($"Expected: {actual} to be greater than or equal to {greaterThanOrEqualTo}", userMessage)); } + /// + /// Validate that a given enum value has the expected flag set. + /// + /// The enum type. + /// The flag which should be present in . + /// The value which should contain the flag . + public static void HasFlag(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. /// 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 index e5ef61d996a8..2e1911ba11d3 100644 --- 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 index e447937b7b6c..1bd529a087a6 100644 --- a/src/libraries/System.Security.Cryptography/src/Resources/Strings.resx +++ b/src/libraries/System.Security.Cryptography/src/Resources/Strings.resx @@ -834,4 +834,7 @@ There was a problem with the PKCS12 (PFX) without a supplied password. See https://go.microsoft.com/fwlink/?linkid=2233907 for more information. + + An unknown chain building error occurred. + diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/LocalAppContextSwitches.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/LocalAppContextSwitches.cs index 0af05ab604b6..be8c3d44bf8f 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/LocalAppContextSwitches.cs +++ b/src/libraries/System.Security.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/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Chain.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Chain.cs index 50eaba6eb91b..fdc0b02b2d6e 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Chain.cs +++ b/src/libraries/System.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 index 23bef2b46c25..92cc1abb1b62 100644 --- 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(() => 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 index e3deb00b354d..cf14136eb6e0 100644 --- 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(); } }