From 7f0d193c944767450cdd96e18b54fc7804fbe371 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 19 May 2021 10:44:43 +0200 Subject: [PATCH 1/6] Fix misuse of MD API in SSL constant-flow HMAC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sequence of calls starts-update-starts-update-finish is not a guaranteed valid way to abort an operation and start a new one. Our software implementation just happens to support it, but alt implementations may very well not support it. Signed-off-by: Manuel Pégourié-Gonnard --- ChangeLog.d/fix-ssl-cf-hmac-alt.txt | 5 +++++ library/ssl_tls.c | 3 +++ 2 files changed, 8 insertions(+) create mode 100644 ChangeLog.d/fix-ssl-cf-hmac-alt.txt diff --git a/ChangeLog.d/fix-ssl-cf-hmac-alt.txt b/ChangeLog.d/fix-ssl-cf-hmac-alt.txt new file mode 100644 index 0000000000..e77613f03d --- /dev/null +++ b/ChangeLog.d/fix-ssl-cf-hmac-alt.txt @@ -0,0 +1,5 @@ +Bugfix + * Fix a regression introduced in 2.16.8 which broke (D)TLS CBC ciphersuites + (when the encrypt-then-MAC extension is not in use) with some ALT + implementations of the underlying hash (SHA-1, SHA-256, SHA-384), causing + the affected side to wrongly reject valid messages. Fixes #4118. diff --git a/library/ssl_tls.c b/library/ssl_tls.c index c749a8611c..a6c6297481 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1895,6 +1895,9 @@ int mbedtls_ssl_cf_hmac( MD_CHK( mbedtls_md_update( ctx, data + offset, 1 ) ); } + /* The context needs to finish() before it starts() again */ + MD_CHK( mbedtls_md_finish( ctx, aux_out ) ); + /* Now compute HASH(okey + inner_hash) */ MD_CHK( mbedtls_md_starts( ctx ) ); MD_CHK( mbedtls_md_update( ctx, okey, block_size ) ); From e0b455fb51e5501e855381b06ebf6b83aade5029 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 31 May 2021 15:23:00 +0200 Subject: [PATCH 2/6] Remove spurious dependencies on PEM Signed-off-by: Gilles Peskine --- tests/suites/test_suite_pkparse.data | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/suites/test_suite_pkparse.data b/tests/suites/test_suite_pkparse.data index cd842cf8bd..d4a0e44219 100644 --- a/tests/suites/test_suite_pkparse.data +++ b/tests/suites/test_suite_pkparse.data @@ -989,7 +989,7 @@ depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_BP512R1_ENABLED pk_parse_public_keyfile_ec:"data_files/ec_bp512_pub.pem":0 Parse EC Key #1 (SEC1 DER) -depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP192R1_ENABLED +depends_on:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP192R1_ENABLED pk_parse_keyfile_ec:"data_files/ec_prv.sec1.der":"NULL":0 Parse EC Key #2 (SEC1 PEM) @@ -1005,15 +1005,15 @@ depends_on:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP192R1_ENABLED pk_parse_keyfile_ec:"data_files/ec_prv.pk8.der":"NULL":0 Parse EC Key #4a (PKCS8 DER, no public key) -depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED +depends_on:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED pk_parse_keyfile_ec:"data_files/ec_prv.pk8nopub.der":"NULL":0 Parse EC Key #4b (PKCS8 DER, no public key, with parameters) -depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED +depends_on:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED pk_parse_keyfile_ec:"data_files/ec_prv.pk8nopubparam.der":"NULL":0 Parse EC Key #4c (PKCS8 DER, with parameters) -depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED +depends_on:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED pk_parse_keyfile_ec:"data_files/ec_prv.pk8param.der":"NULL":0 Parse EC Key #5 (PKCS8 PEM) @@ -1069,7 +1069,7 @@ depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_BP512R1_ENABLED pk_parse_keyfile_ec:"data_files/ec_bp512_prv.pem":"NULL":0 Parse EC Key #15 (SEC1 DER, secp256k1, SpecifiedECDomain) -depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256K1_ENABLED:MBEDTLS_PK_PARSE_EC_EXTENDED +depends_on:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256K1_ENABLED:MBEDTLS_PK_PARSE_EC_EXTENDED pk_parse_keyfile_ec:"data_files/ec_prv.specdom.der":"NULL":0 Key ASN1 (No data) From 5f45bd2baba17c092f76cb9878e439ff93c7d8f6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 31 May 2021 15:40:31 +0200 Subject: [PATCH 3/6] New macros TEST_EQUAL, ASSERT_ALLOC, ASSERT_ALLOC_WEAK Backports some test helper macros added after 2.16. This will facilitate backporting new test code. Signed-off-by: Gilles Peskine --- tests/suites/helpers.function | 60 +++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/suites/helpers.function b/tests/suites/helpers.function index ceac6703ff..ae207fd042 100644 --- a/tests/suites/helpers.function +++ b/tests/suites/helpers.function @@ -160,6 +160,66 @@ typedef enum } \ } while( 0 ) +/** Evaluate two expressions and fail the test case if they have different + * values. + * + * \param expr1 An expression to evaluate. + * \param expr2 The expected value of \p expr1. This can be any + * expression, but it is typically a constant. + */ +#define TEST_EQUAL( expr1, expr2 ) \ + TEST_ASSERT( ( expr1 ) == ( expr2 ) ) + +/** Allocate memory dynamically and fail the test case if this fails. + * The allocated memory will be filled with zeros. + * + * You must set \p pointer to \c NULL before calling this macro and + * put `mbedtls_free( pointer )` in the test's cleanup code. + * + * If \p length is zero, the resulting \p pointer will be \c NULL. + * This is usually what we want in tests since API functions are + * supposed to accept null pointers when a buffer size is zero. + * + * This macro expands to an instruction, not an expression. + * It may jump to the \c exit label. + * + * \param pointer An lvalue where the address of the allocated buffer + * will be stored. + * This expression may be evaluated multiple times. + * \param length Number of elements to allocate. + * This expression may be evaluated multiple times. + * + */ +#define ASSERT_ALLOC( pointer, length ) \ + do \ + { \ + TEST_ASSERT( ( pointer ) == NULL ); \ + if( ( length ) != 0 ) \ + { \ + ( pointer ) = mbedtls_calloc( sizeof( *( pointer ) ), \ + ( length ) ); \ + TEST_ASSERT( ( pointer ) != NULL ); \ + } \ + } \ + while( 0 ) + +/** Allocate memory dynamically. If the allocation fails, skip the test case. + * + * This macro behaves like #ASSERT_ALLOC, except that if the allocation + * fails, it marks the test as skipped rather than failed. + */ +#define ASSERT_ALLOC_WEAK( pointer, length ) \ + do \ + { \ + TEST_ASSERT( ( pointer ) == NULL ); \ + if( ( length ) != 0 ) \ + { \ + ( pointer ) = mbedtls_calloc( sizeof( *( pointer ) ), \ + ( length ) ); \ + TEST_ASSUME( ( pointer ) != NULL ); \ + } \ + } \ + while( 0 ) /** Compare two buffers and fail the test case if they differ. * * This macro expands to an instruction, not an expression. From 8e7d7ee1ae13027df7353ebedf71b998617a3469 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 31 May 2021 16:56:08 +0200 Subject: [PATCH 4/6] Fix ecp_muladd test cases never getting executed These test cases had been backported from Mbed TLS 2.x with a dependency symbol that didn't exist in 2.16. Declare that symbol. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_ecp.function | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/suites/test_suite_ecp.function b/tests/suites/test_suite_ecp.function index 9c90e9c2a5..3e901f7ddb 100644 --- a/tests/suites/test_suite_ecp.function +++ b/tests/suites/test_suite_ecp.function @@ -1,6 +1,25 @@ /* BEGIN_HEADER */ #include "mbedtls/ecp.h" +/* Backported from Mbed TLS 2.x for test dependencies. */ +#if defined(MBEDTLS_ECP_DP_SECP192R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP224R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP384R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP521R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP256R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP384R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_BP512R1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP192K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP224K1_ENABLED) || \ + defined(MBEDTLS_ECP_DP_SECP256K1_ENABLED) +#define MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED +#endif +#if defined(MBEDTLS_ECP_DP_CURVE25519_ENABLED) || \ + defined(MBEDTLS_ECP_DP_CURVE448_ENABLED) +#define MBEDTLS_ECP_MONTGOMERY_ENABLED +#endif + #define ECP_PF_UNKNOWN -1 #define ECP_PT_RESET( x ) \ From 995d89c9f21a66602c9c3b3044ef03e779978d8b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 1 Jun 2021 11:22:56 +0200 Subject: [PATCH 5/6] Fix null pointer arithmetic in error case When mbedtls_nist_kw_wrap was called with output=NULL and out_size=0, it performed arithmetic on the null pointer before detecting that the output buffer is too small and returning an error code. This was unlikely to have consequences on real-world hardware today, but it is undefined behavior and UBSan with Clang 10 flagged it. So fix it (fix #4025). Fix a similar-looking pattern in unwrap, though I haven't verified that it's reachable there. Signed-off-by: Gilles Peskine --- library/nist_kw.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/library/nist_kw.c b/library/nist_kw.c index 8341ff1303..278b7e91ab 100644 --- a/library/nist_kw.c +++ b/library/nist_kw.c @@ -219,8 +219,6 @@ int mbedtls_nist_kw_wrap( mbedtls_nist_kw_context *ctx, uint64_t t = 0; unsigned char outbuff[KW_SEMIBLOCK_LENGTH * 2]; unsigned char inbuff[KW_SEMIBLOCK_LENGTH * 2]; - unsigned char *R2 = output + KW_SEMIBLOCK_LENGTH; - unsigned char *A = output; *out_len = 0; /* @@ -296,6 +294,9 @@ int mbedtls_nist_kw_wrap( mbedtls_nist_kw_context *ctx, } else { + unsigned char *R2 = output + KW_SEMIBLOCK_LENGTH; + unsigned char *A = output; + /* * Do the wrapping function W, as defined in RFC 3394 section 2.2.1 */ @@ -359,7 +360,7 @@ static int unwrap( mbedtls_nist_kw_context *ctx, uint64_t t = 0; unsigned char outbuff[KW_SEMIBLOCK_LENGTH * 2]; unsigned char inbuff[KW_SEMIBLOCK_LENGTH * 2]; - unsigned char *R = output + ( semiblocks - 2 ) * KW_SEMIBLOCK_LENGTH; + unsigned char *R = NULL; *out_len = 0; if( semiblocks < MIN_SEMIBLOCKS_COUNT ) @@ -369,6 +370,7 @@ static int unwrap( mbedtls_nist_kw_context *ctx, memcpy( A, input, KW_SEMIBLOCK_LENGTH ); memmove( output, input + KW_SEMIBLOCK_LENGTH, ( semiblocks - 1 ) * KW_SEMIBLOCK_LENGTH ); + R = output + ( semiblocks - 2 ) * KW_SEMIBLOCK_LENGTH; /* Calculate intermediate values */ for( t = s; t >= 1; t-- ) From 58d238a3a4fa0d3afa2aa7c17120eee19d171d3d Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Thu, 10 Jun 2021 15:51:28 +0100 Subject: [PATCH 6/6] Disable OS X builds on Travis Signed-off-by: Dave Rodgman --- .travis.yml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/.travis.yml b/.travis.yml index 8789c444b4..9ba98ab8b9 100644 --- a/.travis.yml +++ b/.travis.yml @@ -32,12 +32,6 @@ jobs: script: - tests/scripts/all.sh -k 'test_depends_*' 'build_key_exchanges' - - name: macOS - os: osx - compiler: clang - script: - - tests/scripts/all.sh -k test_default_out_of_box - - name: Windows os: windows script: