Harden UnixPkcs12Reader native memory usage

bartonjs · web-flow · commit 74afd1bd44ad · 2024-02-12T21:32:23.000-08:00
* Do not do the allocation and parsing in the constructor, since throwing there prevents Dispose from running
* If something goes wrong between AllocHGlobal and moving the pointer into a PointerMemoryManager, call FreeHGlobal.
    * Past that point it will be correctly freed by Dispose.
* Switch from AllocHGlobal/FreeHGlobal to NativeMemory.Alloc/Free
* Harden the Dispose method against mid-execution failure.
diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AndroidCertificatePal.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AndroidCertificatePal.cs
@@ -118,8 +118,9 @@ ref MemoryMarshal.GetReference(rawData),
 
         private static AndroidCertificatePal ReadPkcs12(ReadOnlySpan<byte> rawData, SafePasswordHandle password, bool ephemeralSpecified)
         {
-            using (var reader = new AndroidPkcs12Reader(rawData))
+            using (var reader = new AndroidPkcs12Reader())
             {
+                reader.ParsePkcs12(rawData);
                 reader.Decrypt(password, ephemeralSpecified);
 
                 UnixPkcs12Reader.CertAndKey certAndKey = reader.GetSingleCert();
diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AndroidPkcs12Reader.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AndroidPkcs12Reader.cs
@@ -11,17 +11,17 @@ namespace System.Security.Cryptography.X509Certificates
 {
     internal sealed class AndroidPkcs12Reader : UnixPkcs12Reader
     {
-        internal AndroidPkcs12Reader(ReadOnlySpan<byte> data)
+        internal AndroidPkcs12Reader()
         {
-            ParsePkcs12(data);
         }
 
         public static bool IsPkcs12(ReadOnlySpan<byte> data)
         {
             try
             {
-                using (var reader = new AndroidPkcs12Reader(data))
+                using (var reader = new AndroidPkcs12Reader())
                 {
+                    reader.ParsePkcs12(data);
                     return true;
                 }
             }
diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.Pkcs12.iOS.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.Pkcs12.iOS.cs
@@ -14,8 +14,9 @@ private static AppleCertificatePal ImportPkcs12(
             SafePasswordHandle password,
             bool ephemeralSpecified)
         {
-            using (ApplePkcs12Reader reader = new ApplePkcs12Reader(rawData))
+            using (ApplePkcs12Reader reader = new ApplePkcs12Reader())
             {
+                reader.ParsePkcs12(rawData);
                 reader.Decrypt(password, ephemeralSpecified);
                 return ImportPkcs12(reader.GetSingleCert());
             }
diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.Pkcs12.macOS.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.Pkcs12.macOS.cs
@@ -15,8 +15,9 @@ private static AppleCertificatePal ImportPkcs12(
             bool exportable,
             SafeKeychainHandle keychain)
         {
-            using (ApplePkcs12Reader reader = new ApplePkcs12Reader(rawData))
+            using (ApplePkcs12Reader reader = new ApplePkcs12Reader())
             {
+                reader.ParsePkcs12(rawData);
                 reader.Decrypt(password, ephemeralSpecified: false);
 
                 UnixPkcs12Reader.CertAndKey certAndKey = reader.GetSingleCert();
diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/ApplePkcs12Reader.iOS.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/ApplePkcs12Reader.iOS.cs
@@ -10,9 +10,8 @@ namespace System.Security.Cryptography.X509Certificates
 {
     internal sealed class ApplePkcs12Reader : UnixPkcs12Reader
     {
-        internal ApplePkcs12Reader(ReadOnlySpan<byte> data)
+        internal ApplePkcs12Reader()
         {
-            ParsePkcs12(data);
         }
 
         protected override ICertificatePalCore ReadX509Der(ReadOnlyMemory<byte> data)
diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/ApplePkcs12Reader.macOS.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/ApplePkcs12Reader.macOS.cs
@@ -11,9 +11,8 @@ namespace System.Security.Cryptography.X509Certificates
 {
     internal sealed class ApplePkcs12Reader : UnixPkcs12Reader
     {
-        internal ApplePkcs12Reader(ReadOnlySpan<byte> data)
+        internal ApplePkcs12Reader()
         {
-            ParsePkcs12(data);
         }
 
         protected override ICertificatePalCore ReadX509Der(ReadOnlyMemory<byte> data)
diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslPkcs12Reader.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslPkcs12Reader.cs
@@ -9,9 +9,8 @@ namespace System.Security.Cryptography.X509Certificates
 {
     internal sealed class OpenSslPkcs12Reader : UnixPkcs12Reader
     {
-        private OpenSslPkcs12Reader(ReadOnlySpan<byte> data)
+        private OpenSslPkcs12Reader()
         {
-            ParsePkcs12(data);
         }
 
         protected override ICertificatePalCore ReadX509Der(ReadOnlyMemory<byte> data)
@@ -89,7 +88,8 @@ private static bool TryRead(
 
             try
             {
-                pkcs12Reader = new OpenSslPkcs12Reader(data);
+                pkcs12Reader = new OpenSslPkcs12Reader();
+                pkcs12Reader.ParsePkcs12(data);
                 return true;
             }
             catch (CryptographicException e)
diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.Android.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.Android.cs
@@ -118,8 +118,9 @@ private static ICertificatePal[] ReadPkcs12Collection(
             SafePasswordHandle password,
             bool ephemeralSpecified)
         {
-            using (var reader = new AndroidPkcs12Reader(rawData))
+            using (var reader = new AndroidPkcs12Reader())
             {
+                reader.ParsePkcs12(rawData);
                 reader.Decrypt(password, ephemeralSpecified);
 
                 ICertificatePal[] certs = new ICertificatePal[reader.GetCertCount()];
diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.iOS.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.iOS.cs
@@ -46,10 +46,11 @@ private static ILoaderPal FromBlob(ReadOnlySpan<byte> rawData, SafePasswordHandl
             if (contentType == X509ContentType.Pkcs12)
             {
                 X509Certificate.EnforceIterationCountLimit(ref rawData, readingFromFile, password.PasswordProvided);
-                ApplePkcs12Reader reader = new ApplePkcs12Reader(rawData);
+                ApplePkcs12Reader reader = new ApplePkcs12Reader();
 
                 try
                 {
+                    reader.ParsePkcs12(rawData);
                     reader.Decrypt(password, ephemeralSpecified);
                     return new ApplePkcs12CertLoader(reader, password);
                 }
diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.macOS.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.macOS.cs
@@ -72,10 +72,11 @@ private static ApplePkcs12CertLoader ImportPkcs12(
             bool ephemeralSpecified,
             SafeKeychainHandle keychain)
         {
-            ApplePkcs12Reader reader = new ApplePkcs12Reader(rawData);
+            ApplePkcs12Reader reader = new ApplePkcs12Reader();
 
             try
             {
+                reader.ParsePkcs12(rawData);
                 reader.Decrypt(password, ephemeralSpecified);
                 return new ApplePkcs12CertLoader(reader, keychain, password, exportable);
             }
diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/UnixPkcs12Reader.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/UnixPkcs12Reader.cs
@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Formats.Asn1;
+using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
 using System.Security.Cryptography.Asn1;
 using System.Security.Cryptography.Asn1.Pkcs12;
@@ -30,7 +31,7 @@ internal abstract class UnixPkcs12Reader : IDisposable
         protected abstract ICertificatePalCore ReadX509Der(ReadOnlyMemory<byte> data);
         protected abstract AsymmetricAlgorithm LoadKey(ReadOnlyMemory<byte> safeBagBagValue);
 
-        protected void ParsePkcs12(ReadOnlySpan<byte> data)
+        internal void ParsePkcs12(ReadOnlySpan<byte> data)
         {
             try
             {
@@ -42,10 +43,19 @@ protected void ParsePkcs12(ReadOnlySpan<byte> data)
 
                 unsafe
                 {
-                    IntPtr tmpPtr = Marshal.AllocHGlobal(encodedData.Length);
-                    Span<byte> tmpSpan = new Span<byte>((byte*)tmpPtr, encodedData.Length);
-                    encodedData.CopyTo(tmpSpan);
-                    _tmpManager = new PointerMemoryManager<byte>((void*)tmpPtr, encodedData.Length);
+                    void* tmpPtr = NativeMemory.Alloc((uint)encodedData.Length);
+
+                    try
+                    {
+                        Span<byte> tmpSpan = new Span<byte>((byte*)tmpPtr, encodedData.Length);
+                        encodedData.CopyTo(tmpSpan);
+                        _tmpManager = new PointerMemoryManager<byte>(tmpPtr, encodedData.Length);
+                    }
+                    catch
+                    {
+                        NativeMemory.Free(tmpPtr);
+                        throw;
+                    }
                 }
 
                 ReadOnlyMemory<byte> tmpMemory = _tmpManager.Memory;
@@ -115,26 +125,27 @@ public void Dispose()
             // Generally, having a MemoryManager cleaned up in a Dispose is a bad practice.
             // In this case, the UnixPkcs12Reader is only ever created in a using statement,
             // never accessed by a second thread, and there isn't a manual call to Dispose
-            // mixed in anywhere.
-            if (_tmpManager != null)
+            // mixed in anywhere outside of an aborted allocation path.
+
+            PointerMemoryManager<byte>? manager = _tmpManager;
+            _tmpManager = null;
+
+            if (manager != null)
             {
                 unsafe
                 {
-                    Span<byte> tmp = _tmpManager.GetSpan();
+                    Span<byte> tmp = manager.GetSpan();
                     CryptographicOperations.ZeroMemory(tmp);
-
-                    fixed (byte* ptr = tmp)
-                    {
-                        Marshal.FreeHGlobal((IntPtr)ptr);
-                    }
+                    NativeMemory.Free(Unsafe.AsPointer(ref MemoryMarshal.GetReference(tmp)));
                 }
 
-                ((IDisposable)_tmpManager).Dispose();
-                _tmpManager = null;
+                ((IDisposable)manager).Dispose();
             }
 
-            ContentInfoAsn[]? rentedContents = Interlocked.Exchange(ref _safeContentsValues, null);
-            CertAndKey[]? rentedCerts = Interlocked.Exchange(ref _certs, null);
+            ContentInfoAsn[]? rentedContents = _safeContentsValues;
+            CertAndKey[]? rentedCerts = _certs;
+            _safeContentsValues = null;
+            _certs = null;
 
             if (rentedContents != null)
             {

Original file line number	Diff line number	Diff line change
`@@ -118,8 +118,9 @@ ref MemoryMarshal.GetReference(rawData),`
`118`	`118`
`119`	`119`	`private static AndroidCertificatePal ReadPkcs12(ReadOnlySpan<byte> rawData, SafePasswordHandle password, bool ephemeralSpecified)`
`120`	`120`	`{`
`121`		`- using (var reader = new AndroidPkcs12Reader(rawData))`
	`121`	`+ using (var reader = new AndroidPkcs12Reader())`
`122`	`122`	`{`
	`123`	`+ reader.ParsePkcs12(rawData);`
`123`	`124`	`reader.Decrypt(password, ephemeralSpecified);`
`124`	`125`
`125`	`126`	`UnixPkcs12Reader.CertAndKey certAndKey = reader.GetSingleCert();`
Original file line number	Diff line number	Diff line change
`@@ -11,17 +11,17 @@ namespace System.Security.Cryptography.X509Certificates`
`11`	`11`	`{`
`12`	`12`	`internal sealed class AndroidPkcs12Reader : UnixPkcs12Reader`
`13`	`13`	`{`
`14`		`- internal AndroidPkcs12Reader(ReadOnlySpan<byte> data)`
	`14`	`+ internal AndroidPkcs12Reader()`
`15`	`15`	`{`
`16`		`- ParsePkcs12(data);`
`17`	`16`	`}`
`18`	`17`
`19`	`18`	`public static bool IsPkcs12(ReadOnlySpan<byte> data)`
`20`	`19`	`{`
`21`	`20`	`try`
`22`	`21`	`{`
`23`		`- using (var reader = new AndroidPkcs12Reader(data))`
	`22`	`+ using (var reader = new AndroidPkcs12Reader())`
`24`	`23`	`{`
	`24`	`+ reader.ParsePkcs12(data);`
`25`	`25`	`return true;`
`26`	`26`	`}`
`27`	`27`	`}`
Original file line number	Diff line number	Diff line change
`@@ -14,8 +14,9 @@ private static AppleCertificatePal ImportPkcs12(`
`14`	`14`	`SafePasswordHandle password,`
`15`	`15`	`bool ephemeralSpecified)`
`16`	`16`	`{`
`17`		`- using (ApplePkcs12Reader reader = new ApplePkcs12Reader(rawData))`
	`17`	`+ using (ApplePkcs12Reader reader = new ApplePkcs12Reader())`
`18`	`18`	`{`
	`19`	`+ reader.ParsePkcs12(rawData);`
`19`	`20`	`reader.Decrypt(password, ephemeralSpecified);`
`20`	`21`	`return ImportPkcs12(reader.GetSingleCert());`
`21`	`22`	`}`
Original file line number	Diff line number	Diff line change
`@@ -15,8 +15,9 @@ private static AppleCertificatePal ImportPkcs12(`
`15`	`15`	`bool exportable,`
`16`	`16`	`SafeKeychainHandle keychain)`
`17`	`17`	`{`
`18`		`- using (ApplePkcs12Reader reader = new ApplePkcs12Reader(rawData))`
	`18`	`+ using (ApplePkcs12Reader reader = new ApplePkcs12Reader())`
`19`	`19`	`{`
	`20`	`+ reader.ParsePkcs12(rawData);`
`20`	`21`	`reader.Decrypt(password, ephemeralSpecified: false);`
`21`	`22`
`22`	`23`	`UnixPkcs12Reader.CertAndKey certAndKey = reader.GetSingleCert();`
Original file line number	Diff line number	Diff line change
`@@ -10,9 +10,8 @@ namespace System.Security.Cryptography.X509Certificates`
`10`	`10`	`{`
`11`	`11`	`internal sealed class ApplePkcs12Reader : UnixPkcs12Reader`
`12`	`12`	`{`
`13`		`- internal ApplePkcs12Reader(ReadOnlySpan<byte> data)`
	`13`	`+ internal ApplePkcs12Reader()`
`14`	`14`	`{`
`15`		`- ParsePkcs12(data);`
`16`	`15`	`}`
`17`	`16`
`18`	`17`	`protected override ICertificatePalCore ReadX509Der(ReadOnlyMemory<byte> data)`
Original file line number	Diff line number	Diff line change
`@@ -11,9 +11,8 @@ namespace System.Security.Cryptography.X509Certificates`
`11`	`11`	`{`
`12`	`12`	`internal sealed class ApplePkcs12Reader : UnixPkcs12Reader`
`13`	`13`	`{`
`14`		`- internal ApplePkcs12Reader(ReadOnlySpan<byte> data)`
	`14`	`+ internal ApplePkcs12Reader()`
`15`	`15`	`{`
`16`		`- ParsePkcs12(data);`
`17`	`16`	`}`
`18`	`17`
`19`	`18`	`protected override ICertificatePalCore ReadX509Der(ReadOnlyMemory<byte> data)`
Original file line number	Diff line number	Diff line change
`@@ -9,9 +9,8 @@ namespace System.Security.Cryptography.X509Certificates`
`9`	`9`	`{`
`10`	`10`	`internal sealed class OpenSslPkcs12Reader : UnixPkcs12Reader`
`11`	`11`	`{`
`12`		`- private OpenSslPkcs12Reader(ReadOnlySpan<byte> data)`
	`12`	`+ private OpenSslPkcs12Reader()`
`13`	`13`	`{`
`14`		`- ParsePkcs12(data);`
`15`	`14`	`}`
`16`	`15`
`17`	`16`	`protected override ICertificatePalCore ReadX509Der(ReadOnlyMemory<byte> data)`
`@@ -89,7 +88,8 @@ private static bool TryRead(`
`89`	`88`
`90`	`89`	`try`
`91`	`90`	`{`
`92`		`- pkcs12Reader = new OpenSslPkcs12Reader(data);`
	`91`	`+ pkcs12Reader = new OpenSslPkcs12Reader();`
	`92`	`+ pkcs12Reader.ParsePkcs12(data);`
`93`	`93`	`return true;`
`94`	`94`	`}`
`95`	`95`	`catch (CryptographicException e)`
Original file line number	Diff line number	Diff line change
`@@ -118,8 +118,9 @@ private static ICertificatePal[] ReadPkcs12Collection(`
`118`	`118`	`SafePasswordHandle password,`
`119`	`119`	`bool ephemeralSpecified)`
`120`	`120`	`{`
`121`		`- using (var reader = new AndroidPkcs12Reader(rawData))`
	`121`	`+ using (var reader = new AndroidPkcs12Reader())`
`122`	`122`	`{`
	`123`	`+ reader.ParsePkcs12(rawData);`
`123`	`124`	`reader.Decrypt(password, ephemeralSpecified);`
`124`	`125`
`125`	`126`	`ICertificatePal[] certs = new ICertificatePal[reader.GetCertCount()];`
Original file line number	Diff line number	Diff line change
`@@ -46,10 +46,11 @@ private static ILoaderPal FromBlob(ReadOnlySpan<byte> rawData, SafePasswordHandl`
`46`	`46`	`if (contentType == X509ContentType.Pkcs12)`
`47`	`47`	`{`
`48`	`48`	`X509Certificate.EnforceIterationCountLimit(ref rawData, readingFromFile, password.PasswordProvided);`
`49`		`- ApplePkcs12Reader reader = new ApplePkcs12Reader(rawData);`
	`49`	`+ ApplePkcs12Reader reader = new ApplePkcs12Reader();`
`50`	`50`
`51`	`51`	`try`
`52`	`52`	`{`
	`53`	`+ reader.ParsePkcs12(rawData);`
`53`	`54`	`reader.Decrypt(password, ephemeralSpecified);`
`54`	`55`	`return new ApplePkcs12CertLoader(reader, password);`
`55`	`56`	`}`
Original file line number	Diff line number	Diff line change
`@@ -72,10 +72,11 @@ private static ApplePkcs12CertLoader ImportPkcs12(`
`72`	`72`	`bool ephemeralSpecified,`
`73`	`73`	`SafeKeychainHandle keychain)`
`74`	`74`	`{`
`75`		`- ApplePkcs12Reader reader = new ApplePkcs12Reader(rawData);`
	`75`	`+ ApplePkcs12Reader reader = new ApplePkcs12Reader();`
`76`	`76`
`77`	`77`	`try`
`78`	`78`	`{`
	`79`	`+ reader.ParsePkcs12(rawData);`
`79`	`80`	`reader.Decrypt(password, ephemeralSpecified);`
`80`	`81`	`return new ApplePkcs12CertLoader(reader, keychain, password, exportable);`
`81`	`82`	`}`