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

[iOS] X509KeyStorageFlags in X509Certificate constructor are ignored #52434

Closed
filipnavara opened this issue May 7, 2021 · 3 comments · Fixed by #57153
Closed

[iOS] X509KeyStorageFlags in X509Certificate constructor are ignored #52434

filipnavara opened this issue May 7, 2021 · 3 comments · Fixed by #57153
Milestone

Comments

@filipnavara
Copy link
Member

The implementation of PKCS#12 certificate loading ignores the flags passed to the constructor. It always imports the key as ephemeral even if asked to persist it which is easy to fix.

The exportable flag is also ignored. It has to be investigated how it should map on iOS. It may control storing keys in the secure enclave. The keychain is per-application storage which greatly reduces the security risk of non-exportable keys actually being exportable.

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Security untriaged New issue has not been triaged by the area owner labels May 7, 2021
@ghost
Copy link

ghost commented May 7, 2021

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

Issue Details

The implementation of PKCS#12 certificate loading ignores the flags passed to the constructor. It always imports the key as ephemeral even if asked to persist it which is easy to fix.

The exportable flag is also ignored. It has to be investigated how it should map on iOS. It may control storing keys in the secure enclave. The keychain is per-application storage which greatly reduces the security risk of non-exportable keys actually being exportable.

Author: filipnavara
Assignees: -
Labels:

area-System.Security, untriaged

Milestone: -

@bartonjs bartonjs added os-ios Apple iOS and removed untriaged New issue has not been triaged by the area owner labels Jul 2, 2021
@bartonjs bartonjs added this to the 6.0.0 milestone Jul 2, 2021
@mdh1418
Copy link
Member

mdh1418 commented Aug 6, 2021

I might be missing something, but is this resolved by #55425?
It seems like the X509KeyStorageFlags keyStorageFlags are being used now?

bool ephemeralSpecified = keyStorageFlags.HasFlag(X509KeyStorageFlags.EphemeralKeySet);

I'm not very familiar, what is the exportable flag?

@bartonjs
Copy link
Member

bartonjs commented Aug 6, 2021

  • The Exportable flag means that the private keys, once imported, can be exported again (e.g. cert.GetRSAPrivateKey().ExportRSAPrivateKey()).
  • The PersistKeySet flag means that the intention is to add the certificate to a certificate store, so the private key should be written to a place that it can be used again later.
    • For macOS if PersistKeySet is used we import the cert+key into the default keychain, otherwise we use a temporary keychain.
  • The EphemeralKeySet flag means the private key shouldn't ever be written to disk.
    • This isn't supported on macOS because there's no way (at least, there used to not be) to get a SecIdentityRef without putting the key and cert into a keychain, which means the key was written to disk.

On iOS we need to do the appropriate equivalent things. EphemeralKeySet works on iOS, but PersistKeySet and Exportable might need specific support.

macOS version:

if ((keyStorageFlags & X509KeyStorageFlags.EphemeralKeySet) == X509KeyStorageFlags.EphemeralKeySet)
{
throw new PlatformNotSupportedException(SR.Cryptography_X509_NoEphemeralPfx);
}
bool exportable = (keyStorageFlags & X509KeyStorageFlags.Exportable) == X509KeyStorageFlags.Exportable;
bool persist =
(keyStorageFlags & X509KeyStorageFlags.PersistKeySet) == X509KeyStorageFlags.PersistKeySet;

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 10, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 12, 2021
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Aug 16, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants