From 6dba3200d4d656635f28a28f7c8180078c8ed303 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 2 Jun 2021 23:21:07 +0200 Subject: [PATCH 1/4] Fail the build if MBEDTLS_ECP_MAX_BITS is not large enough Signed-off-by: Gilles Peskine --- ChangeLog.d/ecp_max_bits.txt | 4 ++++ include/mbedtls/ecp.h | 42 +++++++++++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 ChangeLog.d/ecp_max_bits.txt diff --git a/ChangeLog.d/ecp_max_bits.txt b/ChangeLog.d/ecp_max_bits.txt new file mode 100644 index 0000000000..bfbe11f13c --- /dev/null +++ b/ChangeLog.d/ecp_max_bits.txt @@ -0,0 +1,4 @@ +Security + * It was possible to configure MBEDTLS_ECP_MAX_BITS to a value that is + too small, leading to buffer overflows in ECC operations. Fail the build + in such a case. diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index b974e5984b..a4aa3a30e1 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -171,6 +171,40 @@ typedef struct mbedtls_ecp_point } mbedtls_ecp_point; +/* Determine the minimum safe value of MBEDTLS_ECP_MAX_BITS. */ +#if !defined(MBEDTLS_ECP_C) +#define MBEDTLS_ECP_MAX_BITS_MIN 0 +/* Note: the curves must be listed in DECREASING size! */ +#elif defined(MBEDTLS_ECP_DP_SECP521R1_ENABLED) +#define MBEDTLS_ECP_MAX_BITS_MIN 521 +#elif defined(MBEDTLS_ECP_DP_BP512R1_ENABLED) +#define MBEDTLS_ECP_MAX_BITS_MIN 512 +#elif defined(MBEDTLS_ECP_DP_CURVE448_ENABLED) +#define MBEDTLS_ECP_MAX_BITS_MIN 448 +#elif defined(MBEDTLS_ECP_DP_BP384R1_ENABLED) +#define MBEDTLS_ECP_MAX_BITS_MIN 384 +#elif defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED) +#define MBEDTLS_ECP_MAX_BITS_MIN 384 +#elif defined(MBEDTLS_ECP_DP_BP256R1_ENABLED) +#define MBEDTLS_ECP_MAX_BITS_MIN 256 +#elif defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) +#define MBEDTLS_ECP_MAX_BITS_MIN 256 +#elif defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) +#define MBEDTLS_ECP_MAX_BITS_MIN 256 +#elif defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) +#define MBEDTLS_ECP_MAX_BITS_MIN 255 +#elif defined(MBEDTLS_ECP_DP_SECP224K1_ENABLED) +#define MBEDTLS_ECP_MAX_BITS_MIN 225 // n is slightly above 2^224 +#elif defined(MBEDTLS_ECP_DP_SECP224R1_ENABLED) +#define MBEDTLS_ECP_MAX_BITS_MIN 224 +#elif defined(MBEDTLS_ECP_DP_SECP192K1_ENABLED) +#define MBEDTLS_ECP_MAX_BITS_MIN 192 +#elif defined(MBEDTLS_ECP_DP_SECP192R1_ENABLED) +#define MBEDTLS_ECP_MAX_BITS_MIN 192 +#else +#error "MBEDTLS_ECP_C enabled, but no curve?" +#endif + #if !defined(MBEDTLS_ECP_ALT) /* * default mbed TLS elliptic curve arithmetic implementation @@ -245,7 +279,13 @@ mbedtls_ecp_group; * \{ */ -#if !defined(MBEDTLS_ECP_MAX_BITS) +#if defined(MBEDTLS_ECP_MAX_BITS) + +#if MBEDTLS_ECP_MAX_BITS < MBEDTLS_ECP_MAX_BITS_MIN +#error "MBEDTLS_ECP_MAX_BITS is smaller than the largest supported curve" +#endif + +#else /** * The maximum size of the groups, that is, of \c N and \c P. */ From e57bad4b424d5a3f51ffbab39c487da86563d2a5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 31 May 2021 21:44:25 +0200 Subject: [PATCH 2/4] Check MBEDTLS_ECP_MAX_xxx constants in unit tests Signed-off-by: Gilles Peskine --- tests/suites/test_suite_ecp.function | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/suites/test_suite_ecp.function b/tests/suites/test_suite_ecp.function index 934598dd37..3ca563e8c9 100644 --- a/tests/suites/test_suite_ecp.function +++ b/tests/suites/test_suite_ecp.function @@ -386,6 +386,8 @@ void mbedtls_ecp_curve_info( int id, int tls_id, int size, char * name ) TEST_ASSERT( by_id == by_name ); TEST_ASSERT( by_id->bit_size == size ); + TEST_ASSERT( size <= MBEDTLS_ECP_MAX_BITS ); + TEST_ASSERT( size <= MBEDTLS_ECP_MAX_BYTES * 8 ); } /* END_CASE */ @@ -794,6 +796,7 @@ void ecp_muladd( int id, TEST_EQUAL( 0, mbedtls_ecp_point_write_binary( &grp, &R, MBEDTLS_ECP_PF_UNCOMPRESSED, &len, actual_result, sizeof( actual_result ) ) ); + TEST_ASSERT( len <= MBEDTLS_ECP_MAX_PT_LEN ); ASSERT_COMPARE( expected_result->x, expected_result->len, actual_result, len ); @@ -865,6 +868,7 @@ void ecp_write_binary( int id, char * x, char * y, char * z, int format, if( ret == 0 ) { + TEST_ASSERT( olen <= MBEDTLS_ECP_MAX_PT_LEN ); TEST_ASSERT( mbedtls_test_hexcmp( buf, out->x, olen, out->len ) == 0 ); } From 33c92f01a00fc2590a27341be15ca1c91e106a52 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 2 Jun 2021 23:34:02 +0200 Subject: [PATCH 3/4] Determine MBEDTLS_ECP_MAX_BITS automatically MBEDTLS_ECP_MAX_BITS is now determined automatically from the configured curves and no longer needs to be configured explicitly to save RAM. Setting it explicit in config.h is still supported for backward compatibility. Signed-off-by: Gilles Peskine --- ChangeLog.d/ecp_max_bits.txt | 4 ++++ configs/config-suite-b.h | 3 +-- configs/config-thread.h | 3 +-- include/mbedtls/config.h | 2 +- include/mbedtls/ecp.h | 10 ++++++++-- 5 files changed, 15 insertions(+), 7 deletions(-) diff --git a/ChangeLog.d/ecp_max_bits.txt b/ChangeLog.d/ecp_max_bits.txt index bfbe11f13c..834dedaa56 100644 --- a/ChangeLog.d/ecp_max_bits.txt +++ b/ChangeLog.d/ecp_max_bits.txt @@ -2,3 +2,7 @@ Security * It was possible to configure MBEDTLS_ECP_MAX_BITS to a value that is too small, leading to buffer overflows in ECC operations. Fail the build in such a case. + +Features + * MBEDTLS_ECP_MAX_BITS is now determined automatically from the configured + curves and no longer needs to be configured explicitly to save RAM. diff --git a/configs/config-suite-b.h b/configs/config-suite-b.h index 6eb03a97e4..9cad382739 100644 --- a/configs/config-suite-b.h +++ b/configs/config-suite-b.h @@ -80,8 +80,7 @@ #define MBEDTLS_AES_ROM_TABLES /* Save RAM by adjusting to our exact needs */ -#define MBEDTLS_ECP_MAX_BITS 384 -#define MBEDTLS_MPI_MAX_SIZE 48 // 384 bits is 48 bytes +#define MBEDTLS_MPI_MAX_SIZE 48 // 48 bytes for a 384-bit elliptic curve /* Save RAM at the expense of speed, see ecp.h */ #define MBEDTLS_ECP_WINDOW_SIZE 2 diff --git a/configs/config-thread.h b/configs/config-thread.h index 47dd5e2227..8464fcb1bd 100644 --- a/configs/config-thread.h +++ b/configs/config-thread.h @@ -81,8 +81,7 @@ #define MBEDTLS_AES_ROM_TABLES /* Save RAM by adjusting to our exact needs */ -#define MBEDTLS_ECP_MAX_BITS 256 -#define MBEDTLS_MPI_MAX_SIZE 32 // 256 bits is 32 bytes +#define MBEDTLS_MPI_MAX_SIZE 32 // 32 bytes for a 256-bit elliptic curve /* Save ROM and a few bytes of RAM by specifying our own ciphersuite list */ #define MBEDTLS_SSL_CIPHERSUITES MBEDTLS_TLS_ECJPAKE_WITH_AES_128_CCM_8 diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index d0e61c5453..79aacf7b8a 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -3616,7 +3616,7 @@ //#define MBEDTLS_HMAC_DRBG_MAX_SEED_INPUT 384 /**< Maximum size of (re)seed buffer */ /* ECP options */ -//#define MBEDTLS_ECP_MAX_BITS 521 /**< Maximum bit size of groups */ +//#define MBEDTLS_ECP_MAX_BITS 521 /**< Maximum bit size of groups. Normally determined automatically from the configured curves. */ //#define MBEDTLS_ECP_WINDOW_SIZE 6 /**< Maximum window size used */ //#define MBEDTLS_ECP_FIXED_POINT_OPTIM 1 /**< Enable fixed-point speed-up */ diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index a4aa3a30e1..42630e981c 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -285,11 +285,17 @@ mbedtls_ecp_group; #error "MBEDTLS_ECP_MAX_BITS is smaller than the largest supported curve" #endif -#else +#elif defined(MBEDTLS_ECP_C) /** * The maximum size of the groups, that is, of \c N and \c P. */ -#define MBEDTLS_ECP_MAX_BITS 521 /**< The maximum size of groups, in bits. */ +#define MBEDTLS_ECP_MAX_BITS MBEDTLS_ECP_MAX_BITS_MIN + +#else +/* MBEDTLS_ECP_MAX_BITS is not relevant without MBEDTLS_ECP_C, but set it + * to a nonzero value so that code that unconditionally allocates an array + * of a size based on it keeps working if built without ECC support. */ +#define MBEDTLS_ECP_MAX_BITS 1 #endif #define MBEDTLS_ECP_MAX_BYTES ( ( MBEDTLS_ECP_MAX_BITS + 7 ) / 8 ) From 3223940938a5aa537b31f288e1395643c341e4dc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 7 Jun 2021 20:24:51 +0200 Subject: [PATCH 4/4] Update MBEDTLS_ECP_MAX_BITS_MIN when adding a curve Signed-off-by: Gilles Peskine --- include/mbedtls/ecp.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index 42630e981c..48eec61d5e 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -96,6 +96,7 @@ extern "C" { * - Add it at the end of this enum, otherwise you'll break the ABI by * changing the numerical value for existing curves. * - Increment MBEDTLS_ECP_DP_MAX below if needed. + * - Update the calculation of MBEDTLS_ECP_MAX_BITS_MIN below. * - Add the corresponding MBEDTLS_ECP_DP_xxx_ENABLED macro definition to * config.h. * - List the curve as a dependency of MBEDTLS_ECP_C and