From 4b151fabb7b65a774f5fb5cbcb061314e54564c9 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 20 Sep 2017 13:46:37 +0100 Subject: [PATCH 1/4] DHM: Add negative tests for parameter checking A bug in the dhm_check_range() function makes it pass even when the parameters are not in the range. This commit adds tests for signalling this problem as well as a couple of other negative tests. --- tests/suites/test_suite_dhm.data | 18 +++++++++++++++--- tests/suites/test_suite_dhm.function | 7 +++++-- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/tests/suites/test_suite_dhm.data b/tests/suites/test_suite_dhm.data index f2cdeffa5..e351ebdd4 100644 --- a/tests/suites/test_suite_dhm.data +++ b/tests/suites/test_suite_dhm.data @@ -1,11 +1,23 @@ Diffie-Hellman full exchange #1 -dhm_do_dhm:10:"23":10:"5" +dhm_do_dhm:10:"23":10:"5":0 Diffie-Hellman full exchange #2 -dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622" +dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622":0 Diffie-Hellman full exchange #3 -dhm_do_dhm:10:"93450983094850938450983409623982317398171298719873918739182739712938719287391879381271":10:"9345098309485093845098340962223981329819812792137312973297123912791271" +dhm_do_dhm:10:"93450983094850938450983409623982317398171298719873918739182739712938719287391879381271":10:"9345098309485093845098340962223981329819812792137312973297123912791271":0 + +Diffie-Hellman trivial subgroup #1 +dhm_do_dhm:10:"23":10:"1":MBEDTLS_ERR_DHM_BAD_INPUT_DATA + +Diffie-Hellman trivial subgroup #2 +dhm_do_dhm:10:"23":10:"-1":MBEDTLS_ERR_DHM_BAD_INPUT_DATA + +Diffie-Hellman small modulus +dhm_do_dhm:10:"3":10:"5":MBEDTLS_ERR_DHM_MAKE_PARAMS_FAILED + +Diffie-Hellman zero modulus +dhm_do_dhm:10:"0":10:"5":MBEDTLS_ERR_DHM_BAD_INPUT_DATA Diffie-Hallman load parameters from file dhm_file:"data_files/dhparams.pem":"9e35f430443a09904f3a39a979797d070df53378e79c2438bef4e761f3c714553328589b041c809be1d6c6b5f1fc9f47d3a25443188253a992a56818b37ba9de5a40d362e56eff0be5417474c125c199272c8fe41dea733df6f662c92ae76556e755d10c64e6a50968f67fc6ea73d0dca8569be2ba204e23580d8bca2f4975b3":"02":128 diff --git a/tests/suites/test_suite_dhm.function b/tests/suites/test_suite_dhm.function index b9b8e1956..4fd8fff23 100644 --- a/tests/suites/test_suite_dhm.function +++ b/tests/suites/test_suite_dhm.function @@ -9,7 +9,7 @@ /* BEGIN_CASE */ void dhm_do_dhm( int radix_P, char *input_P, - int radix_G, char *input_G ) + int radix_G, char *input_G, int result ) { mbedtls_dhm_context ctx_srv; mbedtls_dhm_context ctx_cli; @@ -44,7 +44,10 @@ void dhm_do_dhm( int radix_P, char *input_P, /* * First key exchange */ - TEST_ASSERT( mbedtls_dhm_make_params( &ctx_srv, x_size, ske, &ske_len, &rnd_pseudo_rand, &rnd_info ) == 0 ); + TEST_ASSERT( mbedtls_dhm_make_params( &ctx_srv, x_size, ske, &ske_len, &rnd_pseudo_rand, &rnd_info ) == result ); + if ( result != 0 ) + goto exit; + ske[ske_len++] = 0; ske[ske_len++] = 0; TEST_ASSERT( mbedtls_dhm_read_params( &ctx_cli, &p, ske + ske_len ) == 0 ); From aa325d7b7f5656edfb2b61cbab2189fd01818975 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 20 Sep 2017 15:33:24 +0100 Subject: [PATCH 2/4] DHM: Fix dhm_check_range() always returning 0 Although the variable ret was initialised to an error, the MBEDTLS_MPI_CHK macro was overwriting it. Therefore it ended up being 0 whenewer the bignum computation was successfull and stayed 0 independently of the actual check. --- ChangeLog | 6 +++++- library/dhm.c | 11 +++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index e199682ea..ce0e83173 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,6 +1,10 @@ mbed TLS ChangeLog (Sorted per branch, date) -= mbed TLS x.x.x released xxxx-xx-xx += mbed TLS x.x.x branch released xxxx-xx-xx + +Security + * Fix dhm_check_range() failing to detect trivial subgroups and essentially + always returning 0. Reported by prashantkspatil. Bugfix * Fix ssl_parse_record_header() to silently discard invalid DTLS records diff --git a/library/dhm.c b/library/dhm.c index bec52a11d..620610dab 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -93,6 +93,9 @@ static int dhm_read_bignum( mbedtls_mpi *X, * * Parameter should be: 2 <= public_param <= P - 2 * + * This means that we need to return an error if + * public_param < 2 or public param > P-2 + * * For more information on the attack, see: * http://www.cl.cam.ac.uk/~rja14/Papers/psandqs.pdf * http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2005-2643 @@ -100,17 +103,17 @@ static int dhm_read_bignum( mbedtls_mpi *X, static int dhm_check_range( const mbedtls_mpi *param, const mbedtls_mpi *P ) { mbedtls_mpi L, U; - int ret = MBEDTLS_ERR_DHM_BAD_INPUT_DATA; + int ret = 0; mbedtls_mpi_init( &L ); mbedtls_mpi_init( &U ); MBEDTLS_MPI_CHK( mbedtls_mpi_lset( &L, 2 ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_sub_int( &U, P, 2 ) ); - if( mbedtls_mpi_cmp_mpi( param, &L ) >= 0 && - mbedtls_mpi_cmp_mpi( param, &U ) <= 0 ) + if( mbedtls_mpi_cmp_mpi( param, &L ) < 0 || + mbedtls_mpi_cmp_mpi( param, &U ) > 0 ) { - ret = 0; + ret = MBEDTLS_ERR_DHM_BAD_INPUT_DATA; } cleanup: From b174c84a3b5aa3353e02a565a9cfe36cc6795384 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 20 Sep 2017 16:26:04 +0100 Subject: [PATCH 3/4] Refine dhm_check_range() fix Changelog entry --- ChangeLog | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index ce0e83173..3da4a84d7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,8 +3,8 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS x.x.x branch released xxxx-xx-xx Security - * Fix dhm_check_range() failing to detect trivial subgroups and essentially - always returning 0. Reported by prashantkspatil. + * Fix dhm_check_range() failing to detect trivial subgroups and potentially + leaking 1 bit of the private key. Reported by prashantkspatil. Bugfix * Fix ssl_parse_record_header() to silently discard invalid DTLS records From 1ad1c6d4e18b32118863a63a42f4a6d70084e6ca Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 21 Sep 2017 09:02:11 +0100 Subject: [PATCH 4/4] Fix typo --- library/dhm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/dhm.c b/library/dhm.c index 620610dab..71b4f85d7 100644 --- a/library/dhm.c +++ b/library/dhm.c @@ -94,7 +94,7 @@ static int dhm_read_bignum( mbedtls_mpi *X, * Parameter should be: 2 <= public_param <= P - 2 * * This means that we need to return an error if - * public_param < 2 or public param > P-2 + * public_param < 2 or public_param > P-2 * * For more information on the attack, see: * http://www.cl.cam.ac.uk/~rja14/Papers/psandqs.pdf