From e6cca7c937c0c484d6f0a78140ef50a274a556fa 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 4d46ca742..d550b21ed 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -280,37 +280,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 8fe3b79cdb4825668c54367e2b1ed5c6f08b5144 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 d550b21ed..fa55e6a8d 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -281,13 +281,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 bebca5a0a..34c23fe8b 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -579,22 +579,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 */ @@ -686,22 +704,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 3e9f5228c8072ce2821d2158ba8f2e561d5e5c21 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 6713bcbf6..fdf571d2e 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -198,7 +198,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 27c15c7853e33cc942a43c18d0c3afe3f608b95f 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 fdf571d2e..5ee34aec9 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -157,9 +157,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 56427c2d2ba9046963669f1c6e0941b46e91523b 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 5ee34aec9..87ccf42fa 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -160,7 +160,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 e0ced3a3d6527e5a43878f75bd3591e4a34db1d0 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 34c23fe8b..f2702f12b 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -602,9 +602,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 ); @@ -730,10 +730,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 ); @@ -751,8 +751,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 8ece8572b287a0fded3ed4061baa1df03f1ad92b 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 fa55e6a8d..00926ff97 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -358,28 +358,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