Re-enable asymmetric key generation on macOS. #16588

bartonjs · 2017-03-01T23:50:37Z

Generate RSA and ECDSA keys via a temporary keychain.

When no keychain is given, SecKeyGeneratePair will generate the keys into the
default/login keychain. Since the keys were not expected to have been in the
keychain, they permanently leak out and the keychain grows over time.

Instead, have the managed code provide a temporary keychain into which the keys
will be created. Then, export them and re-import them to the NULL keychain. The
temporary keychain can then be deleted, and the hard problem of delayed cleanup is
avoided.

Files on disk will still leak if the process terminates during the key generation
phase, but that is a much smaller window than other 'temporary file on disk' solutions.

Generate RSA and ECDSA keys via a temporary keychain. When no keychain is given, SecKeyGeneratePair will generate the keys into the default/login keychain. Since the keys were not expected to have been in the keychain, they permanently leak out and the keychain grows over time. Instead, have the managed code provide a temporary keychain into which the keys will be created. Then, export them and re-import them to the NULL keychain. The temporary keychain can then be deleted, and the hard problem of delayed cleanup is avoided. Files on disk will still leak if the process terminates during the key generation phase, but that is a much smaller window than other 'temporary file on disk' solutions.

stephentoub · 2017-03-02T12:35:01Z

src/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.Ecc.cs

+                    }
+
+                    Debug.Fail($"Unexpected result from AppleCryptoNative_EccGenerateKey: {result}");
+                    throw new CryptographicException();


Should we use the osStatus regardless? i.e.

Debug.Assert(result == 0, $"Unexpected result from {nameof(AppleCryptoNative_EccGenerateKey)}: {result}"); throw CreateExceptionForOSStatus(osStatus);

?

A non-boolean value should mean that we don't trust osStatus to report the true cause of the error (it might even still be noErr).

stephentoub · 2017-03-02T12:35:32Z

src/Common/src/Interop/OSX/System.Security.Cryptography.Native.Apple/Interop.RSA.cs

+                    }
+
+                    Debug.Fail($"Unexpected result from AppleCryptoNative_RsaGenerateKey: {result}");
+                    throw new CryptographicException();


Same question

stephentoub · 2017-03-02T12:36:13Z

src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_ecc.cpp

@@ -4,8 +4,8 @@

 #include "pal_ecc.h"

-extern "C" int
-AppleCryptoNative_EccGenerateKey(int32_t keySizeBits, SecKeyRef* pPublicKey, SecKeyRef* pPrivateKey, int32_t* pOSStatus)
+extern "C" int AppleCryptoNative_EccGenerateKey(


The return type should be changed from int to int32_t

stephentoub · 2017-03-02T12:37:22Z

src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_rsa.cpp

@@ -7,8 +7,8 @@
 static int32_t ExecuteCFDataTransform(
    SecTransformRef xform, uint8_t* pbData, int32_t cbData, CFDataRef* pDataOut, CFErrorRef* pErrorOut);

-extern "C" int
-AppleCryptoNative_RsaGenerateKey(int32_t keySizeBits, SecKeyRef* pPublicKey, SecKeyRef* pPrivateKey, int32_t* pOSStatus)
+extern "C" int AppleCryptoNative_RsaGenerateKey(


int => int32_t... maybe search and replace in these files in case there are more of 'em

Fixed these two int => int32_ts. There are others, which is causing clang-format to have a conniption; I'll do that as a style only change after this one goes in.

stephentoub · 2017-03-02T12:38:26Z

src/Native/Unix/System.Security.Cryptography.Native.Apple/pal_seckey.cpp

+
+    SecItemImportExportKeyParameters keyParams = {};
+    keyParams.version = SEC_KEY_IMPORT_EXPORT_PARAMS_VERSION;
+    keyParams.passphrase = CFSTR("ExportImportPassphrase");


What is this string? Is it part of the spec?

Does CFSTR allocate anything, and if so, does it need to be freed somewhere?

Apple's export API doesn't (under most circumstances) allow plaintext export. So I have to give it a non-null value for the export passphrase (and the same value on import to decrypt the blob).

CFSTR has the following behaviors:

The returned CFString has the following semantics:

Because CFSTR is not a Create or Copy function, you are not responsible for releasing the string when you no longer need it.

The string is not released by CFString. In other words, CFString guarantees its validity until the program terminates.

I assume the docs meant "not released by CFRelease", since that's the only thing that makes sense.

The string can be retained and released in a balanced fashion, like any other CFString.

https://developer.apple.com/library/content/documentation/CoreFoundation/Conceptual/CFStrings/Articles/CreatingAndCopying.html

That said, I've usually been calling CFRelease (because point 2 says it doesn't hurt), so I'll do so here, too.

stephentoub · 2017-03-02T12:41:21Z

src/System.Security.Cryptography.Algorithms/tests/DefaultRSAProvider.cs

@@ -43,7 +43,7 @@ public bool SupportsSha2Oaep
            get { return RuntimeInformation.IsOSPlatform(OSPlatform.Windows); }
        }

