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

ECDiffieHellmanCng derivekey methods fail if the private key isn't a MicrosoftSoftwareKeyStorageProvider key #71009

Open
MattR-entrust opened this issue Apr 20, 2022 · 28 comments

Comments

@MattR-entrust
Copy link

In the latest dotnet, the ECDiffieHellmanCng DeriveKeyFromHash and DeriveKeyMaterial methods will fail if the ECDH key isn't a MicrosoftSoftwareKeyStorageProvider key. This is because these methods take the public key from any KSP provider and convert them to a MicrosoftSoftwareKeyStorageProvider public key before sending them to the native dll from the provider of the private key. This breaks the contract of NCryptSecretAgreement unless the private key provider is also MicrosoftSoftwareKeyStorageProvider.
This issue came up when attempting to use a Hardware Security Module (HSM) protected non-exportable private ECDH key. The only work-around I found is to make an unsafe call to NCryptSecretAgreement etc. in my own code, by-passing these CNG methods.
I think that the code fix would be to ensure that the CNG wrapper functions import the public key into the private key's KSP provider before calling down to the ncrypt dll.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 20, 2022
@mkArtakMSFT
Copy link
Member

@blowdart can you please move this to the right area / repo? Thanks!

@mkArtakMSFT mkArtakMSFT removed the untriaged New issue has not been triaged by the area owner label Jun 20, 2022
@mkArtakMSFT mkArtakMSFT transferred this issue from dotnet/sdk Jun 20, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 20, 2022
@ghost
Copy link

ghost commented Jun 20, 2022

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

In the latest dotnet, the ECDiffieHellmanCng DeriveKeyFromHash and DeriveKeyMaterial methods will fail if the ECDH key isn't a MicrosoftSoftwareKeyStorageProvider key. This is because these methods take the public key from any KSP provider and convert them to a MicrosoftSoftwareKeyStorageProvider public key before sending them to the native dll from the provider of the private key. This breaks the contract of NCryptSecretAgreement unless the private key provider is also MicrosoftSoftwareKeyStorageProvider.
This issue came up when attempting to use a Hardware Security Module (HSM) protected non-exportable private ECDH key. The only work-around I found is to make an unsafe call to NCryptSecretAgreement etc. in my own code, by-passing these CNG methods.
I think that the code fix would be to ensure that the CNG wrapper functions import the public key into the private key's KSP provider before calling down to the ncrypt dll.

Author: MattR-entrust
Assignees: -
Labels:

area-System.Security

Milestone: -

@blowdart
Copy link
Contributor

This is right :)

@vcsjones
Copy link
Member

@MattR-entrust

In the latest dotnet,

Do you mean this is a regression in behavior from a previous version of .NET? Did this used to work in .NET Core 3.1 or .NET 5?

Do you have a quick example program that you can show to demonstrate the behavior, assuming new CngProvider("Some other CNG Provider") is an available CNG provider for a given KSP?

@MattR-entrust
Copy link
Author

I have only tested this with the latest .NET. I don't know if the same issue exists in older versions but I expect it will. I should be able to get you a minimal test case shortly.
The symptom is that the alternative provider determines that the pointer to the public key object is invalid in the context of that provider and then fails.
I am confident about my analysis from inspecting the source. Additionally, I managed a successful work round by manually calling down to the ncrypt library. I can attach this code as well. I'm not confident it is completely correct as I'm not primarily a .NET coder, and I copied the protections around calling unsafe code without understanding it.

@MattR-entrust
Copy link
Author

Program.zip

Here's a very simple piece of example code. If the provider is changed, this code will fail. It generates a new ECDH private key and uses a default public key.

@vcsjones
Copy link
Member

@MattR-entrust Can you run your sample program and include the exception that you get (remove the try / catch)? I can reproduce an issue with a different CNG Provider and I want to make sure I have reproduced the exact issue you are seeing.

@MattR-entrust
Copy link
Author

