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 than checking the error result.
jasonleenaylor · Mar 26, 2024 · 969d424 · 969d424
1 parent 5f7e495
commit 969d424
Show file tree

Hide file tree

Showing 5 changed files with 39 additions and 13 deletions.
diff --git a/...ies/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.SecKeyRef.cs b/...ies/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.SecKeyRef.cs
@@ -15,6 +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 kKeyIsSensitive = -6;
+        private const int kKeyIsNotExtractable = -7;
 
         internal enum PAL_KeyAlgorithm : uint
         {
@@ -125,12 +127,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 +140,12 @@ internal static bool TrySecKeyCopyExternalRepresentation(
                     case kSuccess:
                         externalRepresentation = CoreFoundation.CFGetData(data);
                         return true;
+                    case kKeyIsSensitive:
+                        externalRepresentation = [];
+                        return false;
+                    case kKeyIsNotExtractable:
+                        throw new CryptographicException(SR.Cryptography_KeyNotExtractable);
                     case kErrorSeeError:
-                        if (Interop.CoreFoundation.GetErrorCode(errorHandle) is errSecPassphraseRequired or errSecInvalidKeyAttributeMask)
-                        {
-                            externalRepresentation = Array.Empty<byte>();
-                            return false;
-                        }
                         throw CreateExceptionForCFError(errorHandle);
                     default:
                         Debug.Fail($"SecKeyCopyExternalRepresentation returned {result}");

diff --git a/src/libraries/System.Security.Cryptography/src/Resources/Strings.resx b/src/libraries/System.Security.Cryptography/src/Resources/Strings.resx
@@ -465,6 +465,9 @@
   <data name="Cryptography_KeyBlobParsingError" xml:space="preserve">
     <value>Key Blob not in expected format.</value>
   </data>
+  <data name="Cryptography_KeyNotExtractable" xml:space="preserve">
+    <value>The key does not permit being exported.</value>
+  </data>
   <data name="Cryptography_KeyTooSmall" xml:space="preserve">
     <value>The key is too small for the requested operation.</value>
   </data>

diff --git a/src/libraries/System.Security.Cryptography/tests/Resources/Strings.resx b/src/libraries/System.Security.Cryptography/tests/Resources/Strings.resx
@@ -75,6 +75,9 @@
   <data name="Cryptography_Invalid_IA5String" xml:space="preserve">
     <value>The string contains a character not in the 7 bit ASCII character set.</value>
   </data>
+  <data name="Cryptography_KeyNotExtractable" xml:space="preserve">
+    <value>The key does not permit being exported.</value>
+  </data>
   <data name="Cryptography_X509Store_WouldModifyUserTrust" xml:space="preserve">
     <value>Removing the requested certificate would modify user trust settings, and has been denied.</value>
   </data>

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
@@ -72,9 +72,31 @@ int32_t AppleCryptoNative_SecKeyCopyExternalRepresentation(SecKeyRef pKey,
     assert(pErrorOut != NULL);
 
     *pErrorOut = NULL;
+    *ppDataOut = NULL;
+    int32_t ret = 0;
+    CFDictionaryRef attributes = SecKeyCopyAttributes(pKey);
 
-    *ppDataOut = SecKeyCopyExternalRepresentation(pKey, pErrorOut);
-    return *ppDataOut == NULL ? kErrorSeeError : 1;
+    if (attributes != NULL)
+    {
+        if (CFDictionaryGetValue(attributes, kSecAttrIsExtractable) == kCFBooleanFalse)
+        {
+            ret = kKeyIsNotExtractable;
+        }
+        else if (CFDictionaryGetValue(attributes, kSecAttrIsSensitive) == kCFBooleanTrue)
+        {
+            ret = kKeyIsSensitive;
+        }
+
+        CFRelease(attributes);
+    }
+
+    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
@@ -16,6 +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 kKeyIsSensitive = -6;
+static const int32_t kKeyIsNotExtractable = -7;
 
 enum
 {