From 6f43c6038ec63136c37142fbcfa88808a857ebef Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 20 Jan 2020 17:19:03 +0100 Subject: [PATCH 1/6] 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 9f53e2271..c2de5a044 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -274,37 +274,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 84b8e25426e0a5bb5a6985013273ba2188d4e281 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 20 Jan 2020 21:01:51 +0100 Subject: [PATCH 2/6] 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 c2de5a044..0a59c7698 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -275,13 +275,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 f63f2e670..6f5abf3b8 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -374,22 +374,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( char *input_X, char *input_Y ) +{ + mbedtls_mpi X, Y, X0; + mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); mbedtls_mpi_init( &X0 ); + + TEST_ASSERT( mbedtls_mpi_read_string( &X, 16, input_X ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_string( &Y, 16, input_Y ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_string( &X0, 16, input_X ) == 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 */ @@ -481,22 +499,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( char *input_X, char *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_string( &X, 16, input_X ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_string( &Y, 16, input_Y ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_string( &X0, 16, input_X ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_string( &Y0, 16, input_Y ) == 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( char *input_X ) +{ + mbedtls_mpi X, X0; + mbedtls_mpi_init( &X ); mbedtls_mpi_init( &X0 ); + + TEST_ASSERT( mbedtls_mpi_read_string( &X, 16, input_X ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_string( &X0, 16, input_X ) == 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 2aeab87cf7fb08c8882f275a2e2d507cd573921a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 20 Jan 2020 21:12:50 +0100 Subject: [PATCH 3/6] 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 9692127b9..fd9d5b8a8 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -190,7 +190,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 6a26967382b5bb62c17975b46920a115ad3551b0 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 20 Jan 2020 21:17:43 +0100 Subject: [PATCH 4/6] 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 fd9d5b8a8..dafeb4c3a 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -151,9 +151,10 @@ int mbedtls_mpi_shrink( mbedtls_mpi *X, size_t nblimbs ) mbedtls_mpi_uint *p; size_t i; - /* 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 774c163eaec90939b4051fd4c901b3264b92490b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 21 Jan 2020 13:59:51 +0100 Subject: [PATCH 5/6] 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 dafeb4c3a..827a3cb66 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -154,7 +154,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 261aea19564afa03645390b4ce0460c2e4c53a9c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 3 Feb 2020 16:18:30 +0100 Subject: [PATCH 6/6] 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 0a59c7698..a6a642306 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -352,28 +352,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