From 3099b43c6b9e9e5116aa9e3f0240cc90d792d353 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 10 Oct 2017 20:10:46 +0200 Subject: [PATCH 01/12] Timing: fix mbedtls_set_alarm(0) on Unix/POSIX The POSIX/Unix implementation of mbedtls_set_alarm did not set the mbedtls_timing_alarmed flag when called with 0, which was inconsistent with what the documentation implied and with the Windows behavior. --- ChangeLog | 7 ++++--- library/timing.c | 6 ++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index 0064f1ec09..5c3eaa5465 100644 --- a/ChangeLog +++ b/ChangeLog @@ -12,6 +12,9 @@ Security was independently reported by Tim Nordell via e-mail and by Florin Petriuc and sjorsdewit on GitHub. Fix proposed by Florin Petriuc in #1022. Fixes #707. +Features + * Allow comments in test data files. + Bugfix * Fix ssl_parse_record_header() to silently discard invalid DTLS records as recommended in RFC 6347 Section 4.1.2.7. @@ -50,15 +53,13 @@ Bugfix * Fix word size check in in pk.c to not depend on MBEDTLS_HAVE_INT64. * Fix crash when calling mbedtls_ssl_cache_free() twice. Found by MilenkoMitrovic, #1104 + * Fix mbedtls_timing_alarm(0) on Unix. Changes * Extend cert_write example program by options to set the CRT version and the message digest. Further, allow enabling/disabling of authority identifier, subject identifier and basic constraints extensions. -Features - * Allow comments in test data files. - = mbed TLS 2.1.9 branch released 2017-08-10 Security diff --git a/library/timing.c b/library/timing.c index 5d8b25b997..4ec4bf45b7 100644 --- a/library/timing.c +++ b/library/timing.c @@ -310,6 +310,12 @@ void mbedtls_set_alarm( int seconds ) mbedtls_timing_alarmed = 0; signal( SIGALRM, sighandler ); alarm( seconds ); + if( seconds == 0 ) + { + /* alarm(0) cancelled any previous pending alarm, but the + handler won't fire, so raise the flag straight away. */ + mbedtls_timing_alarmed = 1; + } } #endif /* _WIN32 && !EFIX64 && !EFI32 */ From 02c3167b40be04aa7856491673bd61e882e2849e Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 10 Oct 2017 19:46:45 +0200 Subject: [PATCH 02/12] timing interface documentation: minor clarifications --- include/mbedtls/timing.h | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/include/mbedtls/timing.h b/include/mbedtls/timing.h index ae7a713e7a..579de33101 100644 --- a/include/mbedtls/timing.h +++ b/include/mbedtls/timing.h @@ -1,7 +1,7 @@ /** * \file timing.h * - * \brief Portable interface to the CPU cycle counter + * \brief Portable interface to timeouts and to the CPU cycle counter * * Copyright (C) 2006-2015, ARM Limited, All Rights Reserved * SPDX-License-Identifier: Apache-2.0 @@ -65,6 +65,9 @@ extern volatile int mbedtls_timing_alarmed; * \warning This is only a best effort! Do not rely on this! * In particular, it is known to be unreliable on virtual * machines. + * + * \note This value starts at an unspecified origin and + * may wrap around. */ unsigned long mbedtls_timing_hardclock( void ); @@ -73,6 +76,8 @@ unsigned long mbedtls_timing_hardclock( void ); * * \param val points to a timer structure * \param reset if set to 1, the timer is restarted + * + * \return Elapsed time in ms (before the reset, if there is a reset) */ unsigned long mbedtls_timing_get_timer( struct mbedtls_timing_hr_time *val, int reset ); @@ -80,6 +85,7 @@ unsigned long mbedtls_timing_get_timer( struct mbedtls_timing_hr_time *val, int * \brief Setup an alarm clock * * \param seconds delay before the "mbedtls_timing_alarmed" flag is set + * (must be >=0) * * \warning Only one alarm at a time is supported. In a threaded * context, this means one for the whole process, not one per @@ -91,11 +97,15 @@ void mbedtls_set_alarm( int seconds ); * \brief Set a pair of delays to watch * (See \c mbedtls_timing_get_delay().) * - * \param data Pointer to timing data + * \param data Pointer to timing data. * Must point to a valid \c mbedtls_timing_delay_context struct. * \param int_ms First (intermediate) delay in milliseconds. + * The effect if int_ms > fin_ms is unspecified. * \param fin_ms Second (final) delay in milliseconds. * Pass 0 to cancel the current delay. + * + * \note To set a single delay, either use \c mbedtls_timing_set_timer + * directly or use this function with int_ms == fin_ms. */ void mbedtls_timing_set_delay( void *data, uint32_t int_ms, uint32_t fin_ms ); @@ -106,7 +116,7 @@ void mbedtls_timing_set_delay( void *data, uint32_t int_ms, uint32_t fin_ms ); * \param data Pointer to timing data * Must point to a valid \c mbedtls_timing_delay_context struct. * - * \return -1 if cancelled (fin_ms = 0) + * \return -1 if cancelled (fin_ms = 0), * 0 if none of the delays are passed, * 1 if only the intermediate delay is passed, * 2 if the final delay is passed. From b29e70bb0184f0a93bfbecee526b18a2f94f525b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 16 Oct 2017 19:33:06 +0200 Subject: [PATCH 03/12] mbedtls_timing_get_timer: don't use uninitialized memory mbedtls_timing_get_timer with reset=1 is called both to initialize a timer object and to reset an already-initialized object. In an initial call, the content of the data structure is indeterminate, so the code should not read from it. This could crash if signed overflows trap, for example. As a consequence, on reset, we can't return the previously elapsed time as was previously done on Windows. Return 0 as was done on Unix. --- ChangeLog | 1 + include/mbedtls/timing.h | 13 ++++++++++-- library/timing.c | 45 ++++++++++++++++++++-------------------- 3 files changed, 35 insertions(+), 24 deletions(-) diff --git a/ChangeLog b/ChangeLog index 5c3eaa5465..76d4b54ad6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -54,6 +54,7 @@ Bugfix * Fix crash when calling mbedtls_ssl_cache_free() twice. Found by MilenkoMitrovic, #1104 * Fix mbedtls_timing_alarm(0) on Unix. + * Fix use of uninitialized memory in mbedtls_timing_get_timer when reset=1. Changes * Extend cert_write example program by options to set the CRT version diff --git a/include/mbedtls/timing.h b/include/mbedtls/timing.h index 579de33101..bfb8579a07 100644 --- a/include/mbedtls/timing.h +++ b/include/mbedtls/timing.h @@ -75,9 +75,18 @@ unsigned long mbedtls_timing_hardclock( void ); * \brief Return the elapsed time in milliseconds * * \param val points to a timer structure - * \param reset if set to 1, the timer is restarted + * \param reset If 0, query the elapsed time. Otherwise (re)start the timer. * - * \return Elapsed time in ms (before the reset, if there is a reset) + * \return Elapsed time since the previous reset in ms. When + * restarting, this is always 0. + * + * \note To initialize a timer, call this function with reset=1. + * + * Determining the elapsed time and resetting the timer is not + * atomic on all platforms, so after the sequence + * `{ get_timer(1); ...; time1 = get_timer(1); ...; time2 = + * get_timer(0) }` the value time1+time2 is only approximately + * the delay since the first reset. */ unsigned long mbedtls_timing_get_timer( struct mbedtls_timing_hr_time *val, int reset ); diff --git a/library/timing.c b/library/timing.c index 4ec4bf45b7..af0d6ccf35 100644 --- a/library/timing.c +++ b/library/timing.c @@ -239,21 +239,23 @@ volatile int mbedtls_timing_alarmed = 0; unsigned long mbedtls_timing_get_timer( struct mbedtls_timing_hr_time *val, int reset ) { - unsigned long delta; - LARGE_INTEGER offset, hfreq; struct _hr_time *t = (struct _hr_time *) val; - QueryPerformanceCounter( &offset ); - QueryPerformanceFrequency( &hfreq ); - - delta = (unsigned long)( ( 1000 * - ( offset.QuadPart - t->start.QuadPart ) ) / - hfreq.QuadPart ); - if( reset ) + { QueryPerformanceCounter( &t->start ); - - return( delta ); + return( 0 ); + } + else + { + unsigned long delta; + LARGE_INTEGER now, hfreq; + QueryPerformanceCounter( &now ); + QueryPerformanceFrequency( &hfreq ); + delta = (unsigned long)( ( now.QuadPart - t->start.QuadPart ) * 1000ul + / hfreq.QuadPart ); + return( delta ); + } } /* It's OK to use a global because alarm() is supposed to be global anyway */ @@ -280,23 +282,22 @@ void mbedtls_set_alarm( int seconds ) unsigned long mbedtls_timing_get_timer( struct mbedtls_timing_hr_time *val, int reset ) { - unsigned long delta; - struct timeval offset; struct _hr_time *t = (struct _hr_time *) val; - gettimeofday( &offset, NULL ); - if( reset ) { - t->start.tv_sec = offset.tv_sec; - t->start.tv_usec = offset.tv_usec; + gettimeofday( &t->start, NULL ); return( 0 ); } - - delta = ( offset.tv_sec - t->start.tv_sec ) * 1000 - + ( offset.tv_usec - t->start.tv_usec ) / 1000; - - return( delta ); + else + { + unsigned long delta; + struct timeval now; + gettimeofday( &now, NULL ); + delta = ( now.tv_sec - t->start.tv_sec ) * 1000ul + + ( now.tv_usec - t->start.tv_usec ) / 1000; + return( delta ); + } } static void sighandler( int signum ) From 105e6bcb7d1106507f1c63af72b1d6fbf19ad4ba Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 10 Oct 2017 20:09:26 +0200 Subject: [PATCH 04/12] Timing self test: print some diagnosis information Print some not-very-nice-looking but helpful diagnosis information if the timing selftest fails. Since the failures tend to be due to heavy system load that's hard to reproduce, this information is necessary to understand what's going on. --- library/timing.c | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/library/timing.c b/library/timing.c index af0d6ccf35..77e0147471 100644 --- a/library/timing.c +++ b/library/timing.c @@ -380,13 +380,21 @@ static void busy_msleep( unsigned long msec ) (void) j; } -#define FAIL do \ -{ \ - if( verbose != 0 ) \ - mbedtls_printf( "failed\n" ); \ - \ - return( 1 ); \ -} while( 0 ) +#define FAIL do \ + { \ + if( verbose != 0 ) \ + { \ + mbedtls_printf( "failed at line %d\n", __LINE__ ); \ + mbedtls_printf( " cycles=%lu ratio=%lu millisecs=%lu secs=%lu hardfail=%d a=%lu b=%lu\n", \ + cycles, ratio, millisecs, secs, hardfail, \ + (unsigned long) a, (unsigned long) b ); \ + mbedtls_printf( " elapsed(hires)=%lu elapsed(ctx)=%lu status(ctx)=%d\n", \ + mbedtls_timing_get_timer( &hires, 0 ), \ + mbedtls_timing_get_timer( &ctx.timer, 0 ), \ + mbedtls_timing_get_delay( &ctx ) ); \ + } \ + return( 1 ); \ + } while( 0 ) /* * Checkup routine @@ -396,17 +404,16 @@ static void busy_msleep( unsigned long msec ) */ int mbedtls_timing_self_test( int verbose ) { - unsigned long cycles, ratio; - unsigned long millisecs, secs; - int hardfail; + unsigned long cycles = 0, ratio = 0; + unsigned long millisecs = 0, secs = 0; + int hardfail = 0; struct mbedtls_timing_hr_time hires; - uint32_t a, b; + uint32_t a = 0, b = 0; mbedtls_timing_delay_context ctx; if( verbose != 0 ) mbedtls_printf( " TIMING tests note: will take some time!\n" ); - if( verbose != 0 ) mbedtls_printf( " TIMING test #1 (set_alarm / get_timer): " ); @@ -423,12 +430,7 @@ int mbedtls_timing_self_test( int verbose ) /* For some reason on Windows it looks like alarm has an extra delay * (maybe related to creating a new thread). Allow some room here. */ if( millisecs < 800 * secs || millisecs > 1200 * secs + 300 ) - { - if( verbose != 0 ) - mbedtls_printf( "failed\n" ); - - return( 1 ); - } + FAIL; } if( verbose != 0 ) @@ -477,7 +479,6 @@ int mbedtls_timing_self_test( int verbose ) * On a 4Ghz 32-bit machine the cycle counter wraps about once per second; * since the whole test is about 10ms, it shouldn't happen twice in a row. */ - hardfail = 0; hard_test: if( hardfail > 1 ) From 9d7dfb74d137c8f940a2e8c21ba66178462e9620 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 20 Dec 2017 20:23:46 +0100 Subject: [PATCH 05/12] selftest: refactor to separate the list of tests from the logic No behavior change. --- programs/test/selftest.c | 275 +++++++++++++++++++-------------------- 1 file changed, 133 insertions(+), 142 deletions(-) diff --git a/programs/test/selftest.c b/programs/test/selftest.c index 82e9581f0d..399f0d7216 100644 --- a/programs/test/selftest.c +++ b/programs/test/selftest.c @@ -96,9 +96,120 @@ static int run_test_snprintf( void ) test_snprintf( 5, "123", 3 ) != 0 ); } +#if defined(MBEDTLS_SELF_TEST) +#if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) +int mbedtls_memory_buffer_alloc_free_and_self_test( int verbose ) +{ + if( verbose != 0 ) + { +#if defined(MBEDTLS_MEMORY_DEBUG) + mbedtls_memory_buffer_alloc_status( ); +#endif + } + mbedtls_memory_buffer_alloc_free( ); + return( mbedtls_memory_buffer_alloc_self_test( verbose ) ); +} +#endif + +typedef struct +{ + const char *name; + int ( *function )( int ); +} selftest_t; + +const selftest_t selftests[] = +{ +#if defined(MBEDTLS_MD2_C) + {"md2", mbedtls_md2_self_test}, +#endif +#if defined(MBEDTLS_MD4_C) + {"md4", mbedtls_md4_self_test}, +#endif +#if defined(MBEDTLS_MD5_C) + {"md5", mbedtls_md5_self_test}, +#endif +#if defined(MBEDTLS_RIPEMD160_C) + {"ripemd160", mbedtls_ripemd160_self_test}, +#endif +#if defined(MBEDTLS_SHA1_C) + {"sha1", mbedtls_sha1_self_test}, +#endif +#if defined(MBEDTLS_SHA256_C) + {"sha256", mbedtls_sha256_self_test}, +#endif +#if defined(MBEDTLS_SHA512_C) + {"sha512", mbedtls_sha512_self_test}, +#endif +#if defined(MBEDTLS_ARC4_C) + {"arc4", mbedtls_arc4_self_test}, +#endif +#if defined(MBEDTLS_DES_C) + {"des", mbedtls_des_self_test}, +#endif +#if defined(MBEDTLS_AES_C) + {"aes", mbedtls_aes_self_test}, +#endif +#if defined(MBEDTLS_GCM_C) && defined(MBEDTLS_AES_C) + {"gcm", mbedtls_gcm_self_test}, +#endif +#if defined(MBEDTLS_CCM_C) && defined(MBEDTLS_AES_C) + {"ccm", mbedtls_ccm_self_test}, +#endif +#if defined(MBEDTLS_BASE64_C) + {"base64", mbedtls_base64_self_test}, +#endif +#if defined(MBEDTLS_BIGNUM_C) + {"mpi", mbedtls_mpi_self_test}, +#endif +#if defined(MBEDTLS_RSA_C) + {"rsa", mbedtls_rsa_self_test}, +#endif +#if defined(MBEDTLS_X509_USE_C) + {"x509", mbedtls_x509_self_test}, +#endif +#if defined(MBEDTLS_XTEA_C) + {"xtea", mbedtls_xtea_self_test}, +#endif +#if defined(MBEDTLS_CAMELLIA_C) + {"camellia", mbedtls_camellia_self_test}, +#endif +#if defined(MBEDTLS_CTR_DRBG_C) + {"ctr_drbg", mbedtls_ctr_drbg_self_test}, +#endif +#if defined(MBEDTLS_HMAC_DRBG_C) + {"hmac_drbg", mbedtls_hmac_drbg_self_test}, +#endif +#if defined(MBEDTLS_ECP_C) + {"ecp", mbedtls_ecp_self_test}, +#endif +#if defined(MBEDTLS_DHM_C) + {"dhm", mbedtls_dhm_self_test}, +#endif +#if defined(MBEDTLS_ENTROPY_C) + {"entropy", mbedtls_entropy_self_test}, +#endif +#if defined(MBEDTLS_PKCS5_C) + {"pkcs5", mbedtls_pkcs5_self_test}, +#endif +/* Slower test after the faster ones */ +#if defined(MBEDTLS_TIMING_C) + {"timing", mbedtls_timing_self_test}, +#endif +/* Heap test comes last */ +#if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) + {"memory_buffer_alloc", mbedtls_memory_buffer_alloc_free_and_self_test}, +#endif + {NULL, NULL} +}; +#endif /* MBEDTLS_SELF_TEST */ + int main( int argc, char *argv[] ) { - int ret = 0, v; +#if defined(MBEDTLS_SELF_TEST) + const selftest_t *test; +#endif /* MBEDTLS_SELF_TEST */ + int v; + int suites_tested = 0, suites_failed = 0; #if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) unsigned char buf[1000000]; #endif @@ -139,132 +250,14 @@ int main( int argc, char *argv[] ) mbedtls_memory_buffer_alloc_init( buf, sizeof(buf) ); #endif -#if defined(MBEDTLS_MD2_C) - if( ( ret = mbedtls_md2_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_MD4_C) - if( ( ret = mbedtls_md4_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_MD5_C) - if( ( ret = mbedtls_md5_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_RIPEMD160_C) - if( ( ret = mbedtls_ripemd160_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_SHA1_C) - if( ( ret = mbedtls_sha1_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_SHA256_C) - if( ( ret = mbedtls_sha256_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_SHA512_C) - if( ( ret = mbedtls_sha512_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_ARC4_C) - if( ( ret = mbedtls_arc4_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_DES_C) - if( ( ret = mbedtls_des_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_AES_C) - if( ( ret = mbedtls_aes_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_GCM_C) && defined(MBEDTLS_AES_C) - if( ( ret = mbedtls_gcm_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_CCM_C) && defined(MBEDTLS_AES_C) - if( ( ret = mbedtls_ccm_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_BASE64_C) - if( ( ret = mbedtls_base64_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_BIGNUM_C) - if( ( ret = mbedtls_mpi_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_RSA_C) - if( ( ret = mbedtls_rsa_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_X509_USE_C) - if( ( ret = mbedtls_x509_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_XTEA_C) - if( ( ret = mbedtls_xtea_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_CAMELLIA_C) - if( ( ret = mbedtls_camellia_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_CTR_DRBG_C) - if( ( ret = mbedtls_ctr_drbg_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_HMAC_DRBG_C) - if( ( ret = mbedtls_hmac_drbg_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_ECP_C) - if( ( ret = mbedtls_ecp_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_DHM_C) - if( ( ret = mbedtls_dhm_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_ENTROPY_C) - if( ( ret = mbedtls_entropy_self_test( v ) ) != 0 ) - return( ret ); -#endif - -#if defined(MBEDTLS_PKCS5_C) - if( ( ret = mbedtls_pkcs5_self_test( v ) ) != 0 ) - return( ret ); -#endif - -/* Slow tests last */ - -#if defined(MBEDTLS_TIMING_C) - if( ( ret = mbedtls_timing_self_test( v ) ) != 0 ) - return( ret ); -#endif + for( test = selftests; test->name != NULL; test++ ) + { + if( test->function( v ) != 0 ) + { + suites_failed++; + } + suites_tested++; + } #else mbedtls_printf( " MBEDTLS_SELF_TEST not defined.\n" ); @@ -272,26 +265,24 @@ int main( int argc, char *argv[] ) if( v != 0 ) { -#if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) && defined(MBEDTLS_MEMORY_DEBUG) - mbedtls_memory_buffer_alloc_status(); -#endif - } + mbedtls_printf( " Executed %d test suites\n\n", suites_tested ); -#if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) - mbedtls_memory_buffer_alloc_free(); - - if( ( ret = mbedtls_memory_buffer_alloc_self_test( v ) ) != 0 ) - return( ret ); -#endif - - if( v != 0 ) - { - mbedtls_printf( " [ All tests passed ]\n\n" ); + if( suites_failed > 0) + { + mbedtls_printf( " [ %d tests FAILED ]\n\n", suites_failed ); + } + else + { + mbedtls_printf( " [ All tests passed ]\n\n" ); + } #if defined(_WIN32) mbedtls_printf( " Press Enter to exit this program.\n" ); fflush( stdout ); getchar(); #endif } - return( ret ); + if( suites_failed > 0) + return( EXIT_FAILURE ); + + return( EXIT_SUCCESS ); } From 6d51b6331600566a7c31326afc790181cba262b7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 20 Dec 2017 20:25:03 +0100 Subject: [PATCH 06/12] selftest: fixed an erroneous return code --- programs/test/selftest.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/programs/test/selftest.c b/programs/test/selftest.c index 399f0d7216..6279f58a9e 100644 --- a/programs/test/selftest.c +++ b/programs/test/selftest.c @@ -224,7 +224,7 @@ int main( int argc, char *argv[] ) if( pointer != NULL ) { mbedtls_printf( "all-bits-zero is not a NULL pointer\n" ); - return( 1 ); + return( EXIT_FAILURE ); } /* @@ -233,7 +233,7 @@ int main( int argc, char *argv[] ) if( run_test_snprintf() != 0 ) { mbedtls_printf( "the snprintf implementation is broken\n" ); - return( 0 ); + return( EXIT_FAILURE ); } if( argc == 2 && strcmp( argv[1], "-quiet" ) == 0 ) From edede44d97b88a01003cbb6de7def73bdd402c55 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 15 Dec 2017 15:01:27 +0100 Subject: [PATCH 07/12] selftest: allow running a subset of the tests If given command line arguments, interpret them as test names and only run those tests. --- ChangeLog | 2 ++ programs/test/selftest.c | 46 ++++++++++++++++++++++++++++++++++------ 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/ChangeLog b/ChangeLog index 76d4b54ad6..9fd8cce093 100644 --- a/ChangeLog +++ b/ChangeLog @@ -14,6 +14,8 @@ Security Features * Allow comments in test data files. + * The selftest program can execute a subset of the tests based on command + line arguments. Bugfix * Fix ssl_parse_record_header() to silently discard invalid DTLS records diff --git a/programs/test/selftest.c b/programs/test/selftest.c index 6279f58a9e..7fd308d4fe 100644 --- a/programs/test/selftest.c +++ b/programs/test/selftest.c @@ -208,9 +208,10 @@ int main( int argc, char *argv[] ) #if defined(MBEDTLS_SELF_TEST) const selftest_t *test; #endif /* MBEDTLS_SELF_TEST */ + char **argp = argc >= 1 ? argv + 1 : argv; int v; int suites_tested = 0, suites_failed = 0; -#if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) +#if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) && defined(MBEDTLS_SELF_TEST) unsigned char buf[1000000]; #endif void *pointer; @@ -236,8 +237,13 @@ int main( int argc, char *argv[] ) return( EXIT_FAILURE ); } - if( argc == 2 && strcmp( argv[1], "-quiet" ) == 0 ) + if( argc >= 2 && ( strcmp( argv[1], "--quiet" ) == 0 || + strcmp( argv[1], "-quiet" ) == 0 || + strcmp( argv[1], "-q" ) == 0 ) ) + { v = 0; + ++argp; + } else { v = 1; @@ -250,13 +256,41 @@ int main( int argc, char *argv[] ) mbedtls_memory_buffer_alloc_init( buf, sizeof(buf) ); #endif - for( test = selftests; test->name != NULL; test++ ) + if( *argp != NULL ) { - if( test->function( v ) != 0 ) + /* Run the specified tests */ + for( ; *argp != NULL; argp++ ) { - suites_failed++; + for( test = selftests; test->name != NULL; test++ ) + { + if( !strcmp( *argp, test->name ) ) + { + if( test->function( v ) != 0 ) + { + suites_failed++; + } + suites_tested++; + break; + } + } + if( test->name == NULL ) + { + mbedtls_printf( " Test suite %s not available -> failed\n\n", *argp ); + suites_failed++; + } + } + } + else + { + /* Run all the tests */ + for( test = selftests; test->name != NULL; test++ ) + { + if( test->function( v ) != 0 ) + { + suites_failed++; + } + suites_tested++; } - suites_tested++; } #else From a66fb3f22115ae89cb98c05ee385afd314bdae75 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 20 Dec 2017 18:09:27 +0100 Subject: [PATCH 08/12] selftest: allow excluding a subset of the tests E.g. "selftest -x timing" runs all the self-tests except timing. --- programs/test/selftest.c | 50 +++++++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/programs/test/selftest.c b/programs/test/selftest.c index 7fd308d4fe..d21d47cef4 100644 --- a/programs/test/selftest.c +++ b/programs/test/selftest.c @@ -208,8 +208,9 @@ int main( int argc, char *argv[] ) #if defined(MBEDTLS_SELF_TEST) const selftest_t *test; #endif /* MBEDTLS_SELF_TEST */ - char **argp = argc >= 1 ? argv + 1 : argv; - int v; + char **argp; + int v = 1; /* v=1 for verbose mode */ + int exclude_mode = 0; int suites_tested = 0, suites_failed = 0; #if defined(MBEDTLS_MEMORY_BUFFER_ALLOC_C) && defined(MBEDTLS_SELF_TEST) unsigned char buf[1000000]; @@ -237,18 +238,25 @@ int main( int argc, char *argv[] ) return( EXIT_FAILURE ); } - if( argc >= 2 && ( strcmp( argv[1], "--quiet" ) == 0 || - strcmp( argv[1], "-quiet" ) == 0 || - strcmp( argv[1], "-q" ) == 0 ) ) + for( argp = argv + ( argc >= 1 ? 1 : argc ); *argp != NULL; ++argp ) { - v = 0; - ++argp; + if( strcmp( *argp, "--quiet" ) == 0 || + strcmp( *argp, "-quiet" ) == 0 || + strcmp( *argp, "-q" ) == 0 ) + { + v = 0; + } + else if( strcmp( *argp, "--exclude" ) == 0 || + strcmp( *argp, "-x" ) == 0 ) + { + exclude_mode = 1; + } + else + break; } - else - { - v = 1; + + if( v != 0 ) mbedtls_printf( "\n" ); - } #if defined(MBEDTLS_SELF_TEST) @@ -256,7 +264,7 @@ int main( int argc, char *argv[] ) mbedtls_memory_buffer_alloc_init( buf, sizeof(buf) ); #endif - if( *argp != NULL ) + if( *argp != NULL && exclude_mode == 0 ) { /* Run the specified tests */ for( ; *argp != NULL; argp++ ) @@ -282,9 +290,24 @@ int main( int argc, char *argv[] ) } else { - /* Run all the tests */ + /* Run all the tests except excluded ones */ for( test = selftests; test->name != NULL; test++ ) { + if( exclude_mode ) + { + char **excluded; + for( excluded = argp; *excluded != NULL; ++excluded ) + { + if( !strcmp( *excluded, test->name ) ) + break; + } + if( *excluded ) + { + if( v ) + mbedtls_printf( " Skip: %s\n", test->name ); + continue; + } + } if( test->function( v ) != 0 ) { suites_failed++; @@ -294,6 +317,7 @@ int main( int argc, char *argv[] ) } #else + (void) exclude_mode; mbedtls_printf( " MBEDTLS_SELF_TEST not defined.\n" ); #endif From e2bf3b802a667c9b88d02143a2e9da4c5182a4c7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 17 Oct 2017 19:39:04 +0200 Subject: [PATCH 09/12] Timing self test: increased tolerance mbedtls_timing_self_test fails annoyingly often when running on a busy machine such as can be expected of a continous integration system. Increase the tolerances in the delay test, to reduce the chance of failures that are only due to missing a deadline on a busy machine. --- library/timing.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/timing.c b/library/timing.c index 77e0147471..93e1826d9c 100644 --- a/library/timing.c +++ b/library/timing.c @@ -445,19 +445,19 @@ int mbedtls_timing_self_test( int verbose ) { mbedtls_timing_set_delay( &ctx, a, a + b ); - busy_msleep( a - a / 8 ); + busy_msleep( a - a / 4 ); if( mbedtls_timing_get_delay( &ctx ) != 0 ) FAIL; - busy_msleep( a / 4 ); + busy_msleep( a / 2 ); if( mbedtls_timing_get_delay( &ctx ) != 1 ) FAIL; - busy_msleep( b - a / 8 - b / 8 ); + busy_msleep( b - a / 4 - b / 4 ); if( mbedtls_timing_get_delay( &ctx ) != 1 ) FAIL; - busy_msleep( b / 4 ); + busy_msleep( b / 2 ); if( mbedtls_timing_get_delay( &ctx ) != 2 ) FAIL; } From d39496233b5430be16c853f9bf8d7447c2ec86a6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 27 Oct 2017 18:42:32 +0200 Subject: [PATCH 10/12] Timing self test: increased duration Increase the duration of the self test, otherwise it tends to fail on a busy machine even with the recently upped tolerance. But run the loop only once, it's enough for a simple smoke test. --- ChangeLog | 2 ++ library/timing.c | 30 ++++++++++++------------------ 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/ChangeLog b/ChangeLog index 9fd8cce093..c6031b4184 100644 --- a/ChangeLog +++ b/ChangeLog @@ -16,6 +16,8 @@ Features * Allow comments in test data files. * The selftest program can execute a subset of the tests based on command line arguments. + * Improve the timing self-tests to be more robust when run on a + heavily-loaded machine. Bugfix * Fix ssl_parse_record_header() to silently discard invalid DTLS records diff --git a/library/timing.c b/library/timing.c index 93e1826d9c..2c8d4bbbfa 100644 --- a/library/timing.c +++ b/library/timing.c @@ -439,28 +439,22 @@ int mbedtls_timing_self_test( int verbose ) if( verbose != 0 ) mbedtls_printf( " TIMING test #2 (set/get_delay ): " ); - for( a = 200; a <= 400; a += 200 ) { - for( b = 200; b <= 400; b += 200 ) - { - mbedtls_timing_set_delay( &ctx, a, a + b ); + a = 800; + b = 400; + mbedtls_timing_set_delay( &ctx, a, a + b ); /* T = 0 */ - busy_msleep( a - a / 4 ); - if( mbedtls_timing_get_delay( &ctx ) != 0 ) - FAIL; + busy_msleep( a - a / 4 ); /* T = a - a/4 */ + if( mbedtls_timing_get_delay( &ctx ) != 0 ) + FAIL; - busy_msleep( a / 2 ); - if( mbedtls_timing_get_delay( &ctx ) != 1 ) - FAIL; + busy_msleep( a / 4 + b / 4 ); /* T = a + b/4 */ + if( mbedtls_timing_get_delay( &ctx ) != 1 ) + FAIL; - busy_msleep( b - a / 4 - b / 4 ); - if( mbedtls_timing_get_delay( &ctx ) != 1 ) - FAIL; - - busy_msleep( b / 2 ); - if( mbedtls_timing_get_delay( &ctx ) != 2 ) - FAIL; - } + busy_msleep( b ); /* T = a + b + b/4 */ + if( mbedtls_timing_get_delay( &ctx ) != 2 ) + FAIL; } mbedtls_timing_set_delay( &ctx, 0, 0 ); From 36816929e51c7df02ed5238f9e7628834b2e3a6f Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 20 Dec 2017 22:32:47 +0100 Subject: [PATCH 11/12] Timing self test: shorten redundant tests We don't need to test multiple delays in a self-test. Save 5s of busy-wait. --- library/timing.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/timing.c b/library/timing.c index 2c8d4bbbfa..de936e101e 100644 --- a/library/timing.c +++ b/library/timing.c @@ -417,8 +417,8 @@ int mbedtls_timing_self_test( int verbose ) if( verbose != 0 ) mbedtls_printf( " TIMING test #1 (set_alarm / get_timer): " ); - for( secs = 1; secs <= 3; secs++ ) { + secs = 1; (void) mbedtls_timing_get_timer( &hires, 1 ); mbedtls_set_alarm( (int) secs ); From 83cd34a39e1df8c3c3ad73212342022a1d3582b4 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 21 Dec 2017 11:07:37 +0100 Subject: [PATCH 12/12] selftest: fix build error in some configurations Include stdlib.h for EXIT_FAILURE. --- programs/test/selftest.c | 1 + 1 file changed, 1 insertion(+) diff --git a/programs/test/selftest.c b/programs/test/selftest.c index d21d47cef4..846d34da7c 100644 --- a/programs/test/selftest.c +++ b/programs/test/selftest.c @@ -51,6 +51,7 @@ #include "mbedtls/ecp.h" #include "mbedtls/timing.h" +#include #include #include