The supplied handle is invalid.
at System.Security.Cryptography.NCryptNative.DeriveSecretAgreement(SafeNCryptKeyHandle privateKey, SafeNCryptKeyHandle otherPartyPublicKey)
at System.Security.Cryptography.ECDiffieHellmanCng.DeriveSecretAgreementHandle(CngKey otherPartyPublicKey)
at System.Security.Cryptography.ECDiffieHellmanCng.DeriveSecretAgreementHandle(ECDiffieHellmanPublicKey otherPartyPublicKey)
at System.Security.Cryptography.ECDiffieHellmanCng.DeriveKeyFromHash(ECDiffieHellmanPublicKey otherPartyPublicKey, HashAlgorithmName hashAlgorithm, Byte[] secretPrepend, Byte[] secretAppend)
at HSMTest.Program.Main() in G:\Documents\CNG derive\CNGPropertiesTest\HSMTest\Program.cs:line 54

@MattR-entrust
Copy link
Author

The actual error is bubbling up from the native provider dll so the precise behaviour might vary depending on the provider being used.
With a 3rd party provider bug which NPE-d when passed an invalid public key handle, the stack trace was

StackTrace " at System.Security.Cryptography.NCryptNative.UnsafeNativeMethods.NCryptSecretAgreement(SafeNCryptKeyHandle hPrivKey, SafeNCryptKeyHandle hPubKey, SafeNCryptSecretHandle& phSecret, Int32 dwFlags)
at System.Security.Cryptography.NCryptNative.DeriveSecretAgreement(SafeNCryptKeyHandle privateKey, SafeNCryptKeyHandle otherPartyPublicKey)
at System.Security.Cryptography.ECDiffieHellmanCng.DeriveSecretAgreementHandle(CngKey otherPartyPublicKey)
at System.Security.Cryptography.ECDiffieHellmanCng.DeriveSecretAgreementHandle(ECDiffieHellmanPublicKey otherPartyPublicKey)
at System.Security.Cryptography.ECDiffieHellmanCng.DeriveKeyFromHash(ECDiffieHellmanPublicKey otherPartyPublicKey, HashAlgorithmName hashAlgorithm, Byte[] secretPrepend, Byte[] secretAppend)
at System.Security.Cryptography.ECDiffieHellman.DeriveKeyFromHash(ECDiffieHellmanPublicKey otherPartyPublicKey, HashAlgorithmName hashAlgorithm)
at MessageSecurityTest.Form1.GetEncryptionKeyCNG_new(String deviceMacId, String deviceInternalSerialNo, Byte[] devicePublicKey)

This was a 3rd party bug being triggered by the .NET bug

@vcsjones
Copy link
Member

vcsjones commented Jun 26, 2022

Thanks for the repro and the stack trace. I can reproduce it, and I think I have a work around that will work for now.

Using your example program, if you replace DeriveKeyFromHash with:

ecdhkey.KeyDerivationFunction = ECDiffieHellmanKeyDerivationFunction.Hash;
ecdhkey.HashAlgorithm = CngAlgorithm.Sha256;
ecdhkey.SecretPrepend =  prepend.ToString().HexToByteArray();
ecdhkey.SecretAppend = append.ToString().HexToByteArray();
key = ecdhkey.DeriveKeyMaterial(cngpub);

Does that work with your CNG Provider?

@MattR-entrust
Copy link
Author

Yes, that works for me. Thanks.
In general terms, should I be able to perform any of the supported derive key operations by setting the ecdhkey properties correctly and then calling ecdhkey.DeriveKeyMaterial(cngpub)?

@bartonjs
Copy link
Member

bartonjs commented Jul 5, 2022

should I be able to perform any of the supported derive key operations by setting the ecdhkey properties correctly and then calling ecdhkey.DeriveKeyMaterial(cngpub)

On Windows, yes. Not on other platforms.

Setting properties on an instance of ECDiffieHellmanCng was the original design of the ECDiffieHellman class, despite the fact that it made the base class and the only implementing class have unclear semantics of what DeriveKeyMaterial would mean. The DeriveKeyFromHash and other methods were added later, lifting all of the various modes from the ECDHCng KeyDerivationFunction property up to methods themselves.

@MattR-entrust
Copy link
Author

Thanks again. That covers everything I need at the moment.

@bartonjs
Copy link
Member

bartonjs commented Jul 6, 2022

What's the action here, @vcsjones? Change our simple import+call into doing a CngKey import into the same provider as this.CngKey has?

