Merged PR 38844: [release/8.0] Fix unbound MAC work in GetCertContentType

steveharter · krwq · commit 57275c8c907c · 2024-04-12T08:36:21.000Z
Fix unbound MAC work in GetCertContentType
diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Pal.macOS.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Pal.macOS.cs
@@ -111,6 +111,7 @@ private static DSA DecodeDsaPublicKey(byte[] encodedKeyValue, byte[] encodedPara
             public X509ContentType GetCertContentType(ReadOnlySpan<byte> rawData)
             {
                 const int errSecUnknownFormat = -25257;
+
                 if (rawData.IsEmpty)
                 {
                     // Throw to match Windows and Unix behavior.
@@ -119,7 +120,7 @@ public X509ContentType GetCertContentType(ReadOnlySpan<byte> rawData)
 
                 X509ContentType contentType = Interop.AppleCrypto.X509GetContentType(rawData);
 
-                // Apple doesn't seem to recognize PFX files with no MAC, so try a quick maybe-it's-a-PFX test
+                // Apple's native check can't check for PKCS12, so do a quick decode test to see if it is PKCS12 / PFX.
                 if (contentType == X509ContentType.Unknown)
                 {
                     try
@@ -128,9 +129,11 @@ public X509ContentType GetCertContentType(ReadOnlySpan<byte> rawData)
                         {
                             fixed (byte* pin = rawData)
                             {
+                                AsnValueReader reader = new AsnValueReader(rawData, AsnEncodingRules.BER);
+
                                 using (var manager = new PointerMemoryManager<byte>(pin, rawData.Length))
                                 {
-                                    PfxAsn.Decode(manager.Memory, AsnEncodingRules.BER);
+                                    PfxAsn.Decode(ref reader, manager.Memory, out _);
                                 }
 
                                 contentType = X509ContentType.Pkcs12;
diff --git a/src/libraries/System.Security.Cryptography/tests/X509Certificates/PfxIterationCountTests.X509Certificate2.cs b/src/libraries/System.Security.Cryptography/tests/X509Certificates/PfxIterationCountTests.X509Certificate2.cs
@@ -1,8 +1,13 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using Microsoft.DotNet.RemoteExecutor;
+using Microsoft.DotNet.XUnitExtensions;
+using Xunit;
+
 namespace System.Security.Cryptography.X509Certificates.Tests
 {
+    [SkipOnPlatform(TestPlatforms.Browser, "Browser doesn't support X.509 certificates")]
     public class PfxIterationCountTests_X509Certificate2 : PfxIterationCountTests
     {
         internal override X509Certificate Import(byte[] blob)
@@ -22,5 +27,29 @@ internal override X509Certificate Import(string fileName, string password)
 
         internal override X509Certificate Import(string fileName, SecureString password)
             => new X509Certificate2(fileName, password);
+
+
+        [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
+        public static void Import_IterationCountLimitExceeded_ThrowsInAllottedTime()
+        {
+            const int AllottedTime = 5000;
+
+            if (!PfxTests.Pkcs12PBES2Supported)
+            {
+                throw new SkipTestException("Pkcs12NoPassword100MRounds uses PBES2, which is not supported on this version.");
+            }
+
+            RemoteInvokeOptions options = new()
+            {
+                TimeOut = AllottedTime
+            };
+
+            RemoteExecutor.Invoke(static () =>
+            {
+                byte[] blob = TestData.Pkcs12NoPassword100MRounds;
+                CryptographicException ce = Assert.Throws<CryptographicException>(() => new X509Certificate2(blob));
+                Assert.Contains(FwlinkId, ce.Message);
+            }, options).Dispose();
+        }
     }
 }
diff --git a/src/libraries/System.Security.Cryptography/tests/X509Certificates/TestData.cs b/src/libraries/System.Security.Cryptography/tests/X509Certificates/TestData.cs
@@ -3437,6 +3437,26 @@ internal static DSAParameters GetDSA1024Params()
             "04020105000420AD0EB570ACFB8357A8E99B17672353CFBA69C76FFE5B6BC113" +
             "05577F12AE24040408D04E60444B79672302030927C1").HexToByteArray();
 
+        internal static readonly byte[] Pkcs12NoPassword100MRounds = Convert.FromBase64String(
+            "MIIDygIBAzCCA4QGCSqGSIb3DQEHAaCCA3UEggNxMIIDbTCCA2kGCSqGSIb3DQEHBqCCA1owggNW" +
+            "AgEAMIIDTwYJKoZIhvcNAQcBMF4GCSqGSIb3DQEFDTBRMDAGCSqGSIb3DQEFDDAjBBCNparJkj/3" +
+            "Uk8N7n0KCMeQAgEBMAwGCCqGSIb3DQILBQAwHQYJYIZIAWUDBAEqBBAcqpBrSDFcXYAWVWKcsEi9" +
+            "gIIC4P/ANdPYWI1vBH1U5sZGMIwLjY96pYaBelyZd0ZfKA8QfGHVNP9+E9hplBKGvRfIMiqmFutj" +
+            "RO4v7Ls8HZEk0hwBt9+6zXPWDJLxBDfSMHUd08+ZAH1yzEqq8aBMyIRVHOQkJFuFuCQJ9Ke5HzVi" +
+            "39S1rgHpnKYFvy+xZAhgI9OO1YxuFt4P9nhlEV/JCoyEQ/2iY99kKc3z7ArrV7BBFhfYGKhWQCBu" +
+            "kAmNBKweRldNWgDuW21WJEl5sByOmyDwpiK55Zxy1K1aIY8DYJTtIzzcX4CILaj6tClMH1G9w4jW" +
+            "BkQI2CG4vCsMl/28BbIP9EyH2C+gBAxvc1N32y3NSvO0/GPVenmQFF9KBMc4FVy4Z21syMKzUkBi" +
+            "PtIbDkcQbGAfyPgFk4SXCgn8OpIIvOOGI50/r+Hj14qex9VIrlwAAWCH8Y+YjwqFAQJYHQpb47zp" +
+            "B1fTwJFOrsXrBgLUzJLZKLR43yW2E9u6b8RsTuFHjh985naCHLuWPYOXS1zduBpHKpwoPUyCwD2r" +
+            "DAokCvA7RCsSXroUkpJarN4CAqsEB8COnzV1Dl2xcAYMerJxrTCKX6WIQUYo0/qeCoqTT38lDAlE" +
+            "7Ydjyx12iVM6eWejAdjORvlVtCQQtCxz8fZpdFGbMP8rf35A8hu++e4u0CLHnhTx38zPIm6H6YfN" +
+            "qj5h1Kz0xLzqnRfa7EGfDEERSHOy/DqNY4nUNG2DTjGOHy1QJelToG7Vo2L7CCZV+leX0nwLNExf" +
+            "hKEp+uQCiYSJe9iDm9fS9VymED79OJbr2bxdq3MggEGksLZv/H0ZT8Wsue0vq9jQ6J6YIEM+DKYr" +
+            "Zt2l4WgTBEKbpqmRvOqYRh9O8Sp+3IRNPzMC2ehzlYXqoPbtG4vxpoRsAMCM/W2x61jbsBSaNSFA" +
+            "eaUwcnKswRg30UonHUAIOJkqtadI57WE/Rat5eHVyya9f7ZN8bTFZjx0BQs6Bo8PK9yfqoidSN8w" +
+            "PTAfMAcGBSsOAwIaBBTt8zpgzygINykjoAwr2GKEywYFwgQUA+L1vfCVASwiE++gTfRgIScMGycC" +
+            "BAX14QA=");
+
         internal static readonly byte[] Pkcs12OpenSslOneCertDefaultEmptyPassword =
             ("308209CF0201033082098506092A864886F70D010701A0820976048209723082" +
             "096E308203E206092A864886F70D010706A08203D3308203CF020100308203C8" +
diff --git a/src/native/libs/System.Security.Cryptography.Native.Apple/pal_x509.c b/src/native/libs/System.Security.Cryptography.Native.Apple/pal_x509.c
@@ -70,14 +70,18 @@ PAL_X509ContentType AppleCryptoNative_X509GetContentType(uint8_t* pbData, int32_
     // The sniffing order is:
     // * X509 DER
     // * PKCS7 PEM/DER
-    // * PKCS12 DER (or PEM if Apple has non-standard support for that)
     // * X509 PEM or PEM aggregate (or DER, but that already matched)
     //
     // If the X509 PEM check is done first SecItemImport will erroneously match
     // some PKCS#7 blobs and say they were certificates.
     //
     // Likewise, if the X509 DER check isn't done first, Apple will report it as
     // being a PKCS#7.
+    //
+    // This does not attempt to open a PFX / PKCS12 as Apple does not provide
+    // a suitable API to determine if it is PKCS12 without doing potentially
+    // unbound MAC / KDF work. Instead, let that return Unknown and let the managed
+    // decoding do the check.
     SecCertificateRef certref = SecCertificateCreateWithData(NULL, cfData);
 
     if (certref != NULL)
@@ -104,41 +108,6 @@ PAL_X509ContentType AppleCryptoNative_X509GetContentType(uint8_t* pbData, int32_
         }
     }
 
-    dataFormat = kSecFormatPKCS12;
-    actualFormat = dataFormat;
-    itemType = kSecItemTypeAggregate;
-    actualType = itemType;
-
-    osStatus = SecItemImport(cfData, NULL, &actualFormat, &actualType, 0, NULL, NULL, NULL);
-
-    if (osStatus == errSecPassphraseRequired)
-    {
-        dataFormat = kSecFormatPKCS12;
-        actualFormat = dataFormat;
-        itemType = kSecItemTypeAggregate;
-        actualType = itemType;
-
-        SecItemImportExportKeyParameters importParams;
-        memset(&importParams, 0, sizeof(SecItemImportExportKeyParameters));
-
-        importParams.version = SEC_KEY_IMPORT_EXPORT_PARAMS_VERSION;
-        importParams.passphrase = CFSTR("");
-
-        osStatus = SecItemImport(cfData, NULL, &actualFormat, &actualType, 0, &importParams, NULL, NULL);
-
-        CFRelease(importParams.passphrase);
-        importParams.passphrase = NULL;
-    }
-
-    if (osStatus == noErr || osStatus == errSecPkcs12VerifyFailure)
-    {
-        if (actualType == itemType && actualFormat == dataFormat)
-        {
-            CFRelease(cfData);
-            return PAL_Pkcs12;
-        }
-    }
-
     dataFormat = kSecFormatX509Cert;
     actualFormat = dataFormat;
     itemType = kSecItemTypeCertificate;

Original file line number	Diff line number	Diff line change
`@@ -111,6 +111,7 @@ private static DSA DecodeDsaPublicKey(byte[] encodedKeyValue, byte[] encodedPara`
`111`	`111`	`public X509ContentType GetCertContentType(ReadOnlySpan<byte> rawData)`
`112`	`112`	`{`
`113`	`113`	`const int errSecUnknownFormat = -25257;`
	`114`	`+`
`114`	`115`	`if (rawData.IsEmpty)`
`115`	`116`	`{`
`116`	`117`	`// Throw to match Windows and Unix behavior.`
`@@ -119,7 +120,7 @@ public X509ContentType GetCertContentType(ReadOnlySpan<byte> rawData)`
`119`	`120`
`120`	`121`	`X509ContentType contentType = Interop.AppleCrypto.X509GetContentType(rawData);`
`121`	`122`
`122`		`- // Apple doesn't seem to recognize PFX files with no MAC, so try a quick maybe-it's-a-PFX test`
	`123`	`+ // Apple's native check can't check for PKCS12, so do a quick decode test to see if it is PKCS12 / PFX.`
`123`	`124`	`if (contentType == X509ContentType.Unknown)`
`124`	`125`	`{`
`125`	`126`	`try`
`@@ -128,9 +129,11 @@ public X509ContentType GetCertContentType(ReadOnlySpan<byte> rawData)`
`128`	`129`	`{`
`129`	`130`	`fixed (byte* pin = rawData)`
`130`	`131`	`{`
	`132`	`+ AsnValueReader reader = new AsnValueReader(rawData, AsnEncodingRules.BER);`
	`133`	`+`
`131`	`134`	`using (var manager = new PointerMemoryManager<byte>(pin, rawData.Length))`
`132`	`135`	`{`
`133`		`- PfxAsn.Decode(manager.Memory, AsnEncodingRules.BER);`
	`136`	`+ PfxAsn.Decode(ref reader, manager.Memory, out _);`
`134`	`137`	`}`
`135`	`138`
`136`	`139`	`contentType = X509ContentType.Pkcs12;`