From b6d5693be256e3e0e3b0489186c46654ff4040a1 Mon Sep 17 00:00:00 2001 From: Nick Child Date: Tue, 25 May 2021 15:31:21 -0400 Subject: [PATCH 1/2] pk.c: Ensure hash_len equals hash in pk_hashlen_helper The function `pk_hashlen_helper` exists to ensure a valid hash_len is used in pk_verify and pk_sign functions. This function has been used to adjust to the corrsponding hash_len if the user passes in 0 for the hash_len argument based on the md algorithm given. If the user does not pass in 0 as the hash_len, then it is not adjusted. This is problematic if the user gives a hash_len and hash buffer that is less than the associated length of the md algorithm. This error would go unchecked and eventually lead to buffer overread when given to specific pk_sign/verify functions, since they both ignore the hash_len argument if md_alg is not MBEDTLS_MD_NONE. This commit, adds a conditional to `pk_hashlen_helper` so that an error is thrown if the user specifies a hash_length (not 0) and it is not equal to the expected for the associated message digest algorithm. This aligns better with the api documentation where it states "If hash_len is 0, then the length associated with md_alg is used instead, or an error returned if it is invalid" Signed-off-by: Nick Child Signed-off-by: Nayna Jain --- ChangeLog.d/ensure_hash_len_is_valid.txt | 5 +++++ library/pk.c | 5 ++++- 2 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 ChangeLog.d/ensure_hash_len_is_valid.txt diff --git a/ChangeLog.d/ensure_hash_len_is_valid.txt b/ChangeLog.d/ensure_hash_len_is_valid.txt new file mode 100644 index 000000000..d3e293066 --- /dev/null +++ b/ChangeLog.d/ensure_hash_len_is_valid.txt @@ -0,0 +1,5 @@ +Bugfix + * mbedtls_pk_sign() and mbedtls_pk_verify() and their extended and + restartable variants now always honor the specified hash length if + nonzero. Before, for RSA, hash_len was ignored in favor of the length of + the specified hash algorithm. diff --git a/library/pk.c b/library/pk.c index ecf002d45..05cc2134f 100644 --- a/library/pk.c +++ b/library/pk.c @@ -235,12 +235,15 @@ static inline int pk_hashlen_helper( mbedtls_md_type_t md_alg, size_t *hash_len { const mbedtls_md_info_t *md_info; - if( *hash_len != 0 ) + if( *hash_len != 0 && md_alg == MBEDTLS_MD_NONE ) return( 0 ); if( ( md_info = mbedtls_md_info_from_type( md_alg ) ) == NULL ) return( -1 ); + if ( *hash_len != 0 && *hash_len != mbedtls_md_get_size( md_info ) ) + return ( -1 ); + *hash_len = mbedtls_md_get_size( md_info ); return( 0 ); } From 8930e14f3a8f03f1e6485dc2d42ebc4334c97e9c Mon Sep 17 00:00:00 2001 From: Nick Child Date: Thu, 17 Jun 2021 11:59:29 -0400 Subject: [PATCH 2/2] test_suite_pk.function: Do not use MD_MAX_SIZE In order to for tests to pass from the previous commit (which it mandatory for all pk verify/sign functions to be given a hash_len that is exactly equal to the message digest length of md_alg) the hash_len that is supplied to the fucntion cannot be MBEDTLS_MD_MAX_SIZE. This would result in all tests failing. Since the md alg for all of these funtions are SHA256, we can use mbedtls functions to get the required length of a SHA256 digest (32 bytes). Then that number can be used for allocating the hash buffer. Signed-off-by: Nick Child --- tests/suites/test_suite_pk.function | 41 ++++++++++++++++------------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index a249ba251..82d15738d 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -883,8 +883,9 @@ exit: void pk_sign_verify( int type, int parameter, int sign_ret, int verify_ret ) { mbedtls_pk_context pk; - size_t sig_len; - unsigned char hash[MBEDTLS_MD_MAX_SIZE]; + size_t sig_len, hash_len; + mbedtls_md_type_t md = MBEDTLS_MD_SHA256; + unsigned char *hash = NULL; unsigned char sig[MBEDTLS_PK_SIGNATURE_MAX_SIZE]; void *rs_ctx = NULL; #if defined(MBEDTLS_ECDSA_C) && defined(MBEDTLS_ECP_RESTARTABLE) @@ -898,40 +899,43 @@ void pk_sign_verify( int type, int parameter, int sign_ret, int verify_ret ) mbedtls_ecp_set_max_ops( 42000 ); #endif + hash_len = mbedtls_md_get_size( mbedtls_md_info_from_type( md ) ); + ASSERT_ALLOC( hash, hash_len ); + mbedtls_pk_init( &pk ); USE_PSA_INIT( ); - memset( hash, 0x2a, sizeof hash ); + memset( hash, 0x2a, hash_len ); memset( sig, 0, sizeof sig ); TEST_ASSERT( mbedtls_pk_setup( &pk, mbedtls_pk_info_from_type( type ) ) == 0 ); TEST_ASSERT( pk_genkey( &pk, parameter ) == 0 ); - TEST_ASSERT( mbedtls_pk_sign_restartable( &pk, MBEDTLS_MD_SHA256, - hash, sizeof hash, sig, &sig_len, + TEST_ASSERT( mbedtls_pk_sign_restartable( &pk, md, + hash, hash_len, sig, &sig_len, mbedtls_test_rnd_std_rand, NULL, rs_ctx ) == sign_ret ); if( sign_ret == 0 ) TEST_ASSERT( sig_len <= MBEDTLS_PK_SIGNATURE_MAX_SIZE ); else sig_len = MBEDTLS_PK_SIGNATURE_MAX_SIZE; - TEST_ASSERT( mbedtls_pk_verify( &pk, MBEDTLS_MD_SHA256, - hash, sizeof hash, sig, sig_len ) == verify_ret ); + TEST_ASSERT( mbedtls_pk_verify( &pk, md, + hash, hash_len, sig, sig_len ) == verify_ret ); if( verify_ret == 0 ) { hash[0]++; - TEST_ASSERT( mbedtls_pk_verify( &pk, MBEDTLS_MD_SHA256, - hash, sizeof hash, sig, sig_len ) != 0 ); + TEST_ASSERT( mbedtls_pk_verify( &pk, md, + hash, hash_len, sig, sig_len ) != 0 ); hash[0]--; sig[0]++; - TEST_ASSERT( mbedtls_pk_verify( &pk, MBEDTLS_MD_SHA256, - hash, sizeof hash, sig, sig_len ) != 0 ); + TEST_ASSERT( mbedtls_pk_verify( &pk, md, + hash, hash_len, sig, sig_len ) != 0 ); sig[0]--; } - TEST_ASSERT( mbedtls_pk_sign( &pk, MBEDTLS_MD_SHA256, hash, sizeof hash, + TEST_ASSERT( mbedtls_pk_sign( &pk, md, hash, hash_len, sig, &sig_len, mbedtls_test_rnd_std_rand, NULL ) == sign_ret ); @@ -940,19 +944,19 @@ void pk_sign_verify( int type, int parameter, int sign_ret, int verify_ret ) else sig_len = MBEDTLS_PK_SIGNATURE_MAX_SIZE; - TEST_ASSERT( mbedtls_pk_verify_restartable( &pk, MBEDTLS_MD_SHA256, - hash, sizeof hash, sig, sig_len, rs_ctx ) == verify_ret ); + TEST_ASSERT( mbedtls_pk_verify_restartable( &pk, md, + hash, hash_len, sig, sig_len, rs_ctx ) == verify_ret ); if( verify_ret == 0 ) { hash[0]++; - TEST_ASSERT( mbedtls_pk_verify_restartable( &pk, MBEDTLS_MD_SHA256, - hash, sizeof hash, sig, sig_len, rs_ctx ) != 0 ); + TEST_ASSERT( mbedtls_pk_verify_restartable( &pk, md, + hash, hash_len, sig, sig_len, rs_ctx ) != 0 ); hash[0]--; sig[0]++; - TEST_ASSERT( mbedtls_pk_verify_restartable( &pk, MBEDTLS_MD_SHA256, - hash, sizeof hash, sig, sig_len, rs_ctx ) != 0 ); + TEST_ASSERT( mbedtls_pk_verify_restartable( &pk, md, + hash, hash_len, sig, sig_len, rs_ctx ) != 0 ); sig[0]--; } @@ -961,6 +965,7 @@ exit: mbedtls_pk_restart_free( rs_ctx ); #endif mbedtls_pk_free( &pk ); + mbedtls_free( hash ); USE_PSA_DONE( ); } /* END_CASE */