From 24e52ad7e319bbd70351f936fe52415abbbc59e9 Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Mon, 12 Feb 2024 16:15:19 -0800 Subject: [PATCH 1/4] Harden UnixPkcs12Reader AllocHGlobal * Do not do the allocation and parsing in the constructor, since that 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. --- .../X509Certificates/AndroidCertificatePal.cs | 3 ++- .../X509Certificates/AndroidPkcs12Reader.cs | 6 ++--- .../AppleCertificatePal.Pkcs12.iOS.cs | 3 ++- .../AppleCertificatePal.Pkcs12.macOS.cs | 3 ++- .../X509Certificates/ApplePkcs12Reader.iOS.cs | 3 +-- .../ApplePkcs12Reader.macOS.cs | 3 +-- .../X509Certificates/OpenSslPkcs12Reader.cs | 6 ++--- .../X509Certificates/StorePal.Android.cs | 3 ++- .../X509Certificates/StorePal.iOS.cs | 3 ++- .../X509Certificates/StorePal.macOS.cs | 3 ++- .../X509Certificates/UnixPkcs12Reader.cs | 24 +++++++++++++++---- 11 files changed, 39 insertions(+), 21 deletions(-) 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 index f75879604e3f2..30a6cdbce3c29 100644 --- 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 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 index a22e15530798f..10800a71d537b 100644 --- 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 data) + internal AndroidPkcs12Reader() { - ParsePkcs12(data); } public static bool IsPkcs12(ReadOnlySpan 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 index 26a1f569abe17..baa791d59f34e 100644 --- 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 index 118f7067691e6..6e329434278de 100644 --- 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 index 5900a979ed83c..e493436e01d7b 100644 --- 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 data) + internal ApplePkcs12Reader() { - ParsePkcs12(data); } protected override ICertificatePalCore ReadX509Der(ReadOnlyMemory 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 index 7c1121a6c86ac..8f3274d15d232 100644 --- 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 data) + internal ApplePkcs12Reader() { - ParsePkcs12(data); } protected override ICertificatePalCore ReadX509Der(ReadOnlyMemory 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 index 7ce3eb5f31973..c0a4616273c04 100644 --- 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 data) + private OpenSslPkcs12Reader() { - ParsePkcs12(data); } protected override ICertificatePalCore ReadX509Der(ReadOnlyMemory 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 index 0c3e92bfd94cc..962287bc2630e 100644 --- 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 index ae90eabcf23a6..edccc0b79e337 100644 --- 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 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 index af87c145119b0..b424e971b09e4 100644 --- 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 index b4f39384cf0e3..d8fe618c507d5 100644 --- 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 @@ -30,7 +30,7 @@ internal abstract class UnixPkcs12Reader : IDisposable protected abstract ICertificatePalCore ReadX509Der(ReadOnlyMemory data); protected abstract AsymmetricAlgorithm LoadKey(ReadOnlyMemory safeBagBagValue); - protected void ParsePkcs12(ReadOnlySpan data) + internal void ParsePkcs12(ReadOnlySpan data) { try { @@ -42,10 +42,24 @@ protected void ParsePkcs12(ReadOnlySpan data) unsafe { - IntPtr tmpPtr = Marshal.AllocHGlobal(encodedData.Length); - Span tmpSpan = new Span((byte*)tmpPtr, encodedData.Length); - encodedData.CopyTo(tmpSpan); - _tmpManager = new PointerMemoryManager((void*)tmpPtr, encodedData.Length); + IntPtr tmpPtr = IntPtr.Zero; + + try + { + tmpPtr = Marshal.AllocHGlobal(encodedData.Length); + Span tmpSpan = new Span((byte*)tmpPtr, encodedData.Length); + encodedData.CopyTo(tmpSpan); + _tmpManager = new PointerMemoryManager((void*)tmpPtr, encodedData.Length); + } + catch + { + if (tmpPtr != IntPtr.Zero) + { + Marshal.FreeHGlobal(tmpPtr); + } + + throw; + } } ReadOnlyMemory tmpMemory = _tmpManager.Memory; From b7940c8f84f3f8fc9dcf03aa58cabe4512879f0b Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Mon, 12 Feb 2024 18:39:26 -0800 Subject: [PATCH 2/4] Switch to NativeMemory.Alloc --- .../X509Certificates/UnixPkcs12Reader.cs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) 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 index d8fe618c507d5..f643c646e9a89 100644 --- 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 @@ -42,22 +42,17 @@ internal void ParsePkcs12(ReadOnlySpan data) unsafe { - IntPtr tmpPtr = IntPtr.Zero; + void* tmpPtr = NativeMemory.Alloc((uint)encodedData.Length); try { - tmpPtr = Marshal.AllocHGlobal(encodedData.Length); Span tmpSpan = new Span((byte*)tmpPtr, encodedData.Length); encodedData.CopyTo(tmpSpan); - _tmpManager = new PointerMemoryManager((void*)tmpPtr, encodedData.Length); + _tmpManager = new PointerMemoryManager(tmpPtr, encodedData.Length); } catch { - if (tmpPtr != IntPtr.Zero) - { - Marshal.FreeHGlobal(tmpPtr); - } - + NativeMemory.Free(tmpPtr); throw; } } @@ -139,7 +134,7 @@ public void Dispose() fixed (byte* ptr = tmp) { - Marshal.FreeHGlobal((IntPtr)ptr); + NativeMemory.Free(ptr); } } From 9f1326d457ddf9e2af5fafd8619de93bb847e543 Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Mon, 12 Feb 2024 18:56:40 -0800 Subject: [PATCH 3/4] Ensure that Dispose only disposes the memory manager once ever. --- .../X509Certificates/UnixPkcs12Reader.cs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) 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 index f643c646e9a89..08237b730c1f9 100644 --- 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 @@ -124,12 +124,15 @@ 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? manager = Interlocked.Exchange(ref _tmpManager, null); + + if (manager != null) { unsafe { - Span tmp = _tmpManager.GetSpan(); + Span tmp = manager.GetSpan(); CryptographicOperations.ZeroMemory(tmp); fixed (byte* ptr = tmp) @@ -138,8 +141,7 @@ public void Dispose() } } - ((IDisposable)_tmpManager).Dispose(); - _tmpManager = null; + ((IDisposable)manager).Dispose(); } ContentInfoAsn[]? rentedContents = Interlocked.Exchange(ref _safeContentsValues, null); From ee16bc20c001791f66969d4aa4fb3f45f4d88040 Mon Sep 17 00:00:00 2001 From: Jeremy Barton Date: Mon, 12 Feb 2024 19:16:52 -0800 Subject: [PATCH 4/4] Apply feedback --- .../X509Certificates/UnixPkcs12Reader.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) 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 index 08237b730c1f9..1f5a24fa15be2 100644 --- 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; @@ -126,7 +127,8 @@ public void Dispose() // never accessed by a second thread, and there isn't a manual call to Dispose // mixed in anywhere outside of an aborted allocation path. - PointerMemoryManager? manager = Interlocked.Exchange(ref _tmpManager, null); + PointerMemoryManager? manager = _tmpManager; + _tmpManager = null; if (manager != null) { @@ -134,18 +136,16 @@ public void Dispose() { Span tmp = manager.GetSpan(); CryptographicOperations.ZeroMemory(tmp); - - fixed (byte* ptr = tmp) - { - NativeMemory.Free(ptr); - } + NativeMemory.Free(Unsafe.AsPointer(ref MemoryMarshal.GetReference(tmp))); } ((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) {