Prefer using non-deprecated EC OSSL APIs where possible#127190
Prefer using non-deprecated EC OSSL APIs where possible#127190PranavSenthilnathan wants to merge 16 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security |
There was a problem hiding this comment.
Pull request overview
This PR introduces new OpenSSL 3.0+ EVP_PKEY-based EC key generation/import paths (to avoid deprecated EC_KEY APIs where possible), with managed code updated to prefer these paths and fall back to legacy behavior when needed.
Changes:
- Add native CryptoNative exports to generate/import EC keys via EVP_PKEY (named curves and explicit parameters).
- Update managed ECOpenSsl/ECDH code to use the new EVP_PKEY paths first, with legacy EC_KEY fallback.
- Extend the OpenSSL shim to light up additional OpenSSL 3.0 param-building/keygen APIs.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/libs/System.Security.Cryptography.Native/pal_ecc_import_export.h | Adds new native API declarations for EVP_PKEY EC key generation/import. |
| src/native/libs/System.Security.Cryptography.Native/pal_ecc_import_export.c | Implements EVP_PKEY-based EC keygen and fromdata import (named + explicit). |
| src/native/libs/System.Security.Cryptography.Native/opensslshim.h | Lights up OpenSSL 3.0 param_build and related functions used by new native code. |
| src/native/libs/System.Security.Cryptography.Native/entrypoints.c | Exposes the new native functions via the CryptoNative entrypoint table. |
| src/libraries/Common/src/System/Security/Cryptography/ECOpenSsl.cs | Prefers EVP_PKEY EC keygen/import for OpenSSL 3.0 with fallback to EC_KEY. |
| src/libraries/Common/src/System/Security/Cryptography/ECDiffieHellmanOpenSslPublicKey.cs | Stores/uses EVP_PKEY handles directly instead of wrapping EC_KEY. |
| src/libraries/Common/src/System/Security/Cryptography/ECDiffieHellmanOpenSsl.Derive.cs | Uses EVP_PKEY curve-name detection and imports via new ECOpenSsl helpers. |
| src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EcDsa.ImportExport.cs | Adds P/Invoke wrappers for the new CryptoNative EVP_PKEY EC APIs. |
| internal static SafeEvpPKeyHandle GenerateECKey(ECCurve curve, out int keySize) | ||
| { | ||
| return ImportECKeyCore(new ECOpenSsl(curve), out keySize); | ||
| if (curve.IsNamed) | ||
| { | ||
| string oid = !string.IsNullOrEmpty(curve.Oid.Value) ? curve.Oid.Value : curve.Oid.FriendlyName!; | ||
|
|
||
| SafeEvpPKeyHandle? pkey = Interop.Crypto.EvpPKeyGenerateByEcKeyOid(oid); |
There was a problem hiding this comment.
GenerateECKey(ECCurve curve, out int keySize) uses curve.Oid.Value / curve.Oid.FriendlyName! without calling curve.Validate() first. Since ECCurve is a struct, callers can pass a default/invalid named curve (e.g., CurveType = Named with Oid == null), which would currently throw a NullReferenceException instead of the expected crypto/PlatformNotSupported exception. Consider calling curve.Validate() up front (matching ECOpenSsl.GenerateKey) before trying the EVP_PKEY path/fallback.
| internal static SafeEvpPKeyHandle GenerateECKey(ECCurve curve, out int keySize) | ||
| { | ||
| return ImportECKeyCore(new ECOpenSsl(curve), out keySize); | ||
| if (curve.IsNamed) | ||
| { | ||
| string oid = !string.IsNullOrEmpty(curve.Oid.Value) ? curve.Oid.Value : curve.Oid.FriendlyName!; | ||
|
|
||
| SafeEvpPKeyHandle? pkey = Interop.Crypto.EvpPKeyGenerateByEcKeyOid(oid); | ||
|
|
||
| if (pkey is not null) | ||
| { | ||
| keySize = Interop.Crypto.EvpPKeyGetEcFieldDegree(pkey); | ||
| return pkey; | ||
| } | ||
| } | ||
| else if (curve.IsPrime || curve.IsCharacteristic2) | ||
| { | ||
| byte[] pField = curve.IsPrime ? curve.Prime! : curve.Polynomial!; | ||
|
|
There was a problem hiding this comment.
GenerateECKey(ECCurve curve, ...) now reads explicit-curve fields (Prime/Polynomial/A/B/G/Order/Cofactor) without validating the input first. This can throw NullReferenceException for malformed curves and bypass the normal curve.Validate() error behavior. Consider calling curve.Validate() at the start (matching ECOpenSsl.GenerateKey) before branching on curve.IsNamed/IsPrime/....
| REQUIRED_FUNCTION(EC_GROUP_get0_seed) \ | ||
| REQUIRED_FUNCTION(EC_GROUP_get_cofactor) \ | ||
| REQUIRED_FUNCTION(EC_GROUP_get_curve_GFp) \ | ||
| REQUIRED_FUNCTION(EC_GROUP_get_curve) \ |
There was a problem hiding this comment.
The shim marks EC_GROUP_get_curve as a REQUIRED function, but this symbol is not declared in osslcompat_30.h and is not available on OpenSSL < 3.0. This will break portable builds (compile-time) and/or runtime loading on systems with OpenSSL 1.x. Consider making this a LIGHTUP function and only calling it under NEED_OPENSSL_3_0, with the existing EC_GROUP_get_curve_GFp/GF2m fallback for older versions.
| REQUIRED_FUNCTION(EC_GROUP_get_curve) \ | |
| LIGHTUP_FUNCTION(EC_GROUP_get_curve) \ |
There was a problem hiding this comment.
EC_GROUP_get_curve is available in 1.1.1.
| REQUIRED_FUNCTION(EC_POINT_get_affine_coordinates_GFp) \ | ||
| REQUIRED_FUNCTION(EC_POINT_get_affine_coordinates) \ | ||
| REQUIRED_FUNCTION(EC_POINT_mul) \ | ||
| REQUIRED_FUNCTION(EC_POINT_new) \ | ||
| REQUIRED_FUNCTION(EC_POINT_point2oct) \ |
There was a problem hiding this comment.
The shim adds the generic EC_POINT_get_affine_coordinates / EC_POINT_set_affine_coordinates / EC_POINT_point2oct as REQUIRED functions. These APIs are OpenSSL 3.x additions and will prevent loading against OpenSSL 1.x in distro-agnostic builds. These should be LIGHTUP (or wrapped) and only used when NEED_OPENSSL_3_0 is in effect, with GFp/GF2m alternatives otherwise.
There was a problem hiding this comment.
These are available in 1.1.1.
| #endif | ||
|
|
||
| return 1; | ||
| return EC_POINT_get_affine_coordinates(group, p, x, y, NULL) ? 1 : 0; |
There was a problem hiding this comment.
EcPointGetAffineCoordinates now unconditionally calls EC_POINT_get_affine_coordinates, which is not available on OpenSSL < 3.0. This will break builds targeting OpenSSL 1.x. Please keep the previous GFp/GF2m branching for older OpenSSL, and only use EC_POINT_get_affine_coordinates when building/running with OpenSSL 3 (NEED_OPENSSL_3_0 + API_EXISTS).
| return EC_POINT_get_affine_coordinates(group, p, x, y, NULL) ? 1 : 0; | |
| #ifdef NEED_OPENSSL_3_0 | |
| if (API_EXISTS(EC_POINT_get_affine_coordinates)) | |
| return EC_POINT_get_affine_coordinates(group, p, x, y, NULL) ? 1 : 0; | |
| #endif | |
| const EC_METHOD* method = EC_GROUP_method_of(group); | |
| if (method == NULL) | |
| return 0; | |
| switch (MethodToCurveType(method)) | |
| { | |
| case PrimeMontgomery: | |
| case PrimeShortWeierstrass: | |
| return EC_POINT_get_affine_coordinates_GFp(group, p, x, y, NULL) ? 1 : 0; | |
| #if HAVE_OPENSSL_EC2M | |
| case Characteristic2: | |
| if (API_EXISTS(EC_POINT_get_affine_coordinates_GF2m)) | |
| return EC_POINT_get_affine_coordinates_GF2m(group, p, x, y, NULL) ? 1 : 0; | |
| return 0; | |
| #endif | |
| default: | |
| return 0; | |
| } |
There was a problem hiding this comment.
They're in OSSL 1.1.1.
| if (!EC_GROUP_get_curve(group, pBn, aBn, bBn, NULL)) | ||
| goto error; |
There was a problem hiding this comment.
CryptoNative_GetECCurveParameters switched to EC_GROUP_get_curve, which is an OpenSSL 3.x API and isn’t declared in osslcompat_30.h. This will fail to compile when building against OpenSSL < 3.0. Please keep the existing EC_GROUP_get_curve_GFp/GF2m fallback for older OpenSSL, and only call EC_GROUP_get_curve when OpenSSL 3 APIs are available.
There was a problem hiding this comment.
They're in OSSL 1.1.1.
| if (!EC_GROUP_set_curve(group, pBn, aBn, bBn, NULL)) | ||
| goto error; |
There was a problem hiding this comment.
CryptoNative_EcKeyCreateByExplicitParameters now calls EC_GROUP_set_curve / EC_POINT_set_affine_coordinates, which are OpenSSL 3.x APIs. Since this function is used for OpenSSL 1.x builds too, it needs to retain the GFp/GF2m variants (or otherwise guard these calls under NEED_OPENSSL_3_0 with fallbacks).
There was a problem hiding this comment.
They're in OSSL 1.1.1.
| int nid = 0; | ||
| if (CryptoNative_EvpPKeyGetEcGroupNid(pkey, &nid) && nid != NID_undef) | ||
| { | ||
| EC_GROUP* group = EC_GROUP_new_by_curve_name(nid); | ||
| if (group) | ||
| { | ||
| isChar2 = (EC_GROUP_get_field_type(group) == NID_X9_62_characteristic_two_field); | ||
| EC_GROUP_free(group); | ||
| } | ||
| } |
There was a problem hiding this comment.
CryptoNative_EvpPKeyGetEcFieldDegree only falls back to reading OSSL_PKEY_PARAM_EC_FIELD_TYPE when CryptoNative_EvpPKeyGetEcGroupNid fails. If the NID is available but EC_GROUP_new_by_curve_name(nid) fails (returns NULL), isChar2 stays at the default (prime-field) and the degree calculation for binary curves will be wrong. Consider adding a fallback to query OSSL_PKEY_PARAM_EC_FIELD_TYPE when group could not be created as well.
There was a problem hiding this comment.
If nid is available then group creation should not fail.
| if (!API_EXISTS(EVP_PKEY_fromdata) || | ||
| !API_EXISTS(EVP_PKEY_fromdata_init) || | ||
| !API_EXISTS(EVP_PKEY_CTX_new_from_name) || | ||
| !API_EXISTS(OSSL_PARAM_BLD_new)) |
There was a problem hiding this comment.
In the portable (FEATURE_DISTRO_AGNOSTIC_SSL) build, this function gates on OSSL_PARAM_BLD_new but then unconditionally calls other OSSL_PARAM_BLD_* and OSSL_PARAM_free symbols. Since these are LIGHTUP functions in the shim, the guard should also verify all required OSSL_PARAM_BLD_push_*, OSSL_PARAM_BLD_to_param, OSSL_PARAM_BLD_free, and OSSL_PARAM_free APIs are present to avoid calling through a missing function pointer at runtime.
| !API_EXISTS(OSSL_PARAM_BLD_new)) | |
| !API_EXISTS(OSSL_PARAM_BLD_new) || | |
| !API_EXISTS(OSSL_PARAM_BLD_push_utf8_string) || | |
| !API_EXISTS(OSSL_PARAM_BLD_push_octet_string) || | |
| !API_EXISTS(OSSL_PARAM_BLD_to_param) || | |
| !API_EXISTS(OSSL_PARAM_BLD_free) || | |
| !API_EXISTS(OSSL_PARAM_free)) |
There was a problem hiding this comment.
_new should cover all the others too since they were shipped together.
| !API_EXISTS(EVP_PKEY_CTX_new_from_name) || | ||
| !API_EXISTS(EVP_PKEY_CTX_new_from_pkey) || | ||
| !API_EXISTS(EVP_PKEY_generate) || | ||
| !API_EXISTS(OSSL_PARAM_BLD_new)) |
There was a problem hiding this comment.
Same as above: the FEATURE_DISTRO_AGNOSTIC_SSL guard only checks OSSL_PARAM_BLD_new, but the implementation relies on multiple OSSL_PARAM_BLD_* functions and OSSL_PARAM_free. Please extend the API_EXISTS checks to cover every LIGHTUP symbol used in this code path so portable builds running on older OpenSSL versions don’t crash when a symbol is absent.
| !API_EXISTS(OSSL_PARAM_BLD_new)) | |
| !API_EXISTS(OSSL_PARAM_BLD_new) || | |
| !API_EXISTS(OSSL_PARAM_BLD_push_BN) || | |
| !API_EXISTS(OSSL_PARAM_BLD_push_octet_string) || | |
| !API_EXISTS(OSSL_PARAM_BLD_to_param) || | |
| !API_EXISTS(OSSL_PARAM_BLD_free) || | |
| !API_EXISTS(OSSL_PARAM_free)) |
There was a problem hiding this comment.
_new should cover all the others too since they were shipped together.
| bool? explicitEncoding = Interop.Crypto.EvpPKeyEcHasExplicitEncoding(_key.Value); | ||
| if (explicitEncoding.HasValue) | ||
| { | ||
| // This may happen when EVP_PKEY was created by provider and getting EC_KEY is not possible. | ||
| thisIsNamed = Interop.Crypto.EvpPKeyHasCurveName(_key.Value); | ||
| thisIsNamed = !explicitEncoding.Value; | ||
| } |
There was a problem hiding this comment.
EvpPKeyEcHasExplicitEncoding returns false both for named-curve encoding and when the encoding cannot be read (native returns 0 for either case). The current logic treats false as "named" (thisIsNamed = !explicitEncoding.Value) and similarly for otherIsNamed, which can misclassify explicit keys from providers that don't expose OSSL_PKEY_PARAM_EC_ENCODING. Consider disambiguating by also checking whether the key has a group name (e.g., Interop.Crypto.EvpPKeyHasCurveName) when the encoding result is false, or change the native API to return a distinct value for "unavailable/unknown".
There was a problem hiding this comment.
Not being able to read the encoding means it's named.
bartonjs
left a comment
There was a problem hiding this comment.
I still have most of the native code to review, but here is the feedback so far.
| byte[]? qx, int qxLength, | ||
| byte[]? qy, int qyLength, | ||
| byte[]? d, int dLength) |
There was a problem hiding this comment.
For the internal calls, do we need both qx and qxLength? Can this just be ReadOnlySpan so they're tracked together? (Or, if the array-ness is useful. does the caller always pass array.Length? If so, the internal call can take that away)
| throw new PlatformNotSupportedException(SR.Format(SR.Cryptography_CurveNotSupported, oid)); | ||
| } | ||
|
|
||
| if (rc != 1 || pkey.IsInvalid) |
There was a problem hiding this comment.
Why does it make sense that we'd get not-success, but just clear the error queue and return null? If this code is right, it probably needs some comments.
| SafeEvpPKeyHandle pkey; | ||
| int rc = EvpPKeyCreateByEcKeyParameters(out pkey, oid, qx, qxLength, qy, qyLength, d, dLength); | ||
|
|
||
| if (rc == -1) |
There was a problem hiding this comment.
In our shim, -1 usually means "the caller (this code) did something nonsensical". Communicating a "normal" error should use something else, like 2. (If this is exactly matching what we did with EC_KEY, then, well... it could be better)
| } | ||
|
|
||
| [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_EvpPKeyGenerateByEcKeyOid", StringMarshalling = StringMarshalling.Utf8)] | ||
| private static partial int EvpPKeyGenerateByEcKeyOid( |
There was a problem hiding this comment.
maybe you mean by EcCurveOid? "EcKey", especially in this file, means EC_KEY
| { | ||
| thisIsNamed = Interop.Crypto.EcKeyHasCurveName(ecKey); | ||
| // Pre-3.0 fallback: check via EC_KEY. | ||
| using (SafeEcKeyHandle ecKey = Interop.Crypto.EvpPkeyGetEcKey(_key.Value)) |
There was a problem hiding this comment.
Is this always what the fallback code will look like? Does it exist in more than one place? If yes+yes, move it into EvpPKeyEcHasExplicitEncoding and get rid of the nullable bool.
| [InlineData(ECDSA_P256_OID_VALUE)] | ||
| [InlineData(ECDSA_P384_OID_VALUE)] | ||
| [InlineData(ECDSA_P521_OID_VALUE)] | ||
| public void EcKeyAndEvpPKeySignVerifyCrossCompatible(string oid) |
There was a problem hiding this comment.
I'm not really sure about any of the "compare EC_KEY with EVP_PKEY" tests. What we really want is "does the EC_KEY ctor work?", given that we already have "do the non-interop paths work" (and maybe we should have more "here's an EVP_PKEY" tests, but I don't know that we care to smash them off each other)
That feels like
- Named, explicit-prime, explicit-char2 exports work as expected.
- ECDH
- From-EC_KEY works as the left side and right side of a DeriveKey
- From-EC_KEY(public only) fails as the left side of DeriveKey
- ECDSA
- From-EC_KEY(public only) throws something sensible from Sign
- From-EC_KEY(public only) verifies a known signature
- From-EC_KEY(public private) signs a thing that platform-import-this-key can verify.
Maybe what I've seen is just those 8 tests, but it feels like more than that.
This would, of course, be easier if the ECDH/ECDSA tests were written more like the MLDSA tests... we'd just extend the KAT-verifying base class (which also would check things like signing with a public key) to do a mode where it imports the key via EC_KEY instead of using the platform public "figure it out, so probably EVP_PKEY" approach. (That's not "go rewrite the tests for this", but rather "a musing out loud from a hindsight perspective" thing)
| LIGHTUP_FUNCTION(EVP_PKEY_CTX_new_from_name) \ | ||
| LIGHTUP_FUNCTION(EVP_PKEY_CTX_new_from_pkey) \ | ||
| REQUIRED_FUNCTION(EVP_PKEY_new_raw_private_key) \ | ||
| REQUIRED_FUNCTION(EVP_PKEY_new_raw_public_key) \ |
There was a problem hiding this comment.
Is this a bad merge from X25519?
| #ifdef NEED_OPENSSL_3_0 | ||
|
|
||
| int nid = OBJ_txt2nid(oid); | ||
| if (!nid) |
There was a problem hiding this comment.
Other code compares != NID_undef. This should for consistency.
| return -1; | ||
| } | ||
|
|
||
| const char* groupName = OBJ_nid2sn(nid); |
There was a problem hiding this comment.
Can you cite something from the OSSL project that says to use sn here? It looks like their CMS uses OBJ_obj2txt which prefers ln over sn.
Really it's anything that turns the SPKI form into a key object, since that form will receive the curve by OID.
| } | ||
| #endif | ||
|
|
||
| #ifdef NEED_OPENSSL_3_0 |
There was a problem hiding this comment.
It feels weird to me that we're not using the same function for 3+ and 1.1.1 to generate an EVP_PKEY from a curve OID... even if the 1.1.1 version makes an EC_KEY and wraps it in EVP_PKEY before returning. (Ideally we want to insulate the managed code, as much as possible, from knowing the difference)
Adds new native entry points that use the modern OpenSSL 3.0
EVP_PKEYparameter and keygen APIs for EC keygeneration, import, and export, avoiding the deprecated
EC_KEYAPIs where possible.On pre-3.0 OpenSSL, the existing
EC_KEYcode paths are preserved as fallbacks. All new lightup APIs are guarded withAPI_EXISTSunderFEATURE_DISTRO_AGNOSTIC_SSL.Unified EC APIs (EC_GROUP_get/set_curve, EC_POINT_get/set_affine_coordinates) are promoted to REQUIRED (available
since OpenSSL 1.1.1, which is the minimum supported version).