Add support for opening a PFX with an ephemeral key

[bartonjs]

Currently opening a PFX on Windows creates a temporary keyfile on disk which is deleted if the cert is disposed/finalized without being added to a cert store.

While it might not be feasible to change the default to ephemeral, we should add the option to allow better guarantees against key-leak.

(PKCS12_NO_PERSIST_KEY, Vista+)

Rationale

Currently .NET supports loading a PFX in two different key-persistence modes. The default mode, which I call perphemeral (a portmanteau of persistent and ephemeral), is to let the key file be created on disk, and then delete that file when the certificate is disposed. Alternatively, the PFX can be loaded with X509KeyStorageFlags.PersistKeySet, and then the key files will not be deleted upon certificate disposal.

Both current modes have some pros and some cons:

Persisted

• Pro: You can add it to a persisted X509Store meaningfully.
• Pro: new X509Certificate2(cert.Handle) has no surprises
• Con: If you open a PFX persisted to add it to an X509Store, and detect the duplicate, you still have emitted the key. (Cleanup is possible, but hard to do correctly)
• Con: If you meant to use the key ephemerally this leaks a file on disk every time you use it.

Perphemeral (default)

• Pro: If your application terminates normally, no net key material was leaked to disk.
• Con: If you add a perphemeral cert to an X509Store the key file is still deleted, so while it is temporarily a public/private pair in the store it finishes as a public-only import.
• Con: new X509Certificate2(cert.Handle) makes the cleanup hard to assess from a user perspective (it's stable, but hard to describe).
• Con: If your application does not terminate normally (or it was held outside of the scope of finalization), the keyfile leaks as if you had opened it with PersistKeySet.
• Con: Under parallel loads of the same PFX there's a if-available-use race condition (out of our reach) where two different objects were given the same backing keyfile; disposing one copy eliminates the keyfile of the other.

When seeking ephemeral behavior (load from a PFX, do some signing/decrypting, dispose) it seems significantly better to just load it ephemerally.

Ephemeral (new proposal)

• Pro: No key material is ever written to disk (modulo the pagefile) so it can't create a persisted file under any circumstance.
• Pro: Immune to parallel load race conditions
• Pro: new X509Certificate2(cert.Handle) behaves just fine.
• Con: Adding the cert to an X509Store may behave as adding the public key only.
• Con: Existing interop code expecting to load the key name structure will fail.

Proposed API:

     public enum X509KeyStorageFlags
     {
         DefaultKeySet = 0,
+        EphemeralKeys = 32,
         Exportable = 4,
         MachineKeySet = 2,
         PersistKeySet = 16,
         UserKeySet = 1,
         UserProtected = 8,
}

Opening an X509Certificate2 (or Importing into a collection) with both PersistKeySet and EphemeralKeySet defined will be an error. On Windows EphemeralKeySet will also force loading into CNG. On Unix this flag has no effect.

Since the key handle is now actually owned by the native object, we need to track that dependency like an interior pointer. To that end, new public members have to be added into the CNG contract:

    public abstract partial class SafeNCryptHandle : System.Runtime.InteropServices.SafeHandle
    {
        protected SafeNCryptHandle() : base (default(System.IntPtr), default(bool)) { }
+       protected SafeNCryptHandle(System.IntPtr handle, System.Runtime.InteropServices.SafeHandle parentHandle) : base (default(System.IntPtr), default(bool)) { }
        public override bool IsInvalid { get { return default(bool); } }
        protected override bool ReleaseHandle() { return default(bool); }
        protected abstract bool ReleaseNativeHandle();
    }
    public sealed partial class SafeNCryptKeyHandle : Microsoft.Win32.SafeHandles.SafeNCryptHandle
    {
        public SafeNCryptKeyHandle() { }
+       public SafeNCryptKeyHandle(System.IntPtr handle, System.Runtime.InteropServices.SafeHandle parentHandle) : base (default(System.IntPtr), default(System.Runtime.InteropServices.SafeHandle)) { }
        protected override bool ReleaseNativeHandle() { return default(bool); }
    }

The parentHandle constructors of these SafeHandles will require that parentHandle be

• Not null (ArgumentNullException)
• Not IsClosed (ArgumentException)
• Not IsInvalid (ArgumentException)

otherwise, it will AddRef in the constructor and Release in ReleaseHandle.

[ghost]

Would it be worthwhile to mock up a version that could be embedded inside CertLoader and guys so we could at least unblock that test coverage in Pkcs.Tests before "Future" comes along?

This does mean one of the PR feedbacks get retroactively addressed: Pkcs.Tests will have Windows/Unix versions so the Configuration values become correctly named :-)

[bartonjs]

Really this won't be all that hard to do, we just need an import flag and to plumb it through (Unix already works this way). I just didn't want to commit to a release while dumping ideas out of my head :).

(We'd probably want to make X509Store.Add throw on Windows when given an ephemerally-keyed private key... though if the underlying API complains already, all the better)

[ghost]

I know the coding isn’t hard – it just sounded like the api-review politics would push this past RTM since you marked the bug as “future.”

[ghost]

Never mind – I see the milestone has been changed ☺

[ghost]

Add support for loading Pfx files with ephemeral keys.

Prior to Windows Vista, loading a certificate from a PFX file using the crypt32 function PFXImportCertStore() creates a permanent key file on disk to hold the certificate's private key. It's up to the caller to delete the key if he doesn't want it taking permanent residence on the disk.

This problem bubbles up to .NET because the X509Certificate2 class uses PFXImportCertStore() to load PFX's. The Dispose() method deletes the key off the disk unless X509KeyStorageFlags.PersistKeySet was passed to the X509Certificate2 constructor.

It would be more robust, however, to create the key in memory if the key is meant to be ephemeral.

Post-XP windows offers such an ability by passing the PKCS12_NO_PERSIST_KEY flag to PFXImportCertStore().

https://msdn.microsoft.com/en-us/library/windows/desktop/aa387314(v=vs.85).aspx

But the X509Certificate2 type does not surface the ability to specify this flag.

Motivation

We have a number of unit tests in the crypto area that need to load test certificates with private keys. Such tests take the form:

1. X509Certificate2 cert = new X509Certificate2(pfxData, password, X509KeyStorageFlags.DefaultKeySet))
2. ... test code ...
3. cert.Dispose();

(ok, in reality, we'd use the using construct. But for this particular discussion, what goes on in the Dispose() method is important, so I'd like to represent it with something other than a closing brace.)

If all goes well, line 1 creates the key on disk and line 3 deletes it.

In practice, however, developers do ctrl-C tests, and they do step through tests in debuggers until they get the info they want, then hit shift-F5. In such cases, a private key is left on their disk forever.

We also have had to disable tests that load PFX's due to race conditions when multiple tests loaded the same Pfx concurrently. This is apparently not thread-safe on Windows as these operations are racing to create a private key container with the same filename on the disk.

https://github.com/dotnet/corefx/issues/2583

Today, it's our practice to disable tests that load Pfx's, if indeed they get written at all. This means that critical parts of the crypto stack aren't gettting test coverage. If we had the ability to load Pfx's entirely in memory, this inhibition would go away.

Proposed api addition

As it happens, X509Certificate2 already lets you specify the flags to PFXImportCertStore() via the X509StorageFlag enum. Today, this enum is defined as follows:

  namespace System.Security.Cryptography.X509Certificates
  {
      [Flags]
      public enum X509KeyStorageFlags
      {
          DefaultKeySet = 0x00,  // ==> 0
          UserKeySet    = 0x01,  // ==> CRYPT_USER_KEYSET
          MachineKeySet = 0x02,  // ==> CRYPT_MACHINE_KEYSET
          Exportable    = 0x04,  // ==> CRYPT_EXPORTABLE
          UserProtected = 0x08,  // ==> CRYPT_USER_PROTECTED

          PersistKeySet = 0x10,  // ==> (none) This is used by the framework and not passed to Windows. 
      }
  }

Note that though the bits in this enum (other than PersistKeySet) have a 1-1 correspondence with the Windows flag bits, their numerical values are not the same. X509Certificate2 marshals this enum into the corresponding Windows flags, but throws an exception if you set any bits outside the defined range. So there is no option to sneak PKCS12_NO_PERSIST_KEY through by casting its integer value to the enum.

The proposal is to add a new value to this enum:

  namespace System.Security.Cryptography.X509Certificates
  {
      [Flags]
      public enum X509KeyStorageFlags
      {
          ...
          LoadPfxKeyEphemerally = 0x20, // ==> PKCS12_NO_PERSIST_KEY
      }
  }

Discussion topic: The name?

If we were to follow the existing naming pattern, we would call the new value Pkcs12NoPersistKey or perhaps just NoPersistKey

Unfortunately, this enum already has a value named PersistKeySet. Unlike all the other bits in this enum, PersistKeySet was invented by the framework and has no correspondence to any Windows flag. In fact, it's purpose is to prevent X509Certificate2.Dispose() from deleting the on-disk key that PFXImportCertStore() creates. (You would do this, for example, if you were loading the PFX for the purpose of installing it into the machine's certificate store.)

Having two orthogonal flags with opposite names is very confusing so I believe in this case, breaking the normal naming pattern is the lesser evil.

It is illegal to specify both PersistKeySet and LoadPfxKeyEphemerally.

PersistKeySet says "I want that key on disk to stay around after I've disposed the certificate." LoadPfxKeyEphemerally says "Don't create a key on disk at all. Create it in memory."

We do not have the option of persisting an ephemeral key after disposing the certificate (though we could conceivably clone it to make it look like it survived.) However, it's probably best and simplest to throw an ArgumentException in this case with a (paraphrased) message "Permission to speak freely, sir. I don't think you know what you're doing."

Unix

On Unix, PFX's already load their keys ephemerally. So the LoadPfxKeyEphemerally flag is a NOP on that platform.

[ghost]

cc @bartonjs

I've added a more formal writeup for opening Pfx's with ephemeral keys. Please take a look (esp. the part about Unix.)

The Store.Open behavior, we may want to peel off separately. That scenario already exists in theory (you could always P/Invoke to PFXImportCertStore() yourself and open the X509 with a handle) so changing those would be considered breaking.

[bartonjs]

Yeah, when I was jotting down the notes I forgot that you needed to do PersistKeySet to effectively add to the store.

I'd call it something more like EphemeralKeys; and I'd further propose making it be the default in corefx and net463 (quirked).

[ghost]

Making it the default is likely too dangerous. I’ve already discovered through experimentation that

•      Going ephemeral breaks all the ECMS decrypt tests (because GetMsgPraram(KEY_PROV_INFO) now returns “no property.”. So now I have the impetus I didn’t have before to get rid of that call and just use the keySpec from CertificateGetPrivateKey.)
  

•      Even after that fix, Decrypt still doesn’t work with ephemeral CAPI based PFX’s. Haven’t figured out why. Fortunately, there’s only one test in Pkcs.Tests.dll that needs to use the Capi-based PFX.
  

So it’s useful enough for unit testing (with these caveats) but it’s not a substitutable item.

[bartonjs]

Sad. But I'll definitely accept "some things don't work" as a reason to not make it the default.

[ghost]

Ok, it gets worse – GetRSAPrivateKey() and HasPrivateKey won’t work either (same root cause – no key provider info.)

Sure, we could try to make those api support these freaky “ephemeral certificates”, but that would be adding code paths to import and export ephemeral keys (both capi and CNG, and possibly having to update the code for incoming algorithms). At this point, the tail is wagging the dog – we''d be adding new code paths to the product to make testing easier, and we won’t be testing the paths that real applications use.

So I think this is a dead end. We’ll have to figure out how to unblock those tests some other way. I don’t like any of the available solutions but changing the setting to LoadPfx and marking the tests that use them as [Outer(/Wastes disk space if interrupted/)] is the least bad solution I can see.

[bartonjs]

I think this is an important scenario for products, not just for tests. Reopening the issue.

[ghost]

I think this is an important scenario for products

I’ll need more than that to sell this in an api review.

[bartonjs]

In order to support ephemeral keys we need to support checking to see if it has an ephemeral key.

The property CERT_KEY_PROV_HANDLE_PROP_ID emits an HCRYPTPROV for a CAPI ephemeral key. CERT_NCRYPT_KEY_HANDLE_PROP_ID emits an NCRYPT_KEY_HANDLE for a CNG key.

If we wire those up to HasPrivateKey and Get*PrivateKey (cloning/up-reffing the handles, of course) it seems like everything'll just work.

https://msdn.microsoft.com/en-us/library/windows/desktop/aa376079(v=vs.85).aspx

[ghost]

Getting the key handle isn’t the tough part. CryptAcquireCertificatePrivateKey() already does that in one step (and tells us whether got a CAPI or NCrypt handle.)

The cloning part is the tough part. The underlying crypto key handles don’t have refcounts so we can’t count on it staying around after the cert context is freed. So we have to export/re-import (assuming the key will let us), which is a different code path from opening the key separately using the provider/container name info. (I don’t recall whether the import/export can be done in an algorithm-independent way.)

So we would still be lacking code coverage for the real-life case where keys aren’t ephemeral. And for the cases where these ephemeral keys just aren’t supported by Windows (the CMS decrypt on CAPI example.)

So my questions come back to: if it doesn’t solve our impactful test problem, then what is the benefit of doing the feature?

[bartonjs]

I have this working locally now. PKCS was slightly difficult in that it reaches back into the cert context, so it had to duplicate the logic of extracting the key handle via CERT_NCRYPT_KEY_HANDLE_PROP_ID.

The EnvelopedCms/RC4 case failed, since I made Ephemeral always load as CNG; so that one needs to continue to load only-non-ephemerally. And I'd need to go back in and make all of the PKCS tests now load both ephemerally and not (except the one that can't), since the logic was duplicated.

Looks like this'll be 3 different changes:

• CNG needs to add a public SetParentHandle, so that ephemeral keys can bump a refcount on their owning certificate (so disposing the cert doesn't make that handle now be pointing to a freed key). That also allows a better solution to the "what do we do if freeKey is false?" case from CryptAcquireCertificatePrivateKey.
• X509 adds the EphemeralKeys flag/enum value, and adds support for it.
  • PKCS could add support for ephemeral-state keys at the same time, but can't really have tests for it yet, since the flag won't exist.
• PKCS tests use Ephemeral via [Theory], make sure the PKCS library supports it.

As for the benefits:

• Abnormal termination is guaranteed to not leak a key file
• Avoids the potential Win32 race condition in parallel loading PFX files
• By being a new flag, and suggesting that you're hip to The Future, CNG can always be preferred, avoiding the "SHA256? What's that?" problem with CAPI.

[johnib]

where we at with this? :)

[bartonjs]

@johnib The feature finished on 19 Oct 2016. It shipped in .NET Core 2.0. It also shipped in .NET Framework 4.7.2.

[replaysMike]

@bartonjs is there a specific reason this isn't handled in .Net Standard rather than the core/framework runtime? I'm kind of stuck here trying to use EphemeralKeySet which isn't available. I've tried supplying the raw mask, but something isn't working right as the IsPrivateKey=true but the PrivateKey is null. Works fine when using MachineKeySet but I'm trying to prevent any files from being written here.

EDIT: It would seem EphemeralKeySet works on RSA keys, but not RSA-PKCS1-KeyEx keys. The latter never loads the private key.

[bartonjs]

EphemeralKeySet forces loading in CNG on Windows, so cert.PrivateKey will always report null. It's a dead property, don't call it. Use cert.GetRSAPrivateKey() (or the appropriate algorithm).

[replaysMike]

hmm maybe I've been looking at the wrong part of the problem then. When enabling ephemeral certificate loading it breaks certificate based authentication in ASP.Net Core. I assumed it was the loading of the client certificate but its not, as I compared the RawData. It's something about the way the server certificate loads.

[bartonjs]

EphemeralKeySet loads don't work with SslStream on Windows (#23749).

[replaysMike]

EphemeralKeySet loads don't work with SslStream on Windows (#23749).

Well shoot, that's likely the issue I'm encountering. I can assume SslStream is being used by Kestrel when dealing with https connections.