Skip to content

Make CertLoader reject duplicate attributes #104344

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 4, 2024
Merged

Make CertLoader reject duplicate attributes #104344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9ada73a
Make CertLoader reject duplicate attributes
bartonjs Jul 3, 2024
aabdbd8
Create a new HashSet per import, rather than cache in a ThreadStatic
bartonjs Jul 3, 2024
764754c
Delete out of date comment, unnecessary code
bartonjs Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/libraries/Common/src/System/Security/Cryptography/Oids.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
21 changes: 21 additions & 0 deletions src/libraries/Common/src/System/Security/Cryptography/X509Certificates/Pkcs12LoaderLimits.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ public sealed class Pkcs12LoaderLimits
private bool _preserveUnknownAttributes;
private bool _ignorePrivateKeys;
private bool _ignoreEncryptedAuthSafes;
private bool _allowDuplicateAttributes;

/// <summary>
/// Gets a shared reference to the default loader limits.
Expand Down Expand Up @@ -72,6 +73,7 @@ public sealed class Pkcs12LoaderLimits
PreserveKeyName = true,
PreserveCertificateAlias = true,
PreserveUnknownAttributes = true,
AllowDuplicateAttributes = true,
});

/// <summary>
Expand Down Expand Up @@ -117,6 +119,7 @@ public Pkcs12LoaderLimits(Pkcs12LoaderLimits copyFrom)
_preserveUnknownAttributes = copyFrom._preserveUnknownAttributes;
_ignorePrivateKeys = copyFrom._ignorePrivateKeys;
_ignoreEncryptedAuthSafes = copyFrom._ignoreEncryptedAuthSafes;
_allowDuplicateAttributes = copyFrom._allowDuplicateAttributes;
}

/// <summary>
Expand Down Expand Up @@ -366,6 +369,24 @@ public bool IgnoreEncryptedAuthSafes
}
}

/// <summary>
/// Gets or sets a value indicating whether duplicate attributes are permitted.
/// </summary>
/// <value>
/// <see langword="true" /> to permit duplicate attributes;
/// <see langword="false" /> to fail loading when duplicate attributes are found.
/// The default is <see langword="false" />.
/// </value>
internal bool AllowDuplicateAttributes
{
get => _allowDuplicateAttributes;
set
{
CheckReadOnly();
_allowDuplicateAttributes = value;
}
}

private static void CheckNonNegative(
int? value,
[CallerArgumentExpression(nameof(value))] string? paramName = null)
Expand Down
54 changes: 43 additions & 11 deletions .../Common/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.Pkcs12.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -268,12 +269,19 @@ private static void ProcessSafeContents(
AsnValueReader reader = outer.ReadSequence();
outer.ThrowIfNotEmpty();

HashSet<string> 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)
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -355,48 +371,64 @@ private static void ProcessSafeContents(
}
}

private static void RejectDuplicateAttributes(AttributeAsn[] bagAttributes, HashSet<string> 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,
Func<Pkcs12LoaderLimits, string, bool> filter)
{
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;

if (filter(loaderLimits, attrType))
{
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);
}
}
}
Expand Down
80 changes: 80 additions & 0 deletions src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/TestData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
44 changes: 44 additions & 0 deletions ...stem/Security/Cryptography/X509Certificates/X509CertificateLoaderPkcs12CollectionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<X509Certificate2Collection> 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<Pkcs12LoadLimitExceededException>(() => func());
Assert.Contains("AllowDuplicateAttributes", ex.Message);
}
}

private sealed class CollectionDisposer : IDisposable
{
private readonly X509Certificate2Collection _coll;
Expand Down
Loading