From d4d60579e4550d034a884d1a413942fbe36a8625 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jan 2018 07:12:01 +0000 Subject: [PATCH 1/3] Address issues found by coverity 1) `mbedtls_rsa_import_raw` used an uninitialized return value when it was called without any input parameters. While not sensible, this is allowed and should be a succeeding no-op. 2) The MPI test for prime generation missed a return value check for a call to `mbedtls_mpi_shift_r`. This is neither critical nor new but should be fixed. 3) Both the RSA keygeneration example program and the RSA test suites contained code initializing an RSA context after a potentially failing call to CTR DRBG initialization, leaving the corresponding RSA context free call in the cleanup section of the respective function orphaned. While this defect existed before, Coverity picked up on it again because of newly introduced MPI's that were also wrongly initialized only after the call to CTR DRBG init. The commit fixes both the old and the new issue by moving the initializtion of both the RSA context and all MPI's prior to the first potentially failing call. --- library/rsa.c | 2 +- programs/pkey/rsa_genkey.c | 9 ++++----- tests/suites/test_suite_mpi.function | 3 ++- tests/suites/test_suite_rsa.data | 3 +++ tests/suites/test_suite_rsa.function | 7 +++---- 5 files changed, 13 insertions(+), 11 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index ad1ef6db2..7e921fd54 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -104,7 +104,7 @@ int mbedtls_rsa_import_raw( mbedtls_rsa_context *ctx, unsigned char const *D, size_t D_len, unsigned char const *E, size_t E_len ) { - int ret; + int ret = 0; if( N != NULL ) { diff --git a/programs/pkey/rsa_genkey.c b/programs/pkey/rsa_genkey.c index 3dae0a6c8..939921761 100644 --- a/programs/pkey/rsa_genkey.c +++ b/programs/pkey/rsa_genkey.c @@ -71,6 +71,10 @@ int main( void ) const char *pers = "rsa_genkey"; mbedtls_ctr_drbg_init( &ctr_drbg ); + mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, 0 ); + mbedtls_mpi_init( &N ); mbedtls_mpi_init( &P ); mbedtls_mpi_init( &Q ); + mbedtls_mpi_init( &D ); mbedtls_mpi_init( &E ); mbedtls_mpi_init( &DP ); + mbedtls_mpi_init( &DQ ); mbedtls_mpi_init( &QP ); mbedtls_printf( "\n . Seeding the random number generator..." ); fflush( stdout ); @@ -87,11 +91,6 @@ int main( void ) mbedtls_printf( " ok\n . Generating the RSA key [ %d-bit ]...", KEY_SIZE ); fflush( stdout ); - mbedtls_rsa_init( &rsa, MBEDTLS_RSA_PKCS_V15, 0 ); - mbedtls_mpi_init( &N ); mbedtls_mpi_init( &P ); mbedtls_mpi_init( &Q ); - mbedtls_mpi_init( &D ); mbedtls_mpi_init( &E ); mbedtls_mpi_init( &DP ); - mbedtls_mpi_init( &DQ ); mbedtls_mpi_init( &QP ); - if( ( ret = mbedtls_rsa_gen_key( &rsa, mbedtls_ctr_drbg_random, &ctr_drbg, KEY_SIZE, EXPONENT ) ) != 0 ) { diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index b94c88980..6ae27af5b 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -830,7 +830,8 @@ void mbedtls_mpi_gen_prime( int bits, int safe, int ref_ret ) TEST_ASSERT( mbedtls_mpi_is_prime( &X, rnd_std_rand, NULL ) == 0 ); if( safe ) { - mbedtls_mpi_shift_r( &X, 1 ); /* X = ( X - 1 ) / 2 */ + /* X = ( X - 1 ) / 2 */ + TEST_ASSERT( mbedtls_mpi_shift_r( &X, 1 ) == 0 ); TEST_ASSERT( mbedtls_mpi_is_prime( &X, rnd_std_rand, NULL ) == 0 ); } } diff --git a/tests/suites/test_suite_rsa.data b/tests/suites/test_suite_rsa.data index 46046119d..2747da726 100644 --- a/tests/suites/test_suite_rsa.data +++ b/tests/suites/test_suite_rsa.data @@ -526,6 +526,9 @@ mbedtls_rsa_import_raw:"b38ac65c8141f7f5c96e14470e851936a67bf94cc6821a39ac12c05f RSA Import Raw (N,-,-,-,E), successive mbedtls_rsa_import_raw:"b38ac65c8141f7f5c96e14470e851936a67bf94cc6821a39ac12c05f7c0b06d9e6ddba2224703b02e25f31452f9c4a8417b62675fdc6df46b94813bc7b9769a892c482b830bfe0ad42e46668ace68903617faf6681f4babf1cc8e4b0420d3c7f61dc45434c6b54e2c3ee0fc07908509d79c9826e673bf8363255adb0add2401039a7bcd1b4ecf0fbe6ec8369d2da486eec59559dd1d54c9b24190965eafbdab203b35255765261cd0909acf93c3b8b8428cbb448de4715d1b813d0c94829c229543d391ce0adab5351f97a3810c1f73d7b1458b97daed4209c50e16d064d2d5bfda8c23893d755222793146d0a78c3d64f35549141486c3b0961a7b4c1a2034f":"":"":"":"03":1:0:0:0 +RSA Import Raw (-,-,-,-,-) +mbedtls_rsa_import_raw:"":"":"":"":"":0:0:0:MBEDTLS_ERR_RSA_BAD_INPUT_DATA + RSA Export (N,P,Q,D,E) mbedtls_rsa_export:16:"b38ac65c8141f7f5c96e14470e851936a67bf94cc6821a39ac12c05f7c0b06d9e6ddba2224703b02e25f31452f9c4a8417b62675fdc6df46b94813bc7b9769a892c482b830bfe0ad42e46668ace68903617faf6681f4babf1cc8e4b0420d3c7f61dc45434c6b54e2c3ee0fc07908509d79c9826e673bf8363255adb0add2401039a7bcd1b4ecf0fbe6ec8369d2da486eec59559dd1d54c9b24190965eafbdab203b35255765261cd0909acf93c3b8b8428cbb448de4715d1b813d0c94829c229543d391ce0adab5351f97a3810c1f73d7b1458b97daed4209c50e16d064d2d5bfda8c23893d755222793146d0a78c3d64f35549141486c3b0961a7b4c1a2034f":16:"e79a373182bfaa722eb035f772ad2a9464bd842de59432c18bbab3a7dfeae318c9b915ee487861ab665a40bd6cda560152578e8579016c929df99fea05b4d64efca1d543850bc8164b40d71ed7f3fa4105df0fb9b9ad2a18ce182c8a4f4f975bea9aa0b9a1438a27a28e97ac8330ef37383414d1bd64607d6979ac050424fd17":16:"c6749cbb0db8c5a177672d4728a8b22392b2fc4d3b8361d5c0d5055a1b4e46d821f757c24eef2a51c561941b93b3ace7340074c058c9bb48e7e7414f42c41da4cccb5c2ba91deb30c586b7fb18af12a52995592ad139d3be429add6547e044becedaf31fa3b39421e24ee034fbf367d11f6b8f88ee483d163b431e1654ad3e89":16:"77B1D99300D6A54E864962DA09AE10CF19A7FB888456BC2672B72AEA52B204914493D16C184AD201EC3F762E1FBD8702BA796EF953D9EA2F26300D285264F11B0C8301D0207FEB1E2C984445C899B0ACEBAA74EF014DD1D4BDDB43202C08D2FF9692D8D788478DEC829EB52AFB5AE068FBDBAC499A27FACECC391E75C936D55F07BB45EE184DAB45808E15722502F279F89B38C1CB292557E5063597F52C75D61001EDC33F4739353E33E56AD273B067C1A2760208529EA421774A5FFFCB3423B1E0051E7702A55D80CBF2141569F18F87BFF538A1DA8EDBB2693A539F68E0D62D77743F89EACF3B1723BDB25CE2F333FA63CACF0E67DF1A431893BB9B352FCB":16:"3":1:0 diff --git a/tests/suites/test_suite_rsa.function b/tests/suites/test_suite_rsa.function index 639bcb89d..ee3516ad1 100644 --- a/tests/suites/test_suite_rsa.function +++ b/tests/suites/test_suite_rsa.function @@ -878,17 +878,16 @@ void mbedtls_rsa_import( int radix_N, char *input_N, const int have_E = ( strlen( input_E ) > 0 ); mbedtls_ctr_drbg_init( &ctr_drbg ); - mbedtls_entropy_init( &entropy ); - TEST_ASSERT( mbedtls_ctr_drbg_seed( &ctr_drbg, mbedtls_entropy_func, &entropy, - (const unsigned char *) pers, strlen( pers ) ) == 0 ); - mbedtls_rsa_init( &ctx, 0, 0 ); mbedtls_mpi_init( &N ); mbedtls_mpi_init( &P ); mbedtls_mpi_init( &Q ); mbedtls_mpi_init( &D ); mbedtls_mpi_init( &E ); + TEST_ASSERT( mbedtls_ctr_drbg_seed( &ctr_drbg, mbedtls_entropy_func, &entropy, + (const unsigned char *) pers, strlen( pers ) ) == 0 ); + if( have_N ) TEST_ASSERT( mbedtls_mpi_read_string( &N, radix_N, input_N ) == 0 ); From adb0b2e935618e51e33346ccb278c8341f38ab08 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jan 2018 10:35:11 +0000 Subject: [PATCH 2/3] Update Visual Studio project files This commit updates the Visual Studio project file `visualc/VS2010/mbedTLS.vcxproj` to add the newly introduced `rsa_internal.h` and `rsa_internal.c`. --- visualc/VS2010/mbedTLS.vcxproj | 2 ++ 1 file changed, 2 insertions(+) diff --git a/visualc/VS2010/mbedTLS.vcxproj b/visualc/VS2010/mbedTLS.vcxproj index 65730cd41..f13f83cc1 100644 --- a/visualc/VS2010/mbedTLS.vcxproj +++ b/visualc/VS2010/mbedTLS.vcxproj @@ -199,6 +199,7 @@ + @@ -267,6 +268,7 @@ + From 997e2184c55c47bf5e89195de73a8259bca42486 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 10 Jan 2018 10:39:20 +0000 Subject: [PATCH 3/3] Adapt ChangeLog --- ChangeLog | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ChangeLog b/ChangeLog index 0061fe87d..d81d59673 100644 --- a/ChangeLog +++ b/ChangeLog @@ -97,6 +97,10 @@ Bugfix * Fix use of uninitialized memory in mbedtls_timing_get_timer when reset=1. * Fix possible memory leaks in mbedtls_gcm_self_test(). * Added missing return code checks in mbedtls_aes_self_test(). + * Fix issues in RSA key generation program programs/x509/rsa_genkey and the + RSA test suite where the failure of CTR DRBG initialization lead to + freeing an RSA context and several MPI's without proper initialization + beforehand. Changes * Extend cert_write example program by options to set the CRT version