From 9967fe088198510190630dbc2602a99b76233081 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Fri, 15 Mar 2024 17:19:23 -0400 Subject: [PATCH 1/2] Harden SecKeyCopyExternalRepresentation against sensitive keys. The previous approach relied on examining the error code returned from SecKeyCopyExternalRepresentation when a key needed to be exported with a password. Apple has changed the error code which resulted in breaking the detection of sensitive values. This change looks for the kSecAttrIsSensitive attribute on a key which according to the Apple documentation, "When set to kCFBooleanTrue, the item can only be exported in an encrypted format". This should be less brittle change checking for the error code. --- .../Interop.SecKeyRef.cs | 15 ++++----------- .../pal_seckey.c | 15 +++++++++++++++ .../pal_seckey.h | 1 + 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.SecKeyRef.cs b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.SecKeyRef.cs index 5186018760af23..4aaf9c1849d4e8 100644 --- a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.SecKeyRef.cs +++ b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.SecKeyRef.cs @@ -15,6 +15,7 @@ internal static partial class AppleCrypto private const int kSuccess = 1; private const int kErrorSeeError = -2; private const int kPlatformNotSupported = -5; + private const int kOperationNotSupported = -6; internal enum PAL_KeyAlgorithm : uint { @@ -125,12 +126,6 @@ internal static bool TrySecKeyCopyExternalRepresentation( SafeSecKeyRefHandle key, out byte[] externalRepresentation) { - const int errSecPassphraseRequired = -25260; - - // macOS Sonoma 14.4 started returning errSecInvalidKeyAttributeMask when a key could not be exported - // because it must be exported with a password. - const int errSecInvalidKeyAttributeMask = -67738; - int result = AppleCryptoNative_SecKeyCopyExternalRepresentation( key, out SafeCFDataHandle data, @@ -144,12 +139,10 @@ internal static bool TrySecKeyCopyExternalRepresentation( case kSuccess: externalRepresentation = CoreFoundation.CFGetData(data); return true; + case kOperationNotSupported: + externalRepresentation = []; + return false; case kErrorSeeError: - if (Interop.CoreFoundation.GetErrorCode(errorHandle) is errSecPassphraseRequired or errSecInvalidKeyAttributeMask) - { - externalRepresentation = Array.Empty(); - return false; - } throw CreateExceptionForCFError(errorHandle); default: Debug.Fail($"SecKeyCopyExternalRepresentation returned {result}"); diff --git a/src/native/libs/System.Security.Cryptography.Native.Apple/pal_seckey.c b/src/native/libs/System.Security.Cryptography.Native.Apple/pal_seckey.c index 05147521f1898e..865976a1cfb1cc 100644 --- a/src/native/libs/System.Security.Cryptography.Native.Apple/pal_seckey.c +++ b/src/native/libs/System.Security.Cryptography.Native.Apple/pal_seckey.c @@ -72,6 +72,21 @@ int32_t AppleCryptoNative_SecKeyCopyExternalRepresentation(SecKeyRef pKey, assert(pErrorOut != NULL); *pErrorOut = NULL; + *ppDataOut = NULL; + CFDictionaryRef attributes = SecKeyCopyAttributes(pKey); + + if (attributes != NULL) + { + CFBooleanRef isSensitive = CFDictionaryGetValue(attributes, kSecAttrIsSensitive); + + if (isSensitive == kCFBooleanTrue) + { + CFRelease(attributes); + return kOperationNotSupported; + } + + CFRelease(attributes); + } *ppDataOut = SecKeyCopyExternalRepresentation(pKey, pErrorOut); return *ppDataOut == NULL ? kErrorSeeError : 1; diff --git a/src/native/libs/System.Security.Cryptography.Native.Apple/pal_seckey.h b/src/native/libs/System.Security.Cryptography.Native.Apple/pal_seckey.h index 083a9f9af5b97d..c521c53777bf0a 100644 --- a/src/native/libs/System.Security.Cryptography.Native.Apple/pal_seckey.h +++ b/src/native/libs/System.Security.Cryptography.Native.Apple/pal_seckey.h @@ -16,6 +16,7 @@ static const int32_t kErrorSeeError = -2; static const int32_t kErrorUnknownAlgorithm = -3; static const int32_t kErrorUnknownState = -4; static const int32_t kPlatformNotSupported = -5; +static const int32_t kOperationNotSupported = -6; enum { From f50a08a653514a82bfdb68a76367991c8479e2b0 Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Tue, 26 Mar 2024 12:29:12 -0400 Subject: [PATCH 2/2] Handle non-extractable keys --- .../Interop.SecKeyRef.cs | 7 +++++-- .../src/Resources/Strings.resx | 3 +++ .../tests/Resources/Strings.resx | 3 +++ .../pal_seckey.c | 21 ++++++++++++------- .../pal_seckey.h | 3 ++- 5 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.SecKeyRef.cs b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.SecKeyRef.cs index 4aaf9c1849d4e8..93a88661d25ab8 100644 --- a/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.SecKeyRef.cs +++ b/src/libraries/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.SecKeyRef.cs @@ -15,7 +15,8 @@ internal static partial class AppleCrypto private const int kSuccess = 1; private const int kErrorSeeError = -2; private const int kPlatformNotSupported = -5; - private const int kOperationNotSupported = -6; + private const int kKeyIsSensitive = -6; + private const int kKeyIsNotExtractable = -7; internal enum PAL_KeyAlgorithm : uint { @@ -139,9 +140,11 @@ internal static bool TrySecKeyCopyExternalRepresentation( case kSuccess: externalRepresentation = CoreFoundation.CFGetData(data); return true; - case kOperationNotSupported: + case kKeyIsSensitive: externalRepresentation = []; return false; + case kKeyIsNotExtractable: + throw new CryptographicException(SR.Cryptography_KeyNotExtractable); case kErrorSeeError: throw CreateExceptionForCFError(errorHandle); default: diff --git a/src/libraries/System.Security.Cryptography/src/Resources/Strings.resx b/src/libraries/System.Security.Cryptography/src/Resources/Strings.resx index 8f39a254b6cc56..a0a9e1e4afcf72 100644 --- a/src/libraries/System.Security.Cryptography/src/Resources/Strings.resx +++ b/src/libraries/System.Security.Cryptography/src/Resources/Strings.resx @@ -465,6 +465,9 @@ Key Blob not in expected format. + + The key does not permit being exported. + The key is too small for the requested operation. diff --git a/src/libraries/System.Security.Cryptography/tests/Resources/Strings.resx b/src/libraries/System.Security.Cryptography/tests/Resources/Strings.resx index 684a789df83a61..2588869aee8097 100644 --- a/src/libraries/System.Security.Cryptography/tests/Resources/Strings.resx +++ b/src/libraries/System.Security.Cryptography/tests/Resources/Strings.resx @@ -75,6 +75,9 @@ The string contains a character not in the 7 bit ASCII character set. + + The key does not permit being exported. + Removing the requested certificate would modify user trust settings, and has been denied. diff --git a/src/native/libs/System.Security.Cryptography.Native.Apple/pal_seckey.c b/src/native/libs/System.Security.Cryptography.Native.Apple/pal_seckey.c index 865976a1cfb1cc..e717eeb92a59cc 100644 --- a/src/native/libs/System.Security.Cryptography.Native.Apple/pal_seckey.c +++ b/src/native/libs/System.Security.Cryptography.Native.Apple/pal_seckey.c @@ -73,23 +73,30 @@ int32_t AppleCryptoNative_SecKeyCopyExternalRepresentation(SecKeyRef pKey, *pErrorOut = NULL; *ppDataOut = NULL; + int32_t ret = 0; CFDictionaryRef attributes = SecKeyCopyAttributes(pKey); if (attributes != NULL) { - CFBooleanRef isSensitive = CFDictionaryGetValue(attributes, kSecAttrIsSensitive); - - if (isSensitive == kCFBooleanTrue) + if (CFDictionaryGetValue(attributes, kSecAttrIsExtractable) == kCFBooleanFalse) + { + ret = kKeyIsNotExtractable; + } + else if (CFDictionaryGetValue(attributes, kSecAttrIsSensitive) == kCFBooleanTrue) { - CFRelease(attributes); - return kOperationNotSupported; + ret = kKeyIsSensitive; } CFRelease(attributes); } - *ppDataOut = SecKeyCopyExternalRepresentation(pKey, pErrorOut); - return *ppDataOut == NULL ? kErrorSeeError : 1; + if (ret == 0) + { + *ppDataOut = SecKeyCopyExternalRepresentation(pKey, pErrorOut); + ret = *ppDataOut == NULL ? kErrorSeeError : 1; + } + + return ret; } SecKeyRef AppleCryptoNative_SecKeyCopyPublicKey(SecKeyRef privateKey) diff --git a/src/native/libs/System.Security.Cryptography.Native.Apple/pal_seckey.h b/src/native/libs/System.Security.Cryptography.Native.Apple/pal_seckey.h index c521c53777bf0a..97543db915c576 100644 --- a/src/native/libs/System.Security.Cryptography.Native.Apple/pal_seckey.h +++ b/src/native/libs/System.Security.Cryptography.Native.Apple/pal_seckey.h @@ -16,7 +16,8 @@ static const int32_t kErrorSeeError = -2; static const int32_t kErrorUnknownAlgorithm = -3; static const int32_t kErrorUnknownState = -4; static const int32_t kPlatformNotSupported = -5; -static const int32_t kOperationNotSupported = -6; +static const int32_t kKeyIsSensitive = -6; +static const int32_t kKeyIsNotExtractable = -7; enum {