Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[macOS] Use data keys on macOS for RSA, EC cryptography #51976

Merged
merged 1 commit into from
May 3, 2021

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Apr 28, 2021

Resurrects the remaining work from #38101 (cc @vcsjones). Two code paths in S.S.C.X509Certificates are changed to explicitly import keys though the legacy CSSM keys because they are the only ones that reliably work with the macOS keychain APIs.

It also enables more of the RSA OAEP code paths through native code instead of using the managed RsaPaddingProcesser.

@ghost
Copy link

ghost commented Apr 28, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

Test run for #51926 + reunification of the key import/export to use data keys on macOS. This should in theory be significantly faster but last CI run had one failing test and that needs to be investigated.

Author: filipnavara
Assignees: -
Labels:

area-System.Security

Milestone: -

@filipnavara
Copy link
Member Author

filipnavara commented Apr 28, 2021

Hmm, just as expected it runs into the same trap as dotnet/corefx#27394. RSA PKCS #1 encryption/decryption on macOS 10.14 and older is broken for empty data (https://github.com/apple-opensource/Security/blob/e7e7d09ddf756349e64613a6890b0807ca06001f/OSX/sec/Security/SecRSAKey.c#L507). It's not a problem on recent iOS and macOS versions. It is a solvable problem by doing the de-padding on the managed side.

@filipnavara filipnavara force-pushed the ios-crypto-reunify branch 6 times, most recently from cc75c55 to a7aea91 Compare April 28, 2021 15:04
@filipnavara filipnavara force-pushed the ios-crypto-reunify branch 2 times, most recently from 6a31b21 to 973f8a4 Compare April 28, 2021 19:26
@filipnavara filipnavara marked this pull request as ready for review April 29, 2021 07:49
using (PinAndClear.Track(rsaPrivateKey))
{
return Interop.AppleCrypto.ImportEphemeralKey(rsaPrivateKey, true);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side-note for reviewers: The GetPrivateKey method is called from one place which disposes the returned SafeHandle. In case we return a new handle here it will dispose it and then dispose the original RSA key. In the case of DSA below we reuse the internal handle and it gets disposed twice which is harmless.

@bartonjs bartonjs merged commit d272a6f into dotnet:main May 3, 2021
@filipnavara filipnavara deleted the ios-crypto-reunify branch May 4, 2021 01:45
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants