From bececeb0b9b02ca8b59ca3f4a3b5b9a3acb0cf40 Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Mon, 16 Jan 2023 16:16:49 +0000 Subject: [PATCH 1/6] ecp_curves: Hardcod Montgomery const for curve25519 This patch adds two embedded constants used by `ecp_use_curve25519()`. The method has been updated to read that into an mpi instead of calculating it on the fly. Signed-off-by: Minos Galanakis --- library/ecp_curves.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/library/ecp_curves.c b/library/ecp_curves.c index e62dcea65..3b517b751 100644 --- a/library/ecp_curves.c +++ b/library/ecp_curves.c @@ -4627,9 +4627,21 @@ static int ecp_mod_p256k1(mbedtls_mpi *); #if defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) /* Constants used by ecp_use_curve25519() */ static const mbedtls_mpi_sint curve25519_a24 = 0x01DB42; -static const unsigned char curve25519_part_of_n[] = { - 0x14, 0xDE, 0xF9, 0xDE, 0xA2, 0xF7, 0x9C, 0xD6, - 0x58, 0x12, 0x63, 0x1A, 0x5C, 0xF5, 0xD3, 0xED, + +/* P = 2^255 - 19 */ +static const mbedtls_mpi_uint curve25519_p[] = { + MBEDTLS_BYTES_TO_T_UINT_8(0xED, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF), + MBEDTLS_BYTES_TO_T_UINT_8(0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF), + MBEDTLS_BYTES_TO_T_UINT_8(0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF), + MBEDTLS_BYTES_TO_T_UINT_8(0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0X7F) +}; + +/* N = 2^252 + 27742317777372353535851937790883648493 */ +static const mbedtls_mpi_uint curve25519_n[] = { + MBEDTLS_BYTES_TO_T_UINT_8(0XED, 0XD3, 0XF5, 0X5C, 0X1A, 0X63, 0X12, 0X58), + MBEDTLS_BYTES_TO_T_UINT_8(0XD6, 0X9C, 0XF7, 0XA2, 0XDE, 0XF9, 0XDE, 0X14), + MBEDTLS_BYTES_TO_T_UINT_8(0X00, 0X00, 0X00, 0X00, 0x00, 0x00, 0x00, 0x00), + MBEDTLS_BYTES_TO_T_UINT_8(0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10) }; /* @@ -4642,16 +4654,11 @@ static int ecp_use_curve25519(mbedtls_ecp_group *grp) /* Actually ( A + 2 ) / 4 */ MBEDTLS_MPI_CHK(mbedtls_mpi_lset(&grp->A, curve25519_a24)); - /* P = 2^255 - 19 */ - MBEDTLS_MPI_CHK(mbedtls_mpi_lset(&grp->P, 1)); - MBEDTLS_MPI_CHK(mbedtls_mpi_shift_l(&grp->P, 255)); - MBEDTLS_MPI_CHK(mbedtls_mpi_sub_int(&grp->P, &grp->P, 19)); + ecp_mpi_load(&grp->P, curve25519_p, sizeof(curve25519_p)); + grp->pbits = mbedtls_mpi_bitlen(&grp->P); - /* N = 2^252 + 27742317777372353535851937790883648493 */ - MBEDTLS_MPI_CHK(mbedtls_mpi_read_binary(&grp->N, - curve25519_part_of_n, sizeof(curve25519_part_of_n))); - MBEDTLS_MPI_CHK(mbedtls_mpi_set_bit(&grp->N, 252, 1)); + ecp_mpi_load(&grp->N, curve25519_n, sizeof(curve25519_n)); /* Y intentionally not set, since we use x/z coordinates. * This is used as a marker to identify Montgomery curves! */ From 146fed98496a1a05dc181f1838eea58bc039f3f6 Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Mon, 16 Jan 2023 17:17:08 +0000 Subject: [PATCH 2/6] ecp_curves: Hardcode Montgomery const for curve448. This patch adds two embedded constants used by `ecp_use_curve448()`. The method has been updated to read that into an mpi instead of calculating it on the fly. Signed-off-by: Minos Galanakis --- library/ecp_curves.c | 45 ++++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/library/ecp_curves.c b/library/ecp_curves.c index 3b517b751..88c4c850a 100644 --- a/library/ecp_curves.c +++ b/library/ecp_curves.c @@ -4681,11 +4681,29 @@ cleanup: #if defined(MBEDTLS_ECP_DP_CURVE448_ENABLED) /* Constants used by ecp_use_curve448() */ static const mbedtls_mpi_sint curve448_a24 = 0x98AA; -static const unsigned char curve448_part_of_n[] = { - 0x83, 0x35, 0xDC, 0x16, 0x3B, 0xB1, 0x24, - 0xB6, 0x51, 0x29, 0xC9, 0x6F, 0xDE, 0x93, - 0x3D, 0x8D, 0x72, 0x3A, 0x70, 0xAA, 0xDC, - 0x87, 0x3D, 0x6D, 0x54, 0xA7, 0xBB, 0x0D, + +/* P = 2^448 - 2^224 - 1 */ +static const mbedtls_mpi_uint curve448_p[] = { + MBEDTLS_BYTES_TO_T_UINT_8(0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF), + MBEDTLS_BYTES_TO_T_UINT_8(0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF), + MBEDTLS_BYTES_TO_T_UINT_8(0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF), + MBEDTLS_BYTES_TO_T_UINT_8(0XFF, 0XFF, 0XFF, 0XFF, 0XFE, 0XFF, 0XFF, 0XFF), + MBEDTLS_BYTES_TO_T_UINT_8(0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF), + MBEDTLS_BYTES_TO_T_UINT_8(0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF), + MBEDTLS_BYTES_TO_T_UINT_8(0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF), + MBEDTLS_BYTES_TO_T_UINT_8(0X00, 0X00, 0X00, 0X00, 0X00, 0X00, 0X00, 0X00) +}; + +/* N = 2^446 - 13818066809895115352007386748515426880336692474882178609894547503885 */ +static const mbedtls_mpi_uint curve448_n[] = { + MBEDTLS_BYTES_TO_T_UINT_8(0XF3, 0X44, 0X58, 0XAB, 0X92, 0XC2, 0X78, 0X23), + MBEDTLS_BYTES_TO_T_UINT_8(0X55, 0X8F, 0XC5, 0X8D, 0X72, 0XC2, 0X6C, 0X21), + MBEDTLS_BYTES_TO_T_UINT_8(0X90, 0X36, 0XD6, 0XAE, 0X49, 0XDB, 0X4E, 0XC4), + MBEDTLS_BYTES_TO_T_UINT_8(0XE9, 0X23, 0XCA, 0X7C, 0XFF, 0XFF, 0XFF, 0XFF), + MBEDTLS_BYTES_TO_T_UINT_8(0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF), + MBEDTLS_BYTES_TO_T_UINT_8(0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF), + MBEDTLS_BYTES_TO_T_UINT_8(0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0XFF, 0X3F), + MBEDTLS_BYTES_TO_T_UINT_8(0X00, 0X00, 0X00, 0X00, 0X00, 0X00, 0X00, 0X00) }; /* @@ -4693,20 +4711,12 @@ static const unsigned char curve448_part_of_n[] = { */ static int ecp_use_curve448(mbedtls_ecp_group *grp) { - mbedtls_mpi Ns; int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; - mbedtls_mpi_init(&Ns); - /* Actually ( A + 2 ) / 4 */ MBEDTLS_MPI_CHK(mbedtls_mpi_lset(&grp->A, curve448_a24)); - /* P = 2^448 - 2^224 - 1 */ - MBEDTLS_MPI_CHK(mbedtls_mpi_lset(&grp->P, 1)); - MBEDTLS_MPI_CHK(mbedtls_mpi_shift_l(&grp->P, 224)); - MBEDTLS_MPI_CHK(mbedtls_mpi_sub_int(&grp->P, &grp->P, 1)); - MBEDTLS_MPI_CHK(mbedtls_mpi_shift_l(&grp->P, 224)); - MBEDTLS_MPI_CHK(mbedtls_mpi_sub_int(&grp->P, &grp->P, 1)); + ecp_mpi_load(&grp->P, curve448_p, sizeof(curve448_p)); grp->pbits = mbedtls_mpi_bitlen(&grp->P); /* Y intentionally not set, since we use x/z coordinates. @@ -4715,17 +4725,12 @@ static int ecp_use_curve448(mbedtls_ecp_group *grp) MBEDTLS_MPI_CHK(mbedtls_mpi_lset(&grp->G.Z, 1)); mbedtls_mpi_free(&grp->G.Y); - /* N = 2^446 - 13818066809895115352007386748515426880336692474882178609894547503885 */ - MBEDTLS_MPI_CHK(mbedtls_mpi_set_bit(&grp->N, 446, 1)); - MBEDTLS_MPI_CHK(mbedtls_mpi_read_binary(&Ns, - curve448_part_of_n, sizeof(curve448_part_of_n))); - MBEDTLS_MPI_CHK(mbedtls_mpi_sub_mpi(&grp->N, &grp->N, &Ns)); + ecp_mpi_load(&grp->N, curve448_n, sizeof(curve448_n)); /* Actually, the required msb for private keys */ grp->nbits = 447; cleanup: - mbedtls_mpi_free(&Ns); if (ret != 0) { mbedtls_ecp_group_free(grp); } From d61dbd4df7deace80bd3e315f6547ba3f925c5c4 Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Tue, 17 Jan 2023 15:52:44 +0000 Subject: [PATCH 3/6] ecp_curves: Update `mbedtls_ecp_group_free()`. This patch updates the method to not free the `grp->P` and `grp->N` structure members. The contents of `P` and `N` are stored in static memory at `curve448_p/n` and `curve25519p/n` and no longer dynamically allocated. Signed-off-by: Minos Galanakis --- library/ecp.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index d9d5425ed..08fbe86c7 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -582,11 +582,9 @@ void mbedtls_ecp_group_free(mbedtls_ecp_group *grp) } if (grp->h != 1) { - mbedtls_mpi_free(&grp->P); mbedtls_mpi_free(&grp->A); mbedtls_mpi_free(&grp->B); mbedtls_ecp_point_free(&grp->G); - mbedtls_mpi_free(&grp->N); } if (!ecp_group_is_static_comb_table(grp) && grp->T != NULL) { From e9fa7a74cd6fb62bbc241576f35abdd67f19507b Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Wed, 18 Jan 2023 09:46:52 +0000 Subject: [PATCH 4/6] ecp_curves: Update pre-processor define guards for `ecp_mpi_load()`. This patch adjusts the logic, so that the method is included, when the following components are enabled: * MBEDTLS_ECP_DP_CURVE448_ENABLED * MBEDTLS_ECP_DP_CURVE25519_ENABLED * ECP_LOAD_GROUP Signed-off-by: Minos Galanakis --- library/ecp_curves.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/library/ecp_curves.c b/library/ecp_curves.c index 88c4c850a..727283f73 100644 --- a/library/ecp_curves.c +++ b/library/ecp_curves.c @@ -4502,7 +4502,9 @@ static const mbedtls_ecp_point brainpoolP512r1_T[32] = { #endif #endif /* MBEDTLS_ECP_DP_BP512R1_ENABLED */ -#if defined(ECP_LOAD_GROUP) + +#if defined(ECP_LOAD_GROUP) || defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) || \ + defined(MBEDTLS_ECP_DP_CURVE448_ENABLED) /* * Create an MPI from embedded constants * (assumes len is an exact multiple of sizeof mbedtls_mpi_uint) @@ -4513,7 +4515,9 @@ static inline void ecp_mpi_load(mbedtls_mpi *X, const mbedtls_mpi_uint *p, size_ X->n = len / sizeof(mbedtls_mpi_uint); X->p = (mbedtls_mpi_uint *) p; } +#endif +#if defined(ECP_LOAD_GROUP) /* * Set an MPI to static value 1 */ From c8e381ab1c732824732ebfdfbd331e544e3de62c Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Thu, 19 Jan 2023 16:08:34 +0000 Subject: [PATCH 5/6] pkarse: Update `pk_group_id_from_specified()` clean-up. This path updates the clean-up logic of to individually free each of the the group's structure members rather than invoke `mbedtls_ecp_group_free()`. Signed-off-by: Minos Galanakis --- library/pkparse.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/library/pkparse.c b/library/pkparse.c index 990b55452..53e6dd0ba 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -429,7 +429,11 @@ static int pk_group_id_from_specified(const mbedtls_asn1_buf *params, ret = pk_group_id_from_group(&grp, grp_id); cleanup: - mbedtls_ecp_group_free(&grp); + mbedtls_mpi_free(&grp.N); + mbedtls_mpi_free(&grp.P); + mbedtls_mpi_free(&grp.A); + mbedtls_mpi_free(&grp.B); + mbedtls_ecp_point_free(&grp.G); return ret; } From 8692ec8bc0954b50c7bc33295b4a54bf946ccc32 Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Fri, 20 Jan 2023 15:27:32 +0000 Subject: [PATCH 6/6] pkarse: Added `pk_group_id_from_specified()` documentation. Signed-off-by: Minos Galanakis --- library/pkparse.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/library/pkparse.c b/library/pkparse.c index 53e6dd0ba..ccca692b7 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -429,6 +429,13 @@ static int pk_group_id_from_specified(const mbedtls_asn1_buf *params, ret = pk_group_id_from_group(&grp, grp_id); cleanup: + /* The API respecting lifecycle for mbedtls_ecp_group struct is + * _init(), _load() and _free(). In pk_group_id_from_specified() the + * temporary grp breaks that flow and it's members are populated + * by pk_group_id_from_group(). As such mbedtls_ecp_group_free() + * which is assuming a group populated by _setup() may not clean-up + * properly -> Manually free it's members. + */ mbedtls_mpi_free(&grp.N); mbedtls_mpi_free(&grp.P); mbedtls_mpi_free(&grp.A);