From 062f353804db3faa67f49cc0f5b8bf10beba3a06 Mon Sep 17 00:00:00 2001 From: TRodziewicz Date: Tue, 25 May 2021 15:15:57 +0200 Subject: [PATCH] Changes after code review Signed-off-by: TRodziewicz --- ChangeLog.d/issue4313.txt | 23 +--- .../remove_mbedtls_check_params_option.md | 40 ++----- library/psa_crypto.c | 5 +- tests/include/test/macros.h | 8 -- tests/suites/test_suite_aes.function | 5 +- tests/suites/test_suite_aria.function | 2 +- tests/suites/test_suite_blowfish.function | 2 +- tests/suites/test_suite_camellia.function | 2 +- tests/suites/test_suite_cipher.function | 2 +- tests/suites/test_suite_ecdh.function | 2 +- tests/suites/test_suite_ecjpake.function | 2 +- tests/suites/test_suite_ecp.function | 2 +- tests/suites/test_suite_gcm.function | 2 +- tests/suites/test_suite_shax.data | 3 - tests/suites/test_suite_shax.function | 108 +----------------- 15 files changed, 29 insertions(+), 179 deletions(-) diff --git a/ChangeLog.d/issue4313.txt b/ChangeLog.d/issue4313.txt index 026a190bd5..1fb61234be 100644 --- a/ChangeLog.d/issue4313.txt +++ b/ChangeLog.d/issue4313.txt @@ -1,25 +1,4 @@ Removals * Remove the following macros: MBEDTLS_CHECK_PARAMS, MBEDTLS_CHECK_PARAMS_ASSERT, MBEDTLS_PARAM_FAILED, - MBEDTLS_PARAM_FAILED_ALT, TEST_INVALID_PARAM, TEST_INVALID_PARAM_RET, - the following macros have been inactivated MBEDTLS_INTERNAL_VALIDATE_RET - and MBEDTLS_INTERNAL_VALIDATE, structures: param_failed_ctx_t, - mbedtls_test_param_failed_location_record_t, functions: - mbedtls_test_param_failed_get_location_record(), - mbedtls_test_param_failed_expect_call(), - mbedtls_test_param_failed_check_expected_call(), - mbedtls_test_param_failed_get_state_buf(), - mbedtls_test_param_failed_reset_state(), - mbedtls_param_failed(). Remove the following functions from all.sh: - component_test_check_params_functionality(), - component_test_check_params_without_platform(), - component_test_check_params_silent(). - Remove the following test functions from test_suite_*.function files: - aes_check_params(), aria_invalid_param(), blowfish_invalid_param(), - camellia_invalid_param(), ccm_invalid_param(), chacha20_bad_params(), - chachapoly_bad_params(), cipher_invalid_param_conditional(), - dhm_invalid_params(), ecdh_invalid_param(), ecdsa_invalid_param(), - ecjpake_invalid_param(), ecp_invalid_param(), gcm_invalid_param(), - mpi_invalid_param(), invalid_parameters() (pk), poly1305_bad_params(), - rsa_invalid_param(), sha1_invalid_param(), sha256_invalid_param(), - sha512_invalid_param(). Fixes #4313. + MBEDTLS_PARAM_FAILED_ALT. Fixes #4313. diff --git a/docs/3.0-migration-guide.d/remove_mbedtls_check_params_option.md b/docs/3.0-migration-guide.d/remove_mbedtls_check_params_option.md index 146b1c7592..6f43aa37a8 100644 --- a/docs/3.0-migration-guide.d/remove_mbedtls_check_params_option.md +++ b/docs/3.0-migration-guide.d/remove_mbedtls_check_params_option.md @@ -1,10 +1,11 @@ Remove MBEDTLS_CHECK_PARAMS option ---------------------------------- -This change affects the way of how parameters are validated. +This change does not affect users who use the default configuration; it only +affects users who enabled that option. -The option `MBEDTLS_CHECK_PARAMS` (disabled by default) enables certain kinds of -“parameter validation”. It covers two kinds of validations: +The option `MBEDTLS_CHECK_PARAMS` (disabled by default) enabled certain kinds +of “parameter validation”. It covered two kinds of validations: - In some functions that require a valid pointer, “parameter validation” checks that the pointer is non-null. With the feature disabled, a null pointer is not @@ -14,34 +15,17 @@ runtime crash. 90% of the uses of the feature are of this kind. checks that the value is a valid one. With the feature disabled, an invalid value causes a silent default to one of the valid values. -The default reaction to a failed check is to call a function mbedtls_param_failed -which the application must provide. If this function returns, its caller returns -an error `MBEDTLS_ERR_xxx_BAD_INPUT_DATA`. +The default reaction to a failed check was to call a function +`mbedtls_param_failed()` which the application had to provide. If this function +returned, its caller returned an error `MBEDTLS_ERR_xxx_BAD_INPUT_DATA`. -This feature is only used in some classic (non-PSA) cryptography modules. It is -not used in X.509, TLS or in PSA crypto, and it has not been implemented in all +This feature was only used in some classic (non-PSA) cryptography modules. It was +not used in X.509, TLS or in PSA crypto, and it was not implemented in all classic crypto modules. -Removal of `MBEDTLS_CHECK_PARAMS` and all dependent features means changing -code that does something like this: -``` -#if MBEDTLS_CHECK_PARAMS -#define VALIDATE(cond) do {if(cond) return BAD_INPUT_DATA;} while (0) -#else -#define VALIDATE(cond) do {} while (0) -#endif -... -VALIDATE(coin == HEADS || coin == TAILS); -VALIDATE(data != NULL); -if (coin == HEADS) heads(); -else tails(); -``` -to something like this: -``` -if (coin == HEADS) heads(); -else if (coin == TAILS) tails(); -else return BAD_INPUT_DATA; -``` +This feature has been removed. The library no longer checks for NULL pointers; +checks for enum-like arguments will be kept or re-introduced on a case-by-case +basis, but their presence will no longer be dependent on a compile-time option. Validation of enum-like values is somewhat useful, but not extremely important, because the parameters concerned are usually constants in applications. diff --git a/library/psa_crypto.c b/library/psa_crypto.c index 552750ce39..c3dc6e7549 100644 --- a/library/psa_crypto.c +++ b/library/psa_crypto.c @@ -1021,10 +1021,7 @@ psa_status_t psa_wipe_key_slot( psa_key_slot_t *slot ) /* * As the return error code may not be handled in case of multiple errors, - * do our best to report an unexpected lock counter: if available - * call MBEDTLS_PARAM_FAILED that may terminate execution (if called as - * part of the execution of a test suite this will stop the test suite - * execution). + * do our best to report an unexpected lock counter. */ if( slot->lock_count != 1 ) { diff --git a/tests/include/test/macros.h b/tests/include/test/macros.h index cad39aacaa..1c0e2bdd69 100644 --- a/tests/include/test/macros.h +++ b/tests/include/test/macros.h @@ -177,14 +177,6 @@ } \ } while( 0 ) -#if defined(MBEDTLS_CHECK_PARAMS) && !defined(MBEDTLS_PARAM_FAILED_ALT) -#define TEST_INVALID_PARAM_RET( PARAM_ERR_VALUE, TEST ) \ - do { if( ( TEST ) != ( PARAM_ERR_VALUE ) ) goto exit; } while( 0 ) - -#define TEST_INVALID_PARAM( TEST ) \ - do { TEST; } while( 0 ) -#endif /* MBEDTLS_CHECK_PARAMS && !MBEDTLS_PARAM_FAILED_ALT */ - /** * \brief This macro tests the statement passed to it as a test step or * individual test in a test case. The macro assumes the test will not fail. diff --git a/tests/suites/test_suite_aes.function b/tests/suites/test_suite_aes.function index a740391035..fe4dd3e820 100644 --- a/tests/suites/test_suite_aes.function +++ b/tests/suites/test_suite_aes.function @@ -206,9 +206,6 @@ void aes_crypt_xts_size( int size, int retval ) mbedtls_aes_xts_init( &ctx ); memset( data_unit, 0x00, sizeof( data_unit ) ); - - /* Valid pointers are passed for builds with MBEDTLS_CHECK_PARAMS, as - * otherwise we wouldn't get to the size check we're interested in. */ TEST_ASSERT( mbedtls_aes_crypt_xts( &ctx, MBEDTLS_AES_ENCRYPT, length, data_unit, src, output ) == retval ); } /* END_CASE */ @@ -359,7 +356,7 @@ exit: } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */ +/* BEGIN_CASE depends_on:NOT_DEFINED */ void aes_invalid_mode( ) { mbedtls_aes_context aes_ctx; diff --git a/tests/suites/test_suite_aria.function b/tests/suites/test_suite_aria.function index b24e4984ed..514706455a 100644 --- a/tests/suites/test_suite_aria.function +++ b/tests/suites/test_suite_aria.function @@ -23,7 +23,7 @@ void aria_valid_param( ) } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */ +/* BEGIN_CASE depends_on:NOT_DEFINED */ void aria_invalid_param( ) { mbedtls_aria_context ctx; diff --git a/tests/suites/test_suite_blowfish.function b/tests/suites/test_suite_blowfish.function index 5681a9e945..fdecfb6f92 100644 --- a/tests/suites/test_suite_blowfish.function +++ b/tests/suites/test_suite_blowfish.function @@ -14,7 +14,7 @@ void blowfish_valid_param( ) } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */ +/* BEGIN_CASE depends_on:NOT_DEFINED */ void blowfish_invalid_param( ) { mbedtls_blowfish_context ctx; diff --git a/tests/suites/test_suite_camellia.function b/tests/suites/test_suite_camellia.function index fb0a349177..0633b73d11 100644 --- a/tests/suites/test_suite_camellia.function +++ b/tests/suites/test_suite_camellia.function @@ -14,7 +14,7 @@ void camellia_valid_param( ) } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */ +/* BEGIN_CASE depends_on:NOT_DEFINED */ void camellia_invalid_param( ) { mbedtls_camellia_context ctx; diff --git a/tests/suites/test_suite_cipher.function b/tests/suites/test_suite_cipher.function index ea5fa2e5e4..f1095b1f03 100644 --- a/tests/suites/test_suite_cipher.function +++ b/tests/suites/test_suite_cipher.function @@ -207,7 +207,7 @@ exit: } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */ +/* BEGIN_CASE depends_on:NOT_DEFINED */ void cipher_invalid_param_conditional( ) { mbedtls_cipher_context_t valid_ctx; diff --git a/tests/suites/test_suite_ecdh.function b/tests/suites/test_suite_ecdh.function index 5fced62d42..5fa5b6781d 100644 --- a/tests/suites/test_suite_ecdh.function +++ b/tests/suites/test_suite_ecdh.function @@ -50,7 +50,7 @@ void ecdh_valid_param( ) } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */ +/* BEGIN_CASE depends_on:NOT_DEFINED */ void ecdh_invalid_param( ) { mbedtls_ecdh_context ctx; diff --git a/tests/suites/test_suite_ecjpake.function b/tests/suites/test_suite_ecjpake.function index 2a6d8935ef..311733b0dd 100644 --- a/tests/suites/test_suite_ecjpake.function +++ b/tests/suites/test_suite_ecjpake.function @@ -98,7 +98,7 @@ cleanup: * END_DEPENDENCIES */ -/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */ +/* BEGIN_CASE depends_on:NOT_DEFINED */ void ecjpake_invalid_param( ) { mbedtls_ecjpake_context ctx; diff --git a/tests/suites/test_suite_ecp.function b/tests/suites/test_suite_ecp.function index 9b8ba8201e..81a6514cb4 100644 --- a/tests/suites/test_suite_ecp.function +++ b/tests/suites/test_suite_ecp.function @@ -38,7 +38,7 @@ exit: } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */ +/* BEGIN_CASE depends_on:NOT_DEFINED */ void ecp_invalid_param( ) { mbedtls_ecp_group grp; diff --git a/tests/suites/test_suite_gcm.function b/tests/suites/test_suite_gcm.function index 35691f725f..ae306b5e3d 100644 --- a/tests/suites/test_suite_gcm.function +++ b/tests/suites/test_suite_gcm.function @@ -181,7 +181,7 @@ exit: } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */ +/* BEGIN_CASE depends_on:NOT_DEFINED */ void gcm_invalid_param( ) { mbedtls_gcm_context ctx; diff --git a/tests/suites/test_suite_shax.data b/tests/suites/test_suite_shax.data index 12eec84efc..1cc58af413 100644 --- a/tests/suites/test_suite_shax.data +++ b/tests/suites/test_suite_shax.data @@ -1,9 +1,6 @@ SHA-1 - Valid parameters sha1_valid_param: -SHA-1 - Invalid parameters -sha1_invalid_param: - # Test the operation of SHA-1 and SHA-2 SHA-1 Test Vector NIST CAVS #1 depends_on:MBEDTLS_SHA1_C diff --git a/tests/suites/test_suite_shax.function b/tests/suites/test_suite_shax.function index 1d4cf7192b..a8f5b1848d 100644 --- a/tests/suites/test_suite_shax.function +++ b/tests/suites/test_suite_shax.function @@ -11,46 +11,6 @@ void sha1_valid_param( ) } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_SHA1_C:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */ -void sha1_invalid_param( ) -{ - mbedtls_sha1_context ctx; - unsigned char buf[64] = { 0 }; - size_t const buflen = sizeof( buf ); - - TEST_INVALID_PARAM( mbedtls_sha1_init( NULL ) ); - - TEST_INVALID_PARAM( mbedtls_sha1_clone( NULL, &ctx ) ); - TEST_INVALID_PARAM( mbedtls_sha1_clone( &ctx, NULL ) ); - - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA, - mbedtls_sha1_starts_ret( NULL ) ); - - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA, - mbedtls_sha1_update_ret( NULL, buf, buflen ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA, - mbedtls_sha1_update_ret( &ctx, NULL, buflen ) ); - - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA, - mbedtls_sha1_finish_ret( NULL, buf ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA, - mbedtls_sha1_finish_ret( &ctx, NULL ) ); - - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA, - mbedtls_internal_sha1_process( NULL, buf ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA, - mbedtls_internal_sha1_process( &ctx, NULL ) ); - - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA, - mbedtls_sha1_ret( NULL, buflen, buf ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA1_BAD_INPUT_DATA, - mbedtls_sha1_ret( buf, buflen, NULL ) ); - -exit: - return; -} -/* END_CASE */ - /* BEGIN_CASE depends_on:MBEDTLS_SHA1_C */ void mbedtls_sha1( data_t * src_str, data_t * hash ) { @@ -72,7 +32,7 @@ void sha256_valid_param( ) } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_SHA256_C:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */ +/* BEGIN_CASE depends_on:MBEDTLS_SHA256_C:NOT_DEFINED */ void sha256_invalid_param( ) { mbedtls_sha256_context ctx; @@ -81,38 +41,10 @@ void sha256_invalid_param( ) int valid_type = 0; int invalid_type = 42; - TEST_INVALID_PARAM( mbedtls_sha256_init( NULL ) ); - - TEST_INVALID_PARAM( mbedtls_sha256_clone( NULL, &ctx ) ); - TEST_INVALID_PARAM( mbedtls_sha256_clone( &ctx, NULL ) ); - - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA, - mbedtls_sha256_starts_ret( NULL, valid_type ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA, + TEST_EQUAL( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA, mbedtls_sha256_starts_ret( &ctx, invalid_type ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA, - mbedtls_sha256_update_ret( NULL, buf, buflen ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA, - mbedtls_sha256_update_ret( &ctx, NULL, buflen ) ); - - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA, - mbedtls_sha256_finish_ret( NULL, buf ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA, - mbedtls_sha256_finish_ret( &ctx, NULL ) ); - - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA, - mbedtls_internal_sha256_process( NULL, buf ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA, - mbedtls_internal_sha256_process( &ctx, NULL ) ); - - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA, - mbedtls_sha256_ret( NULL, buflen, - buf, valid_type ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA, - mbedtls_sha256_ret( buf, buflen, - NULL, valid_type ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA, + TEST_EQUAL( MBEDTLS_ERR_SHA256_BAD_INPUT_DATA, mbedtls_sha256_ret( buf, buflen, buf, invalid_type ) ); @@ -156,7 +88,7 @@ void sha512_valid_param( ) } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_SHA512_C:MBEDTLS_CHECK_PARAMS:!MBEDTLS_PARAM_FAILED_ALT */ +/* BEGIN_CASE depends_on:MBEDTLS_SHA512_C:NOT_DEFINED */ void sha512_invalid_param( ) { mbedtls_sha512_context ctx; @@ -165,38 +97,10 @@ void sha512_invalid_param( ) int valid_type = 0; int invalid_type = 42; - TEST_INVALID_PARAM( mbedtls_sha512_init( NULL ) ); - - TEST_INVALID_PARAM( mbedtls_sha512_clone( NULL, &ctx ) ); - TEST_INVALID_PARAM( mbedtls_sha512_clone( &ctx, NULL ) ); - - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA, - mbedtls_sha512_starts_ret( NULL, valid_type ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA, + TEST_EQUAL( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA, mbedtls_sha512_starts_ret( &ctx, invalid_type ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA, - mbedtls_sha512_update_ret( NULL, buf, buflen ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA, - mbedtls_sha512_update_ret( &ctx, NULL, buflen ) ); - - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA, - mbedtls_sha512_finish_ret( NULL, buf ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA, - mbedtls_sha512_finish_ret( &ctx, NULL ) ); - - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA, - mbedtls_internal_sha512_process( NULL, buf ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA, - mbedtls_internal_sha512_process( &ctx, NULL ) ); - - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA, - mbedtls_sha512_ret( NULL, buflen, - buf, valid_type ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA, - mbedtls_sha512_ret( buf, buflen, - NULL, valid_type ) ); - TEST_INVALID_PARAM_RET( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA, + TEST_EQUAL( MBEDTLS_ERR_SHA512_BAD_INPUT_DATA, mbedtls_sha512_ret( buf, buflen, buf, invalid_type ) );