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

ECCurve friendly names are not cross platform #36342

Closed
eiriktsarpalis opened this issue May 13, 2020 · 6 comments · Fixed by #36362
Closed

ECCurve friendly names are not cross platform #36342

eiriktsarpalis opened this issue May 13, 2020 · 6 comments · Fixed by #36362
Labels
area-System.Security enhancement Product code improvement that does NOT require public API changes/additions untriaged New issue has not been triaged by the area owner

Comments

@eiriktsarpalis
Copy link
Member

Consider the following reproduction on Linux:

using System.Security.Cryptography;

[Theory]
[InlineData("nistP256")]   // fails
[InlineData("secp256r1")]  // fails
[InlineData("ECDSA_P256")] // passes
public static void Repro(string curveFriendlyName)
{
    var ecParameters = new ECParameters()
    {
        Curve = ECCurve.CreateFromFriendlyName(curveFriendlyName),
        Q = new ECPoint()
        {
            X = "65eda5a12577c2bae829437fe338701a10aaa375e1bb5b5de108de439c08551d".HexToByteArray(),
            Y = "1e52ed75701163f7f9e40ddf9f341b3dc9ba860af7e0ca7ca7e9eecd0084d19c".HexToByteArray(),
        },
    };

    ECDsa ecDsa = ECDsa.Create(ecParameters);
}

The first two identifiers will fail with the following exception:

System.PlatformNotSupportedException : The specified curve 'secp256r1' or its parameters are not valid for this platform.
Stack Trace:
    /home/eirik/devel/dotnet/runtime/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EcDsa.ImportExport.cs(37,0): at Interop.Crypto.EcKeyCreateByKeyParameters(String oid, Byte[] qx, Int32 qxLength, Byte[] qy, Int32 qyLength, Byte[] d, Int32 dLength)
    /home/eirik/devel/dotnet/runtime/src/libraries/Common/src/System/Security/Cryptography/ECOpenSsl.ImportExport.cs(115,0): at System.Security.Cryptography.ECOpenSsl.ImportNamedCurveParameters(ECParameters parameters)
    /home/eirik/devel/dotnet/runtime/src/libraries/Common/src/System/Security/Cryptography/ECOpenSsl.ImportExport.cs(33,0): at System.Security.Cryptography.ECOpenSsl.ImportParameters(ECParameters parameters)
    /home/eirik/devel/dotnet/runtime/src/libraries/Common/src/System/Security/Cryptography/ECDsaOpenSsl.cs(317,0): at System.Security.Cryptography.ECDsaImplementation.ECDsaOpenSsl.ImportParameters(ECParameters parameters)
    /home/eirik/devel/dotnet/runtime/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/ECDsaOpenSsl.cs(37,0): at System.Security.Cryptography.ECDsa.Create(ECParameters parameters)
    /home/eirik/devel/dotnet/runtime/src/libraries/System.Security.Cryptography.Encoding/tests/Cbor.Tests/CborWriterTests.cs(303,0): at System.Formats.Cbor.Tests.CborWriterTests.Repro(String curveFriendlyName)

Note that all three identifiers are recognized on Windows, so we should probably attempt to do the same in the OpenSSL implementation for the sake of consistency?

@eiriktsarpalis eiriktsarpalis added enhancement Product code improvement that does NOT require public API changes/additions area-System.Security labels May 13, 2020
@ghost
Copy link

ghost commented May 13, 2020

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
Notify danmosemsft if you want to be subscribed.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label May 13, 2020
@eiriktsarpalis
Copy link
Member Author

Realted to #36002

@vcsjones
Copy link
Member

Some more common names could be added to the static lookup table in OidLookup.cs but there will always be platform differences for what are accepted as friendly names since this API defers to CAPI / OpenSSL. For example, "prime256v1" will work on Linux but fail on Windows in your test case.

@bartonjs
Copy link
Member

I think that "prime256v1" is unique to OpenSSL, so I'm OK with leaving it out. (Or including it, whichever).

the secpXXXr1 and nistPXXX variants should be added to the static table, though. They're in a big, official-looking document of the Win10 supported curves by name and OID.

@vcsjones
Copy link
Member

They're in a big, official-looking document of the Win10 supported curves by name and OID.

Is this document public? If so I can go through it and backfill the lookup table with whatever is missing unless one of y'all wanted to this.

@bartonjs
Copy link
Member

https://docs.microsoft.com/en-us/windows/win32/seccng/cng-named-elliptic-curves

But I think we only need to do special work for the "original 3" curves; after those everything seems to have only ever gotten one name, so it'll either map (and work) or is unknown to the receiving OS (and won't).

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security enhancement Product code improvement that does NOT require public API changes/additions untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants