Skip to content

Commit

Permalink
Make CertLoader reject duplicate attributes
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@bartonjs
bartonjs authored Jul 4, 2024
1 parent 6bfd058 commit 6f9076c
Show file tree
Hide file tree
Showing 9 changed files with 452 additions and 31 deletions.
1 change: 1 addition & 0 deletions src/libraries/Common/src/System/Security/Cryptography/Oids.cs
View file
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
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
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
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
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

0 comments on commit 6f9076c

Please sign in to comment.