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

Backport 2.28: Doc: Add note on special use of A in ecp group structure #8046

Merged
merged 2 commits into from
Aug 10, 2023

Conversation

ivq
Copy link
Contributor

@ivq ivq commented Aug 9, 2023

Description

Backport of #6999.
The changes made on #6999 are:

  1. mbedtls_ecp_group_a_is_minus_3(): 2.28 does not have MBEDTLS_PRIVATE.
  2. Added macro inline in ecp.h.

PR checklist

  • changelog provided
  • backport not required as this is a backport
  • tests not required

@tom-cosgrove-arm
Copy link
Contributor

Why is the build failing on Windows?

[2023-08-09T14:49:57.888Z] "c:\windows\workspace\mbed-tls-pr-head_PR-8046-head\src\ALL_BUILD.vcxproj" (default target) (1) ->

[2023-08-09T14:49:57.888Z] "C:\Windows\workspace\mbed-tls-pr-head_PR-8046-head\src\programs\x509\cert_req.vcxproj" (default target) (10) ->

[2023-08-09T14:49:57.888Z] (ClCompile target) -> 

[2023-08-09T14:49:57.888Z]   C:\Windows\workspace\mbed-tls-pr-head_PR-8046-head\src\include\mbedtls/ecp.h(1031): error C2054: expected '(' to follow 'inline' [C:\Windows\workspace\mbed-tls-pr-head_PR-8046-head\src\programs\x509\cert_req.vcxproj]

[2023-08-09T14:49:57.888Z]   C:\Windows\workspace\mbed-tls-pr-head_PR-8046-head\src\include\mbedtls/ecp.h(1032): error C2085: 'mbedtls_ecp_group_a_is_minus_3' : not in formal parameter list [C:\Windows\workspace\mbed-tls-pr-head_PR-8046-head\src\programs\x509\cert_req.vcxproj]

[2023-08-09T14:49:57.888Z]   C:\Windows\workspace\mbed-tls-pr-head_PR-8046-head\src\include\mbedtls/ecp.h(1032): error C2143: syntax error : missing ';' before '{' [C:\Windows\workspace\mbed-tls-pr-head_PR-8046-head\src\programs\x509\cert_req.vcxproj]

@tom-cosgrove-arm
Copy link
Contributor

Ah: some versions of MSVC don't support inline!!! https://stackoverflow.com/a/24736425/7761803

@tom-cosgrove-arm
Copy link
Contributor

tom-cosgrove-arm commented Aug 9, 2023

This is done by mbedtls/build_info.h in development, but library/common.h in 2.28 - looking at other files in include/ in 2.28, might be easiest just to add the following to ecp.h

#if (defined(__ARMCC_VERSION) || defined(_MSC_VER)) && \
    !defined(inline) && !defined(__cplusplus)
#define inline __inline
#endif

@ivq
Copy link
Contributor Author

ivq commented Aug 10, 2023

This is done by mbedtls/build_info.h in development, but library/common.h in 2.28 - looking at other files in include/ in 2.28, might be easiest just to add the following to ecp.h

#if (defined(__ARMCC_VERSION) || defined(_MSC_VER)) && \
    !defined(inline) && !defined(__cplusplus)
#define inline __inline
#endif

Thanks. Added

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces priority-medium Medium priority - this can be reviewed as time permits size-xs Estimated task size: extra small (a few hours at most) labels Aug 10, 2023
@tom-cosgrove-arm
Copy link
Contributor

Why is there no change to ecp.c? I would expect something like

diff --git a/library/ecp.c b/library/ecp.c
index 2d80b6f33..e2ce197d2 100644
--- a/library/ecp.c
+++ b/library/ecp.c
@@ -1528,7 +1528,7 @@ static int ecp_double_jac(const mbedtls_ecp_group *grp, mbedtls_ecp_point *R,
     mbedtls_mpi_init(&M); mbedtls_mpi_init(&S); mbedtls_mpi_init(&T); mbedtls_mpi_init(&U);
 
     /* Special case for A = -3 */
-    if (grp->A.p == NULL) {
+    if (mbedtls_ecp_group_a_is_minus_3(grp)) {
         /* M = 3(X + Z^2)(X - Z^2) */
         MBEDTLS_MPI_CHK(mbedtls_mpi_mul_mod(grp, &S,  &P->Z,  &P->Z));
         MBEDTLS_MPI_CHK(mbedtls_mpi_add_mod(grp, &T,  &P->X,  &S));
@@ -2806,7 +2806,7 @@ static int ecp_check_pubkey_sw(const mbedtls_ecp_group *grp, const mbedtls_ecp_p
     MBEDTLS_MPI_CHK(mbedtls_mpi_mul_mod(grp, &RHS, &pt->X,   &pt->X));
 
     /* Special case for A = -3 */
-    if (grp->A.p == NULL) {
+    if (mbedtls_ecp_group_a_is_minus_3(grp)) {
         MBEDTLS_MPI_CHK(mbedtls_mpi_sub_int(&RHS, &RHS, 3));  MOD_SUB(RHS);
     } else {
         MBEDTLS_MPI_CHK(mbedtls_mpi_add_mod(grp, &RHS, &RHS, &grp->A));

@gilles-peskine-arm
Copy link
Contributor

@tom-cosgrove-arm
Copy link
Contributor

@tom-cosgrove-arm to minimize risk

Okay, that's fine then

Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@tom-cosgrove-arm tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Aug 10, 2023
@mpg mpg added this pull request to the merge queue Aug 10, 2023
Merged via the queue into Mbed-TLS:mbedtls-2.28 with commit a35283c Aug 10, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces priority-medium Medium priority - this can be reviewed as time permits size-xs Estimated task size: extra small (a few hours at most)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants