From b0ef434911127cfe4681d2394c5d571fe486b80d Mon Sep 17 00:00:00 2001 From: Ron Eldor Date: Sun, 1 Apr 2018 17:43:26 +0300 Subject: [PATCH 01/79] Add doxygen.sh script to git hooks Add the doxygen.sh script to the pre-push git script --- tests/git-scripts/pre-push.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/git-scripts/pre-push.sh b/tests/git-scripts/pre-push.sh index 7407f44b97..86edf5a305 100755 --- a/tests/git-scripts/pre-push.sh +++ b/tests/git-scripts/pre-push.sh @@ -46,3 +46,4 @@ run_test ./tests/scripts/check-doxy-blocks.pl run_test ./tests/scripts/check-names.sh run_test ./tests/scripts/check-generated-files.sh run_test ./tests/scripts/check-files.py +run_test ./tests/scripts/doxygen.sh From 65e619a1fa3503628a30c8196049d1ee431c45f7 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 23 Aug 2018 15:46:33 +0100 Subject: [PATCH 02/79] Improve documentation of mbedtls_x509_crt_parse() Fixes #1883. --- include/mbedtls/x509_crt.h | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/include/mbedtls/x509_crt.h b/include/mbedtls/x509_crt.h index d41ec93a66..4be6173595 100644 --- a/include/mbedtls/x509_crt.h +++ b/include/mbedtls/x509_crt.h @@ -175,19 +175,34 @@ int mbedtls_x509_crt_parse_der( mbedtls_x509_crt *chain, const unsigned char *bu size_t buflen ); /** - * \brief Parse one or more certificates and add them - * to the chained list. Parses permissively. If some - * certificates can be parsed, the result is the number - * of failed certificates it encountered. If none complete - * correctly, the first error is returned. + * \brief Parse one DER-encoded or multiple concatenated PEM-encoded + * certificates and add them to the chained list. * - * \param chain points to the start of the chain - * \param buf buffer holding the certificate data in PEM or DER format - * \param buflen size of the buffer - * (including the terminating null byte for PEM data) + * For PEM-encoded CRTs, the function parses permissively: + * If at least one certificate can be parsed, the function + * returns the number of certificates for which parsing failed + * (hence \c 0 if all certificates were parsed successfully). + * If no certificate could be parsed, the function returns + * the first (negative) error encountered during parsing. + * + * PEM encoded certificates may be interleaved by other data + * such as human readable descriptions of their content, as + * long as the certificates are enclosed in the PEM specific + * '-----{BEGIN/END} CERTIFICATE-----' delimiters. + * + * \param chain The chain to which to add the parsed certificates. + * \param buf The buffer holding the certificate data in PEM or DER format. + * For certificates in PEM encoding, this may be a concatenation + * of multiple certificates; for DER encoding, the buffer must + * comprise exactly one certificate. + * \param buflen The size of \p buf, including the terminating \c NULL byte + * in case of PEM encoded data. + * + * \return \c 0 if all certificates were parsed successfully. + * \return The (positive) number of certificates that couldn't + * be parsed if parsing was partly successful (see above). + * \return A negative X509- or PEM-specific error code otherwise. * - * \return 0 if all certificates parsed successfully, a positive number - * if partly successful or a specific X509 or PEM error code */ int mbedtls_x509_crt_parse( mbedtls_x509_crt *chain, const unsigned char *buf, size_t buflen ); From 89a91121df12e9e76199544af805221f2a1fda1b Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 23 Aug 2018 16:14:00 +0100 Subject: [PATCH 03/79] Improve wording --- include/mbedtls/x509_crt.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/x509_crt.h b/include/mbedtls/x509_crt.h index 4be6173595..9227701d31 100644 --- a/include/mbedtls/x509_crt.h +++ b/include/mbedtls/x509_crt.h @@ -175,7 +175,7 @@ int mbedtls_x509_crt_parse_der( mbedtls_x509_crt *chain, const unsigned char *bu size_t buflen ); /** - * \brief Parse one DER-encoded or multiple concatenated PEM-encoded + * \brief Parse one DER-encoded or one or more concatenated PEM-encoded * certificates and add them to the chained list. * * For PEM-encoded CRTs, the function parses permissively: @@ -201,7 +201,7 @@ int mbedtls_x509_crt_parse_der( mbedtls_x509_crt *chain, const unsigned char *bu * \return \c 0 if all certificates were parsed successfully. * \return The (positive) number of certificates that couldn't * be parsed if parsing was partly successful (see above). - * \return A negative X509- or PEM-specific error code otherwise. + * \return A negative X509 or PEM error code otherwise. * */ int mbedtls_x509_crt_parse( mbedtls_x509_crt *chain, const unsigned char *buf, size_t buflen ); From e8658e28930326b3ea8bf5e835f06061d7efce4a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 24 Aug 2018 10:01:17 +0100 Subject: [PATCH 04/79] Improve documentation of mbedtls_x509_crt_parse() --- include/mbedtls/x509_crt.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/x509_crt.h b/include/mbedtls/x509_crt.h index 9227701d31..afe886e59f 100644 --- a/include/mbedtls/x509_crt.h +++ b/include/mbedtls/x509_crt.h @@ -178,8 +178,8 @@ int mbedtls_x509_crt_parse_der( mbedtls_x509_crt *chain, const unsigned char *bu * \brief Parse one DER-encoded or one or more concatenated PEM-encoded * certificates and add them to the chained list. * - * For PEM-encoded CRTs, the function parses permissively: - * If at least one certificate can be parsed, the function + * For CRTs in PEM encoding, the function parses permissively: + * if at least one certificate can be parsed, the function * returns the number of certificates for which parsing failed * (hence \c 0 if all certificates were parsed successfully). * If no certificate could be parsed, the function returns From a86de14fcad994d31306d62a6e42cac76540ed45 Mon Sep 17 00:00:00 2001 From: Simon Butcher Date: Sun, 30 Sep 2018 12:09:47 +0100 Subject: [PATCH 05/79] Strip trailing whitespace in bn_mul.h Remove the trailing whitespace from the inline assembly for AMD64 target, to overcome a warning in Clang, which was objecting to the string literal generated by the inline assembly being greater than 4096 characters specified by the ISO C99 standard. (-Woverlength-strings) This is a cosmetic change and doesn't change the logic of the code in any way. This change only fixes the problem for AMD64 target, and leaves other targets as they are. Fixes #482. --- include/mbedtls/bn_mul.h | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/include/mbedtls/bn_mul.h b/include/mbedtls/bn_mul.h index b587317d95..80e4b380d1 100644 --- a/include/mbedtls/bn_mul.h +++ b/include/mbedtls/bn_mul.h @@ -170,19 +170,19 @@ #define MULADDC_INIT \ asm( \ - "xorq %%r8, %%r8 \n\t" + "xorq %%r8, %%r8\n" #define MULADDC_CORE \ - "movq (%%rsi), %%rax \n\t" \ - "mulq %%rbx \n\t" \ - "addq $8, %%rsi \n\t" \ - "addq %%rcx, %%rax \n\t" \ - "movq %%r8, %%rcx \n\t" \ - "adcq $0, %%rdx \n\t" \ - "nop \n\t" \ - "addq %%rax, (%%rdi) \n\t" \ - "adcq %%rdx, %%rcx \n\t" \ - "addq $8, %%rdi \n\t" + "movq (%%rsi), %%rax\n" \ + "mulq %%rbx\n" \ + "addq $8, %%rsi\n" \ + "addq %%rcx, %%rax\n" \ + "movq %%r8, %%rcx\n" \ + "adcq $0, %%rdx\n" \ + "nop \n" \ + "addq %%rax, (%%rdi)\n" \ + "adcq %%rdx, %%rcx\n" \ + "addq $8, %%rdi\n" #define MULADDC_STOP \ : "+c" (c), "+D" (d), "+S" (s) \ From df0500d7bcd9c2a7e4d2028ce9d3f7a6b80df3ae Mon Sep 17 00:00:00 2001 From: Simon Butcher Date: Sun, 30 Sep 2018 12:27:26 +0100 Subject: [PATCH 06/79] Add Changelog entry for #482 Add Changelog entry for inline assembly/literal strings too long issue with Clang. --- ChangeLog | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ChangeLog b/ChangeLog index 513f24f3ab..13604e8c93 100644 --- a/ChangeLog +++ b/ChangeLog @@ -7,6 +7,9 @@ Bugfix invalidated keys of a lifetime of less than a 1s. Fixes #1968. * Fix failure in hmac_drbg in the benchmark sample application, when MBEDTLS_THREADING_C is defined. Found by TrinityTonic, #1095 + * Fix for Clang, which was reporting a warning for the bignum.c inline + assembly for AMD64 targets creating string literals greater than those + permitted by the ISO C99 standard. Found by Aaron Jones. Fixes #482. Changes * Add tests for session resumption in DTLS. From 5908dd44558e1ea36b95a89056597e1d055fe2e4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 2 Oct 2018 22:43:06 +0200 Subject: [PATCH 07/79] Minor readability improvement Polish the beginning of mbedtls_rsa_rsaes_pkcs1_v15_decrypt a little, to prepare for some behavior changes. --- library/rsa.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index 88c1cf1007..38ce62d53b 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1387,18 +1387,20 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, int mode, size_t *olen, const unsigned char *input, unsigned char *output, - size_t output_max_len) + size_t output_max_len ) { int ret; - size_t ilen, pad_count = 0, i; - unsigned char *p, bad, pad_done = 0; + size_t ilen = ctx->len; + size_t pad_count = 0; + size_t i; + unsigned bad = 0; + unsigned char pad_done = 0; unsigned char buf[MBEDTLS_MPI_MAX_SIZE]; + unsigned char *p = buf; if( mode == MBEDTLS_RSA_PRIVATE && ctx->padding != MBEDTLS_RSA_PKCS_V15 ) return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA ); - ilen = ctx->len; - if( ilen < 16 || ilen > sizeof( buf ) ) return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA ); @@ -1409,9 +1411,6 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, if( ret != 0 ) goto cleanup; - p = buf; - bad = 0; - /* * Check and get padding len in "constant-time" */ From e2a10de27541ec85be4428cc3ae961d893261ca9 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 2 Oct 2018 22:44:41 +0200 Subject: [PATCH 08/79] Fix a timing-based Bleichenbacher attack on PKCS#1v1.5 decryption mbedtls_rsa_rsaes_pkcs1_v15_decrypt took care of calculating the padding length without leaking the amount of padding or the validity of the padding. However it then skipped the copying of the data if the padding was invalid, which could allow an adversary to find out whether the padding was valid through precise timing measurements, especially if for a local attacker who could observe memory access via cache timings. Avoid this leak by always copying from the decryption buffer to the output buffer, even when the padding is invalid. With invalid padding, copy the same amount of data as what is expected on valid padding: the minimum valid padding size if this fits in the output buffer, otherwise the output buffer size. To avoid leaking payload data from an unsuccessful decryption, zero the decryption buffer before copying if the padding was invalid. --- library/rsa.c | 88 +++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 79 insertions(+), 9 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index 38ce62d53b..5859bbeb15 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1378,6 +1378,37 @@ cleanup: #endif /* MBEDTLS_PKCS1_V21 */ #if defined(MBEDTLS_PKCS1_V15) +/** Turn zero-or-nonzero into zero-or-all-bits-one, without branches. + * + * \param value The value to analyze. + * \return \c 0 if \p value is zero, otherwise \c 0xff. + */ +static unsigned unsigned_all_or_nothing( unsigned value ) +{ + /* MSVC has a warning about unary minus on unsigned, but this is + * well-defined and precisely what we want to do here */ +#if defined(_MSC_VER) +#pragma warning( push ) +#pragma warning( disable : 4146 ) +#endif + return( - ( ( value | - value ) >> ( sizeof( value ) * 8 - 1 ) ) ); +#if defined(_MSC_VER) +#pragma warning( pop ) +#endif +} + +/** Choose between two integer values, without branches. + * + * \param mask Either \c 0 or \c ~0. + * \param if0 Value to use if \p mask = \c 0. + * \param if1 Value to use if \p mask = \c ~0. + * \return \c if1 if \p value is zero, otherwise \c if0. + */ +static unsigned choose_int_from_mask( unsigned mask, unsigned if1, unsigned if0 ) +{ + return( ( mask & if1 ) | (~mask & if0 ) ); +} + /* * Implementation of the PKCS#1 v2.1 RSAES-PKCS1-V1_5-DECRYPT function */ @@ -1395,6 +1426,10 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, size_t i; unsigned bad = 0; unsigned char pad_done = 0; + size_t plaintext_size = 0; + size_t plaintext_max_size = ( output_max_len > ilen - 11 ? + ilen - 11 : + output_max_len ); unsigned char buf[MBEDTLS_MPI_MAX_SIZE]; unsigned char *p = buf; @@ -1448,23 +1483,58 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, bad |= *p++; /* Must be zero */ } + /* There must be at least 8 bytes of padding. */ bad |= ( pad_count < 8 ); - if( bad ) - { - ret = MBEDTLS_ERR_RSA_INVALID_PADDING; - goto cleanup; - } + /* Set bad to zero if the padding is valid and + * all-bits-one otherwise. The whole calculation of bad + * is done in such a way to avoid branches. */ + bad = unsigned_all_or_nothing( bad ); - if( ilen - ( p - buf ) > output_max_len ) + /* If the padding is valid, set plaintext_size to the number of + * remaining bytes after stripping the padding. If the padding + * is invalid, avoid leaking this fact through the size of the + * output: use the maximum message size that fits in the output + * buffer. Do it without branches to avoid leaking the padding + * validity through timing. RSA keys are small enough that all the + * size_t values involved fit in unsigned int. */ + plaintext_size = choose_int_from_mask( bad, + (unsigned) plaintext_max_size, + (unsigned) ( ilen - ( p - buf ) ) ); + + /* Check if the decrypted plaintext fits in the output buffer. + * If the padding is bad, this will always be the case, + * thus we don't leak the padding validity by trying to produce + * a larger output than what the caller expects. */ + if( plaintext_size > output_max_len ) { ret = MBEDTLS_ERR_RSA_OUTPUT_TOO_LARGE; goto cleanup; } - *olen = ilen - (p - buf); - memcpy( output, p, *olen ); - ret = 0; + /* Set ret to INVALID_PADDING if the padding is bad and to 0 + * otherwise. At this point, the variable bad is zero if + * the padding is good and can be any nonzero value otherwise. + * Do this without branches to avoid timing attacks. */ + ret = - ( bad & ( - MBEDTLS_ERR_RSA_INVALID_PADDING ) ); + + /* If the padding is bad, zero the data that we're about to copy + * to the output buffer. We need to copy the same amount of data + * from the same buffer whether the padding is good or not to + * avoid leaking the padding validity through overall timing or + * through memory or cache access patterns. */ + for( i = 11; i < ilen; i++ ) + buf[i] &= ~bad; + + /* Copy the decrypted plaintext from the end of the buffer. */ + memcpy( output, buf + ilen - plaintext_size, plaintext_size ); + + /* Report the amount of data we copied to the output buffer. + * When the padding is invalid, the value of *olen when this + * function returns is not specified. Making it equivalent to + * the good-padding case limits the risks of leaking the + * padding validity. */ + *olen = plaintext_size; cleanup: mbedtls_platform_zeroize( buf, sizeof( buf ) ); From ddffa06501085d3800962b86a2b7eddb18f4e3b4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 3 Oct 2018 13:40:16 +0200 Subject: [PATCH 09/79] Add ChangeLog entry --- ChangeLog | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/ChangeLog b/ChangeLog index 820c26b408..32311bfaa1 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,13 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS 2.xx.x branch released xxxx-xx-xx + +Security + * Fix a timing variation in RSA PKCS#1 v1.5 decryption that could + lead to a Bleichenbacher-style attack. In TLS, this affects + RSA-based ciphersuites without DHE or ECDHE. Reported by Yuval Yarom, + Eyal Ronen, Adi Shamir, David Wong and Daniel Genkin. + = mbed TLS 2.13.1 branch released 2018-09-06 API Changes From 331d80e162d5b8528405e5e4814ec60cf8ab8618 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Oct 2018 18:32:29 +0200 Subject: [PATCH 10/79] Evolve choose_int_from_mask to if_int Make the function more robust by taking an arbitrary zero/nonzero argument instead of insisting on zero/all-bits-one. Update and fix its documentation. --- library/rsa.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index 5859bbeb15..f2b2fb4ed3 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1381,9 +1381,9 @@ cleanup: /** Turn zero-or-nonzero into zero-or-all-bits-one, without branches. * * \param value The value to analyze. - * \return \c 0 if \p value is zero, otherwise \c 0xff. + * \return Zero if \p value is zero, otherwise all-bits-one. */ -static unsigned unsigned_all_or_nothing( unsigned value ) +static unsigned all_or_nothing_int( unsigned value ) { /* MSVC has a warning about unary minus on unsigned, but this is * well-defined and precisely what we want to do here */ @@ -1399,13 +1399,17 @@ static unsigned unsigned_all_or_nothing( unsigned value ) /** Choose between two integer values, without branches. * - * \param mask Either \c 0 or \c ~0. - * \param if0 Value to use if \p mask = \c 0. - * \param if1 Value to use if \p mask = \c ~0. - * \return \c if1 if \p value is zero, otherwise \c if0. + * This is equivalent to `cond ? if1 : if0`, but is likely to be compiled + * to code using bitwise operation rather than a branch. + * + * \param cond Condition to test. + * \param if1 Value to use if \p cond is nonzero. + * \param if0 Value to use if \p cond is zero. + * \return \c if1 if \p cond is nonzero, otherwise \c if0. */ -static unsigned choose_int_from_mask( unsigned mask, unsigned if1, unsigned if0 ) +static unsigned if_int( unsigned cond, unsigned if1, unsigned if0 ) { + unsigned mask = all_or_nothing_int( cond ); return( ( mask & if1 ) | (~mask & if0 ) ); } @@ -1489,7 +1493,7 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, /* Set bad to zero if the padding is valid and * all-bits-one otherwise. The whole calculation of bad * is done in such a way to avoid branches. */ - bad = unsigned_all_or_nothing( bad ); + bad = all_or_nothing_int( bad ); /* If the padding is valid, set plaintext_size to the number of * remaining bytes after stripping the padding. If the padding @@ -1498,9 +1502,9 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, * buffer. Do it without branches to avoid leaking the padding * validity through timing. RSA keys are small enough that all the * size_t values involved fit in unsigned int. */ - plaintext_size = choose_int_from_mask( bad, - (unsigned) plaintext_max_size, - (unsigned) ( ilen - ( p - buf ) ) ); + plaintext_size = if_int( bad, + (unsigned) plaintext_max_size, + (unsigned) ( ilen - ( p - buf ) ) ); /* Check if the decrypted plaintext fits in the output buffer. * If the padding is bad, this will always be the case, From 9265ff4ee6a843b7f1634facee2b087c8c7359fb Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Oct 2018 19:13:43 +0200 Subject: [PATCH 11/79] Bleichenbacher fix: don't leak the plaintext length (step 1) mbedtls_rsa_rsaes_pkcs1_v15_decrypt takes care not to reveal whether the padding is valid or not, even through timing or memory access patterns. This is a defense against an attack published by Bleichenbacher. The attacker can also obtain the same information by observing the length of the plaintext. The current implementation leaks the length of the plaintext through timing and memory access patterns. This commit is a first step towards fixing this leak. It reduces the leak to a single memmove call inside the working buffer. --- library/rsa.c | 69 +++++++++++++++++++++++++++++---------------------- 1 file changed, 40 insertions(+), 29 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index f2b2fb4ed3..f4aafa97c1 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1434,6 +1434,7 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, size_t plaintext_max_size = ( output_max_len > ilen - 11 ? ilen - 11 : output_max_len ); + unsigned output_too_large; unsigned char buf[MBEDTLS_MPI_MAX_SIZE]; unsigned char *p = buf; @@ -1490,11 +1491,6 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, /* There must be at least 8 bytes of padding. */ bad |= ( pad_count < 8 ); - /* Set bad to zero if the padding is valid and - * all-bits-one otherwise. The whole calculation of bad - * is done in such a way to avoid branches. */ - bad = all_or_nothing_int( bad ); - /* If the padding is valid, set plaintext_size to the number of * remaining bytes after stripping the padding. If the padding * is invalid, avoid leaking this fact through the size of the @@ -1506,38 +1502,53 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, (unsigned) plaintext_max_size, (unsigned) ( ilen - ( p - buf ) ) ); - /* Check if the decrypted plaintext fits in the output buffer. - * If the padding is bad, this will always be the case, - * thus we don't leak the padding validity by trying to produce - * a larger output than what the caller expects. */ - if( plaintext_size > output_max_len ) - { - ret = MBEDTLS_ERR_RSA_OUTPUT_TOO_LARGE; - goto cleanup; - } + /* Set output_too_large to 0 if the plaintext fits in the output + * buffer and to 1 otherwise. This is the sign bit (1 for negative) + * of (output_max_len - plaintext_size). */ + output_too_large = ( ( output_max_len - plaintext_size ) >> + ( ( sizeof( output_max_len ) * 8 - 1 ) ) ); - /* Set ret to INVALID_PADDING if the padding is bad and to 0 - * otherwise. At this point, the variable bad is zero if - * the padding is good and can be any nonzero value otherwise. - * Do this without branches to avoid timing attacks. */ - ret = - ( bad & ( - MBEDTLS_ERR_RSA_INVALID_PADDING ) ); + /* Set ret without branches to avoid timing attacks. Return: + * - INVALID_PADDING if the padding is bad (bad != 0). + * - OUTPUT_TOO_LARGE if the padding is good but the decrypted + * plaintext does not fit in the output buffer. + * - 0 if the padding is correct. */ + ret = - if_int( bad, - MBEDTLS_ERR_RSA_INVALID_PADDING, + if_int( output_too_large, - MBEDTLS_ERR_RSA_OUTPUT_TOO_LARGE, + 0 ) ); - /* If the padding is bad, zero the data that we're about to copy - * to the output buffer. We need to copy the same amount of data + /* If the padding is bad or the plaintext is too large, zero the + * data that we're about to copy to the output buffer. + * We need to copy the same amount of data * from the same buffer whether the padding is good or not to * avoid leaking the padding validity through overall timing or * through memory or cache access patterns. */ for( i = 11; i < ilen; i++ ) - buf[i] &= ~bad; + buf[i] &= ~( bad | output_too_large ); - /* Copy the decrypted plaintext from the end of the buffer. */ - memcpy( output, buf + ilen - plaintext_size, plaintext_size ); + /* If the plaintext is too large, truncate it to the buffer size. + * Copy anyway to avoid revealing the length through timing, because + * revealing the length is as bad as revealing the padding validity + * for a Bleichenbacher attack. */ + plaintext_size = if_int( output_too_large, + (unsigned) plaintext_max_size, + (unsigned) plaintext_size ); - /* Report the amount of data we copied to the output buffer. - * When the padding is invalid, the value of *olen when this - * function returns is not specified. Making it equivalent to - * the good-padding case limits the risks of leaking the - * padding validity. */ + /* Move the plaintext to the beginning of the working buffer so that + * its position no longer depends on the padding and we have enough + * room from the beginning of the plaintext to copy a number of bytes + * that does not depend on the padding. */ + /* FIXME: we need a constant-time, constant-trace memmove here. */ + memmove( buf, p, plaintext_size ); + + /* Finally copy the decrypted plaintext plus trailing data + * into the output buffer. */ + memcpy( output, buf, plaintext_max_size ); + + /* Report the amount of data we copied to the output buffer. In case + * of errors (bad padding or output too large), the value of *olen + * when this function returns is not specified. Making it equivalent + * to the good case limits the risks of leaking the padding validity. */ *olen = plaintext_size; cleanup: From a1af5c8ea2586c2f4f93908cc84398041ee8c806 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Oct 2018 21:18:30 +0200 Subject: [PATCH 12/79] Bleichenbacher fix: don't leak the plaintext length (step 2) Replace memmove(to, to + offset, length) by a functionally equivalent function that strives to make the same memory access patterns regardless of the value of length. This fixes an information leak through timing (especially timing of memory accesses via cache probes) that leads to a Bleichenbacher-style attack on PKCS#1 v1.5 decryption using the plaintext length as the observable. --- library/rsa.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index f4aafa97c1..f4a680ce34 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1397,6 +1397,22 @@ static unsigned all_or_nothing_int( unsigned value ) #endif } +/** Check whether a size is out of bounds, without branches. + * + * This is equivalent to `size > max`, but is likely to be compiled to + * to code using bitwise operation rather than a branch. + * + * \param size Size to check. + * \param max Maximum desired value for \p size. + * \return \c 0 if `size <= max`. + * \return \c 1 if `size > max`. + */ +static unsigned size_greater_than( size_t size, size_t max ) +{ + /* Return the sign bit (1 for negative) of (max - size). */ + return( ( max - size ) >> ( sizeof( size_t ) * 8 - 1 ) ); +} + /** Choose between two integer values, without branches. * * This is equivalent to `cond ? if1 : if0`, but is likely to be compiled @@ -1413,6 +1429,42 @@ static unsigned if_int( unsigned cond, unsigned if1, unsigned if0 ) return( ( mask & if1 ) | (~mask & if0 ) ); } +/** Shift some data towards the left inside a buffer without leaking + * the length of the data through side channels. + * + * `mem_move_to_left(start, total, offset)` is functionally equivalent to + * ``` + * memmove(start, start + offset, total - offset); + * memset(start + offset, 0, total - offset); + * ``` + * but it strives to use a memory access pattern (and thus total timing) + * that does not depend on \p offset. This timing independence comes at + * the expense of performance. + * + * \param start Pointer to the start of the buffer. + * \param total Total size of the buffer. + * \param offset Offset from which to copy \p total - \p offset bytes. + */ +static void mem_move_to_left( void *start, + size_t total, + size_t offset ) +{ + volatile unsigned char *buf = start; + size_t i, n; + if( total == 0 ) + return; + for( i = 0; i < total; i++ ) + { + unsigned no_op = size_greater_than( total - offset, i ); + /* The first `total - offset` passes are a no-op. The last + * `offset` passes shift the data one byte to the left and + * zero out the last byte. */ + for( n = 0; n < total - 1; n++ ) + buf[n] = if_int( no_op, buf[n], buf[n+1] ); + buf[total-1] = if_int( no_op, buf[total-1], 0 ); + } +} + /* * Implementation of the PKCS#1 v2.1 RSAES-PKCS1-V1_5-DECRYPT function */ @@ -1538,8 +1590,7 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, * its position no longer depends on the padding and we have enough * room from the beginning of the plaintext to copy a number of bytes * that does not depend on the padding. */ - /* FIXME: we need a constant-time, constant-trace memmove here. */ - memmove( buf, p, plaintext_size ); + mem_move_to_left( buf, ilen, ilen - plaintext_size ); /* Finally copy the decrypted plaintext plus trailing data * into the output buffer. */ From 8c9440a2cb73df1a48acc0a13c0bb41634e582ca Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Oct 2018 21:24:21 +0200 Subject: [PATCH 13/79] Use branch-free size comparison for the padding size In mbedtls_rsa_rsaes_pkcs1_v15_decrypt, use size_greater_than (which is based on bitwise operations) instead of the < operator to compare sizes when the values being compared must not leak. Some compilers compile < to a branch at least under some circumstances (observed with gcc 5.4 for arm-gnueabi -O9 on a toy program). --- library/rsa.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index f4a680ce34..19bafa1f07 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1541,7 +1541,7 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, } /* There must be at least 8 bytes of padding. */ - bad |= ( pad_count < 8 ); + bad |= size_greater_than( 8, pad_count ); /* If the padding is valid, set plaintext_size to the number of * remaining bytes after stripping the padding. If the padding @@ -1555,10 +1555,9 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, (unsigned) ( ilen - ( p - buf ) ) ); /* Set output_too_large to 0 if the plaintext fits in the output - * buffer and to 1 otherwise. This is the sign bit (1 for negative) - * of (output_max_len - plaintext_size). */ - output_too_large = ( ( output_max_len - plaintext_size ) >> - ( ( sizeof( output_max_len ) * 8 - 1 ) ) ); + * buffer and to 1 otherwise. */ + output_too_large = size_greater_than( plaintext_size, + plaintext_max_size ); /* Set ret without branches to avoid timing attacks. Return: * - INVALID_PADDING if the padding is bad (bad != 0). From eeedabe25eb0d8872fbe9dc1944ed9ac5d6786aa Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 4 Oct 2018 22:45:13 +0200 Subject: [PATCH 14/79] Minor optimization in the PKCS#1v1.5 unpadding step Rather than doing the quadratic-time constant-memory-trace on the whole working buffer, do it on the section of the buffer where the data to copy has to lie, which can be significantly smaller if the output buffer is significantly smaller than the working buffer, e.g. for TLS RSA ciphersuites (48 bytes vs MBEDTLS_MPI_MAX_SIZE). --- library/rsa.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index 19bafa1f07..5434d1419d 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1585,15 +1585,19 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, (unsigned) plaintext_max_size, (unsigned) plaintext_size ); - /* Move the plaintext to the beginning of the working buffer so that - * its position no longer depends on the padding and we have enough - * room from the beginning of the plaintext to copy a number of bytes - * that does not depend on the padding. */ - mem_move_to_left( buf, ilen, ilen - plaintext_size ); + /* Move the plaintext to the leftmost position where it can start in + * the working buffer, i.e. make it start plaintext_max_size from + * the end of the buffer. Do this with a memory access trace that + * does not depend on the plaintext size. After this move, the + * starting location of the plaintext is no longer sensitive + * information. */ + p = buf + ilen - plaintext_max_size; + mem_move_to_left( p, plaintext_max_size, + plaintext_max_size - plaintext_size ); - /* Finally copy the decrypted plaintext plus trailing data + /* Finally copy the decrypted plaintext plus trailing zeros * into the output buffer. */ - memcpy( output, buf, plaintext_max_size ); + memcpy( output, p, plaintext_max_size ); /* Report the amount of data we copied to the output buffer. In case * of errors (bad padding or output too large), the value of *olen From 85a7442753f806f739f2c8fb8ea0b12ee8c139f0 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 5 Oct 2018 14:50:21 +0200 Subject: [PATCH 15/79] mbedtls_rsa_rsaes_pkcs1_v15_decrypt: remove the variable p Get rid of the variable p. This makes it more apparent where the code accesses the buffer at an offset whose value is sensitive. No intended behavior change in this commit. --- library/rsa.c | 50 ++++++++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index 5434d1419d..6aa02da2b8 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1478,17 +1478,26 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, { int ret; size_t ilen = ctx->len; - size_t pad_count = 0; size_t i; - unsigned bad = 0; - unsigned char pad_done = 0; - size_t plaintext_size = 0; size_t plaintext_max_size = ( output_max_len > ilen - 11 ? ilen - 11 : output_max_len ); - unsigned output_too_large; unsigned char buf[MBEDTLS_MPI_MAX_SIZE]; - unsigned char *p = buf; + /* The following variables take sensitive values: their value must + * not leak into the observable behavior of the function other than + * the designated outputs (output, olen, return value). Otherwise + * this would open the execution of the function to + * side-channel-based variants of the Bleichenbacher padding oracle + * attack. Potential side channels include overall timing, memory + * access patterns (especially visible to an adversary who has access + * to a shared memory cache), and branches (especially visible to + * an adversary who has access to a shared code cache or to a shared + * branch predictor). */ + size_t pad_count = 0; + unsigned bad = 0; + unsigned char pad_done = 0; + size_t plaintext_size = 0; + unsigned output_too_large; if( mode == MBEDTLS_RSA_PRIVATE && ctx->padding != MBEDTLS_RSA_PKCS_V15 ) return( MBEDTLS_ERR_RSA_BAD_INPUT_DATA ); @@ -1506,38 +1515,35 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, /* * Check and get padding len in "constant-time" */ - bad |= *p++; /* First byte must be 0 */ + bad |= buf[0]; /* First byte must be 0 */ /* This test does not depend on secret data */ if( mode == MBEDTLS_RSA_PRIVATE ) { - bad |= *p++ ^ MBEDTLS_RSA_CRYPT; + bad |= buf[1] ^ MBEDTLS_RSA_CRYPT; /* Get padding len, but always read till end of buffer * (minus one, for the 00 byte) */ - for( i = 0; i < ilen - 3; i++ ) + for( i = 2; i < ilen - 1; i++ ) { - pad_done |= ((p[i] | (unsigned char)-p[i]) >> 7) ^ 1; + pad_done |= ((buf[i] | (unsigned char)-buf[i]) >> 7) ^ 1; pad_count += ((pad_done | (unsigned char)-pad_done) >> 7) ^ 1; } - p += pad_count; - bad |= *p++; /* Must be zero */ + bad |= buf[pad_count + 2]; } else { - bad |= *p++ ^ MBEDTLS_RSA_SIGN; + bad |= buf[1] ^ MBEDTLS_RSA_SIGN; /* Get padding len, but always read till end of buffer * (minus one, for the 00 byte) */ - for( i = 0; i < ilen - 3; i++ ) + for( i = 2; i < ilen - 1; i++ ) { - pad_done |= ( p[i] != 0xFF ); + pad_done |= ( buf[i] != 0xFF ); pad_count += ( pad_done == 0 ); } - - p += pad_count; - bad |= *p++; /* Must be zero */ + bad |= buf[pad_count + 2]; } /* There must be at least 8 bytes of padding. */ @@ -1552,7 +1558,7 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, * size_t values involved fit in unsigned int. */ plaintext_size = if_int( bad, (unsigned) plaintext_max_size, - (unsigned) ( ilen - ( p - buf ) ) ); + (unsigned) ( ilen - pad_count - 3 ) ); /* Set output_too_large to 0 if the plaintext fits in the output * buffer and to 1 otherwise. */ @@ -1591,13 +1597,13 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, * does not depend on the plaintext size. After this move, the * starting location of the plaintext is no longer sensitive * information. */ - p = buf + ilen - plaintext_max_size; - mem_move_to_left( p, plaintext_max_size, + mem_move_to_left( buf + ilen - plaintext_max_size, + plaintext_max_size, plaintext_max_size - plaintext_size ); /* Finally copy the decrypted plaintext plus trailing zeros * into the output buffer. */ - memcpy( output, p, plaintext_max_size ); + memcpy( output, buf + ilen - plaintext_max_size, plaintext_max_size ); /* Report the amount of data we copied to the output buffer. In case * of errors (bad padding or output too large), the value of *olen From 40b57f4acde4480bf33c0c393c4c77d0c2c98d5c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 5 Oct 2018 15:06:12 +0200 Subject: [PATCH 16/79] Remove a remaining sensitive memory access in PKCS#1 v1.5 decryption --- library/rsa.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index 6aa02da2b8..f581fd73c3 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1512,14 +1512,14 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, if( ret != 0 ) goto cleanup; - /* - * Check and get padding len in "constant-time" - */ - bad |= buf[0]; /* First byte must be 0 */ + /* Check and get padding length in constant time and constant + * memory trace. The first byte must be 0. */ + bad |= buf[0]; - /* This test does not depend on secret data */ if( mode == MBEDTLS_RSA_PRIVATE ) { + /* Decode EME-PKCS1-v1_5 padding: 0x00 || 0x02 || PS || 0x00 + * where PS must be at least 8 nonzero bytes. */ bad |= buf[1] ^ MBEDTLS_RSA_CRYPT; /* Get padding len, but always read till end of buffer @@ -1529,23 +1529,26 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, pad_done |= ((buf[i] | (unsigned char)-buf[i]) >> 7) ^ 1; pad_count += ((pad_done | (unsigned char)-pad_done) >> 7) ^ 1; } - - bad |= buf[pad_count + 2]; } else { + /* Decode EMSA-PKCS1-v1_5 padding: 0x00 || 0x01 || PS || 0x00 + * where PS must be at least 8 bytes with the value 0xFF. */ bad |= buf[1] ^ MBEDTLS_RSA_SIGN; /* Get padding len, but always read till end of buffer * (minus one, for the 00 byte) */ for( i = 2; i < ilen - 1; i++ ) { - pad_done |= ( buf[i] != 0xFF ); - pad_count += ( pad_done == 0 ); + pad_done |= if_int( buf[i], 0, 1 ); + pad_count += if_int( pad_done, 0, 1 ); + bad |= if_int( pad_done, 0, buf[i] ^ 0xFF ); } - bad |= buf[pad_count + 2]; } + /* If pad_done is still zero, there's no data, only unfinished padding. */ + bad |= if_int( pad_done, 0, 1 ); + /* There must be at least 8 bytes of padding. */ bad |= size_greater_than( 8, pad_count ); @@ -1580,8 +1583,9 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, * from the same buffer whether the padding is good or not to * avoid leaking the padding validity through overall timing or * through memory or cache access patterns. */ + bad = all_or_nothing_int( bad | output_too_large ); for( i = 11; i < ilen; i++ ) - buf[i] &= ~( bad | output_too_large ); + buf[i] &= ~bad; /* If the plaintext is too large, truncate it to the buffer size. * Copy anyway to avoid revealing the length through timing, because From c5ccd7a1e7ff3c084bd07fa4a6d59284c8a778b4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 5 Oct 2018 15:42:52 +0200 Subject: [PATCH 17/79] Indicate the memory access variations in the changelog entry --- ChangeLog | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/ChangeLog b/ChangeLog index 32311bfaa1..0bb9ec04a4 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,10 +3,11 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.xx.x branch released xxxx-xx-xx Security - * Fix a timing variation in RSA PKCS#1 v1.5 decryption that could - lead to a Bleichenbacher-style attack. In TLS, this affects - RSA-based ciphersuites without DHE or ECDHE. Reported by Yuval Yarom, - Eyal Ronen, Adi Shamir, David Wong and Daniel Genkin. + * Fix timing variations and memory access variations in RSA PKCS#1 v1.5 + decryption that could lead to a Bleichenbacher-style padding oracle + attack. In TLS, this affects RSA-based ciphersuites without DHE or + ECDHE. Reported by Yuval Yarom, Eyal Ronen, Adi Shamir, David Wong and + Daniel Genkin. = mbed TLS 2.13.1 branch released 2018-09-06 From ec2a5fdee143f52d5800f2e2f3282634800a46c1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 5 Oct 2018 18:11:27 +0200 Subject: [PATCH 18/79] PKCS#1 v1.5 decoding: fix empty payload case --- library/rsa.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index f581fd73c3..69542f3528 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1522,9 +1522,9 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, * where PS must be at least 8 nonzero bytes. */ bad |= buf[1] ^ MBEDTLS_RSA_CRYPT; - /* Get padding len, but always read till end of buffer - * (minus one, for the 00 byte) */ - for( i = 2; i < ilen - 1; i++ ) + /* Read the whole buffer. Set pad_done to nonzero if we find + * the 0x00 byte and remember the padding length in pad_count. */ + for( i = 2; i < ilen; i++ ) { pad_done |= ((buf[i] | (unsigned char)-buf[i]) >> 7) ^ 1; pad_count += ((pad_done | (unsigned char)-pad_done) >> 7) ^ 1; @@ -1536,9 +1536,10 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, * where PS must be at least 8 bytes with the value 0xFF. */ bad |= buf[1] ^ MBEDTLS_RSA_SIGN; - /* Get padding len, but always read till end of buffer - * (minus one, for the 00 byte) */ - for( i = 2; i < ilen - 1; i++ ) + /* Read the whole buffer. Set pad_done to nonzero if we find + * the 0x00 byte and remember the padding length in pad_count. + * If there's a non-0xff byte in the padding, the padding is bad. */ + for( i = 2; i < ilen; i++ ) { pad_done |= if_int( buf[i], 0, 1 ); pad_count += if_int( pad_done, 0, 1 ); From 695a34654ab43f1f0b2e42cfde2f51b21a7e4de7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 5 Oct 2018 18:15:25 +0200 Subject: [PATCH 19/79] Add tests for PKCS#1 v1.5 decoding Functional tests for various payload sizes and output buffer sizes. When the padding is bad or the plaintext is too large for the output buffer, verify that function writes some outputs. This doesn't validate that the implementation is time-constant, but it at least validates that it doesn't just return early without outputting anything. --- tests/suites/test_suite_pkcs1_v15.data | 90 +++++++++++++ tests/suites/test_suite_pkcs1_v15.function | 148 +++++++++++++++++++++ 2 files changed, 238 insertions(+) diff --git a/tests/suites/test_suite_pkcs1_v15.data b/tests/suites/test_suite_pkcs1_v15.data index 0309400075..a4d6eb5457 100644 --- a/tests/suites/test_suite_pkcs1_v15.data +++ b/tests/suites/test_suite_pkcs1_v15.data @@ -33,3 +33,93 @@ pkcs1_rsassa_v15_sign:1024:16:"d17f655bf27c8b16d35462c905cc04a26f37e2a67fa9c0ce0 RSASSA-V15 Verification Test Vector Int pkcs1_rsassa_v15_verify:1024:16:"a2ba40ee07e3b2bd2f02ce227f36a195024486e49c19cb41bbbdfbba98b22b0e577c2eeaffa20d883a76e65e394c69d4b3c05a1e8fadda27edb2a42bc000fe888b9b32c22d15add0cd76b3e7936e19955b220dd17d4ea904b1ec102b2e4de7751222aa99151024c7cb41cc5ea21d00eeb41f7c800834d2c6e06bce3bce7ea9a5":16:"010001":MBEDTLS_MD_SHA1:MBEDTLS_MD_SHA1:"859eef2fd78aca00308bdc471193bf55bf9d78db8f8a672b484634f3c9c26e6478ae10260fe0dd8c082e53a5293af2173cd50c6d5d354febf78b26021c25c02712e78cd4694c9f469777e451e7f8e9e04cd3739c6bbfedae487fb55644e9ca74ff77a53cb729802f6ed4a5ffa8ba159890fc":"e3b5d5d002c1bce50c2b65ef88a188d83bce7e61":"2154f928615e5101fcdeb57bc08fc2f35c3d5996403861ae3efb1d0712f8bb05cc21f7f5f11f62e5b6ea9f0f2b62180e5cbe7ba535032d6ac8068fff7f362f73d2c3bf5eca6062a1723d7cfd5abb6dcf7e405f2dc560ffe6fc37d38bee4dc9e24fe2bece3e3b4a3f032701d3f0947b42930083dd4ad241b3309b514595482d42":0 + +RSAES-V15 decoding: good, payload=max, tight output buffer +pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0002505152535455565700":117:117:0 + +RSAES-V15 decoding: good, payload=max, larger output buffer +pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0002505152535455565700":117:128:0 + +RSAES-V15 decoding: good, payload=max-1, tight output buffer +pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"000250515253545556575800":116:116:0 + +RSAES-V15 decoding: good, payload=max-1, larger output buffer +pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"000250515253545556575800":116:117:0 + +RSAES-V15 decoding: good, payload=1 +pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"00025050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505000":1:1:0 + +RSAES-V15 decoding: good, empty payload +pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0002505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505000":0:0:0 + +RSAES-V15 decoding: payload=max, output too large +pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0002505152535455565700":117:116:MBEDTLS_ERR_RSA_OUTPUT_TOO_LARGE + +RSAES-V15 decoding: payload=max-1, output too large +pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"000250515253545556575800":116:115:MBEDTLS_ERR_RSA_OUTPUT_TOO_LARGE + +RSAES-V15 decoding: bad first byte +pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0102505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING + +RSAES-V15 decoding: bad second byte (0 instead of 2) +pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0000505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING + +RSAES-V15 decoding: bad second byte (1 instead of 2) +pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0001505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING + +RSAES-V15 decoding: padding too short (0) +pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"000200":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING + +RSAES-V15 decoding: padding too short (7) +pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0002505050505050500000ffffffffffffffffff00":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING + +RSAES-V15 decoding: unfinished padding +pkcs1_v15_decode:MBEDTLS_RSA_PRIVATE:"0002505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050505050":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING + +EMSA-V15 decoding: good, payload=max, tight output buffer +pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001ffffffffffffffff00":117:117:0 + +EMSA-V15 decoding: good, payload=max, larger output buffer +pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001ffffffffffffffff00":117:128:0 + +EMSA-V15 decoding: good, payload=max-1, tight output buffer +pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001ffffffffffffffffff00":116:116:0 + +EMSA-V15 decoding: good, payload=max-1, larger output buffer +pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001ffffffffffffffffff00":116:117:0 + +EMSA-V15 decoding: good, payload=1 +pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff00":1:1:0 + +EMSA-V15 decoding: good, empty payload +pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff00":0:0:0 + +EMSA-V15 decoding: bad first byte +pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0101ffffffffffffffff00":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING + +EMSA-V15 decoding: bad second byte (0 instead of 1) +pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0000ffffffffffffffff00":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING + +EMSA-V15 decoding: bad second byte (2 instead of 1) +pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0002ffffffffffffffff00":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING + +EMSA-V15 decoding: padding too short (0) +pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"000100":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING + +EMSA-V15 decoding: padding too short (7) +pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001ffffffffffffff0000ffffffffffffffff00":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING + +EMSA-V15 decoding: invalid padding at first byte +pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001fffffffffffffffe00":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING + +EMSA-V15 decoding: invalid padding at last byte +pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001feffffffffffffff00":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING + +EMSA-V15 decoding: unfinished padding +pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING + +EMSA-V15 decoding: unfinished padding with invalid first byte +pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001feffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING + +EMSA-V15 decoding: unfinished padding with invalid last byte +pkcs1_v15_decode:MBEDTLS_RSA_PUBLIC:"0001fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe":0:42:MBEDTLS_ERR_RSA_INVALID_PADDING diff --git a/tests/suites/test_suite_pkcs1_v15.function b/tests/suites/test_suite_pkcs1_v15.function index 83f417ca81..0723623a5f 100644 --- a/tests/suites/test_suite_pkcs1_v15.function +++ b/tests/suites/test_suite_pkcs1_v15.function @@ -93,6 +93,154 @@ exit: } /* END_CASE */ +/* BEGIN_CASE */ +void pkcs1_v15_decode( int mode, + data_t *input, + int expected_plaintext_length_arg, + int output_size_arg, + int expected_result ) +{ + size_t expected_plaintext_length = expected_plaintext_length_arg; + size_t output_size = output_size_arg; + rnd_pseudo_info rnd_info; + mbedtls_mpi Nmpi, Empi, Pmpi, Qmpi; + mbedtls_rsa_context ctx; + static unsigned char N[128] = { + 0xc4, 0x79, 0x4c, 0x6d, 0xb2, 0xe9, 0xdf, 0xc5, + 0xe5, 0xd7, 0x55, 0x4b, 0xfb, 0x6c, 0x2e, 0xec, + 0x84, 0xd0, 0x88, 0x12, 0xaf, 0xbf, 0xb4, 0xf5, + 0x47, 0x3c, 0x7e, 0x92, 0x4c, 0x58, 0xc8, 0x73, + 0xfe, 0x8f, 0x2b, 0x8f, 0x8e, 0xc8, 0x5c, 0xf5, + 0x05, 0xeb, 0xfb, 0x0d, 0x7b, 0x2a, 0x93, 0xde, + 0x15, 0x0d, 0xc8, 0x13, 0xcf, 0xd2, 0x6f, 0x0d, + 0x9d, 0xad, 0x30, 0xe5, 0x70, 0x20, 0x92, 0x9e, + 0xb3, 0x6b, 0xba, 0x5c, 0x50, 0x0f, 0xc3, 0xb2, + 0x7e, 0x64, 0x07, 0x94, 0x7e, 0xc9, 0x4e, 0xc1, + 0x65, 0x04, 0xaf, 0xb3, 0x9f, 0xde, 0xa8, 0x46, + 0xfa, 0x6c, 0xf3, 0x03, 0xaf, 0x1c, 0x1b, 0xec, + 0x75, 0x44, 0x66, 0x77, 0xc9, 0xde, 0x51, 0x33, + 0x64, 0x27, 0xb0, 0xd4, 0x8d, 0x31, 0x6a, 0x11, + 0x27, 0x3c, 0x99, 0xd4, 0x22, 0xc0, 0x9d, 0x12, + 0x01, 0xc7, 0x4a, 0x73, 0xac, 0xbf, 0xc2, 0xbb + }; + static unsigned char E[1] = { 0x03 }; + static unsigned char P[64] = { + 0xe5, 0x53, 0x1f, 0x88, 0x51, 0xee, 0x59, 0xf8, + 0xc1, 0xe4, 0xcc, 0x5b, 0xb3, 0x75, 0x8d, 0xc8, + 0xe8, 0x95, 0x2f, 0xd0, 0xef, 0x37, 0xb4, 0xcd, + 0xd3, 0x9e, 0x48, 0x8b, 0x81, 0x58, 0x60, 0xb9, + 0x27, 0x1d, 0xb6, 0x28, 0x92, 0x64, 0xa3, 0xa5, + 0x64, 0xbd, 0xcc, 0x53, 0x68, 0xdd, 0x3e, 0x55, + 0xea, 0x9d, 0x5e, 0xcd, 0x1f, 0x96, 0x87, 0xf1, + 0x29, 0x75, 0x92, 0x70, 0x8f, 0x28, 0xfb, 0x2b + }; + static unsigned char Q[64] = { + 0xdb, 0x53, 0xef, 0x74, 0x61, 0xb4, 0x20, 0x3b, + 0x3b, 0x87, 0x76, 0x75, 0x81, 0x56, 0x11, 0x03, + 0x59, 0x31, 0xe3, 0x38, 0x4b, 0x8c, 0x7a, 0x9c, + 0x05, 0xd6, 0x7f, 0x1e, 0x5e, 0x60, 0xf0, 0x4e, + 0x0b, 0xdc, 0x34, 0x54, 0x1c, 0x2e, 0x90, 0x83, + 0x14, 0xef, 0xc0, 0x96, 0x5c, 0x30, 0x10, 0xcc, + 0xc1, 0xba, 0xa0, 0x54, 0x3f, 0x96, 0x24, 0xca, + 0xa3, 0xfb, 0x55, 0xbc, 0x71, 0x29, 0x4e, 0xb1 + }; + unsigned char original[128]; + unsigned char intermediate[128]; + static unsigned char default_content[128] = { + /* A randomly generated pattern. */ + 0x4c, 0x27, 0x54, 0xa0, 0xce, 0x0d, 0x09, 0x4a, + 0x1c, 0x38, 0x8e, 0x2d, 0xa3, 0xc4, 0xe0, 0x19, + 0x4c, 0x99, 0xb2, 0xbf, 0xe6, 0x65, 0x7e, 0x58, + 0xd7, 0xb6, 0x8a, 0x05, 0x2f, 0xa5, 0xec, 0xa4, + 0x35, 0xad, 0x10, 0x36, 0xff, 0x0d, 0x08, 0x50, + 0x74, 0x47, 0xc9, 0x9c, 0x4a, 0xe7, 0xfd, 0xfa, + 0x83, 0x5f, 0x14, 0x5a, 0x1e, 0xe7, 0x35, 0x08, + 0xad, 0xf7, 0x0d, 0x86, 0xdf, 0xb8, 0xd4, 0xcf, + 0x32, 0xb9, 0x5c, 0xbe, 0xa3, 0xd2, 0x89, 0x70, + 0x7b, 0xc6, 0x48, 0x7e, 0x58, 0x4d, 0xf3, 0xef, + 0x34, 0xb7, 0x57, 0x54, 0x79, 0xc5, 0x8e, 0x0a, + 0xa3, 0xbf, 0x6d, 0x42, 0x83, 0x25, 0x13, 0xa2, + 0x95, 0xc0, 0x0d, 0x32, 0xec, 0x77, 0x91, 0x2b, + 0x68, 0xb6, 0x8c, 0x79, 0x15, 0xfb, 0x94, 0xde, + 0xb9, 0x2b, 0x94, 0xb3, 0x28, 0x23, 0x86, 0x3d, + 0x37, 0x00, 0xe6, 0xf1, 0x1f, 0x4e, 0xd4, 0x42 + }; + unsigned char final[128]; + size_t output_length = 0x7EA0; + + memset( &rnd_info, 0, sizeof( rnd_pseudo_info ) ); + mbedtls_mpi_init( &Nmpi ); mbedtls_mpi_init( &Empi ); + mbedtls_mpi_init( &Pmpi ); mbedtls_mpi_init( &Qmpi ); + mbedtls_rsa_init( &ctx, MBEDTLS_RSA_PKCS_V15, 0 ); + + TEST_ASSERT( mbedtls_mpi_read_binary( &Nmpi, N, sizeof( N ) ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &Empi, E, sizeof( E ) ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &Pmpi, P, sizeof( P ) ) == 0 ); + TEST_ASSERT( mbedtls_mpi_read_binary( &Qmpi, Q, sizeof( Q ) ) == 0 ); + + TEST_ASSERT( mbedtls_rsa_import( &ctx, &Nmpi, &Pmpi, &Qmpi, + NULL, &Empi ) == 0 ); + TEST_ASSERT( mbedtls_rsa_complete( &ctx ) == 0 ); + + TEST_ASSERT( input->len <= sizeof( N ) ); + memcpy( original, input->x, input->len ); + memset( original + input->len, 'd', sizeof( original ) - input->len ); + if( mode == MBEDTLS_RSA_PRIVATE ) + TEST_ASSERT( mbedtls_rsa_public( &ctx, original, intermediate ) == 0 ); + else + TEST_ASSERT( mbedtls_rsa_private( &ctx, &rnd_pseudo_rand, &rnd_info, + original, intermediate ) == 0 ); + + memcpy( final, default_content, sizeof( final ) ); + TEST_ASSERT( mbedtls_rsa_pkcs1_decrypt( &ctx, + &rnd_pseudo_rand, &rnd_info, + mode, + &output_length, + intermediate, + final, + output_size ) == expected_result ); + if( expected_result == 0 ) + { + TEST_ASSERT( output_length == expected_plaintext_length ); + TEST_ASSERT( memcmp( original + sizeof( N ) - output_length, + final, + output_length ) == 0 ); + } + else if( expected_result == MBEDTLS_ERR_RSA_INVALID_PADDING || + expected_result == MBEDTLS_ERR_RSA_OUTPUT_TOO_LARGE ) + { + size_t max_payload_length = + output_size > sizeof( N ) - 11 ? sizeof( N ) - 11 : output_size; + size_t i; + size_t count = 0; + +#if !defined(MBEDTLS_RSA_ALT) + /* Check that the output in invalid cases is what the default + * implementation currently does. Alternative implementations + * may produce different output, so we only perform these precise + * checks when using the default implementation. */ + TEST_ASSERT( output_length == max_payload_length ); + for( i = 0; i < max_payload_length; i++ ) + TEST_ASSERT( final[i] == 0 ); +#endif + /* Even in alternative implementations, the outputs must have + * changed, otherwise it indicates at least a timing vulnerability + * because no write to the outputs is performed in the bad case. */ + TEST_ASSERT( output_length != 0x7EA0 ); + for( i = 0; i < max_payload_length; i++ ) + count += ( final[i] == default_content[i] ); + /* If more than 16 bytes are unchanged in final, that's evidence + * that final wasn't overwritten. */ + TEST_ASSERT( count < 16 ); + } + +exit: + mbedtls_mpi_free( &Nmpi ); mbedtls_mpi_free( &Empi ); + mbedtls_mpi_free( &Pmpi ); mbedtls_mpi_free( &Qmpi ); + mbedtls_rsa_free( &ctx ); +} +/* END_CASE */ + /* BEGIN_CASE */ void pkcs1_rsassa_v15_sign( int mod, int radix_P, char * input_P, int radix_Q, char * input_Q, int radix_N, char * input_N, From 9b430704d151efdf917eb388446bded760e86dd8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 12 Oct 2018 19:15:34 +0200 Subject: [PATCH 20/79] Fix likely-harmless undefined behavior surrounding volatile The code was making two unsequenced reads from volatile locations. This is undefined behavior. It was probably harmless because we didn't care in what order the reads happened and the reads were from ordinary memory, but UB is UB and IAR8 complained. --- library/rsa.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/library/rsa.c b/library/rsa.c index 69542f3528..b401189d2b 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1460,7 +1460,11 @@ static void mem_move_to_left( void *start, * `offset` passes shift the data one byte to the left and * zero out the last byte. */ for( n = 0; n < total - 1; n++ ) - buf[n] = if_int( no_op, buf[n], buf[n+1] ); + { + unsigned char current = buf[n]; + unsigned char next = buf[n+1]; + buf[n] = if_int( no_op, current, next ); + } buf[total-1] = if_int( no_op, buf[total-1], 0 ); } } From 4899247bf2cf1cae088e0c11a36f09166561875b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 12 Oct 2018 19:19:12 +0200 Subject: [PATCH 21/79] Fix undefined behavior in unsigned-to-signed conversion The code assumed that `int x = - (unsigned) u` with 0 <= u < INT_MAX sets `x` to the negative of u, but actually this calculates (UINT_MAX - u) and then converts this value to int, which overflows. Cast to int before applying the unary minus operator to guarantee the desired behavior. --- library/rsa.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/rsa.c b/library/rsa.c index b401189d2b..b9708646f8 100644 --- a/library/rsa.c +++ b/library/rsa.c @@ -1578,9 +1578,9 @@ int mbedtls_rsa_rsaes_pkcs1_v15_decrypt( mbedtls_rsa_context *ctx, * - OUTPUT_TOO_LARGE if the padding is good but the decrypted * plaintext does not fit in the output buffer. * - 0 if the padding is correct. */ - ret = - if_int( bad, - MBEDTLS_ERR_RSA_INVALID_PADDING, - if_int( output_too_large, - MBEDTLS_ERR_RSA_OUTPUT_TOO_LARGE, - 0 ) ); + ret = - (int) if_int( bad, - MBEDTLS_ERR_RSA_INVALID_PADDING, + if_int( output_too_large, - MBEDTLS_ERR_RSA_OUTPUT_TOO_LARGE, + 0 ) ); /* If the padding is bad or the plaintext is too large, zero the * data that we're about to copy to the output buffer. From cb9debda6bb3fd4f6c53fd94895dfcc5f885502f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Oct 2018 10:44:27 +0100 Subject: [PATCH 22/79] Guard PK-parse module by ASN.1-parse module in check_config.h --- include/mbedtls/check_config.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 9e6bb8a46a..ebd28eaf10 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -127,6 +127,10 @@ #error "MBEDTLS_ECP_C defined, but not all prerequisites" #endif +#if defined(MBEDTLS_PK_PARSE_C) && !defined(MBEDTLS_ASN1_PARSE_C) +#error "MBEDTLS_PK_PARSE_C defined, but not all prerequesites" +#endif + #if defined(MBEDTLS_ENTROPY_C) && (!defined(MBEDTLS_SHA512_C) && \ !defined(MBEDTLS_SHA256_C)) #error "MBEDTLS_ENTROPY_C defined, but not all prerequisites" From 8a89f9fcd2cc4a43089414ebf33130217f95d2f7 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Oct 2018 10:46:32 +0100 Subject: [PATCH 23/79] Make PBE-related parts of PKCS12 depend on MBEDTLS_ASN1_PARSE_C --- include/mbedtls/pkcs12.h | 4 ++++ library/pkcs12.c | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/include/mbedtls/pkcs12.h b/include/mbedtls/pkcs12.h index a621ef5b15..69f04177c8 100644 --- a/include/mbedtls/pkcs12.h +++ b/include/mbedtls/pkcs12.h @@ -46,6 +46,8 @@ extern "C" { #endif +#if defined(MBEDTLS_ASN1_PARSE_C) + /** * \brief PKCS12 Password Based function (encryption / decryption) * for pbeWithSHAAnd128BitRC4 @@ -87,6 +89,8 @@ int mbedtls_pkcs12_pbe( mbedtls_asn1_buf *pbe_params, int mode, const unsigned char *input, size_t len, unsigned char *output ); +#endif /* MBEDTLS_ASN1_PARSE_C */ + /** * \brief The PKCS#12 derivation function uses a password and a salt * to produce pseudo-random bits for a particular "purpose". diff --git a/library/pkcs12.c b/library/pkcs12.c index 16a15cb63e..7edf064c13 100644 --- a/library/pkcs12.c +++ b/library/pkcs12.c @@ -48,6 +48,8 @@ #include "mbedtls/des.h" #endif +#if defined(MBEDTLS_ASN1_PARSE_C) + static int pkcs12_parse_pbe_params( mbedtls_asn1_buf *params, mbedtls_asn1_buf *salt, int *iterations ) { @@ -226,6 +228,8 @@ exit: return( ret ); } +#endif /* MBEDTLS_ASN1_PARSE_C */ + static void pkcs12_fill_buffer( unsigned char *data, size_t data_len, const unsigned char *filler, size_t fill_len ) { From 1ea604d3ee70bf6c735b3d03bcfe892cca7cdd96 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Oct 2018 10:57:33 +0100 Subject: [PATCH 24/79] Guard mbedtls_pkcs5_pbes2() by MBEDTLS_ASN1_PARSE_C Previously, mbedtls_pkcs5_pbes2() was unconditionally declared in `pkcs5.h` but defined as a stub returning `MBEDTLS_ERR_PKCS5_FEATURE_UNAVAILABLE` in case MBEDTLS_ASN1_PARSE_C was not defined. In line with the previous commits, this commit removes declaration and definition from both `pkcs5.h` and `pkcs5.c` in case MBEDTLS_ASN1_PARSE_C is not defined. --- include/mbedtls/pkcs5.h | 4 ++++ library/pkcs5.c | 17 +---------------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/include/mbedtls/pkcs5.h b/include/mbedtls/pkcs5.h index 9a3c9fddcc..d4bb36dfae 100644 --- a/include/mbedtls/pkcs5.h +++ b/include/mbedtls/pkcs5.h @@ -44,6 +44,8 @@ extern "C" { #endif +#if defined(MBEDTLS_ASN1_PARSE_C) + /** * \brief PKCS#5 PBES2 function * @@ -62,6 +64,8 @@ int mbedtls_pkcs5_pbes2( const mbedtls_asn1_buf *pbe_params, int mode, const unsigned char *data, size_t datalen, unsigned char *output ); +#endif /* MBEDTLS_ASN1_PARSE_C */ + /** * \brief PKCS#5 PBKDF2 using HMAC * diff --git a/library/pkcs5.c b/library/pkcs5.c index f04f0ab25e..50133435ce 100644 --- a/library/pkcs5.c +++ b/library/pkcs5.c @@ -54,22 +54,7 @@ #define mbedtls_printf printf #endif -#if !defined(MBEDTLS_ASN1_PARSE_C) -int mbedtls_pkcs5_pbes2( const mbedtls_asn1_buf *pbe_params, int mode, - const unsigned char *pwd, size_t pwdlen, - const unsigned char *data, size_t datalen, - unsigned char *output ) -{ - ((void) pbe_params); - ((void) mode); - ((void) pwd); - ((void) pwdlen); - ((void) data); - ((void) datalen); - ((void) output); - return( MBEDTLS_ERR_PKCS5_FEATURE_UNAVAILABLE ); -} -#else +#if defined(MBEDTLS_ASN1_PARSE_C) static int pkcs5_parse_pbkdf2_params( const mbedtls_asn1_buf *params, mbedtls_asn1_buf *salt, int *iterations, int *keylen, mbedtls_md_type_t *md_type ) From 44da18a2944c27d63af58494b9fec76e8b51b0d8 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 12 Oct 2018 10:42:13 +0100 Subject: [PATCH 25/79] Duplicate mbedtls_asn1_find_named_data in asn1write.c to avoid dep. This commit duplicates the public function mbedtls_asn1_find_named_data() defined in library/asn1parse.c within library/asn1write.c in order to avoid a dependency of the ASN.1 writing module on the ASN.1 parsing module. The duplication is unproblematic from a semantic and an efficiency perspective becasue it is just a short list traversal that doesn't actually do any ASN.1 parsing. --- library/asn1write.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/library/asn1write.c b/library/asn1write.c index 72acdf3012..119a6d0d1c 100644 --- a/library/asn1write.c +++ b/library/asn1write.c @@ -328,14 +328,36 @@ int mbedtls_asn1_write_octet_string( unsigned char **p, unsigned char *start, return( (int) len ); } -mbedtls_asn1_named_data *mbedtls_asn1_store_named_data( mbedtls_asn1_named_data **head, + +/* This is a copy of the ASN.1 parsing function mbedtls_asn1_find_named_data(), + * which is replicated to avoid a dependency ASN1_WRITE_C on ASN1_PARSE_C. */ +static mbedtls_asn1_named_data *asn1_find_named_data( + mbedtls_asn1_named_data *list, + const char *oid, size_t len ) +{ + while( list != NULL ) + { + if( list->oid.len == len && + memcmp( list->oid.p, oid, len ) == 0 ) + { + break; + } + + list = list->next; + } + + return( list ); +} + +mbedtls_asn1_named_data *mbedtls_asn1_store_named_data( + mbedtls_asn1_named_data **head, const char *oid, size_t oid_len, const unsigned char *val, size_t val_len ) { mbedtls_asn1_named_data *cur; - if( ( cur = mbedtls_asn1_find_named_data( *head, oid, oid_len ) ) == NULL ) + if( ( cur = asn1_find_named_data( *head, oid, oid_len ) ) == NULL ) { // Add new entry if not present yet based on OID // From b14c331eb903cd59f62a705ed4b8b112c441650e Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 16 Oct 2018 13:45:22 +0100 Subject: [PATCH 26/79] Add dependency of key_app_writer example program on PK parse module --- programs/pkey/key_app_writer.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/programs/pkey/key_app_writer.c b/programs/pkey/key_app_writer.c index 13602c2e53..cd0c230644 100644 --- a/programs/pkey/key_app_writer.c +++ b/programs/pkey/key_app_writer.c @@ -87,10 +87,12 @@ USAGE_OUT \ "\n" -#if !defined(MBEDTLS_PK_WRITE_C) || !defined(MBEDTLS_FS_IO) +#if !defined(MBEDTLS_PK_PARSE_C) || \ + !defined(MBEDTLS_PK_WRITE_C) || \ + !defined(MBEDTLS_FS_IO) int main( void ) { - mbedtls_printf( "MBEDTLS_PK_WRITE_C and/or MBEDTLS_FS_IO not defined.\n" ); + mbedtls_printf( "MBEDTLS_PK_PARSE_C and/or MBEDTLS_PK_WRITE_C and/or MBEDTLS_FS_IO not defined.\n" ); return( 0 ); } #else @@ -433,4 +435,4 @@ exit: return( exit_code ); } -#endif /* MBEDTLS_PK_WRITE_C && MBEDTLS_FS_IO */ +#endif /* MBEDTLS_PK_PARSE_C && MBEDTLS_PK_WRITE_C && MBEDTLS_FS_IO */ From 19d858e8e6c1e44136b79e10367ccf28f6b1585a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 16 Oct 2018 13:46:25 +0100 Subject: [PATCH 27/79] Add dependency of pkwrite test suite on pkparse module --- tests/suites/test_suite_pkwrite.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_pkwrite.function b/tests/suites/test_suite_pkwrite.function index 3ad782d33e..43c275ef25 100644 --- a/tests/suites/test_suite_pkwrite.function +++ b/tests/suites/test_suite_pkwrite.function @@ -5,7 +5,7 @@ /* END_HEADER */ /* BEGIN_DEPENDENCIES - * depends_on:MBEDTLS_PK_WRITE_C:MBEDTLS_BIGNUM_C:MBEDTLS_FS_IO + * depends_on:MBEDTLS_PK_PARSE_C:MBEDTLS_PK_WRITE_C:MBEDTLS_BIGNUM_C:MBEDTLS_FS_IO * END_DEPENDENCIES */ From 0fbbc64fee0b83981baac2752fef5dd4fb094302 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 16 Oct 2018 13:48:23 +0100 Subject: [PATCH 28/79] Add dependency of mbedtls_asn1_write_len() test on ASN.1 parsing --- tests/suites/test_suite_asn1write.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_asn1write.function b/tests/suites/test_suite_asn1write.function index aae44a8c6a..57a9741254 100644 --- a/tests/suites/test_suite_asn1write.function +++ b/tests/suites/test_suite_asn1write.function @@ -78,7 +78,7 @@ void mbedtls_asn1_write_ia5_string( char * str, data_t * asn1, } /* END_CASE */ -/* BEGIN_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_ASN1PARSE_C */ void mbedtls_asn1_write_len( int len, data_t * asn1, int buf_len, int result ) { From 27516979c14459d5519700b47765c8fd684d29cc Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 18 Oct 2018 11:49:25 +0100 Subject: [PATCH 29/79] Entropy: Fall through to /dev/random if getrandom() syscall unknown This commit fixes issue #1212 related to platform-specific entropy polling in an syscall-emulated environment. Previously, the implementation of the entropy gathering function `mbedtls_platform_entropy_poll()` for linux machines used the following logic to determine how to obtain entropy from the kernel: 1. If the getrandom() system call identifier SYS_getrandom is present and the kernel version is 3.17 or higher, use syscall( SYS_getrandom, ... ) 2. Otherwise, fall back to reading from /dev/random. There are two issues with this: 1. Portability: When cross-compiling the code for a different architecture and running it through system call emulation in qemu, qemu reports the host kernel version through uname but, as of v.2.5.0, doesn't support emulating the getrandom() syscall. This leads to `mbedtls_platform_entropy_poll()` failing even though reading from /dev/random would have worked. 2. Style: Extracting the linux kernel version from the output of `uname` is slightly tedious. This commit fixes both by implementing the suggestion in #1212: - It removes the kernel-version detection through uname(). - Instead, it checks whether `syscall( SYS_getrandom, ... )` fails with errno set to ENOSYS indicating an unknown system call. If so, it falls through to trying to read from /dev/random. Fixes #1212. --- library/entropy_poll.c | 57 +++++++----------------------------------- 1 file changed, 9 insertions(+), 48 deletions(-) diff --git a/library/entropy_poll.c b/library/entropy_poll.c index 040aa117dc..ae3d2d5b66 100644 --- a/library/entropy_poll.c +++ b/library/entropy_poll.c @@ -99,6 +99,7 @@ int mbedtls_platform_entropy_poll( void *data, unsigned char *output, size_t len #include #if defined(SYS_getrandom) #define HAVE_GETRANDOM +#include static int getrandom_wrapper( void *buf, size_t buflen, unsigned int flags ) { @@ -108,47 +109,8 @@ static int getrandom_wrapper( void *buf, size_t buflen, unsigned int flags ) memset( buf, 0, buflen ); #endif #endif - return( syscall( SYS_getrandom, buf, buflen, flags ) ); } - -#include -/* Check if version is at least 3.17.0 */ -static int check_version_3_17_plus( void ) -{ - int minor; - struct utsname un; - const char *ver; - - /* Get version information */ - uname(&un); - ver = un.release; - - /* Check major version; assume a single digit */ - if( ver[0] < '3' || ver[0] > '9' || ver [1] != '.' ) - return( -1 ); - - if( ver[0] - '0' > 3 ) - return( 0 ); - - /* Ok, so now we know major == 3, check minor. - * Assume 1 or 2 digits. */ - if( ver[2] < '0' || ver[2] > '9' ) - return( -1 ); - - minor = ver[2] - '0'; - - if( ver[3] >= '0' && ver[3] <= '9' ) - minor = 10 * minor + ver[3] - '0'; - else if( ver [3] != '.' ) - return( -1 ); - - if( minor < 17 ) - return( -1 ); - - return( 0 ); -} -static int has_getrandom = -1; #endif /* SYS_getrandom */ #endif /* __linux__ */ @@ -159,22 +121,21 @@ int mbedtls_platform_entropy_poll( void *data, { FILE *file; size_t read_len; + int ret; ((void) data); #if defined(HAVE_GETRANDOM) - if( has_getrandom == -1 ) - has_getrandom = ( check_version_3_17_plus() == 0 ); - - if( has_getrandom ) + ret = getrandom_wrapper( output, len, 0 ); + if( ret >= 0 ) { - int ret; - - if( ( ret = getrandom_wrapper( output, len, 0 ) ) < 0 ) - return( MBEDTLS_ERR_ENTROPY_SOURCE_FAILED ); - *olen = ret; return( 0 ); } + else if( errno != ENOSYS ) + return( MBEDTLS_ERR_ENTROPY_SOURCE_FAILED ); + /* Fall through if the system call isn't known. */ +#else + ((void) ret; #endif /* HAVE_GETRANDOM */ *olen = 0; From 5e0924cb52965169eadc73410061454610338026 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 18 Oct 2018 12:11:54 +0100 Subject: [PATCH 30/79] Adapt ChangeLog --- ChangeLog | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ChangeLog b/ChangeLog index 513f24f3ab..a7e41100b3 100644 --- a/ChangeLog +++ b/ChangeLog @@ -7,6 +7,9 @@ Bugfix invalidated keys of a lifetime of less than a 1s. Fixes #1968. * Fix failure in hmac_drbg in the benchmark sample application, when MBEDTLS_THREADING_C is defined. Found by TrinityTonic, #1095 + * Fix runtime error in `mbedtls_platform_entropy_poll()` when run + through qemu user emulation. Reported and fix suggested by randombit + in #1212. Fixes #1212. Changes * Add tests for session resumption in DTLS. From f343de12dc39d6430ee0f435faab61e7cc438c8f Mon Sep 17 00:00:00 2001 From: Brian J Murray Date: Mon, 22 Oct 2018 16:40:49 -0700 Subject: [PATCH 31/79] typo fix --- library/ecp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ecp.c b/library/ecp.c index 9e2c085bb6..b8364183d7 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -407,7 +407,7 @@ int mbedtls_ecp_is_zero( mbedtls_ecp_point *pt ) } /* - * Compare two points lazyly + * Compare two points lazily */ int mbedtls_ecp_point_cmp( const mbedtls_ecp_point *P, const mbedtls_ecp_point *Q ) From 9543373668c933027eb2976307db91ff71529b1e Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 24 Oct 2018 13:31:49 +0100 Subject: [PATCH 32/79] Use brackets around shift operations Use `( x >> y ) & z` instead of `x >> y & z`. Both are equivalent by operator precedence, but the former is more readable and the commonly used idiom in the library. --- library/ssl_ticket.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/ssl_ticket.c b/library/ssl_ticket.c index 985b7cd507..97ea6419ca 100644 --- a/library/ssl_ticket.c +++ b/library/ssl_ticket.c @@ -188,9 +188,9 @@ static int ssl_save_session( const mbedtls_ssl_session *session, if( left < 3 + cert_len ) return( MBEDTLS_ERR_SSL_BUFFER_TOO_SMALL ); - *p++ = (unsigned char)( cert_len >> 16 & 0xFF ); - *p++ = (unsigned char)( cert_len >> 8 & 0xFF ); - *p++ = (unsigned char)( cert_len & 0xFF ); + *p++ = (unsigned char)( ( cert_len >> 16 ) & 0xFF ); + *p++ = (unsigned char)( ( cert_len >> 8 ) & 0xFF ); + *p++ = (unsigned char)( ( cert_len ) & 0xFF ); if( session->peer_cert != NULL ) memcpy( p, session->peer_cert->raw.p, cert_len ); From 137015c1b12c4cbf7ca60c49911f59f48b5600e4 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 24 Oct 2018 13:33:02 +0100 Subject: [PATCH 33/79] Fix unsafe bounds checks in ssl_load_session() Fixes #659 reported by Guido Vranken. --- library/ssl_ticket.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/ssl_ticket.c b/library/ssl_ticket.c index 97ea6419ca..5a5445f2f1 100644 --- a/library/ssl_ticket.c +++ b/library/ssl_ticket.c @@ -215,14 +215,14 @@ static int ssl_load_session( mbedtls_ssl_session *session, size_t cert_len; #endif /* MBEDTLS_X509_CRT_PARSE_C */ - if( p + sizeof( mbedtls_ssl_session ) > end ) + if( sizeof( mbedtls_ssl_session ) > (size_t)( end - p ) ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); memcpy( session, p, sizeof( mbedtls_ssl_session ) ); p += sizeof( mbedtls_ssl_session ); #if defined(MBEDTLS_X509_CRT_PARSE_C) - if( p + 3 > end ) + if( 3 > (size_t)( end - p ) ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); cert_len = ( p[0] << 16 ) | ( p[1] << 8 ) | p[2]; @@ -236,7 +236,7 @@ static int ssl_load_session( mbedtls_ssl_session *session, { int ret; - if( p + cert_len > end ) + if( cert_len > (size_t)( end - p ) ) return( MBEDTLS_ERR_SSL_BAD_INPUT_DATA ); session->peer_cert = mbedtls_calloc( 1, sizeof( mbedtls_x509_crt ) ); From 6934e9b2887e65887d9d27c5a3e12442b840648c Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 24 Oct 2018 13:41:37 +0100 Subject: [PATCH 34/79] Indentation fix --- library/ssl_ticket.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_ticket.c b/library/ssl_ticket.c index 5a5445f2f1..8492c19a8c 100644 --- a/library/ssl_ticket.c +++ b/library/ssl_ticket.c @@ -247,7 +247,7 @@ static int ssl_load_session( mbedtls_ssl_session *session, mbedtls_x509_crt_init( session->peer_cert ); if( ( ret = mbedtls_x509_crt_parse_der( session->peer_cert, - p, cert_len ) ) != 0 ) + p, cert_len ) ) != 0 ) { mbedtls_x509_crt_free( session->peer_cert ); mbedtls_free( session->peer_cert ); From a7d2fa7891ab22c886d803604b9e2453eda2fd8d Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 24 Oct 2018 13:41:43 +0100 Subject: [PATCH 35/79] Adapt ChangeLog --- ChangeLog | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ChangeLog b/ChangeLog index 513f24f3ab..58a51ff9dd 100644 --- a/ChangeLog +++ b/ChangeLog @@ -7,6 +7,9 @@ Bugfix invalidated keys of a lifetime of less than a 1s. Fixes #1968. * Fix failure in hmac_drbg in the benchmark sample application, when MBEDTLS_THREADING_C is defined. Found by TrinityTonic, #1095 + * Fix an unsafe bounds check when restoring an SSL session from a ticket. + This could lead to a buffer overflow, but only in case ticket authentication + was broken. Reported and fix suggested by Guido Vranken in #659. Changes * Add tests for session resumption in DTLS. From c388a8c394b69267153cba1168286a31fd2e4b1f Mon Sep 17 00:00:00 2001 From: Krzysztof Stachowiak Date: Wed, 31 Oct 2018 16:49:20 +0100 Subject: [PATCH 36/79] Fix typo in a test condition code --- 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 25aaff3b0b..d0d1cdb651 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -2180,7 +2180,7 @@ static int x509_crt_find_parent( } /* extra precaution against mistakes in the caller */ - if( parent == NULL ) + if( *parent == NULL ) { *parent_is_trusted = 0; *signature_is_good = 0; From 79e4f4e933983ae771ee1a2c6744e35e6ab01400 Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Fri, 5 Oct 2018 12:21:15 +0100 Subject: [PATCH 37/79] test: Print verbosely on failures in verbose mode Update the test runner to print detail about why the test failed when it fails, if the runner is running in verbose mode. --- tests/scripts/run-test-suites.pl | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/tests/scripts/run-test-suites.pl b/tests/scripts/run-test-suites.pl index 6fe6abfa5d..73f33bb615 100755 --- a/tests/scripts/run-test-suites.pl +++ b/tests/scripts/run-test-suites.pl @@ -50,10 +50,20 @@ my ($failed_suites, $total_tests_run, $failed, $suite_cases_passed, $suite_cases_failed, $suite_cases_skipped, $total_cases_passed, $total_cases_failed, $total_cases_skipped ); +sub pad_print_center { + my( $width, $padchar, $string ) = @_; + my $padlen = ( $width - length( $string ) - 2 ) / 2; + print $padchar x( $padlen ), " $string ", $padchar x( $padlen ), "\n"; +} + for my $suite (@suites) { print "$suite ", "." x ( 72 - length($suite) - 2 - 4 ), " "; - my $result = `$prefix$suite`; + my $command = "$prefix$suite"; + if( $verbose ) { + $command .= ' -v'; + } + my $result = `$command`; $suite_cases_passed = () = $result =~ /.. PASS/g; $suite_cases_failed = () = $result =~ /.. FAILED/g; @@ -64,6 +74,11 @@ for my $suite (@suites) } else { $failed_suites++; print "FAIL\n"; + if( $verbose ) { + pad_print_center( 72, '-', "Begin $suite" ); + print $result; + pad_print_center( 72, '-', "End $suite" ); + } } my ($passed, $tests, $skipped) = $result =~ /([0-9]*) \/ ([0-9]*) tests.*?([0-9]*) skipped/; From f4b521dd10a4d336e1409af2458be2f216e2d9c5 Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Fri, 5 Oct 2018 12:45:15 +0100 Subject: [PATCH 38/79] test: Use GetOpt::Long for argument parsing Simplify argument parsing by using a core perl library for parsing arguments. --- tests/scripts/run-test-suites.pl | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/scripts/run-test-suites.pl b/tests/scripts/run-test-suites.pl index 73f33bb615..237326aff2 100755 --- a/tests/scripts/run-test-suites.pl +++ b/tests/scripts/run-test-suites.pl @@ -24,14 +24,10 @@ use strict; use utf8; use open qw(:std utf8); -use constant FALSE => 0; -use constant TRUE => 1; +use Getopt::Long; my $verbose; -my $switch = shift; -if ( defined($switch) && ( $switch eq "-v" || $switch eq "--verbose" ) ) { - $verbose = TRUE; -} +GetOptions( "verbose|v" => \$verbose ); # All test suites = executable files, excluding source files, debug # and profiling information, etc. We can't just grep {! /\./} because From 8396a71449d9632aa962e0ab66de5b5de3c368df Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Fri, 5 Oct 2018 13:23:35 +0100 Subject: [PATCH 39/79] test: Enable multiple levels of verbosity Enable passing a number to "-v" in order to set the level of verbosity. Print detailed test failure information at verbosity level 1 or higher. Display summary messages at the verbosity level 2 or higher. Print detailed test information at verbosity level 3 or higher, whether the test failed or not. This enables a more readable output style that includes detailed failure information when a failure occurs. --- tests/scripts/run-test-suites.pl | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/tests/scripts/run-test-suites.pl b/tests/scripts/run-test-suites.pl index 237326aff2..d0d4046215 100755 --- a/tests/scripts/run-test-suites.pl +++ b/tests/scripts/run-test-suites.pl @@ -26,8 +26,8 @@ use open qw(:std utf8); use Getopt::Long; -my $verbose; -GetOptions( "verbose|v" => \$verbose ); +my $verbose = 0; +GetOptions( "verbose|v:1" => \$verbose ); # All test suites = executable files, excluding source files, debug # and profiling information, etc. We can't just grep {! /\./} because @@ -67,6 +67,11 @@ for my $suite (@suites) if( $result =~ /PASSED/ ) { print "PASS\n"; + if( $verbose > 2 ) { + pad_print_center( 72, '-', "Begin $suite" ); + print $result; + pad_print_center( 72, '-', "End $suite" ); + } } else { $failed_suites++; print "FAIL\n"; @@ -80,7 +85,7 @@ for my $suite (@suites) my ($passed, $tests, $skipped) = $result =~ /([0-9]*) \/ ([0-9]*) tests.*?([0-9]*) skipped/; $total_tests_run += $tests - $skipped; - if ( $verbose ) { + if( $verbose > 1 ) { print "(test cases passed:", $suite_cases_passed, " failed:", $suite_cases_failed, " skipped:", $suite_cases_skipped, @@ -98,7 +103,7 @@ print "-" x 72, "\n"; print $failed_suites ? "FAILED" : "PASSED"; printf " (%d suites, %d tests run)\n", scalar @suites, $total_tests_run; -if ( $verbose ) { +if( $verbose > 1 ) { print " test cases passed :", $total_cases_passed, "\n"; print " failed :", $total_cases_failed, "\n"; print " skipped :", $total_cases_skipped, "\n"; From c242eea7327109977c82a74d7416f76008a5ba1f Mon Sep 17 00:00:00 2001 From: Ron Eldor Date: Mon, 5 Nov 2018 16:22:36 +0200 Subject: [PATCH 40/79] Change data file suffix for ott Change the suffix of the data files searched in `mbedtls_test.py` to `datax` as the generated files have this suffix. --- tests/scripts/mbedtls_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/scripts/mbedtls_test.py b/tests/scripts/mbedtls_test.py index 8e8a89ba91..f9e88cf999 100755 --- a/tests/scripts/mbedtls_test.py +++ b/tests/scripts/mbedtls_test.py @@ -185,7 +185,7 @@ class MbedTlsTest(BaseHostTest): binary_path = self.get_config_item('image_path') script_dir = os.path.split(os.path.abspath(__file__))[0] suite_name = os.path.splitext(os.path.basename(binary_path))[0] - data_file = ".".join((suite_name, 'data')) + data_file = ".".join((suite_name, 'datax')) data_file = os.path.join(script_dir, '..', 'mbedtls', suite_name, data_file) if os.path.exists(data_file): From e2dae7e1f5cfe4bdca22d62b6b2c3c1bde4ddca1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 5 Nov 2018 16:54:40 +0000 Subject: [PATCH 41/79] Add explicit integer to enumeration casts to programs/pkey/gen_key.c Fixes #2170. --- programs/pkey/gen_key.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/programs/pkey/gen_key.c b/programs/pkey/gen_key.c index f01bf5fcdd..31abb0cb8e 100644 --- a/programs/pkey/gen_key.c +++ b/programs/pkey/gen_key.c @@ -322,7 +322,8 @@ int main( int argc, char *argv[] ) mbedtls_printf( "\n . Generating the private key ..." ); fflush( stdout ); - if( ( ret = mbedtls_pk_setup( &key, mbedtls_pk_info_from_type( opt.type ) ) ) != 0 ) + if( ( ret = mbedtls_pk_setup( &key, + mbedtls_pk_info_from_type( (mbedtls_pk_type_t) opt.type ) ) ) != 0 ) { mbedtls_printf( " failed\n ! mbedtls_pk_setup returned -0x%04x", -ret ); goto exit; @@ -344,7 +345,8 @@ int main( int argc, char *argv[] ) #if defined(MBEDTLS_ECP_C) if( opt.type == MBEDTLS_PK_ECKEY ) { - ret = mbedtls_ecp_gen_key( opt.ec_curve, mbedtls_pk_ec( key ), + ret = mbedtls_ecp_gen_key( (mbedtls_ecp_group_id) opt.ec_curve, + mbedtls_pk_ec( key ), mbedtls_ctr_drbg_random, &ctr_drbg ); if( ret != 0 ) { From 9772da8792a75881d935db6eb9e073fb112d5888 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 5 Nov 2018 11:43:07 +0000 Subject: [PATCH 42/79] Add missing bracket Wasn't spotted earlier because it's guarded by `! HAVE_GETRANDOM`. --- library/entropy_poll.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/entropy_poll.c b/library/entropy_poll.c index ae3d2d5b66..4556f88a55 100644 --- a/library/entropy_poll.c +++ b/library/entropy_poll.c @@ -135,7 +135,7 @@ int mbedtls_platform_entropy_poll( void *data, return( MBEDTLS_ERR_ENTROPY_SOURCE_FAILED ); /* Fall through if the system call isn't known. */ #else - ((void) ret; + ((void) ret); #endif /* HAVE_GETRANDOM */ *olen = 0; From fe936c35c1f6e18c5819ce8c30fdcdef95608636 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 5 Nov 2018 16:56:02 +0000 Subject: [PATCH 43/79] Adapt ChangeLog --- ChangeLog | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ChangeLog b/ChangeLog index 83eb554260..24a0aa9b13 100644 --- a/ChangeLog +++ b/ChangeLog @@ -44,6 +44,9 @@ Bugfix after use. * Use `mbedtls_platform_zeroize()` instead of `memset()` for zeroization of sensitive data in the example programs aescrypt2 and crypt_and_hash. + * Add explicit integer to enumeration type casts to example program + programs/pkey/gen_key which previously led to compilation failure + on some toolchains. Reported by phoenixmcallister. Fixes #2170. Changes * Removed support for Yotta as a build tool. From f6d6e3082077fea70b5559a60994c1f847172211 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Wed, 7 Nov 2018 11:57:51 +0000 Subject: [PATCH 44/79] Fix incomplete assertion in ssl_write_handshake_msg() ssl_write_handshake_msg() includes the assertion that `ssl->handshake != NULL` when handling a record which is (a) a handshake message, and NOT (b) a HelloRequest. However, it later calls `ssl_append_flight()` for any record different from a HelloRequest handshake record, that is, records satisfying !(a) || !(b), instead of (a) && !(b) as covered by the assertion (specifically, CCS or Alert records). Since `ssl_append_flight()` assumes that `ssl->handshake != NULL`, this rightfully triggers static analyzer warnings. This commit expands the scope of the assertion to check that `ssl->handshake != NULL` for any record which is not a HelloRequest. --- library/ssl_tls.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 82e65251f0..4dd291052f 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -3200,8 +3200,10 @@ int mbedtls_ssl_write_handshake_msg( mbedtls_ssl_context *ssl ) } } - if( ssl->out_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE && - hs_type != MBEDTLS_SSL_HS_HELLO_REQUEST && + /* Whenever we send anything different from a + * HelloRequest we should be in a handshake - double check. */ + if( ! ( ssl->out_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE && + hs_type == MBEDTLS_SSL_HS_HELLO_REQUEST ) && ssl->handshake == NULL ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); @@ -3295,8 +3297,8 @@ int mbedtls_ssl_write_handshake_msg( mbedtls_ssl_context *ssl ) /* Either send now, or just save to be sent (and resent) later */ #if defined(MBEDTLS_SSL_PROTO_DTLS) if( ssl->conf->transport == MBEDTLS_SSL_TRANSPORT_DATAGRAM && - ( ssl->out_msgtype != MBEDTLS_SSL_MSG_HANDSHAKE || - hs_type != MBEDTLS_SSL_HS_HELLO_REQUEST ) ) + ! ( ssl->out_msgtype == MBEDTLS_SSL_MSG_HANDSHAKE && + hs_type == MBEDTLS_SSL_HS_HELLO_REQUEST ) ) { if( ( ret = ssl_flight_append( ssl ) ) != 0 ) { From 7a977881b422c85e1a0e56a91afa7cad08326d48 Mon Sep 17 00:00:00 2001 From: Ron Eldor Date: Mon, 19 Nov 2018 13:45:22 +0200 Subject: [PATCH 45/79] Change buf size to a valid size Change the size of `buf` to a valid hash size, in `ecdsa_prim_random()` --- tests/suites/test_suite_ecdsa.function | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_ecdsa.function b/tests/suites/test_suite_ecdsa.function index 7f89952940..71078329b6 100644 --- a/tests/suites/test_suite_ecdsa.function +++ b/tests/suites/test_suite_ecdsa.function @@ -14,7 +14,7 @@ void ecdsa_prim_random( int id ) mbedtls_ecp_point Q; mbedtls_mpi d, r, s; rnd_pseudo_info rnd_info; - unsigned char buf[66]; + unsigned char buf[MBEDTLS_MD_MAX_SIZE]; mbedtls_ecp_group_init( &grp ); mbedtls_ecp_point_init( &Q ); From 11cdb0559e46dbe9301bd37a36d583e3d2fefd3b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 20 Nov 2018 16:47:47 +0100 Subject: [PATCH 46/79] mbedtls_mpi_write_binary: don't leak the exact size of the number In mbedtls_mpi_write_binary, avoid leaking the size of the number through timing or branches, if possible. More precisely, if the number fits in the output buffer based on its allocated size, the new code's trace doesn't depend on the value of the number. --- library/bignum.c | 45 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/library/bignum.c b/library/bignum.c index 423e375fd1..6e4ceeebf7 100644 --- a/library/bignum.c +++ b/library/bignum.c @@ -321,6 +321,10 @@ int mbedtls_mpi_get_bit( const mbedtls_mpi *X, size_t pos ) return( ( X->p[pos / biL] >> ( pos % biL ) ) & 0x01 ); } +/* Get a specific byte, without range checks. */ +#define GET_BYTE( X, i ) \ + ( ( ( X )->p[( i ) / ciL] >> ( ( ( i ) % ciL ) * 8 ) ) & 0xff ) + /* * Set a bit to a specific value of 0 or 1 */ @@ -704,19 +708,40 @@ cleanup: /* * Export X into unsigned binary data, big endian */ -int mbedtls_mpi_write_binary( const mbedtls_mpi *X, unsigned char *buf, size_t buflen ) +int mbedtls_mpi_write_binary( const mbedtls_mpi *X, + unsigned char *buf, size_t buflen ) { - size_t i, j, n; + size_t stored_bytes = X->n * ciL; + size_t bytes_to_copy; + unsigned char *p; + size_t i; - n = mbedtls_mpi_size( X ); + if( stored_bytes < buflen ) + { + /* There is enough space in the output buffer. Write initial + * null bytes and record the position at which to start + * writing the significant bytes. In this case, the execution + * trace of this function does not depend on the value of the + * number. */ + bytes_to_copy = stored_bytes; + p = buf + buflen - stored_bytes; + memset( buf, 0, buflen - stored_bytes ); + } + else + { + /* The output buffer is smaller than the allocated size of X. + * However X may fit if its leading bytes are zero. */ + bytes_to_copy = buflen; + p = buf; + for( i = bytes_to_copy; i < stored_bytes; i++ ) + { + if( GET_BYTE( X, i ) != 0 ) + return( MBEDTLS_ERR_MPI_BUFFER_TOO_SMALL ); + } + } - if( buflen < n ) - return( MBEDTLS_ERR_MPI_BUFFER_TOO_SMALL ); - - memset( buf, 0, buflen ); - - for( i = buflen - 1, j = 0; n > 0; i--, j++, n-- ) - buf[i] = (unsigned char)( X->p[j / ciL] >> ((j % ciL) << 3) ); + for( i = 0; i < bytes_to_copy; i++ ) + p[bytes_to_copy - i - 1] = GET_BYTE( X, i ); return( 0 ); } From 3459c749fb6eb5dc8cc8ee9961505b5d23c12a32 Mon Sep 17 00:00:00 2001 From: Simon Butcher Date: Thu, 22 Nov 2018 10:14:03 +0000 Subject: [PATCH 47/79] Create a block list for Travis CI, and fix the Coverity email --- .travis.yml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 4d23652c67..4fc31c923f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,6 +4,13 @@ compiler: - gcc sudo: false cache: ccache + +# blocklist +branches: + except: + - development-psa + - coverity_scan + script: - tests/scripts/recursion.pl library/*.c - tests/scripts/check-generated-files.sh @@ -34,7 +41,7 @@ addons: coverity_scan: project: name: "ARMmbed/mbedtls" - notification_email: p.j.bakker@polarssl.org + notification_email: simon.butcher@arm.com build_command_prepend: build_command: make branch_pattern: coverity_scan From 80a23a5bc4d14e03f02fc3f4a3ba60ced179d85d Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Fri, 23 Nov 2018 10:33:20 +0000 Subject: [PATCH 48/79] check-files: Don't check same-named files The check-files script contains the strings "TODO" and "todo" in order to search for files that contain TODO items. So, any check-files script would need to be excluded from the list of files that gets checked for "TODO". Normally, the script excludes itself from checks, but with the addition of the crypto submodule, there is another copy of the script present from the project root. We must avoid checking check-files scripts for TODO items. This also helps if you run check-files from another working tree in your working tree. --- tests/scripts/check-files.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/scripts/check-files.py b/tests/scripts/check-files.py index ed6787289a..e4339b1b53 100755 --- a/tests/scripts/check-files.py +++ b/tests/scripts/check-files.py @@ -138,7 +138,9 @@ class TodoIssueTracker(IssueTracker): super().__init__() self.heading = "TODO present:" self.files_exemptions = [ - __file__, "benchmark.c", "pull_request_template.md" + os.path.basename(__file__), + "benchmark.c", + "pull_request_template.md", ] def issue_with_line(self, line): From 043980585c29feee1b03091adccdf5fcba60aab8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 23 Nov 2018 21:11:30 +0100 Subject: [PATCH 49/79] Factor record_issue into its own method --- tests/scripts/check-files.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/scripts/check-files.py b/tests/scripts/check-files.py index ed6787289a..8fce8be5a4 100755 --- a/tests/scripts/check-files.py +++ b/tests/scripts/check-files.py @@ -43,11 +43,14 @@ class IssueTracker(object): for i, line in enumerate(iter(f.readline, b"")): self.check_file_line(filepath, line, i + 1) + def record_issue(self, filepath, line_number): + if filepath not in self.files_with_issues.keys(): + self.files_with_issues[filepath] = [] + self.files_with_issues[filepath].append(line_number) + def check_file_line(self, filepath, line, line_number): if self.issue_with_line(line): - if filepath not in self.files_with_issues.keys(): - self.files_with_issues[filepath] = [] - self.files_with_issues[filepath].append(line_number) + self.record_issue(filepath, line_number) def output_file_issues(self, logger): if self.files_with_issues.values(): From c117d5928cf5411f36555d57ea962a7d50460362 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 23 Nov 2018 21:11:52 +0100 Subject: [PATCH 50/79] check-files: detect merge artifacts Detect Git merge artifacts. These are lines starting with "<<<<<<", "|||||||" or ">>>>>>>" followed by a space, or containing just "=======". For "=======", exempt Markdown files, because this can be used to underline a title, as a compromise between false negatives and false positives. --- tests/scripts/check-files.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/scripts/check-files.py b/tests/scripts/check-files.py index 8fce8be5a4..b263e4d275 100755 --- a/tests/scripts/check-files.py +++ b/tests/scripts/check-files.py @@ -135,6 +135,27 @@ class TabIssueTracker(IssueTracker): return b"\t" in line +class MergeArtifactIssueTracker(IssueTracker): + + def __init__(self): + super().__init__() + self.heading = "Merge artifact:" + + def issue_with_line(self, filepath, line): + # Detect leftover git conflict markers. + if line.startswith(b'<<<<<<< ') or line.startswith(b'>>>>>>> '): + return True + if line.startswith(b'||||||| '): # from merge.conflictStyle=diff3 + return True + if line.rstrip(b'\r\n') == b'=======' and \ + not filepath.endswith('.md'): + return True + return False + + def check_file_line(self, filepath, line, line_number): + if self.issue_with_line(filepath, line): + self.record_issue(filepath, line_number) + class TodoIssueTracker(IssueTracker): def __init__(self): @@ -170,6 +191,7 @@ class IntegrityChecker(object): LineEndingIssueTracker(), TrailingWhitespaceIssueTracker(), TabIssueTracker(), + MergeArtifactIssueTracker(), TodoIssueTracker(), ] From d9aa84dc0d42dcb5e23ba2bb47ce39592193b8f1 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 11 Sep 2018 15:34:17 +0200 Subject: [PATCH 51/79] CTR_DRBG: clean stack buffers Wipe stack buffers that may contain sensitive data (data that contributes to the DRBG state. --- library/ctr_drbg.c | 44 ++++++++++++++++++-------------------------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/library/ctr_drbg.c b/library/ctr_drbg.c index fead18f723..5595381213 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -299,9 +299,7 @@ static int ctr_drbg_update_internal( mbedtls_ctr_drbg_context *ctx, * Crypt counter block */ if( ( ret = mbedtls_aes_crypt_ecb( &ctx->aes_ctx, MBEDTLS_AES_ENCRYPT, ctx->counter, p ) ) != 0 ) - { - return( ret ); - } + goto exit; p += MBEDTLS_CTR_DRBG_BLOCKSIZE; } @@ -313,12 +311,12 @@ static int ctr_drbg_update_internal( mbedtls_ctr_drbg_context *ctx, * Update key and counter */ if( ( ret = mbedtls_aes_setkey_enc( &ctx->aes_ctx, tmp, MBEDTLS_CTR_DRBG_KEYBITS ) ) != 0 ) - { - return( ret ); - } + goto exit; memcpy( ctx->counter, tmp + MBEDTLS_CTR_DRBG_KEYSIZE, MBEDTLS_CTR_DRBG_BLOCKSIZE ); - return( 0 ); +exit: + mbedtls_platform_zeroize( tmp, sizeof( tmp ) ); + return( ret ); } /* CTR_DRBG_Instantiate with derivation function (SP 800-90A §10.2.1.3.2) @@ -347,6 +345,7 @@ void mbedtls_ctr_drbg_update( mbedtls_ctr_drbg_context *ctx, block_cipher_df( add_input, additional, add_len ); ctr_drbg_update_internal( ctx, add_input ); + mbedtls_platform_zeroize( add_input, sizeof( add_input ) ); } } @@ -399,20 +398,18 @@ int mbedtls_ctr_drbg_reseed( mbedtls_ctr_drbg_context *ctx, * Reduce to 384 bits */ if( ( ret = block_cipher_df( seed, seed, seedlen ) ) != 0 ) - { - return( ret ); - } + goto exit; /* * Update state */ if( ( ret = ctr_drbg_update_internal( ctx, seed ) ) != 0 ) - { - return( ret ); - } + goto exit; ctx->reseed_counter = 1; - return( 0 ); +exit: + mbedtls_platform_zeroize( seed, sizeof( seed ) ); + return( ret ); } /* CTR_DRBG_Generate with derivation function (SP 800-90A §10.2.1.5.2) @@ -467,13 +464,9 @@ int mbedtls_ctr_drbg_random_with_add( void *p_rng, if( add_len > 0 ) { if( ( ret = block_cipher_df( add_input, additional, add_len ) ) != 0 ) - { - return( ret ); - } + goto exit; if( ( ret = ctr_drbg_update_internal( ctx, add_input ) ) != 0 ) - { - return( ret ); - } + goto exit; } while( output_len > 0 ) @@ -489,9 +482,7 @@ int mbedtls_ctr_drbg_random_with_add( void *p_rng, * Crypt counter block */ if( ( ret = mbedtls_aes_crypt_ecb( &ctx->aes_ctx, MBEDTLS_AES_ENCRYPT, ctx->counter, tmp ) ) != 0 ) - { - return( ret ); - } + goto exit; use_len = ( output_len > MBEDTLS_CTR_DRBG_BLOCKSIZE ) ? MBEDTLS_CTR_DRBG_BLOCKSIZE : output_len; @@ -504,12 +495,13 @@ int mbedtls_ctr_drbg_random_with_add( void *p_rng, } if( ( ret = ctr_drbg_update_internal( ctx, add_input ) ) != 0 ) - { - return( ret ); - } + goto exit; ctx->reseed_counter++; +exit: + mbedtls_platform_zeroize( add_input, sizeof( add_input ) ); + mbedtls_platform_zeroize( tmp, sizeof( tmp ) ); return( 0 ); } From afa803775a1bf4f5bd0cd147bf4984785b576a04 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 11 Sep 2018 15:35:41 +0200 Subject: [PATCH 52/79] HMAC_DRBG: clean stack buffers Wipe stack buffers that may contain sensitive data (data that contributes to the DRBG state. --- library/hmac_drbg.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/hmac_drbg.c b/library/hmac_drbg.c index dad55ff861..65c8daccd8 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -89,6 +89,8 @@ void mbedtls_hmac_drbg_update( mbedtls_hmac_drbg_context *ctx, mbedtls_md_hmac_update( &ctx->md_ctx, ctx->V, md_len ); mbedtls_md_hmac_finish( &ctx->md_ctx, ctx->V ); } + + mbedtls_platform_zeroize( K, sizeof( K ) ); } /* @@ -154,6 +156,7 @@ int mbedtls_hmac_drbg_reseed( mbedtls_hmac_drbg_context *ctx, ctx->reseed_counter = 1; /* 4. Done */ + mbedtls_platform_zeroize( seed, seedlen ); return( 0 ); } From 1b09f4027e1b2ad6c383e8f1f50ce8a9bc9e5804 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 11 Sep 2018 18:53:58 +0200 Subject: [PATCH 53/79] Add ChangeLog entry for wiping sensitive buffers --- ChangeLog | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ChangeLog b/ChangeLog index 8f0e8c1c79..3d0c7e8ba6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS 2.x.x branch released xxxx-xx-xx + +Security + * Wipe sensitive buffers on the stack in the CTR_DRBG and HMAC_DRBG + modules. + = mbed TLS 2.14.0 branch released 2018-11-19 Security From d919993b76e51e6d822e5eb0af1d7ce1e23964bf Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 11 Sep 2018 16:41:54 +0200 Subject: [PATCH 54/79] CTR_DRBG: deprecate mbedtls_ctr_drbg_update because it ignores errors Deprecate mbedtls_ctr_drbg_update (which returns void) in favor of a new function mbedtls_ctr_drbg_update_ret which reports error. --- include/mbedtls/ctr_drbg.h | 36 ++++++++++++++++++-- library/ctr_drbg.c | 41 ++++++++++++++++------- tests/suites/test_suite_ctr_drbg.function | 8 +++-- 3 files changed, 67 insertions(+), 18 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index c91ca58b35..9a3aba0d3f 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -248,9 +248,12 @@ int mbedtls_ctr_drbg_reseed( mbedtls_ctr_drbg_context *ctx, * \param additional The data to update the state with. * \param add_len Length of \p additional data. * + * \return \c 0 on success. + * \return An error from the underlying AES cipher on failure. */ -void mbedtls_ctr_drbg_update( mbedtls_ctr_drbg_context *ctx, - const unsigned char *additional, size_t add_len ); +int mbedtls_ctr_drbg_update_ret( mbedtls_ctr_drbg_context *ctx, + const unsigned char *additional, + size_t add_len ); /** * \brief This function updates a CTR_DRBG instance with additional @@ -290,6 +293,35 @@ int mbedtls_ctr_drbg_random_with_add( void *p_rng, int mbedtls_ctr_drbg_random( void *p_rng, unsigned char *output, size_t output_len ); + +#if ! defined(MBEDTLS_DEPRECATED_REMOVED) +#if defined(MBEDTLS_DEPRECATED_WARNING) +#define MBEDTLS_DEPRECATED __attribute__((deprecated)) +#else +#define MBEDTLS_DEPRECATED +#endif +/** + * \brief This function updates the state of the CTR_DRBG context. + * + * \deprecated Superseded by mbedtls_ctr_drbg_update_ret() + * in 2.16.0. + * + * \note If \p add_len is greater than + * #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT, only the first + * #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT Bytes are used. + * The remaining Bytes are silently discarded. + * + * \param ctx The CTR_DRBG context. + * \param additional The data to update the state with. + * \param add_len Length of \p additional data. + */ +MBEDTLS_DEPRECATED void mbedtls_ctr_drbg_update( + mbedtls_ctr_drbg_context *ctx, + const unsigned char *additional, + size_t add_len ); +#undef MBEDTLS_DEPRECATED +#endif /* !MBEDTLS_DEPRECATED_REMOVED */ + #if defined(MBEDTLS_FS_IO) /** * \brief This function writes a seed file. diff --git a/library/ctr_drbg.c b/library/ctr_drbg.c index 5595381213..a264dedb07 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -331,24 +331,39 @@ exit: * and with outputs * ctx = initial_working_state */ -void mbedtls_ctr_drbg_update( mbedtls_ctr_drbg_context *ctx, - const unsigned char *additional, size_t add_len ) +int mbedtls_ctr_drbg_update_ret( mbedtls_ctr_drbg_context *ctx, + const unsigned char *additional, + size_t add_len ) { unsigned char add_input[MBEDTLS_CTR_DRBG_SEEDLEN]; + int ret; - if( add_len > 0 ) - { - /* MAX_INPUT would be more logical here, but we have to match - * block_cipher_df()'s limits since we can't propagate errors */ - if( add_len > MBEDTLS_CTR_DRBG_MAX_SEED_INPUT ) - add_len = MBEDTLS_CTR_DRBG_MAX_SEED_INPUT; + if( add_len == 0 ) + return( 0 ); - block_cipher_df( add_input, additional, add_len ); - ctr_drbg_update_internal( ctx, add_input ); - mbedtls_platform_zeroize( add_input, sizeof( add_input ) ); - } + if( ( ret = block_cipher_df( add_input, additional, add_len ) ) != 0 ) + goto exit; + if( ( ret = ctr_drbg_update_internal( ctx, add_input ) ) != 0 ) + goto exit; + +exit: + mbedtls_platform_zeroize( add_input, sizeof( add_input ) ); + return( ret ); } +#if !defined(MBEDTLS_DEPRECATED_REMOVED) +void mbedtls_ctr_drbg_update( mbedtls_ctr_drbg_context *ctx, + const unsigned char *additional, + size_t add_len ) +{ + /* MAX_INPUT would be more logical here, but we have to match + * block_cipher_df()'s limits since we can't propagate errors */ + if( add_len > MBEDTLS_CTR_DRBG_MAX_SEED_INPUT ) + add_len = MBEDTLS_CTR_DRBG_MAX_SEED_INPUT; + (void) mbedtls_ctr_drbg_update_ret( ctx, additional, add_len ); +} +#endif /* MBEDTLS_DEPRECATED_REMOVED */ + /* CTR_DRBG_Reseed with derivation function (SP 800-90A §10.2.1.4.2) * mbedtls_ctr_drbg_reseed(ctx, additional, len) * implements @@ -573,7 +588,7 @@ int mbedtls_ctr_drbg_update_seed_file( mbedtls_ctr_drbg_context *ctx, const char if( fread( buf, 1, n, f ) != n ) ret = MBEDTLS_ERR_CTR_DRBG_FILE_IO_ERROR; else - mbedtls_ctr_drbg_update( ctx, buf, n ); + ret = mbedtls_ctr_drbg_update_ret( ctx, buf, n ); fclose( f ); diff --git a/tests/suites/test_suite_ctr_drbg.function b/tests/suites/test_suite_ctr_drbg.function index f10e98aa54..4a97826f63 100644 --- a/tests/suites/test_suite_ctr_drbg.function +++ b/tests/suites/test_suite_ctr_drbg.function @@ -244,9 +244,11 @@ void ctr_drbg_entropy_usage( ) } TEST_ASSERT( last_idx == test_offset_idx ); - /* Call update with too much data (sizeof entropy > MAX(_SEED)_INPUT) - * (just make sure it doesn't cause memory corruption) */ - mbedtls_ctr_drbg_update( &ctx, entropy, sizeof( entropy ) ); + /* Call update with too much data (sizeof entropy > MAX(_SEED)_INPUT). + * Make sure it's detected as an error and doesn't cause memory + * corruption. */ + TEST_ASSERT( mbedtls_ctr_drbg_update_ret( + &ctx, entropy, sizeof( entropy ) ) != 0 ); /* Now enable PR, so the next few calls should all reseed */ mbedtls_ctr_drbg_set_prediction_resistance( &ctx, MBEDTLS_CTR_DRBG_PR_ON ); From e0e9c573ad613bdb897cd548ec55a96b3f843190 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 11 Sep 2018 16:47:16 +0200 Subject: [PATCH 55/79] HMAC_DRBG: deprecate mbedtls_hmac_drbg_update because it ignores errors Deprecate mbedtls_hmac_drbg_update (which returns void) in favor of a new function mbedtls_hmac_drbg_update_ret which reports error. --- include/mbedtls/hmac_drbg.h | 30 ++++++++++++++- library/hmac_drbg.c | 73 ++++++++++++++++++++++++++++--------- 2 files changed, 84 insertions(+), 19 deletions(-) diff --git a/include/mbedtls/hmac_drbg.h b/include/mbedtls/hmac_drbg.h index 3bc675ec7c..146367b9de 100644 --- a/include/mbedtls/hmac_drbg.h +++ b/include/mbedtls/hmac_drbg.h @@ -195,10 +195,13 @@ void mbedtls_hmac_drbg_set_reseed_interval( mbedtls_hmac_drbg_context *ctx, * \param additional Additional data to update state with, or NULL * \param add_len Length of additional data, or 0 * + * \return \c 0 on success, or an error from the underlying + * hash calculation. + * * \note Additional data is optional, pass NULL and 0 as second * third argument if no additional data is being used. */ -void mbedtls_hmac_drbg_update( mbedtls_hmac_drbg_context *ctx, +int mbedtls_hmac_drbg_update_ret( mbedtls_hmac_drbg_context *ctx, const unsigned char *additional, size_t add_len ); /** @@ -257,6 +260,31 @@ int mbedtls_hmac_drbg_random( void *p_rng, unsigned char *output, size_t out_len */ void mbedtls_hmac_drbg_free( mbedtls_hmac_drbg_context *ctx ); +#if ! defined(MBEDTLS_DEPRECATED_REMOVED) +#if defined(MBEDTLS_DEPRECATED_WARNING) +#define MBEDTLS_DEPRECATED __attribute__((deprecated)) +#else +#define MBEDTLS_DEPRECATED +#endif +/** + * \brief HMAC_DRBG update state + * + * \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. + */ +MBEDTLS_DEPRECATED void mbedtls_hmac_drbg_update( + mbedtls_hmac_drbg_context *ctx, + const unsigned char *additional, size_t add_len ); +#undef MBEDTLS_DEPRECATED +#endif /* !MBEDTLS_DEPRECATED_REMOVED */ + #if defined(MBEDTLS_FS_IO) /** * \brief Write a seed file diff --git a/library/hmac_drbg.c b/library/hmac_drbg.c index 65c8daccd8..7f65354775 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -66,33 +66,60 @@ void mbedtls_hmac_drbg_init( mbedtls_hmac_drbg_context *ctx ) /* * HMAC_DRBG update, using optional additional data (10.1.2.2) */ -void mbedtls_hmac_drbg_update( mbedtls_hmac_drbg_context *ctx, - const unsigned char *additional, size_t add_len ) +int mbedtls_hmac_drbg_update_ret( mbedtls_hmac_drbg_context *ctx, + const unsigned char *additional, + size_t add_len ) { size_t md_len = mbedtls_md_get_size( ctx->md_ctx.md_info ); unsigned char rounds = ( additional != NULL && add_len != 0 ) ? 2 : 1; unsigned char sep[1]; unsigned char K[MBEDTLS_MD_MAX_SIZE]; + int ret; for( sep[0] = 0; sep[0] < rounds; sep[0]++ ) { /* Step 1 or 4 */ - mbedtls_md_hmac_reset( &ctx->md_ctx ); - mbedtls_md_hmac_update( &ctx->md_ctx, ctx->V, md_len ); - mbedtls_md_hmac_update( &ctx->md_ctx, sep, 1 ); + if( ( ret = mbedtls_md_hmac_reset( &ctx->md_ctx ) ) != 0 ) + goto exit; + if( ( ret = mbedtls_md_hmac_update( &ctx->md_ctx, + ctx->V, md_len ) ) != 0 ) + goto exit; + if( ( ret = mbedtls_md_hmac_update( &ctx->md_ctx, + sep, 1 ) ) != 0 ) + goto exit; if( rounds == 2 ) - mbedtls_md_hmac_update( &ctx->md_ctx, additional, add_len ); - mbedtls_md_hmac_finish( &ctx->md_ctx, K ); + { + if( ( ret = mbedtls_md_hmac_update( &ctx->md_ctx, + additional, add_len ) ) != 0 ) + goto exit; + } + if( ( ret = mbedtls_md_hmac_finish( &ctx->md_ctx, K ) ) != 0 ) + goto exit; /* Step 2 or 5 */ - mbedtls_md_hmac_starts( &ctx->md_ctx, K, md_len ); - mbedtls_md_hmac_update( &ctx->md_ctx, ctx->V, md_len ); - mbedtls_md_hmac_finish( &ctx->md_ctx, ctx->V ); + if( ( ret = mbedtls_md_hmac_starts( &ctx->md_ctx, K, md_len ) ) != 0 ) + goto exit; + if( ( ret = mbedtls_md_hmac_update( &ctx->md_ctx, + ctx->V, md_len ) ) != 0 ) + goto exit; + if( ( ret = mbedtls_md_hmac_finish( &ctx->md_ctx, ctx->V ) ) != 0 ) + goto exit; } +exit: mbedtls_platform_zeroize( K, sizeof( K ) ); + return( ret ); } +#if !defined(MBEDTLS_DEPRECATED_REMOVED) +void mbedtls_hmac_drbg_update( mbedtls_hmac_drbg_context *ctx, + const unsigned char *additional, + size_t add_len ) +{ + (void) mbedtls_hmac_drbg_update_ret( ctx, additional, add_len ); +} +#endif /* MBEDTLS_DEPRECATED_REMOVED */ + /* * Simplified HMAC_DRBG initialisation (for use with deterministic ECDSA) */ @@ -113,7 +140,8 @@ int mbedtls_hmac_drbg_seed_buf( mbedtls_hmac_drbg_context *ctx, mbedtls_md_hmac_starts( &ctx->md_ctx, ctx->V, mbedtls_md_get_size( md_info ) ); memset( ctx->V, 0x01, mbedtls_md_get_size( md_info ) ); - mbedtls_hmac_drbg_update( ctx, data, data_len ); + if( ( ret = mbedtls_hmac_drbg_update_ret( ctx, data, data_len ) ) != 0 ) + return( ret ); return( 0 ); } @@ -126,6 +154,7 @@ int mbedtls_hmac_drbg_reseed( mbedtls_hmac_drbg_context *ctx, { unsigned char seed[MBEDTLS_HMAC_DRBG_MAX_SEED_INPUT]; size_t seedlen; + int ret; /* III. Check input length */ if( len > MBEDTLS_HMAC_DRBG_MAX_INPUT || @@ -150,14 +179,16 @@ int mbedtls_hmac_drbg_reseed( mbedtls_hmac_drbg_context *ctx, } /* 2. Update state */ - mbedtls_hmac_drbg_update( ctx, seed, seedlen ); + if( ( ret = mbedtls_hmac_drbg_update_ret( ctx, seed, seedlen ) ) != 0 ) + goto exit; /* 3. Reset reseed_counter */ ctx->reseed_counter = 1; +exit: /* 4. Done */ mbedtls_platform_zeroize( seed, seedlen ); - return( 0 ); + return( ret ); } /* @@ -276,7 +307,11 @@ int mbedtls_hmac_drbg_random_with_add( void *p_rng, /* 2. Use additional data if any */ if( additional != NULL && add_len != 0 ) - mbedtls_hmac_drbg_update( ctx, additional, add_len ); + { + if( ( ret = mbedtls_hmac_drbg_update_ret( ctx, + additional, add_len ) ) != 0 ) + goto exit; + } /* 3, 4, 5. Generate bytes */ while( left != 0 ) @@ -293,13 +328,16 @@ int mbedtls_hmac_drbg_random_with_add( void *p_rng, } /* 6. Update */ - mbedtls_hmac_drbg_update( ctx, additional, add_len ); + if( ( ret = mbedtls_hmac_drbg_update_ret( ctx, + additional, add_len ) ) != 0 ) + goto exit; /* 7. Update reseed counter */ ctx->reseed_counter++; +exit: /* 8. Done */ - return( 0 ); + return( ret ); } /* @@ -391,8 +429,7 @@ int mbedtls_hmac_drbg_update_seed_file( mbedtls_hmac_drbg_context *ctx, const ch if( fread( buf, 1, n, f ) != n ) ret = MBEDTLS_ERR_HMAC_DRBG_FILE_IO_ERROR; else - mbedtls_hmac_drbg_update( ctx, buf, n ); - + ret = mbedtls_hmac_drbg_update_ret( ctx, buf, n ); fclose( f ); mbedtls_platform_zeroize( buf, sizeof( buf ) ); From b7f71c8bc1f84584b7aec8c6c02bbd693d3be21f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 11 Sep 2018 16:54:57 +0200 Subject: [PATCH 56/79] HMAC_DRBG: report all errors from HMAC functions Make sure that any error from mbedtls_md_hmac_xxx is propagated. --- library/hmac_drbg.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/library/hmac_drbg.c b/library/hmac_drbg.c index 7f65354775..d5ac2f6ad0 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -137,7 +137,9 @@ int mbedtls_hmac_drbg_seed_buf( mbedtls_hmac_drbg_context *ctx, * Use the V memory location, which is currently all 0, to initialize the * MD context with an all-zero key. Then set V to its initial value. */ - mbedtls_md_hmac_starts( &ctx->md_ctx, ctx->V, mbedtls_md_get_size( md_info ) ); + if( ( ret = mbedtls_md_hmac_starts( &ctx->md_ctx, ctx->V, + mbedtls_md_get_size( md_info ) ) ) != 0 ) + return( ret ); memset( ctx->V, 0x01, mbedtls_md_get_size( md_info ) ); if( ( ret = mbedtls_hmac_drbg_update_ret( ctx, data, data_len ) ) != 0 ) @@ -166,7 +168,8 @@ int mbedtls_hmac_drbg_reseed( mbedtls_hmac_drbg_context *ctx, memset( seed, 0, MBEDTLS_HMAC_DRBG_MAX_SEED_INPUT ); /* IV. Gather entropy_len bytes of entropy for the seed */ - if( ctx->f_entropy( ctx->p_entropy, seed, ctx->entropy_len ) != 0 ) + if( ( ret = ctx->f_entropy( ctx->p_entropy, + seed, ctx->entropy_len ) ) != 0 ) return( MBEDTLS_ERR_HMAC_DRBG_ENTROPY_SOURCE_FAILED ); seedlen = ctx->entropy_len; @@ -214,7 +217,8 @@ int mbedtls_hmac_drbg_seed( mbedtls_hmac_drbg_context *ctx, * Use the V memory location, which is currently all 0, to initialize the * MD context with an all-zero key. Then set V to its initial value. */ - mbedtls_md_hmac_starts( &ctx->md_ctx, ctx->V, md_size ); + if( ( ret = mbedtls_md_hmac_starts( &ctx->md_ctx, ctx->V, md_size ) ) != 0 ) + return( ret ); memset( ctx->V, 0x01, md_size ); ctx->f_entropy = f_entropy; @@ -318,9 +322,13 @@ int mbedtls_hmac_drbg_random_with_add( void *p_rng, { size_t use_len = left > md_len ? md_len : left; - mbedtls_md_hmac_reset( &ctx->md_ctx ); - mbedtls_md_hmac_update( &ctx->md_ctx, ctx->V, md_len ); - mbedtls_md_hmac_finish( &ctx->md_ctx, ctx->V ); + if( ( ret = mbedtls_md_hmac_reset( &ctx->md_ctx ) ) != 0 ) + goto exit; + if( ( ret = mbedtls_md_hmac_update( &ctx->md_ctx, + ctx->V, md_len ) ) != 0 ) + goto exit; + if( ( ret = mbedtls_md_hmac_finish( &ctx->md_ctx, ctx->V ) ) != 0 ) + goto exit; memcpy( out, ctx->V, use_len ); out += use_len; @@ -430,6 +438,7 @@ int mbedtls_hmac_drbg_update_seed_file( mbedtls_hmac_drbg_context *ctx, const ch ret = MBEDTLS_ERR_HMAC_DRBG_FILE_IO_ERROR; else ret = mbedtls_hmac_drbg_update_ret( ctx, buf, n ); + fclose( f ); mbedtls_platform_zeroize( buf, sizeof( buf ) ); From 8220466297a06e91944151d4a1e2f7781e3011ae Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 11 Sep 2018 18:43:09 +0200 Subject: [PATCH 57/79] Streamline mbedtls_xxx_drbg_update_seed_file Refactor mbedtls_ctr_drbg_update_seed_file and mbedtls_hmac_drbg_update_seed_file to make the error logic clearer. The new code does not use fseek, so it works with non-seekable files. --- library/ctr_drbg.c | 31 ++++++++++++++++--------------- library/hmac_drbg.c | 31 ++++++++++++++++--------------- 2 files changed, 32 insertions(+), 30 deletions(-) diff --git a/library/ctr_drbg.c b/library/ctr_drbg.c index a264dedb07..fb121575bb 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -568,35 +568,36 @@ exit: int mbedtls_ctr_drbg_update_seed_file( mbedtls_ctr_drbg_context *ctx, const char *path ) { int ret = 0; - FILE *f; + FILE *f = NULL; size_t n; unsigned char buf[ MBEDTLS_CTR_DRBG_MAX_INPUT ]; + unsigned char c; if( ( f = fopen( path, "rb" ) ) == NULL ) return( MBEDTLS_ERR_CTR_DRBG_FILE_IO_ERROR ); - fseek( f, 0, SEEK_END ); - n = (size_t) ftell( f ); - fseek( f, 0, SEEK_SET ); - - if( n > MBEDTLS_CTR_DRBG_MAX_INPUT ) + n = fread( buf, 1, sizeof( buf ), f ); + if( fread( &c, 1, 1, f ) != 0 ) { - fclose( f ); - return( MBEDTLS_ERR_CTR_DRBG_INPUT_TOO_BIG ); + ret = MBEDTLS_ERR_CTR_DRBG_INPUT_TOO_BIG; + goto exit; } - - if( fread( buf, 1, n, f ) != n ) + if( n == 0 || ferror( f ) ) + { ret = MBEDTLS_ERR_CTR_DRBG_FILE_IO_ERROR; - else - ret = mbedtls_ctr_drbg_update_ret( ctx, buf, n ); - + goto exit; + } fclose( f ); + f = NULL; + ret = mbedtls_ctr_drbg_update_ret( ctx, buf, n ); + +exit: mbedtls_platform_zeroize( buf, sizeof( buf ) ); - + if( f != NULL ) + fclose( f ); if( ret != 0 ) return( ret ); - return( mbedtls_ctr_drbg_write_seed_file( ctx, path ) ); } #endif /* MBEDTLS_FS_IO */ diff --git a/library/hmac_drbg.c b/library/hmac_drbg.c index d5ac2f6ad0..c50330e7d8 100644 --- a/library/hmac_drbg.c +++ b/library/hmac_drbg.c @@ -417,35 +417,36 @@ exit: int mbedtls_hmac_drbg_update_seed_file( mbedtls_hmac_drbg_context *ctx, const char *path ) { int ret = 0; - FILE *f; + FILE *f = NULL; size_t n; unsigned char buf[ MBEDTLS_HMAC_DRBG_MAX_INPUT ]; + unsigned char c; if( ( f = fopen( path, "rb" ) ) == NULL ) return( MBEDTLS_ERR_HMAC_DRBG_FILE_IO_ERROR ); - fseek( f, 0, SEEK_END ); - n = (size_t) ftell( f ); - fseek( f, 0, SEEK_SET ); - - if( n > MBEDTLS_HMAC_DRBG_MAX_INPUT ) + n = fread( buf, 1, sizeof( buf ), f ); + if( fread( &c, 1, 1, f ) != 0 ) { - fclose( f ); - return( MBEDTLS_ERR_HMAC_DRBG_INPUT_TOO_BIG ); + ret = MBEDTLS_ERR_HMAC_DRBG_INPUT_TOO_BIG; + goto exit; } - - if( fread( buf, 1, n, f ) != n ) + if( n == 0 || ferror( f ) ) + { ret = MBEDTLS_ERR_HMAC_DRBG_FILE_IO_ERROR; - else - ret = mbedtls_hmac_drbg_update_ret( ctx, buf, n ); - + goto exit; + } fclose( f ); + f = NULL; + ret = mbedtls_hmac_drbg_update_ret( ctx, buf, n ); + +exit: mbedtls_platform_zeroize( buf, sizeof( buf ) ); - + if( f != NULL ) + fclose( f ); if( ret != 0 ) return( ret ); - return( mbedtls_hmac_drbg_write_seed_file( ctx, path ) ); } #endif /* MBEDTLS_FS_IO */ From 5da0505842aca20e129d33b21b07d703aa1de51c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 11 Sep 2018 18:59:55 +0200 Subject: [PATCH 58/79] Add ChangeLog entry for deprecation of mbedtls_xxx_drbg_update Fixes ARMmbed/mbedtls#1798 --- ChangeLog | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/ChangeLog b/ChangeLog index 3d0c7e8ba6..a3e0aeb88d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -6,6 +6,19 @@ Security * Wipe sensitive buffers on the stack in the CTR_DRBG and HMAC_DRBG modules. +API Changes + * The following functions in the random generator modules have been + deprecated and replaced as shown below. The new functions change + the return type from void to int to allow returning error codes when + using MBEDTLS__ALT for the underlying AES or message digest + primitive. Fixes #1798. + mbedtls_ctr_drbg_update() -> mbedtls_ctr_drbg_update_ret() + mbedtls_hmac_drbg_update() -> mbedtls_hmac_drbg_update_ret() + +New deprecations + * Deprecate mbedtls_ctr_drbg_update and mbedtls_hmac_drbg_update + in favor of functions that can return an error code. + = mbed TLS 2.14.0 branch released 2018-11-19 Security From c4a8017e3ed289f2600948489f0a60b2648e35cd Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 12 Sep 2018 19:15:53 +0200 Subject: [PATCH 59/79] mbedtls_ctr_drbg_update_ret: correct doc for input length limit Unlike mbedtls_ctr_drbg_update, this function returns an error if the length limit is exceeded, rather than silently truncating the input. --- include/mbedtls/ctr_drbg.h | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/include/mbedtls/ctr_drbg.h b/include/mbedtls/ctr_drbg.h index 9a3aba0d3f..10f9389d9f 100644 --- a/include/mbedtls/ctr_drbg.h +++ b/include/mbedtls/ctr_drbg.h @@ -239,16 +239,15 @@ int mbedtls_ctr_drbg_reseed( mbedtls_ctr_drbg_context *ctx, /** * \brief This function updates the state of the CTR_DRBG context. * - * \note If \p add_len is greater than - * #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT, only the first - * #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT Bytes are used. - * The remaining Bytes are silently discarded. - * * \param ctx The CTR_DRBG context. * \param additional The data to update the state with. - * \param add_len Length of \p additional data. + * \param add_len Length of \p additional in bytes. This must be at + * most #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT. * * \return \c 0 on success. + * \return #MBEDTLS_ERR_CTR_DRBG_INPUT_TOO_BIG if + * \p add_len is more than + * #MBEDTLS_CTR_DRBG_MAX_SEED_INPUT. * \return An error from the underlying AES cipher on failure. */ int mbedtls_ctr_drbg_update_ret( mbedtls_ctr_drbg_context *ctx, From 056f19c79f14414a7b3745d29825783dce3a85af Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 29 Nov 2018 12:45:01 +0100 Subject: [PATCH 60/79] Tweak RSA vulnerability changelog entry * Correct the list of authors. * Add the CVE number. * Improve the impact description. --- ChangeLog | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index 0bb9ec04a4..4f90d560ba 100644 --- a/ChangeLog +++ b/ChangeLog @@ -5,9 +5,10 @@ mbed TLS ChangeLog (Sorted per branch, date) Security * Fix timing variations and memory access variations in RSA PKCS#1 v1.5 decryption that could lead to a Bleichenbacher-style padding oracle - attack. In TLS, this affects RSA-based ciphersuites without DHE or - ECDHE. Reported by Yuval Yarom, Eyal Ronen, Adi Shamir, David Wong and - Daniel Genkin. + attack. In TLS, this affects servers that accept ciphersuites based on + RSA decryption (i.e. ciphersuites whose name contains RSA but not + (EC)DH(E)). Reported by Eyal Ronen, Robert Gillham, Daniel Genkin, Adi + Shamir, David Wong and Yuval Yarom. CVE-2018-19608 = mbed TLS 2.13.1 branch released 2018-09-06 From 50da016e5ce5eacdc7c41f55122ec8450f0b94c4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 29 Nov 2018 12:46:05 +0100 Subject: [PATCH 61/79] Add changelog entry for mbedtls_mpi_write_binary fix --- ChangeLog | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ChangeLog b/ChangeLog index 4f90d560ba..5d2e8db181 100644 --- a/ChangeLog +++ b/ChangeLog @@ -9,6 +9,13 @@ Security RSA decryption (i.e. ciphersuites whose name contains RSA but not (EC)DH(E)). Reported by Eyal Ronen, Robert Gillham, Daniel Genkin, Adi Shamir, David Wong and Yuval Yarom. CVE-2018-19608 + * In mbedtls_mpi_write_binary(), don't leak the exact size of the number + via branching and memory access patterns. An attacker who could submit + a plaintext for RSA PKCS#1 v1.5 decryption but only observe the timing + of the decryption and not its result could nonetheless decrypt RSA + plaintexts and forge RSA signatures. Other asymmetric algorithms may + have been similarly vulnerable. Reported by Eyal Ronen, Robert Gillham, + Daniel Genkin, Adi Shamir, David Wong and Yuval Yarom. = mbed TLS 2.13.1 branch released 2018-09-06 From 89ac8c9266184bb8edc2c04111f7b0df220bb440 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 30 Oct 2018 11:24:05 +0000 Subject: [PATCH 62/79] ECP: Add mbedtls_ecp_tls_read_group_id() `mbedtls_ecp_tls_read_group()` both parses the group ID and loads the group into the structure provided. We want to support alternative implementations of ECDH in the future and for that we need to parse the group ID without populating an `mbedtls_ecp_group` structure (because alternative implementations might not use that). This commit moves the part that parses the group ID to a new function. There is no need to test the new function directly, because the tests for `mbedtls_ecp_tls_read_group()` are already implicitly testing it. There is no intended change in behaviour in this commit. --- include/mbedtls/ecp.h | 31 ++++++++++++++++++++++++++----- library/ecp.c | 23 +++++++++++++++++++++-- 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index 2fb1af49a2..0e5b47fc21 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -632,7 +632,7 @@ int mbedtls_ecp_point_read_binary( const mbedtls_ecp_group *grp, mbedtls_ecp_poi /** * \brief This function imports a point from a TLS ECPoint record. * - * \note On function return, \p buf is updated to point to immediately + * \note On function return, \p *buf is updated to point immediately * after the ECPoint record. * * \param grp The ECP group used. @@ -641,7 +641,8 @@ int mbedtls_ecp_point_read_binary( const mbedtls_ecp_group *grp, mbedtls_ecp_poi * \param len The length of the buffer. * * \return \c 0 on success. - * \return An \c MBEDTLS_ERR_MPI_XXX error code on initialization failure. + * \return An \c MBEDTLS_ERR_MPI_XXX error code on initialization + * failure. * \return #MBEDTLS_ERR_ECP_BAD_INPUT_DATA if input is invalid. */ int mbedtls_ecp_tls_read_point( const mbedtls_ecp_group *grp, mbedtls_ecp_point *pt, @@ -687,19 +688,39 @@ int mbedtls_ecp_group_load( mbedtls_ecp_group *grp, mbedtls_ecp_group_id id ); /** * \brief This function sets a group from a TLS ECParameters record. * - * \note \p buf is updated to point right after the ECParameters record - * on exit. + * \note \p buf is updated to point right after the ECParameters + * record on exit. * * \param grp The destination group. * \param buf The address of the pointer to the start of the input buffer. * \param len The length of the buffer. * * \return \c 0 on success. - * \return An \c MBEDTLS_ERR_MPI_XXX error code on initialization failure. + * \return An \c MBEDTLS_ERR_MPI_XXX error code on initialization + * failure. * \return #MBEDTLS_ERR_ECP_BAD_INPUT_DATA if input is invalid. + * \return #MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE if the group is not + * recognised. */ int mbedtls_ecp_tls_read_group( mbedtls_ecp_group *grp, const unsigned char **buf, size_t len ); +/** + * \brief This function reads a group from a TLS ECParameters record. + * + * \note \p buf is updated to point right after the ECParameters + * record on exit. + * + * \param grp Output parameter to hold the group id. + * \param buf The address of the pointer to the start of the input buffer. + * \param len The length of the buffer. + * + * \return \c 0 on success. + * \return #MBEDTLS_ERR_ECP_BAD_INPUT_DATA if input is invalid. + * \return #MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE if the group is not + * recognised. + */ +int mbedtls_ecp_tls_read_group_id( mbedtls_ecp_group_id *grp, + const unsigned char **buf, size_t len ); /** * \brief This function writes the TLS ECParameters record for a group. * diff --git a/library/ecp.c b/library/ecp.c index de5725c700..49255b1036 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -833,7 +833,24 @@ int mbedtls_ecp_tls_write_point( const mbedtls_ecp_group *grp, const mbedtls_ecp /* * Set a group from an ECParameters record (RFC 4492) */ -int mbedtls_ecp_tls_read_group( mbedtls_ecp_group *grp, const unsigned char **buf, size_t len ) +int mbedtls_ecp_tls_read_group( mbedtls_ecp_group *grp, + const unsigned char **buf, size_t len ) +{ + int ret; + mbedtls_ecp_group_id grp_id; + + if( ( ret = mbedtls_ecp_tls_read_group_id( &grp_id, buf, len ) ) != 0 ) + return( ret ); + + return mbedtls_ecp_group_load( grp, grp_id ); +} + +/* + * Read a group id from an ECParameters record (RFC 4492) and convert it to + * mbedtls_ecp_group_id. + */ +int mbedtls_ecp_tls_read_group_id( mbedtls_ecp_group_id *grp, + const unsigned char **buf, size_t len ) { uint16_t tls_id; const mbedtls_ecp_curve_info *curve_info; @@ -860,7 +877,9 @@ int mbedtls_ecp_tls_read_group( mbedtls_ecp_group *grp, const unsigned char **bu if( ( curve_info = mbedtls_ecp_curve_info_from_tls_id( tls_id ) ) == NULL ) return( MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE ); - return mbedtls_ecp_group_load( grp, curve_info->grp_id ); + *grp = curve_info->grp_id; + + return( 0 ); } /* From f61e486179843ea7b6346066629b1bfa9f37b5fc Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 30 Oct 2018 11:53:25 +0000 Subject: [PATCH 63/79] ECDH: Add mbedtls_ecdh_setup() In the future we want to support alternative ECDH implementations. We can't make assumptions about the structure of the context they might use, and therefore shouldn't access the members of `mbedtls_ecdh_context`. Currently the lifecycle of the context can't be done without direct manipulation. This commit adds `mbedtls_ecdh_setup()` to complete covering the context lifecycle with functions. --- include/mbedtls/ecdh.h | 27 +++++++++++++++++++++++---- library/ecdh.c | 30 ++++++++++++++++++++++++++---- 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/include/mbedtls/ecdh.h b/include/mbedtls/ecdh.h index 27f2ffc6aa..68a6989c82 100644 --- a/include/mbedtls/ecdh.h +++ b/include/mbedtls/ecdh.h @@ -134,6 +134,24 @@ int mbedtls_ecdh_compute_shared( mbedtls_ecp_group *grp, mbedtls_mpi *z, */ void mbedtls_ecdh_init( mbedtls_ecdh_context *ctx ); +/** + * \brief This function sets up the ECDH context with the information + * given. + * + * This function should be called after mbedtls_ecdh_init() but + * before mbedtls_ecdh_make_params(). There is no need to call + * this function before mbedtls_ecdh_read_params(). + * + * This is the first function used by a TLS server for ECDHE + * ciphersuites. + * + * \param ctx The ECDH context to set up. + * \param grp_id The group id of the group to set up the context for. + * + * \return \c 0 on success. + */ +int mbedtls_ecdh_setup( mbedtls_ecdh_context *ctx, mbedtls_ecp_group_id grp_id ); + /** * \brief This function frees a context. * @@ -145,8 +163,8 @@ void mbedtls_ecdh_free( mbedtls_ecdh_context *ctx ); * \brief This function generates a public key and a TLS * ServerKeyExchange payload. * - * This is the first function used by a TLS server for ECDHE - * ciphersuites. + * This is the second function used by a TLS server for ECDHE + * ciphersuites. (It is called after mbedtls_ecdh_setup().) * * \note This function assumes that the ECP group (grp) of the * \p ctx context has already been properly set, @@ -242,8 +260,9 @@ int mbedtls_ecdh_make_public( mbedtls_ecdh_context *ctx, size_t *olen, * \brief This function parses and processes a TLS ClientKeyExchange * payload. * - * This is the second function used by a TLS server for ECDH(E) - * ciphersuites. + * This is the third function used by a TLS server for ECDH(E) + * ciphersuites. (It is called after mbedtls_ecdh_setup() and + * mbedtls_ecdh_make_params().) * * \see ecp.h * diff --git a/library/ecdh.c b/library/ecdh.c index e6ae99994e..702ba1a409 100644 --- a/library/ecdh.c +++ b/library/ecdh.c @@ -145,6 +145,23 @@ void mbedtls_ecdh_init( mbedtls_ecdh_context *ctx ) #endif } +/* + * Setup context + */ +int mbedtls_ecdh_setup( mbedtls_ecdh_context *ctx, mbedtls_ecp_group_id grp_id ) +{ + int ret; + + ret = mbedtls_ecp_group_load( &ctx->grp, grp_id ); + if( ret != 0 ) + { + mbedtls_ecdh_free( ctx ); + return( MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE ); + } + + return( 0 ); +} + /* * Free context */ @@ -240,12 +257,17 @@ int mbedtls_ecdh_read_params( mbedtls_ecdh_context *ctx, const unsigned char **buf, const unsigned char *end ) { int ret; + mbedtls_ecp_group_id grp_id; - if( ( ret = mbedtls_ecp_tls_read_group( &ctx->grp, buf, end - *buf ) ) != 0 ) + if( ( ret = mbedtls_ecp_tls_read_group_id( &grp_id, buf, end - *buf ) ) + != 0 ) return( ret ); - if( ( ret = mbedtls_ecp_tls_read_point( &ctx->grp, &ctx->Qp, buf, end - *buf ) ) - != 0 ) + if( ( ret = mbedtls_ecdh_setup( ctx, grp_id ) ) != 0 ) + return( ret ); + + if( ( ret = mbedtls_ecp_tls_read_point( &ctx->grp, &ctx->Qp, buf, + end - *buf ) ) != 0 ) return( ret ); return( 0 ); @@ -259,7 +281,7 @@ int mbedtls_ecdh_get_params( mbedtls_ecdh_context *ctx, const mbedtls_ecp_keypai { int ret; - if( ( ret = mbedtls_ecp_group_copy( &ctx->grp, &key->grp ) ) != 0 ) + if( ( ret = mbedtls_ecdh_setup( ctx, key->grp.id ) ) != 0 ) return( ret ); /* If it's not our key, just import the public part as Qp */ From fc03e8dfa9912ff08a5431cf11bcad6da4da754a Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 4 Oct 2018 17:17:54 +0100 Subject: [PATCH 64/79] ECDH: Adapt tests for mbedtls_ecdh_setup() The recently added `mbedtls_ecdh_setup()` function is not used in the tests yet. This commit adapts the tests to the new workflow. Having done that, the old lifecycle is not tested anymore, so we add a new test to ensure backward compatibility. --- tests/suites/test_suite_ecdh.data | 4 +++ tests/suites/test_suite_ecdh.function | 44 +++++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_ecdh.data b/tests/suites/test_suite_ecdh.data index 0165a7e0a4..89e5e3a803 100644 --- a/tests/suites/test_suite_ecdh.data +++ b/tests/suites/test_suite_ecdh.data @@ -69,3 +69,7 @@ ecdh_restart:MBEDTLS_ECP_DP_SECP256R1:"C88F01F510D9AC3F70A292DAA2316DE544E9AAB8A ECDH restartable rfc 5903 p256 restart disabled max_ops=250 depends_on:MBEDTLS_ECP_DP_SECP256R1_ENABLED ecdh_restart:MBEDTLS_ECP_DP_SECP256R1:"C88F01F510D9AC3F70A292DAA2316DE544E9AAB8AFE84049C62A9C57862D1433":"C6EF9C5D78AE012A011164ACB397CE2088685D8F06BF9BE0B283AB46476BEE53":"D6840F6B42F6EDAFD13116E0E12565202FEF8E9ECE7DCE03812464D04B9442DE":0:250:0:0 + +ECDH exchange legacy context +depends_on:MBEDTLS_ECP_DP_SECP192R1_ENABLED +ecdh_exchange_legacy:MBEDTLS_ECP_DP_SECP192R1 diff --git a/tests/suites/test_suite_ecdh.function b/tests/suites/test_suite_ecdh.function index 965230885e..52fecf350a 100644 --- a/tests/suites/test_suite_ecdh.function +++ b/tests/suites/test_suite_ecdh.function @@ -134,16 +134,16 @@ void ecdh_exchange( int id ) mbedtls_ecdh_init( &cli ); memset( &rnd_info, 0x00, sizeof( rnd_pseudo_info ) ); - TEST_ASSERT( mbedtls_ecp_group_load( &srv.grp, id ) == 0 ); + TEST_ASSERT( mbedtls_ecdh_setup( &srv, id ) == 0 ); memset( buf, 0x00, sizeof( buf ) ); vbuf = buf; TEST_ASSERT( mbedtls_ecdh_make_params( &srv, &len, buf, 1000, - &rnd_pseudo_rand, &rnd_info ) == 0 ); + &rnd_pseudo_rand, &rnd_info ) == 0 ); TEST_ASSERT( mbedtls_ecdh_read_params( &cli, &vbuf, buf + len ) == 0 ); memset( buf, 0x00, sizeof( buf ) ); TEST_ASSERT( mbedtls_ecdh_make_public( &cli, &len, buf, 1000, - &rnd_pseudo_rand, &rnd_info ) == 0 ); + &rnd_pseudo_rand, &rnd_info ) == 0 ); TEST_ASSERT( mbedtls_ecdh_read_public( &srv, buf, len ) == 0 ); TEST_ASSERT( mbedtls_ecdh_calc_secret( &srv, &len, buf, 1000, @@ -273,3 +273,41 @@ exit: mbedtls_ecdh_free( &cli ); } /* END_CASE */ + +/* BEGIN_CASE */ +void ecdh_exchange_legacy( int id ) +{ + mbedtls_ecdh_context srv, cli; + unsigned char buf[1000]; + const unsigned char *vbuf; + size_t len; + + rnd_pseudo_info rnd_info; + + mbedtls_ecdh_init( &srv ); + mbedtls_ecdh_init( &cli ); + memset( &rnd_info, 0x00, sizeof( rnd_pseudo_info ) ); + + TEST_ASSERT( mbedtls_ecp_group_load( &srv.grp, id ) == 0 ); + + memset( buf, 0x00, sizeof( buf ) ); vbuf = buf; + TEST_ASSERT( mbedtls_ecdh_make_params( &srv, &len, buf, 1000, + &rnd_pseudo_rand, &rnd_info ) == 0 ); + TEST_ASSERT( mbedtls_ecdh_read_params( &cli, &vbuf, buf + len ) == 0 ); + + memset( buf, 0x00, sizeof( buf ) ); + TEST_ASSERT( mbedtls_ecdh_make_public( &cli, &len, buf, 1000, + &rnd_pseudo_rand, &rnd_info ) == 0 ); + TEST_ASSERT( mbedtls_ecdh_read_public( &srv, buf, len ) == 0 ); + + TEST_ASSERT( mbedtls_ecdh_calc_secret( &srv, &len, buf, 1000, + &rnd_pseudo_rand, &rnd_info ) == 0 ); + TEST_ASSERT( mbedtls_ecdh_calc_secret( &cli, &len, buf, 1000, NULL, + NULL ) == 0 ); + TEST_ASSERT( mbedtls_mpi_cmp_mpi( &srv.z, &cli.z ) == 0 ); + +exit: + mbedtls_ecdh_free( &srv ); + mbedtls_ecdh_free( &cli ); +} +/* END_CASE */ From c9c32f3f639bb64153c3130110f38797392cb615 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 13 Aug 2018 15:52:45 +0100 Subject: [PATCH 65/79] ECDH: Add flexible context and legacy flag We want to support alternative software implementations and we extend the ECDH context to enable this. The actual functional change that makes use of the new context is out of scope for this commit. Changing the context breaks the API and therefore it has to be excluded from the default configuration by a compile time flag. We add the compile time flag to the module header instead of `config.h`, because this is not a standalone feature, it only enables adding new implementations in the future. The new context features a union of the individual implementations and a selector that chooses the implementation in use. An alternative is to use an opaque context and function pointers, like for example the PK module does it, but it is more dangerous, error prone and tedious to implement. We leave the group ID and the point format at the top level of the structure, because they are very simple and adding an abstraction layer around them away does not come with any obvious benefit. Other alternatives considered: - Using the module level replacement mechanism in the ECP module. This would have made the use of the replacement feature more difficult and the benefit limited. - Replacing our Montgomery implementations with a new one directly. This would have prevented using Montgomery curves across implementations. (For example use implementation A for Curve448 and implementation B for Curve22519.) Also it would have been inflexible and limited to Montgomery curves. - Encoding the implementation selector and the alternative context in `mbedtls_ecp_point` somehow and rewriting `mbedtls_ecp_mul()` to dispatch between implementations. This would have been a dangerous and ugly hack, and very likely to break legacy applications. - Same as above just with hardcoding the selector and using a compile time option to make the selection. Rejected for the same reasons as above. - Using the PK module to provide to provide an entry point for alternative implementations. Like most of the above options this wouldn't have come with a new compile time option, but conceptually would have been very out of place and would have meant much more work to complete the abstraction around the context. In retrospect: - We could have used the group ID as the selector, but this would have made the code less flexible and only marginally simpler. On the other hand it would have allowed to get rid of the compile time option if a tight integration of the alternative is possible. (It does not seem possible at this point.) - We could have used the same approach we do in this commit to the `mbedtls_ecp_point` structure. Completing the abstraction around this structure would have been a much bigger and much riskier code change with increase in memory footprint, potential decrease in performance and no immediate benefit. --- include/mbedtls/ecdh.h | 67 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/include/mbedtls/ecdh.h b/include/mbedtls/ecdh.h index 68a6989c82..cbd48414a3 100644 --- a/include/mbedtls/ecdh.h +++ b/include/mbedtls/ecdh.h @@ -36,6 +36,18 @@ #include "ecp.h" +/* + * Use a backward compatible ECDH context. + * + * This flag is always enabled for now and future versions might add a + * configuration option that conditionally undefines this flag. + * The configuration option in question may have a different name. + * + * Features undefining this flag, must have a warning in their description in + * config.h stating that the feature breaks backward compatibility. + */ +#define MBEDTLS_ECDH_LEGACY_CONTEXT + #ifdef __cplusplus extern "C" { #endif @@ -49,6 +61,39 @@ typedef enum MBEDTLS_ECDH_THEIRS, /**< The key of the peer. */ } mbedtls_ecdh_side; +#if !defined(MBEDTLS_ECDH_LEGACY_CONTEXT) +/** + * Defines the ECDH implementation used. + * + * Later versions of the library may add new variants, therefore users should + * not make any assumptions about them. + */ +typedef enum +{ + MBEDTLS_ECDH_VARIANT_NONE = 0, /*!< Implementation not defined. */ + MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0,/*!< The default Mbed TLS implementation */ +} mbedtls_ecdh_variant; + +/** + * The context used by the default ECDH implementation. + * + * Later versions might change the structure of this context, therefore users + * should not make any assumptions about the structure of + * mbedtls_ecdh_context_mbed. + */ +typedef struct mbedtls_ecdh_context_mbed +{ + mbedtls_ecp_group grp; /*!< The elliptic curve used. */ + mbedtls_mpi d; /*!< The private key. */ + mbedtls_ecp_point Q; /*!< The public key. */ + mbedtls_ecp_point Qp; /*!< The value of the public key of the peer. */ + mbedtls_mpi z; /*!< The shared secret. */ +#if defined(MBEDTLS_ECP_RESTARTABLE) + mbedtls_ecp_restart_ctx rs; /*!< The restart context for EC computations. */ +#endif +} mbedtls_ecdh_context_mbed; +#endif + /** * * \warning Performing multiple operations concurrently on the same @@ -58,6 +103,7 @@ typedef enum */ typedef struct mbedtls_ecdh_context { +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) mbedtls_ecp_group grp; /*!< The elliptic curve used. */ mbedtls_mpi d; /*!< The private key. */ mbedtls_ecp_point Q; /*!< The public key. */ @@ -70,7 +116,26 @@ typedef struct mbedtls_ecdh_context #if defined(MBEDTLS_ECP_RESTARTABLE) int restart_enabled; /*!< The flag for restartable mode. */ mbedtls_ecp_restart_ctx rs; /*!< The restart context for EC computations. */ -#endif +#endif /* MBEDTLS_ECP_RESTARTABLE */ +#else + uint8_t point_format; /*!< The format of point export in TLS messages + as defined in RFC 4492. */ + mbedtls_ecp_group_id grp_id;/*!< The elliptic curve used. */ + mbedtls_ecdh_variant var; /*!< The ECDH implementation/structure used. */ + union + { + mbedtls_ecdh_context_mbed mbed_ecdh; + } ctx; /*!< Implementation-specific context. The + context in use is specified by the \c var + field. */ +#if defined(MBEDTLS_ECP_RESTARTABLE) + uint8_t restart_enabled; /*!< The flag for restartable mode. Functions of + an alternative implementation not supporting + restartable mode must return + MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED error + if this flag is set. */ +#endif /* MBEDTLS_ECP_RESTARTABLE */ +#endif /* MBEDTLS_ECDH_LEGACY_CONTEXT */ } mbedtls_ecdh_context; From 52735ef2fec4502870877f5dfcd06cd7a36774aa Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 15 Aug 2018 10:19:16 +0100 Subject: [PATCH 66/79] ECDH: Prevent direct access in non-legacy mode Some sample programs access structure fields directly. Making these work is desirable in the long term, but these are not essential for the core functionality in non-legacy mode. --- programs/pkey/ecdh_curve25519.c | 4 ++-- programs/test/benchmark.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/programs/pkey/ecdh_curve25519.c b/programs/pkey/ecdh_curve25519.c index 5db04088f9..7fbf1678f9 100644 --- a/programs/pkey/ecdh_curve25519.c +++ b/programs/pkey/ecdh_curve25519.c @@ -35,12 +35,12 @@ #define MBEDTLS_EXIT_FAILURE EXIT_FAILURE #endif /* MBEDTLS_PLATFORM_C */ -#if !defined(MBEDTLS_ECDH_C) || \ +#if !defined(MBEDTLS_ECDH_C) || !defined(MBEDTLS_ECDH_LEGACY_CONTEXT) || \ !defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) || \ !defined(MBEDTLS_ENTROPY_C) || !defined(MBEDTLS_CTR_DRBG_C) int main( void ) { - mbedtls_printf( "MBEDTLS_ECDH_C and/or " + mbedtls_printf( "MBEDTLS_ECDH_C and/or MBEDTLS_ECDH_LEGACY_CONTEXT and/or " "MBEDTLS_ECP_DP_CURVE25519_ENABLED and/or " "MBEDTLS_ENTROPY_C and/or MBEDTLS_CTR_DRBG_C " "not defined\n" ); diff --git a/programs/test/benchmark.c b/programs/test/benchmark.c index e7d29c396f..dd4303b89d 100644 --- a/programs/test/benchmark.c +++ b/programs/test/benchmark.c @@ -862,7 +862,7 @@ int main( int argc, char *argv[] ) } #endif -#if defined(MBEDTLS_ECDH_C) +#if defined(MBEDTLS_ECDH_C) && defined(MBEDTLS_ECDH_LEGACY_CONTEXT) if( todo.ecdh ) { mbedtls_ecdh_context ecdh; From fabc6001ff1cf2132550e34248b5f16f42b7e86d Mon Sep 17 00:00:00 2001 From: Simon Butcher Date: Sat, 1 Dec 2018 22:43:08 +0000 Subject: [PATCH 67/79] Clarify attribution for the Bleichenbacher's Cat fix --- ChangeLog | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 81c7534cfe..3f23854513 100644 --- a/ChangeLog +++ b/ChangeLog @@ -7,8 +7,11 @@ Security decryption that could lead to a Bleichenbacher-style padding oracle attack. In TLS, this affects servers that accept ciphersuites based on RSA decryption (i.e. ciphersuites whose name contains RSA but not - (EC)DH(E)). Reported by Eyal Ronen, Robert Gillham, Daniel Genkin, Adi - Shamir, David Wong and Yuval Yarom. CVE-2018-19608 + (EC)DH(E)). Discovered by Eyal Ronen (Weizmann Institute), Robert Gillham + (University of Adelaide), Daniel Genkin (University of Michigan), + Adi Shamir (Weizmann Institute), David Wong (NCC Group), and Yuval Yarom + (University of Adelaide, Data61). The attack is described in more detail + in the paper available here: http://cat.eyalro.net/cat.pdf CVE-2018-19608 * In mbedtls_mpi_write_binary(), don't leak the exact size of the number via branching and memory access patterns. An attacker who could submit a plaintext for RSA PKCS#1 v1.5 decryption but only observe the timing From 5a3e1bfda0ef95f18b01a14eac21a68255c21451 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 13 Aug 2018 15:54:22 +0100 Subject: [PATCH 68/79] ECDH: Make the implementation use the new context The functionality from public API functions are moved to `xxx_internal()` functions. The public API functions are modified to do basic parameter validation and dispatch the call to the right implementation. There is no intended change in behaviour when `MBEDTLS_ECDH_LEGACY_CONTEXT` is enabled. --- library/ecdh.c | 374 ++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 304 insertions(+), 70 deletions(-) diff --git a/library/ecdh.c b/library/ecdh.c index 702ba1a409..d68db8ac74 100644 --- a/library/ecdh.c +++ b/library/ecdh.c @@ -38,6 +38,10 @@ #include +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) +typedef mbedtls_ecdh_context mbedtls_ecdh_context_mbed; +#endif + #if !defined(MBEDTLS_ECDH_GEN_PUBLIC_ALT) /* * Generate public key (restartable version) @@ -124,38 +128,48 @@ int mbedtls_ecdh_compute_shared( mbedtls_ecp_group *grp, mbedtls_mpi *z, } #endif /* !MBEDTLS_ECDH_COMPUTE_SHARED_ALT */ -/* - * Initialize context - */ -void mbedtls_ecdh_init( mbedtls_ecdh_context *ctx ) +static void ecdh_init_internal( mbedtls_ecdh_context_mbed *ctx ) { mbedtls_ecp_group_init( &ctx->grp ); mbedtls_mpi_init( &ctx->d ); mbedtls_ecp_point_init( &ctx->Q ); mbedtls_ecp_point_init( &ctx->Qp ); mbedtls_mpi_init( &ctx->z ); - ctx->point_format = MBEDTLS_ECP_PF_UNCOMPRESSED; - mbedtls_ecp_point_init( &ctx->Vi ); - mbedtls_ecp_point_init( &ctx->Vf ); - mbedtls_mpi_init( &ctx->_d ); #if defined(MBEDTLS_ECP_RESTARTABLE) - ctx->restart_enabled = 0; mbedtls_ecp_restart_init( &ctx->rs ); #endif } /* - * Setup context + * Initialize context */ -int mbedtls_ecdh_setup( mbedtls_ecdh_context *ctx, mbedtls_ecp_group_id grp_id ) +void mbedtls_ecdh_init( mbedtls_ecdh_context *ctx ) +{ +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + ecdh_init_internal( ctx ); + mbedtls_ecp_point_init( &ctx->Vi ); + mbedtls_ecp_point_init( &ctx->Vf ); + mbedtls_mpi_init( &ctx->_d ); +#else + memset( ctx, 0, sizeof( mbedtls_ecdh_context ) ); + + ctx->var = MBEDTLS_ECDH_VARIANT_NONE; +#endif + ctx->point_format = MBEDTLS_ECP_PF_UNCOMPRESSED; +#if defined(MBEDTLS_ECP_RESTARTABLE) + ctx->restart_enabled = 0; +#endif +} + +static int ecdh_setup_internal( mbedtls_ecdh_context_mbed *ctx, + mbedtls_ecp_group_id grp_id ) { int ret; ret = mbedtls_ecp_group_load( &ctx->grp, grp_id ); if( ret != 0 ) { - mbedtls_ecdh_free( ctx ); return( MBEDTLS_ERR_ECP_FEATURE_UNAVAILABLE ); } @@ -163,21 +177,35 @@ int mbedtls_ecdh_setup( mbedtls_ecdh_context *ctx, mbedtls_ecp_group_id grp_id ) } /* - * Free context + * Setup context */ -void mbedtls_ecdh_free( mbedtls_ecdh_context *ctx ) +int mbedtls_ecdh_setup( mbedtls_ecdh_context *ctx, mbedtls_ecp_group_id grp_id ) { if( ctx == NULL ) - return; + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + return( ecdh_setup_internal( ctx, grp_id ) ); +#else + switch( grp_id ) + { + default: + ctx->point_format = MBEDTLS_ECP_PF_UNCOMPRESSED; + ctx->var = MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0; + ctx->grp_id = grp_id; + ecdh_init_internal( &ctx->ctx.mbed_ecdh ); + return( ecdh_setup_internal( &ctx->ctx.mbed_ecdh, grp_id ) ); + } +#endif +} + +static void ecdh_free_internal( mbedtls_ecdh_context_mbed *ctx ) +{ mbedtls_ecp_group_free( &ctx->grp ); mbedtls_mpi_free( &ctx->d ); mbedtls_ecp_point_free( &ctx->Q ); mbedtls_ecp_point_free( &ctx->Qp ); mbedtls_mpi_free( &ctx->z ); - mbedtls_ecp_point_free( &ctx->Vi ); - mbedtls_ecp_point_free( &ctx->Vf ); - mbedtls_mpi_free( &ctx->_d ); #if defined(MBEDTLS_ECP_RESTARTABLE) mbedtls_ecp_restart_free( &ctx->rs ); @@ -190,21 +218,50 @@ void mbedtls_ecdh_free( mbedtls_ecdh_context *ctx ) */ void mbedtls_ecdh_enable_restart( mbedtls_ecdh_context *ctx ) { + if( ctx == NULL ) + return; + ctx->restart_enabled = 1; } #endif /* - * Setup and write the ServerKeyExhange parameters (RFC 4492) - * struct { - * ECParameters curve_params; - * ECPoint public; - * } ServerECDHParams; + * Free context */ -int mbedtls_ecdh_make_params( mbedtls_ecdh_context *ctx, size_t *olen, - unsigned char *buf, size_t blen, - int (*f_rng)(void *, unsigned char *, size_t), - void *p_rng ) +void mbedtls_ecdh_free( mbedtls_ecdh_context *ctx ) +{ + if( ctx == NULL ) + return; + +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + mbedtls_ecp_point_free( &ctx->Vi ); + mbedtls_ecp_point_free( &ctx->Vf ); + mbedtls_mpi_free( &ctx->_d ); + ecdh_free_internal( ctx ); +#else + switch( ctx->var ) + { + case MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0: + ecdh_free_internal( &ctx->ctx.mbed_ecdh ); + break; + default: + break; + } + + ctx->point_format = MBEDTLS_ECP_PF_UNCOMPRESSED; + ctx->var = MBEDTLS_ECDH_VARIANT_NONE; + ctx->grp_id = MBEDTLS_ECP_DP_NONE; +#endif +} + +static int ecdh_make_params_internal( mbedtls_ecdh_context_mbed *ctx, + size_t *olen, int point_format, + unsigned char *buf, size_t blen, + int (*f_rng)(void *, + unsigned char *, + size_t), + void *p_rng, + int restart_enabled ) { int ret; size_t grp_len, pt_len; @@ -212,12 +269,14 @@ int mbedtls_ecdh_make_params( mbedtls_ecdh_context *ctx, size_t *olen, mbedtls_ecp_restart_ctx *rs_ctx = NULL; #endif - if( ctx == NULL || ctx->grp.pbits == 0 ) + if( ctx->grp.pbits == 0 ) return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); #if defined(MBEDTLS_ECP_RESTARTABLE) - if( ctx->restart_enabled ) + if( restart_enabled ) rs_ctx = &ctx->rs; +#else + (void) restart_enabled; #endif @@ -231,14 +290,14 @@ int mbedtls_ecdh_make_params( mbedtls_ecdh_context *ctx, size_t *olen, return( ret ); #endif /* MBEDTLS_ECP_RESTARTABLE */ - if( ( ret = mbedtls_ecp_tls_write_group( &ctx->grp, &grp_len, buf, blen ) ) - != 0 ) + if( ( ret = mbedtls_ecp_tls_write_group( &ctx->grp, &grp_len, buf, + blen ) ) != 0 ) return( ret ); buf += grp_len; blen -= grp_len; - if( ( ret = mbedtls_ecp_tls_write_point( &ctx->grp, &ctx->Q, ctx->point_format, + if( ( ret = mbedtls_ecp_tls_write_point( &ctx->grp, &ctx->Q, point_format, &pt_len, buf, blen ) ) != 0 ) return( ret ); @@ -246,6 +305,54 @@ int mbedtls_ecdh_make_params( mbedtls_ecdh_context *ctx, size_t *olen, return( 0 ); } +/* + * Setup and write the ServerKeyExhange parameters (RFC 4492) + * struct { + * ECParameters curve_params; + * ECPoint public; + * } ServerECDHParams; + */ +int mbedtls_ecdh_make_params( mbedtls_ecdh_context *ctx, size_t *olen, + unsigned char *buf, size_t blen, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ) +{ + int restart_enabled = 0; + + if( ctx == NULL ) + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); + +#if defined(MBEDTLS_ECP_RESTARTABLE) + restart_enabled = ctx->restart_enabled; +#else + (void) restart_enabled; +#endif + +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + return( ecdh_make_params_internal( ctx, olen, ctx->point_format, buf, blen, + f_rng, p_rng, restart_enabled ) ); +#else + switch( ctx->var ) + { + case MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0: + return( ecdh_make_params_internal( &ctx->ctx.mbed_ecdh, olen, + ctx->point_format, buf, blen, + f_rng, p_rng, + restart_enabled ) ); + default: + return MBEDTLS_ERR_ECP_BAD_INPUT_DATA; + } +#endif +} + +static int ecdh_read_params_internal( mbedtls_ecdh_context_mbed *ctx, + const unsigned char **buf, + const unsigned char *end ) +{ + return( mbedtls_ecp_tls_read_point( &ctx->grp, &ctx->Qp, buf, + end - *buf ) ); +} + /* * Read the ServerKeyExhange parameters (RFC 4492) * struct { @@ -254,11 +361,15 @@ int mbedtls_ecdh_make_params( mbedtls_ecdh_context *ctx, size_t *olen, * } ServerECDHParams; */ int mbedtls_ecdh_read_params( mbedtls_ecdh_context *ctx, - const unsigned char **buf, const unsigned char *end ) + const unsigned char **buf, + const unsigned char *end ) { int ret; mbedtls_ecp_group_id grp_id; + if( ctx == NULL ) + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); + if( ( ret = mbedtls_ecp_tls_read_group_id( &grp_id, buf, end - *buf ) ) != 0 ) return( ret ); @@ -266,24 +377,26 @@ int mbedtls_ecdh_read_params( mbedtls_ecdh_context *ctx, if( ( ret = mbedtls_ecdh_setup( ctx, grp_id ) ) != 0 ) return( ret ); - if( ( ret = mbedtls_ecp_tls_read_point( &ctx->grp, &ctx->Qp, buf, - end - *buf ) ) != 0 ) - return( ret ); - - return( 0 ); +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + return( ecdh_read_params_internal( ctx, buf, end ) ); +#else + switch( ctx->var ) + { + case MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0: + return( ecdh_read_params_internal( &ctx->ctx.mbed_ecdh, + buf, end ) ); + default: + return MBEDTLS_ERR_ECP_BAD_INPUT_DATA; + } +#endif } -/* - * Get parameters from a keypair - */ -int mbedtls_ecdh_get_params( mbedtls_ecdh_context *ctx, const mbedtls_ecp_keypair *key, - mbedtls_ecdh_side side ) +static int ecdh_get_params_internal( mbedtls_ecdh_context_mbed *ctx, + const mbedtls_ecp_keypair *key, + mbedtls_ecdh_side side ) { int ret; - if( ( ret = mbedtls_ecdh_setup( ctx, key->grp.id ) ) != 0 ) - return( ret ); - /* If it's not our key, just import the public part as Qp */ if( side == MBEDTLS_ECDH_THEIRS ) return( mbedtls_ecp_copy( &ctx->Qp, &key->Q ) ); @@ -300,29 +413,61 @@ int mbedtls_ecdh_get_params( mbedtls_ecdh_context *ctx, const mbedtls_ecp_keypai } /* - * Setup and export the client public value + * Get parameters from a keypair */ -int mbedtls_ecdh_make_public( mbedtls_ecdh_context *ctx, size_t *olen, - unsigned char *buf, size_t blen, - int (*f_rng)(void *, unsigned char *, size_t), - void *p_rng ) +int mbedtls_ecdh_get_params( mbedtls_ecdh_context *ctx, + const mbedtls_ecp_keypair *key, + mbedtls_ecdh_side side ) +{ + int ret; + + if( ctx == NULL ) + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); + + if( ( ret = mbedtls_ecdh_setup( ctx, key->grp.id ) ) != 0 ) + return( ret ); + +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + return( ecdh_get_params_internal( ctx, key, side ) ); +#else + switch( ctx->var ) + { + case MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0: + return( ecdh_get_params_internal( &ctx->ctx.mbed_ecdh, + key, side ) ); + default: + return MBEDTLS_ERR_ECP_BAD_INPUT_DATA; + } +#endif +} + +static int ecdh_make_public_internal( mbedtls_ecdh_context_mbed *ctx, + size_t *olen, int point_format, + unsigned char *buf, size_t blen, + int (*f_rng)(void *, + unsigned char *, + size_t), + void *p_rng, + int restart_enabled ) { int ret; #if defined(MBEDTLS_ECP_RESTARTABLE) mbedtls_ecp_restart_ctx *rs_ctx = NULL; #endif - if( ctx == NULL || ctx->grp.pbits == 0 ) + if( ctx->grp.pbits == 0 ) return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); #if defined(MBEDTLS_ECP_RESTARTABLE) - if( ctx->restart_enabled ) + if( restart_enabled ) rs_ctx = &ctx->rs; +#else + (void) restart_enabled; #endif #if defined(MBEDTLS_ECP_RESTARTABLE) if( ( ret = ecdh_gen_public_restartable( &ctx->grp, &ctx->d, &ctx->Q, - f_rng, p_rng, rs_ctx ) ) != 0 ) + f_rng, p_rng, rs_ctx ) ) != 0 ) return( ret ); #else if( ( ret = mbedtls_ecdh_gen_public( &ctx->grp, &ctx->d, &ctx->Q, @@ -330,23 +475,52 @@ int mbedtls_ecdh_make_public( mbedtls_ecdh_context *ctx, size_t *olen, return( ret ); #endif /* MBEDTLS_ECP_RESTARTABLE */ - return mbedtls_ecp_tls_write_point( &ctx->grp, &ctx->Q, ctx->point_format, - olen, buf, blen ); + return mbedtls_ecp_tls_write_point( &ctx->grp, &ctx->Q, point_format, olen, + buf, blen ); } /* - * Parse and import the client's public value + * Setup and export the client public value */ -int mbedtls_ecdh_read_public( mbedtls_ecdh_context *ctx, - const unsigned char *buf, size_t blen ) +int mbedtls_ecdh_make_public( mbedtls_ecdh_context *ctx, size_t *olen, + unsigned char *buf, size_t blen, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ) { - int ret; - const unsigned char *p = buf; + int restart_enabled = 0; if( ctx == NULL ) return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); - if( ( ret = mbedtls_ecp_tls_read_point( &ctx->grp, &ctx->Qp, &p, blen ) ) != 0 ) +#if defined(MBEDTLS_ECP_RESTARTABLE) + restart_enabled = ctx->restart_enabled; +#endif + +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + return( ecdh_make_public_internal( ctx, olen, ctx->point_format, buf, blen, + f_rng, p_rng, restart_enabled ) ); +#else + switch( ctx->var ) + { + case MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0: + return( ecdh_make_public_internal( &ctx->ctx.mbed_ecdh, olen, + ctx->point_format, buf, blen, + f_rng, p_rng, + restart_enabled ) ); + default: + return MBEDTLS_ERR_ECP_BAD_INPUT_DATA; + } +#endif +} + +static int ecdh_read_public_internal( mbedtls_ecdh_context_mbed *ctx, + const unsigned char *buf, size_t blen ) +{ + int ret; + const unsigned char *p = buf; + + if( ( ret = mbedtls_ecp_tls_read_point( &ctx->grp, &ctx->Qp, &p, + blen ) ) != 0 ) return( ret ); if( (size_t)( p - buf ) != blen ) @@ -356,12 +530,36 @@ int mbedtls_ecdh_read_public( mbedtls_ecdh_context *ctx, } /* - * Derive and export the shared secret + * Parse and import the client's public value */ -int mbedtls_ecdh_calc_secret( mbedtls_ecdh_context *ctx, size_t *olen, - unsigned char *buf, size_t blen, - int (*f_rng)(void *, unsigned char *, size_t), - void *p_rng ) +int mbedtls_ecdh_read_public( mbedtls_ecdh_context *ctx, + const unsigned char *buf, size_t blen ) +{ + if( ctx == NULL ) + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); + +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + return( ecdh_read_public_internal( ctx, buf, blen ) ); +#else + switch( ctx->var ) + { + case MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0: + return( ecdh_read_public_internal( &ctx->ctx.mbed_ecdh, + buf, blen ) ); + default: + return MBEDTLS_ERR_ECP_BAD_INPUT_DATA; + } +#endif +} + +static int ecdh_calc_secret_internal( mbedtls_ecdh_context_mbed *ctx, + size_t *olen, unsigned char *buf, + size_t blen, + int (*f_rng)(void *, + unsigned char *, + size_t), + void *p_rng, + int restart_enabled ) { int ret; #if defined(MBEDTLS_ECP_RESTARTABLE) @@ -372,13 +570,16 @@ int mbedtls_ecdh_calc_secret( mbedtls_ecdh_context *ctx, size_t *olen, return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); #if defined(MBEDTLS_ECP_RESTARTABLE) - if( ctx->restart_enabled ) + if( restart_enabled ) rs_ctx = &ctx->rs; +#else + (void) restart_enabled; #endif #if defined(MBEDTLS_ECP_RESTARTABLE) - if( ( ret = ecdh_compute_shared_restartable( &ctx->grp, - &ctx->z, &ctx->Qp, &ctx->d, f_rng, p_rng, rs_ctx ) ) != 0 ) + if( ( ret = ecdh_compute_shared_restartable( &ctx->grp, &ctx->z, &ctx->Qp, + &ctx->d, f_rng, p_rng, + rs_ctx ) ) != 0 ) { return( ret ); } @@ -397,4 +598,37 @@ int mbedtls_ecdh_calc_secret( mbedtls_ecdh_context *ctx, size_t *olen, return mbedtls_mpi_write_binary( &ctx->z, buf, *olen ); } +/* + * Derive and export the shared secret + */ +int mbedtls_ecdh_calc_secret( mbedtls_ecdh_context *ctx, size_t *olen, + unsigned char *buf, size_t blen, + int (*f_rng)(void *, unsigned char *, size_t), + void *p_rng ) +{ + int restart_enabled = 0; + + if( ctx == NULL ) + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); + +#if defined(MBEDTLS_ECP_RESTARTABLE) + restart_enabled = ctx->restart_enabled; +#endif + +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + return( ecdh_calc_secret_internal( ctx, olen, buf, blen, f_rng, p_rng, + restart_enabled ) ); +#else + switch( ctx->var ) + { + case MBEDTLS_ECDH_VARIANT_MBEDTLS_2_0: + return( ecdh_calc_secret_internal( &ctx->ctx.mbed_ecdh, olen, buf, + blen, f_rng, p_rng, + restart_enabled ) ); + default: + return( MBEDTLS_ERR_ECP_BAD_INPUT_DATA ); + } +#endif +} + #endif /* MBEDTLS_ECDH_C */ From 948f4bedcceacd44dac08797f42019f3dad7cb13 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 22 Aug 2018 01:37:55 +0100 Subject: [PATCH 69/79] Debug: Add functions for ECDH contexts The SSL module accesses ECDH context members directly to print debug information. This can't work with the new context, where we can't make assumptions about the implementation of the context. This commit adds new debug functions to complete the encapsulation of the ECDH context and work around the problem. --- include/mbedtls/debug.h | 36 +++++++++++++++++++++++++++++ library/debug.c | 50 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/include/mbedtls/debug.h b/include/mbedtls/debug.h index ef8db67ff1..736444bb76 100644 --- a/include/mbedtls/debug.h +++ b/include/mbedtls/debug.h @@ -65,6 +65,11 @@ mbedtls_debug_print_crt( ssl, level, __FILE__, __LINE__, text, crt ) #endif +#if defined(MBEDTLS_ECDH_C) +#define MBEDTLS_SSL_DEBUG_ECDH( level, ecdh, attr ) \ + mbedtls_debug_printf_ecdh( ssl, level, __FILE__, __LINE__, ecdh, attr ) +#endif + #else /* MBEDTLS_DEBUG_C */ #define MBEDTLS_SSL_DEBUG_MSG( level, args ) do { } while( 0 ) @@ -73,6 +78,7 @@ #define MBEDTLS_SSL_DEBUG_MPI( level, text, X ) do { } while( 0 ) #define MBEDTLS_SSL_DEBUG_ECP( level, text, X ) do { } while( 0 ) #define MBEDTLS_SSL_DEBUG_CRT( level, text, crt ) do { } while( 0 ) +#define MBEDTLS_SSL_DEBUG_ECDH( level, ecdh, attr ) do { } while( 0 ) #endif /* MBEDTLS_DEBUG_C */ @@ -221,6 +227,36 @@ void mbedtls_debug_print_crt( const mbedtls_ssl_context *ssl, int level, const char *text, const mbedtls_x509_crt *crt ); #endif +#if defined(MBEDTLS_ECDH_C) +typedef enum +{ + MBEDTLS_DEBUG_ECDH_Q, + MBEDTLS_DEBUG_ECDH_QP, + MBEDTLS_DEBUG_ECDH_Z, +} mbedtls_debug_ecdh_attr; + +/** + * \brief Print a field of the ECDH structure in the SSL context to the debug + * output. This function is always used through the + * MBEDTLS_SSL_DEBUG_ECDH() macro, which supplies the ssl context, file + * and line number parameters. + * + * \param ssl SSL context + * \param level error level of the debug message + * \param file file the error has occurred in + * \param line line number the error has occurred in + * \param ecdh the ECDH context + * \param attr the identifier of the attribute being output + * + * \attention This function is intended for INTERNAL usage within the + * library only. + */ +void mbedtls_debug_printf_ecdh( const mbedtls_ssl_context *ssl, int level, + const char *file, int line, + const mbedtls_ecdh_context *ecdh, + mbedtls_debug_ecdh_attr attr ); +#endif + #ifdef __cplusplus } #endif diff --git a/library/debug.c b/library/debug.c index db3924ac54..824cd0236e 100644 --- a/library/debug.c +++ b/library/debug.c @@ -365,4 +365,54 @@ void mbedtls_debug_print_crt( const mbedtls_ssl_context *ssl, int level, } #endif /* MBEDTLS_X509_CRT_PARSE_C */ +#if defined(MBEDTLS_ECDH_C) +static void mbedtls_debug_printf_ecdh_internal( const mbedtls_ssl_context *ssl, + int level, const char *file, + int line, + const mbedtls_ecdh_context *ecdh, + mbedtls_debug_ecdh_attr attr ) +{ +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + const mbedtls_ecdh_context* ctx = ecdh; +#else + const mbedtls_ecdh_context_mbed* ctx = &ecdh->ctx.mbed_ecdh; +#endif + + switch( attr ) + { + case MBEDTLS_DEBUG_ECDH_Q: + mbedtls_debug_print_ecp( ssl, level, file, line, "ECDH: Q", + &ctx->Q ); + break; + case MBEDTLS_DEBUG_ECDH_QP: + mbedtls_debug_print_ecp( ssl, level, file, line, "ECDH: Qp", + &ctx->Qp ); + break; + case MBEDTLS_DEBUG_ECDH_Z: + mbedtls_debug_print_mpi( ssl, level, file, line, "ECDH: z", + &ctx->z ); + break; + default: + break; + } +} + +void mbedtls_debug_printf_ecdh( const mbedtls_ssl_context *ssl, int level, + const char *file, int line, + const mbedtls_ecdh_context *ecdh, + mbedtls_debug_ecdh_attr attr ) +{ +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + mbedtls_debug_printf_ecdh_internal( ssl, level, file, line, ecdh, attr ); +#else + switch( ecdh->var ) + { + default: + mbedtls_debug_printf_ecdh_internal( ssl, level, file, line, ecdh, + attr ); + } +#endif +} +#endif /* MBEDTLS_ECDH_C */ + #endif /* MBEDTLS_DEBUG_C */ From 3fbdadad7b9d3db16936bbb275f5915224a2baba Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 15 Aug 2018 10:26:53 +0100 Subject: [PATCH 70/79] SSL: Make use of the new ECDH interface The SSL module accesses ECDH context members directly. This can't work with the new context, where we can't make any assumption about the implementation of the context. This commit makes use of the new functions to avoid accessing ECDH members directly. The only members that are still accessed directly are the group ID and the point format and they are independent from the implementation. --- library/ssl_cli.c | 22 ++++++++++++++++------ library/ssl_srv.c | 16 ++++++++++------ library/ssl_tls.c | 3 ++- 3 files changed, 28 insertions(+), 13 deletions(-) diff --git a/library/ssl_cli.c b/library/ssl_cli.c index ff576f3a84..afced7a99c 100644 --- a/library/ssl_cli.c +++ b/library/ssl_cli.c @@ -2027,8 +2027,14 @@ static int ssl_parse_server_dh_params( mbedtls_ssl_context *ssl, unsigned char * static int ssl_check_server_ecdh_params( const mbedtls_ssl_context *ssl ) { const mbedtls_ecp_curve_info *curve_info; + mbedtls_ecp_group_id grp_id; +#if defined(MBEDTLS_ECDH_LEGACY_CONTEXT) + grp_id = ssl->handshake->ecdh_ctx.grp.id; +#else + grp_id = ssl->handshake->ecdh_ctx.grp_id; +#endif - curve_info = mbedtls_ecp_curve_info_from_grp_id( ssl->handshake->ecdh_ctx.grp.id ); + curve_info = mbedtls_ecp_curve_info_from_grp_id( grp_id ); if( curve_info == NULL ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); @@ -2038,14 +2044,15 @@ static int ssl_check_server_ecdh_params( const mbedtls_ssl_context *ssl ) MBEDTLS_SSL_DEBUG_MSG( 2, ( "ECDH curve: %s", curve_info->name ) ); #if defined(MBEDTLS_ECP_C) - if( mbedtls_ssl_check_curve( ssl, ssl->handshake->ecdh_ctx.grp.id ) != 0 ) + if( mbedtls_ssl_check_curve( ssl, grp_id ) != 0 ) #else if( ssl->handshake->ecdh_ctx.grp.nbits < 163 || ssl->handshake->ecdh_ctx.grp.nbits > 521 ) #endif return( -1 ); - MBEDTLS_SSL_DEBUG_ECP( 3, "ECDH: Qp", &ssl->handshake->ecdh_ctx.Qp ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_QP ); return( 0 ); } @@ -2967,7 +2974,8 @@ static int ssl_write_client_key_exchange( mbedtls_ssl_context *ssl ) return( ret ); } - MBEDTLS_SSL_DEBUG_ECP( 3, "ECDH: Q", &ssl->handshake->ecdh_ctx.Q ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_Q ); #if defined(MBEDTLS_SSL__ECP_RESTARTABLE) if( ssl->handshake->ecrs_enabled ) @@ -2994,7 +3002,8 @@ ecdh_calc_secret: return( ret ); } - MBEDTLS_SSL_DEBUG_MPI( 3, "ECDH: z", &ssl->handshake->ecdh_ctx.z ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_Z ); } else #endif /* MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED || @@ -3089,7 +3098,8 @@ ecdh_calc_secret: return( ret ); } - MBEDTLS_SSL_DEBUG_ECP( 3, "ECDH: Q", &ssl->handshake->ecdh_ctx.Q ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_Q ); } else #endif /* MBEDTLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED */ diff --git a/library/ssl_srv.c b/library/ssl_srv.c index 36ca0d69f9..bc77f80203 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -3048,8 +3048,8 @@ curve_matching_done: MBEDTLS_SSL_DEBUG_MSG( 2, ( "ECDHE curve: %s", (*curve)->name ) ); - if( ( ret = mbedtls_ecp_group_load( &ssl->handshake->ecdh_ctx.grp, - (*curve)->grp_id ) ) != 0 ) + if( ( ret = mbedtls_ecdh_setup( &ssl->handshake->ecdh_ctx, + (*curve)->grp_id ) ) != 0 ) { MBEDTLS_SSL_DEBUG_RET( 1, "mbedtls_ecp_group_load", ret ); return( ret ); @@ -3071,7 +3071,8 @@ curve_matching_done: ssl->out_msglen += len; - MBEDTLS_SSL_DEBUG_ECP( 3, "ECDH: Q ", &ssl->handshake->ecdh_ctx.Q ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_Q ); } #endif /* MBEDTLS_KEY_EXCHANGE__SOME__ECDHE_ENABLED */ @@ -3794,7 +3795,8 @@ static int ssl_parse_client_key_exchange( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_KEY_EXCHANGE_RP ); } - MBEDTLS_SSL_DEBUG_ECP( 3, "ECDH: Qp ", &ssl->handshake->ecdh_ctx.Qp ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_QP ); if( ( ret = mbedtls_ecdh_calc_secret( &ssl->handshake->ecdh_ctx, &ssl->handshake->pmslen, @@ -3806,7 +3808,8 @@ static int ssl_parse_client_key_exchange( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_KEY_EXCHANGE_CS ); } - MBEDTLS_SSL_DEBUG_MPI( 3, "ECDH: z ", &ssl->handshake->ecdh_ctx.z ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_Z ); } else #endif /* MBEDTLS_KEY_EXCHANGE_ECDHE_RSA_ENABLED || @@ -3919,7 +3922,8 @@ static int ssl_parse_client_key_exchange( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_BAD_HS_CLIENT_KEY_EXCHANGE_RP ); } - MBEDTLS_SSL_DEBUG_ECP( 3, "ECDH: Qp ", &ssl->handshake->ecdh_ctx.Qp ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_QP ); if( ( ret = mbedtls_ssl_psk_derive_premaster( ssl, ciphersuite_info->key_exchange ) ) != 0 ) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 82e65251f0..c64036c0c4 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1333,7 +1333,8 @@ int mbedtls_ssl_psk_derive_premaster( mbedtls_ssl_context *ssl, mbedtls_key_exch *(p++) = (unsigned char)( zlen ); p += zlen; - MBEDTLS_SSL_DEBUG_MPI( 3, "ECDH: z", &ssl->handshake->ecdh_ctx.z ); + MBEDTLS_SSL_DEBUG_ECDH( 3, &ssl->handshake->ecdh_ctx, + MBEDTLS_DEBUG_ECDH_Z ); } else #endif /* MBEDTLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED */ From 36c5f7fe9be9b0f7250cda63fb6a975c4d5a96f1 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Tue, 30 Oct 2018 14:08:52 +0000 Subject: [PATCH 71/79] ECDH: Hide context from tests The tests for the ECDH key exchange that use the context accessed it directly. This can't work with the new context, where we can't make any assumptions about the implementation of the context. This commit works around this problem and comes with the cost of allocating an extra structures on the stack when executing the test. One of the tests is testing an older interface for the sake of backward compatibility. The new ECDH context is not backward compatible and this test doesn't make any sense for it, therefore we skip this test in non-legacy mode. --- tests/suites/test_suite_ecdh.function | 29 ++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/tests/suites/test_suite_ecdh.function b/tests/suites/test_suite_ecdh.function index 52fecf350a..7144763a25 100644 --- a/tests/suites/test_suite_ecdh.function +++ b/tests/suites/test_suite_ecdh.function @@ -129,6 +129,8 @@ void ecdh_exchange( int id ) const unsigned char *vbuf; size_t len; rnd_pseudo_info rnd_info; + unsigned char res_buf[1000]; + size_t res_len; mbedtls_ecdh_init( &srv ); mbedtls_ecdh_init( &cli ); @@ -147,9 +149,11 @@ void ecdh_exchange( int id ) TEST_ASSERT( mbedtls_ecdh_read_public( &srv, buf, len ) == 0 ); TEST_ASSERT( mbedtls_ecdh_calc_secret( &srv, &len, buf, 1000, - &rnd_pseudo_rand, &rnd_info ) == 0 ); - TEST_ASSERT( mbedtls_ecdh_calc_secret( &cli, &len, buf, 1000, NULL, NULL ) == 0 ); - TEST_ASSERT( mbedtls_mpi_cmp_mpi( &srv.z, &cli.z ) == 0 ); + &rnd_pseudo_rand, &rnd_info ) == 0 ); + TEST_ASSERT( mbedtls_ecdh_calc_secret( &cli, &res_len, res_buf, 1000, + NULL, NULL ) == 0 ); + TEST_ASSERT( len == res_len ); + TEST_ASSERT( memcmp( buf, res_buf, len ) == 0 ); exit: mbedtls_ecdh_free( &srv ); @@ -172,7 +176,9 @@ void ecdh_restart( int id, char *dA_str, char *dB_str, char *z_str, unsigned char rnd_buf_B[MBEDTLS_ECP_MAX_BYTES]; rnd_buf_info rnd_info_A, rnd_info_B; int cnt_restart; + mbedtls_ecp_group grp; + mbedtls_ecp_group_init( &grp ); mbedtls_ecdh_init( &srv ); mbedtls_ecdh_init( &cli ); @@ -184,16 +190,20 @@ void ecdh_restart( int id, char *dA_str, char *dB_str, char *z_str, rnd_info_B.buf = rnd_buf_B; rnd_info_B.length = unhexify( rnd_buf_B, dB_str ); - TEST_ASSERT( mbedtls_ecp_group_load( &srv.grp, id ) == 0 ); + /* The ECDH context is not guaranteed ot have an mbedtls_ecp_group structure + * in every configuration, therefore we load it separately. */ + TEST_ASSERT( mbedtls_ecp_group_load( &grp, id ) == 0 ); - /* otherwise we would have to fix the random buffer, - * as in ecdh_primitive_test_vec */ - TEST_ASSERT( srv.grp.nbits % 8 == 0 ); + /* Otherwise we would have to fix the random buffer, + * as in ecdh_primitive_testvec. */ + TEST_ASSERT( grp.nbits % 8 == 0 ); + + TEST_ASSERT( mbedtls_ecdh_setup( &srv, id ) == 0 ); /* set up restart parameters */ mbedtls_ecp_set_max_ops( max_ops ); - if( enable) + if( enable ) { mbedtls_ecdh_enable_restart( &srv ); mbedtls_ecdh_enable_restart( &cli ); @@ -269,12 +279,13 @@ void ecdh_restart( int id, char *dA_str, char *dB_str, char *z_str, TEST_ASSERT( memcmp( buf, z, len ) == 0 ); exit: + mbedtls_ecp_group_free( &grp ); mbedtls_ecdh_free( &srv ); mbedtls_ecdh_free( &cli ); } /* END_CASE */ -/* BEGIN_CASE */ +/* BEGIN_CASE depends_on:MBEDTLS_ECDH_LEGACY_CONTEXT */ void ecdh_exchange_legacy( int id ) { mbedtls_ecdh_context srv, cli; From b8f27060e0e05da8fe318acb041ec17a12c0e698 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Thu, 16 Aug 2018 16:32:43 +0100 Subject: [PATCH 72/79] Add Changelog entry for the new ECDH context --- ChangeLog | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ChangeLog b/ChangeLog index 8f0e8c1c79..f3dca40729 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,10 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS 2.x.x branch released xxxx-xx-xx + +Changes + * Extend ECDH interface to enable alternative implementations. + = mbed TLS 2.14.0 branch released 2018-11-19 Security From c3b680b028495615733c5272ae3a74c6498f1484 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 5 Dec 2018 16:01:13 +0000 Subject: [PATCH 73/79] Clarify requirements on handling ECP group IDs --- include/mbedtls/ecp.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/mbedtls/ecp.h b/include/mbedtls/ecp.h index 0e5b47fc21..1c372980e5 100644 --- a/include/mbedtls/ecp.h +++ b/include/mbedtls/ecp.h @@ -159,6 +159,10 @@ mbedtls_ecp_point; * additions or subtractions. Therefore, it is only an approximative modular * reduction. It must return 0 on success and non-zero on failure. * + * \note Alternative implementations must keep the group IDs distinct. If + * two group structures have the same ID, then they must be + * identical. + * */ typedef struct mbedtls_ecp_group { From af6f2694a47f9e4505bf2b419588a6497dcfa6d9 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 7 Dec 2018 09:55:24 +0000 Subject: [PATCH 74/79] Fix ECC hardware double initialization We initialized the ECC hardware before calling mbedtls_ecp_mul_shortcuts(). This in turn calls mbedtls_ecp_mul_restartable(), which initializes and frees the hardware too. This issue has been introduced by recent changes and caused some accelerators to hang. We move the initialization after the mbedtle_ecp_mul_shortcuts() calls to avoid double initialization. --- library/ecp.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/library/ecp.c b/library/ecp.c index de5725c700..e3b9106dbc 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -2393,11 +2393,6 @@ int mbedtls_ecp_muladd_restartable( mbedtls_ecp_point_init( &mP ); -#if defined(MBEDTLS_ECP_INTERNAL_ALT) - if( ( is_grp_capable = mbedtls_internal_ecp_grp_capable( grp ) ) ) - MBEDTLS_MPI_CHK( mbedtls_internal_ecp_init( grp ) ); -#endif /* MBEDTLS_ECP_INTERNAL_ALT */ - ECP_RS_ENTER( ma ); #if defined(MBEDTLS_ECP_RESTARTABLE) @@ -2425,6 +2420,12 @@ int mbedtls_ecp_muladd_restartable( mul2: #endif MBEDTLS_MPI_CHK( mbedtls_ecp_mul_shortcuts( grp, pR, n, Q, rs_ctx ) ); + +#if defined(MBEDTLS_ECP_INTERNAL_ALT) + if( ( is_grp_capable = mbedtls_internal_ecp_grp_capable( grp ) ) ) + MBEDTLS_MPI_CHK( mbedtls_internal_ecp_init( grp ) ); +#endif /* MBEDTLS_ECP_INTERNAL_ALT */ + #if defined(MBEDTLS_ECP_RESTARTABLE) if( rs_ctx != NULL && rs_ctx->ma != NULL ) rs_ctx->ma->state = ecp_rsma_add; From 855def157f7092ecad6b7d4f510401b3ca9f8530 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 7 Dec 2018 10:04:17 +0000 Subject: [PATCH 75/79] Add changelog entry for ECC hardware bugfix --- ChangeLog | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ChangeLog b/ChangeLog index 3f23854513..ef3c63bc2f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -35,6 +35,10 @@ New deprecations * Deprecate mbedtls_ctr_drbg_update and mbedtls_hmac_drbg_update in favor of functions that can return an error code. +Bugfix + * Fix double initialization of ECC hardware that made some accelerators + hang. + = mbed TLS 2.14.0 branch released 2018-11-19 Security From d2af46f1e68200c00009be4053f0d0e95a40937f Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 7 Dec 2018 10:39:00 +0000 Subject: [PATCH 76/79] Fix typo in ECP alternative documentation --- include/mbedtls/config.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/mbedtls/config.h b/include/mbedtls/config.h index 16ed503ca9..87a81c9eae 100644 --- a/include/mbedtls/config.h +++ b/include/mbedtls/config.h @@ -414,11 +414,11 @@ * unsigned char mbedtls_internal_ecp_grp_capable( * const mbedtls_ecp_group *grp ) * int mbedtls_internal_ecp_init( const mbedtls_ecp_group *grp ) - * void mbedtls_internal_ecp_deinit( const mbedtls_ecp_group *grp ) + * void mbedtls_internal_ecp_free( const mbedtls_ecp_group *grp ) * The mbedtls_internal_ecp_grp_capable function should return 1 if the * replacement functions implement arithmetic for the given group and 0 * otherwise. - * The functions mbedtls_internal_ecp_init and mbedtls_internal_ecp_deinit are + * The functions mbedtls_internal_ecp_init and mbedtls_internal_ecp_free are * called before and after each point operation and provide an opportunity to * implement optimized set up and tear down instructions. * From 60ca6e58b6e691f948772284d2633636c556cf81 Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Fri, 7 Dec 2018 13:06:24 +0000 Subject: [PATCH 77/79] test: Make basic-build-test.sh see summary statuses We've changed the behavior of "-v" to no longer output test summary statuses. Update basic-build-test.sh to use the test runner's verbosity option "-v 2", so that the basic-build-test.sh script can get the summary statuses it needs. --- tests/scripts/basic-build-test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/scripts/basic-build-test.sh b/tests/scripts/basic-build-test.sh index b4058718a2..fbe757d9ef 100755 --- a/tests/scripts/basic-build-test.sh +++ b/tests/scripts/basic-build-test.sh @@ -76,7 +76,7 @@ TEST_OUTPUT=out_${PPID} cd tests # Step 2a - Unit Tests -perl scripts/run-test-suites.pl -v |tee unit-test-$TEST_OUTPUT +perl scripts/run-test-suites.pl -v 2 |tee unit-test-$TEST_OUTPUT echo # Step 2b - System Tests From 683c58253073809aad026f4c0ab1bc5d8453efe5 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 7 Dec 2018 12:09:25 +0000 Subject: [PATCH 78/79] Clarify alternative ECP calling conventions Function calls to alternative implementations have to follow certain rules in order to preserve correct functionality. To avoid accidentally breaking these rules we state them explicitly in the ECP module for ourselves and every contributor to see. --- library/ecp.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/library/ecp.c b/library/ecp.c index e3b9106dbc..d9a3ac2f7e 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -47,6 +47,35 @@ #include MBEDTLS_CONFIG_FILE #endif +/** + * \brief Function level alternative implementation. + * + * The MBEDTLS_ECP_INTERNAL_ALT macro enables alternative implementations to + * replace certain functions in this module. The alternative implementations are + * typically hardware accelerators and need to activate the hardware before the + * computation starts and deactivate it after it finishes. The + * mbedtls_internal_ecp_init() and mbedtls_internal_ecp_free() functions serve + * this purpose. + * + * To preserve the correct functionality the following conditions must hold: + * + * - The alternative implementation must be activated by + * mbedtls_internal_ecp_init() before any of the replaceable functions is + * called. + * - mbedtls_internal_ecp_free() must \b only be called when the alternative + * implementation is activated. + * - mbedtls_internal_ecp_init() must \b not be called when the alternative + * implementation is activated. + * - Public functions must not return while the alternative implementation is + * activated. + * - Replaceable functions are guarded by \c MBEDTLS_ECP_XXX_ALT macros and + * before calling them an \code if( mbedtls_internal_ecp_grp_capable( grp ) ) + * \endcode ensures that the alternative implementation supports the current + * group. + */ +#if defined(MBEDTLS_ECP_INTERNAL_ALT) +#endif + #if defined(MBEDTLS_ECP_C) #include "mbedtls/ecp.h" From 172ba634638770f7ee764fe3e05189093c08d927 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Fri, 7 Dec 2018 12:13:54 +0000 Subject: [PATCH 79/79] Add guard for MBEDTLS_ECP_INTERNAL_ALT MBEDTLS_ECP_RESTARTABLE and MBEDTLS_ECP_INTERNAL_ALT are mutually exclusive, can't work and shouldn't be compiled together. --- include/mbedtls/check_config.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/mbedtls/check_config.h b/include/mbedtls/check_config.h index 425e3ea589..d051cbb71d 100644 --- a/include/mbedtls/check_config.h +++ b/include/mbedtls/check_config.h @@ -114,6 +114,7 @@ defined(MBEDTLS_ECDSA_SIGN_ALT) || \ defined(MBEDTLS_ECDSA_VERIFY_ALT) || \ defined(MBEDTLS_ECDSA_GENKEY_ALT) || \ + defined(MBEDTLS_ECP_INTERNAL_ALT) || \ defined(MBEDTLS_ECP_ALT) ) #error "MBEDTLS_ECP_RESTARTABLE defined, but it cannot coexist with an alternative ECP implementation" #endif