ECParameters otherPartyParameters = otherPartyPublicKey.ExportParameters();
using (ECDiffieHellmanCng otherPartyCng = new ECDiffieHellmanCng())
{
otherPartyCng.ImportParameters(otherPartyParameters);
using (otherKey = (ECDiffieHellmanCngPublicKey)otherPartyCng.PublicKey)
using (CngKey importedKey = otherKey.Import())
{
return DeriveKeyMaterial(importedKey);
}
}

There'd then be an argument that we should check the provider when otherPartyPublicKey is ECDiffieHellmanCngPublicKey otherKey and if they're not the same do a similar import.

And, of course, if that provider fails to import the blob (such as not allowing ephemeral/public-only keys) then I guess we can fall back to the default KSP and assume that the provider understands cross-KSP keys.

@vcsjones
Copy link
Member

vcsjones commented Jul 7, 2022

@bartonjs I was probably thinking that Import should take a CngProvider and feed that in to CngKey.Import. Can make it internal for now and consider making it public in API review later.

Something like:

CngKey importedKey;

try
{
    importedKey = otherKey.Import(Key.Provider);
}
catch (CryptographicException)
{
    importedKey = otherKey.Import();
}

I haven't spiked anything out though. Since there is a reasonable work around I was not going to push to get this fixed for .NET 7.

@vcsjones
Copy link
Member

vcsjones commented Jul 9, 2022

@bartonjs the thing that I am most curious about your input is how, if at all, we can test this. I can test it on my machine with a different CNG provider; but to my understanding, CI only has the Software KSP so we can't test this easily and prevent it from regressing.

@jeffhandley jeffhandley added this to the Future milestone Jul 11, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jul 11, 2022
@bartonjs
Copy link
Member

how, if at all, [can we] test this[?]

The platform crypto provider doesn't seem to like creating ephemeral private keys, and the smart card provider seems to "make" them but they don't work, so I'm assuming your testing thus far has involved named keys.

My thoughts then are, in the CNG test library

[ConditionalFact]
public static void EcdhWithPlatformProvider()
{
    CngAlgorithm alg = CngAlgorithm.ECDiffieHellmanP256;

    CngKeyCreationParameters creationParams = new CngKeyCreationParameters
    {
        Provider = CngProvider.MicrosoftPlatformCryptoProvider,
        KeyCreationOptions = CngKeyCreationOptions.OverwriteExistingKey,
    };

    const string AliceKeyName = "__dotnetTestECDH-A";
    const string BobKeyName = "__dotnetTestECDH-B";

    CngKey aliceKey;
    CngKey bobKey = null;

    try
    {
        aliceKey = CngKey.Create(alg, AliceKeyName, creationParams);
    }
    catch (CryptographicException)
    {
        throw new SkipException("Could not create the Alice key in the platform KSP");
    }

    try
    {
        try
        {
            bobKey = CngKey.Create(alg, BobKeyName, creationParams);
        }
        catch (CryptographicException)
        {
            throw new SkipException("Could not create the Alice key in the platform KSP");
        }

        using (ECDiffieHellmanCng alice = new ECDiffieHellmanCng(aliceKey))
        using (ECDiffieHellmanCng bob = new ECDiffieHellmanCng(bobKey))
        using (ECDiffieHellman charlie = new ECDiffieHellmanCng(ECCurve.NamedCurves.nistP256))
        using (ECDiffieHellman dave = ECDiffieHellman.Create(ECCurve.NamedCurves.nistP256))
        {
            byte[] ab = alice.DeriveKeyFromHash(bob.PublicKey, HashAlgorithmName.SHA384);
            byte[] ba = bob.DeriveKeyFromHash(alice.PublicKey, HashAlgorithmName.SHA384);

            AssertExtensions.SequenceEqual(ab, ba);

            byte[] ac = alice.DeriveKeyFromHash(charlie.PublicKey, HashAlgorithmName.SHA384);
            byte[] ca = charlie.DeriveKeyFromHash(alice.PublicKey, HashAlgorithmName.SHA384);

            AssertExtensions.SequenceEqual(ac, ca);

            byte[] bd = bob.DeriveKeyFromHash(dave.PublicKey, HashAlgorithmName.SHA384);
            byte[] db = dave.DeriveKeyFromHash(bob.PublicKey, HashAlgorithmName.SHA384);

            AssertExtensions.SequenceEqual(bd, db);
        }
    }
    finally
    {
        aliceKey.Delete();
        bobKey?.Delete();
    }
}
  • Two CNG keys from the same, non-software, KSP
  • Two CNG keys from different KSPs
  • A platform KSP and the "I don't care where it comes from" wrapper key (which, yeah, is the previous test case, but not from a black-box perspective)

