From 228b98f24f1f586f23fe458b3607bae861608a60 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 3 Apr 2021 18:25:29 +0200 Subject: [PATCH 1/7] Add a few unit tests for mbedtls_mpi_read_string with leading zeros Signed-off-by: Gilles Peskine --- tests/suites/test_suite_mpi.data | 34 +++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 5229253f6..86019e55c 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -10,21 +10,39 @@ mpi_null: Base test mpi_read_write_string #1 mpi_read_write_string:10:"128":10:"128":100:0:0 +Base test mpi_read_write_string #1 (Leading 0) +mpi_read_write_string:10:"0128":10:"128":100:0:0 + Base test mpi_read_write_string #2 mpi_read_write_string:10:"128":16:"80":100:0:0 -Base test mpi_read_write_string #3 (Read zero) +Base test mpi_read_write_string #3 (Read zero decimal) mpi_read_write_string:10:"0":10:"0":100:0:0 +Base test mpi_read_write_string #3 (Read zero hex) +mpi_read_write_string:16:"0":16:"00":100:0:0 + +Base test mpi_read_write_string #3 (Read minus zero decimal) +mpi_read_write_string:10:"-0":10:"0":100:0:0 + +Base test mpi_read_write_string #3 (Read minus zero hex) +mpi_read_write_string:16:"-0":16:"00":100:0:0 + Base test mpi_read_write_string #3 (Negative decimal) mpi_read_write_string:10:"-23":10:"-23":100:0:0 -Base test mpi_read_write_string #3 (Negative hex) +Base test mpi_read_write_string #3 (Negative decimal, leading 0) +mpi_read_write_string:10:"-023":10:"-23":100:0:0 + +Base test mpi_read_write_string #3 (Negative decimal -> hex) mpi_read_write_string:16:"-20":10:"-32":100:0:0 -Base test mpi_read_write_string #3 (Negative decimal) +Base test mpi_read_write_string #3 (Negative hex) mpi_read_write_string:16:"-23":16:"-23":100:0:0 +Base test mpi_read_write_string #3 (Negative hex, leading 0) +mpi_read_write_string:16:"-023":16:"-23":100:0:0 + Base test mpi_read_write_string #4 (Buffer just fits) mpi_read_write_string:16:"-4":4:"-10":4:0:0 @@ -49,12 +67,18 @@ mpi_read_write_string:10:"29":15:"1e":100:0:0 Test mpi_read_write_string #7 mpi_read_write_string:10:"56125680981752282334141896320372489490613963693556392520816017892111350604111697682705498319512049040516698827829292076808006940873974979584527073481012636016353913462376755556720019831187364993587901952757307830896531678727717924":16:"0941379d00fed1491fe15df284dfde4a142f68aa8d412023195cee66883e6290ffe703f4ea5963bf212713cee46b107c09182b5edcd955adac418bf4918e2889af48e1099d513830cec85c26ac1e158b52620e33ba8692f893efbb2f958b4424":200:0:0 -Test mpi_read_write_string #8 (Empty MPI -> hex) +Test mpi_read_write_string #8 (Empty MPI hex -> hex) mpi_read_write_string:16:"":16:"00":4:0:0 -Test mpi_read_write_string #9 (Empty MPI -> dec) +Test mpi_read_write_string #9 (Empty MPI hex -> dec) mpi_read_write_string:16:"":10:"0":4:0:0 +Test mpi_read_write_string #8 (Empty MPI dec -> hex) +mpi_read_write_string:10:"":16:"00":4:0:0 + +Test mpi_read_write_string #9 (Empty MPI dec -> dec) +mpi_read_write_string:10:"":10:"0":4:0:0 + Test mpi_write_string #10 (Negative hex with odd number of digits) mpi_read_write_string:16:"-1":16:"":3:0:MBEDTLS_ERR_MPI_BUFFER_TOO_SMALL From 984fd07c53ec2b753cfe68fa317ca3f8240e2536 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 3 Apr 2021 18:26:13 +0200 Subject: [PATCH 2/7] Fix and simplify sign handling in mbedtls_mpi_read_string Move the handling of the sign out of the base-specific loops. This both simplifies the code, and corrects an edge case: the code in the non-hexadecimal case depended on mbedtls_mpi_mul_int() preserving the sign bit when multiplying a "negative zero" MPI by an integer, which used to be the case but stopped with PR #2512. Fix #4295. Thanks to Guido Vranken for analyzing the cause of the bug. Credit to OSS-Fuzz. Signed-off-by: Gilles Peskine --- library/bignum.c | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index f133f6c13..bd352e1bb 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -500,6 +500,7 @@ int mbedtls_mpi_read_string( mbedtls_mpi *X, int radix, const char *s ) { int ret; size_t i, j, slen, n; + int sign = 1; mbedtls_mpi_uint d; mbedtls_mpi T; MPI_VALIDATE_RET( X != NULL ); @@ -510,6 +511,12 @@ int mbedtls_mpi_read_string( mbedtls_mpi *X, int radix, const char *s ) mbedtls_mpi_init( &T ); + if( s[0] == '-' ) + { + ++s; + sign = -1; + } + slen = strlen( s ); if( radix == 16 ) @@ -524,12 +531,6 @@ int mbedtls_mpi_read_string( mbedtls_mpi *X, int radix, const char *s ) for( i = slen, j = 0; i > 0; i--, j++ ) { - if( i == 1 && s[i - 1] == '-' ) - { - X->s = -1; - break; - } - MBEDTLS_MPI_CHK( mpi_get_digit( &d, radix, s[i - 1] ) ); X->p[j / ( 2 * ciL )] |= d << ( ( j % ( 2 * ciL ) ) << 2 ); } @@ -540,26 +541,15 @@ int mbedtls_mpi_read_string( mbedtls_mpi *X, int radix, const char *s ) for( i = 0; i < slen; i++ ) { - if( i == 0 && s[i] == '-' ) - { - X->s = -1; - continue; - } - MBEDTLS_MPI_CHK( mpi_get_digit( &d, radix, s[i] ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_mul_int( &T, X, radix ) ); - - if( X->s == 1 ) - { - MBEDTLS_MPI_CHK( mbedtls_mpi_add_int( X, &T, d ) ); - } - else - { - MBEDTLS_MPI_CHK( mbedtls_mpi_sub_int( X, &T, d ) ); - } + MBEDTLS_MPI_CHK( mbedtls_mpi_add_int( X, &T, d ) ); } } + if( sign < 0 && mbedtls_mpi_bitlen( X ) != 0 ) + X->s = -1; + cleanup: mbedtls_mpi_free( &T ); From c3ccae7fafd16c4ab53a4ed9f8999555559a857e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 3 Apr 2021 18:31:01 +0200 Subject: [PATCH 3/7] Unit test function for mbedtls_ecp_muladd Write a simple unit test for mbedtls_ecp_muladd(). Add just one pair of test cases. One of them causes the argument to fix_negative to have an argument with an all-bits-zero least significant limb which briefly triggered a branch in Mbed TLS 2.26+. See https://github.com/ARMmbed/mbedtls/issues/4296 and https://github.com/ARMmbed/mbedtls/pull/4297. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_ecp.data | 8 +++++ tests/suites/test_suite_ecp.function | 46 ++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/tests/suites/test_suite_ecp.data b/tests/suites/test_suite_ecp.data index 2c25cd7c4..398ba597c 100644 --- a/tests/suites/test_suite_ecp.data +++ b/tests/suites/test_suite_ecp.data @@ -344,6 +344,14 @@ ECP point multiplication rng fail Curve25519 depends_on:MBEDTLS_ECP_DP_CURVE25519_ENABLED ecp_test_mul_rng:MBEDTLS_ECP_DP_CURVE25519:"5AC99F33632E5A768DE7E81BF854C27C46E3FBF2ABBACD29EC4AFF517369C660" +ECP point muladd secp256r1 #1 +depends_on:MBEDTLS_ECP_DP_SECP256R1_ENABLED +ecp_muladd:MBEDTLS_ECP_DP_SECP256R1:"01":"04e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e0e1ff20e1ffe120e1e1e173287170a761308491683e345cacaebb500c96e1a7bbd37772968b2c951f0579":"01":"04e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1ffffffff20e120e1e1e1e13a4e135157317b79d4ecf329fed4f9eb00dc67dbddae33faca8b6d8a0255b5ce":"04fab65e09aa5dd948320f86246be1d3fc571e7f799d9005170ed5cc868b67598431a668f96aa9fd0b0eb15f0edf4c7fe1be2885eadcb57e3db4fdd093585d3fa6" + +ECP point muladd secp256r1 #2 +depends_on:MBEDTLS_ECP_DP_SECP256R1_ENABLED +ecp_muladd:MBEDTLS_ECP_DP_SECP256R1:"01":"04e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1ffffffff20e120e1e1e1e13a4e135157317b79d4ecf329fed4f9eb00dc67dbddae33faca8b6d8a0255b5ce":"01":"04e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e1e0e1ff20e1ffe120e1e1e173287170a761308491683e345cacaebb500c96e1a7bbd37772968b2c951f0579":"04fab65e09aa5dd948320f86246be1d3fc571e7f799d9005170ed5cc868b67598431a668f96aa9fd0b0eb15f0edf4c7fe1be2885eadcb57e3db4fdd093585d3fa6" + ECP test vectors secp192k1 depends_on:MBEDTLS_ECP_DP_SECP192K1_ENABLED ecp_test_vect:MBEDTLS_ECP_DP_SECP192K1:"D1E13A359F6E0F0698791938E6D60246030AE4B0D8D4E9DE":"281BCA982F187ED30AD5E088461EBE0A5FADBB682546DF79":"3F68A8E9441FB93A4DD48CB70B504FCC9AA01902EF5BE0F3":"BE97C5D2A1A94D081E3FACE53E65A27108B7467BDF58DE43":"5EB35E922CD693F7947124F5920022C4891C04F6A8B8DCB2":"60ECF73D0FC43E0C42E8E155FFE39F9F0B531F87B34B6C3C":"372F5C5D0E18313C82AEF940EC3AFEE26087A46F1EBAE923":"D5A9F9182EC09CEAEA5F57EA10225EC77FA44174511985FD" diff --git a/tests/suites/test_suite_ecp.function b/tests/suites/test_suite_ecp.function index e37a017a6..9c90e9c2a 100644 --- a/tests/suites/test_suite_ecp.function +++ b/tests/suites/test_suite_ecp.function @@ -699,6 +699,52 @@ exit: } /* END_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED */ +void ecp_muladd( int id, + data_t *u1_bin, data_t *P1_bin, + data_t *u2_bin, data_t *P2_bin, + data_t *expected_result ) +{ + /* Compute R = u1 * P1 + u2 * P2 */ + mbedtls_ecp_group grp; + mbedtls_ecp_point P1, P2, R; + mbedtls_mpi u1, u2; + uint8_t actual_result[MBEDTLS_ECP_MAX_PT_LEN]; + size_t len; + + mbedtls_ecp_group_init( &grp ); + mbedtls_ecp_point_init( &P1 ); + mbedtls_ecp_point_init( &P2 ); + mbedtls_ecp_point_init( &R ); + mbedtls_mpi_init( &u1 ); + mbedtls_mpi_init( &u2 ); + + TEST_EQUAL( 0, mbedtls_ecp_group_load( &grp, id ) ); + TEST_EQUAL( 0, mbedtls_mpi_read_binary( &u1, u1_bin->x, u1_bin->len ) ); + TEST_EQUAL( 0, mbedtls_mpi_read_binary( &u2, u2_bin->x, u2_bin->len ) ); + TEST_EQUAL( 0, mbedtls_ecp_point_read_binary( &grp, &P1, + P1_bin->x, P1_bin->len ) ); + TEST_EQUAL( 0, mbedtls_ecp_point_read_binary( &grp, &P2, + P2_bin->x, P2_bin->len ) ); + + TEST_EQUAL( 0, mbedtls_ecp_muladd( &grp, &R, &u1, &P1, &u2, &P2 ) ); + TEST_EQUAL( 0, mbedtls_ecp_point_write_binary( + &grp, &R, MBEDTLS_ECP_PF_UNCOMPRESSED, + &len, actual_result, sizeof( actual_result ) ) ); + + ASSERT_COMPARE( expected_result->x, expected_result->len, + actual_result, len ); + +exit: + mbedtls_ecp_group_free( &grp ); + mbedtls_ecp_point_free( &P1 ); + mbedtls_ecp_point_free( &P2 ); + mbedtls_ecp_point_free( &R ); + mbedtls_mpi_free( &u1 ); + mbedtls_mpi_free( &u2 ); +} +/* END_CASE */ + /* BEGIN_CASE */ void ecp_fast_mod( int id, char * N_str ) { From 2c8cfcf59f404a8dd84f75e45022507a1ceec800 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 3 Apr 2021 21:40:11 +0200 Subject: [PATCH 4/7] Fix an incorrect comment about fix_negative We're subtracting multiples of 2^bits, not 2^(bits+32). Signed-off-by: Gilles Peskine --- library/ecp_curves.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/ecp_curves.c b/library/ecp_curves.c index b04596b56..396734d17 100644 --- a/library/ecp_curves.c +++ b/library/ecp_curves.c @@ -1048,13 +1048,13 @@ static inline void sub32( uint32_t *dst, uint32_t src, signed char *carry ) /* * If the result is negative, we get it in the form - * c * 2^(bits + 32) + N, with c negative and N positive shorter than 'bits' + * c * 2^bits + N, with c negative and N positive shorter than 'bits' */ static inline int fix_negative( mbedtls_mpi *N, signed char c, mbedtls_mpi *C, size_t bits ) { int ret; - /* C = - c * 2^(bits + 32) */ + /* C = - c * 2^bits */ #if !defined(MBEDTLS_HAVE_INT64) ((void) bits); #else From cd7d074ff9c629149fa5331e322225d864478da5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 9 Apr 2021 17:11:34 +0200 Subject: [PATCH 5/7] mbedtls_mpi_read_string("-0") no longer produces a "negative zero" Signed-off-by: Gilles Peskine --- ChangeLog.d/mpi_read_negative_zero.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 ChangeLog.d/mpi_read_negative_zero.txt diff --git a/ChangeLog.d/mpi_read_negative_zero.txt b/ChangeLog.d/mpi_read_negative_zero.txt new file mode 100644 index 000000000..f540fbfa9 --- /dev/null +++ b/ChangeLog.d/mpi_read_negative_zero.txt @@ -0,0 +1,3 @@ +Bugfix + * Fix mbedtls_mpi_read_string on "-0" returning a ``negative zero'' object, + which the library does fully consistently treat as equal to zero. From 08d67373685e3e97429c028eee6df266f33eca9b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 9 Apr 2021 20:20:26 +0200 Subject: [PATCH 6/7] Explain the problem in more concrete terms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Don't try to make the reader guess what a “negative zero” might mean. Signed-off-by: Gilles Peskine --- ChangeLog.d/mpi_read_negative_zero.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ChangeLog.d/mpi_read_negative_zero.txt b/ChangeLog.d/mpi_read_negative_zero.txt index f540fbfa9..e338de70b 100644 --- a/ChangeLog.d/mpi_read_negative_zero.txt +++ b/ChangeLog.d/mpi_read_negative_zero.txt @@ -1,3 +1,3 @@ Bugfix - * Fix mbedtls_mpi_read_string on "-0" returning a ``negative zero'' object, - which the library does fully consistently treat as equal to zero. + * mbedtls_mpi_read_string on "-0" produced an MPI object that was not treated + as equal to 0 in all cases. Fix it to produce the same object as "0". From 5a1d0fc55fc1c37642e10ecc3660c484a59d939c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 9 Apr 2021 15:46:40 +0200 Subject: [PATCH 7/7] Fix copypasta in test case description Signed-off-by: Gilles Peskine --- tests/suites/test_suite_mpi.data | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 86019e55c..b7f7ee53d 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -34,7 +34,7 @@ mpi_read_write_string:10:"-23":10:"-23":100:0:0 Base test mpi_read_write_string #3 (Negative decimal, leading 0) mpi_read_write_string:10:"-023":10:"-23":100:0:0 -Base test mpi_read_write_string #3 (Negative decimal -> hex) +Base test mpi_read_write_string #3 (Negative hex -> decimal) mpi_read_write_string:16:"-20":10:"-32":100:0:0 Base test mpi_read_write_string #3 (Negative hex)