From b8e75286b4b65643330b34903b5d80eae3a56ce7 Mon Sep 17 00:00:00 2001 From: Andres AG Date: Thu, 19 Jan 2017 11:24:33 +0000 Subject: [PATCH 1/2] Fix data loss in unsigned int cast in PK This patch introduces some additional checks in the PK module for 64-bit systems only. The problem is that the API functions in the PK abstraction accept a size_t value for the hashlen, while the RSA module accepts an unsigned int for the hashlen. Instead of silently casting size_t to unsigned int, this change checks whether the hashlen overflows an unsigned int and returns an error. --- ChangeLog | 9 +++++++++ library/pk.c | 11 ++++++++++- library/pk_wrap.c | 18 ++++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 0a857ba76..0c9c44428 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,14 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS x.x.x branch released xxxx-xx-xx + +Security + * Add checks to prevent signature forgeries for very large messages while + using RSA through the PK module in 64-bit systems. The issue was caused by + some data loss when casting a size_t to an unsigned int value in the + functions rsa_verify_wrap(), rsa_sign_wrap(), rsa_alt_sign_wrap() and + mbedtls_pk_sign(). Found by Jean-Philippe Aumasson. + = mbed TLS 2.4.1 branch released 2016-12-13 Changes diff --git a/library/pk.c b/library/pk.c index 10bd0a582..8d13bc5ce 100644 --- a/library/pk.c +++ b/library/pk.c @@ -29,6 +29,8 @@ #include "mbedtls/pk.h" #include "mbedtls/pk_internal.h" +#include "mbedtls/bignum.h" + #if defined(MBEDTLS_RSA_C) #include "mbedtls/rsa.h" #endif @@ -39,6 +41,8 @@ #include "mbedtls/ecdsa.h" #endif +#include + /* Implementation that should never be optimized out by the compiler */ static void mbedtls_zeroize( void *v, size_t n ) { volatile unsigned char *p = v; while( n-- ) *p++ = 0; @@ -209,6 +213,11 @@ int mbedtls_pk_verify_ext( mbedtls_pk_type_t type, const void *options, int ret; const mbedtls_pk_rsassa_pss_options *pss_opts; +#if defined(MBEDTLS_HAVE_INT64) + if( md_alg == MBEDTLS_MD_NONE && UINT_MAX < hash_len ) + return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); +#endif /* MBEDTLS_HAVE_INT64 */ + if( options == NULL ) return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); @@ -232,7 +241,7 @@ int mbedtls_pk_verify_ext( mbedtls_pk_type_t type, const void *options, return( 0 ); #else return( MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE ); -#endif +#endif /* MBEDTLS_RSA_C && MBEDTLS_PKCS1_V21 */ } /* General case: no options */ diff --git a/library/pk_wrap.c b/library/pk_wrap.c index 712ad4832..db6274cbf 100644 --- a/library/pk_wrap.c +++ b/library/pk_wrap.c @@ -30,6 +30,7 @@ /* Even if RSA not activated, for the sake of RSA-alt */ #include "mbedtls/rsa.h" +#include "mbedtls/bignum.h" #include @@ -49,6 +50,8 @@ #define mbedtls_free free #endif +#include + #if defined(MBEDTLS_PK_RSA_ALT_SUPPORT) /* Implementation that should never be optimized out by the compiler */ static void mbedtls_zeroize( void *v, size_t n ) { @@ -74,6 +77,11 @@ static int rsa_verify_wrap( void *ctx, mbedtls_md_type_t md_alg, { int ret; +#if defined(MBEDTLS_HAVE_INT64) + if( md_alg == MBEDTLS_MD_NONE && UINT_MAX < hash_len ) + return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); +#endif /* MBEDTLS_HAVE_INT64 */ + if( sig_len < ((mbedtls_rsa_context *) ctx)->len ) return( MBEDTLS_ERR_RSA_VERIFY_FAILED ); @@ -93,6 +101,11 @@ static int rsa_sign_wrap( void *ctx, mbedtls_md_type_t md_alg, unsigned char *sig, size_t *sig_len, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { +#if defined(MBEDTLS_HAVE_INT64) + if( md_alg == MBEDTLS_MD_NONE && UINT_MAX < hash_len ) + return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); +#endif /* MBEDTLS_HAVE_INT64 */ + *sig_len = ((mbedtls_rsa_context *) ctx)->len; return( mbedtls_rsa_pkcs1_sign( (mbedtls_rsa_context *) ctx, f_rng, p_rng, MBEDTLS_RSA_PRIVATE, @@ -402,6 +415,11 @@ static int rsa_alt_sign_wrap( void *ctx, mbedtls_md_type_t md_alg, { mbedtls_rsa_alt_context *rsa_alt = (mbedtls_rsa_alt_context *) ctx; +#if defined(MBEDTLS_HAVE_INT64) + if( UINT_MAX < hash_len ) + return( MBEDTLS_ERR_PK_BAD_INPUT_DATA ); +#endif /* MBEDTLS_HAVE_INT64 */ + *sig_len = rsa_alt->key_len_func( rsa_alt->key ); return( rsa_alt->sign_func( rsa_alt->key, f_rng, p_rng, MBEDTLS_RSA_PRIVATE, From 511f83db0712fbcb8b3a5b173ae074ae035a4748 Mon Sep 17 00:00:00 2001 From: Andres AG Date: Wed, 15 Feb 2017 10:52:32 +0000 Subject: [PATCH 2/2] Add PK tests to avoid hashlen overflow for RSA --- tests/suites/test_suite_pk.data | 3 +++ tests/suites/test_suite_pk.function | 36 +++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/tests/suites/test_suite_pk.data b/tests/suites/test_suite_pk.data index 22a7fa8b1..f6ea378ff 100644 --- a/tests/suites/test_suite_pk.data +++ b/tests/suites/test_suite_pk.data @@ -150,3 +150,6 @@ Check pair #5 (RSA vs EC) depends_on:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_RSA_C mbedtls_pk_check_pair:"data_files/ec_256_pub.pem":"data_files/server1.key":MBEDTLS_ERR_PK_TYPE_MISMATCH +RSA hash_len overflow (size_t vs unsigned int) +depends_on:MBEDTLS_RSA_C:MBEDTLS_HAVE_INT64 +pk_rsa_overflow: diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index 08a262346..5fa8a693a 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -5,6 +5,9 @@ #include "mbedtls/ecp.h" #include "mbedtls/rsa.h" +/* For detecting 64-bit compilation */ +#include "mbedtls/bignum.h" + static int rnd_std_rand( void *rng_state, unsigned char *output, size_t len ); #define RSA_KEY_SIZE 512 @@ -414,6 +417,34 @@ exit: } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_RSA_C:MBEDTLS_HAVE_INT64 */ +void pk_rsa_overflow( ) +{ + mbedtls_pk_context pk; + size_t hash_len = (size_t)-1; + + mbedtls_pk_init( &pk ); + + TEST_ASSERT( mbedtls_pk_setup( &pk, + mbedtls_pk_info_from_type( MBEDTLS_PK_RSA ) ) == 0 ); + +#if defined(MBEDTLS_PKCS1_V21) + TEST_ASSERT( mbedtls_pk_verify_ext( MBEDTLS_PK_RSASSA_PSS, NULL, &pk, + MBEDTLS_MD_NONE, NULL, hash_len, NULL, 0 ) == + MBEDTLS_ERR_PK_BAD_INPUT_DATA ); +#endif /* MBEDTLS_PKCS1_V21 */ + + TEST_ASSERT( mbedtls_pk_verify( &pk, MBEDTLS_MD_NONE, NULL, hash_len, + NULL, 0 ) == MBEDTLS_ERR_PK_BAD_INPUT_DATA ); + + TEST_ASSERT( mbedtls_pk_sign( &pk, MBEDTLS_MD_NONE, NULL, hash_len, NULL, 0, + rnd_std_rand, NULL ) == MBEDTLS_ERR_PK_BAD_INPUT_DATA ); + +exit: + mbedtls_pk_free( &pk ); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_RSA_C:MBEDTLS_PK_RSA_ALT_SUPPORT */ void pk_rsa_alt( ) { @@ -461,6 +492,11 @@ void pk_rsa_alt( ) /* Test signature */ TEST_ASSERT( mbedtls_pk_sign( &alt, MBEDTLS_MD_NONE, hash, sizeof hash, sig, &sig_len, rnd_std_rand, NULL ) == 0 ); +#if defined(MBEDTLS_HAVE_INT64) + TEST_ASSERT( mbedtls_pk_sign( &alt, MBEDTLS_MD_NONE, hash, (size_t)-1, + NULL, NULL, rnd_std_rand, NULL ) == + MBEDTLS_ERR_PK_BAD_INPUT_DATA ); +#endif /* MBEDTLS_HAVE_INT64 */ TEST_ASSERT( sig_len == RSA_KEY_LEN ); TEST_ASSERT( mbedtls_pk_verify( &rsa, MBEDTLS_MD_NONE, hash, sizeof hash, sig, sig_len ) == 0 );