From da5ccce88e831b135b9c5a94024b866a10b10445 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Sep 2019 14:40:40 +0200 Subject: [PATCH 01/95] CTR_DRBG documentation clarifications * State explicit whether several numbers are in bits or bytes. * Clarify whether buffer pointer parameters can be NULL. * Explain the value of constants that are dependent on the configuration. --- include/mbedtls/ctr_drbg.h | 60 ++++++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 18 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index cc3df7b11..1b4a24b61 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -15,7 +15,7 @@ * keys and operations that use random values generated to 128-bit security. */ /* - * Copyright (C) 2006-2018, Arm Limited (or its affiliates), All Rights Reserved + * Copyright (C) 2006-2019, Arm Limited (or its affiliates), All Rights Reserved * SPDX-License-Identifier: Apache-2.0 * * Licensed under the Apache License, Version 2.0 (the "License"); you may @@ -56,9 +56,19 @@ #define MBEDTLS_CTR_DRBG_BLOCKSIZE 16 /**< The block size used by the cipher. */ #if defined(MBEDTLS_CTR_DRBG_USE_128_BIT_KEY) -#define MBEDTLS_CTR_DRBG_KEYSIZE 16 /**< The key size used by the cipher (compile-time choice: 128 bits). */ +#define MBEDTLS_CTR_DRBG_KEYSIZE 16 +/**< The key size in bytes used by the cipher. + * + * Compile-time choice: 16 bytes (128 bits) + * because #MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is set. + */ #else -#define MBEDTLS_CTR_DRBG_KEYSIZE 32 /**< The key size used by the cipher (compile-time choice: 256 bits). */ +#define MBEDTLS_CTR_DRBG_KEYSIZE 32 +/**< The key size in bytes used by the cipher. + * + * Compile-time choice: 32 bytes (256 bits) + * because `MBEDTLS_CTR_DRBG_USE_128_BIT_KEY` is not set. + */ #endif #define MBEDTLS_CTR_DRBG_KEYBITS ( MBEDTLS_CTR_DRBG_KEYSIZE * 8 ) /**< The key size for the DRBG operation, in bits. */ @@ -75,17 +85,25 @@ #if !defined(MBEDTLS_CTR_DRBG_ENTROPY_LEN) #if defined(MBEDTLS_SHA512_C) && !defined(MBEDTLS_ENTROPY_FORCE_SHA256) +/** The amount of entropy used per seed by default. + * + * This is 48 bytes because the entropy module uses SHA-512 + * (`MBEDTLS_ENTROPY_FORCE_SHA256` is not set). + * + * \note See mbedtls_ctr_drbg_set_entropy_len() regarding what values are + * acceptable. + */ #define MBEDTLS_CTR_DRBG_ENTROPY_LEN 48 -/**< The amount of entropy used per seed by default: - * - */ #else -#define MBEDTLS_CTR_DRBG_ENTROPY_LEN 32 -/**< Amount of entropy used per seed by default: - * +/** The amount of entropy used per seed by default. + * + * This is 32 bytes because the entropy module uses SHA-256 + * (the SHA-512 module is disabled or `MBEDTLS_ENTROPY_FORCE_SHA256` is set). + * + * \note See mbedtls_ctr_drbg_set_entropy_len() regarding what values are + * acceptable. */ +#define MBEDTLS_CTR_DRBG_ENTROPY_LEN 32 #endif #endif @@ -106,7 +124,7 @@ #if !defined(MBEDTLS_CTR_DRBG_MAX_SEED_INPUT) #define MBEDTLS_CTR_DRBG_MAX_SEED_INPUT 384 -/**< The maximum size of seed or reseed buffer. */ +/**< The maximum size of seed or reseed buffer in bytes. */ #endif /* \} name SECTION: Module settings */ @@ -170,10 +188,12 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * \param ctx The CTR_DRBG context to seed. * \param f_entropy The entropy callback, taking as arguments the * \p p_entropy context, the buffer to fill, and the - length of the buffer. + * length of the buffer. * \param p_entropy The entropy context. * \param custom Personalization data, that is device-specific - identifiers. Can be NULL. + * identifiers. This can be NULL, in which case the + * personalization data is empty regardless of the value + * of \p len. * \param len The length of the personalization data. * * \return \c 0 on success. @@ -213,7 +233,7 @@ void mbedtls_ctr_drbg_set_prediction_resistance( mbedtls_ctr_drbg_context *ctx, * #MBEDTLS_CTR_DRBG_ENTROPY_LEN. * * \param ctx The CTR_DRBG context. - * \param len The amount of entropy to grab. + * \param len The amount of entropy to grab, in bytes. */ void mbedtls_ctr_drbg_set_entropy_len( mbedtls_ctr_drbg_context *ctx, size_t len ); @@ -246,7 +266,8 @@ int mbedtls_ctr_drbg_reseed( mbedtls_ctr_drbg_context *ctx, * \brief This function updates the state of the CTR_DRBG context. * * \param ctx The CTR_DRBG context. - * \param additional The data to update the state with. + * \param additional The data to update the state with. This must not be + * null unless \p add_len is 0. * \param add_len Length of \p additional in bytes. This must be at * most #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT. * @@ -270,8 +291,11 @@ int mbedtls_ctr_drbg_update_ret( mbedtls_ctr_drbg_context *ctx, * #mbedtls_ctr_drbg_context structure. * \param output The buffer to fill. * \param output_len The length of the buffer. - * \param additional Additional data to update. Can be NULL. - * \param add_len The length of the additional data. + * \param additional Additional data to update. Can be NULL, in which + * case the additional data is empty regardless of + * the value of \p add_len. + * \param add_len The length of the additional data + * if \p additional is non-null. * * \return \c 0 on success. * \return #MBEDTLS_ERR_CTR_DRBG_ENTROPY_SOURCE_FAILED or From c1c9292d2fcfdae0a6543dcaa5fff50e837567f5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Sep 2019 14:48:30 +0200 Subject: [PATCH 02/95] CTR_DRBG: Document the maximum size of some parameters --- include/mbedtls/ctr_drbg.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index 1b4a24b61..58b9a65d8 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -195,6 +195,8 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * personalization data is empty regardless of the value * of \p len. * \param len The length of the personalization data. + * This must be at most + * #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT / 2. * * \return \c 0 on success. * \return #MBEDTLS_ERR_CTR_DRBG_ENTROPY_SOURCE_FAILED on failure. @@ -234,6 +236,7 @@ void mbedtls_ctr_drbg_set_prediction_resistance( mbedtls_ctr_drbg_context *ctx, * * \param ctx The CTR_DRBG context. * \param len The amount of entropy to grab, in bytes. + * This must be at most #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT. */ void mbedtls_ctr_drbg_set_entropy_len( mbedtls_ctr_drbg_context *ctx, size_t len ); @@ -255,6 +258,10 @@ void mbedtls_ctr_drbg_set_reseed_interval( mbedtls_ctr_drbg_context *ctx, * \param ctx The CTR_DRBG context. * \param additional Additional data to add to the state. Can be NULL. * \param len The length of the additional data. + * This must be less than + * #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT - \c entropy_len + * where \c entropy_len is the entropy length + * configured for the context. * * \return \c 0 on success. * \return #MBEDTLS_ERR_CTR_DRBG_ENTROPY_SOURCE_FAILED on failure. @@ -296,6 +303,11 @@ int mbedtls_ctr_drbg_update_ret( mbedtls_ctr_drbg_context *ctx, * the value of \p add_len. * \param add_len The length of the additional data * if \p additional is non-null. + * This must be less than #MBEDTLS_CTR_DRBG_MAX_INPUT + * and less than + * #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT - \c entropy_len + * where \c entropy_len is the entropy length + * configured for the context. * * \return \c 0 on success. * \return #MBEDTLS_ERR_CTR_DRBG_ENTROPY_SOURCE_FAILED or From e3dc5942c56583a4ae373588a15cbfa726f95d72 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Sep 2019 14:48:53 +0200 Subject: [PATCH 03/95] CTR_DRBG: Document the security strength and SP 800-90A compliance Document that a derivation function is used. Document the security strength of the DRBG depending on the compile-time configuration and how it is set up. In particular, document how the nonce specified in SP 800-90A is set. Mention how to link the ctr_drbg module with the entropy module. --- include/mbedtls/ctr_drbg.h | 55 +++++++++++++++++++++++++++++++++++--- 1 file changed, 51 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index 58b9a65d8..05a2aba2c 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -9,9 +9,22 @@ * Bit Generators. * * The Mbed TLS implementation of CTR_DRBG uses AES-256 (default) or AES-128 - * as the underlying block cipher. + * as the underlying block cipher, with a derivation function. The security + * strength is: + * - 256 bits under the default configuration of the library, with AES-256 + * (`MBEDTLS_CTR_DRBG_USE_128_BIT_KEY` not set) and + * with #MBEDTLS_CTR_DRBG_ENTROPY_LEN set to 48 or more. + * - 256 bits if AES-256 is used, #MBEDTLS_CTR_DRBG_ENTROPY_LEN is set + * - 128 bits if AES-256 is used but #MBEDTLS_CTR_DRBG_ENTROPY_LEN is + * between 24 and 47 and the DRBG is not initialized with an explicit + * nonce (see mbedtls_ctr_drbg_seed()). + * - 128 bits if AES-128 is used (`MBEDTLS_CTR_DRBG_USE_128_BIT_KEY` set) + * and #MBEDTLS_CTR_DRBG_ENTROPY_LEN is set to 24 or more (which is + * always the case unless it is explicitly set to a different value + * in `config.h`). * - * \warning Using 128-bit keys for CTR_DRBG limits the security of generated + * \warning Using 128-bit keys for CTR_DRBG or using SHA-256 as the entropy + * compression function limits the security of generated * keys and operations that use random values generated to 128-bit security. */ /* @@ -182,8 +195,35 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * \brief This function seeds and sets up the CTR_DRBG * entropy source for future reseeds. * - * \note Personalization data can be provided in addition to the more generic - * entropy source, to make this instantiation as unique as possible. + * A typical choice for the \p f_entropy and \p p_entropy parameters is + * to use the entropy module: + * - \p f_entropy is mbedtls_entropy_func(); + * - \p p_entropy is an instance of ::mbedtls_entropy_context initialized + * with mbedtls_entropy_init() (which registers the platform's default + * entropy sources). + * + * Personalization data can be provided in addition to the more generic + * entropy source, to make this instantiation as unique as possible. + * + * \note The _seed_material_ value passed to the derivation + * function in the CTR_DRBG Instantiate Process + * described in NIST SP 800-90A §10.2.1.3.2 + * is the concatenation of the string obtained from + * calling \p f_entropy and the \p custom string. + * The origin of the nonce depends on the value of + * the entropy length relative to the security strength. + * See the documentation of + * mbedtls_ctr_drbg_set_entropy_len() for information + * about the entropy length. + * - If the entropy length is at least 1.5 times the + * security strength then the nonce is taken from the + * string obtained with \p f_entropy. + * - If the entropy length is less than the security + * strength, then the nonce is taken from \p custom. + * In this case, for compliance with SP 800-90A, + * you must pass a unique value of \p custom at + * each invocation. See SP 800-90A §8.6.7 for more + * details. * * \param ctx The CTR_DRBG context to seed. * \param f_entropy The entropy callback, taking as arguments the @@ -234,6 +274,13 @@ void mbedtls_ctr_drbg_set_prediction_resistance( mbedtls_ctr_drbg_context *ctx, * seed or reseed. The default value is * #MBEDTLS_CTR_DRBG_ENTROPY_LEN. * + * \note For compliance with NIST SP 800-90A, the entropy length + * must be at least 1.5 times security strength, since + * the entropy source is used both as the entropy input + * and to provide the initial nonce: + * - 24 bytes if using AES-128; + * - 48 bytes if using AES-256. + * * \param ctx The CTR_DRBG context. * \param len The amount of entropy to grab, in bytes. * This must be at most #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT. From ca28583e9591149a212685b95c9ffd59166faded Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Sep 2019 14:52:32 +0200 Subject: [PATCH 04/95] CTR_DRBG documentation: add changelog entry This is a documentation-only change, but one that users who care about NIST compliance may be interested in, to review if they're using the module in a compliant way. --- ChangeLog | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ChangeLog b/ChangeLog index 6ba9a5a3a..70488e224 100644 --- a/ChangeLog +++ b/ChangeLog @@ -73,6 +73,8 @@ Bugfix Changes * Add unit tests for AES-GCM when called through mbedtls_cipher_auth_xxx() from the cipher abstraction layer. Fixes #2198. + * Clarify how the interface of the CTR_DRBG module relates to + NIST SP 800-90A. = mbed TLS 2.16.3 branch released 2019-09-06 From bb2b8da7d7e3723b7ff262006effa4e26563b9e0 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 25 Sep 2019 20:22:24 +0200 Subject: [PATCH 05/95] CTR_DRBG: Finish an unfinished paragraph --- include/mbedtls/ctr_drbg.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index 05a2aba2c..03ce87f72 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -15,6 +15,8 @@ * (`MBEDTLS_CTR_DRBG_USE_128_BIT_KEY` not set) and * with #MBEDTLS_CTR_DRBG_ENTROPY_LEN set to 48 or more. * - 256 bits if AES-256 is used, #MBEDTLS_CTR_DRBG_ENTROPY_LEN is set + * to 32 or more, and the DRBG is initialized with an explicit + * nonce in the \c custom parameter to see mbedtls_ctr_drbg_seed(). * - 128 bits if AES-256 is used but #MBEDTLS_CTR_DRBG_ENTROPY_LEN is * between 24 and 47 and the DRBG is not initialized with an explicit * nonce (see mbedtls_ctr_drbg_seed()). From 2d83fe13835265565714e031450344f86b809fa3 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 25 Sep 2019 20:22:40 +0200 Subject: [PATCH 06/95] CTR_DRBG: improve the discussion of entropy length vs strength --- include/mbedtls/ctr_drbg.h | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index 03ce87f72..32d9adf33 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -277,11 +277,30 @@ void mbedtls_ctr_drbg_set_prediction_resistance( mbedtls_ctr_drbg_context *ctx, * #MBEDTLS_CTR_DRBG_ENTROPY_LEN. * * \note For compliance with NIST SP 800-90A, the entropy length - * must be at least 1.5 times security strength, since - * the entropy source is used both as the entropy input - * and to provide the initial nonce: - * - 24 bytes if using AES-128; - * - 48 bytes if using AES-256. + * (\p len bytes = \p len * 8 bits) + * must be at least the security strength. + * Furthermore, if the entropy input is used to provide + * the nonce, the entropy length must be 1.5 times + * the security strength. + * Per NIST SP 800-57A table 2, the achievable security + * strength is 128 bits if using AES-128 and + * 256 bits if using AES-256. + * Therefore, to provide full security, + * the entropy input must be at least: + * - 24 bytes if using AES-128 and the \p custom + * argument to mbedtls_ctr_drbg_seed() may repeat + * (for example because it is empty, or more generally + * constant); + * - 48 bytes if using AES-256 and the \p custom + * argument to mbedtls_ctr_drbg_seed() may repeat + * (for example because it is empty, or more generally + * constant); + * - 16 bytes if using AES-128 and the \p custom + * argument to mbedtls_ctr_drbg_seed() includes + * a nonce; + * - 32 bytes if using AES-256 and the \p custom + * argument to mbedtls_ctr_drbg_seed() includes + * a nonce. * * \param ctx The CTR_DRBG context. * \param len The amount of entropy to grab, in bytes. From 03642fa0262e0f30b2266dfcb3b1c83b46afefcd Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 26 Sep 2019 14:53:44 +0200 Subject: [PATCH 07/95] Remove warning that the previous expanded discussion has obsoleted --- include/mbedtls/ctr_drbg.h | 4 ---- 1 file changed, 4 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index 32d9adf33..ae0da45a5 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -24,10 +24,6 @@ * and #MBEDTLS_CTR_DRBG_ENTROPY_LEN is set to 24 or more (which is * always the case unless it is explicitly set to a different value * in `config.h`). - * - * \warning Using 128-bit keys for CTR_DRBG or using SHA-256 as the entropy - * compression function limits the security of generated - * keys and operations that use random values generated to 128-bit security. */ /* * Copyright (C) 2006-2019, Arm Limited (or its affiliates), All Rights Reserved From c32f74cf8e786d9349d78509a3620ad6fe2d61e4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 26 Sep 2019 14:54:42 +0200 Subject: [PATCH 08/95] Fix wording --- include/mbedtls/ctr_drbg.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index ae0da45a5..6098df8cf 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -16,7 +16,7 @@ * with #MBEDTLS_CTR_DRBG_ENTROPY_LEN set to 48 or more. * - 256 bits if AES-256 is used, #MBEDTLS_CTR_DRBG_ENTROPY_LEN is set * to 32 or more, and the DRBG is initialized with an explicit - * nonce in the \c custom parameter to see mbedtls_ctr_drbg_seed(). + * nonce in the \c custom parameter to mbedtls_ctr_drbg_seed(). * - 128 bits if AES-256 is used but #MBEDTLS_CTR_DRBG_ENTROPY_LEN is * between 24 and 47 and the DRBG is not initialized with an explicit * nonce (see mbedtls_ctr_drbg_seed()). From 99d76f8805c3f2cd00b8d9436a547f16a6a4fec6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 17 Sep 2019 18:18:58 +0200 Subject: [PATCH 09/95] Add a calloc self-test Add a very basic test of calloc to the selftest program. The selftest program acts in its capacity as a platform compatibility checker rather than in its capacity as a test of the library. The main objective is to report whether calloc returns NULL for a size of 0. Also observe whether a free/alloc sequence returns the address that was just freed and whether a size overflow is properly detected. --- programs/test/selftest.c | 83 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/programs/test/selftest.c b/programs/test/selftest.c index 82f08fa1b..4a8ed6843 100644 --- a/programs/test/selftest.c +++ b/programs/test/selftest.c @@ -66,6 +66,8 @@ #else #include #include +#define mbedtls_calloc calloc +#define mbedtls_free free #define mbedtls_printf printf #define mbedtls_snprintf snprintf #define mbedtls_exit exit @@ -78,6 +80,86 @@ #endif +#if defined MBEDTLS_SELF_TEST +/* Sanity check for malloc. This is not expected to fail, and is rather + * intended to display potentially useful information about the platform, + * in particular the behavior of malloc(0). */ +static int calloc_self_test( int verbose ) +{ + int failures = 0; + void *empty1 = mbedtls_calloc( 0, 1 ); + void *empty2 = mbedtls_calloc( 0, 1 ); + void *buffer1 = mbedtls_calloc( 1, 1 ); + void *buffer2 = mbedtls_calloc( 1, 1 ); + uintptr_t old_buffer1; + + if( empty1 == NULL && empty2 == NULL ) + { + if( verbose ) + mbedtls_printf( " CALLOC(0): passed (NULL)\n" ); + } + else if( empty1 == NULL || empty2 == NULL ) + { + if( verbose ) + mbedtls_printf( " CALLOC(0): failed (mix of NULL and non-NULL)\n" ); + ++failures; + } + else if( empty1 == empty2 ) + { + if( verbose ) + mbedtls_printf( " CALLOC(0): passed (same non-null)\n" ); + } + else + { + if( verbose ) + mbedtls_printf( " CALLOC(0): passed (distinct non-null)\n" ); + } + + if( buffer1 == NULL || buffer2 == NULL ) + { + if( verbose ) + mbedtls_printf( " CALLOC(1): failed (NULL)\n" ); + ++failures; + } + else if( buffer1 == buffer2 ) + { + if( verbose ) + mbedtls_printf( " CALLOC(1): failed (same buffer twice)\n" ); + ++failures; + } + else + { + if( verbose ) + mbedtls_printf( " CALLOC(1): passed\n" ); + } + + old_buffer1 = (uintptr_t) buffer1; + mbedtls_free( buffer1 ); + buffer1 = mbedtls_calloc( 1, 1 ); + if( buffer1 == NULL ) + { + if( verbose ) + mbedtls_printf( " CALLOC(1 again): failed (NULL)\n" ); + ++failures; + } + else + { + if( verbose ) + mbedtls_printf( " CALLOC(1 again): passed (%s address)\n", + (uintptr_t) old_buffer1 == (uintptr_t) buffer1 ? + "same" : "different" ); + } + + if( verbose ) + mbedtls_printf( "\n" ); + mbedtls_free( empty1 ); + mbedtls_free( empty2 ); + mbedtls_free( buffer1 ); + mbedtls_free( buffer2 ); + return( failures ); +} +#endif /* MBEDTLS_SELF_TEST */ + static int test_snprintf( size_t n, const char ref_buf[10], int ref_ret ) { int ret; @@ -174,6 +256,7 @@ typedef struct const selftest_t selftests[] = { + {"calloc", calloc_self_test}, #if defined(MBEDTLS_MD2_C) {"md2", mbedtls_md2_self_test}, #endif From 34693b5dd6a740c51367f4bcad47719d12462ce4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 17 Sep 2019 19:04:38 +0200 Subject: [PATCH 10/95] Add a test component with malloc(0) returning NULL Exercise the library functions with calloc returning NULL for a size of 0. Make this a separate job with UBSan (and ASan) to detect places where we try to dereference the result of calloc(0) or to do things like buf = calloc(size, 1); if (buf == NULL && size != 0) return INSUFFICIENT_MEMORY; memcpy(buf, source, size); which has undefined behavior when buf is NULL at the memcpy call even if size is 0. This is needed because other test components jobs either use the system malloc which returns non-NULL on Linux and FreeBSD, or the memory_buffer_alloc malloc which returns NULL but does not give as useful feedback with ASan (because the whole heap is a single C object). --- tests/configs/config-wrapper-malloc-0-null.h | 39 ++++++++++++++++++++ tests/scripts/all.sh | 15 ++++++++ 2 files changed, 54 insertions(+) create mode 100644 tests/configs/config-wrapper-malloc-0-null.h diff --git a/tests/configs/config-wrapper-malloc-0-null.h b/tests/configs/config-wrapper-malloc-0-null.h new file mode 100644 index 000000000..ed74eda63 --- /dev/null +++ b/tests/configs/config-wrapper-malloc-0-null.h @@ -0,0 +1,39 @@ +/* config.h wrapper that forces calloc(0) to return NULL. + * Used for testing. + */ +/* + * Copyright (C) 2019, ARM Limited, All Rights Reserved + * 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. + * + * This file is part of mbed TLS (https://tls.mbed.org) + */ + +#ifndef MBEDTLS_CONFIG_H +/* Don't #define MBEDTLS_CONFIG_H, let config.h do it. */ + +#include "mbedtls/config.h" + +#include +static inline void *custom_calloc( size_t nmemb, size_t size ) +{ + if( nmemb == 0 || size == 0 ) + return( NULL ); + return( calloc( nmemb, size ) ); +} + +#define MBEDTLS_PLATFORM_MEMORY +#define MBEDTLS_PLATFORM_STD_CALLOC custom_calloc + +#endif /* MBEDTLS_CONFIG_H */ diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index bd7e98f6c..ee6d0a264 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1284,6 +1284,21 @@ component_test_platform_calloc_macro () { make test } +component_test_malloc_0_null () { + msg "build: malloc(0) returns NULL (ASan+UBSan build)" + scripts/config.pl full + scripts/config.pl unset MBEDTLS_MEMORY_BUFFER_ALLOC_C + make CC=gcc CFLAGS="'-DMBEDTLS_CONFIG_FILE=\"$PWD/tests/configs/config-wrapper-malloc-0-null.h\"' -O -Werror -Wall -Wextra -fsanitize=address,undefined" LDFLAGS='-fsanitize=address,undefined' + + msg "test: malloc(0) returns NULL (ASan+UBSan build)" + make test + + msg "selftest: malloc(0) returns NULL (ASan+UBSan build)" + # Just the calloc selftest. "make test" ran the others as part of the + # test suites. + if_build_succeeded programs/test/selftest calloc +} + component_test_aes_fewer_tables () { msg "build: default config with AES_FEWER_TABLES enabled" scripts/config.pl set MBEDTLS_AES_FEWER_TABLES From f17079d960599c908f3f3f4d63848a6fa0e0cd2f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 30 Sep 2019 15:01:02 +0200 Subject: [PATCH 11/95] More CTR_DRBG documentation improvements and clarifications --- include/mbedtls/ctr_drbg.h | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index 6098df8cf..f65606eb4 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -200,6 +200,9 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * with mbedtls_entropy_init() (which registers the platform's default * entropy sources). * + * \p f_entropy is always called with a buffer size equal to the entropy + * length described in the documentation of mbedtls_ctr_drbg_set_entropy_len(). + * * Personalization data can be provided in addition to the more generic * entropy source, to make this instantiation as unique as possible. * @@ -227,7 +230,7 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * \param f_entropy The entropy callback, taking as arguments the * \p p_entropy context, the buffer to fill, and the * length of the buffer. - * \param p_entropy The entropy context. + * \param p_entropy The entropy context to pass to \p f_entropy. * \param custom Personalization data, that is device-specific * identifiers. This can be NULL, in which case the * personalization data is empty regardless of the value @@ -257,7 +260,8 @@ void mbedtls_ctr_drbg_free( mbedtls_ctr_drbg_context *ctx ); * The default value is off. * * \note If enabled, entropy is gathered at the beginning of - * every call to mbedtls_ctr_drbg_random_with_add(). + * every call to mbedtls_ctr_drbg_random_with_add() + * or mbedtls_ctr_drbg_random(). * Only use this if your entropy source has sufficient * throughput. * @@ -269,8 +273,9 @@ void mbedtls_ctr_drbg_set_prediction_resistance( mbedtls_ctr_drbg_context *ctx, /** * \brief This function sets the amount of entropy grabbed on each - * seed or reseed. The default value is - * #MBEDTLS_CTR_DRBG_ENTROPY_LEN. + * seed or reseed. + * + * The default value is #MBEDTLS_CTR_DRBG_ENTROPY_LEN. * * \note For compliance with NIST SP 800-90A, the entropy length * (\p len bytes = \p len * 8 bits) @@ -307,7 +312,12 @@ void mbedtls_ctr_drbg_set_entropy_len( mbedtls_ctr_drbg_context *ctx, /** * \brief This function sets the reseed interval. - * The default value is #MBEDTLS_CTR_DRBG_RESEED_INTERVAL. + * + * The reseed interval is the number of calls to mbedtls_ctr_drbg_random() + * or mbedtls_ctr_drbg_random_with_add() after which the entropy function + * is called again. + * + * The default value is #MBEDTLS_CTR_DRBG_RESEED_INTERVAL. * * \param ctx The CTR_DRBG context. * \param interval The reseed interval. @@ -361,7 +371,7 @@ int mbedtls_ctr_drbg_update_ret( mbedtls_ctr_drbg_context *ctx, * \param p_rng The CTR_DRBG context. This must be a pointer to a * #mbedtls_ctr_drbg_context structure. * \param output The buffer to fill. - * \param output_len The length of the buffer. + * \param output_len The length of the buffer in bytes. * \param additional Additional data to update. Can be NULL, in which * case the additional data is empty regardless of * the value of \p add_len. @@ -436,7 +446,7 @@ MBEDTLS_DEPRECATED void mbedtls_ctr_drbg_update( * * \return \c 0 on success. * \return #MBEDTLS_ERR_CTR_DRBG_FILE_IO_ERROR on file error. - * \return #MBEDTLS_ERR_CTR_DRBG_ENTROPY_SOURCE_FAILED on + * \return #MBEDTLS_ERR_CTR_DRBG_ENTROPY_SOURCE_FAILED on reseed * failure. */ int mbedtls_ctr_drbg_write_seed_file( mbedtls_ctr_drbg_context *ctx, const char *path ); @@ -450,8 +460,10 @@ int mbedtls_ctr_drbg_write_seed_file( mbedtls_ctr_drbg_context *ctx, const char * * \return \c 0 on success. * \return #MBEDTLS_ERR_CTR_DRBG_FILE_IO_ERROR on file error. - * \return #MBEDTLS_ERR_CTR_DRBG_ENTROPY_SOURCE_FAILED or - * #MBEDTLS_ERR_CTR_DRBG_INPUT_TOO_BIG on failure. + * \return #MBEDTLS_ERR_CTR_DRBG_ENTROPY_SOURCE_FAILED on + * reseed failure. + * \return #MBEDTLS_ERR_CTR_DRBG_INPUT_TOO_BIG if the existing + * seed file is too large. */ int mbedtls_ctr_drbg_update_seed_file( mbedtls_ctr_drbg_context *ctx, const char *path ); #endif /* MBEDTLS_FS_IO */ From 4bfe4540f37fd858dfb84066b05d55eebbcd7e28 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 30 Sep 2019 15:01:15 +0200 Subject: [PATCH 12/95] HMAC_DRBG documentation improvements clarifications Improve the formatting and writing of the documentation based on what had been done for CTR_DRBG. Document the maximum size and nullability of some buffer parameters. --- include/mbedtls/hmac_drbg.h | 265 ++++++++++++++++++++++-------------- 1 file changed, 162 insertions(+), 103 deletions(-) diff --git a/include/mbedtls/hmac_drbg.h b/include/mbedtls/hmac_drbg.h index f38337aa7..b5dc829b1 100644 --- a/include/mbedtls/hmac_drbg.h +++ b/include/mbedtls/hmac_drbg.h @@ -1,10 +1,14 @@ /** * \file hmac_drbg.h * - * \brief HMAC_DRBG (NIST SP 800-90A) + * \brief The HMAC_DRBG pseudorandom generator. + * + * This module implements the HMAC_DRBG pseudorandom generator described + * in NIST SP 800-90A: Recommendation for Random Number Generation Using + * Deterministic Random Bit Generators. */ /* - * Copyright (C) 2006-2015, ARM Limited, All Rights Reserved + * Copyright (C) 2006-2019, ARM Limited, All Rights Reserved * SPDX-License-Identifier: Apache-2.0 * * Licensed under the Apache License, Version 2.0 (the "License"); you may @@ -104,38 +108,50 @@ typedef struct mbedtls_hmac_drbg_context } mbedtls_hmac_drbg_context; /** - * \brief HMAC_DRBG context initialization - * Makes the context ready for mbedtls_hmac_drbg_seed(), - * mbedtls_hmac_drbg_seed_buf() or - * mbedtls_hmac_drbg_free(). + * \brief HMAC_DRBG context initialization. * - * \param ctx HMAC_DRBG context to be initialized + * This function makes the context ready for mbedtls_hmac_drbg_seed(), + * mbedtls_hmac_drbg_seed_buf() or mbedtls_hmac_drbg_free(). + * + * \param ctx HMAC_DRBG context to be initialized. */ void mbedtls_hmac_drbg_init( mbedtls_hmac_drbg_context *ctx ); /** - * \brief HMAC_DRBG initial seeding - * Seed and setup entropy source for future reseeds. + * \brief HMAC_DRBG initial seeding. * - * \param ctx HMAC_DRBG context to be seeded - * \param md_info MD algorithm to use for HMAC_DRBG - * \param f_entropy Entropy callback (p_entropy, buffer to fill, buffer - * length) - * \param p_entropy Entropy context - * \param custom Personalization data (Device specific identifiers) - * (Can be NULL) - * \param len Length of personalization data + * Set the initial seed and set up the entropy source for future reseeds. + * + * \param ctx HMAC_DRBG context to be seeded. + * \param md_info MD algorithm to use for HMAC_DRBG. + * \param f_entropy The entropy callback, taking as arguments the + * \p p_entropy context, the buffer to fill, and the + * length of the buffer. + * \param p_entropy The entropy context to pass to \p f_entropy. + * \param custom Personalization data, that is device-specific + * identifiers. This can be NULL, in which case the + * personalization data is empty regardless of the value + * of \p len. + * \param len The length of the personalization data. + * This must be at most #MBEDTLS_HMAC_DRBG_MAX_INPUT + * and also at most + * #MBEDTLS_HMAC_DRBG_MAX_SEED_INPUT - \p entropy_len * 3 / 2 + * where \p entropy_len is the entropy length + * (see mbedtls_hmac_drbg_set_entropy_len()). * * \note The "security strength" as defined by NIST is set to: - * 128 bits if md_alg is SHA-1, - * 192 bits if md_alg is SHA-224, - * 256 bits if md_alg is SHA-256 or higher. + * 128 bits if \p md_info is SHA-1, + * 192 bits if \p md_info is SHA-224, + * 256 bits if \p md_info is SHA-256, SHA-384 or SHA-512. * Note that SHA-256 is just as efficient as SHA-224. * - * \return 0 if successful, or - * MBEDTLS_ERR_MD_BAD_INPUT_DATA, or - * MBEDTLS_ERR_MD_ALLOC_FAILED, or - * MBEDTLS_ERR_HMAC_DRBG_ENTROPY_SOURCE_FAILED. + * \return 0 if successful. + * \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA if \p md_info is + * invalid. + * \return #MBEDTLS_ERR_MD_ALLOC_FAILED if there was not enough + * memory to allocate context data. + * \return #MBEDTLS_ERR_HMAC_DRBG_ENTROPY_SOURCE_FAILED + * if the call to \p f_entropy failed. */ int mbedtls_hmac_drbg_seed( mbedtls_hmac_drbg_context *ctx, mbedtls_md_handle_t md_info, @@ -146,100 +162,134 @@ int mbedtls_hmac_drbg_seed( mbedtls_hmac_drbg_context *ctx, /** * \brief Initilisation of simpified HMAC_DRBG (never reseeds). - * (For use with deterministic ECDSA.) * - * \param ctx HMAC_DRBG context to be initialised - * \param md_info MD algorithm to use for HMAC_DRBG - * \param data Concatenation of entropy string and additional data - * \param data_len Length of data in bytes + * This function is meant for use in algorithms that need a pseudorandom + * input such as deterministic ECDSA. * - * \return 0 if successful, or - * MBEDTLS_ERR_MD_BAD_INPUT_DATA, or - * MBEDTLS_ERR_MD_ALLOC_FAILED. + * \param ctx HMAC_DRBG context to be initialised. + * \param md_info MD algorithm to use for HMAC_DRBG. + * \param data Concatenation of the initial entropy string and + * the additional data. + * \param data_len Length of \p data in bytes. + * + * \return 0 if successful. or + * \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA if \p md_info is + * invalid. + * \return #MBEDTLS_ERR_MD_ALLOC_FAILED if there was not enough + * memory to allocate context data. */ int mbedtls_hmac_drbg_seed_buf( mbedtls_hmac_drbg_context *ctx, mbedtls_md_handle_t md_info, const unsigned char *data, size_t data_len ); /** - * \brief Enable / disable prediction resistance (Default: Off) + * \brief This function turns prediction resistance on or off. + * The default value is off. * - * Note: If enabled, entropy is used for ctx->entropy_len before each call! - * Only use this if you have ample supply of good entropy! + * \note If enabled, entropy is gathered at the beginning of + * every call to mbedtls_hmac_drbg_random_with_add() + * or mbedtls_hmac_drbg_random(). + * Only use this if your entropy source has sufficient + * throughput. * - * \param ctx HMAC_DRBG context - * \param resistance MBEDTLS_HMAC_DRBG_PR_ON or MBEDTLS_HMAC_DRBG_PR_OFF + * \param ctx The HMAC_DRBG context. + * \param resistance #MBEDTLS_HMAC_DRBG_PR_ON or #MBEDTLS_HMAC_DRBG_PR_OFF. */ void mbedtls_hmac_drbg_set_prediction_resistance( mbedtls_hmac_drbg_context *ctx, int resistance ); /** - * \brief Set the amount of entropy grabbed on each reseed - * (Default: given by the security strength, which - * depends on the hash used, see \c mbedtls_hmac_drbg_init() ) + * \brief This function sets the amount of entropy grabbed on each + * seed or reseed. * - * \param ctx HMAC_DRBG context - * \param len Amount of entropy to grab, in bytes + * The default value is given by the security strength, which depends on the + * hash used. See mbedtls_hmac_drbg_init(). + * + * \param ctx The HMAC_DRBG context. + * \param len The amount of entropy to grab, in bytes. */ void mbedtls_hmac_drbg_set_entropy_len( mbedtls_hmac_drbg_context *ctx, size_t len ); /** - * \brief Set the reseed interval - * (Default: MBEDTLS_HMAC_DRBG_RESEED_INTERVAL) + * \brief Set the reseed interval. * - * \param ctx HMAC_DRBG context - * \param interval Reseed interval + * The reseed interval is the number of calls to mbedtls_hmac_drbg_random() + * or mbedtls_hmac_drbg_random_with_add() after which the entropy function + * is called again. + * + * The default value is #MBEDTLS_HMAC_DRBG_RESEED_INTERVAL. + * + * \param ctx The HMAC_DRBG context. + * \param interval The reseed interval. */ void mbedtls_hmac_drbg_set_reseed_interval( mbedtls_hmac_drbg_context *ctx, int interval ); /** - * \brief HMAC_DRBG update state + * \brief This function updates the state of the HMAC_DRBG context. * - * \param ctx HMAC_DRBG context - * \param additional Additional data to update state with, or NULL - * \param add_len Length of additional data, or 0 + * \param ctx The HMAC_DRBG context. + * \param additional The data to update the state with. + * If this is \p NULL, there is no additional data. + * \param add_len Length of \p additional in bytes. + * Unused if \p additional is null. * * \return \c 0 on success, or an error from the underlying * hash calculation or * MBEDTLS_ERR_PLATFORM_FAULT_DETECTED. - * - * \note Additional data is optional, pass NULL and 0 as second - * third argument if no additional data is being used. */ int mbedtls_hmac_drbg_update_ret( mbedtls_hmac_drbg_context *ctx, const unsigned char *additional, size_t add_len ); /** - * \brief HMAC_DRBG reseeding (extracts data from entropy source) + * \brief This function reseeds the HMAC_DRBG context, that is + * extracts data from the entropy source. * - * \param ctx HMAC_DRBG context - * \param additional Additional data to add to state (Can be NULL) - * \param len Length of additional data + * \param ctx The HMAC_DRBG context. + * \param additional Additional data to add to the state. + * If this is \c NULL, there is no additional data + * and \p len should be \c 0. + * \param len The length of the additional data. + * This must be at most #MBEDTLS_HMAC_DRBG_MAX_INPUT + * and also at most + * #MBEDTLS_HMAC_DRBG_MAX_SEED_INPUT - \p entropy_len + * where \p entropy_len is the entropy length + * (see mbedtls_hmac_drbg_set_entropy_len()). * - * \return 0 if successful, or - * MBEDTLS_ERR_HMAC_DRBG_ENTROPY_SOURCE_FAILED + * \return 0 if successful. + * \return #MBEDTLS_ERR_HMAC_DRBG_ENTROPY_SOURCE_FAILED + * if a call to the entropy function failed. */ int mbedtls_hmac_drbg_reseed( mbedtls_hmac_drbg_context *ctx, const unsigned char *additional, size_t len ); /** - * \brief HMAC_DRBG generate random with additional update input + * \brief This function updates an HMAC_DRBG instance with additional + * data and uses it to generate random data. * - * Note: Automatically reseeds if reseed_counter is reached or PR is enabled. + * \note The function automatically reseeds if the reseed counter is exceeded. * - * \param p_rng HMAC_DRBG context - * \param output Buffer to fill - * \param output_len Length of the buffer - * \param additional Additional data to update with (can be NULL) - * \param add_len Length of additional data (can be 0) + * \param p_rng The HMAC_DRBG context. This must be a pointer to a + * #mbedtls_hmac_drbg_context structure. + * \param output The buffer to fill. + * \param output_len The length of the buffer in bytes. + * This must be at most #MBEDTLS_HMAC_DRBG_MAX_REQUEST. + * \param additional Additional data to update with. + * If this is \p NULL, there is no additional data + * and \p add_len should be \c 0. + * \param add_len The length of the additional data. + * This must be at most #MBEDTLS_HMAC_DRBG_MAX_INPUT. * - * \return 0 if successful, or - * MBEDTLS_ERR_HMAC_DRBG_ENTROPY_SOURCE_FAILED, or - * MBEDTLS_ERR_HMAC_DRBG_REQUEST_TOO_BIG, or - * MBEDTLS_ERR_HMAC_DRBG_INPUT_TOO_BIG, or - * MBEDTLS_ERR_PLATFORM_FAULT_DETECTED. + * \return \c 0 if successful. + * \return #MBEDTLS_ERR_HMAC_DRBG_ENTROPY_SOURCE_FAILED + * if a call to the entropy source failed. + * \return #MBEDTLS_ERR_HMAC_DRBG_REQUEST_TOO_BIG if + * \p output_len > #MBEDTLS_HMAC_DRBG_MAX_REQUEST. + * \return #MBEDTLS_ERR_HMAC_DRBG_INPUT_TOO_BIG if + * \p add_len > #MBEDTLS_HMAC_DRBG_MAX_INPUT. + * \return #MBEDTLS_ERR_PLATFORM_FAULT_DETECTED if + * a logical fault is detected. */ int mbedtls_hmac_drbg_random_with_add( void *p_rng, unsigned char *output, size_t output_len, @@ -247,26 +297,30 @@ int mbedtls_hmac_drbg_random_with_add( void *p_rng, size_t add_len ); /** - * \brief HMAC_DRBG generate random + * \brief This function uses HMAC_DRBG to generate random data. * - * Note: Automatically reseeds if reseed_counter is reached or PR is enabled. + * \note The function automatically reseeds if the reseed counter is exceeded. * - * \param p_rng HMAC_DRBG context - * \param output Buffer to fill - * \param out_len Length of the buffer + * \param p_rng The HMAC_DRBG context. This must be a pointer to a + * #mbedtls_hmac_drbg_context structure. + * \param output The buffer to fill. + * \param out_len The length of the buffer in bytes. + * This must be at most #MBEDTLS_HMAC_DRBG_MAX_REQUEST. * - * \return 0 if successful, or - * MBEDTLS_ERR_HMAC_DRBG_ENTROPY_SOURCE_FAILED, or - * MBEDTLS_ERR_HMAC_DRBG_REQUEST_TOO_BIG, - * MBEDTLS_ERR_HMAC_DRBG_INPUT_TOO_BIG, or - * MBEDTLS_ERR_PLATFORM_FAULT_DETECTED. + * \return \c 0 if successful. + * \return #MBEDTLS_ERR_HMAC_DRBG_ENTROPY_SOURCE_FAILED + * if a call to the entropy source failed. + * \return #MBEDTLS_ERR_HMAC_DRBG_REQUEST_TOO_BIG if + * \p out_len > #MBEDTLS_HMAC_DRBG_MAX_REQUEST. + * \return #MBEDTLS_ERR_PLATFORM_FAULT_DETECTED if + * a logical fault is detected. */ int mbedtls_hmac_drbg_random( void *p_rng, unsigned char *output, size_t out_len ); /** * \brief Free an HMAC_DRBG context * - * \param ctx HMAC_DRBG context to free. + * \param ctx The HMAC_DRBG context to free. */ void mbedtls_hmac_drbg_free( mbedtls_hmac_drbg_context *ctx ); @@ -277,17 +331,16 @@ void mbedtls_hmac_drbg_free( mbedtls_hmac_drbg_context *ctx ); #define MBEDTLS_DEPRECATED #endif /** - * \brief HMAC_DRBG update state + * \brief This function updates the state of the HMAC_DRBG context. * * \deprecated Superseded by mbedtls_hmac_drbg_update_ret() * in 2.16.0. * - * \param ctx HMAC_DRBG context - * \param additional Additional data to update state with, or NULL - * \param add_len Length of additional data, or 0 - * - * \note Additional data is optional, pass NULL and 0 as second - * third argument if no additional data is being used. + * \param ctx The HMAC_DRBG context. + * \param additional The data to update the state with. + * If this is \p NULL, there is no additional data. + * \param add_len Length of \p additional in bytes. + * Unused if \p additional is null. */ MBEDTLS_DEPRECATED void mbedtls_hmac_drbg_update( mbedtls_hmac_drbg_context *ctx, @@ -297,26 +350,31 @@ MBEDTLS_DEPRECATED void mbedtls_hmac_drbg_update( #if defined(MBEDTLS_FS_IO) /** - * \brief Write a seed file + * \brief This function writes a seed file. * - * \param ctx HMAC_DRBG context - * \param path Name of the file + * \param ctx The HMAC_DRBG context. + * \param path The name of the file. * - * \return 0 if successful, 1 on file error, or - * MBEDTLS_ERR_HMAC_DRBG_ENTROPY_SOURCE_FAILED + * \return \c 0 on success. + * \return #MBEDTLS_ERR_HMAC_DRBG_FILE_IO_ERROR on file error. + * \return #MBEDTLS_ERR_HMAC_DRBG_ENTROPY_SOURCE_FAILED on reseed + * failure. */ int mbedtls_hmac_drbg_write_seed_file( mbedtls_hmac_drbg_context *ctx, const char *path ); /** - * \brief Read and update a seed file. Seed is added to this - * instance + * \brief This function reads and updates a seed file. The seed + * is added to this instance. * - * \param ctx HMAC_DRBG context - * \param path Name of the file + * \param ctx The HMAC_DRBG context. + * \param path The name of the file. * - * \return 0 if successful, 1 on file error, - * MBEDTLS_ERR_HMAC_DRBG_ENTROPY_SOURCE_FAILED or - * MBEDTLS_ERR_HMAC_DRBG_INPUT_TOO_BIG + * \return \c 0 on success. + * \return #MBEDTLS_ERR_HMAC_DRBG_FILE_IO_ERROR on file error. + * \return #MBEDTLS_ERR_HMAC_DRBG_ENTROPY_SOURCE_FAILED on + * reseed failure. + * \return #MBEDTLS_ERR_HMAC_DRBG_INPUT_TOO_BIG if the existing + * seed file is too large. */ int mbedtls_hmac_drbg_update_seed_file( mbedtls_hmac_drbg_context *ctx, const char *path ); #endif /* MBEDTLS_FS_IO */ @@ -324,9 +382,10 @@ int mbedtls_hmac_drbg_update_seed_file( mbedtls_hmac_drbg_context *ctx, const ch #if defined(MBEDTLS_SELF_TEST) /** - * \brief Checkup routine + * \brief The HMAC_DRBG Checkup routine. * - * \return 0 if successful, or 1 if the test failed + * \return \c 0 if successful. + * \return \c 1 if the test failed. */ int mbedtls_hmac_drbg_self_test( int verbose ); #endif From ccb38381e82bed21c66f9ca900d2e532f4372fda Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 30 Sep 2019 15:20:52 +0200 Subject: [PATCH 13/95] HMAC_DRBG: improve the documentation of the entropy length --- include/mbedtls/hmac_drbg.h | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/include/mbedtls/hmac_drbg.h b/include/mbedtls/hmac_drbg.h index b5dc829b1..e8f0de005 100644 --- a/include/mbedtls/hmac_drbg.h +++ b/include/mbedtls/hmac_drbg.h @@ -122,6 +122,32 @@ void mbedtls_hmac_drbg_init( mbedtls_hmac_drbg_context *ctx ); * * Set the initial seed and set up the entropy source for future reseeds. * + * A typical choice for the \p f_entropy and \p p_entropy parameters is + * to use the entropy module: + * - \p f_entropy is mbedtls_entropy_func(); + * - \p p_entropy is an instance of ::mbedtls_entropy_context initialized + * with mbedtls_entropy_init() (which registers the platform's default + * entropy sources). + * + * \note By default, the security strength as defined by NIST is: + * - 128 bits if \p md_info is SHA-1; + * - 192 bits if \p md_info is SHA-224; + * - 256 bits if \p md_info is SHA-256, SHA-384 or SHA-512. + * Note that SHA-256 is just as efficient as SHA-224. + * The security strength can be reduced if a smaller + * entropy length is set with + * mbedtls_hmac_drbg_set_entropy_len(). + * + * \note The default entropy length is the security strength + * (converted from bits to bytes). You can override + * it mbedtls_hmac_drbg_set_entropy_len(). + * \p f_entropy is always called with a length that is + * less than or equal to the entropy length. + * + * \note During the initial seeding, this function calls + * the entropy source to obtain a nonce + * whose length is half the entropy length. + * * \param ctx HMAC_DRBG context to be seeded. * \param md_info MD algorithm to use for HMAC_DRBG. * \param f_entropy The entropy callback, taking as arguments the @@ -137,13 +163,7 @@ void mbedtls_hmac_drbg_init( mbedtls_hmac_drbg_context *ctx ); * and also at most * #MBEDTLS_HMAC_DRBG_MAX_SEED_INPUT - \p entropy_len * 3 / 2 * where \p entropy_len is the entropy length - * (see mbedtls_hmac_drbg_set_entropy_len()). - * - * \note The "security strength" as defined by NIST is set to: - * 128 bits if \p md_info is SHA-1, - * 192 bits if \p md_info is SHA-224, - * 256 bits if \p md_info is SHA-256, SHA-384 or SHA-512. - * Note that SHA-256 is just as efficient as SHA-224. + * described above. * * \return 0 if successful. * \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA if \p md_info is @@ -203,7 +223,7 @@ void mbedtls_hmac_drbg_set_prediction_resistance( mbedtls_hmac_drbg_context *ctx * seed or reseed. * * The default value is given by the security strength, which depends on the - * hash used. See mbedtls_hmac_drbg_init(). + * hash used. See the documentation of mbedtls_hmac_drbg_seed() for details. * * \param ctx The HMAC_DRBG context. * \param len The amount of entropy to grab, in bytes. From 9e2543bd4f5b7fa06287e2e057088b55813bdbda Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 30 Sep 2019 15:25:18 +0200 Subject: [PATCH 14/95] Also mention HMAC_DRBG in the changelog entry There were no tricky compliance issues for HMAC_DBRG, unlike CTR_DRBG, but mention it anyway. For CTR_DRBG, summarize the salient issue. --- ChangeLog | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 70488e224..123b9b76f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -73,8 +73,9 @@ Bugfix Changes * Add unit tests for AES-GCM when called through mbedtls_cipher_auth_xxx() from the cipher abstraction layer. Fixes #2198. - * Clarify how the interface of the CTR_DRBG module relates to - NIST SP 800-90A. + * Clarify how the interface of the CTR_DRBG and HMAC modules relates to + NIST SP 800-90A. In particular CTR_DRBG requires an explicit nonce + to achieve a 256-bit strength if MBEDTLS_ENTROPY_FORCE_SHA256 is set. = mbed TLS 2.16.3 branch released 2019-09-06 From 20a3846725ecae00c50334380dec4078fe28227f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 1 Oct 2019 18:30:02 +0200 Subject: [PATCH 15/95] Consistently use \c NULL and \c 0 --- include/mbedtls/ctr_drbg.h | 10 +++++----- include/mbedtls/hmac_drbg.h | 18 +++++++++--------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index f65606eb4..e47b76986 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -232,7 +232,7 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * length of the buffer. * \param p_entropy The entropy context to pass to \p f_entropy. * \param custom Personalization data, that is device-specific - * identifiers. This can be NULL, in which case the + * identifiers. This can be \c NULL, in which case the * personalization data is empty regardless of the value * of \p len. * \param len The length of the personalization data. @@ -330,7 +330,7 @@ void mbedtls_ctr_drbg_set_reseed_interval( mbedtls_ctr_drbg_context *ctx, * extracts data from the entropy source. * * \param ctx The CTR_DRBG context. - * \param additional Additional data to add to the state. Can be NULL. + * \param additional Additional data to add to the state. Can be \c NULL. * \param len The length of the additional data. * This must be less than * #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT - \c entropy_len @@ -348,7 +348,7 @@ int mbedtls_ctr_drbg_reseed( mbedtls_ctr_drbg_context *ctx, * * \param ctx The CTR_DRBG context. * \param additional The data to update the state with. This must not be - * null unless \p add_len is 0. + * \c NULL unless \p add_len is \c 0. * \param add_len Length of \p additional in bytes. This must be at * most #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT. * @@ -372,11 +372,11 @@ int mbedtls_ctr_drbg_update_ret( mbedtls_ctr_drbg_context *ctx, * #mbedtls_ctr_drbg_context structure. * \param output The buffer to fill. * \param output_len The length of the buffer in bytes. - * \param additional Additional data to update. Can be NULL, in which + * \param additional Additional data to update. Can be \c NULL, in which * case the additional data is empty regardless of * the value of \p add_len. * \param add_len The length of the additional data - * if \p additional is non-null. + * if \p additional is not \c NULL. * This must be less than #MBEDTLS_CTR_DRBG_MAX_INPUT * and less than * #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT - \c entropy_len diff --git a/include/mbedtls/hmac_drbg.h b/include/mbedtls/hmac_drbg.h index e8f0de005..fea75584a 100644 --- a/include/mbedtls/hmac_drbg.h +++ b/include/mbedtls/hmac_drbg.h @@ -155,7 +155,7 @@ void mbedtls_hmac_drbg_init( mbedtls_hmac_drbg_context *ctx ); * length of the buffer. * \param p_entropy The entropy context to pass to \p f_entropy. * \param custom Personalization data, that is device-specific - * identifiers. This can be NULL, in which case the + * identifiers. This can be \c NULL, in which case the * personalization data is empty regardless of the value * of \p len. * \param len The length of the personalization data. @@ -165,7 +165,7 @@ void mbedtls_hmac_drbg_init( mbedtls_hmac_drbg_context *ctx ); * where \p entropy_len is the entropy length * described above. * - * \return 0 if successful. + * \return \c 0 if successful. * \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA if \p md_info is * invalid. * \return #MBEDTLS_ERR_MD_ALLOC_FAILED if there was not enough @@ -192,7 +192,7 @@ int mbedtls_hmac_drbg_seed( mbedtls_hmac_drbg_context *ctx, * the additional data. * \param data_len Length of \p data in bytes. * - * \return 0 if successful. or + * \return \c 0 if successful. or * \return #MBEDTLS_ERR_MD_BAD_INPUT_DATA if \p md_info is * invalid. * \return #MBEDTLS_ERR_MD_ALLOC_FAILED if there was not enough @@ -251,9 +251,9 @@ void mbedtls_hmac_drbg_set_reseed_interval( mbedtls_hmac_drbg_context *ctx, * * \param ctx The HMAC_DRBG context. * \param additional The data to update the state with. - * If this is \p NULL, there is no additional data. + * If this is \c NULL, there is no additional data. * \param add_len Length of \p additional in bytes. - * Unused if \p additional is null. + * Unused if \p additional is \c NULL. * * \return \c 0 on success, or an error from the underlying * hash calculation or @@ -277,7 +277,7 @@ int mbedtls_hmac_drbg_update_ret( mbedtls_hmac_drbg_context *ctx, * where \p entropy_len is the entropy length * (see mbedtls_hmac_drbg_set_entropy_len()). * - * \return 0 if successful. + * \return \c 0 if successful. * \return #MBEDTLS_ERR_HMAC_DRBG_ENTROPY_SOURCE_FAILED * if a call to the entropy function failed. */ @@ -296,7 +296,7 @@ int mbedtls_hmac_drbg_reseed( mbedtls_hmac_drbg_context *ctx, * \param output_len The length of the buffer in bytes. * This must be at most #MBEDTLS_HMAC_DRBG_MAX_REQUEST. * \param additional Additional data to update with. - * If this is \p NULL, there is no additional data + * If this is \c NULL, there is no additional data * and \p add_len should be \c 0. * \param add_len The length of the additional data. * This must be at most #MBEDTLS_HMAC_DRBG_MAX_INPUT. @@ -358,9 +358,9 @@ void mbedtls_hmac_drbg_free( mbedtls_hmac_drbg_context *ctx ); * * \param ctx The HMAC_DRBG context. * \param additional The data to update the state with. - * If this is \p NULL, there is no additional data. + * If this is \c NULL, there is no additional data. * \param add_len Length of \p additional in bytes. - * Unused if \p additional is null. + * Unused if \p additional is \c NULL. */ MBEDTLS_DEPRECATED void mbedtls_hmac_drbg_update( mbedtls_hmac_drbg_context *ctx, From 57553fa2f03c92e9f1e20c26e095ecd5bac85315 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 1 Oct 2019 18:31:28 +0200 Subject: [PATCH 16/95] Do note that xxx_drbg_random functions reseed with PR enabled --- include/mbedtls/ctr_drbg.h | 7 +++++-- include/mbedtls/hmac_drbg.h | 6 ++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index e47b76986..3d176288b 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -366,7 +366,8 @@ int mbedtls_ctr_drbg_update_ret( mbedtls_ctr_drbg_context *ctx, * \brief This function updates a CTR_DRBG instance with additional * data and uses it to generate random data. * - * \note The function automatically reseeds if the reseed counter is exceeded. + * This function automatically reseeds if the reseed counter is exceeded + * or prediction resistance is enabled. * * \param p_rng The CTR_DRBG context. This must be a pointer to a * #mbedtls_ctr_drbg_context structure. @@ -394,7 +395,9 @@ int mbedtls_ctr_drbg_random_with_add( void *p_rng, /** * \brief This function uses CTR_DRBG to generate random data. * - * \note The function automatically reseeds if the reseed counter is exceeded. + * This function automatically reseeds if the reseed counter is exceeded + * or prediction resistance is enabled. + * * * \param p_rng The CTR_DRBG context. This must be a pointer to a * #mbedtls_ctr_drbg_context structure. diff --git a/include/mbedtls/hmac_drbg.h b/include/mbedtls/hmac_drbg.h index fea75584a..410eb7acb 100644 --- a/include/mbedtls/hmac_drbg.h +++ b/include/mbedtls/hmac_drbg.h @@ -288,7 +288,8 @@ int mbedtls_hmac_drbg_reseed( mbedtls_hmac_drbg_context *ctx, * \brief This function updates an HMAC_DRBG instance with additional * data and uses it to generate random data. * - * \note The function automatically reseeds if the reseed counter is exceeded. + * This function automatically reseeds if the reseed counter is exceeded + * or prediction resistance is enabled. * * \param p_rng The HMAC_DRBG context. This must be a pointer to a * #mbedtls_hmac_drbg_context structure. @@ -319,7 +320,8 @@ int mbedtls_hmac_drbg_random_with_add( void *p_rng, /** * \brief This function uses HMAC_DRBG to generate random data. * - * \note The function automatically reseeds if the reseed counter is exceeded. + * This function automatically reseeds if the reseed counter is exceeded + * or prediction resistance is enabled. * * \param p_rng The HMAC_DRBG context. This must be a pointer to a * #mbedtls_hmac_drbg_context structure. From beddfdcd7f0de7a0582efab041b5539452090889 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 1 Oct 2019 18:39:45 +0200 Subject: [PATCH 17/95] Use standard terminology to describe the personalization string NIST and many other sources call it a "personalization string", and certainly not "device-specific identifiers" which is actually somewhat misleading since this is just one of many things that might go into a personalization string. --- include/mbedtls/ctr_drbg.h | 11 +++++------ include/mbedtls/hmac_drbg.h | 12 +++++++----- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index 3d176288b..3599e95d2 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -203,7 +203,7 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * \p f_entropy is always called with a buffer size equal to the entropy * length described in the documentation of mbedtls_ctr_drbg_set_entropy_len(). * - * Personalization data can be provided in addition to the more generic + * You can provide a personalization string in addition to the * entropy source, to make this instantiation as unique as possible. * * \note The _seed_material_ value passed to the derivation @@ -231,11 +231,10 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * \p p_entropy context, the buffer to fill, and the * length of the buffer. * \param p_entropy The entropy context to pass to \p f_entropy. - * \param custom Personalization data, that is device-specific - * identifiers. This can be \c NULL, in which case the - * personalization data is empty regardless of the value - * of \p len. - * \param len The length of the personalization data. + * \param custom The personalization string. + * This can be \c NULL, in which case the personalization + * string is empty regardless of the value of \p len. + * \param len The length of the personalization string. * This must be at most * #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT / 2. * diff --git a/include/mbedtls/hmac_drbg.h b/include/mbedtls/hmac_drbg.h index 410eb7acb..88bfbba1a 100644 --- a/include/mbedtls/hmac_drbg.h +++ b/include/mbedtls/hmac_drbg.h @@ -129,6 +129,9 @@ void mbedtls_hmac_drbg_init( mbedtls_hmac_drbg_context *ctx ); * with mbedtls_entropy_init() (which registers the platform's default * entropy sources). * + * You can provide a personalization string in addition to the + * entropy source, to make this instantiation as unique as possible. + * * \note By default, the security strength as defined by NIST is: * - 128 bits if \p md_info is SHA-1; * - 192 bits if \p md_info is SHA-224; @@ -154,11 +157,10 @@ void mbedtls_hmac_drbg_init( mbedtls_hmac_drbg_context *ctx ); * \p p_entropy context, the buffer to fill, and the * length of the buffer. * \param p_entropy The entropy context to pass to \p f_entropy. - * \param custom Personalization data, that is device-specific - * identifiers. This can be \c NULL, in which case the - * personalization data is empty regardless of the value - * of \p len. - * \param len The length of the personalization data. + * \param custom The personalization string. + * This can be \c NULL, in which case the personalization + * string is empty regardless of the value of \p len. + * \param len The length of the personalization string. * This must be at most #MBEDTLS_HMAC_DRBG_MAX_INPUT * and also at most * #MBEDTLS_HMAC_DRBG_MAX_SEED_INPUT - \p entropy_len * 3 / 2 From 56f628ca26206b8901ba0ead3599046c22e0e3d1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 1 Oct 2019 18:41:12 +0200 Subject: [PATCH 18/95] HMAC_DRBG: note that the initial seeding grabs entropy for the nonce --- include/mbedtls/hmac_drbg.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/mbedtls/hmac_drbg.h b/include/mbedtls/hmac_drbg.h index 88bfbba1a..8b4be8756 100644 --- a/include/mbedtls/hmac_drbg.h +++ b/include/mbedtls/hmac_drbg.h @@ -224,6 +224,9 @@ void mbedtls_hmac_drbg_set_prediction_resistance( mbedtls_hmac_drbg_context *ctx * \brief This function sets the amount of entropy grabbed on each * seed or reseed. * + * During the initial seeding, mbedtls_hmac_drbg_seed() additionally grabs + * half this amount to create the nonce. + * * The default value is given by the security strength, which depends on the * hash used. See the documentation of mbedtls_hmac_drbg_seed() for details. * From 7df4b7b3b629341de979322c20ac4762bd213baf Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 2 Oct 2019 18:23:38 +0200 Subject: [PATCH 19/95] CTR_DRBG: make it easier to understand the security strength Explain how MBEDTLS_CTR_DRBG_ENTROPY_LEN is set next to the security strength statement, rather than giving a partial explanation (current setting only) in the documentation of MBEDTLS_CTR_DRBG_ENTROPY_LEN. --- include/mbedtls/ctr_drbg.h | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index 3599e95d2..4f8d1315c 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -24,6 +24,13 @@ * and #MBEDTLS_CTR_DRBG_ENTROPY_LEN is set to 24 or more (which is * always the case unless it is explicitly set to a different value * in `config.h`). + * + * Note that the value of #MBEDTLS_CTR_DRBG_ENTROPY_LEN defaults to: + * - \c 48 if the module #MBEDTLS_SHA512_C is enabled and the symbol + * #MBEDTLS_ENTROPY_FORCE_SHA256 is not enabled at compile time. + * This is the default configuration of the library. + * - \c 32 if the module #MBEDTLS_SHA512_C is disabled at compile time. + * - \c 32 if #MBEDTLS_ENTROPY_FORCE_SHA256 is enabled at compile time. */ /* * Copyright (C) 2006-2019, Arm Limited (or its affiliates), All Rights Reserved @@ -99,7 +106,7 @@ /** The amount of entropy used per seed by default. * * This is 48 bytes because the entropy module uses SHA-512 - * (`MBEDTLS_ENTROPY_FORCE_SHA256` is not set). + * #MBEDTLS_ENTROPY_FORCE_SHA256 is not set). * * \note See mbedtls_ctr_drbg_set_entropy_len() regarding what values are * acceptable. @@ -109,7 +116,7 @@ /** The amount of entropy used per seed by default. * * This is 32 bytes because the entropy module uses SHA-256 - * (the SHA-512 module is disabled or `MBEDTLS_ENTROPY_FORCE_SHA256` is set). + * (the SHA512 module is disabled or #MBEDTLS_ENTROPY_FORCE_SHA256 is set). * * \note See mbedtls_ctr_drbg_set_entropy_len() regarding what values are * acceptable. From 7b674eac64cfa50ac43765cccb84b6f72b3e3291 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 2 Oct 2019 19:01:31 +0200 Subject: [PATCH 20/95] CTR_DRBG: Improve the explanation of security strength Separate the cases that achieve a 128-bit strength and the cases that achieve a 256-bit strength. --- include/mbedtls/ctr_drbg.h | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index 4f8d1315c..892fd620f 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -292,22 +292,24 @@ void mbedtls_ctr_drbg_set_prediction_resistance( mbedtls_ctr_drbg_context *ctx, * Per NIST SP 800-57A table 2, the achievable security * strength is 128 bits if using AES-128 and * 256 bits if using AES-256. - * Therefore, to provide full security, + * + * To achieve 256-bit security, + * you must use AES-256 and * the entropy input must be at least: - * - 24 bytes if using AES-128 and the \p custom - * argument to mbedtls_ctr_drbg_seed() may repeat - * (for example because it is empty, or more generally - * constant); - * - 48 bytes if using AES-256 and the \p custom - * argument to mbedtls_ctr_drbg_seed() may repeat - * (for example because it is empty, or more generally - * constant); - * - 16 bytes if using AES-128 and the \p custom - * argument to mbedtls_ctr_drbg_seed() includes - * a nonce; - * - 32 bytes if using AES-256 and the \p custom - * argument to mbedtls_ctr_drbg_seed() includes - * a nonce. + * - 48 bytes if the \p custom argument to + * mbedtls_ctr_drbg_seed() may repeat (for example + * because it is empty, or more generally constant); + * - 32 bytes if the \p custom argument to + * mbedtls_ctr_drbg_seed() includes a nonce. + * + * To achieve 128-bit security, + * whether AES-128 or AES-256 is used, + * the entropy input must be at least: + * - 24 bytes if the \p custom argument to + * mbedtls_ctr_drbg_seed() may repeat (for example + * because it is empty, or more generally constant); + * - 16 bytes if the \p custom argument to + * mbedtls_ctr_drbg_seed() includes a nonce. * * \param ctx The CTR_DRBG context. * \param len The amount of entropy to grab, in bytes. From 9640403fa04c5fcb669f65e76c8ccc6d546fc4b2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 2 Oct 2019 19:02:13 +0200 Subject: [PATCH 21/95] CTR_DRBG documentation: further wording improvements --- include/mbedtls/ctr_drbg.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index 892fd620f..e62f115c1 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -1,7 +1,8 @@ /** * \file ctr_drbg.h * - * \brief This file contains CTR_DRBG definitions and functions. + * \brief This file contains definitions and functions for the + * CTR_DRBG pseudorandom generator. * * CTR_DRBG is a standardized way of building a PRNG from a block-cipher * in counter mode operation, as defined in NIST SP 800-90A: @@ -410,7 +411,7 @@ int mbedtls_ctr_drbg_random_with_add( void *p_rng, * \param p_rng The CTR_DRBG context. This must be a pointer to a * #mbedtls_ctr_drbg_context structure. * \param output The buffer to fill. - * \param output_len The length of the buffer. + * \param output_len The length of the buffer in bytes. * * \return \c 0 on success. * \return #MBEDTLS_ERR_CTR_DRBG_ENTROPY_SOURCE_FAILED or From 6e36d0b33c56702b065c257767e9556d261a92ab Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 3 Oct 2019 14:20:46 +0200 Subject: [PATCH 22/95] CTR_DRBG: more consistent formatting and wording In particular, don't use #MBEDTLS_xxx on macros that are undefined in some configurations, since this would be typeset with a literal '#'. --- include/mbedtls/ctr_drbg.h | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index e62f115c1..c06241afa 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -12,26 +12,26 @@ * The Mbed TLS implementation of CTR_DRBG uses AES-256 (default) or AES-128 * as the underlying block cipher, with a derivation function. The security * strength is: + * (if \c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is enabled at compile time) * - 256 bits under the default configuration of the library, with AES-256 - * (`MBEDTLS_CTR_DRBG_USE_128_BIT_KEY` not set) and - * with #MBEDTLS_CTR_DRBG_ENTROPY_LEN set to 48 or more. + * and with #MBEDTLS_CTR_DRBG_ENTROPY_LEN set to 48 or more. * - 256 bits if AES-256 is used, #MBEDTLS_CTR_DRBG_ENTROPY_LEN is set * to 32 or more, and the DRBG is initialized with an explicit * nonce in the \c custom parameter to mbedtls_ctr_drbg_seed(). * - 128 bits if AES-256 is used but #MBEDTLS_CTR_DRBG_ENTROPY_LEN is * between 24 and 47 and the DRBG is not initialized with an explicit * nonce (see mbedtls_ctr_drbg_seed()). - * - 128 bits if AES-128 is used (`MBEDTLS_CTR_DRBG_USE_128_BIT_KEY` set) + * - 128 bits if AES-128 is used (\c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY enabled) * and #MBEDTLS_CTR_DRBG_ENTROPY_LEN is set to 24 or more (which is * always the case unless it is explicitly set to a different value - * in `config.h`). + * in config.h). * * Note that the value of #MBEDTLS_CTR_DRBG_ENTROPY_LEN defaults to: - * - \c 48 if the module #MBEDTLS_SHA512_C is enabled and the symbol - * #MBEDTLS_ENTROPY_FORCE_SHA256 is not enabled at compile time. + * - \c 48 if the module \c MBEDTLS_SHA512_C is enabled and the symbol + * \c MBEDTLS_ENTROPY_FORCE_SHA256 is disabled at compile time. * This is the default configuration of the library. - * - \c 32 if the module #MBEDTLS_SHA512_C is disabled at compile time. - * - \c 32 if #MBEDTLS_ENTROPY_FORCE_SHA256 is enabled at compile time. + * - \c 32 if the module \c MBEDTLS_SHA512_C is disabled at compile time. + * - \c 32 if \c MBEDTLS_ENTROPY_FORCE_SHA256 is enabled at compile time. */ /* * Copyright (C) 2006-2019, Arm Limited (or its affiliates), All Rights Reserved @@ -79,14 +79,14 @@ /**< The key size in bytes used by the cipher. * * Compile-time choice: 16 bytes (128 bits) - * because #MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is set. + * because #MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is enabled. */ #else #define MBEDTLS_CTR_DRBG_KEYSIZE 32 /**< The key size in bytes used by the cipher. * * Compile-time choice: 32 bytes (256 bits) - * because `MBEDTLS_CTR_DRBG_USE_128_BIT_KEY` is not set. + * because \c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is disabled. */ #endif @@ -107,10 +107,10 @@ /** The amount of entropy used per seed by default. * * This is 48 bytes because the entropy module uses SHA-512 - * #MBEDTLS_ENTROPY_FORCE_SHA256 is not set). * * \note See mbedtls_ctr_drbg_set_entropy_len() regarding what values are * acceptable. + * (\c MBEDTLS_ENTROPY_FORCE_SHA256 is disabled). */ #define MBEDTLS_CTR_DRBG_ENTROPY_LEN 48 #else From e1dc2de900f197ac1e40654a1b8a8da524def568 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 3 Oct 2019 14:21:14 +0200 Subject: [PATCH 23/95] Move MBEDTLS_CTR_DRBG_USE_128_BIT_KEY to the correct section It's an on/off feature, so it should be listed in version_features. --- include/mbedtls/config.h | 10 ++++++++-- library/version_features.c | 3 +++ programs/ssl/query_config.c | 16 ++++++++-------- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 347a8fa81..507af3d14 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -747,6 +747,13 @@ #define MBEDTLS_CIPHER_PADDING_ZEROS_AND_LEN #define MBEDTLS_CIPHER_PADDING_ZEROS +/** \def MBEDTLS_CTR_DRBG_USE_128_BIT_KEY + * + * Uncomment this macro to use a 128-bit key in the CTR_DRBG module. + * By default, CTR_DRBG uses a 256-bit key. + */ +//#define MBEDTLS_CTR_DRBG_USE_128_BIT_KEY + /** * \def MBEDTLS_ENABLE_WEAK_CIPHERSUITES * @@ -2516,7 +2523,7 @@ * * Enable the CTR_DRBG AES-based random generator. * The CTR_DRBG generator uses AES-256 by default. - * To use AES-128 instead, enable MBEDTLS_CTR_DRBG_USE_128_BIT_KEY below. + * To use AES-128 instead, enable \c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY above. * * Module: library/ctr_drbg.c * Caller: @@ -3434,7 +3441,6 @@ //#define MBEDTLS_CTR_DRBG_MAX_INPUT 256 /**< Maximum number of additional input bytes */ //#define MBEDTLS_CTR_DRBG_MAX_REQUEST 1024 /**< Maximum number of requested bytes per call */ //#define MBEDTLS_CTR_DRBG_MAX_SEED_INPUT 384 /**< Maximum size of (re)seed buffer */ -//#define MBEDTLS_CTR_DRBG_USE_128_BIT_KEY /**< Use 128-bit key for CTR_DRBG - may reduce security (see ctr_drbg.h) */ /* HMAC_DRBG options */ //#define MBEDTLS_HMAC_DRBG_RESEED_INTERVAL 10000 /**< Interval before reseed is performed by default */ diff --git a/library/version_features.c b/library/version_features.c index 6882830b0..d60758ce5 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -306,6 +306,9 @@ static const char *features[] = { #if defined(MBEDTLS_CIPHER_PADDING_ZEROS) "MBEDTLS_CIPHER_PADDING_ZEROS", #endif /* MBEDTLS_CIPHER_PADDING_ZEROS */ +#if defined(MBEDTLS_CTR_DRBG_USE_128_BIT_KEY) + "MBEDTLS_CTR_DRBG_USE_128_BIT_KEY", +#endif /* MBEDTLS_CTR_DRBG_USE_128_BIT_KEY */ #if defined(MBEDTLS_ENABLE_WEAK_CIPHERSUITES) "MBEDTLS_ENABLE_WEAK_CIPHERSUITES", #endif /* MBEDTLS_ENABLE_WEAK_CIPHERSUITES */ diff --git a/programs/ssl/query_config.c b/programs/ssl/query_config.c index 315065be4..8093c0d7f 100644 --- a/programs/ssl/query_config.c +++ b/programs/ssl/query_config.c @@ -858,6 +858,14 @@ int query_config( const char *config ) } #endif /* MBEDTLS_CIPHER_PADDING_ZEROS */ +#if defined(MBEDTLS_CTR_DRBG_USE_128_BIT_KEY) + if( strcmp( "MBEDTLS_CTR_DRBG_USE_128_BIT_KEY", config ) == 0 ) + { + MACRO_EXPANSION_TO_STR( MBEDTLS_CTR_DRBG_USE_128_BIT_KEY ); + return( 0 ); + } +#endif /* MBEDTLS_CTR_DRBG_USE_128_BIT_KEY */ + #if defined(MBEDTLS_ENABLE_WEAK_CIPHERSUITES) if( strcmp( "MBEDTLS_ENABLE_WEAK_CIPHERSUITES", config ) == 0 ) { @@ -2266,14 +2274,6 @@ int query_config( const char *config ) } #endif /* MBEDTLS_CTR_DRBG_MAX_SEED_INPUT */ -#if defined(MBEDTLS_CTR_DRBG_USE_128_BIT_KEY) - if( strcmp( "MBEDTLS_CTR_DRBG_USE_128_BIT_KEY", config ) == 0 ) - { - MACRO_EXPANSION_TO_STR( MBEDTLS_CTR_DRBG_USE_128_BIT_KEY ); - return( 0 ); - } -#endif /* MBEDTLS_CTR_DRBG_USE_128_BIT_KEY */ - #if defined(MBEDTLS_HMAC_DRBG_RESEED_INTERVAL) if( strcmp( "MBEDTLS_HMAC_DRBG_RESEED_INTERVAL", config ) == 0 ) { From 5953660a6a5d065d8728b17091e60b958a843c72 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 3 Oct 2019 14:21:39 +0200 Subject: [PATCH 24/95] Add a note about CTR_DRBG security strength to config.h --- include/mbedtls/config.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 507af3d14..4ee592062 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -2525,6 +2525,10 @@ * The CTR_DRBG generator uses AES-256 by default. * To use AES-128 instead, enable \c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY above. * + * \note To achieve a 256-bit security strength with CTR_DRBG, + * you must use AES-256 *and* use sufficient entropy. + * See ctr_drbg.h for more details. + * * Module: library/ctr_drbg.c * Caller: * From 4c57b20247578b17c0871040623263c044e6bc56 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 3 Oct 2019 15:13:08 +0200 Subject: [PATCH 25/95] mbedtls_ctr_drbg_seed: correct maximum for len --- include/mbedtls/ctr_drbg.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index c06241afa..f1985d77e 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -244,7 +244,8 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * string is empty regardless of the value of \p len. * \param len The length of the personalization string. * This must be at most - * #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT / 2. + * #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT + * - #MBEDTLS_CTR_DRBG_ENTROPY_LEN. * * \return \c 0 on success. * \return #MBEDTLS_ERR_CTR_DRBG_ENTROPY_SOURCE_FAILED on failure. From 5fc111fe69a3d0ff2ca2e541d404eaaf383b8cb6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 3 Oct 2019 14:22:04 +0200 Subject: [PATCH 26/95] mbedtls_ctr_drbg_set_entropy_len() only matters when reseeding The documentation of CTR_DRBG erroneously claimed that mbedtls_ctr_drbg_set_entropy_len() had an impact on the initial seeding. This is in fact not the case: mbedtls_ctr_drbg_seed() forces the initial seeding to grab MBEDTLS_CTR_DRBG_ENTROPY_LEN bytes of entropy. Fix the documentation and rewrite the discussion of the entropy length and the security strength accordingly. --- include/mbedtls/ctr_drbg.h | 106 ++++++++++++++++++++----------------- 1 file changed, 56 insertions(+), 50 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index f1985d77e..766e64701 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -10,9 +10,13 @@ * Bit Generators. * * The Mbed TLS implementation of CTR_DRBG uses AES-256 (default) or AES-128 - * as the underlying block cipher, with a derivation function. The security - * strength is: * (if \c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is enabled at compile time) + * as the underlying block cipher, with a derivation function. + * The initial seeding grabs #MBEDTLS_CTR_DRBG_ENTROPY_LEN bytes of entropy. + * See the documentation of mbedtls_ctr_drbg_seed() for more details. + * + * Based on NIST SP 800-90A §10.2.1 table 3 and NIST SP 800-57 part 1 table 2, + * here are the security strengths achieved in typical configuration: * - 256 bits under the default configuration of the library, with AES-256 * and with #MBEDTLS_CTR_DRBG_ENTROPY_LEN set to 48 or more. * - 256 bits if AES-256 is used, #MBEDTLS_CTR_DRBG_ENTROPY_LEN is set @@ -102,29 +106,31 @@ * \{ */ +/** \def MBEDTLS_CTR_DRBG_ENTROPY_LEN + * + * \brief The amount of entropy used per seed by default, in bytes. + */ #if !defined(MBEDTLS_CTR_DRBG_ENTROPY_LEN) #if defined(MBEDTLS_SHA512_C) && !defined(MBEDTLS_ENTROPY_FORCE_SHA256) -/** The amount of entropy used per seed by default. - * - * This is 48 bytes because the entropy module uses SHA-512 - * - * \note See mbedtls_ctr_drbg_set_entropy_len() regarding what values are - * acceptable. +/** This is 48 bytes because the entropy module uses SHA-512 * (\c MBEDTLS_ENTROPY_FORCE_SHA256 is disabled). */ #define MBEDTLS_CTR_DRBG_ENTROPY_LEN 48 -#else -/** The amount of entropy used per seed by default. - * - * This is 32 bytes because the entropy module uses SHA-256 - * (the SHA512 module is disabled or #MBEDTLS_ENTROPY_FORCE_SHA256 is set). - * - * \note See mbedtls_ctr_drbg_set_entropy_len() regarding what values are - * acceptable. + +#else /* defined(MBEDTLS_SHA512_C) && !defined(MBEDTLS_ENTROPY_FORCE_SHA256) */ + +/** This is 32 bytes because the entropy module uses SHA-256 + * (the SHA512 module is disabled or + * \c MBEDTLS_ENTROPY_FORCE_SHA256 is enabled). */ +#if !defined(MBEDTLS_CTR_DRBG_USE_128_BIT_KEY) +/** \warning To achieve a 256-bit security strength, you must pass a nonce + * to mbedtls_ctr_drbg_seed(). + */ +#endif /* !defined(MBEDTLS_CTR_DRBG_USE_128_BIT_KEY) */ #define MBEDTLS_CTR_DRBG_ENTROPY_LEN 32 -#endif -#endif +#endif /* defined(MBEDTLS_SHA512_C) && !defined(MBEDTLS_ENTROPY_FORCE_SHA256) */ +#endif /* !defined(MBEDTLS_CTR_DRBG_ENTROPY_LEN) */ #if !defined(MBEDTLS_CTR_DRBG_RESEED_INTERVAL) #define MBEDTLS_CTR_DRBG_RESEED_INTERVAL 10000 @@ -209,7 +215,10 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * entropy sources). * * \p f_entropy is always called with a buffer size equal to the entropy - * length described in the documentation of mbedtls_ctr_drbg_set_entropy_len(). + * length. The entropy length is initially #MBEDTLS_CTR_DRBG_ENTROPY_LEN + * and this value is always used for the initial seeding. You can change + * the entropy length for subsequent seeding by calling + * mbedtls_ctr_drbg_set_entropy_len() after this function. * * You can provide a personalization string in addition to the * entropy source, to make this instantiation as unique as possible. @@ -221,9 +230,6 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * calling \p f_entropy and the \p custom string. * The origin of the nonce depends on the value of * the entropy length relative to the security strength. - * See the documentation of - * mbedtls_ctr_drbg_set_entropy_len() for information - * about the entropy length. * - If the entropy length is at least 1.5 times the * security strength then the nonce is taken from the * string obtained with \p f_entropy. @@ -233,7 +239,18 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * you must pass a unique value of \p custom at * each invocation. See SP 800-90A §8.6.7 for more * details. - * + */ +#if MBEDTLS_CTR_DRBG_ENTROPY_LEN < MBEDTLS_CTR_DRBG_KEYSIZE * 3 / 2 +/** \warning When #MBEDTLS_CTR_DRBG_ENTROPY_LEN is less than + * #MBEDTLS_CTR_DRBG_KEYSIZE * 3 / 2, to achieve the + * maximum security strength permitted by CTR_DRBG, + * you must pass a value of \p custom that is a nonce: + * this value must never be repeated in subsequent + * runs of the same application or on a different + * device. + */ +#endif +/** * \param ctx The CTR_DRBG context to seed. * \param f_entropy The entropy callback, taking as arguments the * \p p_entropy context, the buffer to fill, and the @@ -281,37 +298,26 @@ void mbedtls_ctr_drbg_set_prediction_resistance( mbedtls_ctr_drbg_context *ctx, /** * \brief This function sets the amount of entropy grabbed on each - * seed or reseed. + * subsequent reseed. * * The default value is #MBEDTLS_CTR_DRBG_ENTROPY_LEN. * - * \note For compliance with NIST SP 800-90A, the entropy length - * (\p len bytes = \p len * 8 bits) - * must be at least the security strength. - * Furthermore, if the entropy input is used to provide - * the nonce, the entropy length must be 1.5 times - * the security strength. - * Per NIST SP 800-57A table 2, the achievable security - * strength is 128 bits if using AES-128 and - * 256 bits if using AES-256. + * \note mbedtls_ctr_drbg_seed() always sets the entropy length + * to #MBEDTLS_CTR_DRBG_ENTROPY_LEN, so this function + * only has an effect when it is called after + * mbedtls_ctr_drbg_seed(). * - * To achieve 256-bit security, - * you must use AES-256 and - * the entropy input must be at least: - * - 48 bytes if the \p custom argument to - * mbedtls_ctr_drbg_seed() may repeat (for example - * because it is empty, or more generally constant); - * - 32 bytes if the \p custom argument to - * mbedtls_ctr_drbg_seed() includes a nonce. - * - * To achieve 128-bit security, - * whether AES-128 or AES-256 is used, - * the entropy input must be at least: - * - 24 bytes if the \p custom argument to - * mbedtls_ctr_drbg_seed() may repeat (for example - * because it is empty, or more generally constant); - * - 16 bytes if the \p custom argument to - * mbedtls_ctr_drbg_seed() includes a nonce. + * \note The security strength of CTR_DRBG is bounded by the + * entropy length. Thus: + * - When using AES-256 + * (\c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is disabled, + * which is the default), + * \p len must be at least 32 (in bytes) + * to achieve a 256-bit strength. + * - When using AES-128 + * (\c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is enabled) + * \p len must be at least 16 (in bytes) + * to achieve a 128-bit strength. * * \param ctx The CTR_DRBG context. * \param len The amount of entropy to grab, in bytes. From d41a95e2232229811de689693bb910516bf3db0e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 4 Oct 2019 11:52:22 +0200 Subject: [PATCH 27/95] mbedtls_hmac_drbg_set_entropy_len() only matters when reseeding The documentation of HMAC_DRBG erroneously claimed that mbedtls_hmac_drbg_set_entropy_len() had an impact on the initial seeding. This is in fact not the case: mbedtls_hmac_drbg_seed() forces the entropy length to its chosen value. Fix the documentation. --- include/mbedtls/hmac_drbg.h | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/include/mbedtls/hmac_drbg.h b/include/mbedtls/hmac_drbg.h index 8b4be8756..95aac3b23 100644 --- a/include/mbedtls/hmac_drbg.h +++ b/include/mbedtls/hmac_drbg.h @@ -139,13 +139,13 @@ void mbedtls_hmac_drbg_init( mbedtls_hmac_drbg_context *ctx ); * Note that SHA-256 is just as efficient as SHA-224. * The security strength can be reduced if a smaller * entropy length is set with - * mbedtls_hmac_drbg_set_entropy_len(). + * mbedtls_hmac_drbg_set_entropy_len() afterwards. * - * \note The default entropy length is the security strength - * (converted from bits to bytes). You can override - * it mbedtls_hmac_drbg_set_entropy_len(). - * \p f_entropy is always called with a length that is - * less than or equal to the entropy length. + * \note The entropy length for the initial seeding is + * the security strength (converted from bits to bytes). + * You can set a different entropy length for subsequent + * seeding by calling mbedtls_hmac_drbg_set_entropy_len() + * after this function. * * \note During the initial seeding, this function calls * the entropy source to obtain a nonce @@ -156,6 +156,8 @@ void mbedtls_hmac_drbg_init( mbedtls_hmac_drbg_context *ctx ); * \param f_entropy The entropy callback, taking as arguments the * \p p_entropy context, the buffer to fill, and the * length of the buffer. + * \p f_entropy is always called with a length that is + * less than or equal to the entropy length. * \param p_entropy The entropy context to pass to \p f_entropy. * \param custom The personalization string. * This can be \c NULL, in which case the personalization @@ -222,13 +224,14 @@ void mbedtls_hmac_drbg_set_prediction_resistance( mbedtls_hmac_drbg_context *ctx /** * \brief This function sets the amount of entropy grabbed on each - * seed or reseed. + * reseed. * - * During the initial seeding, mbedtls_hmac_drbg_seed() additionally grabs - * half this amount to create the nonce. + * The default value is set by mbedtls_hmac_drbg_seed(). * - * The default value is given by the security strength, which depends on the - * hash used. See the documentation of mbedtls_hmac_drbg_seed() for details. + * \note mbedtls_hmac_drbg_seed() always sets the entropy length + * to the default value based on the chosen MD algorithm, + * so this function only has an effect if it is called + * after mbedtls_hmac_drbg_seed(). * * \param ctx The HMAC_DRBG context. * \param len The amount of entropy to grab, in bytes. From 6b5e60c26cf2f8e47f50c034c2600fdbc6e8111a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 7 Oct 2019 15:57:50 +0200 Subject: [PATCH 28/95] config.pl full: exclude MBEDTLS_CTR_DRBG_USE_128_BIT_KEY This is a variant toggle, not an extra feature, so it should be tested separately. --- scripts/config.pl | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/config.pl b/scripts/config.pl index 2519623cd..89182020a 100755 --- a/scripts/config.pl +++ b/scripts/config.pl @@ -100,6 +100,7 @@ MBEDTLS_TEST_NULL_ENTROPY MBEDTLS_DEPRECATED_REMOVED MBEDTLS_HAVE_SSE2 MBEDTLS_PLATFORM_NO_STD_FUNCTIONS +MBEDTLS_CTR_DRBG_USE_128_BIT_KEY MBEDTLS_ECP_DP_M221_ENABLED MBEDTLS_ECP_DP_M383_ENABLED MBEDTLS_ECP_DP_M511_ENABLED From 3ccb7f18e044556d557405872cd90399a0b5ddd5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 21 Oct 2019 17:11:33 +0200 Subject: [PATCH 29/95] Unify ASan options in make builds Use a common set of options when building with Asan without CMake. --- tests/scripts/all.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index ee6d0a264..6bdf347fc 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -137,6 +137,9 @@ pre_initialize_variables () { export MAKEFLAGS="-j" fi + # CFLAGS and LDFLAGS for Asan builds that don't use CMake + ASAN_CFLAGS='-Werror -Wall -Wextra -fsanitize=address' + # Gather the list of available components. These are the functions # defined in this script whose name starts with "component_". # Parse the script with sed, because in sh there is no way to list @@ -1387,7 +1390,7 @@ component_test_m32_o0 () { # Build once with -O0, to compile out the i386 specific inline assembly msg "build: i386, make, gcc -O0 (ASan build)" # ~ 30s scripts/config.pl full - make CC=gcc PTHREAD=1 CFLAGS='-O0 -Werror -Wall -Wextra -m32 -fsanitize=address' LDFLAGS='-m32' + make CC=gcc PTHREAD=1 CFLAGS="$ASAN_CFLAGS -m32 -O0" LDFLAGS="-m32 $ASAN_CFLAGS" msg "test: i386, make, gcc -O0 (ASan build)" make test @@ -1403,7 +1406,7 @@ component_test_m32_o1 () { # Build again with -O1, to compile in the i386 specific inline assembly msg "build: i386, make, gcc -O1 (ASan build)" # ~ 30s scripts/config.pl full - make CC=gcc PTHREAD=1 CFLAGS='-O1 -Werror -Wall -Wextra -m32 -fsanitize=address' LDFLAGS='-m32' + make CC=gcc PTHREAD=1 CFLAGS="$ASAN_CFLAGS -m32 -O1" LDFLAGS="-m32 $ASAN_CFLAGS" msg "test: i386, make, gcc -O1 (ASan build)" make test From b3e54396faf2eb0341ef6763cc75aba036bc37c4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 21 Oct 2019 20:09:22 +0200 Subject: [PATCH 30/95] Use UBsan in addition to Asan with 'make test' When building with make with the address sanitizer enabled, also enable the undefined behavior sanitizer. --- tests/scripts/all.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 6bdf347fc..3204464c8 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -138,7 +138,7 @@ pre_initialize_variables () { fi # CFLAGS and LDFLAGS for Asan builds that don't use CMake - ASAN_CFLAGS='-Werror -Wall -Wextra -fsanitize=address' + ASAN_CFLAGS='-Werror -Wall -Wextra -fsanitize=address,undefined' # Gather the list of available components. These are the functions # defined in this script whose name starts with "component_". From 6eec4ab323b2ccdf8d348474dc4c4f074b71466a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 21 Oct 2019 19:06:33 +0200 Subject: [PATCH 31/95] Asan make builds: avoid sanitizer recovery Some sanitizers default to displaying an error message and recovering. This could result in a test being recorded as passing despite a complaint from the sanitizer. Turn off sanitizer recovery to avoid this risk. --- tests/scripts/all.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 3204464c8..91b326ec8 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -138,7 +138,7 @@ pre_initialize_variables () { fi # CFLAGS and LDFLAGS for Asan builds that don't use CMake - ASAN_CFLAGS='-Werror -Wall -Wextra -fsanitize=address,undefined' + ASAN_CFLAGS='-Werror -Wall -Wextra -fsanitize=address,undefined -fno-sanitize-recover=all' # Gather the list of available components. These are the functions # defined in this script whose name starts with "component_". From 6e2cb64a97905192bc6a3bc8e412799336695895 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 21 Oct 2019 19:08:07 +0200 Subject: [PATCH 32/95] 'make test' must fail if Asan fails When running 'make test' with GNU make, if a test suite program displays "PASSED", this was automatically counted as a pass. This would in particular count as passing: * A test suite with the substring "PASSED" in a test description. * A test suite where all the test cases succeeded, but the final cleanup failed, in particular if a sanitizer reported a memory leak. Use the test executable's return status instead to determine whether the test suite passed. It's always 0 on PASSED unless the executable's cleanup code fails, and it's never 0 on any failure. Fix ARMmbed/mbed-crypto#303 --- tests/scripts/run-test-suites.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/scripts/run-test-suites.pl b/tests/scripts/run-test-suites.pl index 1c9dc1dfc..d06badd23 100755 --- a/tests/scripts/run-test-suites.pl +++ b/tests/scripts/run-test-suites.pl @@ -93,7 +93,7 @@ for my $suite (@suites) $suite_cases_failed = () = $result =~ /.. FAILED/g; $suite_cases_skipped = () = $result =~ /.. ----/g; - if( $result =~ /PASSED/ ) { + if( $? == 0 ) { print "PASS\n"; if( $verbose > 2 ) { pad_print_center( 72, '-', "Begin $suite" ); From 1d2a9e88c3817469e32a1efb248700f78eea88ba Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 4 Oct 2019 11:47:35 +0200 Subject: [PATCH 33/95] HMAC_DRBG: support set_entropy_len() before seed() mbedtls_hmac_drbg_seed() always set the entropy length to the default, so a call to mbedtls_hmac_drbg_set_entropy_len() before seed() had no effect. Change this to the more intuitive behavior that set_entropy_len() sets the entropy length and seed() respects that and only uses the default entropy length if there was no call to set_entropy_len(). --- include/mbedtls/hmac_drbg.h | 19 ++++++------------- library/hmac_drbg.c | 25 ++++++++++++++----------- 2 files changed, 20 insertions(+), 24 deletions(-) diff --git a/include/mbedtls/hmac_drbg.h b/include/mbedtls/hmac_drbg.h index 95aac3b23..eec05e471 100644 --- a/include/mbedtls/hmac_drbg.h +++ b/include/mbedtls/hmac_drbg.h @@ -139,13 +139,11 @@ void mbedtls_hmac_drbg_init( mbedtls_hmac_drbg_context *ctx ); * Note that SHA-256 is just as efficient as SHA-224. * The security strength can be reduced if a smaller * entropy length is set with - * mbedtls_hmac_drbg_set_entropy_len() afterwards. + * mbedtls_hmac_drbg_set_entropy_len(). * - * \note The entropy length for the initial seeding is - * the security strength (converted from bits to bytes). - * You can set a different entropy length for subsequent - * seeding by calling mbedtls_hmac_drbg_set_entropy_len() - * after this function. + * \note The default entropy length is the security strength + * (converted from bits to bytes). You can override + * it by calling mbedtls_hmac_drbg_set_entropy_len(). * * \note During the initial seeding, this function calls * the entropy source to obtain a nonce @@ -224,14 +222,9 @@ void mbedtls_hmac_drbg_set_prediction_resistance( mbedtls_hmac_drbg_context *ctx /** * \brief This function sets the amount of entropy grabbed on each - * reseed. + * seed or reseed. * - * The default value is set by mbedtls_hmac_drbg_seed(). - * - * \note mbedtls_hmac_drbg_seed() always sets the entropy length - * to the default value based on the chosen MD algorithm, - * so this function only has an effect if it is called - * after mbedtls_hmac_drbg_seed(). + * See the documentation of mbedtls_hmac_drbg_seed() for the default value. * * \param ctx The HMAC_DRBG context. * \param len The amount of entropy to grab, in bytes. diff --git a/library/hmac_drbg.c b/library/hmac_drbg.c index 82703b0a3..dd895be42 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -320,16 +320,19 @@ int mbedtls_hmac_drbg_seed( mbedtls_hmac_drbg_context *ctx, ctx->reseed_interval = MBEDTLS_HMAC_DRBG_RESEED_INTERVAL; - /* - * See SP800-57 5.6.1 (p. 65-66) for the security strength provided by - * each hash function, then according to SP800-90A rev1 10.1 table 2, - * min_entropy_len (in bits) is security_strength. - * - * (This also matches the sizes used in the NIST test vectors.) - */ - ctx->entropy_len = md_size <= 20 ? 16 : /* 160-bits hash -> 128 bits */ - md_size <= 28 ? 24 : /* 224-bits hash -> 192 bits */ - 32; /* better (256+) -> 256 bits */ + if( ctx->entropy_len == 0 ) + { + /* + * See SP800-57 5.6.1 (p. 65-66) for the security strength provided by + * each hash function, then according to SP800-90A rev1 10.1 table 2, + * min_entropy_len (in bits) is security_strength. + * + * (This also matches the sizes used in the NIST test vectors.) + */ + ctx->entropy_len = md_size <= 20 ? 16 : /* 160-bits hash -> 128 bits */ + md_size <= 28 ? 24 : /* 224-bits hash -> 192 bits */ + 32; /* better (256+) -> 256 bits */ + } if( ( ret = hmac_drbg_reseed_core( ctx, custom, len, HMAC_NONCE_YES ) ) != 0 ) { @@ -349,7 +352,7 @@ void mbedtls_hmac_drbg_set_prediction_resistance( mbedtls_hmac_drbg_context *ctx } /* - * Set entropy length grabbed for reseeds + * Set entropy length grabbed for seeding */ void mbedtls_hmac_drbg_set_entropy_len( mbedtls_hmac_drbg_context *ctx, size_t len ) { From f0bf757f9cbfe719463e67b030442eea8299ee38 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 2 Oct 2019 20:31:54 +0200 Subject: [PATCH 34/95] CTR_DRBG: Don't use functions before they're defined Move the definitions of mbedtls_ctr_drbg_seed_entropy_len() and mbedtls_ctr_drbg_seed() to after they are used. This makes the code easier to read and to maintain. --- library/ctr_drbg.c | 124 ++++++++++++++++++++++----------------------- 1 file changed, 62 insertions(+), 62 deletions(-) diff --git a/library/ctr_drbg.c b/library/ctr_drbg.c index 069303fdc..9222f4ef7 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -62,68 +62,6 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ) #endif } -/* - * Non-public function wrapped by mbedtls_ctr_drbg_seed(). Necessary to allow - * NIST tests to succeed (which require known length fixed entropy) - */ -/* CTR_DRBG_Instantiate with derivation function (SP 800-90A §10.2.1.3.2) - * mbedtls_ctr_drbg_seed_entropy_len(ctx, f_entropy, p_entropy, - * custom, len, entropy_len) - * implements - * CTR_DRBG_Instantiate(entropy_input, nonce, personalization_string, - * security_strength) -> initial_working_state - * with inputs - * custom[:len] = nonce || personalization_string - * where entropy_input comes from f_entropy for entropy_len bytes - * and with outputs - * ctx = initial_working_state - */ -int mbedtls_ctr_drbg_seed_entropy_len( - mbedtls_ctr_drbg_context *ctx, - int (*f_entropy)(void *, unsigned char *, size_t), - void *p_entropy, - const unsigned char *custom, - size_t len, - size_t entropy_len ) -{ - int ret; - unsigned char key[MBEDTLS_CTR_DRBG_KEYSIZE]; - - mbedtls_platform_memset( key, 0, MBEDTLS_CTR_DRBG_KEYSIZE ); - - mbedtls_aes_init( &ctx->aes_ctx ); - - ctx->f_entropy = f_entropy; - ctx->p_entropy = p_entropy; - - ctx->entropy_len = entropy_len; - ctx->reseed_interval = MBEDTLS_CTR_DRBG_RESEED_INTERVAL; - - /* - * Initialize with an empty key - */ - if( ( ret = mbedtls_aes_setkey_enc( &ctx->aes_ctx, key, MBEDTLS_CTR_DRBG_KEYBITS ) ) != 0 ) - { - return( ret ); - } - - if( ( ret = mbedtls_ctr_drbg_reseed( ctx, custom, len ) ) != 0 ) - { - return( ret ); - } - return( 0 ); -} - -int mbedtls_ctr_drbg_seed( mbedtls_ctr_drbg_context *ctx, - int (*f_entropy)(void *, unsigned char *, size_t), - void *p_entropy, - const unsigned char *custom, - size_t len ) -{ - return( mbedtls_ctr_drbg_seed_entropy_len( ctx, f_entropy, p_entropy, custom, len, - MBEDTLS_CTR_DRBG_ENTROPY_LEN ) ); -} - void mbedtls_ctr_drbg_free( mbedtls_ctr_drbg_context *ctx ) { if( ctx == NULL ) @@ -427,6 +365,68 @@ exit: return( ret ); } +/* + * Non-public function wrapped by mbedtls_ctr_drbg_seed(). Necessary to allow + * NIST tests to succeed (which require known length fixed entropy) + */ +/* CTR_DRBG_Instantiate with derivation function (SP 800-90A §10.2.1.3.2) + * mbedtls_ctr_drbg_seed_entropy_len(ctx, f_entropy, p_entropy, + * custom, len, entropy_len) + * implements + * CTR_DRBG_Instantiate(entropy_input, nonce, personalization_string, + * security_strength) -> initial_working_state + * with inputs + * custom[:len] = nonce || personalization_string + * where entropy_input comes from f_entropy for entropy_len bytes + * and with outputs + * ctx = initial_working_state + */ +int mbedtls_ctr_drbg_seed_entropy_len( + mbedtls_ctr_drbg_context *ctx, + int (*f_entropy)(void *, unsigned char *, size_t), + void *p_entropy, + const unsigned char *custom, + size_t len, + size_t entropy_len ) +{ + int ret; + unsigned char key[MBEDTLS_CTR_DRBG_KEYSIZE]; + + memset( key, 0, MBEDTLS_CTR_DRBG_KEYSIZE ); + + mbedtls_aes_init( &ctx->aes_ctx ); + + ctx->f_entropy = f_entropy; + ctx->p_entropy = p_entropy; + + ctx->entropy_len = entropy_len; + ctx->reseed_interval = MBEDTLS_CTR_DRBG_RESEED_INTERVAL; + + /* + * Initialize with an empty key + */ + if( ( ret = mbedtls_aes_setkey_enc( &ctx->aes_ctx, key, MBEDTLS_CTR_DRBG_KEYBITS ) ) != 0 ) + { + return( ret ); + } + + if( ( ret = mbedtls_ctr_drbg_reseed( ctx, custom, len ) ) != 0 ) + { + return( ret ); + } + return( 0 ); +} + +int mbedtls_ctr_drbg_seed( mbedtls_ctr_drbg_context *ctx, + int (*f_entropy)(void *, unsigned char *, size_t), + void *p_entropy, + const unsigned char *custom, + size_t len ) +{ + return( mbedtls_ctr_drbg_seed_entropy_len( ctx, f_entropy, p_entropy, custom, len, + MBEDTLS_CTR_DRBG_ENTROPY_LEN ) ); +} + /* CTR_DRBG_Generate with derivation function (SP 800-90A §10.2.1.5.2) * mbedtls_ctr_drbg_random_with_add(ctx, output, output_len, additional, add_len) * implements From 20dbfb99386a8802c8e30117191d93ec158cccd8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 4 Oct 2019 12:15:55 +0200 Subject: [PATCH 35/95] CTR_DRBG: support set_entropy_len() before seed() mbedtls_ctr_drbg_seed() always set the entropy length to the default, so a call to mbedtls_ctr_drbg_set_entropy_len() before seed() had no effect. Change this to the more intuitive behavior that set_entropy_len() sets the entropy length and seed() respects that and only uses the default entropy length if there was no call to set_entropy_len(). The former test-only function mbedtls_ctr_drbg_seed_entropy_len() is no longer used, but keep it for strict ABI compatibility. --- include/mbedtls/ctr_drbg.h | 16 +++---- library/ctr_drbg.c | 53 ++++++++++++----------- tests/suites/test_suite_ctr_drbg.function | 6 +-- 3 files changed, 35 insertions(+), 40 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index 766e64701..c14507ff6 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -214,11 +214,8 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * with mbedtls_entropy_init() (which registers the platform's default * entropy sources). * - * \p f_entropy is always called with a buffer size equal to the entropy - * length. The entropy length is initially #MBEDTLS_CTR_DRBG_ENTROPY_LEN - * and this value is always used for the initial seeding. You can change - * the entropy length for subsequent seeding by calling - * mbedtls_ctr_drbg_set_entropy_len() after this function. + * The entropy length is #MBEDTLS_CTR_DRBG_ENTROPY_LEN by default. + * You can override it by calling mbedtls_ctr_drbg_set_entropy_len(). * * You can provide a personalization string in addition to the * entropy source, to make this instantiation as unique as possible. @@ -255,6 +252,8 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); * \param f_entropy The entropy callback, taking as arguments the * \p p_entropy context, the buffer to fill, and the * length of the buffer. + * \p f_entropy is always called with a buffer size + * equal to the entropy length. * \param p_entropy The entropy context to pass to \p f_entropy. * \param custom The personalization string. * This can be \c NULL, in which case the personalization @@ -298,15 +297,10 @@ void mbedtls_ctr_drbg_set_prediction_resistance( mbedtls_ctr_drbg_context *ctx, /** * \brief This function sets the amount of entropy grabbed on each - * subsequent reseed. + * seed or reseed. * * The default value is #MBEDTLS_CTR_DRBG_ENTROPY_LEN. * - * \note mbedtls_ctr_drbg_seed() always sets the entropy length - * to #MBEDTLS_CTR_DRBG_ENTROPY_LEN, so this function - * only has an effect when it is called after - * mbedtls_ctr_drbg_seed(). - * * \note The security strength of CTR_DRBG is bounded by the * entropy length. Thus: * - When using AES-256 diff --git a/library/ctr_drbg.c b/library/ctr_drbg.c index 9222f4ef7..4e4705825 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -365,29 +365,22 @@ exit: return( ret ); } -/* - * Non-public function wrapped by mbedtls_ctr_drbg_seed(). Necessary to allow - * NIST tests to succeed (which require known length fixed entropy) - */ /* CTR_DRBG_Instantiate with derivation function (SP 800-90A §10.2.1.3.2) - * mbedtls_ctr_drbg_seed_entropy_len(ctx, f_entropy, p_entropy, - * custom, len, entropy_len) + * mbedtls_ctr_drbg_seed(ctx, f_entropy, p_entropy, custom, len) * implements * CTR_DRBG_Instantiate(entropy_input, nonce, personalization_string, * security_strength) -> initial_working_state * with inputs * custom[:len] = nonce || personalization_string - * where entropy_input comes from f_entropy for entropy_len bytes + * where entropy_input comes from f_entropy for ctx->entropy_len bytes * and with outputs * ctx = initial_working_state */ -int mbedtls_ctr_drbg_seed_entropy_len( - mbedtls_ctr_drbg_context *ctx, - int (*f_entropy)(void *, unsigned char *, size_t), - void *p_entropy, - const unsigned char *custom, - size_t len, - size_t entropy_len ) +int mbedtls_ctr_drbg_seed( mbedtls_ctr_drbg_context *ctx, + int (*f_entropy)(void *, unsigned char *, size_t), + void *p_entropy, + const unsigned char *custom, + size_t len ) { int ret; unsigned char key[MBEDTLS_CTR_DRBG_KEYSIZE]; @@ -399,7 +392,8 @@ int mbedtls_ctr_drbg_seed_entropy_len( ctx->f_entropy = f_entropy; ctx->p_entropy = p_entropy; - ctx->entropy_len = entropy_len; + if( ctx->entropy_len == 0 ) + ctx->entropy_len = MBEDTLS_CTR_DRBG_ENTROPY_LEN; ctx->reseed_interval = MBEDTLS_CTR_DRBG_RESEED_INTERVAL; /* @@ -417,14 +411,15 @@ int mbedtls_ctr_drbg_seed_entropy_len( return( 0 ); } -int mbedtls_ctr_drbg_seed( mbedtls_ctr_drbg_context *ctx, - int (*f_entropy)(void *, unsigned char *, size_t), - void *p_entropy, - const unsigned char *custom, - size_t len ) +/* Backward compatibility wrapper */ +int mbedtls_ctr_drbg_seed_entropy_len( + mbedtls_ctr_drbg_context *ctx, + int (*f_entropy)(void *, unsigned char *, size_t), void *p_entropy, + const unsigned char *custom, size_t len, + size_t entropy_len ) { - return( mbedtls_ctr_drbg_seed_entropy_len( ctx, f_entropy, p_entropy, custom, len, - MBEDTLS_CTR_DRBG_ENTROPY_LEN ) ); + mbedtls_ctr_drbg_set_entropy_len( ctx, entropy_len ); + return( mbedtls_ctr_drbg_seed( ctx, f_entropy, p_entropy, custom, len ) ); } /* CTR_DRBG_Generate with derivation function (SP 800-90A §10.2.1.5.2) @@ -678,8 +673,11 @@ int mbedtls_ctr_drbg_self_test( int verbose ) mbedtls_printf( " CTR_DRBG (PR = TRUE) : " ); test_offset = 0; - CHK( mbedtls_ctr_drbg_seed_entropy_len( &ctx, ctr_drbg_self_test_entropy, - (void *) entropy_source_pr, nonce_pers_pr, 16, 32 ) ); + mbedtls_ctr_drbg_set_entropy_len( &ctx, 32 ); + CHK( mbedtls_ctr_drbg_seed( &ctx, + ctr_drbg_self_test_entropy, + (void *) entropy_source_pr, + nonce_pers_pr, 16 ) ); mbedtls_ctr_drbg_set_prediction_resistance( &ctx, MBEDTLS_CTR_DRBG_PR_ON ); CHK( mbedtls_ctr_drbg_random( &ctx, buf, MBEDTLS_CTR_DRBG_BLOCKSIZE ) ); CHK( mbedtls_ctr_drbg_random( &ctx, buf, MBEDTLS_CTR_DRBG_BLOCKSIZE ) ); @@ -699,8 +697,11 @@ int mbedtls_ctr_drbg_self_test( int verbose ) mbedtls_ctr_drbg_init( &ctx ); test_offset = 0; - CHK( mbedtls_ctr_drbg_seed_entropy_len( &ctx, ctr_drbg_self_test_entropy, - (void *) entropy_source_nopr, nonce_pers_nopr, 16, 32 ) ); + mbedtls_ctr_drbg_set_entropy_len( &ctx, 32 ); + CHK( mbedtls_ctr_drbg_seed( &ctx, + ctr_drbg_self_test_entropy, + (void *) entropy_source_nopr, + nonce_pers_nopr, 16 ) ); CHK( mbedtls_ctr_drbg_random( &ctx, buf, 16 ) ); CHK( mbedtls_ctr_drbg_reseed( &ctx, NULL, 0 ) ); CHK( mbedtls_ctr_drbg_random( &ctx, buf, 16 ) ); diff --git a/tests/suites/test_suite_ctr_drbg.function b/tests/suites/test_suite_ctr_drbg.function index 4a97826f6..01050d92d 100644 --- a/tests/suites/test_suite_ctr_drbg.function +++ b/tests/suites/test_suite_ctr_drbg.function @@ -44,11 +44,11 @@ static void ctr_drbg_validate_internal( int reseed_mode, data_t * nonce, /* CTR_DRBG_Instantiate(entropy[:entropy->len], nonce, perso, ) * where nonce||perso = nonce[nonce->len] */ - TEST_ASSERT( mbedtls_ctr_drbg_seed_entropy_len( + mbedtls_ctr_drbg_set_entropy_len( &ctx, entropy_chunk_len ); + TEST_ASSERT( mbedtls_ctr_drbg_seed( &ctx, mbedtls_test_entropy_func, entropy->x, - nonce->x, nonce->len, - entropy_chunk_len ) == 0 ); + nonce->x, nonce->len ) == 0 ); if( reseed_mode == RESEED_ALWAYS ) mbedtls_ctr_drbg_set_prediction_resistance( &ctx, From bb3d55665e111eb85202a13d90dae8268b468f0d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 18 Oct 2019 16:40:10 +0200 Subject: [PATCH 36/95] Changelog entry for xxx_drbg_set_entropy_len before xxx_drbg_seed --- ChangeLog | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ChangeLog b/ChangeLog index 123b9b76f..125e5b69d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -69,6 +69,10 @@ Bugfix * Remove redundant line for getting the bitlen of a bignum, since the variable holding the returned value is overwritten a line after. Found by irwir in #2377. + * Support mbedtls_hmac_drbg_set_entropy_len() and + mbedtls_ctr_drbg_set_entropy_len() before the DRBG is seeded. Before, + the initial seeding always reset the entropy length to the compile-time + default. Changes * Add unit tests for AES-GCM when called through mbedtls_cipher_auth_xxx() From 6bd8c0ae2ac418b0c157998dac42da53634c36ef Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 17 Oct 2019 10:18:51 +0100 Subject: [PATCH 37/95] ECDSA: Fix side channel vulnerability The blinding applied to the scalar before modular inversion is inadequate. Bignum is not constant time/constant trace, side channel attacks can retrieve the blinded value, factor it (it is smaller than RSA keys and not guaranteed to have only large prime factors). Then the key can be recovered by brute force. Reducing the blinded value makes factoring useless because the adversary can only recover pk*t+z*N instead of pk*t. --- library/ecdsa.c | 1 + 1 file changed, 1 insertion(+) diff --git a/library/ecdsa.c b/library/ecdsa.c index a9734f6f6..657778594 100644 --- a/library/ecdsa.c +++ b/library/ecdsa.c @@ -363,6 +363,7 @@ modn: MBEDTLS_MPI_CHK( mbedtls_mpi_add_mpi( &e, &e, s ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( &e, &e, &t ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( pk, pk, &t ) ); + MBEDTLS_MPI_CHK( mbedtls_mpi_mod_mpi( pk, pk, &grp->N ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_inv_mod( s, pk, &grp->N ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_mul_mpi( s, s, &e ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_mod_mpi( s, s, &grp->N ) ); From d69ae8c21dd7041c04261d5e7148f0065df2d886 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 25 Oct 2019 08:53:01 +0100 Subject: [PATCH 38/95] Add ChangeLog entry --- ChangeLog | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ChangeLog b/ChangeLog index 125e5b69d..95c0d177d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -65,6 +65,13 @@ Changes = mbed TLS 2.16.x branch released xxxx-xx-xx +Security + * Fix side channel vulnerability in ECDSA. Our bignum implementation is not + constant time/constant trace, so side channel attacks can retrieve the + blinded value, factor it (as it is smaller than RSA keys and not guaranteed + to have only large prime factors), and then, by brute force, recover the + key. Reported by Alejandro Cabrera Aldaya and Billy Brumley. + Bugfix * Remove redundant line for getting the bitlen of a bignum, since the variable holding the returned value is overwritten a line after. From 216040d46f23797ef684152dc4f51160e0fe3edd Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 28 Oct 2019 17:28:46 +0100 Subject: [PATCH 39/95] Fix CTR_DRBG benchmark You can't reuse a CTR_DRBG context without free()ing it and re-init()ing. This generally happened to work, but was never guaranteed. It could have failed with alternative implementations of the AES module because mbedtls_ctr_drbg_seed() calls mbedtls_aes_init() on a context which is already initialized if mbedtls_ctr_drbg_seed() hasn't been called before, plausibly causing a memory leak. Calling free() and seed() with no intervening init fails when MBEDTLS_THREADING_C is enabled and all-bits-zero is not a valid mutex representation. So add the missing free() and init(). --- programs/test/benchmark.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/programs/test/benchmark.c b/programs/test/benchmark.c index 9c6aafb4f..71bee4737 100644 --- a/programs/test/benchmark.c +++ b/programs/test/benchmark.c @@ -686,12 +686,13 @@ int main( int argc, char *argv[] ) mbedtls_ctr_drbg_context ctr_drbg; mbedtls_ctr_drbg_init( &ctr_drbg ); - if( mbedtls_ctr_drbg_seed( &ctr_drbg, myrand, NULL, NULL, 0 ) != 0 ) mbedtls_exit(1); TIME_AND_TSC( "CTR_DRBG (NOPR)", mbedtls_ctr_drbg_random( &ctr_drbg, buf, BUFSIZE ) ); + mbedtls_ctr_drbg_free( &ctr_drbg ); + mbedtls_ctr_drbg_init( &ctr_drbg ); if( mbedtls_ctr_drbg_seed( &ctr_drbg, myrand, NULL, NULL, 0 ) != 0 ) mbedtls_exit(1); mbedtls_ctr_drbg_set_prediction_resistance( &ctr_drbg, MBEDTLS_CTR_DRBG_PR_ON ); From a5e2d86c3f05ee56125e6347d74b4527fe823e58 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 28 Oct 2019 17:33:07 +0100 Subject: [PATCH 40/95] Note that mbedtls_ctr_drbg_seed() must not be called twice You can't reuse a CTR_DRBG context without free()ing it and re-init()ing it. This generally happened to work, but was never guaranteed. It could have failed with alternative implementations of the AES module because mbedtls_ctr_drbg_seed() calls mbedtls_aes_init() on a context which is already initialized if mbedtls_ctr_drbg_seed() hasn't been called before, plausibly causing a memory leak. Calling free() and seed() with no intervening init fails when MBEDTLS_THREADING_C is enabled and all-bits-zero is not a valid mutex representation. --- include/mbedtls/ctr_drbg.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index c14507ff6..e0b5ed9c9 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -249,6 +249,13 @@ void mbedtls_ctr_drbg_init( mbedtls_ctr_drbg_context *ctx ); #endif /** * \param ctx The CTR_DRBG context to seed. + * It must have been initialized with + * mbedtls_ctr_drbg_init(). + * After a successful call to mbedtls_ctr_drbg_seed(), + * you may not call mbedtls_ctr_drbg_seed() again on + * the same context unless you call + * mbedtls_ctr_drbg_free() and mbedtls_ctr_drbg_init() + * again first. * \param f_entropy The entropy callback, taking as arguments the * \p p_entropy context, the buffer to fill, and the * length of the buffer. From c514ce474a964868ff703d8f325305360f1f0f7c Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 5 Sep 2019 14:47:19 +0100 Subject: [PATCH 41/95] Add new, constant time mpi comparison --- include/mbedtls/bignum.h | 19 +++++++++ library/bignum.c | 90 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index 1c8607264..b57f2a17f 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -559,6 +559,25 @@ int mbedtls_mpi_cmp_abs( const mbedtls_mpi *X, const mbedtls_mpi *Y ); */ int mbedtls_mpi_cmp_mpi( const mbedtls_mpi *X, const mbedtls_mpi *Y ); +/** + * \brief Compare two MPIs in constant time. + * + * \param X The left-hand MPI. This must point to an initialized MPI + * with the same allocated length as Y. + * \param Y The right-hand MPI. This must point to an initialized MPI + * with the same allocated length as X. + * \param ret The result of the comparison: + * \c 1 if \p X is greater than \p Y. + * \c -1 if \p X is lesser than \p Y. + * \c 0 if \p X is equal to \p Y. + * + * \return 0 on success. + * \return MBEDTLS_ERR_MPI_BAD_INPUT_DATA if the allocated length of + * the two input MPIs is not the same. + */ +int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, + int *ret ); + /** * \brief Compare an MPI with an integer. * diff --git a/library/bignum.c b/library/bignum.c index 842b38b8c..a91aba89c 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1071,6 +1071,96 @@ int mbedtls_mpi_cmp_mpi( const mbedtls_mpi *X, const mbedtls_mpi *Y ) return( 0 ); } +static int ct_lt_mpi_uint( const mbedtls_mpi_uint x, const mbedtls_mpi_uint y ) +{ + mbedtls_mpi_uint ret; + mbedtls_mpi_uint cond; + + /* + * Check if the most significant bits (MSB) of the operands are different. + */ + cond = ( x ^ y ); + /* + * If the MSB are the same then the difference x-y will be negative (and + * have its MSB set to 1 during conversion to unsigned) if and only if x> ( sizeof( mbedtls_mpi_uint ) * 8 - 1 ); + + return ret; +} + +/* + * Compare signed values in constant time + */ +int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, + int *ret ) +{ + size_t i; + unsigned int cond, done; + + MPI_VALIDATE_RET( X != NULL ); + MPI_VALIDATE_RET( Y != NULL ); + MPI_VALIDATE_RET( ret != NULL ); + + if( X->n != Y->n ) + return MBEDTLS_ERR_MPI_BAD_INPUT_DATA; + + /* + * if( X->s > 0 && Y->s < 0 ) + * { + * *ret = 1; + * done = 1; + * } + * else if( Y->s > 0 && X->s < 0 ) + * { + * *ret = -1; + * done = 1; + * } + */ + unsigned int sign_X = X->s; + unsigned int sign_Y = Y->s; + cond = ( ( sign_X ^ sign_Y ) >> ( sizeof( unsigned int ) * 8 - 1 ) ); + *ret = cond * X->s; + done = cond; + + for( i = X->n; i > 0; i-- ) + { + /* + * if( ( X->p[i - 1] > Y->p[i - 1] ) && !done ) + * { + * done = 1; + * *ret = X->s; + * } + */ + cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ); + *ret |= ( cond * ( 1 - done ) ) * X->s; + done |= cond * ( 1 - done ); + + /* + * if( ( X->p[i - 1] < Y->p[i - 1] ) && !done ) + * { + * done = 1; + * *ret = -X->s; + * } + */ + cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ); + *ret |= ( cond * ( 1 - done ) ) * -X->s; + done |= cond * ( 1 - done ); + + } + + return( 0 ); +} + /* * Compare signed values */ From 7ce3a25316b86c0f291f8ad7bfc39d453a3d2791 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 11 Sep 2019 16:07:14 +0100 Subject: [PATCH 42/95] Add tests to constant time mpi comparison --- tests/suites/test_suite_mpi.data | 33 ++++++++++++++++++++++++++++ tests/suites/test_suite_mpi.function | 23 +++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 425e93ad2..f9f51ac27 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -163,6 +163,39 @@ mbedtls_mpi_cmp_mpi:10:"2":10:"-3":1 Base test mbedtls_mpi_cmp_mpi (Mixed values) #6 mbedtls_mpi_cmp_mpi:10:"-2":10:"31231231289798":-1 +Base test mbedtls_mpi_cmp_mpi_ct #1 +mbedtls_mpi_cmp_mpi_ct:1:10:"693":1:10:"693":0:0 + +Base test mbedtls_mpi_cmp_mpi_ct #2 +mbedtls_mpi_cmp_mpi_ct:1:10:"693":1:10:"692":1:0 + +Base test mbedtls_mpi_cmp_mpi_ct #3 +mbedtls_mpi_cmp_mpi_ct:1:10:"693":1:10:"694":-1:0 + +Base test mbedtls_mpi_cmp_mpi_ct (Negative values) #1 +mbedtls_mpi_cmp_mpi_ct:1:10:"-2":1:10:"-2":0:0 + +Base test mbedtls_mpi_cmp_mpi_ct (Negative values) #2 +mbedtls_mpi_cmp_mpi_ct:1:10:"-2":1:10:"-3":1:0 + +Base test mbedtls_mpi_cmp_mpi_ct (Negative values) #3 +mbedtls_mpi_cmp_mpi_ct:1:10:"-2":1:10:"-1":-1:0 + +Base test mbedtls_mpi_cmp_mpi_ct (Mixed values) #4 +mbedtls_mpi_cmp_mpi_ct:1:10:"-3":1:10:"2":-1:0 + +Base test mbedtls_mpi_cmp_mpi_ct (Mixed values) #5 +mbedtls_mpi_cmp_mpi_ct:1:10:"2":1:10:"-3":1:0 + +Base test mbedtls_mpi_cmp_mpi_ct (Mixed values) #6 +mbedtls_mpi_cmp_mpi_ct:2:10:"-2":2:10:"31231231289798":-1:0 + +Base test mbedtls_mpi_cmp_mpi_ct (X is longer in storage) #7 +mbedtls_mpi_cmp_mpi_ct:3:10:"693":2:10:"693":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA + +Base test mbedtls_mpi_cmp_mpi_ct (Y is longer in storage) #8 +mbedtls_mpi_cmp_mpi_ct:3:10:"693":4:10:"693":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA + Base test mbedtls_mpi_cmp_abs #1 mbedtls_mpi_cmp_abs:10:"693":10:"693":0 diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index f982385e1..718b165e1 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -537,6 +537,29 @@ exit: } /* END_CASE */ +/* BEGIN_CASE */ +void mbedtls_mpi_cmp_mpi_ct( int size_X, int radix_X, char * input_X, int size_Y, + int radix_Y, char * input_Y, int input_ret, int input_err ) +{ + int ret; + mbedtls_mpi X, Y; + mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); + + TEST_ASSERT( mbedtls_mpi_read_string( &X, radix_X, input_X ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_string( &Y, radix_Y, input_Y ) == 0 ); + + mbedtls_mpi_grow( &X, size_X ); + mbedtls_mpi_grow( &Y, size_Y ); + + TEST_ASSERT( mbedtls_mpi_cmp_mpi_ct( &X, &Y, &ret ) == input_err ); + if( input_err == 0 ) + TEST_ASSERT( ret == input_ret ); + +exit: + mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); +} +/* END_CASE */ + /* BEGIN_CASE */ void mbedtls_mpi_cmp_abs( int radix_X, char * input_X, int radix_Y, char * input_Y, int input_A ) From fc2a826ab46f6b11f15cc5869ef2276ba35212b4 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 16 Sep 2019 14:27:39 +0100 Subject: [PATCH 43/95] Fix side channel vulnerability in ECDSA --- library/ecp.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index 5c2e0f1bd..69976c812 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -2724,6 +2724,7 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, { /* SEC1 3.2.1: Generate d such that 1 <= n < N */ int count = 0; + int cmp = 0; /* * Match the procedure given in RFC 6979 (deterministic ECDSA): @@ -2734,6 +2735,7 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, */ do { + MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_size, f_rng, p_rng ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, 8 * n_size - grp->nbits ) ); @@ -2748,9 +2750,14 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, */ if( ++count > 30 ) return( MBEDTLS_ERR_ECP_RANDOM_FAILED ); + + ret = mbedtls_mpi_cmp_mpi_ct( d, &grp->N, &cmp ); + if( ret != 0 ) + { + goto cleanup; + } } - while( mbedtls_mpi_cmp_int( d, 1 ) < 0 || - mbedtls_mpi_cmp_mpi( d, &grp->N ) >= 0 ); + while( mbedtls_mpi_cmp_int( d, 1 ) < 0 || cmp >= 0 ); } #endif /* ECP_SHORTWEIERSTRASS */ From 78ed22b40450e17d6547c37089127bddd7484ba1 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 23 Sep 2019 09:19:14 +0100 Subject: [PATCH 44/95] Remove declaration after statement Visual Studio 2013 does not like it for some reason. --- library/bignum.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index a91aba89c..e023afc3a 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1105,7 +1105,7 @@ int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, int *ret ) { size_t i; - unsigned int cond, done; + unsigned int cond, done, sign_X, sign_Y; MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( Y != NULL ); @@ -1126,8 +1126,8 @@ int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, * done = 1; * } */ - unsigned int sign_X = X->s; - unsigned int sign_Y = Y->s; + sign_X = X->s; + sign_Y = Y->s; cond = ( ( sign_X ^ sign_Y ) >> ( sizeof( unsigned int ) * 8 - 1 ) ); *ret = cond * X->s; done = cond; From fd9797b59565773326d7821242d17f30071f1dd6 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 11 Oct 2019 10:22:37 +0100 Subject: [PATCH 45/95] Remove excess vertical space --- library/ecp.c | 1 - 1 file changed, 1 deletion(-) diff --git a/library/ecp.c b/library/ecp.c index 69976c812..952c3c413 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -2735,7 +2735,6 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, */ do { - MBEDTLS_MPI_CHK( mbedtls_mpi_fill_random( d, n_size, f_rng, p_rng ) ); MBEDTLS_MPI_CHK( mbedtls_mpi_shift_r( d, 8 * n_size - grp->nbits ) ); From 81c9fe5f2c26a01c4867d831e60293b9cd456246 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 11 Oct 2019 10:43:40 +0100 Subject: [PATCH 46/95] mbedtls_mpi_cmp_mpi_ct: remove multiplications Multiplication is known to have measurable timing variations based on the operands. For example it typically is much faster if one of the operands is zero. Remove them from constant time code. --- library/bignum.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index e023afc3a..c275b45de 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1098,6 +1098,11 @@ static int ct_lt_mpi_uint( const mbedtls_mpi_uint x, const mbedtls_mpi_uint y ) return ret; } +static int ct_bool_get_mask( unsigned int b ) +{ + return ~( b - 1 ); +} + /* * Compare signed values in constant time */ @@ -1129,7 +1134,7 @@ int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, sign_X = X->s; sign_Y = Y->s; cond = ( ( sign_X ^ sign_Y ) >> ( sizeof( unsigned int ) * 8 - 1 ) ); - *ret = cond * X->s; + *ret = ct_bool_get_mask( cond ) & X->s; done = cond; for( i = X->n; i > 0; i-- ) @@ -1142,8 +1147,8 @@ int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, * } */ cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ); - *ret |= ( cond * ( 1 - done ) ) * X->s; - done |= cond * ( 1 - done ); + *ret |= ct_bool_get_mask( cond & ( 1 - done ) ) & X->s; + done |= cond & ( 1 - done ); /* * if( ( X->p[i - 1] < Y->p[i - 1] ) && !done ) @@ -1153,9 +1158,8 @@ int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, * } */ cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ); - *ret |= ( cond * ( 1 - done ) ) * -X->s; - done |= cond * ( 1 - done ); - + *ret |= ct_bool_get_mask( cond & ( 1 - done ) ) & -X->s; + done |= cond & ( 1 - done ); } return( 0 ); From 8faf1d627bad1d77447a0d1d7ea3fe808acc9288 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 11 Oct 2019 14:21:53 +0100 Subject: [PATCH 47/95] Change mbedtls_mpi_cmp_mpi_ct to check less than The signature of mbedtls_mpi_cmp_mpi_ct() meant to support using it in place of mbedtls_mpi_cmp_mpi(). This meant full comparison functionality and a signed result. To make the function more universal and friendly to constant time coding, we change the result type to unsigned. Theoretically, we could encode the comparison result in an unsigned value, but it would be less intuitive. Therefore we won't be able to represent the result as unsigned anymore and the functionality will be constrained to checking if the first operand is less than the second. This is sufficient to support the current use case and to check any relationship between MPIs. The only drawback is that we need to call the function twice when checking for equality, but this can be optimised later if an when it is needed. --- include/mbedtls/bignum.h | 11 ++--- library/bignum.c | 68 ++++++++++++++-------------- library/ecp.c | 6 +-- tests/suites/test_suite_mpi.data | 44 +++++++++--------- tests/suites/test_suite_mpi.function | 12 +++-- 5 files changed, 71 insertions(+), 70 deletions(-) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index b57f2a17f..69dc0b02d 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -560,23 +560,22 @@ int mbedtls_mpi_cmp_abs( const mbedtls_mpi *X, const mbedtls_mpi *Y ); int mbedtls_mpi_cmp_mpi( const mbedtls_mpi *X, const mbedtls_mpi *Y ); /** - * \brief Compare two MPIs in constant time. + * \brief Check if an MPI is less than the other in constant time. * * \param X The left-hand MPI. This must point to an initialized MPI * with the same allocated length as Y. * \param Y The right-hand MPI. This must point to an initialized MPI * with the same allocated length as X. * \param ret The result of the comparison: - * \c 1 if \p X is greater than \p Y. - * \c -1 if \p X is lesser than \p Y. - * \c 0 if \p X is equal to \p Y. + * \c 1 if \p X is less than \p Y. + * \c 0 if \p X is greater than or equal to \p Y. * * \return 0 on success. * \return MBEDTLS_ERR_MPI_BAD_INPUT_DATA if the allocated length of * the two input MPIs is not the same. */ -int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, - int *ret ); +int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, + unsigned *ret ); /** * \brief Compare an MPI with an integer. diff --git a/library/bignum.c b/library/bignum.c index c275b45de..2c1cbeb96 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1071,7 +1071,8 @@ int mbedtls_mpi_cmp_mpi( const mbedtls_mpi *X, const mbedtls_mpi *Y ) return( 0 ); } -static int ct_lt_mpi_uint( const mbedtls_mpi_uint x, const mbedtls_mpi_uint y ) +static unsigned ct_lt_mpi_uint( const mbedtls_mpi_uint x, + const mbedtls_mpi_uint y ) { mbedtls_mpi_uint ret; mbedtls_mpi_uint cond; @@ -1098,16 +1099,11 @@ static int ct_lt_mpi_uint( const mbedtls_mpi_uint x, const mbedtls_mpi_uint y ) return ret; } -static int ct_bool_get_mask( unsigned int b ) -{ - return ~( b - 1 ); -} - /* * Compare signed values in constant time */ -int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, - int *ret ) +int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, + unsigned *ret ) { size_t i; unsigned int cond, done, sign_X, sign_Y; @@ -1120,45 +1116,49 @@ int mbedtls_mpi_cmp_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, return MBEDTLS_ERR_MPI_BAD_INPUT_DATA; /* - * if( X->s > 0 && Y->s < 0 ) - * { - * *ret = 1; - * done = 1; - * } - * else if( Y->s > 0 && X->s < 0 ) - * { - * *ret = -1; - * done = 1; - * } + * Get sign bits of the signs. */ sign_X = X->s; + sign_X = sign_X >> ( sizeof( unsigned int ) * 8 - 1 ); sign_Y = Y->s; - cond = ( ( sign_X ^ sign_Y ) >> ( sizeof( unsigned int ) * 8 - 1 ) ); - *ret = ct_bool_get_mask( cond ) & X->s; + sign_Y = sign_Y >> ( sizeof( unsigned int ) * 8 - 1 ); + + /* + * If the signs are different, then the positive operand is the bigger. + * That is if X is negative (sign bit 1), then X < Y is true and it is false + * if X is positive (sign bit 0). + */ + cond = ( sign_X ^ sign_Y ); + *ret = cond & sign_X; + + /* + * This is a constant time function, we might have the result, but we still + * need to go through the loop. Record if we have the result already. + */ done = cond; for( i = X->n; i > 0; i-- ) { /* - * if( ( X->p[i - 1] > Y->p[i - 1] ) && !done ) - * { - * done = 1; - * *ret = X->s; - * } + * If Y->p[i - 1] < X->p[i - 1] and both X and Y are negative, then + * X < Y. + * + * Again even if we can make a decision, we just mark the result and + * the fact that we are done and continue looping. */ - cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ); - *ret |= ct_bool_get_mask( cond & ( 1 - done ) ) & X->s; + cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ) & sign_X; + *ret |= cond & ( 1 - done ); done |= cond & ( 1 - done ); /* - * if( ( X->p[i - 1] < Y->p[i - 1] ) && !done ) - * { - * done = 1; - * *ret = -X->s; - * } + * If X->p[i - 1] < Y->p[i - 1] and both X and Y are positive, then + * X < Y. + * + * Again even if we can make a decision, we just mark the result and + * the fact that we are done and continue looping. */ - cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ); - *ret |= ct_bool_get_mask( cond & ( 1 - done ) ) & -X->s; + cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ) & ( 1 - sign_X ); + *ret |= cond & ( 1 - done ); done |= cond & ( 1 - done ); } diff --git a/library/ecp.c b/library/ecp.c index 952c3c413..661112bb7 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -2724,7 +2724,7 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, { /* SEC1 3.2.1: Generate d such that 1 <= n < N */ int count = 0; - int cmp = 0; + unsigned cmp = 0; /* * Match the procedure given in RFC 6979 (deterministic ECDSA): @@ -2750,13 +2750,13 @@ int mbedtls_ecp_gen_privkey( const mbedtls_ecp_group *grp, if( ++count > 30 ) return( MBEDTLS_ERR_ECP_RANDOM_FAILED ); - ret = mbedtls_mpi_cmp_mpi_ct( d, &grp->N, &cmp ); + ret = mbedtls_mpi_lt_mpi_ct( d, &grp->N, &cmp ); if( ret != 0 ) { goto cleanup; } } - while( mbedtls_mpi_cmp_int( d, 1 ) < 0 || cmp >= 0 ); + while( mbedtls_mpi_cmp_int( d, 1 ) < 0 || cmp != 1 ); } #endif /* ECP_SHORTWEIERSTRASS */ diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index f9f51ac27..821fd89ad 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -163,38 +163,38 @@ mbedtls_mpi_cmp_mpi:10:"2":10:"-3":1 Base test mbedtls_mpi_cmp_mpi (Mixed values) #6 mbedtls_mpi_cmp_mpi:10:"-2":10:"31231231289798":-1 -Base test mbedtls_mpi_cmp_mpi_ct #1 -mbedtls_mpi_cmp_mpi_ct:1:10:"693":1:10:"693":0:0 +Base test mbedtls_mpi_lt_mpi_ct #1 +mbedtls_mpi_lt_mpi_ct:1:10:"693":1:10:"693":0:0 -Base test mbedtls_mpi_cmp_mpi_ct #2 -mbedtls_mpi_cmp_mpi_ct:1:10:"693":1:10:"692":1:0 +Base test mbedtls_mpi_lt_mpi_ct #2 +mbedtls_mpi_lt_mpi_ct:1:10:"693":1:10:"692":0:0 -Base test mbedtls_mpi_cmp_mpi_ct #3 -mbedtls_mpi_cmp_mpi_ct:1:10:"693":1:10:"694":-1:0 +Base test mbedtls_mpi_lt_mpi_ct #3 +mbedtls_mpi_lt_mpi_ct:1:10:"693":1:10:"694":1:0 -Base test mbedtls_mpi_cmp_mpi_ct (Negative values) #1 -mbedtls_mpi_cmp_mpi_ct:1:10:"-2":1:10:"-2":0:0 +Base test mbedtls_mpi_lt_mpi_ct (Negative values) #1 +mbedtls_mpi_lt_mpi_ct:1:10:"-2":1:10:"-2":0:0 -Base test mbedtls_mpi_cmp_mpi_ct (Negative values) #2 -mbedtls_mpi_cmp_mpi_ct:1:10:"-2":1:10:"-3":1:0 +Base test mbedtls_mpi_lt_mpi_ct (Negative values) #2 +mbedtls_mpi_lt_mpi_ct:1:10:"-2":1:10:"-3":0:0 -Base test mbedtls_mpi_cmp_mpi_ct (Negative values) #3 -mbedtls_mpi_cmp_mpi_ct:1:10:"-2":1:10:"-1":-1:0 +Base test mbedtls_mpi_lt_mpi_ct (Negative values) #3 +mbedtls_mpi_lt_mpi_ct:1:10:"-2":1:10:"-1":1:0 -Base test mbedtls_mpi_cmp_mpi_ct (Mixed values) #4 -mbedtls_mpi_cmp_mpi_ct:1:10:"-3":1:10:"2":-1:0 +Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #4 +mbedtls_mpi_lt_mpi_ct:1:10:"-3":1:10:"2":1:0 -Base test mbedtls_mpi_cmp_mpi_ct (Mixed values) #5 -mbedtls_mpi_cmp_mpi_ct:1:10:"2":1:10:"-3":1:0 +Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #5 +mbedtls_mpi_lt_mpi_ct:1:10:"2":1:10:"-3":0:0 -Base test mbedtls_mpi_cmp_mpi_ct (Mixed values) #6 -mbedtls_mpi_cmp_mpi_ct:2:10:"-2":2:10:"31231231289798":-1:0 +Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #6 +mbedtls_mpi_lt_mpi_ct:2:10:"-2":2:10:"31231231289798":1:0 -Base test mbedtls_mpi_cmp_mpi_ct (X is longer in storage) #7 -mbedtls_mpi_cmp_mpi_ct:3:10:"693":2:10:"693":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA +Base test mbedtls_mpi_lt_mpi_ct (X is longer in storage) #7 +mbedtls_mpi_lt_mpi_ct:3:10:"693":2:10:"693":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA -Base test mbedtls_mpi_cmp_mpi_ct (Y is longer in storage) #8 -mbedtls_mpi_cmp_mpi_ct:3:10:"693":4:10:"693":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA +Base test mbedtls_mpi_lt_mpi_ct (Y is longer in storage) #8 +mbedtls_mpi_lt_mpi_ct:3:10:"693":4:10:"693":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA Base test mbedtls_mpi_cmp_abs #1 mbedtls_mpi_cmp_abs:10:"693":10:"693":0 diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index 718b165e1..a0aa726a7 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -538,10 +538,12 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -void mbedtls_mpi_cmp_mpi_ct( int size_X, int radix_X, char * input_X, int size_Y, - int radix_Y, char * input_Y, int input_ret, int input_err ) +void mbedtls_mpi_lt_mpi_ct( int size_X, int radix_X, char * input_X, + int size_Y, int radix_Y, char * input_Y, + int input_ret, int input_err ) { - int ret; + unsigned ret; + unsigned input_uret = input_ret; mbedtls_mpi X, Y; mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); @@ -551,9 +553,9 @@ void mbedtls_mpi_cmp_mpi_ct( int size_X, int radix_X, char * input_X, int size_Y mbedtls_mpi_grow( &X, size_X ); mbedtls_mpi_grow( &Y, size_Y ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi_ct( &X, &Y, &ret ) == input_err ); + TEST_ASSERT( mbedtls_mpi_lt_mpi_ct( &X, &Y, &ret ) == input_err ); if( input_err == 0 ) - TEST_ASSERT( ret == input_ret ); + TEST_ASSERT( ret == input_uret ); exit: mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); From a83037714286ec3976964af6525ddf9ddd565085 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 14 Oct 2019 08:59:14 +0100 Subject: [PATCH 48/95] ct_lt_mpi_uint: make use of biL --- library/bignum.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index 2c1cbeb96..2221929a2 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1094,7 +1094,7 @@ static unsigned ct_lt_mpi_uint( const mbedtls_mpi_uint x, ret |= y & cond; - ret = ret >> ( sizeof( mbedtls_mpi_uint ) * 8 - 1 ); + ret = ret >> ( biL - 1 ); return ret; } From afa534245284e0af09d589863ba3fac0bb517bb7 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 14 Oct 2019 09:01:15 +0100 Subject: [PATCH 49/95] mpi_lt_mpi_ct: make use of unsigned consistent --- library/bignum.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 2221929a2..749b68b84 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1106,7 +1106,7 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, unsigned *ret ) { size_t i; - unsigned int cond, done, sign_X, sign_Y; + unsigned cond, done, sign_X, sign_Y; MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( Y != NULL ); @@ -1119,9 +1119,9 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, * Get sign bits of the signs. */ sign_X = X->s; - sign_X = sign_X >> ( sizeof( unsigned int ) * 8 - 1 ); + sign_X = sign_X >> ( sizeof( unsigned ) * 8 - 1 ); sign_Y = Y->s; - sign_Y = sign_Y >> ( sizeof( unsigned int ) * 8 - 1 ); + sign_Y = sign_Y >> ( sizeof( unsigned ) * 8 - 1 ); /* * If the signs are different, then the positive operand is the bigger. From 3480947667a66bf82cefaf9e704b1a1ebac61a2e Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 14 Oct 2019 09:09:32 +0100 Subject: [PATCH 50/95] Document ct_lt_mpi_uint --- library/bignum.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/library/bignum.c b/library/bignum.c index 749b68b84..cd26409bb 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1071,6 +1071,13 @@ int mbedtls_mpi_cmp_mpi( const mbedtls_mpi *X, const mbedtls_mpi *Y ) return( 0 ); } +/** Decide if an integer is less than the other, without branches. + * + * \param x First integer. + * \param y Second integer. + * + * \return 1 if \p x is less than \p y, 0 otherwise + */ static unsigned ct_lt_mpi_uint( const mbedtls_mpi_uint x, const mbedtls_mpi_uint y ) { From 3be2fa44e163db9b9e05e5d14064fc0f5aee4e97 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 14 Oct 2019 09:21:49 +0100 Subject: [PATCH 51/95] mpi_lt_mpi_ct test: hardcode base 16 --- tests/suites/test_suite_mpi.data | 22 +++++++++++----------- tests/suites/test_suite_mpi.function | 8 ++++---- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 821fd89ad..32eaec07f 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -164,37 +164,37 @@ Base test mbedtls_mpi_cmp_mpi (Mixed values) #6 mbedtls_mpi_cmp_mpi:10:"-2":10:"31231231289798":-1 Base test mbedtls_mpi_lt_mpi_ct #1 -mbedtls_mpi_lt_mpi_ct:1:10:"693":1:10:"693":0:0 +mbedtls_mpi_lt_mpi_ct:1:"2B5":1:"2B5":0:0 Base test mbedtls_mpi_lt_mpi_ct #2 -mbedtls_mpi_lt_mpi_ct:1:10:"693":1:10:"692":0:0 +mbedtls_mpi_lt_mpi_ct:1:"2B5":1:"2B4":0:0 Base test mbedtls_mpi_lt_mpi_ct #3 -mbedtls_mpi_lt_mpi_ct:1:10:"693":1:10:"694":1:0 +mbedtls_mpi_lt_mpi_ct:1:"2B5":1:"2B6":1:0 Base test mbedtls_mpi_lt_mpi_ct (Negative values) #1 -mbedtls_mpi_lt_mpi_ct:1:10:"-2":1:10:"-2":0:0 +mbedtls_mpi_lt_mpi_ct:1:"-2":1:"-2":0:0 Base test mbedtls_mpi_lt_mpi_ct (Negative values) #2 -mbedtls_mpi_lt_mpi_ct:1:10:"-2":1:10:"-3":0:0 +mbedtls_mpi_lt_mpi_ct:1:"-2":1:"-3":0:0 Base test mbedtls_mpi_lt_mpi_ct (Negative values) #3 -mbedtls_mpi_lt_mpi_ct:1:10:"-2":1:10:"-1":1:0 +mbedtls_mpi_lt_mpi_ct:1:"-2":1:"-1":1:0 Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #4 -mbedtls_mpi_lt_mpi_ct:1:10:"-3":1:10:"2":1:0 +mbedtls_mpi_lt_mpi_ct:1:"-3":1:"2":1:0 Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #5 -mbedtls_mpi_lt_mpi_ct:1:10:"2":1:10:"-3":0:0 +mbedtls_mpi_lt_mpi_ct:1:"2":1:"-3":0:0 Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #6 -mbedtls_mpi_lt_mpi_ct:2:10:"-2":2:10:"31231231289798":1:0 +mbedtls_mpi_lt_mpi_ct:2:"-2":2:"1C67967269C6":1:0 Base test mbedtls_mpi_lt_mpi_ct (X is longer in storage) #7 -mbedtls_mpi_lt_mpi_ct:3:10:"693":2:10:"693":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA +mbedtls_mpi_lt_mpi_ct:3:"2B5":2:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA Base test mbedtls_mpi_lt_mpi_ct (Y is longer in storage) #8 -mbedtls_mpi_lt_mpi_ct:3:10:"693":4:10:"693":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA +mbedtls_mpi_lt_mpi_ct:3:"2B5":4:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA Base test mbedtls_mpi_cmp_abs #1 mbedtls_mpi_cmp_abs:10:"693":10:"693":0 diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index a0aa726a7..97c338b85 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -538,8 +538,8 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -void mbedtls_mpi_lt_mpi_ct( int size_X, int radix_X, char * input_X, - int size_Y, int radix_Y, char * input_Y, +void mbedtls_mpi_lt_mpi_ct( int size_X, char * input_X, + int size_Y, char * input_Y, int input_ret, int input_err ) { unsigned ret; @@ -547,8 +547,8 @@ void mbedtls_mpi_lt_mpi_ct( int size_X, int radix_X, char * input_X, mbedtls_mpi X, Y; mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); - TEST_ASSERT( mbedtls_mpi_read_string( &X, radix_X, input_X ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_string( &Y, radix_Y, input_Y ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_string( &X, 16, input_X ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_string( &Y, 16, input_Y ) == 0 ); mbedtls_mpi_grow( &X, size_X ); mbedtls_mpi_grow( &Y, size_Y ); From eb8fcf8181b888a2bd6e5b5aa6732f1b5291d583 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 14 Oct 2019 11:33:39 +0100 Subject: [PATCH 52/95] Add more tests for mbedtls_mpi_lt_mpi_ct --- tests/suites/test_suite_mpi.data | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 32eaec07f..553612592 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -196,6 +196,36 @@ mbedtls_mpi_lt_mpi_ct:3:"2B5":2:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA Base test mbedtls_mpi_lt_mpi_ct (Y is longer in storage) #8 mbedtls_mpi_lt_mpi_ct:3:"2B5":4:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA +Base test mbedtls_mpi_lt_mpi_ct (corner case) #1 +mbedtls_mpi_lt_mpi_ct:1:"7FFFFFFFFFFFFFFF":1:"FF":0:0 + +Base test mbedtls_mpi_lt_mpi_ct (corner case) #2 +mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"7FFFFFFFFFFFFFFF":0:0 + +Base test mbedtls_mpi_lt_mpi_ct (corner case) #2 +mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"1":0:0 + +Base test mbedtls_mpi_lt_mpi_ct (corner case) #2 +mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"0":0:0 + +Base test mbedtls_mpi_lt_mpi_ct (corner case) #3 +mbedtls_mpi_lt_mpi_ct:1:"FFFFFFFFFFFFFFFF":1:"FF":0:0 + +Multi-limb mbedtls_mpi_lt_mpi_ct (XY, equal MS limbs) #3 +mbedtls_mpi_lt_mpi_ct:2:"-EEFFFFFFFFFFFFFFF1":2:"-EEFFFFFFFFFFFFFFFF":0:0 + +Multi-limb mbedtls_mpi_lt_mpi_ct (X=Y) #4 +mbedtls_mpi_lt_mpi_ct:2:"EEFFFFFFFFFFFFFFFF":2:"EEFFFFFFFFFFFFFFFF":0:0 + +Multi-limb mbedtls_mpi_lt_mpi_ct (X=-Y) #4 +mbedtls_mpi_lt_mpi_ct:2:"-EEFFFFFFFFFFFFFFFF":2:"EEFFFFFFFFFFFFFFFF":1:0 + Base test mbedtls_mpi_cmp_abs #1 mbedtls_mpi_cmp_abs:10:"693":10:"693":0 From f8dbfd4f05d8dbcce97a26fe147bdc19aa72ae24 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 28 Oct 2019 12:07:52 +0000 Subject: [PATCH 53/95] Bignum: Document assumptions about the sign field --- include/mbedtls/bignum.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index 69dc0b02d..22b373113 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -184,7 +184,7 @@ extern "C" { */ typedef struct mbedtls_mpi { - int s; /*!< integer sign */ + int s; /*!< Sign: -1 if the mpi is negative, 1 otherwise */ size_t n; /*!< total # of limbs */ mbedtls_mpi_uint *p; /*!< pointer to limbs */ } From aa9e7a47171d631be2df4ccc4fe0c379e14f5601 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 28 Oct 2019 12:12:15 +0000 Subject: [PATCH 54/95] Make mbedtls_mpi_lt_mpi_ct more portable The code relied on the assumptions that CHAR_BIT is 8 and that unsigned does not have padding bits. In the Bignum module we already assume that the sign of an MPI is either -1 or 1. Using this, we eliminate the above mentioned dependency. --- library/bignum.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index cd26409bb..c1bed6642 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1123,12 +1123,11 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, return MBEDTLS_ERR_MPI_BAD_INPUT_DATA; /* - * Get sign bits of the signs. + * Set sign_N to 1 if N >= 0, 0 if N < 0. + * We know that N->s == 1 if N >= 0 and N->s == -1 if N < 0. */ - sign_X = X->s; - sign_X = sign_X >> ( sizeof( unsigned ) * 8 - 1 ); - sign_Y = Y->s; - sign_Y = sign_Y >> ( sizeof( unsigned ) * 8 - 1 ); + sign_X = ( X->s & 2 ) >> 1; + sign_Y = ( Y->s & 2 ) >> 1; /* * If the signs are different, then the positive operand is the bigger. From cf7eeef2ccfce13eeb184b25624e2fb6dc0b0661 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 28 Oct 2019 12:23:18 +0000 Subject: [PATCH 55/95] mbedtls_mpi_lt_mpi_ct: Improve documentation --- library/bignum.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index c1bed6642..3328d64e6 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1113,6 +1113,7 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, unsigned *ret ) { size_t i; + /* The value of any of these variables is either 0 or 1 at all times. */ unsigned cond, done, sign_X, sign_Y; MPI_VALIDATE_RET( X != NULL ); @@ -1131,14 +1132,14 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, /* * If the signs are different, then the positive operand is the bigger. - * That is if X is negative (sign bit 1), then X < Y is true and it is false - * if X is positive (sign bit 0). + * That is if X is negative (sign_X == 1), then X < Y is true and it is + * false if X is positive (sign_X == 0). */ cond = ( sign_X ^ sign_Y ); *ret = cond & sign_X; /* - * This is a constant time function, we might have the result, but we still + * This is a constant-time function. We might have the result, but we still * need to go through the loop. Record if we have the result already. */ done = cond; From ec4c42a95f96706d1a03dfae3fcd2e9b4a254ff7 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 28 Oct 2019 12:31:34 +0000 Subject: [PATCH 56/95] Rename variable for better readability --- library/bignum.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 3328d64e6..c760f12bb 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1114,7 +1114,7 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, { size_t i; /* The value of any of these variables is either 0 or 1 at all times. */ - unsigned cond, done, sign_X, sign_Y; + unsigned cond, done, X_is_negative, Y_is_negative; MPI_VALIDATE_RET( X != NULL ); MPI_VALIDATE_RET( Y != NULL ); @@ -1127,16 +1127,16 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, * Set sign_N to 1 if N >= 0, 0 if N < 0. * We know that N->s == 1 if N >= 0 and N->s == -1 if N < 0. */ - sign_X = ( X->s & 2 ) >> 1; - sign_Y = ( Y->s & 2 ) >> 1; + X_is_negative = ( X->s & 2 ) >> 1; + Y_is_negative = ( Y->s & 2 ) >> 1; /* * If the signs are different, then the positive operand is the bigger. - * That is if X is negative (sign_X == 1), then X < Y is true and it is - * false if X is positive (sign_X == 0). + * That is if X is negative (X_is_negative == 1), then X < Y is true and it + * is false if X is positive (X_is_negative == 0). */ - cond = ( sign_X ^ sign_Y ); - *ret = cond & sign_X; + cond = ( X_is_negative ^ Y_is_negative ); + *ret = cond & X_is_negative; /* * This is a constant-time function. We might have the result, but we still @@ -1153,7 +1153,7 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, * Again even if we can make a decision, we just mark the result and * the fact that we are done and continue looping. */ - cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ) & sign_X; + cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ) & X_is_negative; *ret |= cond & ( 1 - done ); done |= cond & ( 1 - done ); @@ -1164,7 +1164,8 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, * Again even if we can make a decision, we just mark the result and * the fact that we are done and continue looping. */ - cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ) & ( 1 - sign_X ); + cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ) + & ( 1 - X_is_negative ); *ret |= cond & ( 1 - done ); done |= cond & ( 1 - done ); } From c8256e7020e1b338e8915c6c42deb6c0bc4eae18 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 28 Oct 2019 12:37:21 +0000 Subject: [PATCH 57/95] mbedtls_mpi_lt_mpi_ct: simplify condition In the case of *ret we might need to preserve a 0 value throughout the loop and therefore we need an extra condition to protect it from being overwritten. The value of done is always 1 after *ret has been set and does not need to be protected from overwriting. Therefore in this case the extra condition can be removed. --- library/bignum.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index c760f12bb..5c94e995b 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1155,7 +1155,7 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, */ cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ) & X_is_negative; *ret |= cond & ( 1 - done ); - done |= cond & ( 1 - done ); + done |= cond; /* * If X->p[i - 1] < Y->p[i - 1] and both X and Y are positive, then @@ -1167,7 +1167,7 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ) & ( 1 - X_is_negative ); *ret |= cond & ( 1 - done ); - done |= cond & ( 1 - done ); + done |= cond; } return( 0 ); From 44e40c079262b650c2700ae583f7d2868e41fc93 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 29 Oct 2019 15:05:12 +0000 Subject: [PATCH 58/95] mbedtls_mpi_lt_mpi_ct: add tests for 32 bit limbs The corner case tests were designed for 64 bit limbs and failed on 32 bit platforms because the numbers in the test ended up being stored in a different number of limbs and the function (correctly) returnd an error upon receiving them. --- tests/suites/test_suite_mpi.data | 35 +++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 553612592..ce16a4fac 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -196,21 +196,46 @@ mbedtls_mpi_lt_mpi_ct:3:"2B5":2:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA Base test mbedtls_mpi_lt_mpi_ct (Y is longer in storage) #8 mbedtls_mpi_lt_mpi_ct:3:"2B5":4:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA -Base test mbedtls_mpi_lt_mpi_ct (corner case) #1 +Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #1 +depends_on:MBEDTLS_HAVE_INT64 mbedtls_mpi_lt_mpi_ct:1:"7FFFFFFFFFFFFFFF":1:"FF":0:0 -Base test mbedtls_mpi_lt_mpi_ct (corner case) #2 +Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #2 +depends_on:MBEDTLS_HAVE_INT64 mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"7FFFFFFFFFFFFFFF":0:0 -Base test mbedtls_mpi_lt_mpi_ct (corner case) #2 +Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #3 +depends_on:MBEDTLS_HAVE_INT64 mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"1":0:0 -Base test mbedtls_mpi_lt_mpi_ct (corner case) #2 +Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #4 +depends_on:MBEDTLS_HAVE_INT64 mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"0":0:0 -Base test mbedtls_mpi_lt_mpi_ct (corner case) #3 +Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #5 +depends_on:MBEDTLS_HAVE_INT64 mbedtls_mpi_lt_mpi_ct:1:"FFFFFFFFFFFFFFFF":1:"FF":0:0 +Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #1 +depends_on:MBEDTLS_HAVE_INT32 +mbedtls_mpi_lt_mpi_ct:1:"7FFFFFFF":1:"FF":0:0 + +Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #2 +depends_on:MBEDTLS_HAVE_INT32 +mbedtls_mpi_lt_mpi_ct:1:"80000000":1:"7FFFFFFF":0:0 + +Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #3 +depends_on:MBEDTLS_HAVE_INT32 +mbedtls_mpi_lt_mpi_ct:1:"80000000":1:"1":0:0 + +Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #4 +depends_on:MBEDTLS_HAVE_INT32 +mbedtls_mpi_lt_mpi_ct:1:"80000000":1:"0":0:0 + +Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #5 +depends_on:MBEDTLS_HAVE_INT32 +mbedtls_mpi_lt_mpi_ct:1:"FFFFFFFF":1:"FF":0:0 + Multi-limb mbedtls_mpi_lt_mpi_ct (X Date: Tue, 29 Oct 2019 15:08:46 +0000 Subject: [PATCH 59/95] ct_lt_mpi_uint: cast the return value explicitely The return value is always either one or zero and therefore there is no risk of losing precision. Some compilers can't deduce this and complain. --- library/bignum.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index 5c94e995b..3ea33f432 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1103,7 +1103,7 @@ static unsigned ct_lt_mpi_uint( const mbedtls_mpi_uint x, ret = ret >> ( biL - 1 ); - return ret; + return (unsigned) ret; } /* From d2aa4aa4549ba1d8589f13862eda2da5518e1377 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 5 Nov 2019 11:42:20 +0000 Subject: [PATCH 60/95] mpi_lt_mpi_ct perform tests for both limb size The corner case tests were designed for 32 and 64 bit limbs independently and performed only on the target platform. On the other platform they are not corner cases anymore, but we can still exercise them. --- tests/suites/test_suite_mpi.data | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index ce16a4fac..0b7aa2e1a 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -197,43 +197,33 @@ Base test mbedtls_mpi_lt_mpi_ct (Y is longer in storage) #8 mbedtls_mpi_lt_mpi_ct:3:"2B5":4:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #1 -depends_on:MBEDTLS_HAVE_INT64 -mbedtls_mpi_lt_mpi_ct:1:"7FFFFFFFFFFFFFFF":1:"FF":0:0 +mbedtls_mpi_lt_mpi_ct:2:"7FFFFFFFFFFFFFFF":2:"FF":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #2 -depends_on:MBEDTLS_HAVE_INT64 -mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"7FFFFFFFFFFFFFFF":0:0 +mbedtls_mpi_lt_mpi_ct:2:"8000000000000000":2:"7FFFFFFFFFFFFFFF":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #3 -depends_on:MBEDTLS_HAVE_INT64 -mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"1":0:0 +mbedtls_mpi_lt_mpi_ct:2:"8000000000000000":2:"1":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #4 -depends_on:MBEDTLS_HAVE_INT64 -mbedtls_mpi_lt_mpi_ct:1:"8000000000000000":1:"0":0:0 +mbedtls_mpi_lt_mpi_ct:2:"8000000000000000":2:"0":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #5 -depends_on:MBEDTLS_HAVE_INT64 -mbedtls_mpi_lt_mpi_ct:1:"FFFFFFFFFFFFFFFF":1:"FF":0:0 +mbedtls_mpi_lt_mpi_ct:2:"FFFFFFFFFFFFFFFF":2:"FF":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #1 -depends_on:MBEDTLS_HAVE_INT32 mbedtls_mpi_lt_mpi_ct:1:"7FFFFFFF":1:"FF":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #2 -depends_on:MBEDTLS_HAVE_INT32 mbedtls_mpi_lt_mpi_ct:1:"80000000":1:"7FFFFFFF":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #3 -depends_on:MBEDTLS_HAVE_INT32 mbedtls_mpi_lt_mpi_ct:1:"80000000":1:"1":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #4 -depends_on:MBEDTLS_HAVE_INT32 mbedtls_mpi_lt_mpi_ct:1:"80000000":1:"0":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #5 -depends_on:MBEDTLS_HAVE_INT32 mbedtls_mpi_lt_mpi_ct:1:"FFFFFFFF":1:"FF":0:0 Multi-limb mbedtls_mpi_lt_mpi_ct (X Date: Tue, 5 Nov 2019 11:56:07 +0000 Subject: [PATCH 61/95] mpi_lt_mpi_ct: Fix test numbering --- tests/suites/test_suite_mpi.data | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 0b7aa2e1a..b5183777a 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -181,19 +181,19 @@ mbedtls_mpi_lt_mpi_ct:1:"-2":1:"-3":0:0 Base test mbedtls_mpi_lt_mpi_ct (Negative values) #3 mbedtls_mpi_lt_mpi_ct:1:"-2":1:"-1":1:0 -Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #4 +Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #1 mbedtls_mpi_lt_mpi_ct:1:"-3":1:"2":1:0 -Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #5 +Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #2 mbedtls_mpi_lt_mpi_ct:1:"2":1:"-3":0:0 -Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #6 +Base test mbedtls_mpi_lt_mpi_ct (Mixed values) #3 mbedtls_mpi_lt_mpi_ct:2:"-2":2:"1C67967269C6":1:0 -Base test mbedtls_mpi_lt_mpi_ct (X is longer in storage) #7 +Base test mbedtls_mpi_lt_mpi_ct (X is longer in storage) mbedtls_mpi_lt_mpi_ct:3:"2B5":2:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA -Base test mbedtls_mpi_lt_mpi_ct (Y is longer in storage) #8 +Base test mbedtls_mpi_lt_mpi_ct (Y is longer in storage) mbedtls_mpi_lt_mpi_ct:3:"2B5":4:"2B5":0:MBEDTLS_ERR_MPI_BAD_INPUT_DATA Base test mbedtls_mpi_lt_mpi_ct (corner case - 64 bit) #1 @@ -226,19 +226,19 @@ mbedtls_mpi_lt_mpi_ct:1:"80000000":1:"0":0:0 Base test mbedtls_mpi_lt_mpi_ct (corner case - 32 bit) #5 mbedtls_mpi_lt_mpi_ct:1:"FFFFFFFF":1:"FF":0:0 -Multi-limb mbedtls_mpi_lt_mpi_ct (XY, equal MS limbs) #3 +Multi-limb mbedtls_mpi_lt_mpi_ct (X>Y, equal MS limbs) mbedtls_mpi_lt_mpi_ct:2:"-EEFFFFFFFFFFFFFFF1":2:"-EEFFFFFFFFFFFFFFFF":0:0 -Multi-limb mbedtls_mpi_lt_mpi_ct (X=Y) #4 +Multi-limb mbedtls_mpi_lt_mpi_ct (X=Y) mbedtls_mpi_lt_mpi_ct:2:"EEFFFFFFFFFFFFFFFF":2:"EEFFFFFFFFFFFFFFFF":0:0 -Multi-limb mbedtls_mpi_lt_mpi_ct (X=-Y) #4 +Multi-limb mbedtls_mpi_lt_mpi_ct (X=-Y) mbedtls_mpi_lt_mpi_ct:2:"-EEFFFFFFFFFFFFFFFF":2:"EEFFFFFFFFFFFFFFFF":1:0 Base test mbedtls_mpi_cmp_abs #1 From 47b56a159e1e71eae8054edec7208b3f77bcacb9 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 5 Nov 2019 12:19:14 +0000 Subject: [PATCH 62/95] mpi_lt_mpi_ct: Add further tests The existing tests did not catch a failure that came up at integration testing. Adding the missing test cases to trigger the bug. --- tests/suites/test_suite_mpi.data | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index b5183777a..4d46ca742 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -229,9 +229,6 @@ mbedtls_mpi_lt_mpi_ct:1:"FFFFFFFF":1:"FF":0:0 Multi-limb mbedtls_mpi_lt_mpi_ct (XY, equal MS limbs) mbedtls_mpi_lt_mpi_ct:2:"-EEFFFFFFFFFFFFFFF1":2:"-EEFFFFFFFFFFFFFFFF":0:0 @@ -241,6 +238,18 @@ mbedtls_mpi_lt_mpi_ct:2:"EEFFFFFFFFFFFFFFFF":2:"EEFFFFFFFFFFFFFFFF":0:0 Multi-limb mbedtls_mpi_lt_mpi_ct (X=-Y) mbedtls_mpi_lt_mpi_ct:2:"-EEFFFFFFFFFFFFFFFF":2:"EEFFFFFFFFFFFFFFFF":1:0 +Multi-limb mbedtls_mpi_lt_mpi_ct (Alternating limbs) #1 +mbedtls_mpi_lt_mpi_ct:2:"11FFFFFFFFFFFFFFFF":2:"FF1111111111111111":1:0 + +Multi-limb mbedtls_mpi_lt_mpi_ct (Alternating limbs) #2 +mbedtls_mpi_lt_mpi_ct:2:"FF1111111111111111":2:"11FFFFFFFFFFFFFFFF":0:0 + +Multi-limb mbedtls_mpi_lt_mpi_ct (Alternating limbs) #3 +mbedtls_mpi_lt_mpi_ct:2:"-11FFFFFFFFFFFFFFFF":2:"-FF1111111111111111":0:0 + +Multi-limb mbedtls_mpi_lt_mpi_ct (Alternating limbs) #4 +mbedtls_mpi_lt_mpi_ct:2:"-FF1111111111111111":2:"-11FFFFFFFFFFFFFFFF":1:0 + Base test mbedtls_mpi_cmp_abs #1 mbedtls_mpi_cmp_abs:10:"693":10:"693":0 From e9db2aa5b47899c7292fe544369e96d0330e6938 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 5 Nov 2019 12:24:52 +0000 Subject: [PATCH 63/95] mpi_lt_mpi_ct: fix condition handling The code previously only set the done flag if the return value was one. This led to overriding the correct return value later on. --- library/bignum.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 3ea33f432..a0964e348 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -1147,26 +1147,25 @@ int mbedtls_mpi_lt_mpi_ct( const mbedtls_mpi *X, const mbedtls_mpi *Y, for( i = X->n; i > 0; i-- ) { /* - * If Y->p[i - 1] < X->p[i - 1] and both X and Y are negative, then - * X < Y. + * If Y->p[i - 1] < X->p[i - 1] then X < Y is true if and only if both + * X and Y are negative. * * Again even if we can make a decision, we just mark the result and * the fact that we are done and continue looping. */ - cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ) & X_is_negative; - *ret |= cond & ( 1 - done ); + cond = ct_lt_mpi_uint( Y->p[i - 1], X->p[i - 1] ); + *ret |= cond & ( 1 - done ) & X_is_negative; done |= cond; /* - * If X->p[i - 1] < Y->p[i - 1] and both X and Y are positive, then - * X < Y. + * If X->p[i - 1] < Y->p[i - 1] then X < Y is true if and only if both + * X and Y are positive. * * Again even if we can make a decision, we just mark the result and * the fact that we are done and continue looping. */ - cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ) - & ( 1 - X_is_negative ); - *ret |= cond & ( 1 - done ); + cond = ct_lt_mpi_uint( X->p[i - 1], Y->p[i - 1] ); + *ret |= cond & ( 1 - done ) & ( 1 - X_is_negative ); done |= cond; } From a8405447aa8ee90c3f3d5e0360d7168ba9b7bb74 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Tue, 12 Nov 2019 03:34:03 -0500 Subject: [PATCH 64/95] Zeroize local AES variables before exiting the function This issue has been reported by Tuba Yavuz, Farhaan Fowze, Ken (Yihang) Bai, Grant Hernandez, and Kevin Butler (University of Florida) and Dave Tian (Purdue University). In AES encrypt and decrypt some variables were left on the stack. The value of these variables can be used to recover the last round key. To follow best practice and to limit the impact of buffer overread vulnerabilities (like Heartbleed) we need to zeroize them before exiting the function. --- ChangeLog | 8 ++++++++ library/aes.c | 24 ++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/ChangeLog b/ChangeLog index 95c0d177d..8a9e6d6de 100644 --- a/ChangeLog +++ b/ChangeLog @@ -71,6 +71,14 @@ Security blinded value, factor it (as it is smaller than RSA keys and not guaranteed to have only large prime factors), and then, by brute force, recover the key. Reported by Alejandro Cabrera Aldaya and Billy Brumley. + * Zeroize local variables in mbedtls_internal_aes_encrypt() and + mbedtls_internal_aes_decrypt() before exiting the function. The value of + these variables can be used to recover the last round key. To follow best + practice and to limit the impact of buffer overread vulnerabilities (like + Heartbleed) we need to zeroize them before exiting the function. + Issue reported by Tuba Yavuz, Farhaan Fowze, Ken (Yihang) Bai, + Grant Hernandez, and Kevin Butler (University of Florida) and + Dave Tian (Purdue University). Bugfix * Remove redundant line for getting the bitlen of a bignum, since the variable diff --git a/library/aes.c b/library/aes.c index 9098d4795..57469873f 100644 --- a/library/aes.c +++ b/library/aes.c @@ -1246,6 +1246,18 @@ int mbedtls_internal_aes_encrypt( mbedtls_aes_context *ctx, PUT_UINT32_LE( X2, output, 8 ); PUT_UINT32_LE( X3, output, 12 ); + mbedtls_platform_zeroize( &X0, sizeof( X0 ) ); + mbedtls_platform_zeroize( &X1, sizeof( X1 ) ); + mbedtls_platform_zeroize( &X2, sizeof( X2 ) ); + mbedtls_platform_zeroize( &X3, sizeof( X3 ) ); + + mbedtls_platform_zeroize( &Y0, sizeof( Y0 ) ); + mbedtls_platform_zeroize( &Y1, sizeof( Y1 ) ); + mbedtls_platform_zeroize( &Y2, sizeof( Y2 ) ); + mbedtls_platform_zeroize( &Y3, sizeof( Y3 ) ); + + mbedtls_platform_zeroize( &RK, sizeof( RK ) ); + return( 0 ); } #endif /* MBEDTLS_AES_SCA_COUNTERMEASURES */ @@ -1513,6 +1525,18 @@ int mbedtls_internal_aes_decrypt( mbedtls_aes_context *ctx, PUT_UINT32_LE( X2, output, 8 ); PUT_UINT32_LE( X3, output, 12 ); + mbedtls_platform_zeroize( &X0, sizeof( X0 ) ); + mbedtls_platform_zeroize( &X1, sizeof( X1 ) ); + mbedtls_platform_zeroize( &X2, sizeof( X2 ) ); + mbedtls_platform_zeroize( &X3, sizeof( X3 ) ); + + mbedtls_platform_zeroize( &Y0, sizeof( Y0 ) ); + mbedtls_platform_zeroize( &Y1, sizeof( Y1 ) ); + mbedtls_platform_zeroize( &Y2, sizeof( Y2 ) ); + mbedtls_platform_zeroize( &Y3, sizeof( Y3 ) ); + + mbedtls_platform_zeroize( &RK, sizeof( RK ) ); + return( 0 ); } #endif /* MBEDTLS_AES_SCA_COUNTERMEASURES */ From 8064dbb6460a6ecd551f02a73fbc0f090fcfee5b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 31 Oct 2019 15:07:08 +0100 Subject: [PATCH 65/95] Clarify that the "FATAL" message is expected The test case "Memory buffer small buffer" emits a message "FATAL: verification of first header failed". In this test case, it's actually expected, but it looks weird to see this message from a passing test. Add a comment that states this explicitly, and modify the test description to indicate that the failure is expected, and change the test function name to be more accurate. Fix #309 --- tests/suites/test_suite_memory_buffer_alloc.data | 4 ++-- tests/suites/test_suite_memory_buffer_alloc.function | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_memory_buffer_alloc.data b/tests/suites/test_suite_memory_buffer_alloc.data index d59f1135a..660ca0a37 100644 --- a/tests/suites/test_suite_memory_buffer_alloc.data +++ b/tests/suites/test_suite_memory_buffer_alloc.data @@ -16,8 +16,8 @@ memory_buffer_alloc_free_alloc:100:64:100:100:0:0:0:1:200:0 Memory buffer alloc - Out of Memory test memory_buffer_alloc_oom_test: -Memory buffer small buffer -memory_buffer_small_buffer: +Memory buffer: heap too small (header verification should fail) +memory_buffer_heap_too_small: Memory buffer underalloc memory_buffer_underalloc: diff --git a/tests/suites/test_suite_memory_buffer_alloc.function b/tests/suites/test_suite_memory_buffer_alloc.function index bc034367a..886d22b07 100644 --- a/tests/suites/test_suite_memory_buffer_alloc.function +++ b/tests/suites/test_suite_memory_buffer_alloc.function @@ -232,11 +232,14 @@ exit: /* END_CASE */ /* BEGIN_CASE depends_on:MBEDTLS_MEMORY_DEBUG */ -void memory_buffer_small_buffer( ) +void memory_buffer_heap_too_small( ) { unsigned char buf[1]; mbedtls_memory_buffer_alloc_init( buf, sizeof( buf ) ); + /* With MBEDTLS_MEMORY_DEBUG enabled, this prints a message + * "FATAL: verification of first header failed". + */ TEST_ASSERT( mbedtls_memory_buffer_alloc_verify() != 0 ); } /* END_CASE */ From 6a1ec6abeacd7540b5fda4c587e41ad105a16154 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 31 Oct 2019 15:07:35 +0100 Subject: [PATCH 66/95] More accurate test case description --- tests/suites/test_suite_memory_buffer_alloc.data | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_memory_buffer_alloc.data b/tests/suites/test_suite_memory_buffer_alloc.data index 660ca0a37..d780fd41b 100644 --- a/tests/suites/test_suite_memory_buffer_alloc.data +++ b/tests/suites/test_suite_memory_buffer_alloc.data @@ -19,5 +19,5 @@ memory_buffer_alloc_oom_test: Memory buffer: heap too small (header verification should fail) memory_buffer_heap_too_small: -Memory buffer underalloc +Memory buffer: attempt to allocate SIZE_MAX memory_buffer_underalloc: From bcdd8bcfcf7401bcfbecec5785049a214e1baa49 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 31 Oct 2019 15:07:45 +0100 Subject: [PATCH 67/95] Enable more test cases without MBEDTLS_MEMORY_DEBUG None of the test cases in tests_suite_memory_buffer_alloc actually need MBEDTLS_MEMORY_DEBUG. Some have additional checks when MBEDTLS_MEMORY_DEBUG but all are useful even without it. So enable them all and #ifdef out the parts that require DEBUG. --- .../test_suite_memory_buffer_alloc.function | 25 +++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/tests/suites/test_suite_memory_buffer_alloc.function b/tests/suites/test_suite_memory_buffer_alloc.function index 886d22b07..cc884c28e 100644 --- a/tests/suites/test_suite_memory_buffer_alloc.function +++ b/tests/suites/test_suite_memory_buffer_alloc.function @@ -29,7 +29,7 @@ void mbedtls_memory_buffer_alloc_self_test( ) } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_MEMORY_DEBUG */ +/* BEGIN_CASE */ void memory_buffer_alloc_free_alloc( int a_bytes, int b_bytes, int c_bytes, int d_bytes, int free_a, int free_b, int free_c, int free_d, int e_bytes, @@ -39,8 +39,11 @@ void memory_buffer_alloc_free_alloc( int a_bytes, int b_bytes, int c_bytes, unsigned char *ptr_a = NULL, *ptr_b = NULL, *ptr_c = NULL, *ptr_d = NULL, *ptr_e = NULL, *ptr_f = NULL; +#if defined(MBEDTLS_MEMORY_DEBUG) size_t reported_blocks; - size_t allocated_bytes = 0, reported_bytes; + size_t reported_bytes; +#endif + size_t allocated_bytes = 0; mbedtls_memory_buffer_alloc_init( buf, sizeof( buf ) ); @@ -78,8 +81,10 @@ void memory_buffer_alloc_free_alloc( int a_bytes, int b_bytes, int c_bytes, allocated_bytes += d_bytes * sizeof(char); } +#if defined(MBEDTLS_MEMORY_DEBUG) mbedtls_memory_buffer_alloc_cur_get( &reported_bytes, &reported_blocks ); TEST_ASSERT( reported_bytes == allocated_bytes ); +#endif if( free_a ) { @@ -117,8 +122,10 @@ void memory_buffer_alloc_free_alloc( int a_bytes, int b_bytes, int c_bytes, allocated_bytes -= d_bytes * sizeof(char); } +#if defined(MBEDTLS_MEMORY_DEBUG) mbedtls_memory_buffer_alloc_cur_get( &reported_bytes, &reported_blocks ); TEST_ASSERT( reported_bytes == allocated_bytes ); +#endif if( e_bytes > 0 ) { @@ -178,8 +185,10 @@ void memory_buffer_alloc_free_alloc( int a_bytes, int b_bytes, int c_bytes, ptr_f = NULL; } +#if defined(MBEDTLS_MEMORY_DEBUG) mbedtls_memory_buffer_alloc_cur_get( &reported_bytes, &reported_blocks ); TEST_ASSERT( reported_bytes == 0 ); +#endif TEST_ASSERT( mbedtls_memory_buffer_alloc_verify() == 0 ); @@ -188,12 +197,14 @@ exit: } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_MEMORY_DEBUG */ +/* BEGIN_CASE */ void memory_buffer_alloc_oom_test( ) { unsigned char buf[1024]; unsigned char *ptr_a = NULL, *ptr_b = NULL, *ptr_c = NULL; +#if defined(MBEDTLS_MEMORY_DEBUG) size_t reported_blocks, reported_bytes; +#endif (void)ptr_c; @@ -210,8 +221,10 @@ void memory_buffer_alloc_oom_test( ) ptr_c = mbedtls_calloc( 431, sizeof(char) ); TEST_ASSERT( ptr_c == NULL ); +#if defined(MBEDTLS_MEMORY_DEBUG) mbedtls_memory_buffer_alloc_cur_get( &reported_bytes, &reported_blocks ); TEST_ASSERT( reported_bytes >= 864 && reported_bytes <= sizeof(buf) ); +#endif mbedtls_free( ptr_a ); ptr_a = NULL; @@ -221,8 +234,10 @@ void memory_buffer_alloc_oom_test( ) ptr_b = NULL; TEST_ASSERT( mbedtls_memory_buffer_alloc_verify() == 0 ); +#if defined(MBEDTLS_MEMORY_DEBUG) mbedtls_memory_buffer_alloc_cur_get( &reported_bytes, &reported_blocks ); TEST_ASSERT( reported_bytes == 0 ); +#endif TEST_ASSERT( mbedtls_memory_buffer_alloc_verify() == 0 ); @@ -231,7 +246,7 @@ exit: } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_MEMORY_DEBUG */ +/* BEGIN_CASE */ void memory_buffer_heap_too_small( ) { unsigned char buf[1]; @@ -244,7 +259,7 @@ void memory_buffer_heap_too_small( ) } /* END_CASE */ -/* BEGIN_CASE depends_on:MBEDTLS_MEMORY_DEBUG */ +/* BEGIN_CASE */ void memory_buffer_underalloc( ) { unsigned char buf[100]; From 8b7f03f1721f8967ad1610b4cc6c55cf383191cd Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 28 Nov 2019 09:45:32 +0100 Subject: [PATCH 68/95] Catch AES failure in mbedtls_ctr_drbg_random The functions mbedtls_ctr_drbg_random() and mbedtls_ctr_drbg_random_with_add() could return 0 if an AES function failed. This could only happen with alternative AES implementations (the built-in implementation of the AES functions involved never fail), typically due to a failure in a hardware accelerator. Bug reported and fix proposed by Johan Uppman Bruce and Christoffer Lauri, Sectra. --- ChangeLog | 8 ++++++++ library/ctr_drbg.c | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 8a9e6d6de..edd89f6ef 100644 --- a/ChangeLog +++ b/ChangeLog @@ -79,6 +79,14 @@ Security Issue reported by Tuba Yavuz, Farhaan Fowze, Ken (Yihang) Bai, Grant Hernandez, and Kevin Butler (University of Florida) and Dave Tian (Purdue University). + * Fix side channel vulnerability in ECDSA key generation. Obtaining precise + timings on the comparison in the key generation enabled the attacker to + learn leading bits of the ephemeral key used during ECDSA signatures and to + recover the private key. Reported by Jeremy Dubeuf. + * Catch failure of AES functions in mbedtls_ctr_drbg_random(). Uncaught + failures could happen with alternative implementations of AES. Bug + reported and fix proposed by Johan Uppman Bruce and Christoffer Lauri, + Sectra. Bugfix * Remove redundant line for getting the bitlen of a bignum, since the variable diff --git a/library/ctr_drbg.c b/library/ctr_drbg.c index 4e4705825..1c71288c3 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -512,7 +512,7 @@ int mbedtls_ctr_drbg_random_with_add( void *p_rng, exit: mbedtls_platform_zeroize( add_input, sizeof( add_input ) ); mbedtls_platform_zeroize( tmp, sizeof( tmp ) ); - return( 0 ); + return( ret ); } int mbedtls_ctr_drbg_random( void *p_rng, unsigned char *output, size_t output_len ) From 10a7f626d97d5c543418730ea08f0169c521c748 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 2 Jan 2020 11:58:00 +0100 Subject: [PATCH 69/95] Add test for record compression in ssl-opt.sh Deprecated but still needs to be tested. --- tests/ssl-opt.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 00ed39150..784fedfec 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1162,6 +1162,18 @@ run_test "Default (compression enabled)" \ -S "error" \ -C "error" +requires_config_enabled MBEDTLS_ZLIB_SUPPORT +run_test "Default (compression enabled)" \ + "$P_SRV debug_level=3" \ + "$P_CLI debug_level=3" \ + 0 \ + -s "Allocating compression buffer" \ + -c "Allocating compression buffer" \ + -s "Record expansion is unknown (compression)" \ + -c "Record expansion is unknown (compression)" \ + -S "error" \ + -C "error" + # Test current time in ServerHello requires_config_enabled MBEDTLS_HAVE_TIME run_test "ServerHello contains gmt_unix_time" \ From e7b49d3cd11a1628ebe4a4cf0534b50f3e1ad408 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 20 Jan 2020 14:32:57 +0000 Subject: [PATCH 70/95] Bump version to Mbed TLS 2.16.4 --- doxygen/input/doc_mainpage.h | 2 +- doxygen/mbedtls.doxyfile | 2 +- include/mbedtls/version.h | 8 ++++---- library/CMakeLists.txt | 6 +++--- tests/suites/test_suite_version.data | 4 ++-- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/doxygen/input/doc_mainpage.h b/doxygen/input/doc_mainpage.h index a6126f3be..de43cdfb5 100644 --- a/doxygen/input/doc_mainpage.h +++ b/doxygen/input/doc_mainpage.h @@ -24,7 +24,7 @@ */ /** - * @mainpage mbed TLS v2.16.3 source code documentation + * @mainpage mbed TLS v2.16.4 source code documentation * * This documentation describes the internal structure of mbed TLS. It was * automatically generated from specially formatted comment blocks in diff --git a/doxygen/mbedtls.doxyfile b/doxygen/mbedtls.doxyfile index 3fcc034e1..574552080 100644 --- a/doxygen/mbedtls.doxyfile +++ b/doxygen/mbedtls.doxyfile @@ -28,7 +28,7 @@ DOXYFILE_ENCODING = UTF-8 # identify the project. Note that if you do not use Doxywizard you need # to put quotes around the project name if it contains spaces. -PROJECT_NAME = "mbed TLS v2.16.3" +PROJECT_NAME = "mbed TLS v2.16.4" # The PROJECT_NUMBER tag can be used to enter a project or revision number. # This could be handy for archiving the generated documentation or diff --git a/include/mbedtls/version.h b/include/mbedtls/version.h index b4eef71e5..aeffb1669 100644 --- a/include/mbedtls/version.h +++ b/include/mbedtls/version.h @@ -40,16 +40,16 @@ */ #define MBEDTLS_VERSION_MAJOR 2 #define MBEDTLS_VERSION_MINOR 16 -#define MBEDTLS_VERSION_PATCH 3 +#define MBEDTLS_VERSION_PATCH 4 /** * The single version number has the following structure: * MMNNPP00 * Major version | Minor version | Patch version */ -#define MBEDTLS_VERSION_NUMBER 0x02100300 -#define MBEDTLS_VERSION_STRING "2.16.3" -#define MBEDTLS_VERSION_STRING_FULL "mbed TLS 2.16.3" +#define MBEDTLS_VERSION_NUMBER 0x02100400 +#define MBEDTLS_VERSION_STRING "2.16.4" +#define MBEDTLS_VERSION_STRING_FULL "mbed TLS 2.16.4" #if defined(MBEDTLS_VERSION_C) diff --git a/library/CMakeLists.txt b/library/CMakeLists.txt index 0156856db..d5e34eb1b 100644 --- a/library/CMakeLists.txt +++ b/library/CMakeLists.txt @@ -162,15 +162,15 @@ endif(USE_STATIC_MBEDTLS_LIBRARY) if(USE_SHARED_MBEDTLS_LIBRARY) add_library(mbedcrypto SHARED ${src_crypto}) - set_target_properties(mbedcrypto PROPERTIES VERSION 2.16.3 SOVERSION 3) + set_target_properties(mbedcrypto PROPERTIES VERSION 2.16.4 SOVERSION 3) target_link_libraries(mbedcrypto ${libs}) add_library(mbedx509 SHARED ${src_x509}) - set_target_properties(mbedx509 PROPERTIES VERSION 2.16.3 SOVERSION 0) + set_target_properties(mbedx509 PROPERTIES VERSION 2.16.4 SOVERSION 0) target_link_libraries(mbedx509 ${libs} mbedcrypto) add_library(mbedtls SHARED ${src_tls}) - set_target_properties(mbedtls PROPERTIES VERSION 2.16.3 SOVERSION 12) + set_target_properties(mbedtls PROPERTIES VERSION 2.16.4 SOVERSION 12) target_link_libraries(mbedtls ${libs} mbedx509) install(TARGETS mbedtls mbedx509 mbedcrypto diff --git a/tests/suites/test_suite_version.data b/tests/suites/test_suite_version.data index c3189c82c..f7dd90cd9 100644 --- a/tests/suites/test_suite_version.data +++ b/tests/suites/test_suite_version.data @@ -1,8 +1,8 @@ Check compiletime library version -check_compiletime_version:"2.16.3" +check_compiletime_version:"2.16.4" Check runtime library version -check_runtime_version:"2.16.3" +check_runtime_version:"2.16.4" Check for MBEDTLS_VERSION_C check_feature:"MBEDTLS_VERSION_C":0 From 140f50206e9d901da83047ee2ec18c925b694f47 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 21 Jan 2020 16:56:03 +0100 Subject: [PATCH 71/95] Add missing return code check on call to mbedtls_md() --- library/x509write_csr.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/x509write_csr.c b/library/x509write_csr.c index 623db76f5..5a5bc6e8d 100644 --- a/library/x509write_csr.c +++ b/library/x509write_csr.c @@ -227,7 +227,9 @@ int mbedtls_x509write_csr_der( mbedtls_x509write_csr *ctx, unsigned char *buf, s /* * Prepare signature */ - mbedtls_md( mbedtls_md_info_from_type( ctx->md_alg ), c, len, hash ); + ret = mbedtls_md( mbedtls_md_info_from_type( ctx->md_alg ), c, len, hash ); + if( ret != 0 ) + return( ret ); if( ( ret = mbedtls_pk_sign( ctx->key, ctx->md_alg, hash, 0, sig, &sig_len, f_rng, p_rng ) ) != 0 ) From c0213a91abe4419743cd81bac4bd9a4d5bd03608 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 22 Jan 2020 19:04:19 +0100 Subject: [PATCH 72/95] Add changelog entry for the unchecked mbedtls_md call --- ChangeLog | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ChangeLog b/ChangeLog index edd89f6ef..79f596d0a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -65,6 +65,11 @@ Changes = mbed TLS 2.16.x branch released xxxx-xx-xx +Bugfix + * Fix an unchecked call to mbedtls_md() in the x509write module. + += mbed TLS 2.16.4 branch released 2020-01-15 + Security * Fix side channel vulnerability in ECDSA. Our bignum implementation is not constant time/constant trace, so side channel attacks can retrieve the From aa377cf11131f6a546c43e58726600de034e031b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Fri, 24 Jan 2020 12:11:56 +0100 Subject: [PATCH 73/95] Fix incrementing pointer instead of value This was introduced by a hasty search-and-replace that didn't account for C's operator precedence when changing those variables to pointer types. --- ChangeLog | 10 +++++++++- library/ecdsa.c | 4 ++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index 79f596d0a..60018b557 100644 --- a/ChangeLog +++ b/ChangeLog @@ -63,7 +63,15 @@ Changes * Reduce RAM consumption during session renegotiation by not storing the peer CRT chain and session ticket twice. -= mbed TLS 2.16.x branch released xxxx-xx-xx += mbed TLS 2.16.5 branch released xxxx-xx-xx + +Security + * Fix potential memory overread when performing an ECDSA signature + operation. The overread only happens with cryptographically low + probability (of the order of 2^-n where n is the bitsize of the curve) + unless the RNG is broken, and could result in information disclosure or + denial of service (application crash or extra resource consumption). + Reported by Peter and Auke (found using static analysis). Bugfix * Fix an unchecked call to mbedtls_md() in the x509write module. diff --git a/library/ecdsa.c b/library/ecdsa.c index 657778594..6cfaa08fe 100644 --- a/library/ecdsa.c +++ b/library/ecdsa.c @@ -297,7 +297,7 @@ static int ecdsa_sign_restartable( mbedtls_ecp_group *grp, *p_sign_tries = 0; do { - if( *p_sign_tries++ > 10 ) + if( (*p_sign_tries)++ > 10 ) { ret = MBEDTLS_ERR_ECP_RANDOM_FAILED; goto cleanup; @@ -310,7 +310,7 @@ static int ecdsa_sign_restartable( mbedtls_ecp_group *grp, *p_key_tries = 0; do { - if( *p_key_tries++ > 10 ) + if( (*p_key_tries)++ > 10 ) { ret = MBEDTLS_ERR_ECP_RANDOM_FAILED; goto cleanup; From 010efeb5a24b9e548b6baf8f81d838f2adec0a5b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 21 Jan 2020 15:02:14 +0100 Subject: [PATCH 74/95] Remove redundant block_size validity check Check the value only once, as soon as we've obtained it. --- library/cipher.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/library/cipher.c b/library/cipher.c index 20af25609..7164741b1 100644 --- a/library/cipher.c +++ b/library/cipher.c @@ -361,6 +361,10 @@ int mbedtls_cipher_update( mbedtls_cipher_context_t *ctx, const unsigned char *i *olen = 0; block_size = mbedtls_cipher_get_block_size( ctx ); + if ( 0 == block_size ) + { + return( MBEDTLS_ERR_CIPHER_INVALID_CONTEXT ); + } if( ctx->cipher_info->mode == MBEDTLS_MODE_ECB ) { @@ -396,11 +400,6 @@ int mbedtls_cipher_update( mbedtls_cipher_context_t *ctx, const unsigned char *i } #endif - if ( 0 == block_size ) - { - return( MBEDTLS_ERR_CIPHER_INVALID_CONTEXT ); - } - if( input == output && ( ctx->unprocessed_len != 0 || ilen % block_size ) ) { @@ -459,11 +458,6 @@ int mbedtls_cipher_update( mbedtls_cipher_context_t *ctx, const unsigned char *i */ if( 0 != ilen ) { - if( 0 == block_size ) - { - return( MBEDTLS_ERR_CIPHER_INVALID_CONTEXT ); - } - /* Encryption: only cache partial blocks * Decryption w/ padding: always keep at least one whole block * Decryption w/o padding: only cache partial blocks From a48fe01f15497e5550d0b96ee8c5633e003845fa Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 21 Jan 2020 16:30:53 +0100 Subject: [PATCH 75/95] Check that mbedtls_mpi_grow succeeds --- tests/suites/test_suite_mpi.function | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index 97c338b85..bebca5a0a 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -550,8 +550,8 @@ void mbedtls_mpi_lt_mpi_ct( int size_X, char * input_X, TEST_ASSERT( mbedtls_mpi_read_string( &X, 16, input_X ) == 0 ); TEST_ASSERT( mbedtls_mpi_read_string( &Y, 16, input_Y ) == 0 ); - mbedtls_mpi_grow( &X, size_X ); - mbedtls_mpi_grow( &Y, size_Y ); + TEST_ASSERT( mbedtls_mpi_grow( &X, size_X ) == 0 ); + TEST_ASSERT( mbedtls_mpi_grow( &Y, size_Y ) == 0 ); TEST_ASSERT( mbedtls_mpi_lt_mpi_ct( &X, &Y, &ret ) == input_err ); if( input_err == 0 ) From 0f14c1584230ea61680cfc7321cc9df306d312a5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 21 Jan 2020 16:52:08 +0100 Subject: [PATCH 76/95] Add missing return code check on calls to mbedtls_md() --- tests/suites/test_suite_ecdsa.function | 4 +++- tests/suites/test_suite_pk.function | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/suites/test_suite_ecdsa.function b/tests/suites/test_suite_ecdsa.function index 4f3143f5b..9e82bb5d2 100644 --- a/tests/suites/test_suite_ecdsa.function +++ b/tests/suites/test_suite_ecdsa.function @@ -527,7 +527,9 @@ void ecdsa_write_restart( int id, char *d_str, int md_alg, TEST_ASSERT( md_info != MBEDTLS_MD_INVALID_HANDLE ); hlen = mbedtls_md_get_size( md_info ); - mbedtls_md( md_info, (const unsigned char *) msg, strlen( msg ), hash ); + TEST_ASSERT( mbedtls_md( md_info, + (const unsigned char *) msg, strlen( msg ), + hash ) == 0 ); mbedtls_ecp_set_max_ops( max_ops ); diff --git a/tests/suites/test_suite_pk.function b/tests/suites/test_suite_pk.function index 61bf95c0c..9153862c1 100644 --- a/tests/suites/test_suite_pk.function +++ b/tests/suites/test_suite_pk.function @@ -735,7 +735,9 @@ void pk_sign_verify_restart( int pk_type, int grp_id, char *d_str, TEST_ASSERT( md_info != MBEDTLS_MD_INVALID_HANDLE ); hlen = mbedtls_md_get_size( md_info ); - mbedtls_md( md_info, (const unsigned char *) msg, strlen( msg ), hash ); + TEST_ASSERT( mbedtls_md( md_info, + (const unsigned char *) msg, strlen( msg ), + hash ) == 0 ); mbedtls_ecp_set_max_ops( max_ops ); From b9082ed820815fd24d1a29d20e512c8eca23e701 Mon Sep 17 00:00:00 2001 From: Jonathan Bennett Date: Fri, 24 Jan 2020 09:12:03 -0600 Subject: [PATCH 77/95] Allow loading symlinked certificates When mbedtls_x509_crt_parse_path() checks each object in the supplied path, it only processes regular files. This change makes it also accept a symlink to a file. Fixes #3005. This was observed to be a problem on Fedora/CentOS/RHEL systems, where the ca-bundle in the default location is actually a symlink. --- library/x509_crt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/x509_crt.c b/library/x509_crt.c index af8f1d67f..b09a5be54 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -2023,7 +2023,7 @@ cleanup: goto cleanup; } - if( !S_ISREG( sb.st_mode ) ) + if( !( S_ISREG( sb.st_mode ) || S_ISLNK( sb.st_mode ) ) ) continue; // Ignore parse errors From 99999b73b14096becb70b0ed6cc2f98ba3f85bbf Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Fri, 24 Jan 2020 18:20:22 +0000 Subject: [PATCH 78/95] Add ChangeLog entry Add a ChangeLog entry for Jonathan Bennett's contribution which allows loading symlinked certificates. --- ChangeLog | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ChangeLog b/ChangeLog index 60018b557..5d77df2ce 100644 --- a/ChangeLog +++ b/ChangeLog @@ -75,6 +75,8 @@ Security Bugfix * Fix an unchecked call to mbedtls_md() in the x509write module. + * Allow loading symlinked certificates. Fixes #3005. Reported and fixed + by Jonathan Bennett via #3008. = mbed TLS 2.16.4 branch released 2020-01-15 From 7489f81be73f6bc8c00fe3447aab02b8ae65b916 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 29 Jan 2020 10:47:34 +0100 Subject: [PATCH 79/95] Fix contributor names in ChangeLog --- ChangeLog | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 5d77df2ce..a80b9751e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -71,7 +71,7 @@ Security probability (of the order of 2^-n where n is the bitsize of the curve) unless the RNG is broken, and could result in information disclosure or denial of service (application crash or extra resource consumption). - Reported by Peter and Auke (found using static analysis). + Found by Auke Zeilstra and Peter Schwabe, using static analysis. Bugfix * Fix an unchecked call to mbedtls_md() in the x509write module. From 9a5c8d4b5bc822f8ba8d63b534fe48ef24ebd365 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 29 Jan 2020 11:32:09 +0100 Subject: [PATCH 80/95] Fix previous ChangeLog merging error --- ChangeLog | 2 -- 1 file changed, 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index a80b9751e..421664c0d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -63,8 +63,6 @@ Changes * Reduce RAM consumption during session renegotiation by not storing the peer CRT chain and session ticket twice. -= mbed TLS 2.16.5 branch released xxxx-xx-xx - Security * Fix potential memory overread when performing an ECDSA signature operation. The overread only happens with cryptographically low From 32b6e6984da82637b33d5c8baf62ec66af183501 Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Wed, 29 Jan 2020 13:09:55 -0500 Subject: [PATCH 81/95] Parse RSA parameters DP, DQ and QP from PKCS1 private keys Otherwise these values are recomputed in mbedtls_rsa_deduce_crt, which currently suffers from side channel issues in the computation of QP (see https://eprint.iacr.org/2020/055). By loading the pre-computed values not only is the side channel avoided, but runtime overhead of loading RSA keys is reduced. Discussion in https://github.com/ARMmbed/mbed-crypto/issues/347 Backport of https://github.com/ARMmbed/mbed-crypto/pull/352 --- library/pkparse.c | 34 ++++++++++++++++++++++++++++++---- library/rsa.c | 11 ++++++++++- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/library/pkparse.c b/library/pkparse.c index 3f202259f..de455528a 100644 --- a/library/pkparse.c +++ b/library/pkparse.c @@ -843,14 +843,40 @@ static int pk_parse_key_pkcs1_der( mbedtls_rsa_context *rsa, goto cleanup; p += len; - /* Complete the RSA private key */ - if( ( ret = mbedtls_rsa_complete( rsa ) ) != 0 ) - goto cleanup; +#if !defined(MBEDTLS_RSA_NO_CRT) + /* + * The RSA CRT parameters DP, DQ and QP are nominally redundant, in + * that they can be easily recomputed from D, P and Q. However by + * parsing them from the PKCS1 structure it is possible to avoid + * recalculating them which both reduces the overhead of loading + * RSA private keys into memory and also avoids side channels which + * can arise when computing those values, since all of D, P, and Q + * are secret. See https://eprint.iacr.org/2020/055 for a + * description of one such attack. + */ - /* Check optional parameters */ + /* Import DP */ + if( ( ret = mbedtls_asn1_get_mpi( &p, end, &rsa->DP ) ) != 0) + goto cleanup; + + /* Import DQ */ + if( ( ret = mbedtls_asn1_get_mpi( &p, end, &rsa->DQ ) ) != 0) + goto cleanup; + + /* Import QP */ + if( ( ret = mbedtls_asn1_get_mpi( &p, end, &rsa->QP ) ) != 0) + goto cleanup; + +#else + /* Verify existance of the CRT params */ if( ( ret = mbedtls_asn1_get_mpi( &p, end, &T ) ) != 0 || ( ret = mbedtls_asn1_get_mpi( &p, end, &T ) ) != 0 || ( ret = mbedtls_asn1_get_mpi( &p, end, &T ) ) != 0 ) + goto cleanup; +#endif + + /* Complete the RSA private key */ + if( ( ret = mbedtls_rsa_complete( rsa ) ) != 0 ) goto cleanup; if( p != end ) diff --git a/library/rsa.c b/library/rsa.c index cde07e380..3f480a318 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -249,6 +249,9 @@ int mbedtls_rsa_complete( mbedtls_rsa_context *ctx ) { int ret = 0; int have_N, have_P, have_Q, have_D, have_E; +#if !defined(MBEDTLS_RSA_NO_CRT) + int have_DP, have_DQ, have_QP; +#endif int n_missing, pq_missing, d_missing, is_pub, is_priv; RSA_VALIDATE_RET( ctx != NULL ); @@ -259,6 +262,12 @@ int mbedtls_rsa_complete( mbedtls_rsa_context *ctx ) have_D = ( mbedtls_mpi_cmp_int( &ctx->D, 0 ) != 0 ); have_E = ( mbedtls_mpi_cmp_int( &ctx->E, 0 ) != 0 ); +#if !defined(MBEDTLS_RSA_NO_CRT) + have_DP = ( mbedtls_mpi_cmp_int( &ctx->DP, 0 ) != 0 ); + have_DQ = ( mbedtls_mpi_cmp_int( &ctx->DQ, 0 ) != 0 ); + have_QP = ( mbedtls_mpi_cmp_int( &ctx->QP, 0 ) != 0 ); +#endif + /* * Check whether provided parameters are enough * to deduce all others. The following incomplete @@ -324,7 +333,7 @@ int mbedtls_rsa_complete( mbedtls_rsa_context *ctx ) */ #if !defined(MBEDTLS_RSA_NO_CRT) - if( is_priv ) + if( is_priv && ! ( have_DP && have_DQ && have_QP ) ) { ret = mbedtls_rsa_deduce_crt( &ctx->P, &ctx->Q, &ctx->D, &ctx->DP, &ctx->DQ, &ctx->QP ); From 7006ca10d94d6481cca6bf335434b9e244b6c5a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 30 Jan 2020 10:58:57 +0100 Subject: [PATCH 82/95] Fix ssl-opt.sh for GnuTLS versions rejecting SHA-1 While the whole script makes (often implicit) assumptions about the version of GnuTLS used, generally speaking it should work out of the box with the version packaged on our reference testing platform, which is Ubuntu 16.04 so far. With the update from Jan 8 2020 (3.4.10-4ubuntu1.6), the patches for rejecting SHA-1 in certificate signatures were backported, so we should avoid presenting SHA-1 signed certificates to a GnuTLS peer in ssl-opt.sh. --- tests/ssl-opt.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 784fedfec..0a71cd1e0 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5393,14 +5393,14 @@ run_test "Per-version suites: TLS 1.2" \ requires_gnutls run_test "ClientHello without extensions, SHA-1 allowed" \ - "$P_SRV debug_level=3 key_file=data_files/server2.key crt_file=data_files/server2.crt" \ + "$P_SRV debug_level=3" \ "$G_CLI --priority=NORMAL:%NO_EXTENSIONS:%DISABLE_SAFE_RENEGOTIATION localhost" \ 0 \ -s "dumping 'client hello extensions' (0 bytes)" requires_gnutls run_test "ClientHello without extensions, SHA-1 forbidden in certificates on server" \ - "$P_SRV debug_level=3 key_file=data_files/server2.key crt_file=data_files/server2.crt allow_sha1=0" \ + "$P_SRV debug_level=3 allow_sha1=0" \ "$G_CLI --priority=NORMAL:%NO_EXTENSIONS:%DISABLE_SAFE_RENEGOTIATION localhost" \ 0 \ -s "dumping 'client hello extensions' (0 bytes)" From d817f540776637a59085de42484180533a0f3c14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 30 Jan 2020 12:45:14 +0100 Subject: [PATCH 83/95] De-duplicate SHA1-independent test in ssl-opt.sh The splitting of this test into two versions depending on whether SHA-1 was allowed by the server was a mistake in 5d2511c4d48eb197697466d1bd6b776cf09b0e7c - the test has nothing to do with SHA-1 in the first place, as the server doesn't request a certificate from the client so it doesn't matter if the server accepts SHA-1 or not. --- tests/ssl-opt.sh | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 0a71cd1e0..67cf50233 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -5392,19 +5392,12 @@ run_test "Per-version suites: TLS 1.2" \ # Test for ClientHello without extensions requires_gnutls -run_test "ClientHello without extensions, SHA-1 allowed" \ +run_test "ClientHello without extensions" \ "$P_SRV debug_level=3" \ "$G_CLI --priority=NORMAL:%NO_EXTENSIONS:%DISABLE_SAFE_RENEGOTIATION localhost" \ 0 \ -s "dumping 'client hello extensions' (0 bytes)" -requires_gnutls -run_test "ClientHello without extensions, SHA-1 forbidden in certificates on server" \ - "$P_SRV debug_level=3 allow_sha1=0" \ - "$G_CLI --priority=NORMAL:%NO_EXTENSIONS:%DISABLE_SAFE_RENEGOTIATION localhost" \ - 0 \ - -s "dumping 'client hello extensions' (0 bytes)" - # Tests for mbedtls_ssl_get_bytes_avail() run_test "mbedtls_ssl_get_bytes_avail: no extra data" \ From a32e45d63245ba8b45e100a4923cf6f187a1d1e6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 31 Jan 2020 12:05:53 +0100 Subject: [PATCH 84/95] Add changelog entry --- ChangeLog | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ChangeLog b/ChangeLog index 421664c0d..f52e79b0a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -70,6 +70,11 @@ Security unless the RNG is broken, and could result in information disclosure or denial of service (application crash or extra resource consumption). Found by Auke Zeilstra and Peter Schwabe, using static analysis. + * To avoid a side channel vulnerability when parsing an RSA private key, + read all the CRT parameters from the DER structure rather than + reconstructing them. Found by Alejandro Cabrera Aldaya and Billy Bob + Brumley. Reported and fix contributed by Jack Lloyd. + ARMmbed/mbed-crypto#352 Bugfix * Fix an unchecked call to mbedtls_md() in the x509write module. From 95ce7dab345e3526909be53c418414030397f7d2 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 31 Jan 2020 12:20:10 +0100 Subject: [PATCH 85/95] Fix duplicated Bugfix section in the changelog --- ChangeLog | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index f52e79b0a..09c844137 100644 --- a/ChangeLog +++ b/ChangeLog @@ -77,9 +77,9 @@ Security ARMmbed/mbed-crypto#352 Bugfix - * Fix an unchecked call to mbedtls_md() in the x509write module. * Allow loading symlinked certificates. Fixes #3005. Reported and fixed by Jonathan Bennett via #3008. + * Fix an unchecked call to mbedtls_md() in the x509write module. = mbed TLS 2.16.4 branch released 2020-01-15 From 16fca92e3d7b5cb3b7fc91c205ebc9f52dbe574a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 20 Jan 2020 17:19:03 +0100 Subject: [PATCH 86/95] Bignum copy/shrink: More precise test case descriptions --- tests/suites/test_suite_mpi.data | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index 4d46ca742..d550b21ed 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -280,37 +280,37 @@ mbedtls_mpi_cmp_abs:10:"2":10:"-3":-1 Base test mbedtls_mpi_cmp_abs (Mix values) #3 mbedtls_mpi_cmp_abs:10:"-2":10:"1":1 -Base test mbedtls_mpi_copy #1 +Copy zero (1 limb) to positive (1 limb) mbedtls_mpi_copy:0:1500 -Base test mpi_copy_self #1 +Copy self: positive (1 limb) mpi_copy_self:14 -Base test mbedtls_mpi_swap #1 +Swap 0 with positive (1 limb) mbedtls_mpi_swap:0:1500 -Test mbedtls_mpi_shrink #1 +Shrink 2 in 2 to 4 mbedtls_mpi_shrink:2:2:4:4 -Test mbedtls_mpi_shrink #2 +Shrink 2 in 4 to 4 mbedtls_mpi_shrink:4:2:4:4 -Test mbedtls_mpi_shrink #3 +Shrink 2 in 8 to 4 mbedtls_mpi_shrink:8:2:4:4 -Test mbedtls_mpi_shrink #4 +Shrink 4 in 8 to 4 mbedtls_mpi_shrink:8:4:4:4 -Test mbedtls_mpi_shrink #5 +Shrink 6 in 8 to 4 yielding 6 mbedtls_mpi_shrink:8:6:4:6 -Test mbedtls_mpi_shrink #6 +Shrink 2 in 4 to 0 yielding 2 mbedtls_mpi_shrink:4:2:0:2 -Test mbedtls_mpi_shrink #7 +Shrink 1 in 4 to 0 yielding 1 mbedtls_mpi_shrink:4:1:0:1 -Test mbedtls_mpi_shrink #8 +Shrink 0 in 4 to 0 yielding 1 mbedtls_mpi_shrink:4:0:0:1 Test mbedtls_mpi_safe_cond_assign #1 From edb621b84aa0892ca729dfbf6179be528377dcb5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 20 Jan 2020 21:01:51 +0100 Subject: [PATCH 87/95] Better coverage for copy and swap Cover more cases: different signs, different zeronesses, repeated argument. --- tests/suites/test_suite_mpi.data | 75 +++++++++++++++++++++- tests/suites/test_suite_mpi.function | 95 ++++++++++++++++++++++------ 2 files changed, 148 insertions(+), 22 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index d550b21ed..fa55e6a8d 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -281,13 +281,82 @@ Base test mbedtls_mpi_cmp_abs (Mix values) #3 mbedtls_mpi_cmp_abs:10:"-2":10:"1":1 Copy zero (1 limb) to positive (1 limb) -mbedtls_mpi_copy:0:1500 +mbedtls_mpi_copy_sint:0:1500 + +Copy zero (1 limb) to negative (1 limb) +mbedtls_mpi_copy_sint:0:-1500 + +Copy positive (1 limb) to zero (1 limb) +mbedtls_mpi_copy_sint:1500:0 + +Copy negative (1 limb) to zero (1 limb) +mbedtls_mpi_copy_sint:-1500:0 + +Copy positive (1 limb) to negative (1 limb) +mbedtls_mpi_copy_sint:1500:-42 + +Copy negative (1 limb) to positive (1 limb) +mbedtls_mpi_copy_sint:-42:1500 + +Copy zero (null) to zero (null) +mbedtls_mpi_copy_binary:"":"" + +Copy zero (null) to positive (1 limb) +mbedtls_mpi_copy_binary:"":"1234" + +Copy positive (1 limb) to zero (null) +mbedtls_mpi_copy_binary:"1234":"" + +Copy positive to larger +mbedtls_mpi_copy_binary:"bead":"ca5cadedb01dfaceacc01ade" + +Copy positive to smaller +mbedtls_mpi_copy_binary:"ca5cadedb01dfaceacc01ade":"bead" Copy self: positive (1 limb) mpi_copy_self:14 -Swap 0 with positive (1 limb) -mbedtls_mpi_swap:0:1500 +Copy self: zero (1 limb) +mpi_copy_self:0 + +Swap zero (1 limb) with positive (1 limb) +mbedtls_mpi_swap_sint:0:1500 + +Swap zero (1 limb) with negative (1 limb) +mbedtls_mpi_swap_sint:0:-1500 + +Swap positive (1 limb) with zero (1 limb) +mbedtls_mpi_swap_sint:1500:0 + +Swap negative (1 limb) with zero (1 limb) +mbedtls_mpi_swap_sint:-1500:0 + +Swap positive (1 limb) with negative (1 limb) +mbedtls_mpi_swap_sint:1500:-42 + +Swap negative (1 limb) with positive (1 limb) +mbedtls_mpi_swap_sint:-42:1500 + +Swap zero (null) with zero (null) +mbedtls_mpi_swap_binary:"":"" + +Swap zero (null) with positive (1 limb) +mbedtls_mpi_swap_binary:"":"1234" + +Swap positive (1 limb) with zero (null) +mbedtls_mpi_swap_binary:"1234":"" + +Swap positive with larger +mbedtls_mpi_swap_binary:"bead":"ca5cadedb01dfaceacc01ade" + +Swap positive with smaller +mbedtls_mpi_swap_binary:"ca5cadedb01dfaceacc01ade":"bead" + +Swap self: 1 limb +mpi_swap_self:"face" + +Swap self: null +mpi_swap_self:"" Shrink 2 in 2 to 4 mbedtls_mpi_shrink:2:2:4:4 diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index bebca5a0a..34c23fe8b 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -579,22 +579,40 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -void mbedtls_mpi_copy( int input_X, int input_A ) +void mbedtls_mpi_copy_sint( int input_X, int input_Y ) { - mbedtls_mpi X, Y, A; - mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); mbedtls_mpi_init( &A ); + mbedtls_mpi X, Y; + mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); TEST_ASSERT( mbedtls_mpi_lset( &X, input_X ) == 0 ); - TEST_ASSERT( mbedtls_mpi_lset( &Y, input_A ) == 0 ); - TEST_ASSERT( mbedtls_mpi_lset( &A, input_A ) == 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &Y ) != 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Y, &A ) == 0 ); + TEST_ASSERT( mbedtls_mpi_lset( &Y, input_Y ) == 0 ); + TEST_ASSERT( mbedtls_mpi_copy( &Y, &X ) == 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &Y ) == 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Y, &A ) != 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &X, input_X ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &Y, input_X ) == 0 ); exit: - mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); mbedtls_mpi_free( &A ); + mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); +} +/* END_CASE */ + +/* BEGIN_CASE */ +void mbedtls_mpi_copy_binary( data_t *input_X, data_t *input_Y ) +{ + mbedtls_mpi X, Y, X0; + mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); mbedtls_mpi_init( &X0 ); + + TEST_ASSERT( mbedtls_mpi_read_binary_le( &X, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary_le( &Y, input_Y->x, input_Y->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary_le( &X0, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &X0 ) == 0 ); + + TEST_ASSERT( mbedtls_mpi_copy( &Y, &X ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &X0 ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Y, &X0 ) == 0 ); + +exit: + mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); mbedtls_mpi_free( &X0 ); } /* END_CASE */ @@ -686,22 +704,61 @@ exit: /* END_CASE */ /* BEGIN_CASE */ -void mbedtls_mpi_swap( int input_X, int input_Y ) +void mbedtls_mpi_swap_sint( int input_X, int input_Y ) { - mbedtls_mpi X, Y, A; - mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); mbedtls_mpi_init( &A ); + mbedtls_mpi X, Y; + mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); TEST_ASSERT( mbedtls_mpi_lset( &X, input_X ) == 0 ); TEST_ASSERT( mbedtls_mpi_lset( &Y, input_Y ) == 0 ); - TEST_ASSERT( mbedtls_mpi_lset( &A, input_X ) == 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &Y ) != 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &A ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &X, input_X ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &Y, input_Y ) == 0 ); + mbedtls_mpi_swap( &X, &Y ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &Y ) != 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Y, &A ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &X, input_Y ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_int( &Y, input_X ) == 0 ); exit: - mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); mbedtls_mpi_free( &A ); + mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); +} +/* END_CASE */ + +/* BEGIN_CASE */ +void mbedtls_mpi_swap_binary( data_t *input_X, data_t *input_Y ) +{ + mbedtls_mpi X, Y, X0, Y0; + mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); + mbedtls_mpi_init( &X0 ); mbedtls_mpi_init( &Y0 ); + + TEST_ASSERT( mbedtls_mpi_read_binary_le( &X, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary_le( &Y, input_Y->x, input_Y->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary_le( &X0, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary_le( &Y0, input_Y->x, input_Y->len ) == 0 ); + + mbedtls_mpi_swap( &X, &Y ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &Y0 ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &Y, &X0 ) == 0 ); + +exit: + mbedtls_mpi_free( &X ); mbedtls_mpi_free( &Y ); + mbedtls_mpi_free( &X0 ); mbedtls_mpi_free( &Y0 ); +} +/* END_CASE */ + +/* BEGIN_CASE */ +void mpi_swap_self( data_t *input_X ) +{ + mbedtls_mpi X, X0; + mbedtls_mpi_init( &X ); mbedtls_mpi_init( &X0 ); + + TEST_ASSERT( mbedtls_mpi_read_binary_le( &X, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary_le( &X0, input_X->x, input_X->len ) == 0 ); + + mbedtls_mpi_swap( &X, &X ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &X0 ) == 0 ); + +exit: + mbedtls_mpi_free( &X ); mbedtls_mpi_free( &X0 ); } /* END_CASE */ From 51c2e06eb85bf18de30d327b25c0d58e74f5de3d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 20 Jan 2020 21:12:50 +0100 Subject: [PATCH 88/95] mpi_copy: make the 0 case slightly more robust If Y was constructed through functions in this module, then Y->n == 0 iff Y->p == NULL. However we do not prevent filling mpi structures manually, and zero may be represented with n=0 and p a valid pointer. Most of the code can cope with such a representation, but for the source of mbedtls_mpi_copy, this would cause an integer underflow. Changing the test for zero from Y->p==NULL to Y->n==0 causes this case to work at no extra cost. --- library/bignum.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index a0964e348..0aee14924 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -198,7 +198,7 @@ int mbedtls_mpi_copy( mbedtls_mpi *X, const mbedtls_mpi *Y ) if( X == Y ) return( 0 ); - if( Y->p == NULL ) + if( Y->n == 0 ) { mbedtls_mpi_free( X ); return( 0 ); From 0660747057ac13e7f0be299a2dda24d3e2138977 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 20 Jan 2020 21:17:43 +0100 Subject: [PATCH 89/95] Improve comments in mpi_shrink --- library/bignum.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index 0aee14924..36b04852b 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -157,9 +157,10 @@ int mbedtls_mpi_shrink( mbedtls_mpi *X, size_t nblimbs ) if( nblimbs > MBEDTLS_MPI_MAX_LIMBS ) return( MBEDTLS_ERR_MPI_ALLOC_FAILED ); - /* Actually resize up in this case */ + /* Actually resize up if there are currently fewer than nblimbs limbs. */ if( X->n <= nblimbs ) return( mbedtls_mpi_grow( X, nblimbs ) ); + /* Now X->n > nblimbs >= 0. */ for( i = X->n - 1; i > 0; i-- ) if( X->p[i] != 0 ) From 8830bd24476f7e62dfe7a43a703a147d021f415d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 21 Jan 2020 13:59:51 +0100 Subject: [PATCH 90/95] Minor comment improvement --- library/bignum.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/bignum.c b/library/bignum.c index 36b04852b..00df10d98 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -160,7 +160,7 @@ int mbedtls_mpi_shrink( mbedtls_mpi *X, size_t nblimbs ) /* Actually resize up if there are currently fewer than nblimbs limbs. */ if( X->n <= nblimbs ) return( mbedtls_mpi_grow( X, nblimbs ) ); - /* Now X->n > nblimbs >= 0. */ + /* After this point, then X->n > nblimbs and in particular X->n > 0. */ for( i = X->n - 1; i > 0; i-- ) if( X->p[i] != 0 ) From 7313e2caff871d5942ea0246c4062896181585eb Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 3 Feb 2020 16:15:47 +0100 Subject: [PATCH 91/95] Move test functions from Lilliput to Blefuscu We normally represent bignums in big-endian order and there is no reason to deviate here. --- tests/suites/test_suite_mpi.function | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/suites/test_suite_mpi.function b/tests/suites/test_suite_mpi.function index 34c23fe8b..f2702f12b 100644 --- a/tests/suites/test_suite_mpi.function +++ b/tests/suites/test_suite_mpi.function @@ -602,9 +602,9 @@ void mbedtls_mpi_copy_binary( data_t *input_X, data_t *input_Y ) mbedtls_mpi X, Y, X0; mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); mbedtls_mpi_init( &X0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &X, input_X->x, input_X->len ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &Y, input_Y->x, input_Y->len ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &X0, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &X, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &Y, input_Y->x, input_Y->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &X0, input_X->x, input_X->len ) == 0 ); TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &X0 ) == 0 ); TEST_ASSERT( mbedtls_mpi_copy( &Y, &X ) == 0 ); @@ -730,10 +730,10 @@ void mbedtls_mpi_swap_binary( data_t *input_X, data_t *input_Y ) mbedtls_mpi_init( &X ); mbedtls_mpi_init( &Y ); mbedtls_mpi_init( &X0 ); mbedtls_mpi_init( &Y0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &X, input_X->x, input_X->len ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &Y, input_Y->x, input_Y->len ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &X0, input_X->x, input_X->len ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &Y0, input_Y->x, input_Y->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &X, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &Y, input_Y->x, input_Y->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &X0, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &Y0, input_Y->x, input_Y->len ) == 0 ); mbedtls_mpi_swap( &X, &Y ); TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &Y0 ) == 0 ); @@ -751,8 +751,8 @@ void mpi_swap_self( data_t *input_X ) mbedtls_mpi X, X0; mbedtls_mpi_init( &X ); mbedtls_mpi_init( &X0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &X, input_X->x, input_X->len ) == 0 ); - TEST_ASSERT( mbedtls_mpi_read_binary_le( &X0, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &X, input_X->x, input_X->len ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &X0, input_X->x, input_X->len ) == 0 ); mbedtls_mpi_swap( &X, &X ); TEST_ASSERT( mbedtls_mpi_cmp_mpi( &X, &X0 ) == 0 ); From f5faa25cf422fa8f6ae8a7b9c721e02cfe9d97ca Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 3 Feb 2020 16:18:30 +0100 Subject: [PATCH 92/95] shrink tests: clearer description --- tests/suites/test_suite_mpi.data | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/suites/test_suite_mpi.data b/tests/suites/test_suite_mpi.data index fa55e6a8d..00926ff97 100644 --- a/tests/suites/test_suite_mpi.data +++ b/tests/suites/test_suite_mpi.data @@ -358,28 +358,28 @@ mpi_swap_self:"face" Swap self: null mpi_swap_self:"" -Shrink 2 in 2 to 4 +Shrink 2 limbs in a buffer of size 2 to 4 mbedtls_mpi_shrink:2:2:4:4 -Shrink 2 in 4 to 4 +Shrink 2 limbs in a buffer of size 4 to 4 mbedtls_mpi_shrink:4:2:4:4 -Shrink 2 in 8 to 4 +Shrink 2 limbs in a buffer of size 8 to 4 mbedtls_mpi_shrink:8:2:4:4 -Shrink 4 in 8 to 4 +Shrink 4 limbs in a buffer of size 8 to 4 mbedtls_mpi_shrink:8:4:4:4 -Shrink 6 in 8 to 4 yielding 6 +Shrink 6 limbs in a buffer of size 8 to 4 yielding 6 mbedtls_mpi_shrink:8:6:4:6 -Shrink 2 in 4 to 0 yielding 2 +Shrink 2 limbs in a buffer of size 4 to 0 yielding 2 mbedtls_mpi_shrink:4:2:0:2 -Shrink 1 in 4 to 0 yielding 1 +Shrink 1 limbs in a buffer of size 4 to 0 yielding 1 mbedtls_mpi_shrink:4:1:0:1 -Shrink 0 in 4 to 0 yielding 1 +Shrink 0 limbs in a buffer of size 4 to 0 yielding 1 mbedtls_mpi_shrink:4:0:0:1 Test mbedtls_mpi_safe_cond_assign #1 From 06c1e239609c89c4769d42f3b4b991bf8c824475 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 20 Sep 2019 19:23:10 +0200 Subject: [PATCH 93/95] Replace -O0 by -O1 or -Os in most components Gcc skips some analyses when compiling with -O0, so we may miss warnings about things like uninitialized variables. --- tests/scripts/all.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 91b326ec8..9cb3ec1da 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1109,8 +1109,8 @@ component_test_no_platform () { scripts/config.pl unset MBEDTLS_FS_IO # Note, _DEFAULT_SOURCE needs to be defined for platforms using glibc version >2.19, # to re-enable platform integration features otherwise disabled in C99 builds - make CC=gcc PTHREAD=1 CFLAGS='-Werror -Wall -Wextra -std=c99 -pedantic -O0 -D_DEFAULT_SOURCE' lib programs - make CC=gcc PTHREAD=1 CFLAGS='-Werror -Wall -Wextra -O0' test + make CC=gcc PTHREAD=1 CFLAGS='-Werror -Wall -Wextra -std=c99 -pedantic -Os -D_DEFAULT_SOURCE' lib programs + make CC=gcc PTHREAD=1 CFLAGS='-Werror -Wall -Wextra -Os' test } component_build_no_std_function () { @@ -1119,21 +1119,21 @@ component_build_no_std_function () { scripts/config.pl full scripts/config.pl set MBEDTLS_PLATFORM_NO_STD_FUNCTIONS scripts/config.pl unset MBEDTLS_ENTROPY_NV_SEED - make CC=gcc PTHREAD=1 CFLAGS='-Werror -Wall -Wextra -O0' + make CC=gcc PTHREAD=1 CFLAGS='-Werror -Wall -Wextra -Os' } component_build_no_ssl_srv () { msg "build: full config except ssl_srv.c, make, gcc" # ~ 30s scripts/config.pl full scripts/config.pl unset MBEDTLS_SSL_SRV_C - make CC=gcc PTHREAD=1 CFLAGS='-Werror -Wall -Wextra -O0' + make CC=gcc PTHREAD=1 CFLAGS='-Werror -Wall -Wextra -O1' } component_build_no_ssl_cli () { msg "build: full config except ssl_cli.c, make, gcc" # ~ 30s scripts/config.pl full scripts/config.pl unset MBEDTLS_SSL_CLI_C - make CC=gcc PTHREAD=1 CFLAGS='-Werror -Wall -Wextra -O0' + make CC=gcc PTHREAD=1 CFLAGS='-Werror -Wall -Wextra -O1' } component_build_no_sockets () { @@ -1143,7 +1143,7 @@ component_build_no_sockets () { scripts/config.pl full scripts/config.pl unset MBEDTLS_NET_C # getaddrinfo() undeclared, etc. scripts/config.pl set MBEDTLS_NO_PLATFORM_ENTROPY # uses syscall() on GNU/Linux - make CC=gcc PTHREAD=1 CFLAGS='-Werror -Wall -Wextra -O0 -std=c99 -pedantic' lib + make CC=gcc PTHREAD=1 CFLAGS='-Werror -Wall -Wextra -O1 -std=c99 -pedantic' lib } component_test_memory_buffer_allocator_backtrace () { From a4c1c4b55d1884fef8dad38a3510020949a5d630 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 20 Sep 2019 19:56:06 +0200 Subject: [PATCH 94/95] Test GCC and Clang with common build options Goals: * Build with common compilers with common options, so that we don't miss a (potentially useful) warning only triggered with certain build options. * A previous commit removed -O0 test jobs, leaving only the one with -m32. We have inline assembly that is disabled with -O0, falling back to generic C code. This commit restores a test that runs the generic C code on a 64-bit platform. --- tests/scripts/all.sh | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 9cb3ec1da..ae2e11283 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1375,6 +1375,30 @@ component_test_cmake_shared () { make test } +test_build_opt () { + info=$1 cc=$2; shift 2 + for opt in "$@"; do + msg "build/test: $cc $opt, $info" # ~ 30s + make CC="$cc" CFLAGS="$opt -Wall -Wextra -Werror" + # We're confident enough in compilers to not run _all_ the tests, + # but at least run the unit tests. In particular, runs with + # optimizations use inline assembly whereas runs with -O0 + # skip inline assembly. + make test # ~30s + make clean + done +} + +component_test_clang_opt () { + scripts/config.pl full + test_build_opt 'full config' clang -O0 -Os -O2 +} + +component_test_gcc_opt () { + scripts/config.pl full + test_build_opt 'full config' gcc -O0 -Os -O2 +} + component_build_mbedtls_config_file () { msg "build: make with MBEDTLS_CONFIG_FILE" # ~40s # Use the full config so as to catch a maximum of places where From f3a13486f44fa4ae5261f90397ce963c58f71fff Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 4 Feb 2020 14:42:15 +0000 Subject: [PATCH 95/95] Revert "Merge pull request #3012 from Patater/dev/jp-bennett/development-2.16" This reverts commit 7550e857bf85bc169271b9edefb1e8ee04bc3042, reversing changes made to d0c25753241b0ea2b120bfa506d558f76c8c1430. stat() will never return S_IFLNK as the file type, as stat() explicitly follows symlinks. Fixes #3005. --- ChangeLog | 4 ++-- library/x509_crt.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index 09c844137..2e9bf1137 100644 --- a/ChangeLog +++ b/ChangeLog @@ -63,6 +63,8 @@ Changes * Reduce RAM consumption during session renegotiation by not storing the peer CRT chain and session ticket twice. += mbed TLS 2.16.X branch released XXXX-XX-XX + Security * Fix potential memory overread when performing an ECDSA signature operation. The overread only happens with cryptographically low @@ -77,8 +79,6 @@ Security ARMmbed/mbed-crypto#352 Bugfix - * Allow loading symlinked certificates. Fixes #3005. Reported and fixed - by Jonathan Bennett via #3008. * Fix an unchecked call to mbedtls_md() in the x509write module. = mbed TLS 2.16.4 branch released 2020-01-15 diff --git a/library/x509_crt.c b/library/x509_crt.c index b09a5be54..af8f1d67f 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -2023,7 +2023,7 @@ cleanup: goto cleanup; } - if( !( S_ISREG( sb.st_mode ) || S_ISLNK( sb.st_mode ) ) ) + if( !S_ISREG( sb.st_mode ) ) continue; // Ignore parse errors