bc-csharp icon indicating copy to clipboard operation
bc-csharp copied to clipboard

[BUG] PFX generated via bouncycastle Pkcs12Store are not loading in .Net 9

Open hiddenshadow21 opened this issue 8 months ago • 10 comments

Describe the Bug

Client certificate generated when creating whole chain (CA, server, and client certificates) is not loading. It fails AllowDuplicates check added in .Net 9 (check discussion about it here).

As a last step of generating the client pfx we are attaching issuer certificate to have correct chain using bouncyCastle Pkcs12Store as shown below:

        Pkcs12Store store = new Pkcs12StoreBuilder().Build();
        var serverCert = DotNetUtilities.FromX509Certificate(certificateHolder.Certificate);

        store.Load(new MemoryStream(certBytes), Array.Empty<char>());
        store.SetCertificateEntry(serverCert.SubjectDN.ToString(), new X509CertificateEntry(serverCert));

        var memoryStream = new MemoryStream();
        store.Save(memoryStream, Array.Empty<char>(), GetSeededSecureRandom());

While debugging we found that loading our client.pfx fails duplicate check for OID 2.16.840.1.113894.746875.1.1. This one is added automatically, under the hood, when saving PKCS12 store with certificate that does not have private key attached and has an EKU (see source code here).

Are we misusing the BouncyCastle API in this scenario, or is there a recommended way to prevent this OID duplication? Is bouncycastle not compatible with .Net 9?

To Reproduce

Steps to reproduce the behavior:

  1. Download uploaded app
  2. Run scenario 3.

Expected Behavior

The PFX should load successfully without triggering the duplicate attribute check.

Screenshots and Logs

Image

Desktop

  • OS: Windows 11

Additional Context

There is issue in .Net 9 regarding this problem - https://github.com/dotnet/runtime/issues/113726

In .NET, a workaround was introduced via the internal property Pkcs12LoaderLimits.AllowDuplicateAttributes, but it is not publicly accessible.

Sample .Net 9 console app: PfxLimitsNet9.zip

There are 3 scenarios in the app:

  1. Self-signed certificates - working correctly
  2. Creating CA and server certificate (without including any in the chain) - loading these certificates is working correctly as well.
  3. Creating whole chain with client cert containing server cert in the pfx chain - not working

hiddenshadow21 avatar Apr 09 '25 12:04 hiddenshadow21

We have this OID as Org.BouncyCastle.Asn1.Misc.MiscObjectIdentifiers.id_oracle_pkcs12_trusted_key_usage. We became aware of a problem with duplicate attributes having this OID via an issue raised against bc-java at the end of last year: https://github.com/bcgit/bc-java/issues/1945 .

The duplication was addressed as part of this commit .

We'd appreciate if you could try one of the recent beta releases (e.g. https://www.nuget.org/packages/BouncyCastle.Cryptography/2.6.0-beta.114) and see if the issue is resolved for you.

peterdettman avatar Apr 17 '25 11:04 peterdettman

Unfortunately the problem still exists in version 2.6.0-beta.114. There is only one attribute, but it has 2 values, which is not allowed in .Net 9:

        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));
                }
            }
        }

hiddenshadow21 avatar Apr 17 '25 12:04 hiddenshadow21

Thanks, I will look into it further.

peterdettman avatar Apr 17 '25 13:04 peterdettman

@peterdettman Did you have a chance to take a peek at this issue?

hiddenshadow21 avatar Apr 23 '25 12:04 hiddenshadow21

Probably the restriction disallowing multiple values is too strict, at least for the Oracle TrustedCert OID involved here.

I've checked with the latest JDK code and it appears it can generate multiple attribute values (in the case of an EKU with multiple OIDs), so I don't think BC is inconsistent with that.

Assuming that's the case, it might be best to create a sample PKCS#12 file using JDK (or just keytool), with a trusted cert entry (and suitable ExtendedKeyUsage), then verify failure to load it in .NET 9 and report to dotnet the incompatibility with JDK.

I think it's also worth adding an option to bc-csharp to not include this attribute when saving.

peterdettman avatar Apr 24 '25 13:04 peterdettman

Hi Peter,

Thanks a lot for the detailed analysis and suggestions.

Would you be able to perform the test with keytool/JDK and .NET 9 yourself to confirm the behavior? We would prefer not to handle this testing ourselves if possible.

Also, adding an option to bc-csharp to omit writing this attribute when saving sounds like a great idea - it would definitely help.

Thanks again for all your help and for considering these improvements!

hiddenshadow21 avatar Apr 28 '25 12:04 hiddenshadow21

I've added the option to Pkcs12StoreBuilder and will follow up on the reproducer and bug report when I get the chance.

peterdettman avatar Apr 29 '25 06:04 peterdettman

Specifying multiple collection attributes is always tricky, and a security faux pas; because it's going to be one of:

  • First one wins.
  • Last one wins.
  • Union all of them.
  • Intersect all of them.

In this particular it appears to be last one wins (at least for OpenJDK): https://github.com/openjdk/jdk/blob/c59debb3844d009ac501a48c31822a07f00521e9/src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java#L2470-L2475

That's probably the most common interpretation, followed by first one wins (for an implementation that looks the attribute up by OID, instead of doing a foreach-style bulk processor).

I don't quite see where you're acquiring a second instance of the attribute on the same bag; but it's probably best if you tidy it up to only write one.

.NET's new X509CertificateLoader.LoadPkcs12 defaults to blocking any duplicated attributes to avoid the potential security bug of backend confusion about multiple values (and since we haven't ever, including for this one, seen a legitimate instance of a multi-valued attribute in a PKCS12 SafeBag, there's no attribute/-set that is permitted to bypass that check).

bartonjs avatar May 20 '25 00:05 bartonjs

@bartonjs Thanks for commenting. bc-csharp has already addressed writing of multiple instances of the attribute and now will only write one. We have no criticism of blocking duplicated attributes.

However my current understanding is that a single "OracleTrustedKeyUsage" attribute may be multi-valued - one value per ExtendedKeyUsage. Many occurrences may therefore be single-valued in practice, but it could be multi-valued. I find the restriction against multi-valued attributes overly strict.

For this particular attribute I believe the issue could be demonstrated by generating a certificate with more than one EKU, then using JDK (perhaps keytool) to generate a PKCS#12 file and add that certificate as a trusted cert entry. The resulting PKCS#12 keystore should now contain a multi-valued "OracleTrustedKeyUsage" attribute and will not load with .NET 9.

Even if we (BouncyCastle) are mistaken about this particular attribute, I still think the restriction against multi-valued attributes is overly strict, but we'll wait and see.

To be clear it is the "attrSet.AttrValues.Length > 1" part of this check that I think is overly restrictive:

https://github.com/dotnet/runtime/blob/b6a87649d3538a49b1d406f782306c8c71896b59/src/libraries/Common/src/System/Security/Cryptography/X509Certificates/X509CertificateLoader.Pkcs12.cs#L398C71-L398C100 .

peterdettman avatar May 20 '25 07:05 peterdettman

Assuming I'm reading OpenJDK's code correctly, the attribute is itself something like SEQUENCE OF OBJECT IDENTIFIER; which means that the expected usage would be one attribute instance in one attribute set, but the interpreted attribute value itself contains multiple EKUs.

If the attribute is just a single encoded OID and OpenJDK is actually reading one attribute set with multiple instances... then we'd need to consider a registry of allowable multi-valued attributes.

bartonjs avatar May 20 '25 16:05 bartonjs