-
Notifications
You must be signed in to change notification settings - Fork 119
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
Add support for POINT_CONVERSION_HYBRID #1936
Conversation
f3872e8
to
8721a2c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1936 +/- ##
=======================================
Coverage ? 78.74%
=======================================
Files ? 590
Lines ? 101428
Branches ? 14383
=======================================
Hits ? 79867
Misses ? 20924
Partials ? 637 ☔ View full report in Codecov by Sentry. |
8721a2c
to
96df068
Compare
crypto/fipsmodule/ec/oct.c
Outdated
@@ -110,6 +112,11 @@ size_t ec_point_to_bytes(const EC_GROUP *group, const EC_AFFINE *point, | |||
uint8_t y_buf[EC_MAX_BYTES]; | |||
ec_felem_to_bytes(group, y_buf, &field_len, &point->Y); | |||
buf[0] = form + (y_buf[field_len - 1] & 1); | |||
if (form == POINT_CONVERSION_HYBRID) { | |||
// |POINT_CONVERSION_HYBRID| specifies y's solution of the quadratic | |||
// equation, but also encodes the actual point along with it. |
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.
// equation, but also encodes the actual point along with it. | |
// equation, but also encodes the y coordinate along with it. |
crypto/fipsmodule/ec/oct.c
Outdated
if (len != 1 + 2 * field_len || in[0] != POINT_CONVERSION_UNCOMPRESSED) { | ||
// |POINT_CONVERSION_HYBRID| has the solution of y encoded in the first byte | ||
// as well. | ||
if (len != 1 + 2 * field_len || (in[0] != POINT_CONVERSION_UNCOMPRESSED && |
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.
why do we handle the hybrid case in function called ec_point_from_uncompressed
?
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.
When we ignore the first byte (which reveals the encoding format), uncompressed form is essentially identical to hybrid. It seemed like a bit redundant to write another ec_point_from_hybrid
function with uncompressed
parsing logic.
I could rename this function for a bit more clarity if needed? I was also considering adding a check in the hybrid decoding to see if y
actually had the same solution that the first byte indicated, is that something we should do?
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.
I'd prefer having a separate ec_point_from_hybrid
function, it's redundant but simpler.
I was also considering adding a check in the hybrid decoding to see if y actually had the same solution that the first byte indicated, is that something we should do?
That's a great question. Since we are doing this for compatibility with OpenSSL I guess you should see if they are checking the point consistency?
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.
Sure, I did a quick look into what OpenSSL was doing. They have a check for consistency that goes both ways.
Our's will look slightly different since we save the date in EC_FELEM
, but I'll figure something out here.
crypto/fipsmodule/ec/oct.c
Outdated
// |POINT_CONVERSION_HYBRID| has the solution of y encoded in the first byte | ||
// as well. | ||
if (len != 1 + 2 * field_len || (in[0] != POINT_CONVERSION_UNCOMPRESSED && | ||
(in[0] & ~1u) != POINT_CONVERSION_HYBRID)) { |
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.
please explain the magic & ~1u
?
96df068
to
e9012fa
Compare
0bb6c2f
to
1f12d24
Compare
## What's Changed * 800-131Ar1: length of the key-derivation key shall be at least 112 bits. by @skmcgrail in #1924 * Marshalling/Unmarshalling DH public keys by @justsmth in #1916 * Also prune SSM documents from ec2-test-framework by @samuel40791765 in #1925 * Use illegal_parameter instead of decode_error for invalid key shares by @justsmth in #1923 * Add null check in dh testing by @torben-hansen in #1937 * DH paramgen callback by @justsmth in #1928 * Upstream merge 2024 10 17 by @torben-hansen in #1934 * Remove old Intel CPU types by @justsmth in #1942 * Remove retries on PCT failure in EC and RSA key generation. by @nebeid in #1938 * Add p4p, bump up time by @justsmth in #1943 * PQ README by @jakemas in #1932 * bump mysql CI to 9.1.0 by @justsmth in #1939 * HKDF, HKDF_expand, and PBKDF Truncated SHA2-512 by @skmcgrail in #1946 * Missing functionality + Adding Nmap to our CI by @smittals2 in #1915 * Fix FIPS.md typo by @justsmth in #1950 * Support encode or decode ∞ like OpenSSL by @samuel40791765 in #1930 * Expand support for EVP_PKEY_HMAC by @justsmth in #1933 * Add PKCS7-internal BIO_f_cipher by @WillChilds-Klein in #1836 * Add PKCS7-internal BIO_f_md by @WillChilds-Klein in #1886 * Ruby Support - DSA custom md by @justsmth in #1953 * Add support for POINT_CONVERSION_HYBRID by @samuel40791765 in #1936 * Fixes for Coverity Alerts by @smittals2 in #1960 * Also test w/ gcc 4.8 by @justsmth in #1962 * Actually add support for SSL_get_server/peer_tmp_key by @samuel40791765 in #1945 * Coverity Fix Null Check by @smittals2 in #1965 * ML-KEM keygen Pairwise Consistency Test by @dkostic in #1964 * EDDSA PCT by @torben-hansen in #1968 * Expose AES_cfb1_encrypt and AES_cfb8_encrypt by @skmcgrail in #1967 **Full Changelog**: v1.37.0...v1.38.0 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.
Issues:
Resolves
CryptoAlg-2697
Description of changes:
This is the first time it has come up, but Ruby depends on the
POINT_CONVERSION_HYBRID
format in its tests and expects the underlying libcrypto to support it.There's not too much a difference between
POINT_CONVERSION_HYBRID
andPOINT_CONVERSION_UNCOMPRESSED
except that the first bit contains information about the quadratic equation y is at. We have the building blocks to support and can get this working with some tweaking around.Call-outs:
POINT_CONVERSION_HYBRID
works very similar toPOINT_CONVERSION_UNCOMPRESSED
so we're reusing a lot of code. Most of this change is working with the first bytePOINT_CONVERSION_HYBRID
does differently and point the encoding to the right places for parsing.Testing:
Testing parsing from and back with
POINT_CONVERSION_HYBRID
. What we're parsing to and from hybrid doesn't really matter here, they're already covered by other code paths.Test vectors were generated from our test cases and ran against OpenSSL with this brief test program:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.