@tegobena
Copy link

Is this issue resolved? I am getting the error message "The supplied handle is invalid." with the following stack trace when I use HSM as a provider. It is working fine when I used default Microsoft provider.
I am trying this in .Net6 and I get the same error if I set the values using properties and call DeriveMaterial or call DeriveKeyFromHmac directly with the parameters.

Here is the code:
ECDiffieHellmanPublicKey remoteEcDiffHellmanPublicKey = myremoteEcKeyParam.PublicKey;

var cngKey = CngKey.Open("MyKeyName", hsmProvider);
var eccCurve = new ECDiffieHellmanCng(cngKey);    

eccCurve.KeyDerivationFunction = ECDiffieHellmanKeyDerivationFunction.Hmac;
eccCurve.HashAlgorithm = CngAlgorithm.Sha256;
eccCurve.KeySize = 256;
eccCurve.HmacKey = myKey; //32 byte key
eccCurve.SecretPrepend = "secretToPrepend";

var derivedSecret = eccCurve.DeriveKeyMaterial(remoteEcDiffHellmanPublicKey);

//Or use DeriveKeyFromHmac 

var derivedSecret = eccCurve.DeriveKeyFromHmac(remoteEcDiffHellmanPublicKey, HashAlgorithmName.SHA256,
myKey, secretToPrepend, null);

Stack Trace:
at Interop.NCrypt.DeriveSecretAgreement(SafeNCryptKeyHandle privateKey, SafeNCryptKeyHandle otherPartyPublicKey)
at System.Security.Cryptography.ECDiffieHellmanCng.DeriveSecretAgreementHandle(CngKey otherPartyPublicKey)
at System.Security.Cryptography.ECDiffieHellmanCng.DeriveKeyMaterial(CngKey otherPartyPublicKey)
at System.Security.Cryptography.ECDiffieHellmanCng.DeriveKeyMaterial(ECDiffieHellmanPublicKey otherPartyPublicKey)

@vcsjones
Copy link
Member

@tegobena no, the issue is still open. A few pull requests have been made to work toward fixing using ECC with ECDiffieHellman, but a few more remain.

var derivedSecret = eccCurve.DeriveKeyMaterial(remoteEcDiffHellmanPublicKey);

Are you using the overload that accepts a CngKey or a ECDiffieHellmanPublicKey? DeriveKeyMaterial should work if you use the CngKey overload.

@tegobena
Copy link

@vcsjones Thank you for your quick response! The error is showing up in both cases. I don't have the remote key created in HSM, so I am using the default Microsoft provider for that for the public key.

@vcsjones
Copy link
Member

I don't have the remote key created in HSM, so I am using the default Microsoft provider for that for the public key.

The public key and the private key need to be in the same provider. This is not a limitation of .NET, but rather from CNG. From the NCryptSecretAgreement docs:

This key and the hPubKey key must come from the same key storage provider.
This key and the hPrivKey key must come from the same key storage provider.

So you can't derive a secret key with two different storage providers.

You should be able to export the public key and create an ephemeral CngKey with it in the HSM's provider. Maybe something like this:

static CngKey CopyToStorageProvider(CngKey key, CngProvider targetProvider)
{
    byte[] publicBlob = key.Export(CngKeyBlobFormat.EccPublicBlob);
    return CngKey.Import(publicBlob, CngKeyBlobFormat.EccPublicBlob, targetProvider);
}

@MattR-entrust
Copy link
Author

MattR-entrust commented Nov 15, 2022 via email

@vcsjones
Copy link
Member

