From 9ada73ae85fc166820c4812c5eefa433145b09d2 Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Tue, 2 Jul 2024 20:57:15 -0700 Subject: [PATCH 1/3] Make CertLoader reject duplicate attributes The new Pkcs12 loader will now reject duplicate attributes, either as multiple attribute sets, or a set with multiple values. The LoaderLimits type gains an option to disable this filter, but as of now it is not being made into public API (though the tests show how to get at it anyways, by the cloning behavior on DangerousNoLimits). This change also fixes the over-filtering of the MachineKey attribute, and adds tests for how that behaves under DefaultKeySet. --- .../src/System/Security/Cryptography/Oids.cs | 1 + .../X509Certificates/Pkcs12LoaderLimits.cs | 21 +++ .../X509CertificateLoader.Pkcs12.cs | 76 +++++++-- .../Cryptography/X509Certificates/TestData.cs | 80 ++++++++++ ...9CertificateLoaderPkcs12CollectionTests.cs | 44 ++++++ ...cateLoaderPkcs12Tests.WindowsAttributes.cs | 101 +++++++++--- .../X509CertificateLoaderPkcs12Tests.cs | 149 ++++++++++++++++++ .../Microsoft.Bcl.Cryptography.Tests.csproj | 1 + .../tests/X509Certificates/PfxTests.cs | 32 ++++ 9 files changed, 474 insertions(+), 31 deletions(-) diff --git a/src/libraries/Common/src/System/Security/Cryptography/Oids.cs b/src/libraries/Common/src/System/Security/Cryptography/Oids.cs index 033c9c9db0d9f8..740d464b96e060 100644 --- a/src/libraries/Common/src/System/Security/Cryptography/Oids.cs +++ b/src/libraries/Common/src/System/Security/Cryptography/Oids.cs @@ -52,6 +52,7 @@ internal static partial class Oids internal const string CertificateAuthorityIssuers = "1.3.6.1.5.5.7.48.2"; internal const string Pkcs9ExtensionRequest = "1.2.840.113549.1.9.14"; internal const string MsPkcs12KeyProviderName = "1.3.6.1.4.1.311.17.1"; + internal const string MsPkcs12MachineKeySet = "1.3.6.1.4.1.311.17.2"; // Key wrap algorithms internal const string CmsRc2Wrap = "1.2.840.113549.1.9.16.3.7"; diff --git a/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/Pkcs12LoaderLimits.cs b/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/Pkcs12LoaderLimits.cs index 3200771cee0adb..f19859c54b0352 100644 --- a/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/Pkcs12LoaderLimits.cs +++ b/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/Pkcs12LoaderLimits.cs @@ -22,6 +22,7 @@ public sealed class Pkcs12LoaderLimits private bool _preserveUnknownAttributes; private bool _ignorePrivateKeys; private bool _ignoreEncryptedAuthSafes; + private bool _allowDuplicateAttributes; /// /// Gets a shared reference to the default loader limits. @@ -72,6 +73,7 @@ public sealed class Pkcs12LoaderLimits PreserveKeyName = true, PreserveCertificateAlias = true, PreserveUnknownAttributes = true, + AllowDuplicateAttributes = true, }); /// @@ -117,6 +119,7 @@ public Pkcs12LoaderLimits(Pkcs12LoaderLimits copyFrom) _preserveUnknownAttributes = copyFrom._preserveUnknownAttributes; _ignorePrivateKeys = copyFrom._ignorePrivateKeys; _ignoreEncryptedAuthSafes = copyFrom._ignoreEncryptedAuthSafes; + _allowDuplicateAttributes = copyFrom._allowDuplicateAttributes; } /// @@ -366,6 +369,24 @@ public bool IgnoreEncryptedAuthSafes } } + /// + /// Gets or sets a value indicating whether duplicate attributes are permitted. + /// + /// + /// to permit duplicate attributes; + /// to fail loading when duplicate attributes are found. + /// The default is . + /// + internal bool AllowDuplicateAttributes + { + get => _allowDuplicateAttributes; + set + { + CheckReadOnly(); + _allowDuplicateAttributes = value; + } + } + private static void CheckNonNegative( int? value, [CallerArgumentExpression(nameof(value))] string? paramName = null) diff --git a/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.Pkcs12.cs b/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.Pkcs12.cs index 196af51bc4d91f..9b85a907e47e5b 100644 --- a/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.Pkcs12.cs +++ b/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.Pkcs12.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Buffers; +using System.Collections.Generic; using System.Diagnostics; using System.Formats.Asn1; using System.Security.Cryptography.Asn1; @@ -21,6 +22,9 @@ public static partial class X509CertificateLoader private const int NTE_FAIL = unchecked((int)0x80090020); #endif + [ThreadStatic] + private static HashSet? t_attributeDuplicateCheck; + static partial void LoadPkcs12NoLimits( ReadOnlyMemory data, ReadOnlySpan password, @@ -274,6 +278,11 @@ private static void ProcessSafeContents( if (bag.BagId == Oids.Pkcs12CertBag) { + if (bag.BagAttributes is not null && !loaderLimits.AllowDuplicateAttributes) + { + RejectDuplicateAttributes(bag.BagAttributes); + } + CertBagAsn certBag = CertBagAsn.Decode(bag.BagValue, AsnEncodingRules.BER); if (certBag.CertId == Oids.Pkcs12X509CertBagType) @@ -302,6 +311,11 @@ private static void ProcessSafeContents( } else if (bag.BagId is Oids.Pkcs12KeyBag or Oids.Pkcs12ShroudedKeyBag) { + if (bag.BagAttributes is not null && !loaderLimits.AllowDuplicateAttributes) + { + RejectDuplicateAttributes(bag.BagAttributes); + } + if (loaderLimits.IgnorePrivateKeys) { continue; @@ -344,6 +358,9 @@ private static void ProcessSafeContents( attrType switch { Oids.LocalKeyId => true, + // MsPkcs12MachineKeySet can be forced off with the UserKeySet flag, or on with MachineKeySet, + // so always preserve it. + Oids.MsPkcs12MachineKeySet => true, Oids.Pkcs9FriendlyName => limits.PreserveKeyName, Oids.MsPkcs12KeyProviderName => limits.PreserveStorageProvider, _ => limits.PreserveUnknownAttributes, @@ -355,6 +372,42 @@ private static void ProcessSafeContents( } } + private static void RejectDuplicateAttributes(AttributeAsn[] bagAttributes) + { + // If there's only one attribute set there's no reason to instantiate the HashSet. + if (bagAttributes.Length == 1) + { + // Use >1 instead of=1 to account for MsPkcs12MachineKeySet, which is a named set with no values. + // Though it doesn't really make sense as the only attribute. + if (bagAttributes[0].AttrValues.Length > 1) + { + throw new Pkcs12LoadLimitExceededException(nameof(Pkcs12LoaderLimits.AllowDuplicateAttributes)); + } + + return; + } + + HashSet set = (t_attributeDuplicateCheck ??= new HashSet()); + set.Clear(); + + try + { + foreach (AttributeAsn attrSet in bagAttributes) + { + // Use >1 instead of=1 to account for MsPkcs12MachineKeySet, which is a named set with no values. + // An empty attribute set can't be followed by the same empty set, or a non-empty set. + if (!set.Add(attrSet.AttrType) || attrSet.AttrValues.Length > 1) + { + throw new Pkcs12LoadLimitExceededException(nameof(Pkcs12LoaderLimits.AllowDuplicateAttributes)); + } + } + } + finally + { + set.Clear(); + } + } + private static void FilterAttributes( Pkcs12LoaderLimits loaderLimits, ref SafeBagAsn bag, @@ -362,10 +415,13 @@ private static void FilterAttributes( { if (bag.BagAttributes is not null) { - // Should this dedup/fail-on-dup? int attrIdx = -1; - for (int i = bag.BagAttributes.Length - 1; i > attrIdx; i--) + // Filter the attributes, per the loader limits. + // Because duplicates might be permitted by the options, this filter + // needs to be order preserving. + + for (int i = 0; i < bag.BagAttributes.Length; i++) { string attrType = bag.BagAttributes[i].AttrType; @@ -373,30 +429,28 @@ private static void FilterAttributes( { attrIdx++; - if (i > attrIdx) + if (attrIdx != i) { AttributeAsn attr = bag.BagAttributes[i]; +#if DEBUG bag.BagAttributes[i] = bag.BagAttributes[attrIdx]; +#endif bag.BagAttributes[attrIdx] = attr; - - // After swapping, back up one position to check if the attribute - // swapped into this position should also be preserved. - i++; } } } - attrIdx++; + int attrLen = attrIdx + 1; - if (attrIdx < bag.BagAttributes.Length) + if (attrLen < bag.BagAttributes.Length) { - if (attrIdx == 0) + if (attrLen == 0) { bag.BagAttributes = null; } else { - Array.Resize(ref bag.BagAttributes, attrIdx); + Array.Resize(ref bag.BagAttributes, attrLen); } } } diff --git a/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/TestData.cs b/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/TestData.cs index 458f9b04f4cb46..4c1025a7c1c028 100644 --- a/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/TestData.cs +++ b/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/TestData.cs @@ -4550,5 +4550,85 @@ internal static DSAParameters GetDSA1024Params() "928343622B80009D8C5B0440E74D33A5F7DA48801A4EF4FD65D0F442F26C845F" + "A418D3E0D78FD0285A92A74B433661F516C6955EC40FE46DAB813B6AE940C2DE" + "FE8F8F5E32E6B491C999D598020127").HexToByteArray(); + + // This is a PFX that has a lot of duplicated attributes: + // Integrity: None + // SC[0] Unencrypted + // Key (3DES, "Placeholder") + // MachineKey (empty) + // MachineKey (empty) + // LocalKeyId { 1 } + // LocalKeyId { 1, 2 } + // FriendlyName { keyname000, keyname001 } + // ProviderName Microsoft Enhanced RSA and AES Cryptographic Provider + // ProviderName Microsoft Software Key Storage Provider + // SC[1] Unencrypted + // Cert, CN=Cerificate 1 + // LocalKeyId { 1, 4 } + // FriendlyName { (CAPI CSP), (CNG KSP) } + // + // Note that this test cannot be built by Pkcs12Builder, because that type + // always unifies attribute sets. + internal static readonly byte[] DuplicateAttributesPfx = ( + "308207760201033082076F06092A864886F70D010701A08207600482075C3082" + + "07583082043006092A864886F70D010701A08204210482041D30820419308204" + + "15060B2A864886F70D010C0A0102A08202A6308202A2301C060A2A864886F70D" + + "010C0103300E0408F7A34F4CC26F79890202100004820280BD3E4386BE9699C0" + + "F53C27D4971B17E984BE885F3E1D3B1B75B666EF3F53BED590A70AA071C05057" + + "7CF587B92AF7F84F0D6E79475CA0D46C5F86A6D548ADE9538A955B7033154F2D" + + "A356B1DD930576A0A1D6CC87FF9055BB00CECFF5E61040B709FC19C29046A4AA" + + "BD4B8D1CA8F09B119ED6A8FBDB985E3F22E531D5DB13E292278D73A6C4E05498" + + "79642858CAA162BB21E9A17B4B14341B388665E0F5B90EB1EFB0C4B211B9B49E" + + "F8C7F435F9B2D32A319D43B9133039844E7ED8C0BA663E8681C8EFDF707356DC" + + "A1B78551839300323F046A497DD4C468232993946343764F151AB3EFFFC7FD27" + + "9739CBE00337399AF2341E7842DA1CCDD98B12A7831A4DE9827F8F1CCB5A0F4E" + + "ECB412D208983CA5D801D720B3F1E118C20BB8A1853770435177253EF59A62A2" + + "43E53ABC531F310E245CC1A0626E5456ADC08924F15E1408B2FD30BFDD4A4F32" + + "01B1983DE0F7F42E7EEF2E8EA6D9B0EAB98174A4B4D0410FD04167670FDFC20A" + + "1EFC58AB2A41ABD3EC42D3071F31EA5D0A6B93EC070E1D543F0FC7BB8B88361A" + + "D904E81ADD0C3B0261F1406EACF956F19055FC1C2832F25209DFBD35C8EDD8B4" + + "091B626E8C07D58F8537C519C90E23E94E8E61DCF3862C1DFB63010D2D909037" + + "6CFB21042041B550FE62122E3473B88E479B42153FB17077C4BC1318715BAB99" + + "597226F0C24524FB844CEAA4EC8DD164321DDFB74509FBC4844C205FFC27B067" + + "C9E4A78B8B12F4643F3A4C754E84F244F84D7A075F290C10A3B544264E317BFA" + + "41647EC4F50D6B1B2A691B5F0575B9492484019E88355CADADBC0A30FAEED71E" + + "4392E37BE497900408A85C711BF68B27A84433B0F546DF2FC2CA3FD22C4367BA" + + "BC074313B982E5012B26863FA98148E5DBF43D26423369C13182015A300D0609" + + "2B06010401823711023100300D06092B06010401823711023100301006092A86" + + "4886F70D0109153103040101301306092A864886F70D01091531060401010401" + + "02303906092A864886F70D010914312C1E14006B00650079006E0061006D0065" + + "0030003000301E14006B00650079006E0061006D006500300030003130790609" + + "2B0601040182371101316C1E6A004D006900630072006F0073006F0066007400" + + "200045006E00680061006E006300650064002000520053004100200061006E00" + + "640020004100450053002000430072007900700074006F006700720061007000" + + "6800690063002000500072006F00760069006400650072305D06092B06010401" + + "8237110131501E4E004D006900630072006F0073006F0066007400200053006F" + + "0066007400770061007200650020004B00650079002000530074006F00720061" + + "00670065002000500072006F007600690064006500723082032006092A864886" + + "F70D010701A08203110482030D3082030930820305060B2A864886F70D010C0A" + + "0103A082020F3082020B060A2A864886F70D01091601A08201FB048201F73082" + + "01F33082015CA00302010202080828E93144879296300D06092A864886F70D01" + + "010B0500303C31223020060355040B13194475706C6963617465204174747269" + + "62757465732054657374311630140603550403130D4365727469666963617465" + + "2031301E170D3234303730323231303531395A170D3234303730323231313031" + + "395A303C31223020060355040B13194475706C69636174652041747472696275" + + "7465732054657374311630140603550403130D43657274696669636174652031" + + "30819F300D06092A864886F70D010101050003818D0030818902818100E6EB18" + + "5FCE3E6A5A6F73D260B408FA9DD06DC99D546AE9A1F1F8792FDF818E8A2759FF" + + "D0FCD8A88301117CA992ACEC9AA4E429655AB7A73EE20994155FE97572471D06" + + "2C06295FF4EE218090DF64AAF787BAD7F511DF329F2F685FFC3B819F95F17811" + + "E9C158D97D832208C27214C958D844432481981B03FE8C9E0E8C1A5605020301" + + "0001300D06092A864886F70D01010B0500038181009243F8BCFB5B21BC5BFB45" + + "83DF87F0D1FE7680C62D1F35698F1A25A0D0517F17B977F039985457A85058DC" + + "7375D1ED845EB30B20FF7FC3E8970871F03AEDE3658FF964B1EF1BFD1EEB7CF5" + + "E4A219F1B00DDED1F00BE42F132A2409D0FE78BA131634F5059B06E1B905AE18" + + "6897E1E10716282E0BE25D460AE93483E9BC0329E13181E2301306092A864886" + + "F70D01091531060401010401043081CA06092A864886F70D0109143181BC1E6A" + + "004D006900630072006F0073006F0066007400200045006E00680061006E0063" + + "00650064002000520053004100200061006E0064002000410045005300200043" + + "0072007900700074006F0067007200610070006800690063002000500072006F" + + "007600690064006500721E4E004D006900630072006F0073006F006600740020" + + "0053006F0066007400770061007200650020004B00650079002000530074006F" + + "0072006100670065002000500072006F00760069006400650072").HexToByteArray(); } } diff --git a/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/X509CertificateLoaderPkcs12CollectionTests.cs b/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/X509CertificateLoaderPkcs12CollectionTests.cs index 84d57f6e7e74e7..13fa4bb81971fc 100644 --- a/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/X509CertificateLoaderPkcs12CollectionTests.cs +++ b/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/X509CertificateLoaderPkcs12CollectionTests.cs @@ -742,6 +742,50 @@ public void LoadPfx_VerifyIgnoreEncryptedSafes_EmptyIfIgnored(bool ignoreEncrypt } } + [Theory] + [InlineData(false)] + [InlineData(true)] + public void LoadWithDuplicateAttributes(bool allowDuplicates) + { + Pkcs12LoaderLimits limits = Pkcs12LoaderLimits.Defaults; + + if (allowDuplicates) + { + limits = Pkcs12LoaderLimits.DangerousNoLimits; + } + + // remove the edit lock + limits = new Pkcs12LoaderLimits(limits) + { + PreserveCertificateAlias = false, + PreserveKeyName = false, + PreserveStorageProvider = false, + PreserveUnknownAttributes = false, + }; + + Func func = + () => LoadPfxNoFile(TestData.DuplicateAttributesPfx, TestData.PlaceholderPw, loaderLimits: limits); + + if (allowDuplicates) + { + X509Certificate2Collection coll = func(); + + using (new CollectionDisposer(coll)) + { + Assert.Equal(1, coll.Count); + X509Certificate2 cert = coll[0]; + + Assert.Equal("Certificate 1", cert.GetNameInfo(X509NameType.SimpleName, false)); + Assert.True(cert.HasPrivateKey, "cert.HasPrivateKey"); + } + } + else + { + Pkcs12LoadLimitExceededException ex = Assert.Throws(() => func()); + Assert.Contains("AllowDuplicateAttributes", ex.Message); + } + } + private sealed class CollectionDisposer : IDisposable { private readonly X509Certificate2Collection _coll; diff --git a/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/X509CertificateLoaderPkcs12Tests.WindowsAttributes.cs b/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/X509CertificateLoaderPkcs12Tests.WindowsAttributes.cs index 444094c15b0ca8..3e07c4e25f5632 100644 --- a/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/X509CertificateLoaderPkcs12Tests.WindowsAttributes.cs +++ b/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/X509CertificateLoaderPkcs12Tests.WindowsAttributes.cs @@ -9,9 +9,11 @@ namespace System.Security.Cryptography.X509Certificates.Tests public abstract partial class X509CertificateLoaderPkcs12Tests { [Theory] - [InlineData(true)] - [InlineData(false)] - public void VerifyPreserveKeyName(bool preserveName) + [InlineData(true, true)] + [InlineData(true, false)] + [InlineData(false, true)] + [InlineData(false, false)] + public void VerifyPreserveKeyName(bool preserveName, bool machineKey) { Pkcs12LoaderLimits loaderLimits = new Pkcs12LoaderLimits { @@ -19,7 +21,7 @@ public void VerifyPreserveKeyName(bool preserveName) }; string keyName = Guid.NewGuid().ToString("D"); - byte[] pfx = MakeAttributeTest(keyName: keyName, friendlyName: "Non-preserved"); + byte[] pfx = MakeAttributeTest(keyName: keyName, friendlyName: "Non-preserved", machineKey: machineKey); X509Certificate2 cert = LoadPfxNoFile( pfx, @@ -29,9 +31,8 @@ public void VerifyPreserveKeyName(bool preserveName) using (cert) { using (RSA key = cert.GetRSAPrivateKey()) + using (CngKey cngKey = Assert.IsType(key).Key) { - CngKey cngKey = Assert.IsType(key).Key; - if (preserveName) { Assert.Equal(keyName, cngKey.KeyName); @@ -40,6 +41,9 @@ public void VerifyPreserveKeyName(bool preserveName) { Assert.NotEqual(keyName, cngKey.KeyName); } + + // MachineKey is preserved irrespective of PreserveKeyName + Assert.Equal(machineKey, cngKey.IsMachineKey); } // Alias was not preserved @@ -48,9 +52,11 @@ public void VerifyPreserveKeyName(bool preserveName) } [Theory] - [InlineData(true)] - [InlineData(false)] - public void VerifyPreserveAlias(bool preserveAlias) + [InlineData(true, true)] + [InlineData(true, false)] + [InlineData(false, true)] + [InlineData(false, false)] + public void VerifyPreserveAlias(bool preserveAlias, bool machineKey) { Pkcs12LoaderLimits loaderLimits = new Pkcs12LoaderLimits { @@ -59,7 +65,7 @@ public void VerifyPreserveAlias(bool preserveAlias) string keyName = Guid.NewGuid().ToString("D"); string alias = Guid.NewGuid().ToString("D"); - byte[] pfx = MakeAttributeTest(keyName: keyName, friendlyName: alias); + byte[] pfx = MakeAttributeTest(keyName: keyName, friendlyName: alias, machineKey: machineKey); X509Certificate2 cert = LoadPfxNoFile( pfx, @@ -78,21 +84,27 @@ public void VerifyPreserveAlias(bool preserveAlias) } using (RSA key = cert.GetRSAPrivateKey()) + using (CngKey cngKey = Assert.IsType(key).Key) { - CngKey cngKey = Assert.IsType(key).Key; - // Key name was not preserved Assert.NotEqual(keyName, cngKey.KeyName); + + // MachineKey is preserved irrespective of PreserveCertificateAlias + Assert.Equal(machineKey, cngKey.IsMachineKey); } } } [Theory] - [InlineData(true, true)] - [InlineData(true, false)] - [InlineData(false, true)] - [InlineData(false, false)] - public void VerifyPreservePreserveProvider(bool preserveProvider, bool preserveName) + [InlineData(true, true, true)] + [InlineData(true, true, false)] + [InlineData(true, false, true)] + [InlineData(true, false, false)] + [InlineData(false, true, true)] + [InlineData(false, true, false)] + [InlineData(false, false, true)] + [InlineData(false, false, false)] + public void VerifyPreserveProvider(bool preserveProvider, bool preserveName, bool machineKey) { // This test forces a key creation with CAPI, and verifies that // PreserveStorageProvider keeps the key in CAPI. Additionally, @@ -105,7 +117,7 @@ public void VerifyPreservePreserveProvider(bool preserveProvider, bool preserveN string keyName = Guid.NewGuid().ToString("D"); string alias = Guid.NewGuid().ToString("D"); - byte[] pfx = MakeAttributeTest(keyName: keyName, friendlyName: alias, useCapi: true); + byte[] pfx = MakeAttributeTest(keyName: keyName, friendlyName: alias, useCapi: true, machineKey: machineKey); X509Certificate2 cert = LoadPfxNoFile( pfx, @@ -115,9 +127,8 @@ public void VerifyPreservePreserveProvider(bool preserveProvider, bool preserveN using (cert) { using (RSA key = cert.GetRSAPrivateKey()) + using (CngKey cngKey = Assert.IsType(key).Key) { - CngKey cngKey = Assert.IsType(key).Key; - if (preserveName) { Assert.Equal(keyName, cngKey.KeyName); @@ -137,6 +148,9 @@ public void VerifyPreservePreserveProvider(bool preserveProvider, bool preserveN { Assert.NotEqual(CapiProvider, cngKey.Provider.Provider); } + + // MachineKey is preserved irrespective of PreserveKeyName or PreserveStorageProvider + Assert.Equal(machineKey, cngKey.IsMachineKey); } // Alias is not preserved @@ -144,10 +158,55 @@ public void VerifyPreservePreserveProvider(bool preserveProvider, bool preserveN } } + [Theory] + [InlineData(false)] + [InlineData(true)] + public void VerifyNamesWithDuplicateAttributes(bool noLimits) + { + // This test mainly shows that when duplicate attributes are present contents + // processed by our filter and processed directly by PFXImportCertStore come up + // with the same answer. + + Pkcs12LoaderLimits limits = Pkcs12LoaderLimits.DangerousNoLimits; + + // DangerousNoLimits is tested by reference, by cloning the object we + // use a functional equivalent using the work-limiting and attribute-filtering + // loader. + if (!noLimits) + { + limits = new Pkcs12LoaderLimits(limits); + } + + X509Certificate2 cert = LoadPfxNoFile( + TestData.DuplicateAttributesPfx, + TestData.PlaceholderPw, + X509KeyStorageFlags.DefaultKeySet, + loaderLimits: limits); + + using (cert) + { + Assert.Equal("Certificate 1", cert.GetNameInfo(X509NameType.SimpleName, false)); + Assert.True(cert.HasPrivateKey, "cert.HasPrivateKey"); + Assert.Equal("Microsoft Enhanced RSA and AES Cryptographic Provider", cert.FriendlyName); + + using (RSA key = cert.GetRSAPrivateKey()) + using (CngKey cngKey = Assert.IsType(key).Key) + { + Assert.Equal("Microsoft Enhanced RSA and AES Cryptographic Provider", cngKey.Provider.Provider); + Assert.True(cngKey.IsMachineKey, "cngKey.IsMachineKey"); + + // If keyname000 gets taken, we'll get a random key name on import. What's important is that we + // don't pick the second entry: keyname001. + Assert.NotEqual("keyname001", cngKey.KeyName); + } + } + } + private static byte[] MakeAttributeTest( string? keyName = null, string? friendlyName = null, bool useCapi = false, + bool machineKey = false, [CallerMemberName] string testName = null) { CngKey cngKey = null; @@ -164,6 +223,7 @@ private static byte[] MakeAttributeTest( CspParameters cspParameters = new CspParameters(24) { KeyContainerName = keyName, + Flags = machineKey ? CspProviderFlags.UseMachineKeyStore : CspProviderFlags.NoFlags, }; rsaCsp = new RSACryptoServiceProvider(2048, cspParameters); @@ -174,6 +234,7 @@ private static byte[] MakeAttributeTest( CngKeyCreationParameters cngParams = new CngKeyCreationParameters { ExportPolicy = CngExportPolicies.AllowPlaintextExport, + KeyCreationOptions = machineKey ? CngKeyCreationOptions.MachineKey : CngKeyCreationOptions.None, }; cngKey = CngKey.Create(CngAlgorithm.Rsa, keyName, cngParams); diff --git a/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/X509CertificateLoaderPkcs12Tests.cs b/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/X509CertificateLoaderPkcs12Tests.cs index 6568be9756eb2f..086721eef28b3f 100644 --- a/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/X509CertificateLoaderPkcs12Tests.cs +++ b/src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/X509CertificateLoaderPkcs12Tests.cs @@ -4,6 +4,7 @@ using System.Buffers; using System.IO; using System.IO.MemoryMappedFiles; +using System.Security.Cryptography.Pkcs; using Test.Cryptography; using Xunit; @@ -735,5 +736,153 @@ public void VerifyIndependentLifetime() } } } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void LoadWithDuplicateAttributes(bool allowDuplicates) + { + Pkcs12LoaderLimits limits = Pkcs12LoaderLimits.Defaults; + + if (allowDuplicates) + { + limits = Pkcs12LoaderLimits.DangerousNoLimits; + } + + // remove the edit lock + limits = new Pkcs12LoaderLimits(limits) + { + PreserveCertificateAlias = false, + PreserveKeyName = false, + PreserveStorageProvider = false, + PreserveUnknownAttributes = false, + }; + + Func func = + () => LoadPfxNoFile(TestData.DuplicateAttributesPfx, TestData.PlaceholderPw, loaderLimits: limits); + + if (allowDuplicates) + { + using (X509Certificate2 cert = func()) + { + Assert.Equal("Certificate 1", cert.GetNameInfo(X509NameType.SimpleName, false)); + Assert.True(cert.HasPrivateKey, "cert.HasPrivateKey"); + } + } + else + { + Pkcs12LoadLimitExceededException ex = Assert.Throws(() => func()); + Assert.Contains("AllowDuplicateAttributes", ex.Message); + } + } + +#if NET + [Theory] + [InlineData(false)] + [InlineData(true)] + public void LoadWithDuplicateAttributes_KeyOnly(bool ignorePrivateKeys) + { + byte[] pfx; + + using (ECDsa key = ECDsa.Create(ECCurve.NamedCurves.nistP256)) + { + CertificateRequest req = new CertificateRequest( + "CN=No Duplicates Here", + key, + HashAlgorithmName.SHA256); + + DateTimeOffset now = DateTimeOffset.UtcNow; + + using (X509Certificate2 cert = req.CreateSelfSigned(now, now.AddMinutes(1))) + { + Pkcs12SafeContents contents = new Pkcs12SafeContents(); + Pkcs9LocalKeyId keyId = new Pkcs9LocalKeyId(new byte[] { 2, 2, 4 }); + + Pkcs12CertBag certBag = contents.AddCertificate(cert); + certBag.Attributes.Add(keyId); + + Pkcs12KeyBag keyBag = contents.AddKeyUnencrypted(key); + keyBag.Attributes.Add(keyId); + keyBag.Attributes.Add(keyId); + + Pkcs12Builder builder = new Pkcs12Builder(); + builder.AddSafeContentsUnencrypted(contents); + builder.SealWithoutIntegrity(); + pfx = builder.Encode(); + } + } + + Pkcs12LoaderLimits limits = new Pkcs12LoaderLimits + { + IgnorePrivateKeys = ignorePrivateKeys, + }; + + Exception ex = Assert.Throws( + () => LoadPfxNoFile(pfx, loaderLimits: limits)); + + Assert.Contains("AllowDuplicateAttributes", ex.Message); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void LoadWithDuplicateAttributes_EncryptedOnly(bool ignoreEncryptedAuthSafes) + { + byte[] pfx; + + using (ECDsa key = ECDsa.Create(ECCurve.NamedCurves.nistP256)) + { + CertificateRequest req = new CertificateRequest( + "CN=No Duplicates Here", + key, + HashAlgorithmName.SHA256); + + DateTimeOffset now = DateTimeOffset.UtcNow; + + using (X509Certificate2 cert = req.CreateSelfSigned(now, now.AddMinutes(1))) + { + Pkcs12SafeContents certSafe = new Pkcs12SafeContents(); + Pkcs12SafeContents keySafe = new Pkcs12SafeContents(); + Pkcs9LocalKeyId keyId = new Pkcs9LocalKeyId(new byte[] { 2, 2, 4 }); + + Pkcs12CertBag certBag = certSafe.AddCertificate(cert); + certBag.Attributes.Add(keyId); + + Pkcs12KeyBag keyBag = keySafe.AddKeyUnencrypted(key); + keyBag.Attributes.Add(keyId); + keyBag.Attributes.Add(keyId); + + Pkcs12Builder builder = new Pkcs12Builder(); + builder.AddSafeContentsUnencrypted(certSafe); + + builder.AddSafeContentsEncrypted( + keySafe, + "", + new PbeParameters(PbeEncryptionAlgorithm.TripleDes3KeyPkcs12, HashAlgorithmName.SHA1, 1)); + + builder.SealWithoutIntegrity(); + pfx = builder.Encode(); + } + } + + Pkcs12LoaderLimits limits = new Pkcs12LoaderLimits + { + IgnoreEncryptedAuthSafes = ignoreEncryptedAuthSafes, + }; + + Func func = () => LoadPfxNoFile(pfx, loaderLimits: limits); + + if (ignoreEncryptedAuthSafes) + { + // Assert.NoThrow + func().Dispose(); + } + else + { + Exception ex = Assert.Throws(() => func()); + Assert.Contains("AllowDuplicateAttributes", ex.Message); + } + } +#endif } } diff --git a/src/libraries/Microsoft.Bcl.Cryptography/tests/Microsoft.Bcl.Cryptography.Tests.csproj b/src/libraries/Microsoft.Bcl.Cryptography/tests/Microsoft.Bcl.Cryptography.Tests.csproj index 309d9b2e1c16b0..a08fb7f599bd17 100644 --- a/src/libraries/Microsoft.Bcl.Cryptography/tests/Microsoft.Bcl.Cryptography.Tests.csproj +++ b/src/libraries/Microsoft.Bcl.Cryptography/tests/Microsoft.Bcl.Cryptography.Tests.csproj @@ -34,6 +34,7 @@ + diff --git a/src/libraries/System.Security.Cryptography/tests/X509Certificates/PfxTests.cs b/src/libraries/System.Security.Cryptography/tests/X509Certificates/PfxTests.cs index 7a6a8db3567914..f59c1e7767c91e 100644 --- a/src/libraries/System.Security.Cryptography/tests/X509Certificates/PfxTests.cs +++ b/src/libraries/System.Security.Cryptography/tests/X509Certificates/PfxTests.cs @@ -481,6 +481,38 @@ public static void CollectionPerphemeralImport_HasKeyName() } } + [Fact] + public static void VerifyNamesWithDuplicateAttributes() + { + // This is the same as the Windows Attributes test for X509CertificateLoaderPkcs12Tests, + // but using the legacy X509Certificate2 ctor, to test the settings for that set of + // loader limits with respect to duplicates. + + X509Certificate2 cert = new X509Certificate2(TestData.DuplicateAttributesPfx, TestData.PlaceholderPw); + + using (cert) + { + Assert.Equal("Certificate 1", cert.GetNameInfo(X509NameType.SimpleName, false)); + Assert.True(cert.HasPrivateKey, "cert.HasPrivateKey"); + + if (OperatingSystem.IsWindows()) + { + Assert.Equal("Microsoft Enhanced RSA and AES Cryptographic Provider", cert.FriendlyName); + + using (RSA key = cert.GetRSAPrivateKey()) + using (CngKey cngKey = Assert.IsType(key).Key) + { + Assert.Equal("Microsoft Enhanced RSA and AES Cryptographic Provider", cngKey.Provider.Provider); + Assert.True(cngKey.IsMachineKey, "cngKey.IsMachineKey"); + + // If keyname000 gets taken, we'll get a random key name on import. What's important is that we + // don't pick the second entry: keyname001. + Assert.NotEqual("keyname001", cngKey.KeyName); + } + } + } + } + internal static bool IsPkcs12IterationCountAllowed(long iterationCount, long allowedIterations) { if (allowedIterations == UnlimitedIterations) From aabdbd824c64763e7e58b47f2851fbe302d2c274 Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Wed, 3 Jul 2024 10:57:35 -0700 Subject: [PATCH 2/3] Create a new HashSet per import, rather than cache in a ThreadStatic --- .../X509CertificateLoader.Pkcs12.cs | 33 +++++++------------ 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.Pkcs12.cs b/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.Pkcs12.cs index 9b85a907e47e5b..add475535b1f1e 100644 --- a/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.Pkcs12.cs +++ b/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.Pkcs12.cs @@ -22,9 +22,6 @@ public static partial class X509CertificateLoader private const int NTE_FAIL = unchecked((int)0x80090020); #endif - [ThreadStatic] - private static HashSet? t_attributeDuplicateCheck; - static partial void LoadPkcs12NoLimits( ReadOnlyMemory data, ReadOnlySpan password, @@ -272,6 +269,8 @@ private static void ProcessSafeContents( AsnValueReader reader = outer.ReadSequence(); outer.ThrowIfNotEmpty(); + HashSet duplicateAttributeCheck = new(); + while (reader.HasData) { SafeBagAsn.Decode(ref reader, contentData, out SafeBagAsn bag); @@ -280,7 +279,7 @@ private static void ProcessSafeContents( { if (bag.BagAttributes is not null && !loaderLimits.AllowDuplicateAttributes) { - RejectDuplicateAttributes(bag.BagAttributes); + RejectDuplicateAttributes(bag.BagAttributes, duplicateAttributeCheck); } CertBagAsn certBag = CertBagAsn.Decode(bag.BagValue, AsnEncodingRules.BER); @@ -313,7 +312,7 @@ private static void ProcessSafeContents( { if (bag.BagAttributes is not null && !loaderLimits.AllowDuplicateAttributes) { - RejectDuplicateAttributes(bag.BagAttributes); + RejectDuplicateAttributes(bag.BagAttributes, duplicateAttributeCheck); } if (loaderLimits.IgnorePrivateKeys) @@ -372,12 +371,12 @@ private static void ProcessSafeContents( } } - private static void RejectDuplicateAttributes(AttributeAsn[] bagAttributes) + private static void RejectDuplicateAttributes(AttributeAsn[] bagAttributes, HashSet duplicateAttributeCheck) { // If there's only one attribute set there's no reason to instantiate the HashSet. if (bagAttributes.Length == 1) { - // Use >1 instead of=1 to account for MsPkcs12MachineKeySet, which is a named set with no values. + // Use >1 instead of =1 to account for MsPkcs12MachineKeySet, which is a named set with no values. // Though it doesn't really make sense as the only attribute. if (bagAttributes[0].AttrValues.Length > 1) { @@ -387,25 +386,17 @@ private static void RejectDuplicateAttributes(AttributeAsn[] bagAttributes) return; } - HashSet set = (t_attributeDuplicateCheck ??= new HashSet()); - set.Clear(); + duplicateAttributeCheck.Clear(); - try + foreach (AttributeAsn attrSet in bagAttributes) { - foreach (AttributeAsn attrSet in bagAttributes) + // Use >1 instead of =1 to account for MsPkcs12MachineKeySet, which is a named set with no values. + // An empty attribute set can't be followed by the same empty set, or a non-empty set. + if (!duplicateAttributeCheck.Add(attrSet.AttrType) || attrSet.AttrValues.Length > 1) { - // Use >1 instead of=1 to account for MsPkcs12MachineKeySet, which is a named set with no values. - // An empty attribute set can't be followed by the same empty set, or a non-empty set. - if (!set.Add(attrSet.AttrType) || attrSet.AttrValues.Length > 1) - { - throw new Pkcs12LoadLimitExceededException(nameof(Pkcs12LoaderLimits.AllowDuplicateAttributes)); - } + throw new Pkcs12LoadLimitExceededException(nameof(Pkcs12LoaderLimits.AllowDuplicateAttributes)); } } - finally - { - set.Clear(); - } } private static void FilterAttributes( From 764754c4a9d898de59fa093e6db6fcc6a9ffc8a1 Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Wed, 3 Jul 2024 11:56:17 -0700 Subject: [PATCH 3/3] Delete out of date comment, unnecessary code --- .../X509CertificateLoader.Pkcs12.cs | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.Pkcs12.cs b/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.Pkcs12.cs index add475535b1f1e..0a216a3a191058 100644 --- a/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.Pkcs12.cs +++ b/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.Pkcs12.cs @@ -373,19 +373,6 @@ private static void ProcessSafeContents( private static void RejectDuplicateAttributes(AttributeAsn[] bagAttributes, HashSet duplicateAttributeCheck) { - // If there's only one attribute set there's no reason to instantiate the HashSet. - if (bagAttributes.Length == 1) - { - // Use >1 instead of =1 to account for MsPkcs12MachineKeySet, which is a named set with no values. - // Though it doesn't really make sense as the only attribute. - if (bagAttributes[0].AttrValues.Length > 1) - { - throw new Pkcs12LoadLimitExceededException(nameof(Pkcs12LoaderLimits.AllowDuplicateAttributes)); - } - - return; - } - duplicateAttributeCheck.Clear(); foreach (AttributeAsn attrSet in bagAttributes)