From 02a8b0e232cc5ebaca35547f7acb195df22c7d61 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 165cf87db..9289d1fbc 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2076,15 +2076,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 9dc5b7a27bcb979811b88c16395d4b59c377a8f6 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 9289d1fbc..af523fcd3 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2043,12 +2043,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 ); @@ -2064,14 +2064,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 @@ -2138,7 +2132,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 ) { @@ -2164,7 +2158,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 ); } /* @@ -2176,6 +2180,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; @@ -2186,6 +2191,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 ); @@ -2197,7 +2209,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; @@ -2233,8 +2245,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 18b08c6f4cf382c546bcaec794b510e5889b636d 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 797505ada..e8dac5ea8 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 6ceae1501..0aee44543 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 @@ -805,6 +849,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 da4ea3bd9261f80ac48ceb242c464bf18e804d9e 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 | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/ChangeLog b/ChangeLog index 8c82d08d1..266794379 100644 --- a/ChangeLog +++ b/ChangeLog @@ -14,6 +14,20 @@ 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.1.15 branch released 2018-08-31 Security From 0c9bbb0ff8938453d0fa9b78d11812a81859580f 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 af523fcd3..1c3bfe0e5 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2077,7 +2077,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 73497ceaef9e64f510e255024600413b6c8bf71b Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 16 Oct 2018 15:07:48 +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 1c3bfe0e5..61aa961c6 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -2132,7 +2132,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 2521d16ace71927dcd0010b7233ae65a5fb21105 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 0aee44543..952cac447 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -856,13 +856,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; @@ -877,6 +877,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 */