From cd860dfe0290d21afd69bcc0cae18f8dc6a4d170 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 18 Aug 2022 16:23:05 +0100 Subject: [PATCH 1/7] bignum_mod: Added Montgomery constants This patch adds the Montgomery constants to the `mbedtls_mpi_mont_struct`. Signed-off-by: Minos Galanakis --- library/bignum_mod.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/library/bignum_mod.h b/library/bignum_mod.h index c25eb87423..3b3338c2de 100644 --- a/library/bignum_mod.h +++ b/library/bignum_mod.h @@ -53,7 +53,11 @@ typedef struct size_t limbs; } mbedtls_mpi_mod_residue; -typedef void *mbedtls_mpi_mont_struct; +typedef struct { + mbedtls_mpi_uint const *rr; /* The residue for 2^{2*n*biL} mod N */ + mbedtls_mpi_uint mm; /* Montgomery const for -N^{-1} mod 2^{ciL} */ +} mbedtls_mpi_mont_struct; + typedef void *mbedtls_mpi_opt_red_struct; typedef struct { From 760f5d6b6b82767e0ce6e22a9e384161734bc2ea Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Thu, 11 Aug 2022 12:21:09 +0100 Subject: [PATCH 2/7] bignum_mod: Updated mbedtls_mpi_mod_modulus_setup/free with new fields At the current state, those fields are initialised to 0, NULL. Signed-off-by: Minos Galanakis --- library/bignum_mod.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/bignum_mod.c b/library/bignum_mod.c index f2c11a582a..7cf1b012c5 100644 --- a/library/bignum_mod.c +++ b/library/bignum_mod.c @@ -77,8 +77,8 @@ void mbedtls_mpi_mod_modulus_free( mbedtls_mpi_mod_modulus *m ) switch( m->int_rep ) { case MBEDTLS_MPI_MOD_REP_MONTGOMERY: - mbedtls_free( m->rep.mont ); - break; + m->rep.mont.rr = NULL; + m->rep.mont.mm = 0; break; case MBEDTLS_MPI_MOD_REP_OPT_RED: mbedtls_free( m->rep.ored ); break; @@ -120,8 +120,8 @@ int mbedtls_mpi_mod_modulus_setup( mbedtls_mpi_mod_modulus *m, { case MBEDTLS_MPI_MOD_REP_MONTGOMERY: m->int_rep = int_rep; - m->rep.mont = NULL; - break; + m->rep.mont.rr = NULL; + m->rep.mont.mm = 0; break; case MBEDTLS_MPI_MOD_REP_OPT_RED: m->int_rep = int_rep; m->rep.ored = NULL; From 8b3336331507ee2ccc49e3a13c73fde1820cff5a Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Tue, 11 Oct 2022 11:28:24 +0100 Subject: [PATCH 3/7] bignum_mod: Updated modulus lifecycle with mm and rr. This patch updates the `mbedtls_mpi_mod_modulus_setup/free()` methods to precalculate mm and rr(Montgomery const squared) during setup and zeroize it during free. A static `set_mont_const_square()` is added to manage the memory allocation and parameter checking before invoking the `mbedtls_mpi_core_get_mont_r2_unsafe()` Signed-off-by: Minos Galanakis --- library/bignum_mod.c | 40 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/library/bignum_mod.c b/library/bignum_mod.c index 7cf1b012c5..92c011cffe 100644 --- a/library/bignum_mod.c +++ b/library/bignum_mod.c @@ -77,6 +77,9 @@ void mbedtls_mpi_mod_modulus_free( mbedtls_mpi_mod_modulus *m ) switch( m->int_rep ) { case MBEDTLS_MPI_MOD_REP_MONTGOMERY: + mbedtls_platform_zeroize( (mbedtls_mpi_uint *) m->rep.mont.rr, + m->limbs ); + mbedtls_free( (mbedtls_mpi_uint *)m->rep.mont.rr ); m->rep.mont.rr = NULL; m->rep.mont.mm = 0; break; case MBEDTLS_MPI_MOD_REP_OPT_RED: @@ -93,6 +96,38 @@ void mbedtls_mpi_mod_modulus_free( mbedtls_mpi_mod_modulus *m ) m->int_rep = MBEDTLS_MPI_MOD_REP_INVALID; } +static int set_mont_const_square( const mbedtls_mpi_uint **X, + const mbedtls_mpi_uint *A, + size_t limbs ) +{ + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; + mbedtls_mpi N; + mbedtls_mpi RR; + + mbedtls_mpi_init( &N ); + mbedtls_mpi_init( &RR ); + + if ( A == NULL || limbs == 0 || limbs >= ( MBEDTLS_MPI_MAX_LIMBS / 2 ) - 2 ) + goto cleanup; + + if ( !mbedtls_mpi_grow( &N, limbs )) + memcpy( N.p, A, sizeof(mbedtls_mpi_uint) * limbs ); + else + goto cleanup; + + mbedtls_mpi_core_get_mont_r2_unsafe(&RR, &N); + + *X = RR.p; + RR.p = NULL; + ret = 0; + +cleanup: + mbedtls_mpi_free(&N); + mbedtls_mpi_free(&RR); + ret = ( ret != 0 ) ? MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED : 0; + return( ret ); +} + int mbedtls_mpi_mod_modulus_setup( mbedtls_mpi_mod_modulus *m, const mbedtls_mpi_uint *p, size_t p_limbs, @@ -120,8 +155,9 @@ int mbedtls_mpi_mod_modulus_setup( mbedtls_mpi_mod_modulus *m, { case MBEDTLS_MPI_MOD_REP_MONTGOMERY: m->int_rep = int_rep; - m->rep.mont.rr = NULL; - m->rep.mont.mm = 0; break; + m->rep.mont.mm = mbedtls_mpi_core_montmul_init( m->p ); + set_mont_const_square( &m->rep.mont.rr, m->p, m->limbs ); + break; case MBEDTLS_MPI_MOD_REP_OPT_RED: m->int_rep = int_rep; m->rep.ored = NULL; From dd365a526fc32cd688558e0c4aa9642f4670c97f Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Wed, 19 Oct 2022 01:48:32 +0100 Subject: [PATCH 4/7] test_suite_bignum: Updated `mpi_mod_setup()` test This patch updates the `mpi_mod_setup()` test suite to check for incosistencies in the montgomery constant data's lifecycle. Signed-off-by: Minos Galanakis --- tests/suites/test_suite_bignum_mod.function | 30 ++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/tests/suites/test_suite_bignum_mod.function b/tests/suites/test_suite_bignum_mod.function index 9f73209656..8716f679a3 100644 --- a/tests/suites/test_suite_bignum_mod.function +++ b/tests/suites/test_suite_bignum_mod.function @@ -17,20 +17,48 @@ void mpi_mod_setup( int ext_rep, int int_rep, int iret ) #define MLIMBS 8 mbedtls_mpi_uint mp[MLIMBS]; mbedtls_mpi_mod_modulus m; + const size_t mp_size = sizeof(mbedtls_mpi_uint); + mbedtls_mpi_uint * rr; int ret; - memset( mp, 0xFF, sizeof(mp) ); + memset( mp, 0xFF, mp_size ); mbedtls_mpi_mod_modulus_init( &m ); ret = mbedtls_mpi_mod_modulus_setup( &m, mp, MLIMBS, ext_rep, int_rep ); TEST_EQUAL( ret, iret ); + /* Only test if the constants have been set-up */ + if ( ret == 0 && int_rep == MBEDTLS_MPI_MOD_REP_MONTGOMERY ) + { + /* Test that the consts have been calculated */ + TEST_ASSERT( m.rep.mont.rr != NULL ); + TEST_ASSERT( m.rep.mont.mm != 0 ); + + /* Keep a copy of the memory location used to store Montgomery const */ + rr = (mbedtls_mpi_uint *)m.rep.mont.rr; + } + /* Address sanitiser should catch if we try to free mp */ mbedtls_mpi_mod_modulus_free( &m ); /* Make sure that the modulus doesn't have reference to mp anymore */ TEST_ASSERT( m.p != mp ); + /* Only test if the constants have been set-up */ + if ( ret == 0 && int_rep == MBEDTLS_MPI_MOD_REP_MONTGOMERY ) + { + /* Reuse the allocated buffer for a zeroization check */ + memset( mp, 0x00, mp_size ); + + /* Verify the data and pointers allocated have been properly wiped */ + TEST_ASSERT( m.rep.mont.rr == NULL ); + TEST_ASSERT( m.rep.mont.mm == 0 ); + + /* mbedtls_mpi_mod_modulus_free() has set the + * (mbedtls_mpi_uint *)m.rep.mont.rr -> NULL. + * Verify that the actual data have been zeroed */ + ASSERT_COMPARE( rr, mp_size, &mp, mp_size ); + } exit: /* It should be safe to call an mbedtls free several times */ mbedtls_mpi_mod_modulus_free( &m ); From 771c47055f2b9db6762ab32dab36092952990573 Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Thu, 27 Oct 2022 12:22:22 +0100 Subject: [PATCH 5/7] bignum_mod: Style changes This patch addresses review comments with regards to style of `mbedtls_mpi_mod_modulus_setup/free()`. It also removes a test check which was triggering a use-after-free. Signed-off-by: Minos Galanakis --- library/bignum_mod.c | 9 +++++---- tests/suites/test_suite_bignum_mod.function | 7 ------- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/library/bignum_mod.c b/library/bignum_mod.c index 92c011cffe..705b2dbae6 100644 --- a/library/bignum_mod.c +++ b/library/bignum_mod.c @@ -81,7 +81,8 @@ void mbedtls_mpi_mod_modulus_free( mbedtls_mpi_mod_modulus *m ) m->limbs ); mbedtls_free( (mbedtls_mpi_uint *)m->rep.mont.rr ); m->rep.mont.rr = NULL; - m->rep.mont.mm = 0; break; + m->rep.mont.mm = 0; + break; case MBEDTLS_MPI_MOD_REP_OPT_RED: mbedtls_free( m->rep.ored ); break; @@ -110,11 +111,11 @@ static int set_mont_const_square( const mbedtls_mpi_uint **X, if ( A == NULL || limbs == 0 || limbs >= ( MBEDTLS_MPI_MAX_LIMBS / 2 ) - 2 ) goto cleanup; - if ( !mbedtls_mpi_grow( &N, limbs )) - memcpy( N.p, A, sizeof(mbedtls_mpi_uint) * limbs ); - else + if ( mbedtls_mpi_grow( &N, limbs )) goto cleanup; + memcpy( N.p, A, sizeof(mbedtls_mpi_uint) * limbs ); + mbedtls_mpi_core_get_mont_r2_unsafe(&RR, &N); *X = RR.p; diff --git a/tests/suites/test_suite_bignum_mod.function b/tests/suites/test_suite_bignum_mod.function index 8716f679a3..2e0377a703 100644 --- a/tests/suites/test_suite_bignum_mod.function +++ b/tests/suites/test_suite_bignum_mod.function @@ -18,7 +18,6 @@ void mpi_mod_setup( int ext_rep, int int_rep, int iret ) mbedtls_mpi_uint mp[MLIMBS]; mbedtls_mpi_mod_modulus m; const size_t mp_size = sizeof(mbedtls_mpi_uint); - mbedtls_mpi_uint * rr; int ret; memset( mp, 0xFF, mp_size ); @@ -34,8 +33,6 @@ void mpi_mod_setup( int ext_rep, int int_rep, int iret ) TEST_ASSERT( m.rep.mont.rr != NULL ); TEST_ASSERT( m.rep.mont.mm != 0 ); - /* Keep a copy of the memory location used to store Montgomery const */ - rr = (mbedtls_mpi_uint *)m.rep.mont.rr; } /* Address sanitiser should catch if we try to free mp */ @@ -54,10 +51,6 @@ void mpi_mod_setup( int ext_rep, int int_rep, int iret ) TEST_ASSERT( m.rep.mont.rr == NULL ); TEST_ASSERT( m.rep.mont.mm == 0 ); - /* mbedtls_mpi_mod_modulus_free() has set the - * (mbedtls_mpi_uint *)m.rep.mont.rr -> NULL. - * Verify that the actual data have been zeroed */ - ASSERT_COMPARE( rr, mp_size, &mp, mp_size ); } exit: /* It should be safe to call an mbedtls free several times */ From 4d4c98b1b959a2f1ac270806cfa36d849d286889 Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Thu, 27 Oct 2022 15:58:02 +0100 Subject: [PATCH 6/7] bignum_mod: `mbedtls_mpi_mod_modulus_setup()` refactoring. This patch addresses more review comments, and fixes a circular depedency in the `mbedtls_mpi_mod_modulus_setup()`. Signed-off-by: Minos Galanakis --- library/bignum_mod.c | 28 +++++++++++++-------- tests/suites/test_suite_bignum_mod.function | 7 +----- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/library/bignum_mod.c b/library/bignum_mod.c index 705b2dbae6..60c073ac4e 100644 --- a/library/bignum_mod.c +++ b/library/bignum_mod.c @@ -77,10 +77,13 @@ void mbedtls_mpi_mod_modulus_free( mbedtls_mpi_mod_modulus *m ) switch( m->int_rep ) { case MBEDTLS_MPI_MOD_REP_MONTGOMERY: - mbedtls_platform_zeroize( (mbedtls_mpi_uint *) m->rep.mont.rr, - m->limbs ); - mbedtls_free( (mbedtls_mpi_uint *)m->rep.mont.rr ); - m->rep.mont.rr = NULL; + if (m->rep.mont.rr != NULL) + { + mbedtls_platform_zeroize( (mbedtls_mpi_uint *) m->rep.mont.rr, + m->limbs ); + mbedtls_free( (mbedtls_mpi_uint *)m->rep.mont.rr ); + m->rep.mont.rr = NULL; + } m->rep.mont.mm = 0; break; case MBEDTLS_MPI_MOD_REP_OPT_RED: @@ -104,6 +107,7 @@ static int set_mont_const_square( const mbedtls_mpi_uint **X, int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; mbedtls_mpi N; mbedtls_mpi RR; + *X = NULL; mbedtls_mpi_init( &N ); mbedtls_mpi_init( &RR ); @@ -111,16 +115,18 @@ static int set_mont_const_square( const mbedtls_mpi_uint **X, if ( A == NULL || limbs == 0 || limbs >= ( MBEDTLS_MPI_MAX_LIMBS / 2 ) - 2 ) goto cleanup; - if ( mbedtls_mpi_grow( &N, limbs )) + if ( mbedtls_mpi_grow( &N, limbs ) ) goto cleanup; - memcpy( N.p, A, sizeof(mbedtls_mpi_uint) * limbs ); + memcpy( N.p, A, sizeof(mbedtls_mpi_uint) * limbs ); - mbedtls_mpi_core_get_mont_r2_unsafe(&RR, &N); + ret = mbedtls_mpi_core_get_mont_r2_unsafe(&RR, &N); - *X = RR.p; - RR.p = NULL; - ret = 0; + if ( ret == 0 ) + { + *X = RR.p; + RR.p = NULL; + } cleanup: mbedtls_mpi_free(&N); @@ -157,7 +163,7 @@ int mbedtls_mpi_mod_modulus_setup( mbedtls_mpi_mod_modulus *m, case MBEDTLS_MPI_MOD_REP_MONTGOMERY: m->int_rep = int_rep; m->rep.mont.mm = mbedtls_mpi_core_montmul_init( m->p ); - set_mont_const_square( &m->rep.mont.rr, m->p, m->limbs ); + ret = set_mont_const_square( &m->rep.mont.rr, m->p, m->limbs ); break; case MBEDTLS_MPI_MOD_REP_OPT_RED: m->int_rep = int_rep; diff --git a/tests/suites/test_suite_bignum_mod.function b/tests/suites/test_suite_bignum_mod.function index 2e0377a703..ad89bdf3e7 100644 --- a/tests/suites/test_suite_bignum_mod.function +++ b/tests/suites/test_suite_bignum_mod.function @@ -17,10 +17,9 @@ void mpi_mod_setup( int ext_rep, int int_rep, int iret ) #define MLIMBS 8 mbedtls_mpi_uint mp[MLIMBS]; mbedtls_mpi_mod_modulus m; - const size_t mp_size = sizeof(mbedtls_mpi_uint); int ret; - memset( mp, 0xFF, mp_size ); + memset( mp, 0xFF, sizeof(mp) ); mbedtls_mpi_mod_modulus_init( &m ); ret = mbedtls_mpi_mod_modulus_setup( &m, mp, MLIMBS, ext_rep, int_rep ); @@ -44,13 +43,9 @@ void mpi_mod_setup( int ext_rep, int int_rep, int iret ) /* Only test if the constants have been set-up */ if ( ret == 0 && int_rep == MBEDTLS_MPI_MOD_REP_MONTGOMERY ) { - /* Reuse the allocated buffer for a zeroization check */ - memset( mp, 0x00, mp_size ); - /* Verify the data and pointers allocated have been properly wiped */ TEST_ASSERT( m.rep.mont.rr == NULL ); TEST_ASSERT( m.rep.mont.mm == 0 ); - } exit: /* It should be safe to call an mbedtls free several times */ From 0c61a749b79663e2445e2da7884041badf4c80c4 Mon Sep 17 00:00:00 2001 From: Minos Galanakis Date: Thu, 27 Oct 2022 18:20:33 +0100 Subject: [PATCH 7/7] test_suite_bignum_mod_raw: Removed parameter for `mbedtls_mpi_mod_modulus_setup()` This patch updates the tests `mpi_mod_raw_cond_swap()` & `mpi_mod_raw_cond_assign()` to use a non-zero modulus when invoking `mbedtls_mpi_mod_modulus_setup()` Signed-off-by: Minos Galanakis --- tests/suites/test_suite_bignum_mod_raw.function | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/suites/test_suite_bignum_mod_raw.function b/tests/suites/test_suite_bignum_mod_raw.function index 88b8917809..7c9d5dbe4e 100644 --- a/tests/suites/test_suite_bignum_mod_raw.function +++ b/tests/suites/test_suite_bignum_mod_raw.function @@ -134,6 +134,7 @@ void mpi_mod_raw_cond_assign( data_t * input_X, ASSERT_ALLOC( Y, limbs ); ASSERT_ALLOC( buff_m, limbs ); + memset( buff_m, 0xFF, copy_bytes ); TEST_ASSERT( mbedtls_mpi_mod_modulus_setup( &m, buff_m, copy_limbs, MBEDTLS_MPI_MOD_EXT_REP_BE, @@ -214,6 +215,7 @@ void mpi_mod_raw_cond_swap( data_t * input_X, ASSERT_ALLOC( tmp_Y, limbs ); ASSERT_ALLOC( buff_m, copy_limbs ); + memset( buff_m, 0xFF, copy_bytes ); TEST_ASSERT( mbedtls_mpi_mod_modulus_setup( &m, buff_m, copy_limbs, MBEDTLS_MPI_MOD_EXT_REP_BE,