From 8dfcc291db79c529e00c1cc07db1af5787ccb83a Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Wed, 15 Feb 2023 23:18:08 +0100 Subject: [PATCH 1/7] Hold temporary keychain refs for certificates imported with X509Certificate2Collection.Import --- .../AppleCertificatePal.ImportExport.macOS.cs | 2 +- .../X509Certificates/StorePal.macOS.LoaderPal.cs | 14 +++++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.ImportExport.macOS.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.ImportExport.macOS.cs index 4e9d87d798c3f..7c7b9fc747d35 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.ImportExport.macOS.cs +++ b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.ImportExport.macOS.cs @@ -12,7 +12,7 @@ namespace System.Security.Cryptography.X509Certificates { internal sealed partial class AppleCertificatePal : ICertificatePal { - private SafeKeychainHandle? _tempKeychain; + internal SafeKeychainHandle? _tempKeychain; public static ICertificatePal FromBlob( ReadOnlySpan rawData, diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.macOS.LoaderPal.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.macOS.LoaderPal.cs index c6f33bbf1ce3e..80b41b22ff981 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.macOS.LoaderPal.cs +++ b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.macOS.LoaderPal.cs @@ -97,7 +97,7 @@ public void MoveTo(X509Certificate2Collection collection) using (safeSecKeyRefHandle) { - ICertificatePal newPal; + AppleCertificatePal newPal; // SecItemImport doesn't seem to respect non-exportable import for PKCS#8, // only PKCS#12. @@ -117,6 +117,18 @@ public void MoveTo(X509Certificate2Collection collection) newPal = pal.MoveToKeychain(_keychain, safeSecKeyRefHandle) ?? pal; } + if (_keychain is SafeTemporaryKeychainHandle) + { + // If we used temporary keychain we need to prevent deletion. + // on 10.15+ if keychain is unlinked, certain certificate operations may fail. + bool success = false; + _keychain.DangerousAddRef(ref success); + if (success) + { + newPal._tempKeychain = _keychain; + } + } + X509Certificate2 cert = new X509Certificate2(newPal); collection.Add(cert); } From 2c5d70e14896b3f4626dc32d03ba521f2364a178 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Thu, 16 Feb 2023 09:39:32 +0100 Subject: [PATCH 2/7] Fix incorrect SafeTemporaryKeychainHandle.TrackItem/UntrackItem pairing in X509MoveToKeychain and X509CopyWithPrivateKey --- .../Interop.X509.macOS.cs | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.X509.macOS.cs b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.X509.macOS.cs index 8245a870d782f..36fe969aaff68 100644 --- a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.X509.macOS.cs +++ b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.X509.macOS.cs @@ -264,6 +264,8 @@ internal static SafeSecIdentityHandle X509CopyWithPrivateKey( out identityHandle, out osStatus); + SafeTemporaryKeychainHandle.TrackItem(identityHandle); + if (result == 1) { Debug.Assert(!identityHandle.IsInvalid); @@ -288,13 +290,25 @@ internal static SafeSecIdentityHandle X509CopyWithPrivateKey( { SafeSecIdentityHandle identityHandle; int osStatus; + int result; - int result = AppleCryptoNative_X509MoveToKeychain( - cert, - targetKeychain, - privateKey ?? SafeSecKeyRefHandle.InvalidHandle, - out identityHandle, - out osStatus); + // Ensure the keychain is not disposed, if there is any associated one + using (SafeKeychainHandle keychain = Interop.AppleCrypto.SecKeychainItemCopyKeychain(cert)) + { + // AppleCryptoNative_X509MoveToKeychain can change the keychain of the input + // certificate, so we need to reflect that change. + SafeTemporaryKeychainHandle.UntrackItem(cert); + + result = AppleCryptoNative_X509MoveToKeychain( + cert, + targetKeychain, + privateKey ?? SafeSecKeyRefHandle.InvalidHandle, + out identityHandle, + out osStatus); + + SafeTemporaryKeychainHandle.TrackItem(cert); + SafeTemporaryKeychainHandle.TrackItem(identityHandle); + } if (result == 0) { From 6cb462d23fc0ce10685d4dbaf88f1bc6be16e3b9 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Thu, 16 Feb 2023 09:42:01 +0100 Subject: [PATCH 3/7] Fix build --- .../Interop.X509.macOS.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.X509.macOS.cs b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.X509.macOS.cs index 36fe969aaff68..de60db4c936ed 100644 --- a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.X509.macOS.cs +++ b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.X509.macOS.cs @@ -297,7 +297,7 @@ internal static SafeSecIdentityHandle X509CopyWithPrivateKey( { // AppleCryptoNative_X509MoveToKeychain can change the keychain of the input // certificate, so we need to reflect that change. - SafeTemporaryKeychainHandle.UntrackItem(cert); + SafeTemporaryKeychainHandle.UntrackItem(cert.DangerousGetHandle()); result = AppleCryptoNative_X509MoveToKeychain( cert, From 563749f0e94987d8efe01da99e739a290e42ebf3 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Thu, 16 Feb 2023 09:40:23 +0100 Subject: [PATCH 4/7] Fix AppleCertificatePal.DisposeTempKeychain to properly use DangerousRelease --- .../X509Certificates/AppleCertificatePal.ImportExport.macOS.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.ImportExport.macOS.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.ImportExport.macOS.cs index 7c7b9fc747d35..9e8fb87f1d161 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.ImportExport.macOS.cs +++ b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.ImportExport.macOS.cs @@ -94,7 +94,7 @@ public static ICertificatePal FromBlob( public void DisposeTempKeychain() { - Interlocked.Exchange(ref _tempKeychain, null)?.Dispose(); + Interlocked.Exchange(ref _tempKeychain, null)?.DangerousRelease(); } internal unsafe byte[] ExportPkcs8(ReadOnlySpan password) From 69e362bfcd81fadbec4b60899fa8106e4821169e Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Thu, 16 Feb 2023 10:08:13 +0100 Subject: [PATCH 5/7] Remove AppleCertificatePal._tempKeychain entirely --- .../AppleCertificatePal.ImportExport.macOS.cs | 22 +------------------ .../StorePal.macOS.LoaderPal.cs | 12 ---------- 2 files changed, 1 insertion(+), 33 deletions(-) diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.ImportExport.macOS.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.ImportExport.macOS.cs index 9e8fb87f1d161..27c1657eb3a75 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.ImportExport.macOS.cs +++ b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.ImportExport.macOS.cs @@ -12,8 +12,6 @@ namespace System.Security.Cryptography.X509Certificates { internal sealed partial class AppleCertificatePal : ICertificatePal { - internal SafeKeychainHandle? _tempKeychain; - public static ICertificatePal FromBlob( ReadOnlySpan rawData, SafePasswordHandle password, @@ -53,20 +51,7 @@ public static ICertificatePal FromBlob( using (keychain) { - AppleCertificatePal ret = ImportPkcs12(rawData, password, exportable, keychain); - if (!persist) - { - // If we used temporary keychain we need to prevent deletion. - // on 10.15+ if keychain is unlinked, certain certificate operations may fail. - bool success = false; - keychain.DangerousAddRef(ref success); - if (success) - { - ret._tempKeychain = keychain; - } - } - - return ret; + return ImportPkcs12(rawData, password, exportable, keychain); } } @@ -92,11 +77,6 @@ public static ICertificatePal FromBlob( throw new CryptographicException(); } - public void DisposeTempKeychain() - { - Interlocked.Exchange(ref _tempKeychain, null)?.DangerousRelease(); - } - internal unsafe byte[] ExportPkcs8(ReadOnlySpan password) { Debug.Assert(_identityHandle != null); diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.macOS.LoaderPal.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.macOS.LoaderPal.cs index 80b41b22ff981..23c804b937afb 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.macOS.LoaderPal.cs +++ b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.macOS.LoaderPal.cs @@ -117,18 +117,6 @@ public void MoveTo(X509Certificate2Collection collection) newPal = pal.MoveToKeychain(_keychain, safeSecKeyRefHandle) ?? pal; } - if (_keychain is SafeTemporaryKeychainHandle) - { - // If we used temporary keychain we need to prevent deletion. - // on 10.15+ if keychain is unlinked, certain certificate operations may fail. - bool success = false; - _keychain.DangerousAddRef(ref success); - if (success) - { - newPal._tempKeychain = _keychain; - } - } - X509Certificate2 cert = new X509Certificate2(newPal); collection.Add(cert); } From 4956788d33d5da7ce29691092677a262d30a769d Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Thu, 16 Feb 2023 10:27:53 +0100 Subject: [PATCH 6/7] Deterministically dispose objects when importing PKCS12 --- .../X509Certificates/StorePal.macOS.LoaderPal.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.macOS.LoaderPal.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.macOS.LoaderPal.cs index 23c804b937afb..fa00011b419c1 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.macOS.LoaderPal.cs +++ b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.macOS.LoaderPal.cs @@ -119,6 +119,11 @@ public void MoveTo(X509Certificate2Collection collection) X509Certificate2 cert = new X509Certificate2(newPal); collection.Add(cert); + + if (newPal != pal) + { + pal.Dispose(); + } } } } From 8a1eb6d3ee0d7555b3ea225da5f9687e79db0016 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Thu, 16 Feb 2023 11:04:58 +0100 Subject: [PATCH 7/7] Fix build. --- .../X509Certificates/AppleCertificatePal.ImportExport.iOS.cs | 3 --- .../Cryptography/X509Certificates/AppleCertificatePal.cs | 2 -- 2 files changed, 5 deletions(-) diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.ImportExport.iOS.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.ImportExport.iOS.cs index d042ade14b0be..b6150abb13518 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.ImportExport.iOS.cs +++ b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.ImportExport.iOS.cs @@ -157,8 +157,5 @@ public static ICertificatePal FromBlob( return result ?? FromDerBlob(rawData, GetDerCertContentType(rawData), password, keyStorageFlags); } - - // No temporary keychain on iOS - partial void DisposeTempKeychain(); } } diff --git a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.cs b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.cs index fb8c18acdcb39..969c5e01cc6e1 100644 --- a/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.cs +++ b/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.cs @@ -100,8 +100,6 @@ public void Dispose() _certHandle = null!; _identityHandle = null; - - DisposeTempKeychain(); } internal SafeSecCertificateHandle CertificateHandle => _certHandle;