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

Fix EC curve name case sensitivity on Windows #77801

Merged
merged 8 commits into from
Nov 11, 2022

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Nov 2, 2022

#72816 highlighted two issues with curve comparison on Windows.

The first, is that in some circumstances, we were not populating the Oid.Value given a curve name. This introduces a change where we will always attempt to populate the Oid's Value if we can look one up, given the friendly name. This was an issue when we compare two EC public key parameters where one may have the Oid value present, and the other one may not.

The second to to change to parameter comparison to be case insensitive with the friendly name matching.

Fixes #72816

@ghost
Copy link

ghost commented Nov 2, 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

#72816 highlighted two issues with curve comparison on Windows.

The first, is that in some circumstances, we were not populating the Oid.Value given a curve name. This introduces a change where we will always attempt to populate the Oid's Value if we can look one up, given the friendly name. This was an issue when we compare two EC public key parameters where one may have the Oid value present, and the other one may not.

The second to to change to parameter comparison to be case insensitive with the friendly name matching.

Fixes #72816

Author: vcsjones
Assignees: -
Labels:

area-System.Security

Milestone: -

@vcsjones
Copy link
Member Author

vcsjones commented Nov 2, 2022

Yes I said "EC curve".

@vcsjones
Copy link
Member Author

vcsjones commented Nov 2, 2022

@bartonjs Looks like CNG itself is case sensitive on Windows 8. We can do...:

  1. Change the test to only run on Windows 10+ as a way of saying "Case-insensitive curve names is actually a feature of Windows 10+". (Or a variation where for < 10 we assert it throws)
  2. Create a lookup table of curve names to map them to the correct casing that Windows expects.
  3. Something smarter than those two things that I did not think of.

@vcsjones
Copy link
Member Author

vcsjones commented Nov 4, 2022

I opportunistically went with option 1 there, since that is my preference, but not something I feel very strongly about. My reasoning here is to not make the platform appear to do something that it doesn't really do.

@vcsjones
Copy link
Member Author

vcsjones commented Nov 4, 2022

Okay, the more I dig at this the more I am just finding some case sensitivity inconsistencies. It's largely due to the "Make pre-Windows 10 support 'nistP256' in addition to 'ECDSA_P256'" and some of that "help" that we do is doing case sensitive mapping. So this needs to go back to draft and get more aggressively tested.

@vcsjones vcsjones marked this pull request as draft November 4, 2022 03:22
@vcsjones
Copy link
Member Author

vcsjones commented Nov 5, 2022

Okay. I think I got a more clear picture of what CNG is doing here.

The gist of it: BCRYPT_ECC_CURVE_NAME as a key property is not case sensitive. CNG algorithm identifiers are.

We treat them as interchangeable in the .NET APIs. For example:

ECDsa.Create(ECCurve.CreateFromFriendlyName("ECDSA_P256"));

This works, even though strictly speaking ECDSA_P256 is not a named curve as far as CNG is concerned. It's an algorithm identifier.

Since .NET kind of fudges the difference between an algorithm identifier and a curve name, this PR also now brings case-insensitivity to the EC algorithm identifiers. That is, "EcDsA_P256" will work. This is already the behavior on non-Windows since non-Windows is completely unaware of CNG and we just treat "nistP256" and "ECDSA_P256" as the same thing on macOS / Linux.

There are basically two choke points I was able to identify for named curves: generating a key with GenerateKey and the other is ImportParameters where the the ECParameters type has an ECCurve field.

Most other potentially interesting places like ECPrivateKey, PKCS8, PKCS12, etc. all work off of OID values, so there is no case sensitivity concern there.

I think this PR is ready at this point.

@vcsjones vcsjones marked this pull request as ready for review November 5, 2022 17:27
Brainpool curves are 'true' named cuves and get no special mapping, so use these as additional tests.
@vcsjones vcsjones merged commit 4820105 into dotnet:main Nov 11, 2022
@vcsjones vcsjones deleted the win-curvename-case-insensitive branch November 11, 2022 02:45
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2022
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.

CertificateRequest: when ECDSA named curve is unrecognized, confusing exception raised
2 participants