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

Permit CngKey backed ECDH interoperability from different KSPs #80571

Closed
wants to merge 1 commit into from

Conversation

vcsjones
Copy link
Member

This is a proof-of-concept for fixing ECDiffieHellman that uses CNG keys when performing the exchange with a ECDiffieHellmanCngPublicKey.

The original issue is #71009.

Some background, first.

ECDiffieHellman, unlike other asymmetric cryptographic types, has a dedicated type called ECDiffieHellmanCngPublicKey. The latter type always uses the Microsoft Software Key Storage Provider for the public key. Even if the original ECDiffieHellman was created with the right KSP, when the ECDHCngPublicKey is created, it is exported from the current provider, and shoved in to the Microsoft Software Key Storage Provider. That occurs here:

internal static ECDiffieHellmanCngPublicKey FromKey(CngKey key)
{
CngKeyBlobFormat format;
string? curveName;
byte[] blob = ECCng.ExportKeyBlob(key, false, out format, out curveName);
return new ECDiffieHellmanCngPublicKey(blob, curveName, format);

This is problematic when the other party ECDiffieHellman is not in the Microsoft Software Key Storage Provider.

NCryptSecretAgreement does not perform cross CNG provider agreement:

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

So cross provider ECDH doesn't work at the CNG level.

To fix this, I originally thought about making an internal implementation of Import that preserves the provider. However, this proved problematic for some providers. The Microsoft Platform Crypto Provider, for example, forbids importing keys. Even public ones. So going from PCP -> Software Provider -> PCP doesn't work.

This lead to me to preserve the CngKey handle itself in the ECDHCngPublicKey. This way, if two ECDiffieHellmanCng instances start from the same provider, the provider is preserved.

The original Import method does not return this stashed handle. This is intentional, and it hydrates the blob. This is because Import is public. I would not want this method to start returning a CngKey object that is a handle to the original private key. So it continues to use the public blobs.

What this PR does is allow two ECDiffieHellmanCng objects to interoperate with each other if they were both constructed from a CngKey that are not in the Software Storage Provider.

What this PR does not do is try to further fix "cross" provider interoperability. My general thought here is that our guidance should be CngKeys should be from the same provider. This can be a future improvement. The largest barrier to implementing this is any kind of testability. I have no CNG provider that permits imports other than the software one. So testing this would require making blind changes.

@bartonjs apologies for the probably excessive background, and I would appreciate your input if you think I am off the mark on the approach for this one.

@ghost
Copy link

ghost commented Jan 12, 2023

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

Issue Details

This is a proof-of-concept for fixing ECDiffieHellman that uses CNG keys when performing the exchange with a ECDiffieHellmanCngPublicKey.

The original issue is #71009.

Some background, first.

ECDiffieHellman, unlike other asymmetric cryptographic types, has a dedicated type called ECDiffieHellmanCngPublicKey. The latter type always uses the Microsoft Software Key Storage Provider for the public key. Even if the original ECDiffieHellman was created with the right KSP, when the ECDHCngPublicKey is created, it is exported from the current provider, and shoved in to the Microsoft Software Key Storage Provider. That occurs here:

internal static ECDiffieHellmanCngPublicKey FromKey(CngKey key)
{
CngKeyBlobFormat format;
string? curveName;
byte[] blob = ECCng.ExportKeyBlob(key, false, out format, out curveName);
return new ECDiffieHellmanCngPublicKey(blob, curveName, format);

This is problematic when the other party ECDiffieHellman is not in the Microsoft Software Key Storage Provider.

NCryptSecretAgreement does not perform cross CNG provider agreement:

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

So cross provider ECDH doesn't work at the CNG level.

To fix this, I originally thought about making an internal implementation of Import that preserves the provider. However, this proved problematic for some providers. The Microsoft Platform Crypto Provider, for example, forbids importing keys. Even public ones. So going from PCP -> Software Provider -> PCP doesn't work.

This lead to me to preserve the CngKey handle itself in the ECDHCngPublicKey. This way, if two ECDiffieHellmanCng instances start from the same provider, the provider is preserved.

The original Import method does not return this stashed handle. This is intentional, and it hydrates the blob. This is because Import is public. I would not want this method to start returning a CngKey object that is a handle to the original private key. So it continues to use the public blobs.

What this PR does is allow two ECDiffieHellmanCng objects to interoperate with each other if they were both constructed from a CngKey that are not in the Software Storage Provider.

What this PR does not do is try to further fix "cross" provider interoperability. My general thought here is that our guidance should be CngKeys should be from the same provider. This can be a future improvement. The largest barrier to implementing this is any kind of testability. I have no CNG provider that permits imports other than the software one. So testing this would require making blind changes.

@bartonjs apologies for the probably excessive background, and I would appreciate your input if you think I am off the mark on the approach for this one.

Author: vcsjones
Assignees: -
Labels:

area-System.Security

Milestone: -

@bartonjs
Copy link
Member

We probably need to talk with the CNG team to find out what the answer is for ECDH. If the platform provider won't allow public-only keys, and CNG complains about cross-provider calls, then it feels like CNG doesn't really have a story for ECDH+TPM.

(If it's us that complains about the cross-provider calls, presumably you tested removing that?)

The approach here seems... OK... but I'm not sure it solves a problem, other than letting us run tests of just talking to ourselves.

@vcsjones
Copy link
Member Author

Okay. I'll close this for now, send some emails, and hopefully come back to the table with some other ideas.

@vcsjones vcsjones closed this Jan 18, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 17, 2023
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.

2 participants