From 72d7609f821c2cf98e1cef22c06b4509651eb3d9 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 20 Jan 2020 17:19:03 +0100 Subject: [PATCH 1/7] Bignum copy/shrink: More precise test case descriptions --- tests/suites/test_suite_mpi.data | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index f8ee09c05..500836077 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -205,37 +205,37 @@ mbedtls_mpi_cmp_abs:10:"2":10:"-3":-1 Base test mbedtls_mpi_cmp_abs (Mix values) #3 mbedtls_mpi_cmp_abs:10:"-2":10:"1":1 -Base test mbedtls_mpi_copy #1 +Copy zero (1 limb) to positive (1 limb) mbedtls_mpi_copy:0:1500 -Base test mpi_copy_self #1 +Copy self: positive (1 limb) mpi_copy_self:14 -Base test mbedtls_mpi_swap #1 +Swap 0 with positive (1 limb) mbedtls_mpi_swap:0:1500 -Test mbedtls_mpi_shrink #1 +Shrink 2 in 2 to 4 mbedtls_mpi_shrink:2:2:4:4 -Test mbedtls_mpi_shrink #2 +Shrink 2 in 4 to 4 mbedtls_mpi_shrink:4:2:4:4 -Test mbedtls_mpi_shrink #3 +Shrink 2 in 8 to 4 mbedtls_mpi_shrink:8:2:4:4 -Test mbedtls_mpi_shrink #4 +Shrink 4 in 8 to 4 mbedtls_mpi_shrink:8:4:4:4 -Test mbedtls_mpi_shrink #5 +Shrink 6 in 8 to 4 yielding 6 mbedtls_mpi_shrink:8:6:4:6 -Test mbedtls_mpi_shrink #6 +Shrink 2 in 4 to 0 yielding 2 mbedtls_mpi_shrink:4:2:0:2 -Test mbedtls_mpi_shrink #7 +Shrink 1 in 4 to 0 yielding 1 mbedtls_mpi_shrink:4:1:0:1 -Test mbedtls_mpi_shrink #8 +Shrink 0 in 4 to 0 yielding 1 mbedtls_mpi_shrink:4:0:0:1 Test mbedtls_mpi_safe_cond_assign #1 From 7428b451264df671fcc9345849f44de1ad3dbace Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 20 Jan 2020 21:01:51 +0100 Subject: [PATCH 2/7] Better coverage for copy and swap Cover more cases: different signs, different zeronesses, repeated argument. --- tests/suites/test_suite_mpi.data | 75 +++++++++++++++++++++- tests/suites/test_suite_mpi.function | 95 ++++++++++++++++++++++------ 2 files changed, 148 insertions(+), 22 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 500836077..6dcf575b6 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -206,13 +206,82 @@ Base test mbedtls_mpi_cmp_abs (Mix values) #3 mbedtls_mpi_cmp_abs:10:"-2":10:"1":1 Copy zero (1 limb) to positive (1 limb) -mbedtls_mpi_copy:0:1500 +mbedtls_mpi_copy_sint:0:1500 + +Copy zero (1 limb) to negative (1 limb) +mbedtls_mpi_copy_sint:0:-1500 + +Copy positive (1 limb) to zero (1 limb) +mbedtls_mpi_copy_sint:1500:0 + +Copy negative (1 limb) to zero (1 limb) +mbedtls_mpi_copy_sint:-1500:0 + +Copy positive (1 limb) to negative (1 limb) +mbedtls_mpi_copy_sint:1500:-42 + +Copy negative (1 limb) to positive (1 limb) +mbedtls_mpi_copy_sint:-42:1500 + +Copy zero (null) to zero (null) +mbedtls_mpi_copy_binary:"":"" + +Copy zero (null) to positive (1 limb) +mbedtls_mpi_copy_binary:"":"1234" + +Copy positive (1 limb) to zero (null) +mbedtls_mpi_copy_binary:"1234":"" + +Copy positive to larger +mbedtls_mpi_copy_binary:"bead":"ca5cadedb01dfaceacc01ade" + +Copy positive to smaller +mbedtls_mpi_copy_binary:"ca5cadedb01dfaceacc01ade":"bead" Copy self: positive (1 limb) mpi_copy_self:14 -Swap 0 with positive (1 limb) -mbedtls_mpi_swap:0:1500 +Copy self: zero (1 limb) +mpi_copy_self:0 + +Swap zero (1 limb) with positive (1 limb) +mbedtls_mpi_swap_sint:0:1500 + +Swap zero (1 limb) with negative (1 limb) +mbedtls_mpi_swap_sint:0:-1500 + +Swap positive (1 limb) with zero (1 limb) +mbedtls_mpi_swap_sint:1500:0 + +Swap negative (1 limb) with zero (1 limb) +mbedtls_mpi_swap_sint:-1500:0 + +Swap positive (1 limb) with negative (1 limb) +mbedtls_mpi_swap_sint:1500:-42 + +Swap negative (1 limb) with positive (1 limb) +mbedtls_mpi_swap_sint:-42:1500 + +Swap zero (null) with zero (null) +mbedtls_mpi_swap_binary:"":"" + +Swap zero (null) with positive (1 limb) +mbedtls_mpi_swap_binary:"":"1234" + +Swap positive (1 limb) with zero (null) +mbedtls_mpi_swap_binary:"1234":"" + +Swap positive with larger +mbedtls_mpi_swap_binary:"bead":"ca5cadedb01dfaceacc01ade" + +Swap positive with smaller +mbedtls_mpi_swap_binary:"ca5cadedb01dfaceacc01ade":"bead" + +Swap self: 1 limb +mpi_swap_self:"face" + +Swap self: null +mpi_swap_self:"" Shrink 2 in 2 to 4 mbedtls_mpi_shrink:2:2:4:4 diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index eaae1968e..32785c144 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -604,22 +604,40 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -void mbedtls_mpi_copy( int input_X, int input_A ) +void mbedtls_mpi_copy_sint( int input_X, int input_Y ) { - mbedtls_mpi X, Y, A; - mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); mbedtls_mpi_init( &A ); + mbedtls_mpi X, Y; + mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); TEST_ASSERT( mbedtls_mpi_lset( &X, input_X ) == 0 ); - TEST_ASSERT( mbedtls_mpi_lset( &Y, input_A ) == 0 ); - TEST_ASSERT( mbedtls_mpi_lset( &A, input_A ) == 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &Y ) != 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Y, &A ) == 0 ); + TEST_ASSERT( mbedtls_mpi_lset( &Y, input_Y ) == 0 ); + TEST_ASSERT( mbedtls_mpi_copy( &Y, &X ) == 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &Y ) == 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Y, &A ) != 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &X, input_X ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &Y, input_X ) == 0 ); exit: - mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); mbedtls_mpi_free( &A ); + mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); +} +/* END_CASE */ + +/* BEGIN_CASE */ +void mbedtls_mpi_copy_binary( data_t *input_X, data_t *input_Y ) +{ + mbedtls_mpi X, Y, X0; + mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); mbedtls_mpi_init( &X0 ); + + TEST_ASSERT( mbedtls_mpi_read_binary_le( &X, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary_le( &Y, input_Y->x, input_Y->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary_le( &X0, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &X0 ) == 0 ); + + TEST_ASSERT( mbedtls_mpi_copy( &Y, &X ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &X0 ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Y, &X0 ) == 0 ); + +exit: + mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); mbedtls_mpi_free( &X0 ); } /* END_CASE */ @@ -711,22 +729,61 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -void mbedtls_mpi_swap( int input_X, int input_Y ) +void mbedtls_mpi_swap_sint( int input_X, int input_Y ) { - mbedtls_mpi X, Y, A; - mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); mbedtls_mpi_init( &A ); + mbedtls_mpi X, Y; + mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); TEST_ASSERT( mbedtls_mpi_lset( &X, input_X ) == 0 ); TEST_ASSERT( mbedtls_mpi_lset( &Y, input_Y ) == 0 ); - TEST_ASSERT( mbedtls_mpi_lset( &A, input_X ) == 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &Y ) != 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &A ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &X, input_X ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &Y, input_Y ) == 0 ); + mbedtls_mpi_swap( &X, &Y ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &Y ) != 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Y, &A ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &X, input_Y ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &Y, input_X ) == 0 ); exit: - mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); mbedtls_mpi_free( &A ); + mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); +} +/* END_CASE */ + +/* BEGIN_CASE */ +void mbedtls_mpi_swap_binary( data_t *input_X, data_t *input_Y ) +{ + mbedtls_mpi X, Y, X0, Y0; + mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); + mbedtls_mpi_init( &X0 ); mbedtls_mpi_init( &Y0 ); + + TEST_ASSERT( mbedtls_mpi_read_binary_le( &X, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary_le( &Y, input_Y->x, input_Y->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary_le( &X0, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary_le( &Y0, input_Y->x, input_Y->len ) == 0 ); + + mbedtls_mpi_swap( &X, &Y ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &Y0 ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Y, &X0 ) == 0 ); + +exit: + mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); + mbedtls_mpi_free( &X0 ); mbedtls_mpi_free( &Y0 ); +} +/* END_CASE */ + +/* BEGIN_CASE */ +void mpi_swap_self( data_t *input_X ) +{ + mbedtls_mpi X, X0; + mbedtls_mpi_init( &X ); mbedtls_mpi_init( &X0 ); + + TEST_ASSERT( mbedtls_mpi_read_binary_le( &X, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary_le( &X0, input_X->x, input_X->len ) == 0 ); + + mbedtls_mpi_swap( &X, &X ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &X0 ) == 0 ); + +exit: + mbedtls_mpi_free( &X ); mbedtls_mpi_free( &X0 ); } /* END_CASE */ From db42062cb95e7928c101d401f7ec55ebf5ec0391 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 20 Jan 2020 21:12:50 +0100 Subject: [PATCH 3/7] mpi_copy: make the 0 case slightly more robust If Y was constructed through functions in this module, then Y->n == 0 iff Y->p == NULL. However we do not prevent filling mpi structures manually, and zero may be represented with n=0 and p a valid pointer. Most of the code can cope with such a representation, but for the source of mbedtls_mpi_copy, this would cause an integer underflow. Changing the test for zero from Y->p==NULL to Y->n==0 causes this case to work at no extra cost. --- library/bignum.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index 1d258db0e..231fa66d6 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -199,7 +199,7 @@ int mbedtls_mpi_copy( mbedtls_mpi *X, const mbedtls_mpi *Y ) if( X == Y ) return( 0 ); - if( Y->p == NULL ) + if( Y->n == 0 ) { mbedtls_mpi_free( X ); return( 0 ); From e2f563e22ed295a58359f5f88b7e775977e9d7ef Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 20 Jan 2020 21:17:43 +0100 Subject: [PATCH 4/7] Improve comments in mpi_shrink --- library/bignum.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index 231fa66d6..b6503bbff 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -158,9 +158,10 @@ int mbedtls_mpi_shrink( mbedtls_mpi *X, size_t nblimbs ) if( nblimbs > MBEDTLS_MPI_MAX_LIMBS ) return( MBEDTLS_ERR_MPI_ALLOC_FAILED ); - /* Actually resize up in this case */ + /* Actually resize up if there are currently fewer than nblimbs limbs. */ if( X->n <= nblimbs ) return( mbedtls_mpi_grow( X, nblimbs ) ); + /* Now X->n > nblimbs >= 0. */ for( i = X->n - 1; i > 0; i-- ) if( X->p[i] != 0 ) From 322752ba20246bd9df46437fce7fb235164a89ee Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 21 Jan 2020 13:59:51 +0100 Subject: [PATCH 5/7] Minor comment improvement --- library/bignum.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index b6503bbff..9af17aabd 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -161,7 +161,7 @@ int mbedtls_mpi_shrink( mbedtls_mpi *X, size_t nblimbs ) /* Actually resize up if there are currently fewer than nblimbs limbs. */ if( X->n <= nblimbs ) return( mbedtls_mpi_grow( X, nblimbs ) ); - /* Now X->n > nblimbs >= 0. */ + /* After this point, then X->n > nblimbs and in particular X->n > 0. */ for( i = X->n - 1; i > 0; i-- ) if( X->p[i] != 0 ) From 9a6ecee4deb3cf2ab7151a49fd512199b2a8d922 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 3 Feb 2020 16:15:47 +0100 Subject: [PATCH 6/7] Move test functions from Lilliput to Blefuscu We normally represent bignums in big-endian order and there is no reason to deviate here. --- tests/suites/test_suite_mpi.function | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index 32785c144..2b877b976 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -627,9 +627,9 @@ void mbedtls_mpi_copy_binary( data_t *input_X, data_t *input_Y ) mbedtls_mpi X, Y, X0; mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); mbedtls_mpi_init( &X0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &X, input_X->x, input_X->len ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &Y, input_Y->x, input_Y->len ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &X0, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &X, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &Y, input_Y->x, input_Y->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &X0, input_X->x, input_X->len ) == 0 ); TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &X0 ) == 0 ); TEST_ASSERT( mbedtls_mpi_copy( &Y, &X ) == 0 ); @@ -755,10 +755,10 @@ void mbedtls_mpi_swap_binary( data_t *input_X, data_t *input_Y ) mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); mbedtls_mpi_init( &X0 ); mbedtls_mpi_init( &Y0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &X, input_X->x, input_X->len ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &Y, input_Y->x, input_Y->len ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &X0, input_X->x, input_X->len ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &Y0, input_Y->x, input_Y->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &X, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &Y, input_Y->x, input_Y->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &X0, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &Y0, input_Y->x, input_Y->len ) == 0 ); mbedtls_mpi_swap( &X, &Y ); TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &Y0 ) == 0 ); @@ -776,8 +776,8 @@ void mpi_swap_self( data_t *input_X ) mbedtls_mpi X, X0; mbedtls_mpi_init( &X ); mbedtls_mpi_init( &X0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &X, input_X->x, input_X->len ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &X0, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &X, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &X0, input_X->x, input_X->len ) == 0 ); mbedtls_mpi_swap( &X, &X ); TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &X0 ) == 0 ); From a9da093617ef120f42f2aab5d999b8b3cb6b791b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 3 Feb 2020 16:18:30 +0100 Subject: [PATCH 7/7] shrink tests: clearer description --- tests/suites/test_suite_mpi.data | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 6dcf575b6..d21e2f7db 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -283,28 +283,28 @@ mpi_swap_self:"face" Swap self: null mpi_swap_self:"" -Shrink 2 in 2 to 4 +Shrink 2 limbs in a buffer of size 2 to 4 mbedtls_mpi_shrink:2:2:4:4 -Shrink 2 in 4 to 4 +Shrink 2 limbs in a buffer of size 4 to 4 mbedtls_mpi_shrink:4:2:4:4 -Shrink 2 in 8 to 4 +Shrink 2 limbs in a buffer of size 8 to 4 mbedtls_mpi_shrink:8:2:4:4 -Shrink 4 in 8 to 4 +Shrink 4 limbs in a buffer of size 8 to 4 mbedtls_mpi_shrink:8:4:4:4 -Shrink 6 in 8 to 4 yielding 6 +Shrink 6 limbs in a buffer of size 8 to 4 yielding 6 mbedtls_mpi_shrink:8:6:4:6 -Shrink 2 in 4 to 0 yielding 2 +Shrink 2 limbs in a buffer of size 4 to 0 yielding 2 mbedtls_mpi_shrink:4:2:0:2 -Shrink 1 in 4 to 0 yielding 1 +Shrink 1 limbs in a buffer of size 4 to 0 yielding 1 mbedtls_mpi_shrink:4:1:0:1 -Shrink 0 in 4 to 0 yielding 1 +Shrink 0 limbs in a buffer of size 4 to 0 yielding 1 mbedtls_mpi_shrink:4:0:0:1 Test mbedtls_mpi_safe_cond_assign #1