From 546330145ce0617aa91b923e78fc0e275f0fbb82 Mon Sep 17 00:00:00 2001 From: Tomas Weinfurt Date: Fri, 4 Sep 2020 11:31:14 -0700 Subject: [PATCH] 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 --- .../Interop.Keychain.cs | 43 ++++++++++++++++------ .../Pal.OSX/AppleCertificatePal.Pkcs12.cs | 4 +- .../Cryptography/Pal.OSX/AppleCertificatePal.cs | 23 +++++++++++- 3 files changed, 56 insertions(+), 14 deletions(-) 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 --- 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 random = stackalloc byte[randomSize]; + RandomNumberGenerator.Fill(random); + + // Create hex-like UTF8 string. + Span 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 --- 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 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 --- 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; -- 2.7.4