-        public bool SupportsKeyGeneration => !RuntimeInformation.IsOSPlatform(OSPlatform.OSX);
+        public bool SupportsKeyGeneration => true;


Are there platforms now that don't support key generation?

Apple doesn't support DSA. But RSA and ECDSA are full on again now, so I'll go ahead and remove the conditionals.

Generate RSA and ECDSA keys via a temporary keychain. When no keychain is given, SecKeyGeneratePair will generate the keys into the default/login keychain. Since the keys were not expected to have been in the keychain, they permanently leak out and the keychain grows over time. Instead, have the managed code provide a temporary keychain into which the keys will be created. Then, export them and re-import them to the NULL keychain. The temporary keychain can then be deleted, and the hard problem of delayed cleanup is avoided. Files on disk will still leak if the process terminates during the key generation phase, but that is a much smaller window than other 'temporary file on disk' solutions. Commit migrated from dotnet/corefx@37ddcd3

bartonjs added the area-System.Security label Mar 1, 2017

bartonjs self-assigned this Mar 1, 2017

bartonjs requested review from stephentoub and steveharter March 1, 2017 23:50

dnfclas added the cla-already-signed label Mar 1, 2017

bartonjs changed the title ~~Re-enable async key generation on macOS.~~ Re-enable asymmetric key generation on macOS. Mar 1, 2017

stephentoub reviewed Mar 2, 2017

View reviewed changes

stephentoub approved these changes Mar 2, 2017

View reviewed changes

bartonjs added 3 commits March 2, 2017 07:57

Remove SupportsKeyGeneration conditions in RSA and ECDSA tests.

532ed0f

Properly free and null-out in ExportImportKey

c7ac835

Ensure use of int32_t vs int in shim method signatures.

d69314c

bartonjs merged commit 37ddcd3 into dotnet:dev/apple_crypto Mar 2, 2017

bartonjs deleted the enable_async_keygen branch March 2, 2017 18:36

karelz modified the milestone: 2.0.0 Mar 7, 2017

bartonjs removed their assignment Mar 14, 2017

MuhammadAliYousaf mentioned this pull request Feb 1, 2020

Call keychain APIs using .NetCore on macOS dotnet/runtime#29672

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-enable asymmetric key generation on macOS. #16588

Re-enable asymmetric key generation on macOS. #16588

bartonjs commented Mar 1, 2017

stephentoub Mar 2, 2017 •

edited

Loading

bartonjs Mar 2, 2017

stephentoub Mar 2, 2017

stephentoub Mar 2, 2017

stephentoub Mar 2, 2017

bartonjs Mar 2, 2017

stephentoub Mar 2, 2017

stephentoub Mar 2, 2017

bartonjs Mar 2, 2017

stephentoub Mar 2, 2017

bartonjs Mar 2, 2017

Re-enable asymmetric key generation on macOS. #16588

Re-enable asymmetric key generation on macOS. #16588

Conversation

bartonjs commented Mar 1, 2017

stephentoub Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Mar 2, 2017 •

edited

Loading