From e0b455fb51e5501e855381b06ebf6b83aade5029 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 31 May 2021 15:23:00 +0200 Subject: [PATCH 1/4] 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 2/4] 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 3/4] 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 4/4] 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-- )