From 4b627af36c67f1cecf740035b17abbf0d01f9990 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 Jul 2021 21:02:36 +0200 Subject: [PATCH 01/17] New macro MBEDTLS_CHECK_RETURN Put this macro before a function declaration to indicate that its result must be checked. This commit supports GCC-like compilers and MSVC >=2012. Signed-off-by: Gilles Peskine --- include/mbedtls/build_info.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/include/mbedtls/build_info.h b/include/mbedtls/build_info.h index 23f85ba01..46717608b 100644 --- a/include/mbedtls/build_info.h +++ b/include/mbedtls/build_info.h @@ -53,6 +53,24 @@ #define _CRT_SECURE_NO_DEPRECATE 1 #endif +/** \def MBEDTLS_CHECK_RETURN + * + * This macro appearing at the beginning of the declaration of a function + * indicates that its return value should be checked. + * + * This should appear before most functions returning an error code + * (as \c int in the \c mbedtls_xxx API or + * as ::psa_status_t in the \c psa_xxx API). + */ +#if defined(__GNUC__) +#define MBEDTLS_CHECK_RETURN __attribute__((warn_unused_result)) +#elif defined(_MSC_VER) && _MSC_VER >= 1700 +#include +#define MBEDTLS_CHECK_RETURN _Check_return_ +#else +#define MBEDTLS_CHECK_RETURN +#endif + #if !defined(MBEDTLS_CONFIG_FILE) #include "mbedtls/mbedtls_config.h" #else From 7820a574f158deaa2110f8718704c99d295c0b12 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 7 Jul 2021 21:08:28 +0200 Subject: [PATCH 02/17] Catch failures of AES or DES operations Declare all AES and DES functions that return int as needing to have their result checked, and do check the result in our code. A DES or AES block operation can fail in alternative implementations of mbedtls_internal_aes_encrypt() (under MBEDTLS_AES_ENCRYPT_ALT), mbedtls_internal_aes_decrypt() (under MBEDTLS_AES_DECRYPT_ALT), mbedtls_des_crypt_ecb() (under MBEDTLS_DES_CRYPT_ECB_ALT), mbedtls_des3_crypt_ecb() (under MBEDTLS_DES3_CRYPT_ECB_ALT). A failure can happen if the accelerator peripheral is in a bad state. Several block modes were not catching the error. This commit does the following code changes, grouped together to avoid having an intermediate commit where the build fails: * Add MBEDTLS_CHECK_RETURN to all functions returning int in aes.h and des.h. * Fix all places where this causes a GCC warning, indicating that our code was not properly checking the result of an AES operation: * In library code: on failure, goto exit and return ret. * In pkey programs: goto exit. * In the benchmark program: exit (not ideal since there's no error message, but it's what the code currently does for failures). * In test code: TEST_ASSERT. * Changelog entry. Signed-off-by: Gilles Peskine --- check-return.txt | 12 +++++ include/mbedtls/aes.h | 14 ++++++ include/mbedtls/des.h | 13 +++++ library/aes.c | 48 ++++++++++++++---- library/des.c | 75 +++++++++++++++++++--------- programs/pkey/dh_client.c | 8 ++- programs/pkey/dh_server.c | 8 ++- programs/test/benchmark.c | 10 ++-- tests/suites/test_suite_aes.function | 12 ++--- tests/suites/test_suite_des.function | 24 ++++----- 10 files changed, 164 insertions(+), 60 deletions(-) create mode 100644 check-return.txt diff --git a/check-return.txt b/check-return.txt new file mode 100644 index 000000000..e6371ec69 --- /dev/null +++ b/check-return.txt @@ -0,0 +1,12 @@ +Bugfix + * Failures of alternative implementations of AES or DES single-block + functions enabled with MBEDTLS_AES_ENCRYPT_ALT, MBEDTLS_AES_DECRYPT_ALT, + MBEDTLS_DES_CRYPT_ECB_ALT or MBEDTLS_DES3_CRYPT_ECB_ALT were ignored. + This does not concern the implementation provided with Mbed TLS, + where this function cannot fail, or full-module replacements with + MBEDTLS_AES_ALT or MBEDTLS_DES_ALT. Reported by Armelle Duboc in #1092. + +Changes + * Warn if errors from AES or DES functions are ignored. This is currently + supported on GCC-like compilers and on MSVC and can be configured by + setting MBEDTLS_CHECK_RETURN in mbedtls_config.h. diff --git a/include/mbedtls/aes.h b/include/mbedtls/aes.h index 879c3f2a1..eb75935c1 100644 --- a/include/mbedtls/aes.h +++ b/include/mbedtls/aes.h @@ -163,6 +163,7 @@ void mbedtls_aes_xts_free( mbedtls_aes_xts_context *ctx ); * \return \c 0 on success. * \return #MBEDTLS_ERR_AES_INVALID_KEY_LENGTH on failure. */ +MBEDTLS_CHECK_RETURN int mbedtls_aes_setkey_enc( mbedtls_aes_context *ctx, const unsigned char *key, unsigned int keybits ); @@ -181,6 +182,7 @@ int mbedtls_aes_setkey_enc( mbedtls_aes_context *ctx, const unsigned char *key, * \return \c 0 on success. * \return #MBEDTLS_ERR_AES_INVALID_KEY_LENGTH on failure. */ +MBEDTLS_CHECK_RETURN int mbedtls_aes_setkey_dec( mbedtls_aes_context *ctx, const unsigned char *key, unsigned int keybits ); @@ -201,6 +203,7 @@ int mbedtls_aes_setkey_dec( mbedtls_aes_context *ctx, const unsigned char *key, * \return \c 0 on success. * \return #MBEDTLS_ERR_AES_INVALID_KEY_LENGTH on failure. */ +MBEDTLS_CHECK_RETURN int mbedtls_aes_xts_setkey_enc( mbedtls_aes_xts_context *ctx, const unsigned char *key, unsigned int keybits ); @@ -221,6 +224,7 @@ int mbedtls_aes_xts_setkey_enc( mbedtls_aes_xts_context *ctx, * \return \c 0 on success. * \return #MBEDTLS_ERR_AES_INVALID_KEY_LENGTH on failure. */ +MBEDTLS_CHECK_RETURN int mbedtls_aes_xts_setkey_dec( mbedtls_aes_xts_context *ctx, const unsigned char *key, unsigned int keybits ); @@ -249,6 +253,7 @@ int mbedtls_aes_xts_setkey_dec( mbedtls_aes_xts_context *ctx, * \return \c 0 on success. */ +MBEDTLS_CHECK_RETURN int mbedtls_aes_crypt_ecb( mbedtls_aes_context *ctx, int mode, const unsigned char input[16], @@ -296,6 +301,7 @@ int mbedtls_aes_crypt_ecb( mbedtls_aes_context *ctx, * \return #MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH * on failure. */ +MBEDTLS_CHECK_RETURN int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx, int mode, size_t length, @@ -340,6 +346,7 @@ int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx, * smaller than an AES block in size (16 Bytes) or if \p * length is larger than 2^20 blocks (16 MiB). */ +MBEDTLS_CHECK_RETURN int mbedtls_aes_crypt_xts( mbedtls_aes_xts_context *ctx, int mode, size_t length, @@ -388,6 +395,7 @@ int mbedtls_aes_crypt_xts( mbedtls_aes_xts_context *ctx, * * \return \c 0 on success. */ +MBEDTLS_CHECK_RETURN int mbedtls_aes_crypt_cfb128( mbedtls_aes_context *ctx, int mode, size_t length, @@ -432,6 +440,7 @@ int mbedtls_aes_crypt_cfb128( mbedtls_aes_context *ctx, * * \return \c 0 on success. */ +MBEDTLS_CHECK_RETURN int mbedtls_aes_crypt_cfb8( mbedtls_aes_context *ctx, int mode, size_t length, @@ -486,6 +495,7 @@ int mbedtls_aes_crypt_cfb8( mbedtls_aes_context *ctx, * * \return \c 0 on success. */ +MBEDTLS_CHECK_RETURN int mbedtls_aes_crypt_ofb( mbedtls_aes_context *ctx, size_t length, size_t *iv_off, @@ -572,6 +582,7 @@ int mbedtls_aes_crypt_ofb( mbedtls_aes_context *ctx, * * \return \c 0 on success. */ +MBEDTLS_CHECK_RETURN int mbedtls_aes_crypt_ctr( mbedtls_aes_context *ctx, size_t length, size_t *nc_off, @@ -592,6 +603,7 @@ int mbedtls_aes_crypt_ctr( mbedtls_aes_context *ctx, * * \return \c 0 on success. */ +MBEDTLS_CHECK_RETURN int mbedtls_internal_aes_encrypt( mbedtls_aes_context *ctx, const unsigned char input[16], unsigned char output[16] ); @@ -607,6 +619,7 @@ int mbedtls_internal_aes_encrypt( mbedtls_aes_context *ctx, * * \return \c 0 on success. */ +MBEDTLS_CHECK_RETURN int mbedtls_internal_aes_decrypt( mbedtls_aes_context *ctx, const unsigned char input[16], unsigned char output[16] ); @@ -618,6 +631,7 @@ int mbedtls_internal_aes_decrypt( mbedtls_aes_context *ctx, * \return \c 0 on success. * \return \c 1 on failure. */ +MBEDTLS_CHECK_RETURN int mbedtls_aes_self_test( int verbose ); #endif /* MBEDTLS_SELF_TEST */ diff --git a/include/mbedtls/des.h b/include/mbedtls/des.h index 63a8e00d1..7bd618c27 100644 --- a/include/mbedtls/des.h +++ b/include/mbedtls/des.h @@ -139,6 +139,7 @@ void mbedtls_des_key_set_parity( unsigned char key[MBEDTLS_DES_KEY_SIZE] ); * security risk. We recommend considering stronger ciphers * instead. */ +MBEDTLS_CHECK_RETURN int mbedtls_des_key_check_key_parity( const unsigned char key[MBEDTLS_DES_KEY_SIZE] ); /** @@ -152,6 +153,7 @@ int mbedtls_des_key_check_key_parity( const unsigned char key[MBEDTLS_DES_KEY_SI * security risk. We recommend considering stronger ciphers * instead. */ +MBEDTLS_CHECK_RETURN int mbedtls_des_key_check_weak( const unsigned char key[MBEDTLS_DES_KEY_SIZE] ); /** @@ -166,6 +168,7 @@ int mbedtls_des_key_check_weak( const unsigned char key[MBEDTLS_DES_KEY_SIZE] ); * security risk. We recommend considering stronger ciphers * instead. */ +MBEDTLS_CHECK_RETURN int mbedtls_des_setkey_enc( mbedtls_des_context *ctx, const unsigned char key[MBEDTLS_DES_KEY_SIZE] ); /** @@ -180,6 +183,7 @@ int mbedtls_des_setkey_enc( mbedtls_des_context *ctx, const unsigned char key[MB * security risk. We recommend considering stronger ciphers * instead. */ +MBEDTLS_CHECK_RETURN int mbedtls_des_setkey_dec( mbedtls_des_context *ctx, const unsigned char key[MBEDTLS_DES_KEY_SIZE] ); /** @@ -190,6 +194,7 @@ int mbedtls_des_setkey_dec( mbedtls_des_context *ctx, const unsigned char key[MB * * \return 0 */ +MBEDTLS_CHECK_RETURN int mbedtls_des3_set2key_enc( mbedtls_des3_context *ctx, const unsigned char key[MBEDTLS_DES_KEY_SIZE * 2] ); @@ -201,6 +206,7 @@ int mbedtls_des3_set2key_enc( mbedtls_des3_context *ctx, * * \return 0 */ +MBEDTLS_CHECK_RETURN int mbedtls_des3_set2key_dec( mbedtls_des3_context *ctx, const unsigned char key[MBEDTLS_DES_KEY_SIZE * 2] ); @@ -212,6 +218,7 @@ int mbedtls_des3_set2key_dec( mbedtls_des3_context *ctx, * * \return 0 */ +MBEDTLS_CHECK_RETURN int mbedtls_des3_set3key_enc( mbedtls_des3_context *ctx, const unsigned char key[MBEDTLS_DES_KEY_SIZE * 3] ); @@ -223,6 +230,7 @@ int mbedtls_des3_set3key_enc( mbedtls_des3_context *ctx, * * \return 0 */ +MBEDTLS_CHECK_RETURN int mbedtls_des3_set3key_dec( mbedtls_des3_context *ctx, const unsigned char key[MBEDTLS_DES_KEY_SIZE * 3] ); @@ -239,6 +247,7 @@ int mbedtls_des3_set3key_dec( mbedtls_des3_context *ctx, * security risk. We recommend considering stronger ciphers * instead. */ +MBEDTLS_CHECK_RETURN int mbedtls_des_crypt_ecb( mbedtls_des_context *ctx, const unsigned char input[8], unsigned char output[8] ); @@ -266,6 +275,7 @@ int mbedtls_des_crypt_ecb( mbedtls_des_context *ctx, * security risk. We recommend considering stronger ciphers * instead. */ +MBEDTLS_CHECK_RETURN int mbedtls_des_crypt_cbc( mbedtls_des_context *ctx, int mode, size_t length, @@ -283,6 +293,7 @@ int mbedtls_des_crypt_cbc( mbedtls_des_context *ctx, * * \return 0 if successful */ +MBEDTLS_CHECK_RETURN int mbedtls_des3_crypt_ecb( mbedtls_des3_context *ctx, const unsigned char input[8], unsigned char output[8] ); @@ -308,6 +319,7 @@ int mbedtls_des3_crypt_ecb( mbedtls_des3_context *ctx, * * \return 0 if successful, or MBEDTLS_ERR_DES_INVALID_INPUT_LENGTH */ +MBEDTLS_CHECK_RETURN int mbedtls_des3_crypt_cbc( mbedtls_des3_context *ctx, int mode, size_t length, @@ -338,6 +350,7 @@ void mbedtls_des_setkey( uint32_t SK[32], * * \return 0 if successful, or 1 if the test failed */ +MBEDTLS_CHECK_RETURN int mbedtls_des_self_test( int verbose ); #endif /* MBEDTLS_SELF_TEST */ diff --git a/library/aes.c b/library/aes.c index 8e3358c49..4afc3c48a 100644 --- a/library/aes.c +++ b/library/aes.c @@ -1011,6 +1011,7 @@ int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx, unsigned char *output ) { int i; + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; unsigned char temp[16]; AES_VALIDATE_RET( ctx != NULL ); @@ -1040,7 +1041,9 @@ int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx, while( length > 0 ) { memcpy( temp, input, 16 ); - mbedtls_aes_crypt_ecb( ctx, mode, input, output ); + ret = mbedtls_aes_crypt_ecb( ctx, mode, input, output ); + if( ret != 0 ) + goto exit; for( i = 0; i < 16; i++ ) output[i] = (unsigned char)( output[i] ^ iv[i] ); @@ -1059,7 +1062,9 @@ int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx, for( i = 0; i < 16; i++ ) output[i] = (unsigned char)( input[i] ^ iv[i] ); - mbedtls_aes_crypt_ecb( ctx, mode, output, output ); + ret = mbedtls_aes_crypt_ecb( ctx, mode, output, output ); + if( ret != 0 ) + goto exit; memcpy( iv, output, 16 ); input += 16; @@ -1067,8 +1072,10 @@ int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx, length -= 16; } } + ret = 0; - return( 0 ); +exit: + return( ret ); } #endif /* MBEDTLS_CIPHER_MODE_CBC */ @@ -1222,6 +1229,7 @@ int mbedtls_aes_crypt_cfb128( mbedtls_aes_context *ctx, unsigned char *output ) { int c; + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; size_t n; AES_VALIDATE_RET( ctx != NULL ); @@ -1242,7 +1250,11 @@ int mbedtls_aes_crypt_cfb128( mbedtls_aes_context *ctx, while( length-- ) { if( n == 0 ) - mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, iv, iv ); + { + ret = mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, iv, iv ); + if( ret != 0 ) + goto exit; + } c = *input++; *output++ = (unsigned char)( c ^ iv[n] ); @@ -1256,7 +1268,11 @@ int mbedtls_aes_crypt_cfb128( mbedtls_aes_context *ctx, while( length-- ) { if( n == 0 ) - mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, iv, iv ); + { + ret = mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, iv, iv ); + if( ret != 0 ) + goto exit; + } iv[n] = *output++ = (unsigned char)( iv[n] ^ *input++ ); @@ -1265,8 +1281,10 @@ int mbedtls_aes_crypt_cfb128( mbedtls_aes_context *ctx, } *iv_off = n; + ret = 0; - return( 0 ); +exit: + return( ret ); } /* @@ -1279,6 +1297,7 @@ int mbedtls_aes_crypt_cfb8( mbedtls_aes_context *ctx, const unsigned char *input, unsigned char *output ) { + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; unsigned char c; unsigned char ov[17]; @@ -1291,7 +1310,9 @@ int mbedtls_aes_crypt_cfb8( mbedtls_aes_context *ctx, while( length-- ) { memcpy( ov, iv, 16 ); - mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, iv, iv ); + ret = mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, iv, iv ); + if( ret != 0 ) + goto exit; if( mode == MBEDTLS_AES_DECRYPT ) ov[16] = *input; @@ -1303,8 +1324,10 @@ int mbedtls_aes_crypt_cfb8( mbedtls_aes_context *ctx, memcpy( iv, ov + 1, 16 ); } + ret = 0; - return( 0 ); +exit: + return( ret ); } #endif /* MBEDTLS_CIPHER_MODE_CFB */ @@ -1366,6 +1389,7 @@ int mbedtls_aes_crypt_ctr( mbedtls_aes_context *ctx, unsigned char *output ) { int c, i; + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; size_t n; AES_VALIDATE_RET( ctx != NULL ); @@ -1383,7 +1407,9 @@ int mbedtls_aes_crypt_ctr( mbedtls_aes_context *ctx, while( length-- ) { if( n == 0 ) { - mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, nonce_counter, stream_block ); + ret = mbedtls_aes_crypt_ecb( ctx, MBEDTLS_AES_ENCRYPT, nonce_counter, stream_block ); + if( ret != 0 ) + goto exit; for( i = 16; i > 0; i-- ) if( ++nonce_counter[i - 1] != 0 ) @@ -1396,8 +1422,10 @@ int mbedtls_aes_crypt_ctr( mbedtls_aes_context *ctx, } *nc_off = n; + ret = 0; - return( 0 ); +exit: + return( ret ); } #endif /* MBEDTLS_CIPHER_MODE_CTR */ diff --git a/library/des.c b/library/des.c index 7f90faa04..91d22b5d9 100644 --- a/library/des.c +++ b/library/des.c @@ -28,6 +28,7 @@ #if defined(MBEDTLS_DES_C) #include "mbedtls/des.h" +#include "mbedtls/error.h" #include "mbedtls/platform_util.h" #include @@ -642,6 +643,7 @@ int mbedtls_des_crypt_cbc( mbedtls_des_context *ctx, unsigned char *output ) { int i; + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; unsigned char temp[8]; if( length % 8 ) @@ -654,7 +656,9 @@ int mbedtls_des_crypt_cbc( mbedtls_des_context *ctx, for( i = 0; i < 8; i++ ) output[i] = (unsigned char)( input[i] ^ iv[i] ); - mbedtls_des_crypt_ecb( ctx, output, output ); + ret = mbedtls_des_crypt_ecb( ctx, output, output ); + if( ret != 0 ) + goto exit; memcpy( iv, output, 8 ); input += 8; @@ -667,7 +671,9 @@ int mbedtls_des_crypt_cbc( mbedtls_des_context *ctx, while( length > 0 ) { memcpy( temp, input, 8 ); - mbedtls_des_crypt_ecb( ctx, input, output ); + ret = mbedtls_des_crypt_ecb( ctx, input, output ); + if( ret != 0 ) + goto exit; for( i = 0; i < 8; i++ ) output[i] = (unsigned char)( output[i] ^ iv[i] ); @@ -679,8 +685,10 @@ int mbedtls_des_crypt_cbc( mbedtls_des_context *ctx, length -= 8; } } + ret = 0; - return( 0 ); +exit: + return( ret ); } #endif /* MBEDTLS_CIPHER_MODE_CBC */ @@ -741,6 +749,7 @@ int mbedtls_des3_crypt_cbc( mbedtls_des3_context *ctx, unsigned char *output ) { int i; + int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; unsigned char temp[8]; if( length % 8 ) @@ -753,7 +762,9 @@ int mbedtls_des3_crypt_cbc( mbedtls_des3_context *ctx, for( i = 0; i < 8; i++ ) output[i] = (unsigned char)( input[i] ^ iv[i] ); - mbedtls_des3_crypt_ecb( ctx, output, output ); + ret = mbedtls_des3_crypt_ecb( ctx, output, output ); + if( ret != 0 ) + goto exit; memcpy( iv, output, 8 ); input += 8; @@ -766,7 +777,9 @@ int mbedtls_des3_crypt_cbc( mbedtls_des3_context *ctx, while( length > 0 ) { memcpy( temp, input, 8 ); - mbedtls_des3_crypt_ecb( ctx, input, output ); + ret = mbedtls_des3_crypt_ecb( ctx, input, output ); + if( ret != 0 ) + goto exit; for( i = 0; i < 8; i++ ) output[i] = (unsigned char)( output[i] ^ iv[i] ); @@ -778,8 +791,10 @@ int mbedtls_des3_crypt_cbc( mbedtls_des3_context *ctx, length -= 8; } } + ret = 0; - return( 0 ); +exit: + return( ret ); } #endif /* MBEDTLS_CIPHER_MODE_CBC */ @@ -872,39 +887,43 @@ int mbedtls_des_self_test( int verbose ) switch( i ) { case 0: - mbedtls_des_setkey_dec( &ctx, des3_test_keys ); + ret = mbedtls_des_setkey_dec( &ctx, des3_test_keys ); break; case 1: - mbedtls_des_setkey_enc( &ctx, des3_test_keys ); + ret = mbedtls_des_setkey_enc( &ctx, des3_test_keys ); break; case 2: - mbedtls_des3_set2key_dec( &ctx3, des3_test_keys ); + ret = mbedtls_des3_set2key_dec( &ctx3, des3_test_keys ); break; case 3: - mbedtls_des3_set2key_enc( &ctx3, des3_test_keys ); + ret = mbedtls_des3_set2key_enc( &ctx3, des3_test_keys ); break; case 4: - mbedtls_des3_set3key_dec( &ctx3, des3_test_keys ); + ret = mbedtls_des3_set3key_dec( &ctx3, des3_test_keys ); break; case 5: - mbedtls_des3_set3key_enc( &ctx3, des3_test_keys ); + ret = mbedtls_des3_set3key_enc( &ctx3, des3_test_keys ); break; default: return( 1 ); } + if( ret != 0 ) + goto exit; for( j = 0; j < 100; j++ ) { if( u == 0 ) - mbedtls_des_crypt_ecb( &ctx, buf, buf ); + ret = mbedtls_des_crypt_ecb( &ctx, buf, buf ); else - mbedtls_des3_crypt_ecb( &ctx3, buf, buf ); + ret = mbedtls_des3_crypt_ecb( &ctx3, buf, buf ); + if( ret != 0 ) + goto exit; } if( ( v == MBEDTLS_DES_DECRYPT && @@ -947,41 +966,45 @@ int mbedtls_des_self_test( int verbose ) switch( i ) { case 0: - mbedtls_des_setkey_dec( &ctx, des3_test_keys ); + ret = mbedtls_des_setkey_dec( &ctx, des3_test_keys ); break; case 1: - mbedtls_des_setkey_enc( &ctx, des3_test_keys ); + ret = mbedtls_des_setkey_enc( &ctx, des3_test_keys ); break; case 2: - mbedtls_des3_set2key_dec( &ctx3, des3_test_keys ); + ret = mbedtls_des3_set2key_dec( &ctx3, des3_test_keys ); break; case 3: - mbedtls_des3_set2key_enc( &ctx3, des3_test_keys ); + ret = mbedtls_des3_set2key_enc( &ctx3, des3_test_keys ); break; case 4: - mbedtls_des3_set3key_dec( &ctx3, des3_test_keys ); + ret = mbedtls_des3_set3key_dec( &ctx3, des3_test_keys ); break; case 5: - mbedtls_des3_set3key_enc( &ctx3, des3_test_keys ); + ret = mbedtls_des3_set3key_enc( &ctx3, des3_test_keys ); break; default: return( 1 ); } + if( ret != 0 ) + goto exit; if( v == MBEDTLS_DES_DECRYPT ) { for( j = 0; j < 100; j++ ) { if( u == 0 ) - mbedtls_des_crypt_cbc( &ctx, v, 8, iv, buf, buf ); + ret = mbedtls_des_crypt_cbc( &ctx, v, 8, iv, buf, buf ); else - mbedtls_des3_crypt_cbc( &ctx3, v, 8, iv, buf, buf ); + ret = mbedtls_des3_crypt_cbc( &ctx3, v, 8, iv, buf, buf ); + if( ret != 0 ) + goto exit; } } else @@ -991,9 +1014,11 @@ int mbedtls_des_self_test( int verbose ) unsigned char tmp[8]; if( u == 0 ) - mbedtls_des_crypt_cbc( &ctx, v, 8, iv, buf, buf ); + ret = mbedtls_des_crypt_cbc( &ctx, v, 8, iv, buf, buf ); else - mbedtls_des3_crypt_cbc( &ctx3, v, 8, iv, buf, buf ); + ret = mbedtls_des3_crypt_cbc( &ctx3, v, 8, iv, buf, buf ); + if( ret != 0 ) + goto exit; memcpy( tmp, prv, 8 ); memcpy( prv, buf, 8 ); @@ -1027,6 +1052,8 @@ exit: mbedtls_des_free( &ctx ); mbedtls_des3_free( &ctx3 ); + if( ret != 0 ) + ret = 1; return( ret ); } diff --git a/programs/pkey/dh_client.c b/programs/pkey/dh_client.c index eccb42ad8..d633e4d1b 100644 --- a/programs/pkey/dh_client.c +++ b/programs/pkey/dh_client.c @@ -270,7 +270,9 @@ int main( void ) mbedtls_printf( "...\n . Receiving and decrypting the ciphertext" ); fflush( stdout ); - mbedtls_aes_setkey_dec( &aes, buf, 256 ); + ret = mbedtls_aes_setkey_dec( &aes, buf, 256 ); + if( ret != 0 ) + goto exit; memset( buf, 0, sizeof( buf ) ); @@ -280,7 +282,9 @@ int main( void ) goto exit; } - mbedtls_aes_crypt_ecb( &aes, MBEDTLS_AES_DECRYPT, buf, buf ); + ret = mbedtls_aes_crypt_ecb( &aes, MBEDTLS_AES_DECRYPT, buf, buf ); + if( ret != 0 ) + goto exit; buf[16] = '\0'; mbedtls_printf( "\n . Plaintext is \"%s\"\n\n", (char *) buf ); diff --git a/programs/pkey/dh_server.c b/programs/pkey/dh_server.c index 0ddb85cf8..75713ff58 100644 --- a/programs/pkey/dh_server.c +++ b/programs/pkey/dh_server.c @@ -290,9 +290,13 @@ int main( void ) mbedtls_printf( "...\n . Encrypting and sending the ciphertext" ); fflush( stdout ); - mbedtls_aes_setkey_enc( &aes, buf, 256 ); + ret = mbedtls_aes_setkey_enc( &aes, buf, 256 ); + if( ret != 0 ) + goto exit; memcpy( buf, PLAINTEXT, 16 ); - mbedtls_aes_crypt_ecb( &aes, MBEDTLS_AES_ENCRYPT, buf, buf ); + ret = mbedtls_aes_crypt_ecb( &aes, MBEDTLS_AES_ENCRYPT, buf, buf ); + if( ret != 0 ) + goto exit; if( ( ret = mbedtls_net_send( &client_fd, buf, 16 ) ) != 16 ) { diff --git a/programs/test/benchmark.c b/programs/test/benchmark.c index 5985caf0b..d3faad91e 100644 --- a/programs/test/benchmark.c +++ b/programs/test/benchmark.c @@ -674,7 +674,8 @@ int main( int argc, char *argv[] ) { mbedtls_des3_context des3; mbedtls_des3_init( &des3 ); - mbedtls_des3_set3key_enc( &des3, tmp ); + if( mbedtls_des3_set3key_enc( &des3, tmp ) != 0 ) + mbedtls_exit( 1 ); TIME_AND_TSC( "3DES", mbedtls_des3_crypt_cbc( &des3, MBEDTLS_DES_ENCRYPT, BUFSIZE, tmp, buf, buf ) ); mbedtls_des3_free( &des3 ); @@ -684,7 +685,8 @@ int main( int argc, char *argv[] ) { mbedtls_des_context des; mbedtls_des_init( &des ); - mbedtls_des_setkey_enc( &des, tmp ); + if( mbedtls_des_setkey_enc( &des, tmp ) != 0 ) + mbedtls_exit( 1 ); TIME_AND_TSC( "DES", mbedtls_des_crypt_cbc( &des, MBEDTLS_DES_ENCRYPT, BUFSIZE, tmp, buf, buf ) ); mbedtls_des_free( &des ); @@ -722,7 +724,7 @@ int main( int argc, char *argv[] ) memset( buf, 0, sizeof( buf ) ); memset( tmp, 0, sizeof( tmp ) ); - mbedtls_aes_setkey_enc( &aes, tmp, keysize ); + CHECK_AND_CONTINUE( mbedtls_aes_setkey_enc( &aes, tmp, keysize ) ); TIME_AND_TSC( title, mbedtls_aes_crypt_cbc( &aes, MBEDTLS_AES_ENCRYPT, BUFSIZE, tmp, buf, buf ) ); @@ -743,7 +745,7 @@ int main( int argc, char *argv[] ) memset( buf, 0, sizeof( buf ) ); memset( tmp, 0, sizeof( tmp ) ); - mbedtls_aes_xts_setkey_enc( &ctx, tmp, keysize * 2 ); + CHECK_AND_CONTINUE( mbedtls_aes_xts_setkey_enc( &ctx, tmp, keysize * 2 ) ); TIME_AND_TSC( title, mbedtls_aes_crypt_xts( &ctx, MBEDTLS_AES_ENCRYPT, BUFSIZE, diff --git a/tests/suites/test_suite_aes.function b/tests/suites/test_suite_aes.function index 1892c2595..52af8e02f 100644 --- a/tests/suites/test_suite_aes.function +++ b/tests/suites/test_suite_aes.function @@ -67,7 +67,7 @@ void aes_encrypt_cbc( data_t * key_str, data_t * iv_str, mbedtls_aes_init( &ctx ); - mbedtls_aes_setkey_enc( &ctx, key_str->x, key_str->len * 8 ); + TEST_ASSERT( mbedtls_aes_setkey_enc( &ctx, key_str->x, key_str->len * 8 ) == 0 ); TEST_ASSERT( mbedtls_aes_crypt_cbc( &ctx, MBEDTLS_AES_ENCRYPT, src_str->len, iv_str->x, src_str->x, output ) == cbc_result ); if( cbc_result == 0 ) { @@ -92,7 +92,7 @@ void aes_decrypt_cbc( data_t * key_str, data_t * iv_str, memset(output, 0x00, 100); mbedtls_aes_init( &ctx ); - mbedtls_aes_setkey_dec( &ctx, key_str->x, key_str->len * 8 ); + TEST_ASSERT( mbedtls_aes_setkey_dec( &ctx, key_str->x, key_str->len * 8 ) == 0 ); TEST_ASSERT( mbedtls_aes_crypt_cbc( &ctx, MBEDTLS_AES_DECRYPT, src_str->len, iv_str->x, src_str->x, output ) == cbc_result ); if( cbc_result == 0) { @@ -241,7 +241,7 @@ void aes_encrypt_cfb128( data_t * key_str, data_t * iv_str, mbedtls_aes_init( &ctx ); - mbedtls_aes_setkey_enc( &ctx, key_str->x, key_str->len * 8 ); + TEST_ASSERT( mbedtls_aes_setkey_enc( &ctx, key_str->x, key_str->len * 8 ) == 0 ); TEST_ASSERT( mbedtls_aes_crypt_cfb128( &ctx, MBEDTLS_AES_ENCRYPT, 16, &iv_offset, iv_str->x, src_str->x, output ) == 0 ); TEST_ASSERT( mbedtls_test_hexcmp( output, dst->x, 16, dst->len ) == 0 ); @@ -263,7 +263,7 @@ void aes_decrypt_cfb128( data_t * key_str, data_t * iv_str, mbedtls_aes_init( &ctx ); - mbedtls_aes_setkey_enc( &ctx, key_str->x, key_str->len * 8 ); + TEST_ASSERT( mbedtls_aes_setkey_enc( &ctx, key_str->x, key_str->len * 8 ) == 0 ); TEST_ASSERT( mbedtls_aes_crypt_cfb128( &ctx, MBEDTLS_AES_DECRYPT, 16, &iv_offset, iv_str->x, src_str->x, output ) == 0 ); TEST_ASSERT( mbedtls_test_hexcmp( output, dst->x, 16, dst->len ) == 0 ); @@ -284,7 +284,7 @@ void aes_encrypt_cfb8( data_t * key_str, data_t * iv_str, mbedtls_aes_init( &ctx ); - mbedtls_aes_setkey_enc( &ctx, key_str->x, key_str->len * 8 ); + TEST_ASSERT( mbedtls_aes_setkey_enc( &ctx, key_str->x, key_str->len * 8 ) == 0 ); TEST_ASSERT( mbedtls_aes_crypt_cfb8( &ctx, MBEDTLS_AES_ENCRYPT, src_str->len, iv_str->x, src_str->x, output ) == 0 ); TEST_ASSERT( mbedtls_test_hexcmp( output, dst->x, @@ -306,7 +306,7 @@ void aes_decrypt_cfb8( data_t * key_str, data_t * iv_str, mbedtls_aes_init( &ctx ); - mbedtls_aes_setkey_enc( &ctx, key_str->x, key_str->len * 8 ); + TEST_ASSERT( mbedtls_aes_setkey_enc( &ctx, key_str->x, key_str->len * 8 ) == 0 ); TEST_ASSERT( mbedtls_aes_crypt_cfb8( &ctx, MBEDTLS_AES_DECRYPT, src_str->len, iv_str->x, src_str->x, output ) == 0 ); TEST_ASSERT( mbedtls_test_hexcmp( output, dst->x, diff --git a/tests/suites/test_suite_des.function b/tests/suites/test_suite_des.function index 5b249355b..7256fb537 100644 --- a/tests/suites/test_suite_des.function +++ b/tests/suites/test_suite_des.function @@ -24,7 +24,7 @@ void des_encrypt_ecb( data_t * key_str, data_t * src_str, data_t * dst ) mbedtls_des_init( &ctx ); - mbedtls_des_setkey_enc( &ctx, key_str->x ); + TEST_ASSERT( mbedtls_des_setkey_enc( &ctx, key_str->x ) == 0 ); TEST_ASSERT( mbedtls_des_crypt_ecb( &ctx, src_str->x, output ) == 0 ); TEST_ASSERT( mbedtls_test_hexcmp( output, dst->x, 8, dst->len ) == 0 ); @@ -44,7 +44,7 @@ void des_decrypt_ecb( data_t * key_str, data_t * src_str, data_t * dst ) mbedtls_des_init( &ctx ); - mbedtls_des_setkey_dec( &ctx, key_str->x ); + TEST_ASSERT( mbedtls_des_setkey_dec( &ctx, key_str->x ) == 0 ); TEST_ASSERT( mbedtls_des_crypt_ecb( &ctx, src_str->x, output ) == 0 ); TEST_ASSERT( mbedtls_test_hexcmp( output, dst->x, 8, dst->len ) == 0 ); @@ -65,7 +65,7 @@ void des_encrypt_cbc( data_t * key_str, data_t * iv_str, mbedtls_des_init( &ctx ); - mbedtls_des_setkey_enc( &ctx, key_str->x ); + TEST_ASSERT( mbedtls_des_setkey_enc( &ctx, key_str->x ) == 0 ); TEST_ASSERT( mbedtls_des_crypt_cbc( &ctx, MBEDTLS_DES_ENCRYPT, src_str->len, iv_str->x, src_str->x, output ) == cbc_result ); if( cbc_result == 0 ) { @@ -91,7 +91,7 @@ void des_decrypt_cbc( data_t * key_str, data_t * iv_str, mbedtls_des_init( &ctx ); - mbedtls_des_setkey_dec( &ctx, key_str->x ); + TEST_ASSERT( mbedtls_des_setkey_dec( &ctx, key_str->x ) == 0 ); TEST_ASSERT( mbedtls_des_crypt_cbc( &ctx, MBEDTLS_DES_DECRYPT, src_str->len, iv_str->x, src_str->x, output ) == cbc_result ); if( cbc_result == 0 ) { @@ -117,9 +117,9 @@ void des3_encrypt_ecb( int key_count, data_t * key_str, if( key_count == 2 ) - mbedtls_des3_set2key_enc( &ctx, key_str->x ); + TEST_ASSERT( mbedtls_des3_set2key_enc( &ctx, key_str->x ) == 0 ); else if( key_count == 3 ) - mbedtls_des3_set3key_enc( &ctx, key_str->x ); + TEST_ASSERT( mbedtls_des3_set3key_enc( &ctx, key_str->x ) == 0 ); else TEST_ASSERT( 0 ); @@ -144,9 +144,9 @@ void des3_decrypt_ecb( int key_count, data_t * key_str, if( key_count == 2 ) - mbedtls_des3_set2key_dec( &ctx, key_str->x ); + TEST_ASSERT( mbedtls_des3_set2key_dec( &ctx, key_str->x ) == 0 ); else if( key_count == 3 ) - mbedtls_des3_set3key_dec( &ctx, key_str->x ); + TEST_ASSERT( mbedtls_des3_set3key_dec( &ctx, key_str->x ) == 0 ); else TEST_ASSERT( 0 ); @@ -172,9 +172,9 @@ void des3_encrypt_cbc( int key_count, data_t * key_str, if( key_count == 2 ) - mbedtls_des3_set2key_enc( &ctx, key_str->x ); + TEST_ASSERT( mbedtls_des3_set2key_enc( &ctx, key_str->x ) == 0 ); else if( key_count == 3 ) - mbedtls_des3_set3key_enc( &ctx, key_str->x ); + TEST_ASSERT( mbedtls_des3_set3key_enc( &ctx, key_str->x ) == 0 ); else TEST_ASSERT( 0 ); @@ -205,9 +205,9 @@ void des3_decrypt_cbc( int key_count, data_t * key_str, if( key_count == 2 ) - mbedtls_des3_set2key_dec( &ctx, key_str->x ); + TEST_ASSERT( mbedtls_des3_set2key_dec( &ctx, key_str->x ) == 0 ); else if( key_count == 3 ) - mbedtls_des3_set3key_dec( &ctx, key_str->x ); + TEST_ASSERT( mbedtls_des3_set3key_dec( &ctx, key_str->x ) == 0 ); else TEST_ASSERT( 0 ); From e35f8f6a770f5f994e9ada6b95c5b474331bc6f9 Mon Sep 17 00:00:00 2001 From: Mateusz Starzyk Date: Wed, 4 Aug 2021 15:38:09 +0200 Subject: [PATCH 03/17] Move MBEDTLS_CHECK_RETURN to platform_util. Signed-off-by: Mateusz Starzyk --- include/mbedtls/aes.h | 1 + include/mbedtls/build_info.h | 18 ------------------ include/mbedtls/des.h | 1 + include/mbedtls/platform_util.h | 20 ++++++++++++++++++++ 4 files changed, 22 insertions(+), 18 deletions(-) diff --git a/include/mbedtls/aes.h b/include/mbedtls/aes.h index eb75935c1..5c07b912d 100644 --- a/include/mbedtls/aes.h +++ b/include/mbedtls/aes.h @@ -42,6 +42,7 @@ #include "mbedtls/private_access.h" #include "mbedtls/build_info.h" +#include "mbedtls/platform_util.h" #include #include diff --git a/include/mbedtls/build_info.h b/include/mbedtls/build_info.h index 46717608b..23f85ba01 100644 --- a/include/mbedtls/build_info.h +++ b/include/mbedtls/build_info.h @@ -53,24 +53,6 @@ #define _CRT_SECURE_NO_DEPRECATE 1 #endif -/** \def MBEDTLS_CHECK_RETURN - * - * This macro appearing at the beginning of the declaration of a function - * indicates that its return value should be checked. - * - * This should appear before most functions returning an error code - * (as \c int in the \c mbedtls_xxx API or - * as ::psa_status_t in the \c psa_xxx API). - */ -#if defined(__GNUC__) -#define MBEDTLS_CHECK_RETURN __attribute__((warn_unused_result)) -#elif defined(_MSC_VER) && _MSC_VER >= 1700 -#include -#define MBEDTLS_CHECK_RETURN _Check_return_ -#else -#define MBEDTLS_CHECK_RETURN -#endif - #if !defined(MBEDTLS_CONFIG_FILE) #include "mbedtls/mbedtls_config.h" #else diff --git a/include/mbedtls/des.h b/include/mbedtls/des.h index 7bd618c27..d5289d3fc 100644 --- a/include/mbedtls/des.h +++ b/include/mbedtls/des.h @@ -29,6 +29,7 @@ #include "mbedtls/private_access.h" #include "mbedtls/build_info.h" +#include "mbedtls/platform_util.h" #include #include diff --git a/include/mbedtls/platform_util.h b/include/mbedtls/platform_util.h index 1a0a13513..9c64cfd16 100644 --- a/include/mbedtls/platform_util.h +++ b/include/mbedtls/platform_util.h @@ -60,6 +60,26 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t; #endif /* MBEDTLS_DEPRECATED_WARNING */ #endif /* MBEDTLS_DEPRECATED_REMOVED */ +/** \def MBEDTLS_CHECK_RETURN + * + * This macro appearing at the beginning of the declaration of a function + * indicates that its return value should be checked. + * + * This should appear before most functions returning an error code + * (as \c int in the \c mbedtls_xxx API or + * as ::psa_status_t in the \c psa_xxx API). + */ +#if !defined(MBEDTLS_CHECK_RETURN) +#if defined(__GNUC__) +#define MBEDTLS_CHECK_RETURN __attribute__((warn_unused_result)) +#elif defined(_MSC_VER) && _MSC_VER >= 1700 +#include +#define MBEDTLS_CHECK_RETURN _Check_return_ +#else +#define MBEDTLS_CHECK_RETURN +#endif +#endif + /** * \brief Securely zeroize a buffer * From 2a25804fd4332594ec27f80fc3bf4bdefe15dd8c Mon Sep 17 00:00:00 2001 From: Mateusz Starzyk Date: Fri, 6 Aug 2021 13:56:54 +0200 Subject: [PATCH 04/17] Add MBEDTLS_CHECK_RETURN description to mbedtls_config. Signed-off-by: Mateusz Starzyk --- include/mbedtls/mbedtls_config.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index adc317dfe..0b4a3a6b5 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -256,6 +256,18 @@ */ //#define MBEDTLS_DEPRECATED_REMOVED +/** \def MBEDTLS_CHECK_RETURN + * + * This macro appearing at the beginning of the declaration of a function + * indicates that its return value should be checked. + * + * Default implementation resides in platform_util.h. + * You can override default implementation by defining your own. + * Custom implementation can be empty, which will disable checking + * of functions' return values. + */ +//#define MBEDTLS_CHECK_RETURN + /* \} name SECTION: System support */ /** From ea59237370fcaf04b3e68501d899e4b33cc38b76 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 23 Sep 2021 17:08:26 +0200 Subject: [PATCH 05/17] Move changelog entry to the appropriate directory Signed-off-by: Gilles Peskine --- check-return.txt => ChangeLog.d/check-return.txt | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename check-return.txt => ChangeLog.d/check-return.txt (100%) diff --git a/check-return.txt b/ChangeLog.d/check-return.txt similarity index 100% rename from check-return.txt rename to ChangeLog.d/check-return.txt From 463adf4536b4b04d85ac91eca2b66fe89a35307f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 23 Sep 2021 17:28:59 +0200 Subject: [PATCH 06/17] Define indirection macros MBEDTLS_CHECK_RETURN_xxx Define macros MBEDTLS_CHECK_RETURN_CRITICAL, MBEDTLS_CHECK_RETURN_TYPICAL and MBEDTLS_CHECK_RETURN_OPTIONAL so that we can indicate on a function-by-function basis whether checking the function's return value is almost always necessary (CRITICAL), typically necessary in portable applications but unnecessary in some reasonable cases (TYPICAL), or typically unnecessary (OPTIONAL). Update the documentation of MBEDTLS_CHECK_RETURN accordingly. This is split between the user documentation (Doxygen, in config.h) and the internal documentation (non-Doxygen, in platform_util.h, of minor importance since the macro isn't meant to be used directly). Signed-off-by: Gilles Peskine --- include/mbedtls/mbedtls_config.h | 15 +++++---- include/mbedtls/platform_util.h | 58 ++++++++++++++++++++++++++++---- 2 files changed, 60 insertions(+), 13 deletions(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 0b4a3a6b5..a840c186a 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -258,13 +258,16 @@ /** \def MBEDTLS_CHECK_RETURN * - * This macro appearing at the beginning of the declaration of a function - * indicates that its return value should be checked. + * This macro is used at the beginning of the declaration of a function + * to indicate that its return value should be checked. It should + * instruct the compiler to emit a warning or an error if the function + * is called without checking its return value. * - * Default implementation resides in platform_util.h. - * You can override default implementation by defining your own. - * Custom implementation can be empty, which will disable checking - * of functions' return values. + * There is a default implementation for popular compilers in platform_util.h. + * You can override the default implementation by defining your own here. + * + * If the implementation here is empty, this will effectively disable the + * checking of functions' return values. */ //#define MBEDTLS_CHECK_RETURN diff --git a/include/mbedtls/platform_util.h b/include/mbedtls/platform_util.h index 9c64cfd16..3a28dac9a 100644 --- a/include/mbedtls/platform_util.h +++ b/include/mbedtls/platform_util.h @@ -60,14 +60,12 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t; #endif /* MBEDTLS_DEPRECATED_WARNING */ #endif /* MBEDTLS_DEPRECATED_REMOVED */ -/** \def MBEDTLS_CHECK_RETURN +/* Implementation of the check-return facility. + * See the user documentation in mbedtls_config.h. * - * This macro appearing at the beginning of the declaration of a function - * indicates that its return value should be checked. - * - * This should appear before most functions returning an error code - * (as \c int in the \c mbedtls_xxx API or - * as ::psa_status_t in the \c psa_xxx API). + * Do not use this macro directly to annotate function: instead, + * use one of MBEDTLS_CHECK_RETURN_CRITICAL or MBEDTLS_CHECK_RETURN_TYPICAL + * depending on how important it is to check the return value. */ #if !defined(MBEDTLS_CHECK_RETURN) #if defined(__GNUC__) @@ -80,6 +78,52 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t; #endif #endif +/** Critical-failure function + * + * This macro appearing at the beginning of the declaration of a function + * indicates that its return value should be checked in all applications. + * Omitting the check is very likely to indicate a bug in the application + * and will result in a compile-time warning if #MBEDTLS_CHECK_RETURN + * is implemented for the compiler in use. + * + * \note The use of this macro is a work in progress. + * This macro may be added to more functions in the future. + * Such an extension is not considered an API break, provided that + * there are near-unavoidable circumstances under which the function + * can fail. For example, signature/MAC/AEAD verification functions, + * and functions that require a random generator, are considered + * return-check-critical. + */ +#define MBEDTLS_CHECK_RETURN_CRITICAL MBEDTLS_CHECK_RETURN + +/** Ordinary-failure function + * + * This macro appearing at the beginning of the declaration of a function + * indicates that its return value should be generally be checked in portable + * applications. Omitting the check will result in a compile-time warning if + * #MBEDTLS_CHECK_RETURN is implemented for the compiler in use. + * + * \note The use of this macro is a work in progress. + * This macro will be added to more functions in the future. + * Eventually this should appear before most functions returning + * an error code (as \c int in the \c mbedtls_xxx API or + * as ::psa_status_t in the \c psa_xxx API). + */ +#define MBEDTLS_CHECK_RETURN_TYPICAL MBEDTLS_CHECK_RETURN + +/** Benign-failure function + * + * This macro appearing at the beginning of the declaration of a function + * indicates that it is rarely useful to check its return value. + * + * This macro has an empty expansion. It exists for documentation purposes: + * a #MBEDTLS_CHECK_RETURN_OPTIONAL annotation indicates that the function + * has been analyzed for return-check usefuless, whereas the lack of + * an annotation indicates that the function has not been analyzed and its + * return-check usefulness is unknown. + */ +#define MBEDTLS_CHECK_RETURN_OPTIONAL + /** * \brief Securely zeroize a buffer * From e41803af9cba541de0aeb06d9e6e385649a5cb1c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 23 Sep 2021 17:35:37 +0200 Subject: [PATCH 07/17] Change DES and AES functions to MBEDTLS_CHECK_RETURN_TYPICAL For all of these functions, the only possible failures are a hardware accelerator (not possible unless using an ALT implementation), an internal error or runtime corruption. Exception: the self-tests, which serve little purpose if their status isn't tested. Signed-off-by: Gilles Peskine --- include/mbedtls/aes.h | 28 ++++++++++++++-------------- include/mbedtls/des.h | 26 +++++++++++++------------- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/include/mbedtls/aes.h b/include/mbedtls/aes.h index 5c07b912d..becbfae1d 100644 --- a/include/mbedtls/aes.h +++ b/include/mbedtls/aes.h @@ -164,7 +164,7 @@ void mbedtls_aes_xts_free( mbedtls_aes_xts_context *ctx ); * \return \c 0 on success. * \return #MBEDTLS_ERR_AES_INVALID_KEY_LENGTH on failure. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_aes_setkey_enc( mbedtls_aes_context *ctx, const unsigned char *key, unsigned int keybits ); @@ -183,7 +183,7 @@ int mbedtls_aes_setkey_enc( mbedtls_aes_context *ctx, const unsigned char *key, * \return \c 0 on success. * \return #MBEDTLS_ERR_AES_INVALID_KEY_LENGTH on failure. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_aes_setkey_dec( mbedtls_aes_context *ctx, const unsigned char *key, unsigned int keybits ); @@ -204,7 +204,7 @@ int mbedtls_aes_setkey_dec( mbedtls_aes_context *ctx, const unsigned char *key, * \return \c 0 on success. * \return #MBEDTLS_ERR_AES_INVALID_KEY_LENGTH on failure. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_aes_xts_setkey_enc( mbedtls_aes_xts_context *ctx, const unsigned char *key, unsigned int keybits ); @@ -225,7 +225,7 @@ int mbedtls_aes_xts_setkey_enc( mbedtls_aes_xts_context *ctx, * \return \c 0 on success. * \return #MBEDTLS_ERR_AES_INVALID_KEY_LENGTH on failure. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_aes_xts_setkey_dec( mbedtls_aes_xts_context *ctx, const unsigned char *key, unsigned int keybits ); @@ -254,7 +254,7 @@ int mbedtls_aes_xts_setkey_dec( mbedtls_aes_xts_context *ctx, * \return \c 0 on success. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_aes_crypt_ecb( mbedtls_aes_context *ctx, int mode, const unsigned char input[16], @@ -302,7 +302,7 @@ int mbedtls_aes_crypt_ecb( mbedtls_aes_context *ctx, * \return #MBEDTLS_ERR_AES_INVALID_INPUT_LENGTH * on failure. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx, int mode, size_t length, @@ -347,7 +347,7 @@ int mbedtls_aes_crypt_cbc( mbedtls_aes_context *ctx, * smaller than an AES block in size (16 Bytes) or if \p * length is larger than 2^20 blocks (16 MiB). */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_aes_crypt_xts( mbedtls_aes_xts_context *ctx, int mode, size_t length, @@ -396,7 +396,7 @@ int mbedtls_aes_crypt_xts( mbedtls_aes_xts_context *ctx, * * \return \c 0 on success. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_aes_crypt_cfb128( mbedtls_aes_context *ctx, int mode, size_t length, @@ -441,7 +441,7 @@ int mbedtls_aes_crypt_cfb128( mbedtls_aes_context *ctx, * * \return \c 0 on success. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_aes_crypt_cfb8( mbedtls_aes_context *ctx, int mode, size_t length, @@ -496,7 +496,7 @@ int mbedtls_aes_crypt_cfb8( mbedtls_aes_context *ctx, * * \return \c 0 on success. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_aes_crypt_ofb( mbedtls_aes_context *ctx, size_t length, size_t *iv_off, @@ -583,7 +583,7 @@ int mbedtls_aes_crypt_ofb( mbedtls_aes_context *ctx, * * \return \c 0 on success. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_aes_crypt_ctr( mbedtls_aes_context *ctx, size_t length, size_t *nc_off, @@ -604,7 +604,7 @@ int mbedtls_aes_crypt_ctr( mbedtls_aes_context *ctx, * * \return \c 0 on success. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_internal_aes_encrypt( mbedtls_aes_context *ctx, const unsigned char input[16], unsigned char output[16] ); @@ -620,7 +620,7 @@ int mbedtls_internal_aes_encrypt( mbedtls_aes_context *ctx, * * \return \c 0 on success. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_internal_aes_decrypt( mbedtls_aes_context *ctx, const unsigned char input[16], unsigned char output[16] ); @@ -632,7 +632,7 @@ int mbedtls_internal_aes_decrypt( mbedtls_aes_context *ctx, * \return \c 0 on success. * \return \c 1 on failure. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_CRITICAL int mbedtls_aes_self_test( int verbose ); #endif /* MBEDTLS_SELF_TEST */ diff --git a/include/mbedtls/des.h b/include/mbedtls/des.h index d5289d3fc..be74cb111 100644 --- a/include/mbedtls/des.h +++ b/include/mbedtls/des.h @@ -140,7 +140,7 @@ void mbedtls_des_key_set_parity( unsigned char key[MBEDTLS_DES_KEY_SIZE] ); * security risk. We recommend considering stronger ciphers * instead. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_des_key_check_key_parity( const unsigned char key[MBEDTLS_DES_KEY_SIZE] ); /** @@ -154,7 +154,7 @@ int mbedtls_des_key_check_key_parity( const unsigned char key[MBEDTLS_DES_KEY_SI * security risk. We recommend considering stronger ciphers * instead. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_des_key_check_weak( const unsigned char key[MBEDTLS_DES_KEY_SIZE] ); /** @@ -169,7 +169,7 @@ int mbedtls_des_key_check_weak( const unsigned char key[MBEDTLS_DES_KEY_SIZE] ); * security risk. We recommend considering stronger ciphers * instead. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_des_setkey_enc( mbedtls_des_context *ctx, const unsigned char key[MBEDTLS_DES_KEY_SIZE] ); /** @@ -184,7 +184,7 @@ int mbedtls_des_setkey_enc( mbedtls_des_context *ctx, const unsigned char key[MB * security risk. We recommend considering stronger ciphers * instead. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_des_setkey_dec( mbedtls_des_context *ctx, const unsigned char key[MBEDTLS_DES_KEY_SIZE] ); /** @@ -195,7 +195,7 @@ int mbedtls_des_setkey_dec( mbedtls_des_context *ctx, const unsigned char key[MB * * \return 0 */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_des3_set2key_enc( mbedtls_des3_context *ctx, const unsigned char key[MBEDTLS_DES_KEY_SIZE * 2] ); @@ -207,7 +207,7 @@ int mbedtls_des3_set2key_enc( mbedtls_des3_context *ctx, * * \return 0 */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_des3_set2key_dec( mbedtls_des3_context *ctx, const unsigned char key[MBEDTLS_DES_KEY_SIZE * 2] ); @@ -219,7 +219,7 @@ int mbedtls_des3_set2key_dec( mbedtls_des3_context *ctx, * * \return 0 */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_des3_set3key_enc( mbedtls_des3_context *ctx, const unsigned char key[MBEDTLS_DES_KEY_SIZE * 3] ); @@ -231,7 +231,7 @@ int mbedtls_des3_set3key_enc( mbedtls_des3_context *ctx, * * \return 0 */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_des3_set3key_dec( mbedtls_des3_context *ctx, const unsigned char key[MBEDTLS_DES_KEY_SIZE * 3] ); @@ -248,7 +248,7 @@ int mbedtls_des3_set3key_dec( mbedtls_des3_context *ctx, * security risk. We recommend considering stronger ciphers * instead. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_des_crypt_ecb( mbedtls_des_context *ctx, const unsigned char input[8], unsigned char output[8] ); @@ -276,7 +276,7 @@ int mbedtls_des_crypt_ecb( mbedtls_des_context *ctx, * security risk. We recommend considering stronger ciphers * instead. */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_des_crypt_cbc( mbedtls_des_context *ctx, int mode, size_t length, @@ -294,7 +294,7 @@ int mbedtls_des_crypt_cbc( mbedtls_des_context *ctx, * * \return 0 if successful */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_des3_crypt_ecb( mbedtls_des3_context *ctx, const unsigned char input[8], unsigned char output[8] ); @@ -320,7 +320,7 @@ int mbedtls_des3_crypt_ecb( mbedtls_des3_context *ctx, * * \return 0 if successful, or MBEDTLS_ERR_DES_INVALID_INPUT_LENGTH */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_TYPICAL int mbedtls_des3_crypt_cbc( mbedtls_des3_context *ctx, int mode, size_t length, @@ -351,7 +351,7 @@ void mbedtls_des_setkey( uint32_t SK[32], * * \return 0 if successful, or 1 if the test failed */ -MBEDTLS_CHECK_RETURN +MBEDTLS_CHECK_RETURN_CRITICAL int mbedtls_des_self_test( int verbose ); #endif /* MBEDTLS_SELF_TEST */ From 3f106f762dc0c65583da54c4ed3328ed18718570 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 23 Sep 2021 17:42:39 +0200 Subject: [PATCH 08/17] Move MBEDTLS_CHECK_RETURN to the correct section This is not a boolean macro: it's useful for what it expands to. Signed-off-by: Gilles Peskine --- include/mbedtls/mbedtls_config.h | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index a840c186a..2110cc27b 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -256,21 +256,6 @@ */ //#define MBEDTLS_DEPRECATED_REMOVED -/** \def MBEDTLS_CHECK_RETURN - * - * This macro is used at the beginning of the declaration of a function - * to indicate that its return value should be checked. It should - * instruct the compiler to emit a warning or an error if the function - * is called without checking its return value. - * - * There is a default implementation for popular compilers in platform_util.h. - * You can override the default implementation by defining your own here. - * - * If the implementation here is empty, this will effectively disable the - * checking of functions' return values. - */ -//#define MBEDTLS_CHECK_RETURN - /* \} name SECTION: System support */ /** @@ -3082,6 +3067,21 @@ //#define MBEDTLS_PLATFORM_NV_SEED_READ_MACRO mbedtls_platform_std_nv_seed_read /**< Default nv_seed_read function to use, can be undefined */ //#define MBEDTLS_PLATFORM_NV_SEED_WRITE_MACRO mbedtls_platform_std_nv_seed_write /**< Default nv_seed_write function to use, can be undefined */ +/** \def MBEDTLS_CHECK_RETURN + * + * This macro is used at the beginning of the declaration of a function + * to indicate that its return value should be checked. It should + * instruct the compiler to emit a warning or an error if the function + * is called without checking its return value. + * + * There is a default implementation for popular compilers in platform_util.h. + * You can override the default implementation by defining your own here. + * + * If the implementation here is empty, this will effectively disable the + * checking of functions' return values. + */ +//#define MBEDTLS_CHECK_RETURN + /* PSA options */ /** * Use HMAC_DRBG with the specified hash algorithm for HMAC_DRBG for the From 913fc5fff3831057f03b1de529c76ecd65e34b26 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 23 Sep 2021 17:43:46 +0200 Subject: [PATCH 09/17] Better default for MBEDTLS_CHECK_RETURN in config.h An empty expansion is possible, but as documented its effect is to disable the feature, so that isn't a good example. Instead, use the GCC implementation as the default: it's plausible that it could work even on compilers that don't advertise themselves as sufficiently GCC-like to define __GNUC__, and if not it gives users a concrete idea of what the macro is supposed to do. Signed-off-by: Gilles Peskine --- include/mbedtls/mbedtls_config.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 2110cc27b..13c86ed58 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -3080,7 +3080,7 @@ * If the implementation here is empty, this will effectively disable the * checking of functions' return values. */ -//#define MBEDTLS_CHECK_RETURN +//#define MBEDTLS_CHECK_RETURN __attribute__(__warn_unused_result__) /* PSA options */ /** From a33e6935bc061a897a62a5b4ef053693aaf754bd Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 23 Sep 2021 17:46:12 +0200 Subject: [PATCH 10/17] Use reserved identifier for warn_unused_result This is normally equivalent, but works even if some other header defines a macro called warn_unused_result. Signed-off-by: Gilles Peskine --- include/mbedtls/platform_util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/platform_util.h b/include/mbedtls/platform_util.h index 3a28dac9a..d8379deb3 100644 --- a/include/mbedtls/platform_util.h +++ b/include/mbedtls/platform_util.h @@ -69,7 +69,7 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t; */ #if !defined(MBEDTLS_CHECK_RETURN) #if defined(__GNUC__) -#define MBEDTLS_CHECK_RETURN __attribute__((warn_unused_result)) +#define MBEDTLS_CHECK_RETURN __attribute__((__warn_unused_result__)) #elif defined(_MSC_VER) && _MSC_VER >= 1700 #include #define MBEDTLS_CHECK_RETURN _Check_return_ From 9a7d4c273470fdfb5a33566f8e2b9e36fc8b29d2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 23 Sep 2021 18:07:36 +0200 Subject: [PATCH 11/17] New configuration option MBEDTLS_CHECK_RETURN_WARNING MBEDTLS_CHECK_RETURN_TYPICAL defaults off, but is enabled if MBEDTLS_CHECK_RETURN_WARNING is enabled at compile time. (MBEDTLS_CHECK_RETURN_CRITICAL is always enabled.) The default is off so that a plausible program that builds with one version of Mbed TLS in the default configuration will still build under the next version. Signed-off-by: Gilles Peskine --- ChangeLog.d/check-return.txt | 13 +++++++++---- include/mbedtls/mbedtls_config.h | 23 +++++++++++++++++++++++ include/mbedtls/platform_util.h | 4 ++++ 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/ChangeLog.d/check-return.txt b/ChangeLog.d/check-return.txt index e6371ec69..045b1805e 100644 --- a/ChangeLog.d/check-return.txt +++ b/ChangeLog.d/check-return.txt @@ -6,7 +6,12 @@ Bugfix where this function cannot fail, or full-module replacements with MBEDTLS_AES_ALT or MBEDTLS_DES_ALT. Reported by Armelle Duboc in #1092. -Changes - * Warn if errors from AES or DES functions are ignored. This is currently - supported on GCC-like compilers and on MSVC and can be configured by - setting MBEDTLS_CHECK_RETURN in mbedtls_config.h. +Features + * Warn if errors from certain functions are ignored. This is currently + supported on GCC-like compilers and on MSVC and can be configured through + the macro MBEDTLS_CHECK_RETURN. The warnings are always enabled + (where supported) for critical functions where ignoring the return + value is almost always a bug. Enable the new configuration option + MBEDTLS_CHECK_RETURN_WARNING to get warnings for other functions. This + is currently implemented in the AES and DES modules, and will be extended + to other modules in the future. diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index 13c86ed58..c31a2cee5 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -520,6 +520,29 @@ */ //#define MBEDTLS_CAMELLIA_SMALL_MEMORY +/** + * \def MBEDTLS_CHECK_RETURN_WARNING + * + * If this macro is defined, emit a compile-time warning if application code + * calls a function without checking its return value, but the return value + * should generally be checked in portable applications. + * + * This is only supported on platforms where #MBEDTLS_CHECK_RETURN is + * implemented. Otherwise this option has no effect. + * + * Uncomment to get warnings on using fallible functions without checking + * their return value. + * + * \note This feature is a work in progress. + * Warnings will be added to more functions in the future. + * + * \note A few functions are considered critical, and ignoring the return + * value of these functions will trigger a warning even if this + * macro is not defined. To completely disable return value check + * warnings, define #MBEDTLS_CHECK_RETURN with an empty expansion. + */ +//#define MBEDTLS_CHECK_RETURN_WARNING + /** * \def MBEDTLS_CIPHER_MODE_CBC * diff --git a/include/mbedtls/platform_util.h b/include/mbedtls/platform_util.h index d8379deb3..ce0611da5 100644 --- a/include/mbedtls/platform_util.h +++ b/include/mbedtls/platform_util.h @@ -109,7 +109,11 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t; * an error code (as \c int in the \c mbedtls_xxx API or * as ::psa_status_t in the \c psa_xxx API). */ +#if defined(MBEDTLS_CHECK_RETURN_WARNING) #define MBEDTLS_CHECK_RETURN_TYPICAL MBEDTLS_CHECK_RETURN +#else +#define MBEDTLS_CHECK_RETURN_TYPICAL +#endif /** Benign-failure function * From 409fbbe4a2128c961c28d8f885a08afe6b2bd2fe Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 27 Sep 2021 16:17:51 +0200 Subject: [PATCH 12/17] Minor documentation fix Signed-off-by: Gilles Peskine --- include/mbedtls/platform_util.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/platform_util.h b/include/mbedtls/platform_util.h index ce0611da5..e1f063c34 100644 --- a/include/mbedtls/platform_util.h +++ b/include/mbedtls/platform_util.h @@ -101,7 +101,8 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t; * This macro appearing at the beginning of the declaration of a function * indicates that its return value should be generally be checked in portable * applications. Omitting the check will result in a compile-time warning if - * #MBEDTLS_CHECK_RETURN is implemented for the compiler in use. + * #MBEDTLS_CHECK_RETURN is implemented for the compiler in use and + * #MBEDTLS_CHECK_RETURN_WARNING is enabled in the compile-time configuration. * * \note The use of this macro is a work in progress. * This macro will be added to more functions in the future. From 5c4ca32f936753bfe586649b28abea74b81d285f Mon Sep 17 00:00:00 2001 From: Mateusz Starzyk Date: Thu, 5 Aug 2021 13:56:48 +0200 Subject: [PATCH 13/17] Silence warnings about unused return value This macro is not used inside the library yet, but may be used in deprecated functions in the future, if a function returning void has to change to returning an error. It may also be useful in user code, so it is in a public header. Signed-off-by: Mateusz Starzyk Signed-off-by: Gilles Peskine --- include/mbedtls/platform_util.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/include/mbedtls/platform_util.h b/include/mbedtls/platform_util.h index e1f063c34..0882dc68d 100644 --- a/include/mbedtls/platform_util.h +++ b/include/mbedtls/platform_util.h @@ -129,6 +129,13 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t; */ #define MBEDTLS_CHECK_RETURN_OPTIONAL +/** \def MBEDTLS_IGNORE_RETURN + * + * Silences warning about unused return value given by functions + * with \c MBEDTLS_CHECK_RETURN attribute. + */ +#define MBEDTLS_IGNORE_RETURN(result) if( result ) {} + /** * \brief Securely zeroize a buffer * From cd79dfc4bbfb8b89d4ba9d12024a7e1d90c01cef Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 30 Sep 2021 18:53:36 +0200 Subject: [PATCH 14/17] Fix mistake in the sample implementation of MBEDTLS_CHECK_RETURN Signed-off-by: Gilles Peskine --- include/mbedtls/mbedtls_config.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index c31a2cee5..f7d06d03a 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -3103,7 +3103,7 @@ * If the implementation here is empty, this will effectively disable the * checking of functions' return values. */ -//#define MBEDTLS_CHECK_RETURN __attribute__(__warn_unused_result__) +//#define MBEDTLS_CHECK_RETURN __attribute__((__warn_unused_result__)) /* PSA options */ /** From 252b758dd6a990a6c69e3a8d3a99005977a50407 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 30 Sep 2021 18:54:51 +0200 Subject: [PATCH 15/17] Cleaner implementation of MBEDTLS_IGNORE_RETURN The previous implementation was misparsed in constructs like `if (condition) MBEDTLS_IGNORE_RETURN(...); else ...;`. Implement it as an expression, tested with GCC, Clang and MSVC. Signed-off-by: Gilles Peskine --- include/mbedtls/platform_util.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/platform_util.h b/include/mbedtls/platform_util.h index 0882dc68d..d632eed4f 100644 --- a/include/mbedtls/platform_util.h +++ b/include/mbedtls/platform_util.h @@ -133,8 +133,15 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t; * * Silences warning about unused return value given by functions * with \c MBEDTLS_CHECK_RETURN attribute. +/* GCC doesn't silence the warning with just (void)(result). + * !(void)(result) is known to work up at least up to GCC 10, as well + * as with Clang and MSVC. + * + * https://gcc.gnu.org/onlinedocs/gcc-3.4.6/gcc/Non_002dbugs.html + * https://stackoverflow.com/questions/40576003/ignoring-warning-wunused-result + * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425#c34 */ -#define MBEDTLS_IGNORE_RETURN(result) if( result ) {} +#define MBEDTLS_IGNORE_RETURN(result) ( (void) !( result ) ) /** * \brief Securely zeroize a buffer From fcc93d797b9fdb99900bf274356d38795ffe915e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 30 Sep 2021 18:56:17 +0200 Subject: [PATCH 16/17] Make MBEDTLS_IGNORE_RETURN configurable Signed-off-by: Gilles Peskine --- include/mbedtls/mbedtls_config.h | 8 ++++++++ include/mbedtls/platform_util.h | 10 ++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/mbedtls_config.h b/include/mbedtls/mbedtls_config.h index f7d06d03a..4757aa66c 100644 --- a/include/mbedtls/mbedtls_config.h +++ b/include/mbedtls/mbedtls_config.h @@ -3105,6 +3105,14 @@ */ //#define MBEDTLS_CHECK_RETURN __attribute__((__warn_unused_result__)) +/** \def MBEDTLS_IGNORE_RETURN + * + * This macro requires one argument, which should be a C function call. + * If that function call would cause a #MBEDTLS_CHECK_RETURN warning, this + * warning is suppressed. + */ +//#define MBEDTLS_IGNORE_RETURN( result ) ((void) !(result)) + /* PSA options */ /** * Use HMAC_DRBG with the specified hash algorithm for HMAC_DRBG for the diff --git a/include/mbedtls/platform_util.h b/include/mbedtls/platform_util.h index d632eed4f..6f6b6967a 100644 --- a/include/mbedtls/platform_util.h +++ b/include/mbedtls/platform_util.h @@ -104,6 +104,9 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t; * #MBEDTLS_CHECK_RETURN is implemented for the compiler in use and * #MBEDTLS_CHECK_RETURN_WARNING is enabled in the compile-time configuration. * + * You can use #MBEDTLS_IGNORE_RETURN to explicitly ignore the return value + * of a function that is annotated with #MBEDTLS_CHECK_RETURN. + * * \note The use of this macro is a work in progress. * This macro will be added to more functions in the future. * Eventually this should appear before most functions returning @@ -131,8 +134,10 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t; /** \def MBEDTLS_IGNORE_RETURN * - * Silences warning about unused return value given by functions - * with \c MBEDTLS_CHECK_RETURN attribute. + * Call this macro with one argument, a function call, to suppress a warning + * from #MBEDTLS_CHECK_RETURN due to that function call. + */ +#if !defined(MBEDTLS_IGNORE_RETURN) /* GCC doesn't silence the warning with just (void)(result). * !(void)(result) is known to work up at least up to GCC 10, as well * as with Clang and MSVC. @@ -142,6 +147,7 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t; * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425#c34 */ #define MBEDTLS_IGNORE_RETURN(result) ( (void) !( result ) ) +#endif /** * \brief Securely zeroize a buffer From 2aefc9ef2e08157a848e8ce2200b0a7b8d0640d0 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 30 Sep 2021 20:34:29 +0200 Subject: [PATCH 17/17] Fix typo in comment Signed-off-by: Gilles Peskine --- include/mbedtls/platform_util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/platform_util.h b/include/mbedtls/platform_util.h index 6f6b6967a..36e3718e6 100644 --- a/include/mbedtls/platform_util.h +++ b/include/mbedtls/platform_util.h @@ -139,7 +139,7 @@ MBEDTLS_DEPRECATED typedef int mbedtls_deprecated_numeric_constant_t; */ #if !defined(MBEDTLS_IGNORE_RETURN) /* GCC doesn't silence the warning with just (void)(result). - * !(void)(result) is known to work up at least up to GCC 10, as well + * (void)!(result) is known to work up at least up to GCC 10, as well * as with Clang and MSVC. * * https://gcc.gnu.org/onlinedocs/gcc-3.4.6/gcc/Non_002dbugs.html