And the issue is that the .NET implementation forces the public key into the MS provider regardless of which provider it is

Right, the only way to do that right now is to use DeriveKeyMaterial the accepts a CngKey. Then we won't try to move the public key to the wrong storage provider.

This issue is open so when we transport the public key we will do it for the right provider.

@tegobena
Copy link

tegobena commented Nov 15, 2022

Thank you so much for your help @vcsjones and @MattR-entrust! Using CngKey.Import as you motioned above I don't see the error related to "The supplied key is invalid". It works fine when I use Hash Derivative Function. I am seeing another error when I use HMAC. It looks when I add HmacKey it doesn't set the SecretAgreementFlags correctly. Do you know if there is any work around related to this?

Code modified:
ECDiffieHellmanPublicKey remoteEcDiffHellmanPublicKey = myremoteEcKeyParam.PublicKey;
var cngremoteKey = CngKey.Import(remoteEcDiffHellmanPublicKey.ToByteArray(), CngKeyBlobFormat.EccPublicBlob, hsmProvider);

var cngKey = CngKey.Open("MyKeyName", hsmProvider);
var eccCurve = new ECDiffieHellmanCng(cngKey);

eccCurve.KeyDerivationFunction = ECDiffieHellmanKeyDerivationFunction.Hmac;
eccCurve.HashAlgorithm = CngAlgorithm.Sha256;
eccCurve.KeySize = 256;
eccCurve.HmacKey = myKey; //32 byte key. It looks SecretAgreementFlags is not set properly when I set the HmacKey
eccCurve.SecretPrepend = "secretToPrepend";
var derivedSecret = eccCurve.DeriveKeyMaterial(cngremoteKey);

Error Message: "Invalid flags specified."
Stack Trace:
This exception was originally thrown at this call stack:
Interop.NCrypt.DeriveKeyMaterial(Microsoft.Win32.SafeHandles.SafeNCryptSecretHandle, string, System.ReadOnlySpan<Interop.NCrypt.NCryptBuffer>, Interop.NCrypt.SecretAgreementFlags)
Interop.NCrypt.DeriveKeyMaterial(Microsoft.Win32.SafeHandles.SafeNCryptSecretHandle, string, string, byte[], byte[], byte[], Interop.NCrypt.SecretAgreementFlags)
System.Security.Cryptography.ECDiffieHellmanCng.DeriveKeyMaterial(System.Security.Cryptography.CngKey)

@vcsjones
Copy link
Member

I can't reproduce that behavior using the Microsoft Software provider.

using System.Security.Cryptography;

CngKey a = CngKey.Create(CngAlgorithm.ECDiffieHellmanP256);
CngKey b = CngKey.Create(CngAlgorithm.ECDiffieHellmanP256);

using var ec = new ECDiffieHellmanCng(a);

ec.KeyDerivationFunction = ECDiffieHellmanKeyDerivationFunction.Hmac;
ec.HashAlgorithm = CngAlgorithm.Sha256;
ec.KeySize = 256;
ec.HmacKey = new byte[32];
ec.SecretPrepend = new byte[] { 1, 2, 3 };
var derivedSecret = ec.DeriveKeyMaterial(b);
Console.WriteLine(Convert.ToHexString(derivedSecret));

Is it possible that your CNG provider does not support support the Hmac KDF?

@tegobena
Copy link

Thank you @vcsjones! As you mentioned above HMAC derivation is not supported by the provider.

@samintz
Copy link

samintz commented Mar 30, 2023

I am having a similar but different issue. I use CngKey.Import() to create the public imported CngKey with a valid HSM provider handle. I had to create a ECC_PUBLIC_BLOB to import the data. In the examples above, that blob creation stuff handles inside the .Net code. Inside of the Import method, there are 2 native NCrypt API calls. The first is to NCryptImportKey() and that is succeeding. The second is to NCryptSetProperty() where the "CLR IsEphemeral" property is set to 1.
However, that set property call fails with "Key not valid for use in specified state".
It succeeds on Server 2012 R2. But fails on Win10, Server 2016, and Server 2019.

@vcsjones
Copy link
Member

where the "CLR IsEphemeral" property is set to 1.

This sounds similar to the issue described in #71310.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants