hold ref to temp keychain on OSX to avoild premature cleanup (#41787)

* hold ref to temp keychain on OSX to avoild premature cleanup

* feedback from review

diff --git a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Keychain.cs b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Keychain.cs
index 73e18d8..36b3b7c 100644 (file)
--- a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Keychain.cs
+++ b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Keychain.cs
@@ -22,10 +22,10 @@ internal static partial class Interop
             out SafeKeychainHandle keychain);
 
         [DllImport(Libraries.AppleCryptoNative, EntryPoint = "AppleCryptoNative_SecKeychainCreate")]
-        private static extern int AppleCryptoNative_SecKeychainCreateTemporary(
+        private static extern unsafe int AppleCryptoNative_SecKeychainCreateTemporary(
             string path,
             int utf8PassphraseLength,
-            byte[] utf8Passphrase,
+            byte* utf8Passphrase,
             out SafeTemporaryKeychainHandle keychain);
 
         [DllImport(Libraries.AppleCryptoNative)]
@@ -220,25 +220,46 @@ internal static partial class Interop
             throw CreateExceptionForOSStatus(osStatus);
         }
 
-        internal static SafeTemporaryKeychainHandle CreateTemporaryKeychain()
+        internal static unsafe SafeTemporaryKeychainHandle CreateTemporaryKeychain()
         {
+            const int randomSize = 256;
             string tmpKeychainPath = Path.Combine(
                 Path.GetTempPath(),
                 Guid.NewGuid().ToString("N") + ".keychain");
 
-            // Use a distinct GUID so that if a keychain is abandoned it isn't recoverable.
-            string tmpKeychainPassphrase = Guid.NewGuid().ToString("N");
+            // Use a random password so that if a keychain is abandoned it isn't recoverable.
+            // We use stack to minimize lingering
+            Span<byte> random = stackalloc byte[randomSize];
+            RandomNumberGenerator.Fill(random);
+
+            // Create hex-like UTF8 string.
+            Span<byte> utf8Passphrase =  stackalloc byte[randomSize * 2 +1];
+            utf8Passphrase[randomSize * 2] = 0; // null termination for C string.
+
+            for (int i = 0; i < random.Length; i++)
+            {
+                // Instead of true hexadecimal, we simply take lower and upper 4 bits and we offset them from ASCII 'A'
+                // to get printable form. We dont use managed string to avoid lingering copies.
+                utf8Passphrase[i*2] = (byte)((random[i] & 0x0F) + 65);
+                utf8Passphrase[i*2 + 1] = (byte)((random[i] >> 4) & 0x0F + 65);
+            }
 
-            byte[] utf8Passphrase = System.Text.Encoding.UTF8.GetBytes(tmpKeychainPassphrase);
+            // clear the binary bits.
+            CryptographicOperations.ZeroMemory(random);
 
             SafeTemporaryKeychainHandle keychain;
+            int osStatus;
 
-            int osStatus = AppleCryptoNative_SecKeychainCreateTemporary(
-                tmpKeychainPath,
-                utf8Passphrase.Length,
-                utf8Passphrase,
-                out keychain);
+            fixed (byte* ptr = utf8Passphrase)
+            {
+                osStatus = AppleCryptoNative_SecKeychainCreateTemporary(
+                    tmpKeychainPath,
+                    utf8Passphrase.Length,
+                    ptr,
+                    out keychain);
+            }
 
+            CryptographicOperations.ZeroMemory(utf8Passphrase);
             SafeTemporaryKeychainHandle.TrackKeychain(keychain);
 
             if (osStatus == 0)

diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.Pkcs12.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.Pkcs12.cs
index 94873ee..cd4a1e8 100644 (file)
--- a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.Pkcs12.cs
+++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.Pkcs12.cs
@@ -11,7 +11,7 @@ namespace Internal.Cryptography.Pal
 {
     internal sealed partial class AppleCertificatePal : ICertificatePal
     {
-        private static ICertificatePal ImportPkcs12(
+        private static AppleCertificatePal ImportPkcs12(
             ReadOnlySpan<byte> rawData,
             SafePasswordHandle password,
             bool exportable,
@@ -57,7 +57,7 @@ namespace Internal.Cryptography.Pal
             }
         }
 
-        internal static ICertificatePal ImportPkcs12NonExportable(
+        internal static AppleCertificatePal ImportPkcs12NonExportable(
             AppleCertificatePal cert,
             SafeSecKeyRefHandle privateKey,
             SafePasswordHandle password,

diff --git a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.cs b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.cs
index fd70ac8..c54cd0e 100644 (file)
--- a/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.cs
+++ b/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.OSX/AppleCertificatePal.cs
@@ -10,6 +10,7 @@ using System.Security.Cryptography;
 using System.Security.Cryptography.Apple;
 using System.Security.Cryptography.X509Certificates;
 using System.Text;
+using System.Threading;
 using Microsoft.Win32.SafeHandles;
 
 namespace Internal.Cryptography.Pal
@@ -20,6 +21,7 @@ namespace Internal.Cryptography.Pal
         private SafeSecCertificateHandle _certHandle;
         private CertificateData _certData;
         private bool _readCertData;
+        private SafeKeychainHandle? _tempKeychain;
 
         public static ICertificatePal? FromHandle(IntPtr handle)
         {
@@ -109,7 +111,20 @@ namespace Internal.Cryptography.Pal
 
                 using (keychain)
                 {
-                    return ImportPkcs12(rawData, password, exportable, 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;
                 }
             }
 
@@ -165,6 +180,12 @@ namespace Internal.Cryptography.Pal
 
             _certHandle = null!;
             _identityHandle = null;
+
+            SafeKeychainHandle? tempKeychain = Interlocked.Exchange(ref _tempKeychain, null);
+            if (tempKeychain != null)
+            {
+                tempKeychain.Dispose();
+            }
         }
 
         internal SafeSecCertificateHandle CertificateHandle => _certHandle;