From 9538c9097eebeb38eeb9eef631491c8930669cf2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 20 Sep 2022 20:38:42 +0200 Subject: [PATCH 1/4] 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 0d97e80e55..787b96ab09 100644 --- a/tests/include/test/helpers.h +++ b/tests/include/test/helpers.h @@ -379,7 +379,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 7db8e89da12639595858ea7874f565b29270313a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 20 Sep 2022 18:31:30 +0200 Subject: [PATCH 2/4] 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 c310d48e7a..bfd21893d1 100644 --- a/tests/src/helpers.c +++ b/tests/src/helpers.c @@ -15,6 +15,7 @@ * limitations under the License. */ +#include #include #include #include @@ -120,8 +121,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 @@ -143,8 +148,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 @@ -166,8 +175,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 34cb4628825ea80ad84991bfba3c026387cf7bac Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 20 Sep 2022 21:37:56 +0200 Subject: [PATCH 3/4] 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 787b96ab09..757a43be4a 100644 --- a/tests/include/test/helpers.h +++ b/tests/include/test/helpers.h @@ -58,6 +58,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 91ad925fbf..a7531e700a 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -51,13 +51,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 eca29e4148f5eb16b4f167d94a9dcb1d69592a51 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 21 Sep 2022 22:00:06 +0200 Subject: [PATCH 4/4] 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)