From b13a26cd8c599911919d63204f6db2b8b1a3a9ae Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 3 Apr 2021 18:25:29 +0200 Subject: [PATCH 1/9] Add a few unit tests for mbedtls_mpi_read_string with leading zeros Signed-off-by: Gilles Peskine --- tests/suites/test_suite_mpi.data | 36 ++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index b5f68447f..36e66726b 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 (Negative decimal) [#1] +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) [#2] +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 80f56733b0aeb6ce19f0ea929c746019ba410c45 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 3 Apr 2021 18:26:13 +0200 Subject: [PATCH 2/9] 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 56d7dbe0f..bfca43d90 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -470,6 +470,7 @@ int mbedtls_mpi_read_string( mbedtls_mpi *X, int radix, const char *s ) { int ret = MBEDTLS_ERR_ERROR_CORRUPTION_DETECTED; size_t i, j, slen, n; + int sign = 1; mbedtls_mpi_uint d; mbedtls_mpi T; MPI_VALIDATE_RET( X != NULL ); @@ -480,6 +481,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 ) @@ -494,12 +501,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 ); } @@ -510,26 +511,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 ca91ee4ed8587b59d8f91e1a04c595c44e982b41 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 3 Apr 2021 18:31:01 +0200 Subject: [PATCH 3/9] Unit test function for mbedtls_ecp_muladd Write a simple unit test for mbedtls_ecp_muladd(). Add just one pair of test cases. #2 fails since PR #3512. Thanks to Philippe Antoine (catenacyber) for the test case, found by ecfuzzer. 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 408a9b7fe..59dfa4f2d 100644 --- a/tests/suites/test_suite_ecp.data +++ b/tests/suites/test_suite_ecp.data @@ -458,6 +458,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 Curve448 (RFC 7748 6.2, after decodeUCoordinate) depends_on:MBEDTLS_ECP_DP_CURVE448_ENABLED ecp_test_vec_x:MBEDTLS_ECP_DP_CURVE448:"eb7298a5c0d8c29a1dab27f1a6826300917389449741a974f5bac9d98dc298d46555bce8bae89eeed400584bb046cf75579f51d125498f98":"a01fc432e5807f17530d1288da125b0cd453d941726436c8bbd9c5222c3da7fa639ce03db8d23b274a0721a1aed5227de6e3b731ccf7089b":"ad997351b6106f36b0d1091b929c4c37213e0d2b97e85ebb20c127691d0dad8f1d8175b0723745e639a3cb7044290b99e0e2a0c27a6a301c":"0936f37bc6c1bd07ae3dec7ab5dc06a73ca13242fb343efc72b9d82730b445f3d4b0bd077162a46dcfec6f9b590bfcbcf520cdb029a8b73e":"9d874a5137509a449ad5853040241c5236395435c36424fd560b0cb62b281d285275a740ce32a22dd1740f4aa9161cec95ccc61a18f4ff07" diff --git a/tests/suites/test_suite_ecp.function b/tests/suites/test_suite_ecp.function index 4ee75a628..8d47edaa4 100644 --- a/tests/suites/test_suite_ecp.function +++ b/tests/suites/test_suite_ecp.function @@ -752,6 +752,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 80ba850e277e2e87f820e64e54989738b7c276cb Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 3 Apr 2021 20:36:37 +0200 Subject: [PATCH 4/9] Create a header file for ECP internal functions This header file will contain declarations of functions that are not part of the public ABI/API, and must not be called from other modules, but can be called from unit tests. Signed-off-by: Gilles Peskine --- library/ecp.c | 2 ++ library/ecp_invasive.h | 36 ++++++++++++++++++++++++++++++++++ visualc/VS2010/mbedTLS.vcxproj | 1 + 3 files changed, 39 insertions(+) create mode 100644 library/ecp_invasive.h diff --git a/library/ecp.c b/library/ecp.c index 6a005d510..b07d6a222 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -77,6 +77,8 @@ #include "mbedtls/platform_util.h" #include "mbedtls/error.h" +#include "ecp_invasive.h" + #include #if !defined(MBEDTLS_ECP_ALT) diff --git a/library/ecp_invasive.h b/library/ecp_invasive.h new file mode 100644 index 000000000..7dd8ac211 --- /dev/null +++ b/library/ecp_invasive.h @@ -0,0 +1,36 @@ +/** + * \file ecp_invasive.h + * + * \brief ECP module: interfaces for invasive testing only. + * + * The interfaces in this file are intended for testing purposes only. + * They SHOULD NOT be made available in library integrations except when + * building the library for testing. + */ +/* + * Copyright The Mbed TLS Contributors + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#ifndef MBEDTLS_ECP_INVASIVE_H +#define MBEDTLS_ECP_INVASIVE_H + +#include "common.h" +#include "mbedtls/ecp.h" + +#if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_ECP_C) + +#endif /* MBEDTLS_TEST_HOOKS && MBEDTLS_ECP_C */ + +#endif /* MBEDTLS_ECP_INVASIVE_H */ diff --git a/visualc/VS2010/mbedTLS.vcxproj b/visualc/VS2010/mbedTLS.vcxproj index 09c5341fb..01588312c 100644 --- a/visualc/VS2010/mbedTLS.vcxproj +++ b/visualc/VS2010/mbedTLS.vcxproj @@ -251,6 +251,7 @@ + From 618be2ec41012fe390ad9a0eecc9e8d706fc9859 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 3 Apr 2021 21:47:53 +0200 Subject: [PATCH 5/9] Add unit tests for fix_negative Signed-off-by: Gilles Peskine --- library/ecp_curves.c | 7 +- library/ecp_invasive.h | 14 +++ tests/suites/test_suite_ecp.data | 124 +++++++++++++++++++++++++++ tests/suites/test_suite_ecp.function | 39 +++++++++ 4 files changed, 182 insertions(+), 2 deletions(-) diff --git a/library/ecp_curves.c b/library/ecp_curves.c index 962d5af9b..b167443cf 100644 --- a/library/ecp_curves.c +++ b/library/ecp_curves.c @@ -25,6 +25,8 @@ #include "mbedtls/platform_util.h" #include "mbedtls/error.h" +#include "ecp_invasive.h" + #include #if !defined(MBEDTLS_ECP_ALT) @@ -1028,13 +1030,14 @@ static inline void sub32( uint32_t *dst, uint32_t src, signed char *carry ) STORE32; i++; \ cur = c > 0 ? c : 0; STORE32; \ cur = 0; while( ++i < MAX32 ) { STORE32; } \ - if( c < 0 ) fix_negative( N, c, bits ); + if( c < 0 ) mbedtls_ecp_fix_negative( N, c, bits ); /* * 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' */ -static inline void fix_negative( mbedtls_mpi *N, signed char c, size_t bits ) +MBEDTLS_STATIC_TESTABLE +void mbedtls_ecp_fix_negative( mbedtls_mpi *N, signed char c, size_t bits ) { size_t i; diff --git a/library/ecp_invasive.h b/library/ecp_invasive.h index 7dd8ac211..870d9637c 100644 --- a/library/ecp_invasive.h +++ b/library/ecp_invasive.h @@ -31,6 +31,20 @@ #if defined(MBEDTLS_TEST_HOOKS) && defined(MBEDTLS_ECP_C) +#if defined(MBEDTLS_ECP_DP_SECP224R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED) +/* Preconditions: + * - bits is a multiple of 64 or is 224 + * - c is -1 or -2 + * - 0 <= N < 2^bits + * - N has room for bits+64 bits + * + * Set N to c * 2^bits + N. + */ +void mbedtls_ecp_fix_negative( mbedtls_mpi *N, signed char c, size_t bits ); +#endif + #endif /* MBEDTLS_TEST_HOOKS && MBEDTLS_ECP_C */ #endif /* MBEDTLS_ECP_INVASIVE_H */ diff --git a/tests/suites/test_suite_ecp.data b/tests/suites/test_suite_ecp.data index 59dfa4f2d..106791cb8 100644 --- a/tests/suites/test_suite_ecp.data +++ b/tests/suites/test_suite_ecp.data @@ -516,3 +516,127 @@ ecp_muladd_restart:MBEDTLS_ECP_DP_SECP256R1:"CB28E0999B9C7715FD0A80D8E47A7707971 ECP restartable muladd secp256r1 max_ops=250 depends_on:MBEDTLS_ECP_DP_SECP256R1_ENABLED ecp_muladd_restart:MBEDTLS_ECP_DP_SECP256R1:"CB28E0999B9C7715FD0A80D8E47A77079716CBBF917DD72E97566EA1C066957C":"2B57C0235FB7489768D058FF4911C20FDBE71E3699D91339AFBB903EE17255DC":"C3875E57C85038A0D60370A87505200DC8317C8C534948BEA6559C7C18E6D4CE":"3B4E49C4FDBFC006FF993C81A50EAE221149076D6EC09DDD9FB3B787F85B6483":"2442A5CC0ECD015FA3CA31DC8E2BBC70BF42D60CBCA20085E0822CB04235E970":"6FC98BD7E50211A4A27102FA3549DF79EBCB4BF246B80945CDDFE7D509BBFD7D":250:4:64 + +ECP fix_negative: 0, -1, 224 +fix_negative:"00":-1:224 + +ECP fix_negative: 1, -1, 224 +fix_negative:"01":-1:224 + +ECP fix_negative: 2^32-1, -1, 224 +fix_negative:"ffffffff":-1:224 + +ECP fix_negative: 2^32, -1, 224 +fix_negative:"0100000000":-1:224 + +ECP fix_negative: 2^64-1, -1, 224 +fix_negative:"ffffffffffffffff":-1:224 + +ECP fix_negative: 2^64, -1, 224 +fix_negative:"010000000000000000":-1:224 + +ECP fix_negative: 2^128-1, -1, 224 +fix_negative:"ffffffffffffffffffffffffffffffff":-1:224 + +ECP fix_negative: 2^128, -1, 224 +fix_negative:"0100000000000000000000000000000000":-1:224 + +ECP fix_negative: 2^128+1, -1, 224 +fix_negative:"0100000000000000000000000000000001":-1:224 + +ECP fix_negative: 2^224-1, -1, 224 +fix_negative:"ffffffffffffffffffffffffffffffffffffffffffffffffffffffff":-1:224 + +ECP fix_negative: 0, -2, 224 +fix_negative:"00":-2:224 + +ECP fix_negative: 1, -2, 224 +fix_negative:"01":-2:224 + +ECP fix_negative: 2^32-1, -2, 224 +fix_negative:"ffffffff":-2:224 + +ECP fix_negative: 2^32, -2, 224 +fix_negative:"0100000000":-2:224 + +ECP fix_negative: 2^64-1, -2, 224 +fix_negative:"ffffffffffffffff":-2:224 + +ECP fix_negative: 2^64, -2, 224 +fix_negative:"010000000000000000":-2:224 + +ECP fix_negative: 2^128-1, -2, 224 +fix_negative:"ffffffffffffffffffffffffffffffff":-2:224 + +ECP fix_negative: 2^128, -2, 224 +fix_negative:"0100000000000000000000000000000000":-2:224 + +ECP fix_negative: 2^128+1, -2, 224 +fix_negative:"0100000000000000000000000000000001":-2:224 + +ECP fix_negative: 2^224-1, -2, 224 +fix_negative:"ffffffffffffffffffffffffffffffffffffffffffffffffffffffff":-2:224 + +ECP fix_negative: 0, -1, 256 +fix_negative:"00":-1:256 + +ECP fix_negative: 1, -1, 256 +fix_negative:"01":-1:256 + +ECP fix_negative: 2^32-1, -1, 256 +fix_negative:"ffffffff":-1:256 + +ECP fix_negative: 2^32, -1, 256 +fix_negative:"0100000000":-1:256 + +ECP fix_negative: 2^64-1, -1, 256 +fix_negative:"ffffffffffffffff":-1:256 + +ECP fix_negative: 2^64, -1, 256 +fix_negative:"010000000000000000":-1:256 + +ECP fix_negative: 2^128-1, -1, 256 +fix_negative:"ffffffffffffffffffffffffffffffff":-1:256 + +ECP fix_negative: 2^128, -1, 256 +fix_negative:"0100000000000000000000000000000000":-1:256 + +ECP fix_negative: 2^128+1, -1, 256 +fix_negative:"0100000000000000000000000000000001":-1:256 + +ECP fix_negative: 2^256-1, -1, 256 +fix_negative:"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff":-1:256 + +ECP fix_negative: 0, -2, 256 +fix_negative:"00":-2:256 + +ECP fix_negative: 1, -2, 256 +fix_negative:"01":-2:256 + +ECP fix_negative: 2^32-1, -2, 256 +fix_negative:"ffffffff":-2:256 + +ECP fix_negative: 2^32, -2, 256 +fix_negative:"0100000000":-2:256 + +ECP fix_negative: 2^64-1, -2, 256 +fix_negative:"ffffffffffffffff":-2:256 + +ECP fix_negative: 2^64, -2, 256 +fix_negative:"010000000000000000":-2:256 + +ECP fix_negative: 2^128-1, -2, 256 +fix_negative:"ffffffffffffffffffffffffffffffff":-2:256 + +ECP fix_negative: 2^128, -2, 256 +fix_negative:"0100000000000000000000000000000000":-2:256 + +ECP fix_negative: 2^128+1, -2, 256 +fix_negative:"0100000000000000000000000000000001":-2:256 + +ECP fix_negative: 2^256-1, -2, 256 +fix_negative:"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff":-2:256 + +# The first call to fix_negative in the test case of issue #4296. +ECP fix_negative: #4296.1 +fix_negative:"8A4DD4C8B42C5EAED15FE4F4579F4CE513EC90A94010BF000000000000000000":-1:256 diff --git a/tests/suites/test_suite_ecp.function b/tests/suites/test_suite_ecp.function index 8d47edaa4..0ca2fdf4d 100644 --- a/tests/suites/test_suite_ecp.function +++ b/tests/suites/test_suite_ecp.function @@ -1,6 +1,15 @@ /* BEGIN_HEADER */ #include "mbedtls/ecp.h" +#include "ecp_invasive.h" + +#if defined(MBEDTLS_TEST_HOOKS) && \ + ( defined(MBEDTLS_ECP_DP_SECP224R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED) ) +#define HAVE_FIX_NEGATIVE +#endif + #define ECP_PF_UNKNOWN -1 #define ECP_PT_RESET( x ) \ @@ -1198,6 +1207,36 @@ exit: } /* END_CASE */ +/* BEGIN_CASE depends_on:HAVE_FIX_NEGATIVE */ +void fix_negative( data_t *N_bin, int c, int bits ) +{ + mbedtls_mpi C, M, N; + + mbedtls_mpi_init( &C ); + mbedtls_mpi_init( &M ); + mbedtls_mpi_init( &N ); + + /* C = - c * 2^bits */ + TEST_EQUAL( 0, mbedtls_mpi_lset( &C, -c ) ); + TEST_EQUAL( 0, mbedtls_mpi_shift_l( &C, bits ) ); + + TEST_EQUAL( 0, mbedtls_mpi_read_binary( &N, N_bin->x, N_bin->len ) ); + TEST_EQUAL( 0, mbedtls_mpi_grow( &N, C.n ) ); + + /* M = - ( C - N ) */ + TEST_EQUAL( 0, mbedtls_mpi_sub_mpi( &M, &N, &C ) ); + + mbedtls_ecp_fix_negative( &N, c, bits ); + + TEST_EQUAL( 0, mbedtls_mpi_cmp_mpi( &N, &M ) ); + +exit: + mbedtls_mpi_free( &C ); + mbedtls_mpi_free( &M ); + mbedtls_mpi_free( &N ); +} +/* END_CASE */ + /* BEGIN_CASE depends_on:MBEDTLS_SELF_TEST */ void ecp_selftest( ) { From 349b37273ee1337f9b0b3d622c0e27932580cfdd Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 3 Apr 2021 21:40:11 +0200 Subject: [PATCH 6/9] 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 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/ecp_curves.c b/library/ecp_curves.c index b167443cf..bf84effb6 100644 --- a/library/ecp_curves.c +++ b/library/ecp_curves.c @@ -1034,7 +1034,7 @@ 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' */ MBEDTLS_STATIC_TESTABLE void mbedtls_ecp_fix_negative( mbedtls_mpi *N, signed char c, size_t bits ) @@ -1049,8 +1049,8 @@ void mbedtls_ecp_fix_negative( mbedtls_mpi *N, signed char c, size_t bits ) } N->s = -1; - /* Add |c| * 2^(bits + 32) to the absolute value. Since c and N are - * negative, this adds c * 2^(bits + 32). */ + /* Add |c| * 2^bits to the absolute value. Since c and N are + * negative, this adds c * 2^bits. */ mbedtls_mpi_uint msw = (mbedtls_mpi_uint) -c; #if defined(MBEDTLS_HAVE_INT64) if( bits == 224 ) From ff6a32d79c7f30feca3cac9c3a3dc45c23c99bec Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Sat, 3 Apr 2021 20:21:43 +0200 Subject: [PATCH 7/9] Fix low-probability arithmetic error in ECC Fix the subtraction in fix_negative, which was incorrectly not looking for a carry. This caused the result to be wrong when the least significant limb of N was 0. Fix #4296. The bug was introduced by d10e8fae9e30cac60297b1e1834002db183429e5 "Optimize fix_negative". Thanks to Philippe Antoine (catenacyber) for reporting the bug which was found by his EC differential fuzzer. Credit to OSS-Fuzz. Signed-off-by: Gilles Peskine --- library/ecp_curves.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/library/ecp_curves.c b/library/ecp_curves.c index bf84effb6..165c315d1 100644 --- a/library/ecp_curves.c +++ b/library/ecp_curves.c @@ -1041,12 +1041,20 @@ void mbedtls_ecp_fix_negative( mbedtls_mpi *N, signed char c, size_t bits ) { size_t i; - /* Set N := N - 2^bits */ - --N->p[0]; + /* Set N := 2^bits - 1 - N. We know that 0 <= N < 2^bits, so + * set the absolute value to 0xfff...fff - N. There is no carry + * since we're subtracting from all-bits-one. */ for( i = 0; i <= bits / 8 / sizeof( mbedtls_mpi_uint ); i++ ) { N->p[i] = ~(mbedtls_mpi_uint)0 - N->p[i]; } + /* Add 1, taking care of the carry. */ + i = 0; + do + ++N->p[i]; + while( N->p[i++] == 0 && i <= bits / 8 / sizeof( mbedtls_mpi_uint ) ); + /* Invert the sign. + * Now N = N0 - 2^bits where N0 is the initial value of N. */ N->s = -1; /* Add |c| * 2^bits to the absolute value. Since c and N are From bd43f67a9b5b401eb3946f95504e6925d3798199 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 9 Apr 2021 15:46:40 +0200 Subject: [PATCH 8/9] 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 36e66726b..59fd7824b 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) From 392d1010dc460eccd498932d8b25c127b0714ce1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 9 Apr 2021 15:46:51 +0200 Subject: [PATCH 9/9] Clarify some comments Signed-off-by: Gilles Peskine --- library/ecp_invasive.h | 5 +++-- tests/suites/test_suite_ecp.function | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/library/ecp_invasive.h b/library/ecp_invasive.h index 870d9637c..b5239676f 100644 --- a/library/ecp_invasive.h +++ b/library/ecp_invasive.h @@ -38,9 +38,10 @@ * - bits is a multiple of 64 or is 224 * - c is -1 or -2 * - 0 <= N < 2^bits - * - N has room for bits+64 bits + * - N has room for bits plus one limb * - * Set N to c * 2^bits + N. + * Behavior: + * Set N to c * 2^bits + old_value_of_N. */ void mbedtls_ecp_fix_negative( mbedtls_mpi *N, signed char c, size_t bits ); #endif diff --git a/tests/suites/test_suite_ecp.function b/tests/suites/test_suite_ecp.function index 0ca2fdf4d..6d23377f3 100644 --- a/tests/suites/test_suite_ecp.function +++ b/tests/suites/test_suite_ecp.function @@ -1216,14 +1216,14 @@ void fix_negative( data_t *N_bin, int c, int bits ) mbedtls_mpi_init( &M ); mbedtls_mpi_init( &N ); - /* C = - c * 2^bits */ + /* C = - c * 2^bits (positive since c is negative) */ TEST_EQUAL( 0, mbedtls_mpi_lset( &C, -c ) ); TEST_EQUAL( 0, mbedtls_mpi_shift_l( &C, bits ) ); TEST_EQUAL( 0, mbedtls_mpi_read_binary( &N, N_bin->x, N_bin->len ) ); TEST_EQUAL( 0, mbedtls_mpi_grow( &N, C.n ) ); - /* M = - ( C - N ) */ + /* M = N - C = - ( C - N ) (expected result of fix_negative) */ TEST_EQUAL( 0, mbedtls_mpi_sub_mpi( &M, &N, &C ) ); mbedtls_ecp_fix_negative( &N, c, bits );