-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Prefer using non-deprecated EC OSSL APIs where possible #127190
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
base: main
Are you sure you want to change the base?
Changes from all commits
d64dad9
43775ae
6afa0f7
a619c89
6535dcc
3bdb9d7
556f5d5
883529f
d0b37ac
4cf0d13
49ec2ab
754e355
a21d366
d0c06d8
d7068b7
a214223
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,6 +96,119 @@ internal static SafeEcKeyHandle EcKeyCreateByExplicitCurve(ECCurve curve) | |
| return key; | ||
| } | ||
|
|
||
| [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_EvpPKeyCreateByEcKeyParameters", StringMarshalling = StringMarshalling.Utf8)] | ||
| private static partial int EvpPKeyCreateByEcKeyParameters( | ||
| out SafeEvpPKeyHandle pkey, | ||
| string oid, | ||
| byte[]? qx, int qxLength, | ||
| byte[]? qy, int qyLength, | ||
| byte[]? d, int dLength); | ||
|
|
||
| internal static SafeEvpPKeyHandle? EvpPKeyCreateByEcKeyParameters( | ||
| string oid, | ||
| byte[]? qx, int qxLength, | ||
| byte[]? qy, int qyLength, | ||
| byte[]? d, int dLength) | ||
| { | ||
| SafeEvpPKeyHandle pkey; | ||
| int rc = EvpPKeyCreateByEcKeyParameters(out pkey, oid, qx, qxLength, qy, qyLength, d, dLength); | ||
|
|
||
| if (rc == -1) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
| { | ||
| pkey.Dispose(); | ||
| ErrClearError(); | ||
| throw new PlatformNotSupportedException(SR.Format(SR.Cryptography_CurveNotSupported, oid)); | ||
| } | ||
|
|
||
| if (rc != 1 || pkey.IsInvalid) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| { | ||
| pkey.Dispose(); | ||
| ErrClearError(); | ||
| return null; | ||
| } | ||
|
|
||
| return pkey; | ||
| } | ||
|
|
||
| [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_EvpPKeyCreateByEcExplicitParameters")] | ||
| private static partial SafeEvpPKeyHandle CryptoNative_EvpPKeyCreateByEcExplicitParameters( | ||
| ECCurve.ECCurveType curveType, | ||
| byte[]? qx, int qxLength, | ||
| byte[]? qy, int qyLength, | ||
| byte[]? d, int dLength, | ||
| byte[] p, int pLength, | ||
| byte[] a, int aLength, | ||
| byte[] b, int bLength, | ||
| byte[] gx, int gxLength, | ||
| byte[] gy, int gyLength, | ||
| byte[] order, int orderLength, | ||
| byte[]? cofactor, int cofactorLength, | ||
| byte[]? seed, int seedLength); | ||
|
PranavSenthilnathan marked this conversation as resolved.
|
||
|
|
||
| internal static SafeEvpPKeyHandle? EvpPKeyCreateByEcExplicitParameters( | ||
| ECCurve.ECCurveType curveType, | ||
| byte[]? qx, int qxLength, | ||
| byte[]? qy, int qyLength, | ||
| byte[]? d, int dLength, | ||
| byte[] p, int pLength, | ||
| byte[] a, int aLength, | ||
| byte[] b, int bLength, | ||
| byte[] gx, int gxLength, | ||
| byte[] gy, int gyLength, | ||
| byte[] order, int orderLength, | ||
| byte[]? cofactor, int cofactorLength, | ||
| byte[]? seed, int seedLength) | ||
| { | ||
| SafeEvpPKeyHandle pkey = CryptoNative_EvpPKeyCreateByEcExplicitParameters( | ||
| curveType, | ||
| qx, qxLength, | ||
| qy, qyLength, | ||
| d, dLength, | ||
| p, pLength, | ||
| a, aLength, | ||
| b, bLength, | ||
| gx, gxLength, | ||
| gy, gyLength, | ||
| order, orderLength, | ||
| cofactor, cofactorLength, | ||
| seed, seedLength); | ||
|
|
||
| if (pkey.IsInvalid) | ||
| { | ||
| pkey.Dispose(); | ||
| ErrClearError(); | ||
| return null; | ||
| } | ||
|
|
||
| return pkey; | ||
| } | ||
|
|
||
| [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_EvpPKeyGenerateByEcKeyOid", StringMarshalling = StringMarshalling.Utf8)] | ||
| private static partial int EvpPKeyGenerateByEcKeyOid( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe you mean by EcCurveOid? "EcKey", especially in this file, means EC_KEY |
||
| out SafeEvpPKeyHandle pkey, | ||
| string oid); | ||
|
|
||
| internal static SafeEvpPKeyHandle? EvpPKeyGenerateByEcKeyOid(string oid) | ||
| { | ||
| int rc = EvpPKeyGenerateByEcKeyOid(out SafeEvpPKeyHandle pkey, oid); | ||
|
|
||
| if (rc == -1) | ||
| { | ||
| pkey.Dispose(); | ||
| ErrClearError(); | ||
| throw new PlatformNotSupportedException(SR.Format(SR.Cryptography_CurveNotSupported, oid)); | ||
| } | ||
|
|
||
| if (rc != 1 || pkey.IsInvalid) | ||
| { | ||
| pkey.Dispose(); | ||
| ErrClearError(); | ||
| return null; | ||
| } | ||
|
|
||
| return pkey; | ||
| } | ||
|
|
||
| [LibraryImport(Libraries.CryptoNative)] | ||
| private static partial int CryptoNative_EvpPKeyGetEcGroupNid(SafeEvpPKeyHandle pkey, out int nid); | ||
|
|
||
|
|
@@ -104,11 +217,12 @@ internal static bool EvpPKeyHasCurveName(SafeEvpPKeyHandle pkey) | |
| int rc = CryptoNative_EvpPKeyGetEcGroupNid(pkey, out int nidCurveName); | ||
| if (rc == 1) | ||
| { | ||
| // Key is invalid or doesn't have a curve | ||
| return (nidCurveName != Interop.Crypto.NID_undef); | ||
| return nidCurveName != Interop.Crypto.NID_undef; | ||
| } | ||
|
|
||
| throw Interop.Crypto.CreateOpenSslCryptographicException(); | ||
| // rc == 0 means the group name could not be retrieved | ||
| // (e.g., explicit curve or OpenSSL < 3.0). Treat as no curve name. | ||
| return false; | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -122,7 +236,35 @@ internal static bool EvpPKeyHasCurveName(SafeEvpPKeyHandle pkey) | |
| return nidCurveName != Interop.Crypto.NID_undef ? CurveNidToOidValue(nidCurveName) : null; | ||
| } | ||
|
|
||
| throw Interop.Crypto.CreateOpenSslCryptographicException(); | ||
| // rc == 0 means the group name could not be retrieved | ||
| // (e.g., explicit curve or OpenSSL < 3.0). | ||
| return null; | ||
| } | ||
|
|
||
| [LibraryImport(Libraries.CryptoNative)] | ||
| private static partial int CryptoNative_EvpPKeyEcHasExplicitEncoding(SafeEvpPKeyHandle pkey); | ||
|
|
||
| /// <summary> | ||
| /// Returns true if the key has explicit encoding, false if named (or encoding unavailable), | ||
| /// null if the API is unavailable (pre-3.0) and the caller should use an alternative method. | ||
| /// </summary> | ||
| internal static bool? EvpPKeyEcHasExplicitEncoding(SafeEvpPKeyHandle pkey) | ||
| { | ||
| int result = CryptoNative_EvpPKeyEcHasExplicitEncoding(pkey); | ||
| return result switch | ||
| { | ||
| 1 => true, | ||
| 0 => false, | ||
| _ => null, | ||
| }; | ||
| } | ||
|
|
||
| [LibraryImport(Libraries.CryptoNative)] | ||
| private static partial int CryptoNative_EvpPKeyGetEcFieldDegree(SafeEvpPKeyHandle pkey); | ||
|
|
||
| internal static int EvpPKeyGetEcFieldDegree(SafeEvpPKeyHandle pkey) | ||
| { | ||
| return CryptoNative_EvpPKeyGetEcFieldDegree(pkey); | ||
| } | ||
|
|
||
| [LibraryImport(Libraries.CryptoNative)] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,17 +89,19 @@ public override byte[] DeriveRawSecretAgreement(ECDiffieHellmanPublicKey otherPa | |
| Debug.Assert(_key is not null); // Callers should validate prior. | ||
|
|
||
| bool thisIsNamed; | ||
|
|
||
| using (SafeEcKeyHandle ecKey = Interop.Crypto.EvpPkeyGetEcKey(_key.Value)) | ||
| { | ||
| if (ecKey == null || ecKey.IsInvalid) | ||
| 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; | ||
| } | ||
|
Comment on lines
+93
to
97
|
||
| else | ||
| { | ||
| thisIsNamed = Interop.Crypto.EcKeyHasCurveName(ecKey); | ||
| // Pre-3.0 fallback: check via EC_KEY. | ||
| using (SafeEcKeyHandle ecKey = Interop.Crypto.EvpPkeyGetEcKey(_key.Value)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| { | ||
| thisIsNamed = Interop.Crypto.EcKeyHasCurveName(ecKey); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -118,7 +120,7 @@ public override byte[] DeriveRawSecretAgreement(ECDiffieHellmanPublicKey otherPa | |
| otherKey = new ECDiffieHellmanOpenSslPublicKey(otherParameters); | ||
| } | ||
|
|
||
| bool otherIsNamed = otherKey.HasCurveName; | ||
| bool otherIsNamed = !otherKey.HasExplicitEncoding; | ||
|
|
||
| // We need to always duplicate handle in case this operation is done by multiple threads and one of them disposes the handle | ||
| SafeEvpPKeyHandle? ourKey = _key.Value; | ||
|
|
@@ -143,10 +145,7 @@ public override byte[] DeriveRawSecretAgreement(ECDiffieHellmanPublicKey otherPa | |
| } | ||
| else if (otherIsNamed) | ||
| { | ||
| using (ECOpenSsl tmp = new ECOpenSsl(otherKey.ExportExplicitParameters())) | ||
| { | ||
| theirKey = tmp.CreateEvpPKeyHandle(); | ||
| } | ||
| theirKey = ECOpenSsl.ImportECKey(otherKey.ExportExplicitParameters(), out _); | ||
| } | ||
| else | ||
| { | ||
|
|
@@ -155,11 +154,8 @@ public override byte[] DeriveRawSecretAgreement(ECDiffieHellmanPublicKey otherPa | |
| // This is generally not expected to fail except: | ||
| // - when key can't be accessed but is available (i.e. TPM) | ||
| // - private key is actually missing | ||
| using (ECOpenSsl tmp = new ECOpenSsl(ExportExplicitParameters(true))) | ||
| { | ||
| ourKey = tmp.CreateEvpPKeyHandle(); | ||
| disposeOurKey = true; | ||
| } | ||
| ourKey = ECOpenSsl.ImportECKey(ExportExplicitParameters(true), out _); | ||
| disposeOurKey = true; | ||
| } | ||
| catch (CryptographicException) | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the
internalcalls, 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)