From c644620161a721f2e10729ba3d7aade9a906fd49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Cant=C3=BA?= Date: Mon, 30 Oct 2023 12:08:39 -0500 Subject: [PATCH 1/3] Do not cache unknown friendly names as null --- .../src/System/Security/Cryptography/OidLookup.cs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/OidLookup.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/OidLookup.cs index 80134ead051d4d..098ef5a8440e4a 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/OidLookup.cs +++ b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/OidLookup.cs @@ -12,8 +12,8 @@ internal static partial class OidLookup private static readonly ConcurrentDictionary s_lateBoundOidToFriendlyName = new ConcurrentDictionary(); - private static readonly ConcurrentDictionary s_lateBoundFriendlyNameToOid = - new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); + private static readonly ConcurrentDictionary s_lateBoundFriendlyNameToOid = + new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); // // Attempts to map a friendly name to an OID. Returns null if not a known name. @@ -77,19 +77,13 @@ internal static partial class OidLookup mappedOid = NativeFriendlyNameToOid(friendlyName, oidGroup, fallBackToAllGroups); - if (shouldUseCache) + if (shouldUseCache && mappedOid != null) { s_lateBoundFriendlyNameToOid.TryAdd(friendlyName, mappedOid); // Don't add the reverse here. Friendly Name => OID is a case insensitive search, // so the casing provided as input here may not be the 'correct' one. Just let // ToFriendlyName capture the response and cache it itself. - - // Also, mappedOid could be null here if the lookup failed. Allowing storing null - // means we're able to cache that a lookup failed so we don't repeat it. It's - // theoretically possible, however, the failure could have been intermittent, e.g. - // if the call was forced to follow an AD fallback path and the relevant servers - // were offline. } return mappedOid; From bbda319f5fc63e1a708d7373b4c553aed72ae7f2 Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Wed, 15 Nov 2023 18:36:08 +0000 Subject: [PATCH 2/3] [7.0] Do not use AllocHGlobal in Pkcs12Reader --- .../X509Certificates/UnixPkcs12Reader.cs | 40 ++++--------------- 1 file changed, 8 insertions(+), 32 deletions(-) diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/UnixPkcs12Reader.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/UnixPkcs12Reader.cs index a9a89f6769690e..8b4e999750f8c1 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/UnixPkcs12Reader.cs +++ b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/UnixPkcs12Reader.cs @@ -24,7 +24,7 @@ internal abstract class UnixPkcs12Reader : IDisposable private ContentInfoAsn[]? _safeContentsValues; private CertAndKey[]? _certs; private int _certCount; - private PointerMemoryManager? _tmpManager; + private Memory _tmpMemory; private bool _allowDoubleBind; protected abstract ICertificatePalCore ReadX509Der(ReadOnlyMemory data); @@ -39,19 +39,13 @@ protected void ParsePkcs12(ReadOnlySpan data) // Windows compatibility: Ignore trailing data. ReadOnlySpan encodedData = reader.PeekEncodedValue(); + byte[] dataWithoutTrailing = GC.AllocateUninitializedArray(encodedData.Length, pinned: true); + encodedData.CopyTo(dataWithoutTrailing); + _tmpMemory = MemoryMarshal.CreateFromPinnedArray(dataWithoutTrailing, 0, dataWithoutTrailing.Length); - unsafe - { - IntPtr tmpPtr = Marshal.AllocHGlobal(encodedData.Length); - Span tmpSpan = new Span((byte*)tmpPtr, encodedData.Length); - encodedData.CopyTo(tmpSpan); - _tmpManager = new PointerMemoryManager((void*)tmpPtr, encodedData.Length); - } - - ReadOnlyMemory tmpMemory = _tmpManager.Memory; - reader = new AsnValueReader(tmpMemory.Span, AsnEncodingRules.BER); + reader = new AsnValueReader(_tmpMemory.Span, AsnEncodingRules.BER); - PfxAsn.Decode(ref reader, tmpMemory, out PfxAsn pfxAsn); + PfxAsn.Decode(ref reader, _tmpMemory, out PfxAsn pfxAsn); if (pfxAsn.AuthSafe.ContentType != Oids.Pkcs7Data) { @@ -112,26 +106,8 @@ internal IEnumerable EnumerateAll() public void Dispose() { - // Generally, having a MemoryManager cleaned up in a Dispose is a bad practice. - // In this case, the UnixPkcs12Reader is only ever created in a using statement, - // never accessed by a second thread, and there isn't a manual call to Dispose - // mixed in anywhere. - if (_tmpManager != null) - { - unsafe - { - Span tmp = _tmpManager.GetSpan(); - CryptographicOperations.ZeroMemory(tmp); - - fixed (byte* ptr = tmp) - { - Marshal.FreeHGlobal((IntPtr)ptr); - } - } - - ((IDisposable)_tmpManager).Dispose(); - _tmpManager = null; - } + CryptographicOperations.ZeroMemory(_tmpMemory.Span); + _tmpMemory = Memory.Empty; ContentInfoAsn[]? rentedContents = Interlocked.Exchange(ref _safeContentsValues, null); CertAndKey[]? rentedCerts = Interlocked.Exchange(ref _certs, null); From 98fd5f3850ac46606ef7f3981a158b4c7dc2eb8f Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Wed, 22 Nov 2023 22:39:31 +0000 Subject: [PATCH 3/3] 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 +++- .../tests/ChainTests.cs | 52 +++++++++++++++ .../tests/TestData.cs | 49 ++++++++++++++ .../src/Resources/Strings.resx | 3 + .../LocalAppContextSwitches.cs | 9 +++ .../X509Certificates/X509Chain.cs | 66 ++++++++++++++----- 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 8a912d78d24a19..2766240feeb586 100644 --- a/src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs +++ b/src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs @@ -351,6 +351,20 @@ public static void GreaterThanOrEqualTo(T actual, T greaterThanOrEqualTo, str 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 8ede25072e17e9..d7892386464b27 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.X509Certificates/tests/ChainTests.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs index 66f36f898b4b4a..3fceadc45118b1 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs @@ -1252,6 +1252,58 @@ public static void BuildChainForCertificateWithMD5Signature() } } + [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.X509Certificates/tests/TestData.cs b/src/libraries/System.Security.Cryptography.X509Certificates/tests/TestData.cs index 0e0202fa119df0..454294ab6914e4 100644 --- a/src/libraries/System.Security.Cryptography.X509Certificates/tests/TestData.cs +++ b/src/libraries/System.Security.Cryptography.X509Certificates/tests/TestData.cs @@ -4159,5 +4159,54 @@ internal static DSAParameters GetDSA1024Params() "09463C6E50BCA36EB3F8BCB00D8A415D2D0DB5AE08303B301F300706052B0E03" + "021A0414A57105D833610A6D07EBFBE51E5486CD3F8BCE0D0414DB32290CC077" + "37E9D9446E37F104FA876C861C0102022710"; + + // 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(); } } diff --git a/src/libraries/System.Security.Cryptography/src/Resources/Strings.resx b/src/libraries/System.Security.Cryptography/src/Resources/Strings.resx index fa55fa3a86431c..0f798373a932c8 100644 --- a/src/libraries/System.Security.Cryptography/src/Resources/Strings.resx +++ b/src/libraries/System.Security.Cryptography/src/Resources/Strings.resx @@ -825,4 +825,7 @@ PKCS12 (PFX) without a supplied password has exceeded maximum allowed iterations. 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 0b7eb187e76b0a..342a7ce9eab43b 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 @@ -9,6 +9,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"); @@ -27,5 +29,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 50eaba6eb91b95..fdc0b02b2d6e37 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; } }