From b39740e934167f34265000dc77a4c6bb37e793e7 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 4 Sep 2018 11:19:21 +0100 Subject: [PATCH 1/7] Bignum: Remove dead code Both variables affected by the code are overwritten before their next read. --- library/bignum.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 9f13da442..09b0321b2 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2086,15 +2086,6 @@ static int mpi_miller_rabin( const mbedtls_mpi *X, /* * pick a random A, 1 < A < |X| - 1 */ - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( &A, X->n * ciL, f_rng, p_rng ) ); - - if( mbedtls_mpi_cmp_mpi( &A, &W ) >= 0 ) - { - j = mbedtls_mpi_bitlen( &A ) - mbedtls_mpi_bitlen( &W ); - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( &A, j + 1 ) ); - } - A.p[0] |= 3; - count = 0; do { MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( &A, X->n * ciL, f_rng, p_rng ) ); From 72d555dd7c3f86142ef1f0aa2837d02ea51737e3 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 3 Sep 2018 14:45:23 +0100 Subject: [PATCH 2/7] Bignum: Fix prime validation vulnerability The input distribution to primality testing functions is completely different when used for generating primes and when for validating primes. The constants used in the library are geared towards the prime generation use case and are weak when used for validation. (Maliciously constructed composite numbers can pass the test with high probability) The mbedtls_mpi_is_prime() function is in the public API and although it is not documented, it is reasonable to assume that the primary use case is validating primes. The RSA module too uses it for validating key material. --- library/bignum.c | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 09b0321b2..bf5e0df4a 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2053,12 +2053,12 @@ cleanup: /* * Miller-Rabin pseudo-primality test (HAC 4.24) */ -static int mpi_miller_rabin( const mbedtls_mpi *X, +static int mpi_miller_rabin( const mbedtls_mpi *X, size_t rounds, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { int ret, count; - size_t i, j, k, n, s; + size_t i, j, k, s; mbedtls_mpi W, R, T, A, RR; mbedtls_mpi_init( &W ); mbedtls_mpi_init( &R ); mbedtls_mpi_init( &T ); mbedtls_mpi_init( &A ); @@ -2074,14 +2074,8 @@ static int mpi_miller_rabin( const mbedtls_mpi *X, MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( &R, s ) ); i = mbedtls_mpi_bitlen( X ); - /* - * HAC, table 4.4 - */ - n = ( ( i >= 1300 ) ? 2 : ( i >= 850 ) ? 3 : - ( i >= 650 ) ? 4 : ( i >= 350 ) ? 8 : - ( i >= 250 ) ? 12 : ( i >= 150 ) ? 18 : 27 ); - for( i = 0; i < n; i++ ) + for( i = 0; i < rounds; i++ ) { /* * pick a random A, 1 < A < |X| - 1 @@ -2148,7 +2142,7 @@ cleanup: /* * Pseudo-primality test: small factors, then Miller-Rabin */ -int mbedtls_mpi_is_prime( const mbedtls_mpi *X, +int mpi_is_prime_internal( const mbedtls_mpi *X, int rounds, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { @@ -2174,7 +2168,17 @@ int mbedtls_mpi_is_prime( const mbedtls_mpi *X, return( ret ); } - return( mpi_miller_rabin( &XX, f_rng, p_rng ) ); + return( mpi_miller_rabin( &XX, rounds, f_rng, p_rng ) ); +} + +/* + * Pseudo-primality test, error probability 2^-80 + */ +int mbedtls_mpi_is_prime( const mbedtls_mpi *X, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ) +{ + return mpi_is_prime_internal( X, 40, f_rng, p_rng ); } /* @@ -2186,6 +2190,7 @@ int mbedtls_mpi_gen_prime( mbedtls_mpi *X, size_t nbits, int dh_flag, { int ret; size_t k, n; + int rounds; mbedtls_mpi_uint r; mbedtls_mpi Y; @@ -2196,6 +2201,13 @@ int mbedtls_mpi_gen_prime( mbedtls_mpi *X, size_t nbits, int dh_flag, n = BITS_TO_LIMBS( nbits ); + /* + * 2^-80 error probability, number of rounds chosen per HAC, table 4.4 + */ + rounds = ( ( nbits >= 1300 ) ? 2 : ( nbits >= 850 ) ? 3 : + ( nbits >= 650 ) ? 4 : ( nbits >= 350 ) ? 8 : + ( nbits >= 250 ) ? 12 : ( nbits >= 150 ) ? 18 : 27 ); + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( X, n * ciL, f_rng, p_rng ) ); k = mbedtls_mpi_bitlen( X ); @@ -2207,7 +2219,7 @@ int mbedtls_mpi_gen_prime( mbedtls_mpi *X, size_t nbits, int dh_flag, if( dh_flag == 0 ) { - while( ( ret = mbedtls_mpi_is_prime( X, f_rng, p_rng ) ) != 0 ) + while( ( ret = mpi_is_prime_internal( X, rounds, f_rng, p_rng ) ) != 0 ) { if( ret != MBEDTLS_ERR_MPI_NOT_ACCEPTABLE ) goto cleanup; @@ -2243,8 +2255,10 @@ int mbedtls_mpi_gen_prime( mbedtls_mpi *X, size_t nbits, int dh_flag, */ if( ( ret = mpi_check_small_factors( X ) ) == 0 && ( ret = mpi_check_small_factors( &Y ) ) == 0 && - ( ret = mpi_miller_rabin( X, f_rng, p_rng ) ) == 0 && - ( ret = mpi_miller_rabin( &Y, f_rng, p_rng ) ) == 0 ) + ( ret = mpi_miller_rabin( X, rounds, f_rng, p_rng ) ) + == 0 && + ( ret = mpi_miller_rabin( &Y, rounds, f_rng, p_rng ) ) + == 0 ) { break; } From 0b7416150288f4edea4870092e23998a9ba34182 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 5 Sep 2018 17:04:49 +0100 Subject: [PATCH 3/7] Bignum: Add tests for primality testing Primality tests have to deal with different distribution when generating primes and when validating primes. These new tests are testing if mbedtls_mpi_is_prime() is working properly in the latter setting. The new tests involve pseudoprimes with maximum number of non-witnesses. The non-witnesses were generated by printing them from mpi_miller_rabin(). The pseudoprimes were generated by the following function: void gen_monier( mbedtls_mpi* res, int nbits ) { mbedtls_mpi p_2x_plus_1, p_4x_plus_1, x, tmp; mbedtls_mpi_init( &p_2x_plus_1 ); mbedtls_mpi_init( &p_4x_plus_1 ); mbedtls_mpi_init( &x ); mbedtls_mpi_init( &tmp ); do { mbedtls_mpi_gen_prime( &p_2x_plus_1, nbits >> 1, 0, rnd_std_rand, NULL ); mbedtls_mpi_sub_int( &x, &p_2x_plus_1, 1 ); mbedtls_mpi_div_int( &x, &tmp, &x, 2 ); if( mbedtls_mpi_get_bit( &x, 0 ) == 0 ) continue; mbedtls_mpi_mul_int( &p_4x_plus_1, &x, 4 ); mbedtls_mpi_add_int( &p_4x_plus_1, &p_4x_plus_1, 1 ); if( mbedtls_mpi_is_prime( &p_4x_plus_1, rnd_std_rand, NULL ) == 0 ) break; } while( 1 ); mbedtls_mpi_mul_mpi( res, &p_2x_plus_1, &p_4x_plus_1 ); } --- tests/suites/test_suite_mpi.data | 8 +++ tests/suites/test_suite_mpi.function | 75 ++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 1ab06a32a..296064196 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -680,6 +680,14 @@ Test mbedtls_mpi_is_prime #20 depends_on:MBEDTLS_GENPRIME mbedtls_mpi_is_prime:10:"49979687":0 +Test mbedtls_mpi_is_prime_det (4 non-witnesses) +depends_on:MBEDTLS_GENPRIME +mbedtls_mpi_is_prime_det:"043BD64BA10B11DA83FBD296B04BCA9E0552FAF6E09CAC74E2D7E735ED0DB09FC47ED76145644203EE0C826013BC602F560BCDAAED557D04683859A65D659FF828A245A2C5B1AC41E01E4669A525A45E23AF":"040EA852F7935ACCECC0E87B845281F047D10DC9AAFEF990AF9D3D66770DA30B0C5B5E03EEA8C0CB79B936FE0BB8EE5389EC1D34EB16C58AA3F2E11AF084160CDF6400BE1CC179867AB074866952D9F34EE7042D27F960E715A97FCB93F3182247D0A6AE51BD21CC2F6B0651F9E572C5FB86F3137053FA85FD7A51816D69B3A53A5A438C17754836D04E98CA240B901F828332F2D72D88C497DA45F533F99A6E53EDEA6B0424EC8951B048FA9A80134B37D0A67014597934E3CFC52C5A4DD4751ADF8D66FC79E84E2A3148C4B15C17E12CB659390FD275F39A331FFC80EC699BC3F6FAB868E30E9B14575FCDAB6FAED01E00112DD28704177E09C335AD43A696FEA761E8DF3B0663277A5C3637F9060CB5E5654F72E9A6B0F369E660AD4CF7ABF4195493545B367BD55271CD4BB7D9C15D3F508FE8F7409C2126FC8E73B43A67CD4EFB21E9F15DBF040A2A8D5F5ED75CEAC12B595C0051F3EC9D5A58ACE82A9506E64F780E9836728260FFE1BFD73E8A9869E3D46A35A856D3028F7FEAB9F4F1A04449AEDC80017EE1014080D87F0B50C8EF255324CD89F7D039":82:MBEDTLS_ERR_MPI_NOT_ACCEPTABLE + +Test mbedtls_mpi_is_prime_det (39 non-witnesses) +depends_on:MBEDTLS_GENPRIME +mbedtls_mpi_is_prime_det:"155102B67930FBE8858DF6C0642D77D419A7B7968E622CC7500F3E3F2C5168368C50E0083187":"119B3E2C721834D83416239B04447AA18AE0163E61DCAE97054563D79E094A6FA4485BD6A0501445BF57FE9C058926CDB862E04CC1A95D79D61D9AB3466857A53E04F8D7470C9C86649B226A13DDC534E18DFD5C22FAEA317CA4D4960F18457FD6D2FFB5F3273F74C89980DC774590D8D30D1159CA81999ED94A042D67DA68C82616AD46C2C88288A8EBD0B37AC7C152D9522CA4544642AD1210F6B642FEBF43563FA872B0DEFAFC69D0B6570E8FEA9570D0AADCFA9B06CC8BFD62CEDC221541210EEEF9762448C6D49F26AA767A4D66CB168589E0201923015314E6CD4A480E5936E7CF145F73A564C5B782635B3AFC3028E2632C5D3458224A7C9E8BA1876E8F690463C878292D3DC011E9640331E7F7621F2B5E0F6713DD8C9D6767521C4BA880DA8D11C67753C8493D2C4C4F1443147550D0B25B7FAD04EAFA9F8AA60974C1365C8A794CFEECEB4279B1150909A97E5A7A10B5D91186CA5B25A612036631FE73529C8CFAE51E76FB704A772DE5320EFC1212E7A399B1FEBF57D014AF9129DFF5D2C5DFBBEEAC55F360CF6D22FA90B8E2E9AD0C71AB6495A9452A58D653B8CC26128C66B43EFBA6E39AEC5717A1A3C2AE1449FCABAFE1180B159DA55190CD81A3D9E8D798647E11B827F0A057D6DA5AAD78AB5112EE65E10E8B8B369BA24E1B8AD2CD8548C497016C07A143DE1232F8059BE303572456FA92E76A0F23D1340629228B7D27C02D3833A72745B91A3DBEB5E081117A9F19597F00E4277B414FAEA8C8CEB895C37F956A5A22F8D7A10ADA50B22BAB312504904511AA0EFDD4D3BF20ECB17E8A684564FFB5BBD5E22C429F9A75A4FB4AE468FE7612ED53C7A11212E7EF3435CC9CA6E7DB167B8CCE2BECF35F89013F8F876223C77FA81570970858663C6E32B91080AA47F9C90177F51E6FD7747B910C9489C7B6ACB070996198AD9A40A69711274159210A9A12DBAAA4FB4632446066AB70D735DC95F7C2BCE517E88C064D728DE82B1B043DF4AEE0EFF5131120A4E5B9B4180EB6F6B8A0D1491ABDA069058A9966B1A517D8E7B4997DC52A1E698FD79E271153DF1913FE6787A5D99DE69F39C3F22D26DC731CFBB33FF5C267D85D7A3DAE8E1C87E1DB2F1236212EF1942EA756967FB3D07D629E59EA4034D9A9B5E270DD4A31C8A3DFDA99C1094B5537132C196DA2AEAF5253A019B9AF25B5DCB0D4DD75C7C9C353DA9DAABFB23959A5455312E7E1C21268C1BC14E83DCFDF50C27FD3E8B4EDC04C5F3CB5FCFFF2B57151E1B1EE1A6456DC006BC43E1158674AA4CF7D146DE4A57103BE43ED130C8007294ED2418C7A2B769A7D20EBB5A8367A77B313F81BB119B9954305FF160FF83EED7F808EE6D340A5CCC000CF81AA497D315D350CCE4E86A31456B8AA85B677491FC662933DFA55EB5BFF64B8D85430D676A85D1CAFAFF383E68C4E6C22A51063739EC03FC58C36C07C44E54828BE2152B2E9AFB0F179B157D09B64C147B524BB5424BB1914419424D9100D06EDCFC718F4DF3D562E9E16C446663F35273CA7BC5426B868A80C8D415C9A12A1619CDB7CDB5BEBC70313150BDF8C3AB26B809FE62D28E798EF1EF98C410A2DA0A9071F82154AC569078B0E647E2C085D1D907E634453442803D0492D3D0C78CACB762020C0E589C8B0981321EA2771305FD0413F3B2963FCE9A232F6641DB7E12ADC009A032063C41756E5E19E5711DE12711F07AFE7545B4D83F3EFD7BFD0435297C89DF3D4AF96EBE2CE8D64B93E36EA5D7E5A0492151D0CAEE7449A7D35E1A3C83E22C3B35162C073CC3B1CF76FBDEE84270721FC042EAAEB7325110181415E2031CFB7462F15111291CDAC0560FF9F4C7341F2FA261B97CEF348D074AA2EB4DB153FE6B1410519DA4213B611999868F3B867A2B6D758D333C4989DE80782683CA26ECDE373C71524F01B76349CE8A07A5EBECBB42259CF970DDA756EC996B189FEA045FEE45F23D476960913106ECA2510B8517AA75D56FA4152B2BDDC212014E5D07FD964D6EE532F0616DF74E104659955132331FABF2D2AD265E71C93C648A956FA0A3DB21FF103D516527F2DA0E870340B61EE8A8ED913B60605EB5A67B834D0FC90564386012585609870FEF6530B3E3C037B55506F0B5694F6B0FC":38:MBEDTLS_ERR_MPI_NOT_ACCEPTABLE + Test mbedtls_mpi_gen_prime (Too small) depends_on:MBEDTLS_GENPRIME mbedtls_mpi_gen_prime:2:0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index 6ae27af5b..1fcb005ed 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -1,5 +1,49 @@ /* BEGIN_HEADER */ #include "mbedtls/bignum.h" + +typedef struct mbedtls_test_mpi_random +{ + uint8_t *data; + uint32_t data_len; + size_t pos; + size_t chunk_len; +} mbedtls_test_mpi_random; + +/* + * This function is called by the Miller-Rabin primality test each time it + * chooses a random witness. The witnesses (or non-witnesses as provided by the + * test) are stored in the data member of the state structure. Each number is in + * the format that mbedtls_mpi_read_string understands and is chunk_len long. + */ +int mbedtls_test_mpi_miller_rabin_determinizer( void* state, + unsigned char* buf, + size_t len ) +{ + mbedtls_test_mpi_random *random = (mbedtls_test_mpi_random*) state; + + if( random == NULL || random->data == NULL || buf == NULL ) + return( -1 ); + + if( random->pos + random->chunk_len > random->data_len + || random->chunk_len > len ) + { + return( -1 ); + } + + memset( buf, 0, len ); + + /* The witness is written to the end of the buffer, since the buffer is + * used as big endian, unsigned binary data in mbedtls_mpi_read_binary. + * Writing the witness to the start of the buffer would result in the + * buffer being 'witness 000...000', which would be treated as + * witness * 2^n for some n. */ + memcpy( buf + len - random->chunk_len, &random->data[random->pos], + random->chunk_len ); + + random->pos += random->chunk_len; + + return( 0 ); +} /* END_HEADER */ /* BEGIN_DEPENDENCIES @@ -809,6 +853,37 @@ exit: } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_GENPRIME */ +void mbedtls_mpi_is_prime_det( char *input_X, char *witnesses, + int chunk_len, int div_result ) +{ + mbedtls_mpi X; + int res; + mbedtls_test_mpi_random rand; + uint8_t witness_buf[1000]; + uint8_t input_buf[1000]; + size_t witness_len; + size_t input_len; + + witness_len = unhexify( witness_buf, witnesses ); + input_len = unhexify( input_buf, input_X ); + + mbedtls_mpi_init( &X ); + rand.data = witness_buf; + rand.data_len = witness_len; + rand.pos = 0; + rand.chunk_len = chunk_len; + + TEST_ASSERT( mbedtls_mpi_read_binary( &X, input_buf, input_len ) == 0 ); + res = mbedtls_mpi_is_prime( &X, mbedtls_test_mpi_miller_rabin_determinizer, + &rand ); + TEST_ASSERT( res == div_result ); + +exit: + mbedtls_mpi_free( &X ); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_GENPRIME */ void mbedtls_mpi_gen_prime( int bits, int safe, int ref_ret ) { From 8d3fb2e167de5df9052f0b76f7ac6162b79955b0 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 6 Sep 2018 10:40:04 +0100 Subject: [PATCH 4/7] Changelog: Add entry for prime validation fix --- ChangeLog | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/ChangeLog b/ChangeLog index 0a9dc4f8d..7ab6f678e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -14,6 +14,19 @@ Changes test the handling of large packets and small packets on the client side in the same way as on the server side. +Security + * Fix mbedtls_mpi_is_prime() to use more rounds of probabilistic testing. The + previous settings for the number of rounds made it practical for an + adversary to construct non-primes that would be erroneously accepted as + primes with high probability. This does not have an impact on the + security of TLS, but can matter in other contexts with potentially + adversarially-chosen numbers that should be prime and can be validated. + For example, the number of rounds was enough to securely generate RSA key + pairs or Diffie-Hellman parameters, but was insufficient to validate + Diffie-Hellman parameters properly. + See "Prime and Prejudice" by by Martin R. Albrecht and Jake Massimo and + Kenneth G. Paterson and Juraj Somorovsky. + = mbed TLS 2.7.6 branch released 2018-08-31 Security From 56d7cc472f51fa09433e3be551a74e3c6132e2b9 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 2 Oct 2018 13:21:35 +0100 Subject: [PATCH 5/7] Fix bias in random number generation in Miller-Rabin test When a random number is generated for the Miller-Rabin primality test, if the bit length of the random number is larger than the number being tested, the random number is shifted right to have the same bit length. This introduces bias, as the random number is now guaranteed to be larger than 2^(bit length-1). Changing this to instead zero all bits higher than the tested numbers bit length will remove this bias and keep the random number being uniformly generated. --- library/bignum.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index bf5e0df4a..bf0314b14 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2087,7 +2087,7 @@ static int mpi_miller_rabin( const mbedtls_mpi *X, size_t rounds, j = mbedtls_mpi_bitlen( &A ); k = mbedtls_mpi_bitlen( &W ); if (j > k) { - MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( &A, j - k ) ); + A.p[A.n - 1] &= ( (mbedtls_mpi_uint) 1 << ( k - ( A.n - 1 ) * biL - 1 ) ) - 1; } if (count++ > 30) { From 94759f659313305b28bae061177722667e53fd1a Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 16 Oct 2018 15:09:19 +0100 Subject: [PATCH 6/7] Mark internal function as static --- library/bignum.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index bf0314b14..db2b931cb 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2142,7 +2142,7 @@ cleanup: /* * Pseudo-primality test: small factors, then Miller-Rabin */ -int mpi_is_prime_internal( const mbedtls_mpi *X, int rounds, +static int mpi_is_prime_internal( const mbedtls_mpi *X, int rounds, int (*f_rng)(void *, unsigned char *, size_t), void *p_rng ) { From 0eaa6d5bb69f39eb8b7ca89fe22aceff109652a6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 5 Nov 2018 16:37:06 +0100 Subject: [PATCH 7/7] Fix buffer overflow in test mbedtls_mpi_is_prime_det --- tests/suites/test_suite_mpi.function | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index 1fcb005ed..04dca0fcb 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -860,13 +860,13 @@ void mbedtls_mpi_is_prime_det( char *input_X, char *witnesses, mbedtls_mpi X; int res; mbedtls_test_mpi_random rand; - uint8_t witness_buf[1000]; - uint8_t input_buf[1000]; + uint8_t *witness_buf = NULL; + uint8_t *input_buf = NULL; size_t witness_len; size_t input_len; - witness_len = unhexify( witness_buf, witnesses ); - input_len = unhexify( input_buf, input_X ); + witness_buf = unhexify_alloc( witnesses, &witness_len ); + input_buf = unhexify_alloc( input_X, &input_len ); mbedtls_mpi_init( &X ); rand.data = witness_buf; @@ -881,6 +881,8 @@ void mbedtls_mpi_is_prime_det( char *input_X, char *witnesses, exit: mbedtls_mpi_free( &X ); + mbedtls_free( witness_buf ); + mbedtls_free( input_buf ); } /* END_CASE */