From 60dbc9383109f2609776acbdacc7b19dba036dcc Mon Sep 17 00:00:00 2001 From: Andres AG Date: Fri, 2 Sep 2016 15:23:48 +0100 Subject: [PATCH 1/8] Add missing bounds check in X509 DER write funcs This patch adds checks in both mbedtls_x509write_crt_der and mbedtls_x509write_csr_der before the signature is written to buf using memcpy(). --- ChangeLog | 6 ++++++ library/x509write_crt.c | 3 +++ library/x509write_csr.c | 3 +++ 3 files changed, 12 insertions(+) diff --git a/ChangeLog b/ChangeLog index f8890dc70..afef2ddbd 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,12 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS 2.3.x branch released 2016-xx-xx +Security + * Fix potential stack corruption in mbedtls_x509write_crt_der() and + mbedtls_x509write_csr_der() when the signature is copied to the buffer + without checking whether there is enough space in the destination. It is + not triggerable remotely in SSL/TLS. + Features * Added support for CMAC for AES and 3DES and AES-CMAC-PRF-128, as defined by NIST SP 800-38B, RFC-4493 and RFC-4615. diff --git a/library/x509write_crt.c b/library/x509write_crt.c index 9041d440f..d1d9a22a7 100644 --- a/library/x509write_crt.c +++ b/library/x509write_crt.c @@ -413,6 +413,9 @@ int mbedtls_x509write_crt_der( mbedtls_x509write_cert *ctx, unsigned char *buf, MBEDTLS_ASN1_CHK_ADD( sig_and_oid_len, mbedtls_x509_write_sig( &c2, buf, sig_oid, sig_oid_len, sig, sig_len ) ); + if( len > (size_t)( c2 - buf ) ) + return( MBEDTLS_ERR_ASN1_BUF_TOO_SMALL ); + c2 -= len; memcpy( c2, c, len ); diff --git a/library/x509write_csr.c b/library/x509write_csr.c index 0b9a2851e..8fd856b2a 100644 --- a/library/x509write_csr.c +++ b/library/x509write_csr.c @@ -213,6 +213,9 @@ int mbedtls_x509write_csr_der( mbedtls_x509write_csr *ctx, unsigned char *buf, s MBEDTLS_ASN1_CHK_ADD( sig_and_oid_len, mbedtls_x509_write_sig( &c2, buf, sig_oid, sig_oid_len, sig, sig_len ) ); + if( len > (size_t)( c2 - buf ) ) + return( MBEDTLS_ERR_ASN1_BUF_TOO_SMALL ); + c2 -= len; memcpy( c2, c, len ); From e0af995f1275da47a42b520df0d7d872f20cd26d Mon Sep 17 00:00:00 2001 From: Andres AG Date: Wed, 7 Sep 2016 11:09:44 +0100 Subject: [PATCH 2/8] Add test for bounds in X509 DER write funcs --- ChangeLog | 4 ++-- tests/suites/test_suite_x509write.function | 28 ++++++++++++++++++++-- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/ChangeLog b/ChangeLog index afef2ddbd..d4c82e029 100644 --- a/ChangeLog +++ b/ChangeLog @@ -5,8 +5,8 @@ mbed TLS ChangeLog (Sorted per branch, date) Security * Fix potential stack corruption in mbedtls_x509write_crt_der() and mbedtls_x509write_csr_der() when the signature is copied to the buffer - without checking whether there is enough space in the destination. It is - not triggerable remotely in SSL/TLS. + without checking whether there is enough space in the destination. The + issue cannot be triggered remotely. Features * Added support for CMAC for AES and 3DES and AES-CMAC-PRF-128, as defined by diff --git a/tests/suites/test_suite_x509write.function b/tests/suites/test_suite_x509write.function index c3773ba54..89be31f9a 100644 --- a/tests/suites/test_suite_x509write.function +++ b/tests/suites/test_suite_x509write.function @@ -16,10 +16,11 @@ void x509_csr_check( char *key_file, char *cert_req_check_file, { mbedtls_pk_context key; mbedtls_x509write_csr req; - unsigned char buf[4000]; + unsigned char buf[4096]; unsigned char check_buf[4000]; int ret; size_t olen = 0, pem_len = 0; + int der_len = -1; FILE *f; const char *subject_name = "C=NL,O=PolarSSL,CN=PolarSSL Server 1"; rnd_pseudo_info rnd_info; @@ -52,6 +53,17 @@ void x509_csr_check( char *key_file, char *cert_req_check_file, TEST_ASSERT( olen >= pem_len - 1 ); TEST_ASSERT( memcmp( buf, check_buf, pem_len - 1 ) == 0 ); + der_len = mbedtls_x509write_csr_der( &req, buf, sizeof( buf ), + rnd_pseudo_rand, &rnd_info ); + TEST_ASSERT( der_len >= 0 ); + + if( der_len == 0 ) + goto exit; + + ret = mbedtls_x509write_csr_der( &req, buf, (size_t)( der_len - 1 ), + rnd_pseudo_rand, &rnd_info ); + TEST_ASSERT( ret == MBEDTLS_ERR_ASN1_BUF_TOO_SMALL ); + exit: mbedtls_x509write_csr_free( &req ); mbedtls_pk_free( &key ); @@ -68,11 +80,12 @@ void x509_crt_check( char *subject_key_file, char *subject_pwd, { mbedtls_pk_context subject_key, issuer_key; mbedtls_x509write_cert crt; - unsigned char buf[4000]; + unsigned char buf[4096]; unsigned char check_buf[5000]; mbedtls_mpi serial; int ret; size_t olen = 0, pem_len = 0; + int der_len = -1; FILE *f; rnd_pseudo_info rnd_info; @@ -125,6 +138,17 @@ void x509_crt_check( char *subject_key_file, char *subject_pwd, TEST_ASSERT( olen >= pem_len - 1 ); TEST_ASSERT( memcmp( buf, check_buf, pem_len - 1 ) == 0 ); + der_len = mbedtls_x509write_crt_der( &crt, buf, sizeof( buf ), + rnd_pseudo_rand, &rnd_info ); + TEST_ASSERT( der_len >= 0 ); + + if( der_len == 0 ) + goto exit; + + ret = mbedtls_x509write_crt_der( &crt, buf, (size_t)( der_len - 1 ), + rnd_pseudo_rand, &rnd_info ); + TEST_ASSERT( ret == MBEDTLS_ERR_ASN1_BUF_TOO_SMALL ); + exit: mbedtls_x509write_crt_free( &crt ); mbedtls_pk_free( &issuer_key ); From 5a74d260065908a29dbf57a1a0791f24f530da4a Mon Sep 17 00:00:00 2001 From: Simon Butcher Date: Tue, 11 Oct 2016 14:06:37 +0100 Subject: [PATCH 3/8] Added credit to Changelog for X.509 DER bounds fix --- ChangeLog | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index d4c82e029..0412613b0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -6,7 +6,7 @@ Security * Fix potential stack corruption in mbedtls_x509write_crt_der() and mbedtls_x509write_csr_der() when the signature is copied to the buffer without checking whether there is enough space in the destination. The - issue cannot be triggered remotely. + issue cannot be triggered remotely. (found by Jethro Beekman) Features * Added support for CMAC for AES and 3DES and AES-CMAC-PRF-128, as defined by From c47857dbf49792944a513dc7391208a32d1fc0b2 Mon Sep 17 00:00:00 2001 From: Andres AG Date: Mon, 10 Oct 2016 15:46:20 +0100 Subject: [PATCH 4/8] Add seed cmdline arg to test scripts --- tests/scripts/all.sh | 19 +++++++++++++++++++ tests/ssl-opt.sh | 6 +++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index ee0df0cc4..6b3396059 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -32,6 +32,7 @@ CONFIG_BAK="$CONFIG_H.bak" MEMORY=0 FORCE=0 +RELEASE=0 # Default commands, can be overriden by the environment : ${OPENSSL:="openssl"} @@ -48,6 +49,8 @@ usage() printf " -h|--help\t\tPrint this help.\n" printf " -m|--memory\t\tAdditional optional memory tests.\n" printf " -f|--force\t\tForce the tests to overwrite any modified files.\n" + printf " -s|--seed\t\tInteger seed value to use for this test run.\n" + printf " -r|--release-test\t\tRun this script in release mode. This fixes the seed value to 1.\n" printf " --out-of-source-dir=\t\tDirectory used for CMake out-of-source build tests." printf " --openssl=\t\tPath to OpenSSL executable to use for most tests.\n" printf " --openssl-legacy=\t\tPath to OpenSSL executable to use for legacy tests e.g. SSLv3.\n" @@ -106,6 +109,13 @@ while [ $# -gt 0 ]; do --force|-f) FORCE=1 ;; + --seed|-s) + shift + SEED="$1" + ;; + --release-test|-r) + RELEASE=1 + ;; --out-of-source-dir) shift OUT_OF_SOURCE_DIR="$1" @@ -171,9 +181,15 @@ else fi fi +if [ $RELEASE -eq 1 ]; then + # Fix the seed value to 1 to ensure that the tests are deterministic. + SEED=1 +fi + msg "info: $0 configuration" echo "MEMORY: $MEMORY" echo "FORCE: $FORCE" +echo "SEED: ${SEED-"UNSET"}" echo "OPENSSL: $OPENSSL" echo "OPENSSL_LEGACY: $OPENSSL_LEGACY" echo "GNUTLS_CLI: $GNUTLS_CLI" @@ -187,6 +203,9 @@ export OPENSSL_CMD="$OPENSSL" export GNUTLS_CLI="$GNUTLS_CLI" export GNUTLS_SERV="$GNUTLS_SERV" +# Avoid passing --seed flag in every call to ssl-opt.sh +[ ! -z ${SEED+set} ] && export SEED + # Make sure the tools we need are available. check_tools "$OPENSSL" "$OPENSSL_LEGACY" "$GNUTLS_CLI" "$GNUTLS_SERV" \ "$GNUTLS_LEGACY_CLI" "$GNUTLS_LEGACY_SERV" "doxygen" "dot" \ diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index d9c45cd7a..429d9cd19 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -58,6 +58,7 @@ print_usage() { printf " -n|--number\tExecute only numbered test (comma-separated, e.g. '245,256')\n" printf " -s|--show-numbers\tShow test numbers in front of test names\n" printf " -p|--preserve-logs\tPreserve logs of successful tests as well\n" + printf " --seed\tInteger seed value to use for this test run\n" } get_options() { @@ -81,6 +82,9 @@ get_options() { -p|--preserve-logs) PRESERVE_LOGS=1 ;; + --seed) + shift; SEED="$1" + ;; -h|--help) print_usage exit 0 @@ -595,7 +599,7 @@ unset PORT_BASE # +SRV_PORT will be replaced by either $SRV_PORT or $PXY_PORT later P_SRV="$P_SRV server_addr=127.0.0.1 server_port=$SRV_PORT" P_CLI="$P_CLI server_addr=127.0.0.1 server_port=+SRV_PORT" -P_PXY="$P_PXY server_addr=127.0.0.1 server_port=$SRV_PORT listen_addr=127.0.0.1 listen_port=$PXY_PORT" +P_PXY="$P_PXY server_addr=127.0.0.1 server_port=$SRV_PORT listen_addr=127.0.0.1 listen_port=$PXY_PORT ${SEED:+"seed=$SEED"}" O_SRV="$O_SRV -accept $SRV_PORT -dhparam data_files/dhparams.pem" O_CLI="$O_CLI -connect localhost:+SRV_PORT" G_SRV="$G_SRV -p $SRV_PORT" From b48c8ac45d84bb9871677a1dd28f5160bfb5737f Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Mon, 26 Sep 2016 09:15:44 +0100 Subject: [PATCH 5/8] Add safety check to sample mutex implementation Due to inconsistent freeing strategy in pkparse.c the sample mutex implementation in threading.c could lead to undefined behaviour by destroying the same mutex several times. This fix prevents mutexes from being destroyed several times in the sample threading implementation. --- ChangeLog | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ChangeLog b/ChangeLog index 0412613b0..47dfae6b6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -42,6 +42,8 @@ Bugfix * Fixed the sample applications gen_key.c, cert_req.c and cert_write.c for builds where the configuration MBEDTLS_PEM_WRITE_C is not defined. Found by inestlerode. #559. + * Fixed default threading implementation to avoid accidental double + initialisations and double frees. Changes * Extended test coverage of special cases, and added new timing test suite. From c4424c0a6933e7eb05dcfabac7fa14ea1e4962c6 Mon Sep 17 00:00:00 2001 From: Andres AG Date: Tue, 11 Oct 2016 15:41:40 +0100 Subject: [PATCH 6/8] Fix memory leak in test_suite_cmac.function --- tests/suites/test_suite_cmac.function | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/suites/test_suite_cmac.function b/tests/suites/test_suite_cmac.function index 1f88ddcfe..0cb437b67 100644 --- a/tests/suites/test_suite_cmac.function +++ b/tests/suites/test_suite_cmac.function @@ -93,6 +93,9 @@ void mbedtls_cmac_null_args( ) NULL ) == MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA ); + +exit: + mbedtls_cipher_free( &ctx ); } /* END_CASE */ @@ -144,6 +147,8 @@ void mbedtls_cmac_multiple_blocks( int cipher_type, unhexify( block4, block4_string ); unhexify( expected_result, expected_result_string ); + mbedtls_cipher_init( &ctx ); + /* Validate the test inputs */ TEST_ASSERT( block1_len <= 100 ); TEST_ASSERT( block2_len <= 100 ); @@ -154,8 +159,6 @@ void mbedtls_cmac_multiple_blocks( int cipher_type, TEST_ASSERT( ( cipher_info = mbedtls_cipher_info_from_type( cipher_type ) ) != NULL ); - mbedtls_cipher_init( &ctx ); - TEST_ASSERT( mbedtls_cipher_setup( &ctx, cipher_info ) == 0 ); TEST_ASSERT( mbedtls_cipher_cmac_starts( &ctx, @@ -231,6 +234,8 @@ void mbedtls_cmac_multiple_operations_same_key( int cipher_type, unhexify( expected_result_a, expected_result_a_string ); unhexify( expected_result_b, expected_result_b_string ); + mbedtls_cipher_init( &ctx ); + /* Validate the test inputs */ TEST_ASSERT( block_a1_len <= 100 ); TEST_ASSERT( block_a2_len <= 100 ); @@ -244,8 +249,6 @@ void mbedtls_cmac_multiple_operations_same_key( int cipher_type, TEST_ASSERT( ( cipher_info = mbedtls_cipher_info_from_type( cipher_type ) ) != NULL ); - mbedtls_cipher_init( &ctx ); - TEST_ASSERT( mbedtls_cipher_setup( &ctx, cipher_info ) == 0 ); TEST_ASSERT( mbedtls_cipher_cmac_starts( &ctx, From 99d09d27472e629053641aba5aeefd7657f7f962 Mon Sep 17 00:00:00 2001 From: Janos Follath Date: Wed, 12 Oct 2016 10:00:42 +0100 Subject: [PATCH 7/8] Fix memory leaks in CMAC tests --- library/cmac.c | 21 ++++++++++++++------- tests/suites/test_suite_cmac.function | 2 +- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/library/cmac.c b/library/cmac.c index 62f2a6abb..ee2fe056c 100644 --- a/library/cmac.c +++ b/library/cmac.c @@ -737,19 +737,19 @@ static int cmac_test_subkeys( int verbose, return( MBEDTLS_ERR_CIPHER_FEATURE_UNAVAILABLE ); } - mbedtls_cipher_init( &ctx ); - for( i = 0; i < num_tests; i++ ) { if( verbose != 0 ) mbedtls_printf( " %s CMAC subkey #%u: ", testname, i + 1 ); + mbedtls_cipher_init( &ctx ); + if( ( ret = mbedtls_cipher_setup( &ctx, cipher_info ) ) != 0 ) { if( verbose != 0 ) mbedtls_printf( "test execution failed\n" ); - goto exit; + goto cleanup; } if( ( ret = mbedtls_cipher_setkey( &ctx, key, keybits, @@ -758,7 +758,7 @@ static int cmac_test_subkeys( int verbose, if( verbose != 0 ) mbedtls_printf( "test execution failed\n" ); - goto exit; + goto cleanup; } ret = cmac_generate_subkeys( &ctx, K1, K2 ); @@ -766,7 +766,8 @@ static int cmac_test_subkeys( int verbose, { if( verbose != 0 ) mbedtls_printf( "failed\n" ); - goto exit; + + goto cleanup; } if( ( ret = memcmp( K1, subkeys, block_size ) ) != 0 || @@ -774,16 +775,22 @@ static int cmac_test_subkeys( int verbose, { if( verbose != 0 ) mbedtls_printf( "failed\n" ); - goto exit; + + goto cleanup; } if( verbose != 0 ) mbedtls_printf( "passed\n" ); + + mbedtls_cipher_free( &ctx ); } -exit: + goto exit; + +cleanup: mbedtls_cipher_free( &ctx ); +exit: return( ret ); } diff --git a/tests/suites/test_suite_cmac.function b/tests/suites/test_suite_cmac.function index 0cb437b67..4b31ab2ff 100644 --- a/tests/suites/test_suite_cmac.function +++ b/tests/suites/test_suite_cmac.function @@ -93,7 +93,6 @@ void mbedtls_cmac_null_args( ) NULL ) == MBEDTLS_ERR_CIPHER_BAD_INPUT_DATA ); - exit: mbedtls_cipher_free( &ctx ); } @@ -114,6 +113,7 @@ void mbedtls_cmac_setkey( int cipher_type, int key_size, TEST_ASSERT( ( cipher_info = mbedtls_cipher_info_from_type( cipher_type ) ) != NULL ); + memset( buf, 0x2A, sizeof( buf ) ); TEST_ASSERT( ( result == mbedtls_cipher_cmac( cipher_info, key, key_size, buf, 16, tmp ) ) != 0 ); } From d5766f62e48da954b3fc878a3de66ee3b77e9dee Mon Sep 17 00:00:00 2001 From: Andres AG Date: Tue, 4 Oct 2016 12:06:50 +0100 Subject: [PATCH 8/8] Fix typo in docs for mbedtls_x509write_csr_der() --- include/mbedtls/x509_csr.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mbedtls/x509_csr.h b/include/mbedtls/x509_csr.h index 7a9c2e055..fe9843cb5 100644 --- a/include/mbedtls/x509_csr.h +++ b/include/mbedtls/x509_csr.h @@ -282,7 +282,7 @@ int mbedtls_x509write_csr_der( mbedtls_x509write_csr *ctx, unsigned char *buf, s * * \note f_rng may be NULL if RSA is used for signature and the * signature is made offline (otherwise f_rng is desirable - * for couermeasures against timing attacks). + * for countermeasures against timing attacks). * ECDSA signatures always require a non-NULL f_rng. */ int mbedtls_x509write_csr_pem( mbedtls_x509write_csr *ctx, unsigned char *buf, size_t size,