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

Add EC_POINT_hex2point #7191

Merged
merged 2 commits into from
Jun 1, 2024
Merged

Add EC_POINT_hex2point #7191

merged 2 commits into from
Jun 1, 2024

Conversation

kojo1
Copy link
Contributor

@kojo1 kojo1 commented Jan 30, 2024

Description

Add EC_POINT_hex2point
Enable HAVE_COMP_KEY for Compressed Key

Fixes zd #17090

Testing

compressed and uncompressed in test_wolfSSL_EC_POINT, api.c

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@kojo1 kojo1 force-pushed the ecpoint-h2p branch 2 times, most recently from 2dd78b3 to 890f46f Compare January 30, 2024 22:29
src/pk.c Outdated Show resolved Hide resolved
src/pk.c Outdated Show resolved Hide resolved
@kojo1 kojo1 force-pushed the ecpoint-h2p branch 2 times, most recently from 6ac25c2 to 1259e4a Compare January 31, 2024 04:39
@bandi13
Copy link
Contributor

bandi13 commented Jan 31, 2024

retest this please..

@ejohnstown
Copy link
Contributor

I ran this locally with Valgrind and the "CI/krb5/Build wolfSSL" test failure is legit.

  ./configure --enable-opensslall --disable-shared && make clean && make
  valgrind --leak-test=full ./tests/unit.test

I got the same leak report for function wolfSSL_EC_POINT_hex2point.

I looked into the "CI/openwrt/Compile container (21.02-SNAPSHOT)" failure. It isn't able to download the openwrt sources from the server.

@bandi13
Copy link
Contributor

bandi13 commented Jan 31, 2024

Looks like there's actually an issue here with missing macro definitions. The openwrt / Compile container (21.02-SNAPSHOT) issue has already been resolved (see PR#7193)

@kojo1 kojo1 force-pushed the ecpoint-h2p branch 8 times, most recently from a80a0c5 to d7e7168 Compare February 1, 2024 08:16
@kojo1
Copy link
Contributor Author

kojo1 commented Feb 2, 2024

retest this please

src/pk.c Outdated
WOLFSSL_MSG("EEC_KEY_get_byte_size, XMALLOC");
goto err;
}
octGx[0] = 0x03;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use ECC_POINT_COMP_ODD?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/pk.c Outdated
int key_sz;
byte *octGx = NULL;

#define P_ALLOC 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding P_ALLOC here seems pointless. Just use 1 directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/pk.c Outdated
}
else if (hex[0] == '0' && (hex[1] == '2' || hex[1] == '3')) {
/* compressed mode */
key_sz = ((wolfSSL_EC_GROUP_get_degree(group) + 7) / 8);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend a cleanup to do key_sz = ((wolfSSL_EC_GROUP_get_degree(group) + 7) / 8); above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/pk.c Outdated
}

if (hex[0] == '0' && hex[1] == '4') { /* uncompressed mode */
str_sz = ((wolfSSL_EC_GROUP_get_degree(group) + 7) / 8) * 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

str_sz = key_sz * 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/pk.c Outdated

if (hex[0] == '0' && hex[1] == '4') { /* uncompressed mode */
str_sz = ((wolfSSL_EC_GROUP_get_degree(group) + 7) / 8) * 2;
strGx = (char *)XMALLOC(str_sz + 1, NULL, DYNAMIC_TYPE_ECC);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be easier to just use MAX_ECC_BYTES * 2 for the buffer vs using malloc without a heap hint. Its not really an ECC type, its temp buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/pk.c Outdated
else if (hex[0] == '0' && (hex[1] == '2' || hex[1] == '3')) {
/* compressed mode */
key_sz = ((wolfSSL_EC_GROUP_get_degree(group) + 7) / 8);
octGx = (byte *)XMALLOC(key_sz + 1, NULL, DYNAMIC_TYPE_ECC);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same use MAX_ECC_BYTES and only use a single buffer for either case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@kojo1
Copy link
Contributor Author

kojo1 commented May 22, 2024

retest this please

@dgarske dgarske self-requested a review May 22, 2024 21:47
src/pk.c Outdated
@@ -9788,12 +9787,12 @@ char* wolfSSL_EC_POINT_point2hex(const WOLFSSL_EC_GROUP* group,
* odd.
*/
hex[0] = mp_isodd((mp_int*)point->Y->internal) ?
ECC_POINT_COMP_ODD : ECC_POINT_COMP_EVEN;
0x03 : 0x02;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you switch to using the numeric directly to avoid having the #fndef HAVE_SELFTEST?

I really prefer the ECC_POINT_COMP_ODD, ECC_POINT_COMP_EVEN and ECC_POINT_UNCOMP values. If needed you could add some logic above to ensure these are available.

#ifdef HAVE_SELFTEST
    /* point compression types */
    #define ECC_POINT_COMP_EVEN 0x02
    #define ECC_POINT_COMP_ODD  0x03
    #define ECC_POINT_UNCOMP    0x04
#endif

@kojo1 kojo1 force-pushed the ecpoint-h2p branch 3 times, most recently from e760f12 to cd2818a Compare May 26, 2024 22:30
@dgarske dgarske assigned kojo1 and unassigned kojo1 and SparkiDev May 29, 2024
SparkiDev
SparkiDev previously approved these changes May 29, 2024
@dgarske
Copy link
Contributor

dgarske commented May 30, 2024

Retest this please

tests/api.c Show resolved Hide resolved
@dgarske dgarske removed their assignment May 30, 2024
@dgarske dgarske removed the request for review from SparkiDev May 30, 2024 22:42
@dgarske dgarske assigned dgarske and unassigned wolfSSL-Bot May 31, 2024
@kojo1
Copy link
Contributor Author

kojo1 commented Jun 1, 2024

retest this please

@kojo1 kojo1 force-pushed the ecpoint-h2p branch 3 times, most recently from 2791ba9 to 73b35f1 Compare June 1, 2024 02:07
@dgarske dgarske merged commit 3975af8 into wolfSSL:master Jun 1, 2024
103 checks passed
jefferyq2 pushed a commit to jefferyq2/wolfssl that referenced this pull request Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants