From ce8bdf82a1a991b9f776e61dd59396b19083850c Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 19 Jul 2019 12:42:21 +0100 Subject: [PATCH 1/4] ECP restart: Don't calculate address of sub ctx if ctx is NULL All modules using restartable ECC operations support passing `NULL` as the restart context as a means to not use the feature. The restart contexts for ECDSA and ECP are nested, and when calling restartable ECP operations from restartable ECDSA operations, the address of the ECP restart context to use is calculated by adding the to the address of the ECDSA restart context the offset the of the ECP restart context. If the ECP restart context happens to not reside at offset `0`, this leads to a non-`NULL` pointer being passed to restartable ECP operations from restartable ECDSA-operations; those ECP operations will hence assume that the pointer points to a valid ECP restart address and likely run into a segmentation fault when trying to dereference the non-NULL but close-to-NULL address. The problem doesn't arise currently because luckily the ECP restart context has offset 0 within the ECDSA restart context, but we should not rely on it. This commit fixes the passage from restartable ECDSA to restartable ECP operations by propagating NULL as the restart context pointer. Apart from being fragile, the previous version could also lead to NULL pointer dereference failures in ASanDbg builds which dereferenced the ECDSA restart context even though it's not needed to calculate the address of the offset'ed ECP restart context. --- library/ecdsa.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/ecdsa.c b/library/ecdsa.c index dc19384d6..58e1a5fce 100644 --- a/library/ecdsa.c +++ b/library/ecdsa.c @@ -172,11 +172,11 @@ static void ecdsa_restart_det_free( mbedtls_ecdsa_restart_det_ctx *ctx ) } #endif /* MBEDTLS_ECDSA_DETERMINISTIC */ -#define ECDSA_RS_ECP &rs_ctx->ecp +#define ECDSA_RS_ECP ( rs_ctx == NULL ? NULL : &rs_ctx->ecp ) /* Utility macro for checking and updating ops budget */ #define ECDSA_BUDGET( ops ) \ - MBEDTLS_MPI_CHK( mbedtls_ecp_check_budget( grp, &rs_ctx->ecp, ops ) ); + MBEDTLS_MPI_CHK( mbedtls_ecp_check_budget( grp, ECDSA_RS_ECP, ops ) ); /* Call this when entering a function that needs its own sub-context */ #define ECDSA_RS_ENTER( SUB ) do { \ From af5d8abf266663377980cc4b31d8fb8466909260 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 19 Jul 2019 14:23:46 +0100 Subject: [PATCH 2/4] Don't call memset on NULL pointer in NIST KW test suite Note: There's no need to call `memset()` after `calloc()` because `calloc()` includes zeroization. --- tests/suites/test_suite_nist_kw.function | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tests/suites/test_suite_nist_kw.function b/tests/suites/test_suite_nist_kw.function index f1acde91a..9c34ea619 100644 --- a/tests/suites/test_suite_nist_kw.function +++ b/tests/suites/test_suite_nist_kw.function @@ -170,10 +170,6 @@ void nist_kw_plaintext_lengths( int in_len, int out_len, int mode, int res ) TEST_ASSERT( ciphertext != NULL ); } - memset( plaintext, 0, in_len ); - memset( ciphertext, 0, output_len ); - - TEST_ASSERT( mbedtls_nist_kw_setkey( &ctx, MBEDTLS_CIPHER_ID_AES, key, 8 * sizeof( key ), 1 ) == 0 ); @@ -225,10 +221,6 @@ void nist_kw_ciphertext_lengths( int in_len, int out_len, int mode, int res ) TEST_ASSERT( ciphertext != NULL ); } - memset( plaintext, 0, output_len ); - memset( ciphertext, 0, in_len ); - - TEST_ASSERT( mbedtls_nist_kw_setkey( &ctx, MBEDTLS_CIPHER_ID_AES, key, 8 * sizeof( key ), 0 ) == 0 ); unwrap_ret = mbedtls_nist_kw_unwrap( &ctx, mode, ciphertext, in_len, From ffb45b9ea590569823369e30f67ccfedef95aa50 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 19 Jul 2019 15:07:03 +0100 Subject: [PATCH 3/4] Add test for hardcoded timer callbacks to all.sh --- tests/scripts/all.sh | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 8c53c6700..a77fe1310 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -762,9 +762,27 @@ component_test_full_cmake_clang () { component_test_hardcoded_ciphersuite_cmake_clang() { msg "build: cmake, full config + MBEDTLS_SSL_CONF_SINGLE_CIPHERSUITE, clang" # ~ 50s scripts/config.pl full - scripts/config.pl unset MBEDTLS_MEMORY_BACKTRACE # too slow for tests + scripts/config.pl unset MBEDTLS_MEMORY_BACKTRACE + scripts/config.pl unset MBEDTLS_MEMORY_BUFFER_ALLOC_C scripts/config.pl set MBEDTLS_SSL_CONF_SINGLE_CIPHERSUITE MBEDTLS_SUITE_TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8 - CC=clang cmake -D CMAKE_BUILD_TYPE:String=Check -D ENABLE_TESTING=On . + CC=clang cmake -D LINK_WITH_PTHREAD=1 -D CMAKE_BUILD_TYPE:String=ASanDbg -D ENABLE_TESTING=On . + make + + msg "test: main suites (full config + MBEDTLS_SSL_CONF_SINGLE_CIPHERSUITE)" # ~ 5s + make test + + msg "test: ssl-opt.sh default (full config + MBEDTLS_SSL_CONF_SINGLE_CIPHERSUITE)" # ~ 5s + if_build_succeeded tests/ssl-opt.sh -f '^Default$\|^Default, DTLS$' +} + +component_test_hardcoded_timer_callback_cmake_clang() { + msg "build: cmake, full config + MBEDTLS_SSL_CONF_SINGLE_CIPHERSUITE, clang" # ~ 50s + scripts/config.pl full + scripts/config.pl unset MBEDTLS_MEMORY_BACKTRACE + scripts/config.pl unset MBEDTLS_MEMORY_BUFFER_ALLOC_C + scripts/config.pl set MBEDTLS_SSL_CONF_GET_TIMER mbedtls_timing_get_delay + scripts/config.pl set MBEDTLS_SSL_CONF_SET_TIMER mbedtls_timing_set_delay + CC=clang cmake -D LINK_WITH_PTHREAD=1 -D CMAKE_BUILD_TYPE:String=ASanDbg -D ENABLE_TESTING=On . make msg "test: main suites (full config + MBEDTLS_SSL_CONF_SINGLE_CIPHERSUITE)" # ~ 5s From 95d1b93c696250a2a6866fa208526726e81f2147 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 19 Jul 2019 15:07:19 +0100 Subject: [PATCH 4/4] Don't reset timer during mbedtls_ssl_setup() At that point, the timer might not yet be configured. The timer is reset at the following occasions: - when it is initially configured through mbedtls_ssl_set_timer_cb() or mbedtls_ssl_set_timer_cb_cx() - when a session is reset in mbedtls_ssl_session_reset() - when a handshake finishes via mbedtls_ssl_handshake_wrap() --- library/ssl_tls.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 0b1ebddcf..d106ca6e7 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -7777,8 +7777,6 @@ static int ssl_handshake_init( mbedtls_ssl_context *ssl ) ssl->handshake->retransmit_state = MBEDTLS_SSL_RETRANS_PREPARING; else ssl->handshake->retransmit_state = MBEDTLS_SSL_RETRANS_WAITING; - - ssl_set_timer( ssl, 0 ); } #endif