From 6f9076c28cf83c31bbecc012d6edc246fa11c0bc Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Wed, 3 Jul 2024 21:21:05 -0700 Subject: [PATCH] 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 | 54 +++++-- .../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, 452 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 033c9c9db0d9f..740d464b96e06 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 3200771cee0ad..f19859c54b035 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 196af51bc4d91..0a216a3a19105 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; @@ -268,12 +269,19 @@ private static void ProcessSafeContents( AsnValueReader reader = outer.ReadSequence(); outer.ThrowIfNotEmpty(); + HashSet duplicateAttributeCheck = new(); + while (reader.HasData) { SafeBagAsn.Decode(ref reader, contentData, out SafeBagAsn bag); if (bag.BagId == Oids.Pkcs12CertBag) { + if (bag.BagAttributes is not null && !loaderLimits.AllowDuplicateAttributes) + { + RejectDuplicateAttributes(bag.BagAttributes, duplicateAttributeCheck); + } + CertBagAsn certBag = CertBagAsn.Decode(bag.BagValue, AsnEncodingRules.BER); if (certBag.CertId == Oids.Pkcs12X509CertBagType) @@ -302,6 +310,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, duplicateAttributeCheck); + } + if (loaderLimits.IgnorePrivateKeys) { continue; @@ -344,6 +357,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 +371,21 @@ private static void ProcessSafeContents( } } + private static void RejectDuplicateAttributes(AttributeAsn[] bagAttributes, HashSet duplicateAttributeCheck) + { + duplicateAttributeCheck.Clear(); + + 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) + { + throw new Pkcs12LoadLimitExceededException(nameof(Pkcs12LoaderLimits.AllowDuplicateAttributes)); + } + } + } + private static void FilterAttributes( Pkcs12LoaderLimits loaderLimits, ref SafeBagAsn bag, @@ -362,10 +393,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 +407,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 458f9b04f4cb4..4c1025a7c1c02 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 84d57f6e7e74e..13fa4bb81971f 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 444094c15b0ca..3e07c4e25f563 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 6568be9756eb2..086721eef28b3 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 309d9b2e1c16b..a08fb7f599bd1 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 7a6a8db356791..f59c1e7767c91 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)