From d90faf92b2d24421196ceda6475d5915fa981d1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 19 May 2020 12:38:31 +0200 Subject: [PATCH 01/14] Add config.h option MBEDTLS_ECP_NO_INTERNAL_RNG MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No effect so far, except on dependency checking, as the feature it's meant to disable isn't implemented yet (so the descriptions in config.h and the ChangeLog entry are anticipation for now). Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/ecp-internal-rng.txt | 5 +++++ include/mbedtls/check_config.h | 8 ++++++++ include/mbedtls/config.h | 22 ++++++++++++++++++++++ library/version_features.c | 3 +++ scripts/config.pl | 1 + tests/scripts/all.sh | 18 ++++++++++++++++++ 6 files changed, 57 insertions(+) create mode 100644 ChangeLog.d/ecp-internal-rng.txt diff --git a/ChangeLog.d/ecp-internal-rng.txt b/ChangeLog.d/ecp-internal-rng.txt new file mode 100644 index 000000000..bf11a7391 --- /dev/null +++ b/ChangeLog.d/ecp-internal-rng.txt @@ -0,0 +1,5 @@ +Changes + * The ECP module, enabled by `MBEDTLS_ECP_C`, now depends on + `MBEDTLS_CTR_DRBG_C` or `MBEDTLS_HMAC_DRBG_C` for some side-channel + coutermeasures. If side channels are not a concern, this dependency can + be avoided by enabling the new option `MBEDTLS_ECP_NO_INTERNAL_RNG`. diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 7b916042c..935248f4c 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -123,6 +123,14 @@ #error "MBEDTLS_ECP_C defined, but not all prerequisites" #endif +#if defined(MBEDTLS_ECP_C) && !( \ + defined(MBEDTLS_ECP_ALT) || \ + defined(MBEDTLS_CTR_DRBG_C) || \ + defined(MBEDTLS_HMAC_DRBG_C) || \ + defined(MBEDTLS_ECP_NO_INTERNAL_RNG)) +#error "MBEDTLS_ECP_C requires a DRBG module unless MBEDTLS_ECP_NO_INTERNAL_RNG is defined or an alternative implementation is used" +#endif + #if defined(MBEDTLS_PK_PARSE_C) && !defined(MBEDTLS_ASN1_PARSE_C) #error "MBEDTLS_PK_PARSE_C defined, but not all prerequesites" #endif diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 0bc08cca4..03033619d 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -618,6 +618,28 @@ */ #define MBEDTLS_ECP_NIST_OPTIM +/** + * \def MBEDTLS_ECP_NO_INTERNAL_RNG + * + * When this option is disabled, mbedtls_ecp_mul() will make use of an + * internal RNG when called with a NULL \c f_rng argument, in order to protect + * against some side-channel attacks. + * + * This protection introduces a dependency of the ECP module on one of the + * DRBG modules. For very constrained implementations that don't require this + * protection (for example, because you're only doing signature verification, + * so not manipulating any secret, or because local/physical side-channel + * attacks are outside your threat model), it might be desirable to get rid of + * that dependency. + * + * \warning Enabling this option makes some uses of ECP vulnerable to some + * side-channel attacks. Only enable it if you know that's not a problem for + * your use case. + * + * Uncomment this macro to disable some counter-measures in ECP. + */ +//#define MBEDTLS_ECP_NO_INTERNAL_RNG + /** * \def MBEDTLS_ECDSA_DETERMINISTIC * diff --git a/library/version_features.c b/library/version_features.c index d6deb0149..de0af3f10 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -312,6 +312,9 @@ static const char *features[] = { #if defined(MBEDTLS_ECP_NIST_OPTIM) "MBEDTLS_ECP_NIST_OPTIM", #endif /* MBEDTLS_ECP_NIST_OPTIM */ +#if defined(MBEDTLS_ECP_NO_INTERNAL_RNG) + "MBEDTLS_ECP_NO_INTERNAL_RNG", +#endif /* MBEDTLS_ECP_NO_INTERNAL_RNG */ #if defined(MBEDTLS_ECDSA_DETERMINISTIC) "MBEDTLS_ECDSA_DETERMINISTIC", #endif /* MBEDTLS_ECDSA_DETERMINISTIC */ diff --git a/scripts/config.pl b/scripts/config.pl index 2f04d91a1..b2c895321 100755 --- a/scripts/config.pl +++ b/scripts/config.pl @@ -72,6 +72,7 @@ EOU my @excluded = qw( MBEDTLS_DEPRECATED_REMOVED MBEDTLS_DEPRECATED_WARNING +MBEDTLS_ECP_NO_INTERNAL_RNG MBEDTLS_HAVE_SSE2 MBEDTLS_MEMORY_BACKTRACE MBEDTLS_MEMORY_BUFFER_ALLOC_C diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index c99c2a68f..4103ace65 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -814,6 +814,24 @@ component_test_no_hmac_drbg () { # so there's little value in running those lengthy tests here. } +component_test_ecp_no_internal_rng () { + msg "build: Default plus ECP_NO_INTERNAL_RNG minus DRBG modules" + scripts/config.pl set MBEDTLS_ECP_NO_INTERNAL_RNG + scripts/config.pl unset MBEDTLS_CTR_DRBG_C + scripts/config.pl unset MBEDTLS_HMAC_DRBG_C + scripts/config.pl unset MBEDTLS_ECDSA_DETERMINISTIC # requires HMAC_DRBG + scripts/config.pl unset MBEDTLS_PSA_CRYPTO_C # requires a DRBG + scripts/config.pl unset MBEDTLS_PSA_CRYPTO_STORAGE_C # requires PSA Crypto + + CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . + make + + msg "test: ECP_NO_INTERNAL_RNG, no DRBG module" + make test + + # no SSL tests as they all depend on having a DRBG +} + component_test_full_cmake_clang () { msg "build: cmake, full config, clang" # ~ 50s scripts/config.pl full From 75036a0aff227544e9a65379138b58e734d899f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 22 May 2020 12:12:36 +0200 Subject: [PATCH 02/14] Implement use of internal DRBG for ecp_mul() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The case of MBEDTLS_ECP_RESTARTABLE isn't handled correctly yet: in that case the DRBG instance should persist when resuming the operation. This will be addressed in the next commit. When both CTR_DRBG and HMAC_DRBG are available, CTR_DRBG is preferred since both are suitable but CTR_DRBG tends to be faster and I needed a tie-breaker. There are currently three possible cases to test: - NO_INTERNAL_RNG is set -> tested in test_ecp_no_internal_rng - it's unset and CTR_DRBG is available -> tested in the default config - it's unset and CTR_DRBG is disabled -> tested in test_ecp_internal_rng_no_ctr_drbg Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/md.h | 2 + library/ecp.c | 145 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 142 insertions(+), 5 deletions(-) diff --git a/include/mbedtls/md.h b/include/mbedtls/md.h index 06538c382..7c1ae7e02 100644 --- a/include/mbedtls/md.h +++ b/include/mbedtls/md.h @@ -95,6 +95,8 @@ typedef struct { * \brief This function returns the list of digests supported by the * generic digest module. * + * \note The list starts with the strongest available hashes. + * * \return A statically allocated array of digests. Each element * in the returned list is an integer belonging to the * message-digest enumeration #mbedtls_md_type_t. diff --git a/library/ecp.c b/library/ecp.c index d48e3e89c..20d7f77e7 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -67,6 +67,16 @@ #include "mbedtls/ecp_internal.h" +#if !defined(MBEDTLS_ECP_NO_INTERNAL_RNG) +#if defined(MBEDTLS_CTR_DRBG_C) +#include "mbedtls/ctr_drbg.h" +#elif defined(MBEDTLS_HMAC_DRBG_C) +#include "mbedtls/hmac_drbg.h" +#else +#error "Invalid configuration detected. Include check_config.h to ensure that the configuration is valid." +#endif +#endif /* MBEDTLS_ECP_NO_INTERNAL_RNG */ + #if ( defined(__ARMCC_VERSION) || defined(_MSC_VER) ) && \ !defined(inline) && !defined(__cplusplus) #define inline __inline @@ -85,6 +95,113 @@ static void mbedtls_zeroize( void *v, size_t n ) { static unsigned long add_count, dbl_count, mul_count; #endif +#if !defined(MBEDTLS_ECP_NO_INTERNAL_RNG) +/* + * Currently ecp_mul() takes a RNG function as an argument, used for + * side-channel protection, but it can be NULL. The initial reasonning was + * that people will pass non-NULL RNG when they care about side-channels, but + * unfortunately we have some APIs that call ecp_mul() with a NULL RNG, with + * no opportunity for the user to do anything about it. + * + * The obvious strategies for addressing that include: + * - change those APIs so that they take RNG arguments; + * - require a global RNG to be available to all crypto modules. + * + * Unfortunately those would break compatibility. So what we do instead is + * have our own internal DRBG instance, seeded from the secret scalar. + * + * The following is a light-weight abstraction layer for doing that with + * CTR_DRBG or HMAC_DRBG. + */ + +#if defined(MBEDTLS_CTR_DRBG_C) +/* DRBG context type */ +typedef mbedtls_ctr_drbg_context ecp_drbg_context; + +/* DRBG context init */ +static inline void ecp_drbg_init( ecp_drbg_context *ctx ) +{ + mbedtls_ctr_drbg_init( ctx ); +} + +/* DRBG context free */ +static inline void ecp_drbg_free( ecp_drbg_context *ctx ) +{ + mbedtls_ctr_drbg_free( ctx ); +} + +/* DRBG function */ +static inline int ecp_drbg_random( void *p_rng, + unsigned char *output, size_t output_len ) +{ + return( mbedtls_ctr_drbg_random( p_rng, output, output_len ) ); +} + +/* + * Since CTR_DRBG doesn't have a seed_buf() function the way HMAC_DRBG does, + * we need to pass an entropy function when seeding. So we use a dummy + * function for that, and pass the actual entropy as customisation string. + * (During seeding of CTR_DRBG the entropy input and customisation string are + * concatenated before being used to update the secret state.) + */ +static int ecp_ctr_drbg_null_entropy(void *ctx, unsigned char *out, size_t len) +{ + (void) ctx; + memset( out, 0, len ); + return( 0 ); +} + +/* DRBG context seeding */ +static int ecp_drbg_seed( ecp_drbg_context *ctx, const mbedtls_mpi *secret ) +{ + const unsigned char *secret_p = (const unsigned char *) secret->p; + const size_t secret_size = secret->n * sizeof( mbedtls_mpi_uint ); + + return( mbedtls_ctr_drbg_seed( ctx, ecp_ctr_drbg_null_entropy, NULL, + secret_p, secret_size ) ); +} + +#elif defined(MBEDTLS_HMAC_DRBG_C) +/* DRBG context type */ +typedef mbedtls_hmac_drbg_context ecp_drbg_context; + +/* DRBG context init */ +static inline void ecp_drbg_init( ecp_drbg_context *ctx ) +{ + mbedtls_hmac_drbg_init( ctx ); +} + +/* DRBG context free */ +static inline void ecp_drbg_free( ecp_drbg_context *ctx ) +{ + mbedtls_hmac_drbg_free( ctx ); +} + +/* DRBG function */ +static inline int ecp_drbg_random( void *p_rng, + unsigned char *output, size_t output_len ) +{ + return( mbedtls_hmac_drbg_random( p_rng, output, output_len ) ); +} + +/* DRBG context seeding */ +static int ecp_drbg_seed( ecp_drbg_context *ctx, const mbedtls_mpi *secret ) +{ + const unsigned char *secret_p = (const unsigned char *) secret->p; + const size_t secret_size = secret->n * sizeof( mbedtls_mpi_uint ); + + /* The list starts with strong hashes */ + const mbedtls_md_type_t md_type = mbedtls_md_list()[0]; + const mbedtls_md_info_t *md_info = mbedtls_md_info_from_type( md_type ); + + return( mbedtls_hmac_drbg_seed_buf( ctx, md_info, secret_p, secret_size ) ); +} + +#else +#error "Invalid configuration detected. Include check_config.h to ensure that the configuration is valid." +#endif /* DRBG modules */ +#endif /* MBEDTLS_ECP_NO_INTERNAL_RNG */ + #if defined(MBEDTLS_ECP_DP_SECP192R1_ENABLED) || \ defined(MBEDTLS_ECP_DP_SECP224R1_ENABLED) || \ defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) || \ @@ -1719,6 +1836,11 @@ int mbedtls_ecp_mul( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, #if defined(MBEDTLS_ECP_INTERNAL_ALT) char is_grp_capable = 0; #endif +#if !defined(MBEDTLS_ECP_NO_INTERNAL_RNG) + ecp_drbg_context drbg_ctx; + + ecp_drbg_init( &drbg_ctx ); +#endif /* !MBEDTLS_ECP_NO_INTERNAL_RNG */ /* Common sanity checks */ if( mbedtls_mpi_cmp_int( &P->Z, 1 ) != 0 ) @@ -1728,32 +1850,45 @@ int mbedtls_ecp_mul( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, ( ret = mbedtls_ecp_check_pubkey( grp, P ) ) != 0 ) return( ret ); +#if !defined(MBEDTLS_ECP_NO_INTERNAL_RNG) + if( f_rng == NULL ) + { + MBEDTLS_MPI_CHK( ecp_drbg_seed( &drbg_ctx, m ) ); + f_rng = &ecp_drbg_random; + p_rng = &drbg_ctx; + } +#endif /* !MBEDTLS_ECP_NO_INTERNAL_RNG */ + #if defined(MBEDTLS_ECP_INTERNAL_ALT) if ( is_grp_capable = mbedtls_internal_ecp_grp_capable( grp ) ) { MBEDTLS_MPI_CHK( mbedtls_internal_ecp_init( grp ) ); } - #endif /* MBEDTLS_ECP_INTERNAL_ALT */ + #if defined(ECP_MONTGOMERY) if( ecp_get_type( grp ) == ECP_TYPE_MONTGOMERY ) ret = ecp_mul_mxz( grp, R, m, P, f_rng, p_rng ); - #endif #if defined(ECP_SHORTWEIERSTRASS) if( ecp_get_type( grp ) == ECP_TYPE_SHORT_WEIERSTRASS ) ret = ecp_mul_comb( grp, R, m, P, f_rng, p_rng ); +#endif +#if defined(MBEDTLS_ECP_INTERNAL_ALT) || !defined(MBEDTLS_ECP_NO_INTERNAL_RNG) +cleanup: #endif #if defined(MBEDTLS_ECP_INTERNAL_ALT) -cleanup: - if ( is_grp_capable ) { mbedtls_internal_ecp_free( grp ); } - #endif /* MBEDTLS_ECP_INTERNAL_ALT */ + +#if !defined(MBEDTLS_ECP_NO_INTERNAL_RNG) + ecp_drbg_free( &drbg_ctx ); +#endif + return( ret ); } From 966cb796c4c7337777b93d16cea5dbbacfd44f69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 4 Jun 2020 10:20:12 +0200 Subject: [PATCH 03/14] Update documentation about optional f_rng parameter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/ecp.h | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index 691415e0a..5f02e86b7 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -518,10 +518,13 @@ int mbedtls_ecp_tls_write_group( const mbedtls_ecp_group *grp, size_t *olen, * operations for any valid m. It avoids any if-branch or * array index depending on the value of m. * - * \note If f_rng is not NULL, it is used to randomize intermediate - * results in order to prevent potential timing attacks - * targeting these results. It is recommended to always - * provide a non-NULL f_rng (the overhead is negligible). + * \note If \p f_rng is not NULL, it is used to randomize + * intermediate results to prevent potential timing attacks + * targeting these results. We recommend always providing + * a non-NULL \p f_rng. The overhead is negligible. + * Note: unless #MBEDTLS_ECP_NO_INTERNAL_RNG is defined, when + * \p f_rng is NULL, an internal RNG (seeded from the value + * of \p m) will be used instead. * * \param grp ECP group * \param R Destination point From 6d059bf0518b5424ad7d9c31b8fa1a574bc50b52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 4 Jun 2020 10:31:06 +0200 Subject: [PATCH 04/14] Add Security ChangeLog entry for lack of blinding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/ecp-internal-rng.txt | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/ChangeLog.d/ecp-internal-rng.txt b/ChangeLog.d/ecp-internal-rng.txt index bf11a7391..c0419acad 100644 --- a/ChangeLog.d/ecp-internal-rng.txt +++ b/ChangeLog.d/ecp-internal-rng.txt @@ -3,3 +3,13 @@ Changes `MBEDTLS_CTR_DRBG_C` or `MBEDTLS_HMAC_DRBG_C` for some side-channel coutermeasures. If side channels are not a concern, this dependency can be avoided by enabling the new option `MBEDTLS_ECP_NO_INTERNAL_RNG`. + +Security + * Fix side channel in mbedtls_ecp_check_pub_priv() and + mbedtls_pk_parse_key() / mbedtls_pk_parse_keyfile() (when loading a + private key that didn't include the uncompressed public key), as well as + mbedtls_ecp_mul() / mbedtls_ecp_mul_restartable() when called with a NULL + f_rng argument. An attacker with access to precise enough timing and + memory access information (typically an untrusted operating system + attacking a secure enclave) could fully recover the ECC private key. + Found and reported by Alejandro Cabrera Aldaya and Billy Brumley. From 22fe5236e9603b7ee74ba5316ee18175b4c9610a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 19 Jun 2020 10:36:16 +0200 Subject: [PATCH 05/14] Skip redundant checks for NULL f_rng MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unless MBEDTLS_ECP_NO_INTERNAL_RNG is defined, it's no longer possible for f_rng to be NULL at the places that randomize coordinates. Eliminate the NULL check in this case: - it makes it clearer to reviewers that randomization always happens (unless the user opted out at compile time) - a NULL check in a place where it's easy to prove the value is never NULL might upset or confuse static analyzers (including humans) - removing the check saves a bit of code size Signed-off-by: Manuel Pégourié-Gonnard --- library/ecp.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/library/ecp.c b/library/ecp.c index 20d7f77e7..812740926 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -1447,7 +1447,9 @@ static int ecp_mul_comb_core( const mbedtls_ecp_group *grp, mbedtls_ecp_point *R i = d; MBEDTLS_MPI_CHK( ecp_select_comb( grp, R, T, t_len, x[i] ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_lset( &R->Z, 1 ) ); +#if defined(MBEDTLS_ECP_NO_INTERNAL_RNG) if( f_rng != 0 ) +#endif MBEDTLS_MPI_CHK( ecp_randomize_jac( grp, R, f_rng, p_rng ) ); while( i-- != 0 ) @@ -1576,7 +1578,9 @@ static int ecp_mul_comb( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, * * Avoid the leak by randomizing coordinates before we normalize them. */ +#if defined(MBEDTLS_ECP_NO_INTERNAL_RNG) if( f_rng != 0 ) +#endif MBEDTLS_MPI_CHK( ecp_randomize_jac( grp, R, f_rng, p_rng ) ); MBEDTLS_MPI_CHK( ecp_normalize_jac( grp, R ) ); @@ -1779,7 +1783,9 @@ static int ecp_mul_mxz( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, MOD_ADD( RP.X ); /* Randomize coordinates of the starting point */ +#if defined(MBEDTLS_ECP_NO_INTERNAL_RNG) if( f_rng != NULL ) +#endif MBEDTLS_MPI_CHK( ecp_randomize_mxz( grp, &RP, f_rng, p_rng ) ); /* Loop invariant: R = result so far, RP = R + P */ @@ -1812,7 +1818,9 @@ static int ecp_mul_mxz( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, * * Avoid the leak by randomizing coordinates before we normalize them. */ +#if defined(MBEDTLS_ECP_NO_INTERNAL_RNG) if( f_rng != NULL ) +#endif MBEDTLS_MPI_CHK( ecp_randomize_mxz( grp, R, f_rng, p_rng ) ); MBEDTLS_MPI_CHK( ecp_normalize_mxz( grp, R ) ); From e2828c2d9493f9341b468b9a6a90e60133243e82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 4 Jun 2020 12:32:14 +0200 Subject: [PATCH 06/14] Use HMAC_DRBG by default for ECP internal DRBG MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It results in smaller code than using CTR_DRBG (64 bytes smaller on ARMv6-M with arm-none-eabi-gcc 7.3.1), so let's use this by default when both are available. Signed-off-by: Manuel Pégourié-Gonnard --- library/ecp.c | 84 ++++++++++++++++++++++++++------------------------- 1 file changed, 43 insertions(+), 41 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 812740926..36bd165d6 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -68,10 +68,10 @@ #include "mbedtls/ecp_internal.h" #if !defined(MBEDTLS_ECP_NO_INTERNAL_RNG) -#if defined(MBEDTLS_CTR_DRBG_C) -#include "mbedtls/ctr_drbg.h" -#elif defined(MBEDTLS_HMAC_DRBG_C) +#if defined(MBEDTLS_HMAC_DRBG_C) #include "mbedtls/hmac_drbg.h" +#elif defined(MBEDTLS_CTR_DRBG_C) +#include "mbedtls/ctr_drbg.h" #else #error "Invalid configuration detected. Include check_config.h to ensure that the configuration is valid." #endif @@ -111,10 +111,48 @@ static unsigned long add_count, dbl_count, mul_count; * have our own internal DRBG instance, seeded from the secret scalar. * * The following is a light-weight abstraction layer for doing that with - * CTR_DRBG or HMAC_DRBG. + * HMAC_DRBG (first choice) or CTR_DRBG. */ -#if defined(MBEDTLS_CTR_DRBG_C) +#if defined(MBEDTLS_HMAC_DRBG_C) + +/* DRBG context type */ +typedef mbedtls_hmac_drbg_context ecp_drbg_context; + +/* DRBG context init */ +static inline void ecp_drbg_init( ecp_drbg_context *ctx ) +{ + mbedtls_hmac_drbg_init( ctx ); +} + +/* DRBG context free */ +static inline void ecp_drbg_free( ecp_drbg_context *ctx ) +{ + mbedtls_hmac_drbg_free( ctx ); +} + +/* DRBG function */ +static inline int ecp_drbg_random( void *p_rng, + unsigned char *output, size_t output_len ) +{ + return( mbedtls_hmac_drbg_random( p_rng, output, output_len ) ); +} + +/* DRBG context seeding */ +static int ecp_drbg_seed( ecp_drbg_context *ctx, const mbedtls_mpi *secret ) +{ + const unsigned char *secret_p = (const unsigned char *) secret->p; + const size_t secret_size = secret->n * sizeof( mbedtls_mpi_uint ); + + /* The list starts with strong hashes */ + const mbedtls_md_type_t md_type = mbedtls_md_list()[0]; + const mbedtls_md_info_t *md_info = mbedtls_md_info_from_type( md_type ); + + return( mbedtls_hmac_drbg_seed_buf( ctx, md_info, secret_p, secret_size ) ); +} + +#elif defined(MBEDTLS_CTR_DRBG_C) + /* DRBG context type */ typedef mbedtls_ctr_drbg_context ecp_drbg_context; @@ -161,42 +199,6 @@ static int ecp_drbg_seed( ecp_drbg_context *ctx, const mbedtls_mpi *secret ) secret_p, secret_size ) ); } -#elif defined(MBEDTLS_HMAC_DRBG_C) -/* DRBG context type */ -typedef mbedtls_hmac_drbg_context ecp_drbg_context; - -/* DRBG context init */ -static inline void ecp_drbg_init( ecp_drbg_context *ctx ) -{ - mbedtls_hmac_drbg_init( ctx ); -} - -/* DRBG context free */ -static inline void ecp_drbg_free( ecp_drbg_context *ctx ) -{ - mbedtls_hmac_drbg_free( ctx ); -} - -/* DRBG function */ -static inline int ecp_drbg_random( void *p_rng, - unsigned char *output, size_t output_len ) -{ - return( mbedtls_hmac_drbg_random( p_rng, output, output_len ) ); -} - -/* DRBG context seeding */ -static int ecp_drbg_seed( ecp_drbg_context *ctx, const mbedtls_mpi *secret ) -{ - const unsigned char *secret_p = (const unsigned char *) secret->p; - const size_t secret_size = secret->n * sizeof( mbedtls_mpi_uint ); - - /* The list starts with strong hashes */ - const mbedtls_md_type_t md_type = mbedtls_md_list()[0]; - const mbedtls_md_info_t *md_info = mbedtls_md_info_from_type( md_type ); - - return( mbedtls_hmac_drbg_seed_buf( ctx, md_info, secret_p, secret_size ) ); -} - #else #error "Invalid configuration detected. Include check_config.h to ensure that the configuration is valid." #endif /* DRBG modules */ From 99bf33fa815ec8ce7ab83c0bf2f4f9ee547a9e76 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 10 Jun 2020 09:18:25 +0200 Subject: [PATCH 07/14] Fix typo in a comment Co-authored-by: Janos Follath --- library/ecp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ecp.c b/library/ecp.c index 36bd165d6..8c3379c45 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -98,7 +98,7 @@ static unsigned long add_count, dbl_count, mul_count; #if !defined(MBEDTLS_ECP_NO_INTERNAL_RNG) /* * Currently ecp_mul() takes a RNG function as an argument, used for - * side-channel protection, but it can be NULL. The initial reasonning was + * side-channel protection, but it can be NULL. The initial reasoning was * that people will pass non-NULL RNG when they care about side-channels, but * unfortunately we have some APIs that call ecp_mul() with a NULL RNG, with * no opportunity for the user to do anything about it. From 6d61498e05a8cea68b06f3cd95847539534d44f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 16 Jun 2020 12:51:42 +0200 Subject: [PATCH 08/14] Add fall-back to hash-based KDF for internal ECP DRBG MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dependency on a DRBG module was perhaps a bit strict for LTS branches, so let's have an option that works with no DRBG when at least one SHA module is present. This changes the internal API of ecp_drbg_seed() by adding the size of the MPI as a parameter. Re-computing the size from the number of limbs doesn't work too well here as we're writing out to a fixed-size buffer and for some curves (P-521) that would round up too much. Using mbedtls_mpi_get_len() is not entirely satisfactory either as it would mean using a variable-length encoding, with could open side channels. Signed-off-by: Manuel Pégourié-Gonnard --- include/mbedtls/check_config.h | 5 +- library/ecp.c | 221 +++++++++++++++++++++++++++++++-- tests/scripts/all.sh | 63 ++++++++++ 3 files changed, 279 insertions(+), 10 deletions(-) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 935248f4c..473488cfc 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -127,8 +127,11 @@ defined(MBEDTLS_ECP_ALT) || \ defined(MBEDTLS_CTR_DRBG_C) || \ defined(MBEDTLS_HMAC_DRBG_C) || \ + defined(MBEDTLS_SHA512_C) || \ + defined(MBEDTLS_SHA256_C) || \ + defined(MBEDTLS_SHA1_C) || \ defined(MBEDTLS_ECP_NO_INTERNAL_RNG)) -#error "MBEDTLS_ECP_C requires a DRBG module unless MBEDTLS_ECP_NO_INTERNAL_RNG is defined or an alternative implementation is used" +#error "MBEDTLS_ECP_C requires a DRBG or SHA module unless MBEDTLS_ECP_NO_INTERNAL_RNG is defined or an alternative implementation is used" #endif #if defined(MBEDTLS_PK_PARSE_C) && !defined(MBEDTLS_ASN1_PARSE_C) diff --git a/library/ecp.c b/library/ecp.c index 8c3379c45..88786b399 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -72,6 +72,12 @@ #include "mbedtls/hmac_drbg.h" #elif defined(MBEDTLS_CTR_DRBG_C) #include "mbedtls/ctr_drbg.h" +#elif defined(MBEDTLS_SHA512_C) +#include "mbedtls/sha512.h" +#elif defined(MBEDTLS_SHA256_C) +#include "mbedtls/sha256.h" +#elif defined(MBEDTLS_SHA1_C) +#include "mbedtls/sha1.h" #else #error "Invalid configuration detected. Include check_config.h to ensure that the configuration is valid." #endif @@ -139,16 +145,16 @@ static inline int ecp_drbg_random( void *p_rng, } /* DRBG context seeding */ -static int ecp_drbg_seed( ecp_drbg_context *ctx, const mbedtls_mpi *secret ) +static int ecp_drbg_seed( ecp_drbg_context *ctx, + const mbedtls_mpi *secret, size_t secret_len ) { const unsigned char *secret_p = (const unsigned char *) secret->p; - const size_t secret_size = secret->n * sizeof( mbedtls_mpi_uint ); /* The list starts with strong hashes */ const mbedtls_md_type_t md_type = mbedtls_md_list()[0]; const mbedtls_md_info_t *md_info = mbedtls_md_info_from_type( md_type ); - return( mbedtls_hmac_drbg_seed_buf( ctx, md_info, secret_p, secret_size ) ); + return( mbedtls_hmac_drbg_seed_buf( ctx, md_info, secret_p, secret_len ) ); } #elif defined(MBEDTLS_CTR_DRBG_C) @@ -190,18 +196,113 @@ static int ecp_ctr_drbg_null_entropy(void *ctx, unsigned char *out, size_t len) } /* DRBG context seeding */ -static int ecp_drbg_seed( ecp_drbg_context *ctx, const mbedtls_mpi *secret ) +static int ecp_drbg_seed( ecp_drbg_context *ctx, + const mbedtls_mpi *secret, size_t secret_len ) { const unsigned char *secret_p = (const unsigned char *) secret->p; - const size_t secret_size = secret->n * sizeof( mbedtls_mpi_uint ); return( mbedtls_ctr_drbg_seed( ctx, ecp_ctr_drbg_null_entropy, NULL, - secret_p, secret_size ) ); + secret_p, secret_len ) ); } -#else +#elif defined(MBEDTLS_SHA512_C) || \ + defined(MBEDTLS_SHA256_C) || \ + defined(MBEDTLS_SHA1_C) + +/* This will be used in the self-test function */ +#define ECP_ONE_STEP_KDF + +/* + * We need to expand secret data (the scalar) into a longer stream of bytes. + * + * We'll use the One-Step KDF from NIST SP 800-56C, with option 1 (H is a hash + * function) and empty FixedInfo. (Though we'll make it fit the DRBG API for + * convenience, this is not a full-fledged DRBG, but we don't need one here.) + * + * We need a basic hash abstraction layer to use whatever SHA is available. + */ +#if defined(MBEDTLS_SHA512_C) + +#define HASH_FUNC( in, ilen, out ) mbedtls_sha512_ret( in, ilen, out, 0 ); +#define HASH_BLOCK_BYTES ( 512 / 8 ) + +#elif defined(MBEDTLS_SHA256_C) + +#define HASH_FUNC( in, ilen, out ) mbedtls_sha256_ret( in, ilen, out, 0 ); +#define HASH_BLOCK_BYTES ( 256 / 8 ) + +#else // from a previous #if we know that SHA-1 is available if SHA-2 isn't + +#define HASH_FUNC mbedtls_sha1_ret +#define HASH_BLOCK_BYTES ( 160 / 8 ) + +#endif /* SHA512/SHA256/SHA1 abstraction */ + +/* + * State consists of a 32-bit counter plus the secret value. + * + * We stored them concatenated in a single buffer as that's what will get + * passed to the hash function. + */ +typedef struct { + size_t total_len; + uint8_t buf[4 + MBEDTLS_ECP_MAX_BYTES]; +} ecp_drbg_context; + +static void ecp_drbg_init( ecp_drbg_context *ctx ) +{ + memset( ctx, 0, sizeof( ecp_drbg_context ) ); +} + +static void ecp_drbg_free( ecp_drbg_context *ctx ) +{ + mbedtls_zeroize( ctx, sizeof( ecp_drbg_context ) ); +} + +static int ecp_drbg_seed( ecp_drbg_context *ctx, + const mbedtls_mpi *secret, size_t secret_len ) +{ + ctx->total_len = 4 + secret_len; + memset( ctx->buf, 0, 4); + return( mbedtls_mpi_write_binary( secret, ctx->buf + 4, secret_len ) ); +} + +static int ecp_drbg_random( void *p_rng, unsigned char *output, size_t output_len ) +{ + ecp_drbg_context *ctx = p_rng; + int ret; + size_t len_done = 0; + + while( len_done < output_len ) + { + uint8_t tmp[HASH_BLOCK_BYTES]; + uint8_t use_len; + + /* We don't need to draw more that 255 blocks, so don't bother with + * carry propagation and just return an error instead. */ + ctx->buf[3] += 1; + if( ctx->buf[3] == 0 ) + return( MBEDTLS_ERR_ECP_RANDOM_FAILED ); + + ret = HASH_FUNC( ctx->buf, ctx->total_len, tmp ); + if( ret != 0 ) + return( ret ); + + if( output_len - len_done > HASH_BLOCK_BYTES ) + use_len = HASH_BLOCK_BYTES; + else + use_len = output_len - len_done; + + memcpy( output + len_done, tmp, use_len ); + len_done += use_len; + } + + return( 0 ); +} + +#else /* DRBG/SHA modules */ #error "Invalid configuration detected. Include check_config.h to ensure that the configuration is valid." -#endif /* DRBG modules */ +#endif /* DRBG/SHA modules */ #endif /* MBEDTLS_ECP_NO_INTERNAL_RNG */ #if defined(MBEDTLS_ECP_DP_SECP192R1_ENABLED) || \ @@ -1863,7 +1964,8 @@ int mbedtls_ecp_mul( mbedtls_ecp_group *grp, mbedtls_ecp_point *R, #if !defined(MBEDTLS_ECP_NO_INTERNAL_RNG) if( f_rng == NULL ) { - MBEDTLS_MPI_CHK( ecp_drbg_seed( &drbg_ctx, m ) ); + const size_t m_len = ( grp->nbits + 7 ) / 8; + MBEDTLS_MPI_CHK( ecp_drbg_seed( &drbg_ctx, m, m_len ) ); f_rng = &ecp_drbg_random; p_rng = &drbg_ctx; } @@ -2263,6 +2365,89 @@ cleanup: #if defined(MBEDTLS_SELF_TEST) +#if defined(ECP_ONE_STEP_KDF) +/* + * There are no test vectors from NIST for the One-Step KDF in SP 800-56C, + * but unofficial ones can be found at: + * https://github.com/patrickfav/singlestep-kdf/wiki/NIST-SP-800-56C-Rev1:-Non-Official-Test-Vectors + * + * We only use the ones with empty fixedInfo, and for brevity's sake, only + * 32-bytes output (with SHA-1 that's more than one block, with SHA-256 + * exactly one block, and with SHA-512 less than one block). + */ +#if defined(MBEDTLS_SHA512_C) + +static const uint8_t test_kdf_z[16] = { + 0xeb, 0xf3, 0x19, 0x67, 0x1e, 0xac, 0xcc, 0x6f, + 0xc5, 0xc0, 0x5d, 0x95, 0x8d, 0x17, 0x15, 0x94, +}; +static const uint8_t test_kdf_out[32] = { + 0xa9, 0x48, 0x85, 0x67, 0x54, 0x7c, 0x2a, 0x8e, + 0x9e, 0xd1, 0x67, 0x76, 0xe3, 0x1c, 0x03, 0x92, + 0x41, 0x77, 0x2a, 0x9e, 0xc7, 0xcc, 0xd7, 0x1f, + 0xda, 0x12, 0xe9, 0xba, 0xc9, 0xb2, 0x17, 0x24, +}; + +#elif defined(MBEDTLS_SHA256_C) + +static const uint8_t test_kdf_z[16] = { + 0x0d, 0x5e, 0xc8, 0x9a, 0x68, 0xb1, 0xa7, 0xa0, + 0xdf, 0x95, 0x24, 0x54, 0x3f, 0x4d, 0x70, 0xef, +}; +static const uint8_t test_kdf_out[32] = { + 0x77, 0xbc, 0x94, 0x9e, 0xa0, 0xd3, 0xdd, 0x5c, + 0x8e, 0xb7, 0xeb, 0x84, 0x05, 0x40, 0x60, 0xfa, + 0x96, 0x6e, 0x7e, 0xcd, 0x73, 0x9f, 0xa1, 0xe6, + 0x34, 0x3f, 0x6d, 0x82, 0x16, 0x22, 0xb4, 0x45, +}; + +#elif defined(MBEDTLS_SHA1_C) + +static const uint8_t test_kdf_z[16] = { + 0x4e, 0x1e, 0x70, 0xc9, 0x88, 0x68, 0x19, 0xa3, + 0x1b, 0xc2, 0x9a, 0x53, 0x79, 0x11, 0xad, 0xd9, +}; +static const uint8_t test_kdf_out[32] = { + 0xdd, 0xbf, 0xc4, 0x40, 0x44, 0x9a, 0xab, 0x41, + 0x31, 0xc6, 0xd8, 0xae, 0xc0, 0x8c, 0xe1, 0x49, + 0x6f, 0x27, 0x02, 0x24, 0x1d, 0x0e, 0x27, 0xcc, + 0x15, 0x5c, 0x5c, 0x7c, 0x3c, 0xda, 0x75, 0xb5, +}; + +#else +#error "Need at least one of SHA-512, SHA-256 or SHA-1" +#endif + +static int ecp_kdf_self_test( void ) +{ + int ret; + ecp_drbg_context kdf_ctx; + mbedtls_mpi scalar; + uint8_t out[sizeof( test_kdf_out )]; + + ecp_drbg_init( &kdf_ctx ); + mbedtls_mpi_init( &scalar ); + memset( out, 0, sizeof( out ) ); + + MBEDTLS_MPI_CHK( mbedtls_mpi_read_binary( &scalar, + test_kdf_z, sizeof( test_kdf_z ) ) ); + + MBEDTLS_MPI_CHK( ecp_drbg_seed( &kdf_ctx, + &scalar, sizeof( test_kdf_z ) ) ); + + MBEDTLS_MPI_CHK( ecp_drbg_random( &kdf_ctx, out, sizeof( out ) ) ); + + if( memcmp( out, test_kdf_out, sizeof( out ) ) != 0 ) + ret = -1; + +cleanup: + ecp_drbg_free( &kdf_ctx ); + mbedtls_mpi_free( &scalar ); + + return( ret ); +} +#endif /* ECP_ONE_STEP_KDF */ + /* * Checkup routine */ @@ -2374,6 +2559,24 @@ int mbedtls_ecp_self_test( int verbose ) if( verbose != 0 ) mbedtls_printf( "passed\n" ); +#if defined(ECP_ONE_STEP_KDF) + if( verbose != 0 ) + mbedtls_printf( " ECP test #3 (internal KDF): " ); + + ret = ecp_kdf_self_test(); + if( ret != 0 ) + { + if( verbose != 0 ) + mbedtls_printf( "failed\n" ); + + ret = 1; + goto cleanup; + } + + if( verbose != 0 ) + mbedtls_printf( "passed\n" ); +#endif /* ECP_ONE_STEP_KDF */ + cleanup: if( ret < 0 && verbose != 0 ) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 4103ace65..e7b84f08f 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -814,6 +814,69 @@ component_test_no_hmac_drbg () { # so there's little value in running those lengthy tests here. } +component_test_no_drbg_all_hashes () { + # this tests the internal ECP DRBG using a KDF based on SHA-512 + msg "build: Default minus DRBGs" + scripts/config.pl unset MBEDTLS_CTR_DRBG_C + scripts/config.pl unset MBEDTLS_HMAC_DRBG_C + scripts/config.pl unset MBEDTLS_ECDSA_DETERMINISTIC # requires HMAC_DRBG + scripts/config.pl unset MBEDTLS_PSA_CRYPTO_C # requires a DRBG + scripts/config.pl unset MBEDTLS_PSA_CRYPTO_STORAGE_C # requires PSA Crypto + + CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . + make + + msg "test: Default minus DRBGs" + make test + + # no SSL tests as they all depend on having a DRBG +} + +component_test_no_drbg_no_sha512 () { + # this tests the internal ECP DRBG using a KDF based on SHA-256 + msg "build: Default minus DRBGs minus SHA-512" + scripts/config.pl unset MBEDTLS_CTR_DRBG_C + scripts/config.pl unset MBEDTLS_HMAC_DRBG_C + scripts/config.pl unset MBEDTLS_ECDSA_DETERMINISTIC # requires HMAC_DRBG + scripts/config.pl unset MBEDTLS_PSA_CRYPTO_C # requires a DRBG + scripts/config.pl unset MBEDTLS_PSA_CRYPTO_STORAGE_C # requires PSA Crypto + scripts/config.pl unset MBEDTLS_SHA512_C + + CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . + make + + msg "test: Default minus DRBGs minus SHA-512" + make test + + # no SSL tests as they all depend on having a DRBG +} + +component_test_no_drbg_no_sha2 () { + # this tests the internal ECP DRBG using a KDF based on SHA-1 + msg "build: Default minus DRBGs minus SHA-2" + scripts/config.pl unset MBEDTLS_CTR_DRBG_C + scripts/config.pl unset MBEDTLS_HMAC_DRBG_C + scripts/config.pl unset MBEDTLS_ECDSA_DETERMINISTIC # requires HMAC_DRBG + scripts/config.pl unset MBEDTLS_PSA_CRYPTO_C # requires a DRBG + scripts/config.pl unset MBEDTLS_PSA_CRYPTO_STORAGE_C # requires PSA Crypto + scripts/config.pl unset MBEDTLS_SHA512_C + scripts/config.pl unset MBEDTLS_SHA256_C + scripts/config.pl unset MBEDTLS_ENTROPY_C # requires SHA-2 + scripts/config.pl unset MBEDTLS_PSA_CRYPTO_C # requires Entropy + scripts/config.pl unset MBEDTLS_PSA_CRYPTO_STORAGE_C # requires PSA Crypto + scripts/config.pl unset MBEDTLS_PSA_CRYPTO_SE_C # requires PSA Crypto + scripts/config.pl unset MBEDTLS_USE_PSA_CRYPTO # requires PSA Crypto + scripts/config.pl unset MBEDTLS_SSL_PROTO_TLS1_2 # requires SHA-2 + + CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . + make + + msg "test: Default minus DRBGs minus SHA-2" + make test + + # no SSL tests as they all depend on having a DRBG +} + component_test_ecp_no_internal_rng () { msg "build: Default plus ECP_NO_INTERNAL_RNG minus DRBG modules" scripts/config.pl set MBEDTLS_ECP_NO_INTERNAL_RNG From 601128eb584a92cdc35df79639f862fd5bfccc90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 17 Jun 2020 10:12:43 +0200 Subject: [PATCH 09/14] Fix potential memory overread in seed functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit introduced a potential memory overread by reading secret_len bytes from secret->p, while the is no guarantee that secret has enough limbs for that. Fix that by using an intermediate buffer and mpi_write_binary(). Signed-off-by: Manuel Pégourié-Gonnard --- library/ecp.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 88786b399..8da65ad1d 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -148,13 +148,21 @@ static inline int ecp_drbg_random( void *p_rng, static int ecp_drbg_seed( ecp_drbg_context *ctx, const mbedtls_mpi *secret, size_t secret_len ) { - const unsigned char *secret_p = (const unsigned char *) secret->p; - + int ret; + unsigned char secret_bytes[MBEDTLS_ECP_MAX_BYTES]; /* The list starts with strong hashes */ const mbedtls_md_type_t md_type = mbedtls_md_list()[0]; const mbedtls_md_info_t *md_info = mbedtls_md_info_from_type( md_type ); - return( mbedtls_hmac_drbg_seed_buf( ctx, md_info, secret_p, secret_len ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_write_binary( secret, + secret_bytes, secret_len ) ); + + ret = mbedtls_hmac_drbg_seed_buf( ctx, md_info, secret_bytes, secret_len ); + +cleanup: + mbedtls_zeroize( secret_bytes, secret_len ); + + return( ret ); } #elif defined(MBEDTLS_CTR_DRBG_C) @@ -199,10 +207,19 @@ static int ecp_ctr_drbg_null_entropy(void *ctx, unsigned char *out, size_t len) static int ecp_drbg_seed( ecp_drbg_context *ctx, const mbedtls_mpi *secret, size_t secret_len ) { - const unsigned char *secret_p = (const unsigned char *) secret->p; + int ret; + unsigned char secret_bytes[MBEDTLS_ECP_MAX_BYTES]; - return( mbedtls_ctr_drbg_seed( ctx, ecp_ctr_drbg_null_entropy, NULL, - secret_p, secret_len ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_write_binary( secret, + secret_bytes, secret_len ) ); + + ret = mbedtls_ctr_drbg_seed( ctx, ecp_ctr_drbg_null_entropy, NULL, + secret_bytes, secret_len ); + +cleanup: + mbedtls_zeroize( secret_bytes, secret_len ); + + return( ret ); } #elif defined(MBEDTLS_SHA512_C) || \ From f1aca9fdba2df0aaf2964b88140eb4ecc437dcd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 17 Jun 2020 12:26:54 +0200 Subject: [PATCH 10/14] Update dependencies documentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/ecp-internal-rng.txt | 3 ++- include/mbedtls/config.h | 11 ++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/ChangeLog.d/ecp-internal-rng.txt b/ChangeLog.d/ecp-internal-rng.txt index c0419acad..f6b3c0f7f 100644 --- a/ChangeLog.d/ecp-internal-rng.txt +++ b/ChangeLog.d/ecp-internal-rng.txt @@ -1,6 +1,7 @@ Changes * The ECP module, enabled by `MBEDTLS_ECP_C`, now depends on - `MBEDTLS_CTR_DRBG_C` or `MBEDTLS_HMAC_DRBG_C` for some side-channel + `MBEDTLS_CTR_DRBG_C`, `MBEDTLS_HMAC_DRBG_C`, `MBEDTLS_SHA512_C`, + `MBEDTLS_SHA256_C` or `MBEDTLS_SHA1_C` for some side-channel coutermeasures. If side channels are not a concern, this dependency can be avoided by enabling the new option `MBEDTLS_ECP_NO_INTERNAL_RNG`. diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 03033619d..15df53707 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -626,11 +626,12 @@ * against some side-channel attacks. * * This protection introduces a dependency of the ECP module on one of the - * DRBG modules. For very constrained implementations that don't require this - * protection (for example, because you're only doing signature verification, - * so not manipulating any secret, or because local/physical side-channel - * attacks are outside your threat model), it might be desirable to get rid of - * that dependency. + * DRBG or SHA modules (HMAC-DRBG, CTR-DRBG, SHA-512, SHA-256 or SHA-1). + * For very constrained applications that don't require this protection + * (for example, because you're only doing signature verification, so not + * manipulating any secret, or because local/physical side-channel attacks are + * outside your threat model), it might be desirable to get rid of that + * dependency. * * \warning Enabling this option makes some uses of ECP vulnerable to some * side-channel attacks. Only enable it if you know that's not a problem for From 874598669971cf6f0ee6157a487d906af63e01a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 17 Jun 2020 12:40:57 +0200 Subject: [PATCH 11/14] Zeroize temporary stack buffer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/ecp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/ecp.c b/library/ecp.c index 8da65ad1d..6fdadf22e 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -289,10 +289,10 @@ static int ecp_drbg_random( void *p_rng, unsigned char *output, size_t output_le ecp_drbg_context *ctx = p_rng; int ret; size_t len_done = 0; + uint8_t tmp[HASH_BLOCK_BYTES]; while( len_done < output_len ) { - uint8_t tmp[HASH_BLOCK_BYTES]; uint8_t use_len; /* We don't need to draw more that 255 blocks, so don't bother with @@ -314,6 +314,8 @@ static int ecp_drbg_random( void *p_rng, unsigned char *output, size_t output_le len_done += use_len; } + mbedtls_zeroize( tmp, sizeof( tmp ) ); + return( 0 ); } From 979728838375f7d750c50c60f1672f41b32c008e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 17 Jun 2020 12:57:33 +0200 Subject: [PATCH 12/14] Improve comment justifying a hard-coded limitation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/ecp.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 6fdadf22e..e537dbb49 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -295,8 +295,19 @@ static int ecp_drbg_random( void *p_rng, unsigned char *output, size_t output_le { uint8_t use_len; - /* We don't need to draw more that 255 blocks, so don't bother with - * carry propagation and just return an error instead. */ + /* This function is only called for coordinate randomisation, which + * happens only twice in a scalar multiplication. Each time needs a + * random value in the range [2, p-1], and gets it by drawing len(p) + * bytes from this function, and retrying up to 10 times if unlucky. + * + * So for the largest curve, each scalar multiplication draws at most + * 2 * 66 bytes. The minimum block size is 20 bytes (with SHA-1), so + * that means at most 66 blocks. + * + * Since we don't need to draw more that 255 blocks, don't bother + * with carry propagation and just return an error instead. We can + * change that it we even need to draw more blinding values. + */ ctx->buf[3] += 1; if( ctx->buf[3] == 0 ) return( MBEDTLS_ERR_ECP_RANDOM_FAILED ); From 138109133d2f9121b199dc7777130c17a135ad45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 18 Jun 2020 12:14:34 +0200 Subject: [PATCH 13/14] Remove SHA-1 as a fallback option MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - it's 2020, there shouldn't be too many systems out there where SHA-1 is the only available hash option, so its usefulness is limited - OTOH testing configurations without SHA-2 reveal bugs that are not easy to fix in a fully compatible way So overall, the benefit/cost ratio is not good enough to justify keeping SHA-1 as a fallback option here. Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/ecp-internal-rng.txt | 8 +++---- include/mbedtls/check_config.h | 3 +-- include/mbedtls/config.h | 11 +++++---- library/ecp.c | 38 ++++++-------------------------- tests/scripts/all.sh | 26 ---------------------- 5 files changed, 17 insertions(+), 69 deletions(-) diff --git a/ChangeLog.d/ecp-internal-rng.txt b/ChangeLog.d/ecp-internal-rng.txt index f6b3c0f7f..8b5c5147e 100644 --- a/ChangeLog.d/ecp-internal-rng.txt +++ b/ChangeLog.d/ecp-internal-rng.txt @@ -1,9 +1,9 @@ Changes * The ECP module, enabled by `MBEDTLS_ECP_C`, now depends on - `MBEDTLS_CTR_DRBG_C`, `MBEDTLS_HMAC_DRBG_C`, `MBEDTLS_SHA512_C`, - `MBEDTLS_SHA256_C` or `MBEDTLS_SHA1_C` for some side-channel - coutermeasures. If side channels are not a concern, this dependency can - be avoided by enabling the new option `MBEDTLS_ECP_NO_INTERNAL_RNG`. + `MBEDTLS_CTR_DRBG_C`, `MBEDTLS_HMAC_DRBG_C`, `MBEDTLS_SHA512_C` or + `MBEDTLS_SHA256_C` for some side-channel coutermeasures. If side channels + are not a concern, this dependency can be avoided by enabling the new + option `MBEDTLS_ECP_NO_INTERNAL_RNG`. Security * Fix side channel in mbedtls_ecp_check_pub_priv() and diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 473488cfc..255cef327 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -129,9 +129,8 @@ defined(MBEDTLS_HMAC_DRBG_C) || \ defined(MBEDTLS_SHA512_C) || \ defined(MBEDTLS_SHA256_C) || \ - defined(MBEDTLS_SHA1_C) || \ defined(MBEDTLS_ECP_NO_INTERNAL_RNG)) -#error "MBEDTLS_ECP_C requires a DRBG or SHA module unless MBEDTLS_ECP_NO_INTERNAL_RNG is defined or an alternative implementation is used" +#error "MBEDTLS_ECP_C requires a DRBG or SHA-2 module unless MBEDTLS_ECP_NO_INTERNAL_RNG is defined or an alternative implementation is used" #endif #if defined(MBEDTLS_PK_PARSE_C) && !defined(MBEDTLS_ASN1_PARSE_C) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 15df53707..d8644f0c1 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -626,12 +626,11 @@ * against some side-channel attacks. * * This protection introduces a dependency of the ECP module on one of the - * DRBG or SHA modules (HMAC-DRBG, CTR-DRBG, SHA-512, SHA-256 or SHA-1). - * For very constrained applications that don't require this protection - * (for example, because you're only doing signature verification, so not - * manipulating any secret, or because local/physical side-channel attacks are - * outside your threat model), it might be desirable to get rid of that - * dependency. + * DRBG or SHA modules (HMAC-DRBG, CTR-DRBG, SHA-512 or SHA-256.) For very + * constrained applications that don't require this protection (for example, + * because you're only doing signature verification, so not manipulating any + * secret, or because local/physical side-channel attacks are outside your + * threat model), it might be desirable to get rid of that dependency. * * \warning Enabling this option makes some uses of ECP vulnerable to some * side-channel attacks. Only enable it if you know that's not a problem for diff --git a/library/ecp.c b/library/ecp.c index e537dbb49..c13689ded 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -76,8 +76,6 @@ #include "mbedtls/sha512.h" #elif defined(MBEDTLS_SHA256_C) #include "mbedtls/sha256.h" -#elif defined(MBEDTLS_SHA1_C) -#include "mbedtls/sha1.h" #else #error "Invalid configuration detected. Include check_config.h to ensure that the configuration is valid." #endif @@ -222,9 +220,7 @@ cleanup: return( ret ); } -#elif defined(MBEDTLS_SHA512_C) || \ - defined(MBEDTLS_SHA256_C) || \ - defined(MBEDTLS_SHA1_C) +#elif defined(MBEDTLS_SHA512_C) || defined(MBEDTLS_SHA256_C) /* This will be used in the self-test function */ #define ECP_ONE_STEP_KDF @@ -236,7 +232,7 @@ cleanup: * function) and empty FixedInfo. (Though we'll make it fit the DRBG API for * convenience, this is not a full-fledged DRBG, but we don't need one here.) * - * We need a basic hash abstraction layer to use whatever SHA is available. + * We need a basic hash abstraction layer to use whatever SHA-2 is available. */ #if defined(MBEDTLS_SHA512_C) @@ -248,12 +244,7 @@ cleanup: #define HASH_FUNC( in, ilen, out ) mbedtls_sha256_ret( in, ilen, out, 0 ); #define HASH_BLOCK_BYTES ( 256 / 8 ) -#else // from a previous #if we know that SHA-1 is available if SHA-2 isn't - -#define HASH_FUNC mbedtls_sha1_ret -#define HASH_BLOCK_BYTES ( 160 / 8 ) - -#endif /* SHA512/SHA256/SHA1 abstraction */ +#endif /* SHA512/SHA256 abstraction */ /* * State consists of a 32-bit counter plus the secret value. @@ -301,8 +292,8 @@ static int ecp_drbg_random( void *p_rng, unsigned char *output, size_t output_le * bytes from this function, and retrying up to 10 times if unlucky. * * So for the largest curve, each scalar multiplication draws at most - * 2 * 66 bytes. The minimum block size is 20 bytes (with SHA-1), so - * that means at most 66 blocks. + * 20 * 66 bytes. The minimum block size is 32 (SHA-256), so with + * rounding that means a most 20 * 3 blocks. * * Since we don't need to draw more that 255 blocks, don't bother * with carry propagation and just return an error instead. We can @@ -2402,8 +2393,8 @@ cleanup: * https://github.com/patrickfav/singlestep-kdf/wiki/NIST-SP-800-56C-Rev1:-Non-Official-Test-Vectors * * We only use the ones with empty fixedInfo, and for brevity's sake, only - * 32-bytes output (with SHA-1 that's more than one block, with SHA-256 - * exactly one block, and with SHA-512 less than one block). + * 32-bytes output (with SHA-256 that's exactly one block, and with SHA-512 + * less than one block). */ #if defined(MBEDTLS_SHA512_C) @@ -2431,21 +2422,6 @@ static const uint8_t test_kdf_out[32] = { 0x34, 0x3f, 0x6d, 0x82, 0x16, 0x22, 0xb4, 0x45, }; -#elif defined(MBEDTLS_SHA1_C) - -static const uint8_t test_kdf_z[16] = { - 0x4e, 0x1e, 0x70, 0xc9, 0x88, 0x68, 0x19, 0xa3, - 0x1b, 0xc2, 0x9a, 0x53, 0x79, 0x11, 0xad, 0xd9, -}; -static const uint8_t test_kdf_out[32] = { - 0xdd, 0xbf, 0xc4, 0x40, 0x44, 0x9a, 0xab, 0x41, - 0x31, 0xc6, 0xd8, 0xae, 0xc0, 0x8c, 0xe1, 0x49, - 0x6f, 0x27, 0x02, 0x24, 0x1d, 0x0e, 0x27, 0xcc, - 0x15, 0x5c, 0x5c, 0x7c, 0x3c, 0xda, 0x75, 0xb5, -}; - -#else -#error "Need at least one of SHA-512, SHA-256 or SHA-1" #endif static int ecp_kdf_self_test( void ) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index e7b84f08f..c36322c98 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -851,32 +851,6 @@ component_test_no_drbg_no_sha512 () { # no SSL tests as they all depend on having a DRBG } -component_test_no_drbg_no_sha2 () { - # this tests the internal ECP DRBG using a KDF based on SHA-1 - msg "build: Default minus DRBGs minus SHA-2" - scripts/config.pl unset MBEDTLS_CTR_DRBG_C - scripts/config.pl unset MBEDTLS_HMAC_DRBG_C - scripts/config.pl unset MBEDTLS_ECDSA_DETERMINISTIC # requires HMAC_DRBG - scripts/config.pl unset MBEDTLS_PSA_CRYPTO_C # requires a DRBG - scripts/config.pl unset MBEDTLS_PSA_CRYPTO_STORAGE_C # requires PSA Crypto - scripts/config.pl unset MBEDTLS_SHA512_C - scripts/config.pl unset MBEDTLS_SHA256_C - scripts/config.pl unset MBEDTLS_ENTROPY_C # requires SHA-2 - scripts/config.pl unset MBEDTLS_PSA_CRYPTO_C # requires Entropy - scripts/config.pl unset MBEDTLS_PSA_CRYPTO_STORAGE_C # requires PSA Crypto - scripts/config.pl unset MBEDTLS_PSA_CRYPTO_SE_C # requires PSA Crypto - scripts/config.pl unset MBEDTLS_USE_PSA_CRYPTO # requires PSA Crypto - scripts/config.pl unset MBEDTLS_SSL_PROTO_TLS1_2 # requires SHA-2 - - CC=gcc cmake -D CMAKE_BUILD_TYPE:String=Asan . - make - - msg "test: Default minus DRBGs minus SHA-2" - make test - - # no SSL tests as they all depend on having a DRBG -} - component_test_ecp_no_internal_rng () { msg "build: Default plus ECP_NO_INTERNAL_RNG minus DRBG modules" scripts/config.pl set MBEDTLS_ECP_NO_INTERNAL_RNG From 96951785fcda1afc105facb27b41212404bda2d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 22 Jun 2020 10:18:58 +0200 Subject: [PATCH 14/14] Test multi-block output of the hash-based KDF MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Manuel Pégourié-Gonnard --- library/ecp.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index c13689ded..6b1a96e7e 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -2393,33 +2393,35 @@ cleanup: * https://github.com/patrickfav/singlestep-kdf/wiki/NIST-SP-800-56C-Rev1:-Non-Official-Test-Vectors * * We only use the ones with empty fixedInfo, and for brevity's sake, only - * 32-bytes output (with SHA-256 that's exactly one block, and with SHA-512 + * 40-bytes output (with SHA-256 that's more than one block, and with SHA-512 * less than one block). */ #if defined(MBEDTLS_SHA512_C) static const uint8_t test_kdf_z[16] = { - 0xeb, 0xf3, 0x19, 0x67, 0x1e, 0xac, 0xcc, 0x6f, - 0xc5, 0xc0, 0x5d, 0x95, 0x8d, 0x17, 0x15, 0x94, + 0x3b, 0xa9, 0x79, 0xe9, 0xbc, 0x5e, 0x3e, 0xc7, + 0x61, 0x30, 0x36, 0xb6, 0xf5, 0x1c, 0xd5, 0xaa, }; -static const uint8_t test_kdf_out[32] = { - 0xa9, 0x48, 0x85, 0x67, 0x54, 0x7c, 0x2a, 0x8e, - 0x9e, 0xd1, 0x67, 0x76, 0xe3, 0x1c, 0x03, 0x92, - 0x41, 0x77, 0x2a, 0x9e, 0xc7, 0xcc, 0xd7, 0x1f, - 0xda, 0x12, 0xe9, 0xba, 0xc9, 0xb2, 0x17, 0x24, +static const uint8_t test_kdf_out[40] = { + 0x3e, 0xf6, 0xda, 0xf9, 0x51, 0x60, 0x70, 0x5f, + 0xdf, 0x21, 0xcd, 0xab, 0xac, 0x25, 0x7b, 0x05, + 0xfe, 0xc1, 0xab, 0x7c, 0xc9, 0x68, 0x43, 0x25, + 0x8a, 0xfc, 0x40, 0x6e, 0x5b, 0xf7, 0x98, 0x27, + 0x10, 0xfa, 0x7b, 0x93, 0x52, 0xd4, 0x16, 0xaa, }; #elif defined(MBEDTLS_SHA256_C) static const uint8_t test_kdf_z[16] = { - 0x0d, 0x5e, 0xc8, 0x9a, 0x68, 0xb1, 0xa7, 0xa0, - 0xdf, 0x95, 0x24, 0x54, 0x3f, 0x4d, 0x70, 0xef, + 0xc8, 0x3e, 0x35, 0x8e, 0x99, 0xa6, 0x89, 0xc6, + 0x7d, 0xb4, 0xfe, 0x39, 0xcf, 0x8f, 0x26, 0xe1, }; -static const uint8_t test_kdf_out[32] = { - 0x77, 0xbc, 0x94, 0x9e, 0xa0, 0xd3, 0xdd, 0x5c, - 0x8e, 0xb7, 0xeb, 0x84, 0x05, 0x40, 0x60, 0xfa, - 0x96, 0x6e, 0x7e, 0xcd, 0x73, 0x9f, 0xa1, 0xe6, - 0x34, 0x3f, 0x6d, 0x82, 0x16, 0x22, 0xb4, 0x45, +static const uint8_t test_kdf_out[40] = { + 0x7d, 0xf6, 0x41, 0xf8, 0x3c, 0x47, 0xdc, 0x28, + 0x5f, 0x7f, 0xaa, 0xde, 0x05, 0x64, 0xd6, 0x25, + 0x00, 0x6a, 0x47, 0xd9, 0x1e, 0xa4, 0xa0, 0x8c, + 0xd7, 0xf7, 0x0c, 0x99, 0xaa, 0xa0, 0x72, 0x66, + 0x69, 0x0e, 0x25, 0xaa, 0xa1, 0x63, 0x14, 0x79, }; #endif