From 97483b0fd424504d1b706279909acb0027e0cb30 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 20 Sep 2022 20:38:42 +0200 Subject: [PATCH 1/8] Remove incorrect comment This comment (which used to be attached to the implementation, and should not have been moved to the header file) is incorrect: the library function mbedtls_mpi_read_string preserves leading zeros as desired, but does not create a zero-limb object for an empty string. Signed-off-by: Gilles Peskine --- tests/include/test/helpers.h | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/include/test/helpers.h b/tests/include/test/helpers.h index 93a3e11323..c0677a99d4 100644 --- a/tests/include/test/helpers.h +++ b/tests/include/test/helpers.h @@ -291,7 +291,6 @@ void mbedtls_test_err_add_check( int high, int low, * * \return \c 0 on success, an \c MBEDTLS_ERR_MPI_xxx error code otherwise. */ -/* Since the library has exactly the desired behavior, this is trivial. */ int mbedtls_test_read_mpi( mbedtls_mpi *X, const char *s ); #endif /* MBEDTLS_BIGNUM_C */ From bdc7b8bb6ac3afb96953dec92a01abc2706972e6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 20 Sep 2022 18:31:30 +0200 Subject: [PATCH 2/8] Allow test assertions on constant-flow scalar data When testing a function that is supposed to be constant-flow, we declare the inputs as constant-flow secrets with TEST_CF_SECRET. The result of such a function is itself a constant-flow secret, so it can't be tested with comparison operators. In TEST_EQUAL, TEST_LE_U and TEST_LE_S, declare the values to be compared as public. This way, test code doesn't need to explicitly declare results as public if they're only used by one of these macros. Signed-off-by: Gilles Peskine --- tests/src/helpers.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/src/helpers.c b/tests/src/helpers.c index 4f976a27b0..673a8412b9 100644 --- a/tests/src/helpers.c +++ b/tests/src/helpers.c @@ -15,6 +15,7 @@ * limitations under the License. */ +#include #include #include #include @@ -102,8 +103,12 @@ void mbedtls_test_info_reset( void ) int mbedtls_test_equal( const char *test, int line_no, const char* filename, unsigned long long value1, unsigned long long value2 ) { + TEST_CF_PUBLIC( &value1, sizeof( value1 ) ); + TEST_CF_PUBLIC( &value2, sizeof( value2 ) ); + if( value1 == value2 ) return( 1 ); + if( mbedtls_test_info.result == MBEDTLS_TEST_RESULT_FAILED ) { /* We've already recorded the test as having failed. Don't @@ -125,8 +130,12 @@ int mbedtls_test_equal( const char *test, int line_no, const char* filename, int mbedtls_test_le_u( const char *test, int line_no, const char* filename, unsigned long long value1, unsigned long long value2 ) { + TEST_CF_PUBLIC( &value1, sizeof( value1 ) ); + TEST_CF_PUBLIC( &value2, sizeof( value2 ) ); + if( value1 <= value2 ) return( 1 ); + if( mbedtls_test_info.result == MBEDTLS_TEST_RESULT_FAILED ) { /* We've already recorded the test as having failed. Don't @@ -148,8 +157,12 @@ int mbedtls_test_le_u( const char *test, int line_no, const char* filename, int mbedtls_test_le_s( const char *test, int line_no, const char* filename, long long value1, long long value2 ) { + TEST_CF_PUBLIC( &value1, sizeof( value1 ) ); + TEST_CF_PUBLIC( &value2, sizeof( value2 ) ); + if( value1 <= value2 ) return( 1 ); + if( mbedtls_test_info.result == MBEDTLS_TEST_RESULT_FAILED ) { /* We've already recorded the test as having failed. Don't From 571576fc5cb8de0fc1eb3ea9de3edca641585bc2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 20 Sep 2022 21:37:56 +0200 Subject: [PATCH 3/8] Move the definition of data_t to a header file This way it can be used in helper functions. Signed-off-by: Gilles Peskine --- tests/include/test/helpers.h | 7 +++++++ tests/suites/helpers.function | 7 ------- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/include/test/helpers.h b/tests/include/test/helpers.h index c0677a99d4..823635c00e 100644 --- a/tests/include/test/helpers.h +++ b/tests/include/test/helpers.h @@ -59,6 +59,13 @@ #include "mbedtls/bignum.h" #endif +/** The type of test case arguments that contain binary data. */ +typedef struct data_tag +{ + uint8_t * x; + uint32_t len; +} data_t; + typedef enum { MBEDTLS_TEST_RESULT_SUCCESS = 0, diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index a620178f61..33cfc10624 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -52,13 +52,6 @@ typedef UINT32 uint32_t; #include #endif -/* Type for Hex parameters */ -typedef struct data_tag -{ - uint8_t * x; - uint32_t len; -} data_t; - /*----------------------------------------------------------------------------*/ /* Status and error constants */ From 3aae4e815ec1777ea220bccb6306f2d50b42787b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 20 Sep 2022 21:38:33 +0200 Subject: [PATCH 4/8] New function mbedtls_test_read_mpi_core Allocate and read an MPI from a binary test argument. Signed-off-by: Gilles Peskine --- tests/include/test/helpers.h | 22 ++++++++++++++++++++++ tests/src/helpers.c | 18 ++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/tests/include/test/helpers.h b/tests/include/test/helpers.h index 823635c00e..fe3b787fda 100644 --- a/tests/include/test/helpers.h +++ b/tests/include/test/helpers.h @@ -283,6 +283,28 @@ void mbedtls_test_err_add_check( int high, int low, #endif #if defined(MBEDTLS_BIGNUM_C) +/** Allocate and populate a core MPI from a test case argument. + * + * This function allocates exactly as many limbs as necessary to fit + * the length of the input. In other words, it preserves leading zeros. + * + * The limb array is allocated with mbedtls_calloc() and must later be + * freed with mbedtls_free(). + * + * \param[in,out] pX The address where a pointer to the allocated limb + * array will be stored. + * \c *pX must be null on entry. + * On exit, \c *pX is null on error or if the number + * of limbs is 0. + * \param[out] plimbs The address where the number of limbs will be stored. + * \param[in] input The test argument to read. + * It is interpreted as a big-endian integer in base 256. + * + * \return \c 0 on success, an \c MBEDTLS_ERR_MPI_xxx error code otherwise. + */ +int mbedtls_test_read_mpi_core( mbedtls_mpi_uint **pX, size_t *plimbs, + const data_t *input ); + /** Read an MPI from a hexadecimal string. * * Like mbedtls_mpi_read_string(), but size the resulting bignum based diff --git a/tests/src/helpers.c b/tests/src/helpers.c index 673a8412b9..da2c047b4d 100644 --- a/tests/src/helpers.c +++ b/tests/src/helpers.c @@ -345,6 +345,24 @@ void mbedtls_test_err_add_check( int high, int low, #endif /* MBEDTLS_TEST_HOOKS */ #if defined(MBEDTLS_BIGNUM_C) +#include "bignum_core.h" + +int mbedtls_test_read_mpi_core( mbedtls_mpi_uint **pX, size_t *plimbs, + const data_t *input ) +{ + /* Sanity check */ + if( *pX != NULL ) + return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA ); + + *plimbs = ( input->len + sizeof( **pX ) - 1 ) / sizeof( **pX ); + if( *plimbs == 0 ) + return( 0 ); + *pX = mbedtls_calloc( *plimbs, sizeof( **pX ) ); + if( *pX == NULL ) + return( MBEDTLS_ERR_MPI_ALLOC_FAILED ); + return( mbedtls_mpi_core_read_be( *pX, *plimbs, input->x, input->len ) ); +} + int mbedtls_test_read_mpi( mbedtls_mpi *X, const char *s ) { /* mbedtls_mpi_read_string() currently retains leading zeros. From 5bbdfce44cdd5ffc14c21d23f9f4d452d937c247 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 20 Sep 2022 21:39:25 +0200 Subject: [PATCH 5/8] Streamline mbedtls_mpi_core_lt_ct unit test Use mbedtls_test_read_mpi_core() to read the test data. Among other benefits, X and Y are now allocated to their exact size, so analyzers (Asan, Valgrind, Coverity, ...) have a chance of complaining if the tested function overflows the buffer. Remove TEST_CF_PUBLIC calls which are no longer necessary. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_mpi.data | 32 +++++++++++------------ tests/suites/test_suite_mpi.function | 39 +++++++++++----------------- 2 files changed, 31 insertions(+), 40 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 85812f5080..be4c056d79 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -607,10 +607,10 @@ mbedtls_mpi_core_lt_ct: x=y (0 limbs) mpi_core_lt_ct:"":"":0 mbedtls_mpi_core_lt_ct: x>y (63 bit x, y first byte greater) -mpi_core_lt_ct:"7FFFFFFFFFFFFFFF":"FF":0 +mpi_core_lt_ct:"7FFFFFFFFFFFFFFF":"00000000000000FF":0 mbedtls_mpi_core_lt_ct: xy (64 bit x, y=x-1) mpi_core_lt_ct:"8000000000000000":"7FFFFFFFFFFFFFFF":0 @@ -619,28 +619,28 @@ mbedtls_mpi_core_lt_ct: xy (64 bit x, y=1) -mpi_core_lt_ct:"8000000000000000":"01":0 +mpi_core_lt_ct:"8000000000000000":"0000000000000001":0 mbedtls_mpi_core_lt_ct: xy (64 bit x, y=0) -mpi_core_lt_ct:"8000000000000000":"00":0 +mpi_core_lt_ct:"8000000000000000":"0000000000000000":0 mbedtls_mpi_core_lt_ct: xy (64 bit x, first bytes equal) -mpi_core_lt_ct:"FFFFFFFFFFFFFFFF":"FF":0 +mpi_core_lt_ct:"FFFFFFFFFFFFFFFF":"00000000000000FF":0 mbedtls_mpi_core_lt_ct: xy (31 bit x, y first byte greater) -mpi_core_lt_ct:"7FFFFFFF":"FF":0 +mpi_core_lt_ct:"7FFFFFFF":"000000FF":0 mbedtls_mpi_core_lt_ct: xy (32 bit x, y=x-1) mpi_core_lt_ct:"80000000":"7FFFFFFF":0 @@ -649,22 +649,22 @@ mbedtls_mpi_core_lt_ct: xy (32 bit x, y=1) -mpi_core_lt_ct:"80000000":"01":0 +mpi_core_lt_ct:"80000000":"00000001":0 mbedtls_mpi_core_lt_ct: xy (32 bit x, y=0) -mpi_core_lt_ct:"80000000":"00":0 +mpi_core_lt_ct:"80000000":"00000000":0 mbedtls_mpi_core_lt_ct: xy (32 bit x, first bytes equal) -mpi_core_lt_ct:"FFFFFFFF":"FF":0 +mpi_core_lt_ct:"FFFFFFFF":"000000FF":0 mbedtls_mpi_core_lt_ct: xlen > input_Y->len ? input_X->len : input_Y->len ); + mbedtls_mpi_uint *X = NULL; + size_t X_limbs; + mbedtls_mpi_uint *Y = NULL; + size_t Y_limbs; + int ret; - TEST_LE_U( len, MAX_LEN ); + TEST_EQUAL( 0, mbedtls_test_read_mpi_core( &X, &X_limbs, input_X ) ); + TEST_EQUAL( 0, mbedtls_test_read_mpi_core( &Y, &Y_limbs, input_Y ) ); - TEST_ASSERT( mbedtls_mpi_core_read_be( X, len, input_X->x, input_X->len ) - == 0 ); - TEST_ASSERT( mbedtls_mpi_core_read_be( Y, len, input_Y->x, input_Y->len ) - == 0 ); + /* We need two same-length limb arrays */ + TEST_EQUAL( X_limbs, Y_limbs ); - TEST_CF_SECRET( X, len * sizeof( mbedtls_mpi_uint ) ); - TEST_CF_SECRET( Y, len * sizeof( mbedtls_mpi_uint ) ); - - ret = mbedtls_mpi_core_lt_ct( X, Y, len ); - - TEST_CF_PUBLIC( X, len * sizeof( mbedtls_mpi_uint ) ); - TEST_CF_PUBLIC( Y, len * sizeof( mbedtls_mpi_uint ) ); - TEST_CF_PUBLIC( &ret, sizeof( ret ) ); + TEST_CF_SECRET( X, X_limbs * sizeof( mbedtls_mpi_uint ) ); + TEST_CF_SECRET( Y, X_limbs * sizeof( mbedtls_mpi_uint ) ); + ret = mbedtls_mpi_core_lt_ct( X, Y, X_limbs ); TEST_EQUAL( ret, exp_ret ); exit: - ; - - #undef MAX_LEN + mbedtls_free( X ); + mbedtls_free( Y ); } /* END_CASE */ From 22514eb99bb517be79083b6d156bed19bc19f860 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 21 Sep 2022 23:13:04 +0200 Subject: [PATCH 6/8] Fix typo in documentation Signed-off-by: Gilles Peskine --- tests/scripts/generate_bignum_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/scripts/generate_bignum_tests.py b/tests/scripts/generate_bignum_tests.py index 7626ecd650..8a4d281c0c 100755 --- a/tests/scripts/generate_bignum_tests.py +++ b/tests/scripts/generate_bignum_tests.py @@ -31,7 +31,7 @@ following: function. - arguments(): a method to generate the list of arguments required for the test_function. - - generate_function_test(): a method to generate TestCases for the function. + - generate_function_tests(): a method to generate TestCases for the function. This should create instances of the class with required input data, and call `.create_test_case()` to yield the TestCase. From c217f4825179484e4c95072fcc715f092228c7c0 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 21 Sep 2022 22:00:06 +0200 Subject: [PATCH 7/8] Replace the output file atomically When writing the new .data file, first write the new content, then replace the target. This way, there isn't a temporary state in which the file is partially written. This temporary state can be misleading if the build is interrupted. It's annoying if you're watching changes to the output and the changes appear as emptying the file following by the new version appearing. Now interrupted builds don't leave a file that appears to be up to date but isn't, and when watching the output, there's a single transition to the new version. Signed-off-by: Gilles Peskine --- scripts/mbedtls_dev/test_case.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/mbedtls_dev/test_case.py b/scripts/mbedtls_dev/test_case.py index 6a46e4209b..d0afa592fd 100644 --- a/scripts/mbedtls_dev/test_case.py +++ b/scripts/mbedtls_dev/test_case.py @@ -92,9 +92,11 @@ def write_data_file(filename: str, """ if caller is None: caller = os.path.basename(sys.argv[0]) - with open(filename, 'w') as out: + tempfile = filename + '.new' + with open(tempfile, 'w') as out: out.write('# Automatically generated by {}. Do not edit!\n' .format(caller)) for tc in test_cases: tc.write(out) out.write('\n# End of automatically generated file.\n') + os.replace(tempfile, filename) From 99a82dce74c8a22ef57cfaea69ab1bbcaf798bed Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 5 Oct 2022 11:20:56 +0200 Subject: [PATCH 8/8] Readability improvement Signed-off-by: Gilles Peskine --- tests/src/helpers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/helpers.c b/tests/src/helpers.c index da2c047b4d..557c13c336 100644 --- a/tests/src/helpers.c +++ b/tests/src/helpers.c @@ -354,7 +354,7 @@ int mbedtls_test_read_mpi_core( mbedtls_mpi_uint **pX, size_t *plimbs, if( *pX != NULL ) return( MBEDTLS_ERR_MPI_BAD_INPUT_DATA ); - *plimbs = ( input->len + sizeof( **pX ) - 1 ) / sizeof( **pX ); + *plimbs = CHARS_TO_LIMBS( input->len ); if( *plimbs == 0 ) return( 0 ); *pX = mbedtls_calloc( *plimbs, sizeof( **pX ) );