From 982865618a396d2ad1c54e9d5a6f36ffbf7b2d7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Mon, 12 Jan 2015 19:17:05 +0100 Subject: [PATCH 1/9] Stop assuming chars are signed (They aren't on ARM by default.) --- programs/ssl/ssl_client2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index d4cde0f143..de33fbcd44 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -135,7 +135,7 @@ struct options int tickets; /* enable / disable session tickets */ const char *alpn_string; /* ALPN supported protocols */ int fallback; /* is this a fallback connection? */ - char extended_ms; /* negotiate extended master secret? */ + int extended_ms; /* negotiate extended master secret? */ } opt; static void my_debug( void *ctx, int level, const char *str ) From 352143fa1e3c0f431b8aa8d896e6b086230486f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 13 Jan 2015 10:59:51 +0100 Subject: [PATCH 2/9] Refactor for clearer correctness/security --- library/ssl_tls.c | 112 +++++++++++++++++++++++----------------------- 1 file changed, 56 insertions(+), 56 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index ea34f66b74..cc6a356a89 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1071,71 +1071,41 @@ static void ssl_mac( md_context_t *md_ctx, unsigned char *secret, } #endif /* POLARSSL_SSL_PROTO_SSL3 */ -#define MAC_NONE 0 -#define MAC_PLAINTEXT 1 -#define MAC_CIPHERTEXT 2 - #if defined(POLARSSL_ARC4_C) || defined(POLARSSL_CIPHER_NULL_CIPHER) || \ ( defined(POLARSSL_CIPHER_MODE_CBC) && \ ( defined(POLARSSL_AES_C) || defined(POLARSSL_CAMELLIA_C) ) ) #define POLARSSL_SOME_MODES_USE_MAC #endif -/* - * Is MAC applied on ciphertext, cleartext or not at all? - */ -#if defined(POLARSSL_SOME_MODES_USE_MAC) -static char ssl_get_mac_order( ssl_context *ssl, - const ssl_session *session, - cipher_mode_t mode ) -{ -#if defined(POLARSSL_ARC4_C) || defined(POLARSSL_CIPHER_NULL_CIPHER) - if( mode == POLARSSL_MODE_STREAM ) - return( MAC_PLAINTEXT ); -#endif - -#if defined(POLARSSL_CIPHER_MODE_CBC) && \ - ( defined(POLARSSL_AES_C) || defined(POLARSSL_CAMELLIA_C) ) - if( mode == POLARSSL_MODE_CBC ) - { -#if defined(POLARSSL_SSL_ENCRYPT_THEN_MAC) - if( session != NULL && session->encrypt_then_mac == SSL_ETM_ENABLED ) - { - SSL_DEBUG_MSG( 3, ( "using encrypt then mac" ) ); - return( MAC_CIPHERTEXT ); - } -#else - ((void) ssl); - ((void) session); -#endif - - return( MAC_PLAINTEXT ); - } -#else - ((void) ssl); - ((void) session); -#endif - - return( MAC_NONE ); -} -#endif /* POLARSSL_SOME_MODES_USE_MAC */ - /* * Encryption/decryption functions */ static int ssl_encrypt_buf( ssl_context *ssl ) { size_t i; - const cipher_mode_t mode = cipher_get_cipher_mode( - &ssl->transform_out->cipher_ctx_enc ); + cipher_mode_t mode; + int auth_done = 0; SSL_DEBUG_MSG( 2, ( "=> encrypt buf" ) ); + if( ssl->session_out == NULL || ssl->transform_out == NULL ) + { + SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( POLARSSL_ERR_SSL_INTERNAL_ERROR ); + } + + mode = cipher_get_cipher_mode( &ssl->transform_out->cipher_ctx_enc ); + /* * Add MAC before if needed */ #if defined(POLARSSL_SOME_MODES_USE_MAC) - if( ssl_get_mac_order( ssl, ssl->session_out, mode ) == MAC_PLAINTEXT ) + if( mode == POLARSSL_MODE_STREAM || + ( mode == POLARSSL_MODE_CBC +#if defined(POLARSSL_SSL_ENCRYPT_THEN_MAC) + && ssl->session_out->encrypt_then_mac == SSL_ETM_DISABLED +#endif + ) ) { #if defined(POLARSSL_SSL_PROTO_SSL3) if( ssl->minor_ver == SSL_MINOR_VERSION_0 ) @@ -1170,6 +1140,7 @@ static int ssl_encrypt_buf( ssl_context *ssl ) ssl->transform_out->maclen ); ssl->out_msglen += ssl->transform_out->maclen; + auth_done++; } #endif /* AEAD not the only option */ @@ -1281,6 +1252,7 @@ static int ssl_encrypt_buf( ssl_context *ssl ) } ssl->out_msglen += taglen; + auth_done++; SSL_DEBUG_BUF( 4, "after encrypt: tag", enc_msg + enc_msglen, taglen ); } @@ -1371,7 +1343,7 @@ static int ssl_encrypt_buf( ssl_context *ssl ) #endif #if defined(POLARSSL_SSL_ENCRYPT_THEN_MAC) - if( ssl_get_mac_order( ssl, ssl->session_out, mode ) == MAC_CIPHERTEXT ) + if( auth_done == 0 ) { /* * MAC(MAC_write_key, seq_num + @@ -1383,6 +1355,8 @@ static int ssl_encrypt_buf( ssl_context *ssl ) */ unsigned char pseudo_hdr[13]; + SSL_DEBUG_MSG( 3, ( "using encrypt then mac" ) ); + memcpy( pseudo_hdr + 0, ssl->out_ctr, 8 ); memcpy( pseudo_hdr + 8, ssl->out_hdr, 3 ); pseudo_hdr[11] = (unsigned char)( ( ssl->out_msglen >> 8 ) & 0xFF ); @@ -1398,6 +1372,7 @@ static int ssl_encrypt_buf( ssl_context *ssl ) md_hmac_reset( &ssl->transform_out->md_ctx_enc ); ssl->out_msglen += ssl->transform_out->maclen; + auth_done++; } #endif /* POLARSSL_SSL_ENCRYPT_THEN_MAC */ } @@ -1409,6 +1384,13 @@ static int ssl_encrypt_buf( ssl_context *ssl ) return( POLARSSL_ERR_SSL_INTERNAL_ERROR ); } + /* Make extra sure authentication was performed, exactly once */ + if( auth_done != 1 ) + { + SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( POLARSSL_ERR_SSL_INTERNAL_ERROR ); + } + for( i = 8; i > 0; i-- ) if( ++ssl->out_ctr[i - 1] != 0 ) break; @@ -1430,14 +1412,22 @@ static int ssl_encrypt_buf( ssl_context *ssl ) static int ssl_decrypt_buf( ssl_context *ssl ) { size_t i; - const cipher_mode_t mode = cipher_get_cipher_mode( - &ssl->transform_in->cipher_ctx_dec ); + cipher_mode_t mode; + int auth_done = 0; #if defined(POLARSSL_SOME_MODES_USE_MAC) size_t padlen = 0, correct = 1; #endif SSL_DEBUG_MSG( 2, ( "=> decrypt buf" ) ); + if( ssl->session_in == NULL || ssl->transform_in == NULL ) + { + SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( POLARSSL_ERR_SSL_INTERNAL_ERROR ); + } + + mode = cipher_get_cipher_mode( &ssl->transform_in->cipher_ctx_dec ); + if( ssl->in_msglen < ssl->transform_in->minlen ) { SSL_DEBUG_MSG( 1, ( "in_msglen (%d) < minlen (%d)", @@ -1534,6 +1524,7 @@ static int ssl_decrypt_buf( ssl_context *ssl ) return( ret ); } + auth_done++; if( olen != dec_msglen ) { @@ -1583,11 +1574,13 @@ static int ssl_decrypt_buf( ssl_context *ssl ) * Authenticate before decrypt if enabled */ #if defined(POLARSSL_SSL_ENCRYPT_THEN_MAC) - if( ssl_get_mac_order( ssl, ssl->session_in, mode ) == MAC_CIPHERTEXT ) + if( ssl->session_in->encrypt_then_mac == SSL_ETM_ENABLED ) { unsigned char computed_mac[POLARSSL_SSL_MAX_MAC_SIZE]; unsigned char pseudo_hdr[13]; + SSL_DEBUG_MSG( 3, ( "using encrypt then mac" ) ); + dec_msglen -= ssl->transform_in->maclen; ssl->in_msglen -= ssl->transform_in->maclen; @@ -1616,6 +1609,7 @@ static int ssl_decrypt_buf( ssl_context *ssl ) return( POLARSSL_ERR_SSL_INVALID_MAC ); } + auth_done++; } #endif /* POLARSSL_SSL_ENCRYPT_THEN_MAC */ @@ -1674,7 +1668,7 @@ static int ssl_decrypt_buf( ssl_context *ssl ) padlen = 1 + ssl->in_msg[ssl->in_msglen - 1]; if( ssl->in_msglen < ssl->transform_in->maclen + padlen && - ssl_get_mac_order( ssl, ssl->session_in, mode ) == MAC_PLAINTEXT ) + auth_done == 0 ) { #if defined(POLARSSL_SSL_DEBUG_ALL) SSL_DEBUG_MSG( 1, ( "msglen (%d) < maclen (%d) + padlen (%d)", @@ -1766,10 +1760,8 @@ static int ssl_decrypt_buf( ssl_context *ssl ) * Authenticate if not done yet. * Compute the MAC regardless of the padding result (RFC4346, CBCTIME). */ -#if defined(POLARSSL_ARC4_C) || defined(POLARSSL_CIPHER_NULL_CIPHER) || \ - ( defined(POLARSSL_CIPHER_MODE_CBC) && \ - ( defined(POLARSSL_AES_C) || defined(POLARSSL_CAMELLIA_C) ) ) - if( ssl_get_mac_order( ssl, ssl->session_in, mode ) == MAC_PLAINTEXT ) +#if defined(POLARSSL_SOME_MODES_USE_MAC) + if( auth_done == 0 ) { unsigned char tmp[POLARSSL_SSL_MAX_MAC_SIZE]; @@ -1843,6 +1835,7 @@ static int ssl_decrypt_buf( ssl_context *ssl ) #endif correct = 0; } + auth_done++; /* * Finally check the correct flag @@ -1850,7 +1843,14 @@ static int ssl_decrypt_buf( ssl_context *ssl ) if( correct == 0 ) return( POLARSSL_ERR_SSL_INVALID_MAC ); } -#endif /* AEAD not the only option */ +#endif /* POLARSSL_SOME_MODES_USE_MAC */ + + /* Make extra sure authentication was performed, exactly once */ + if( auth_done != 1 ) + { + SSL_DEBUG_MSG( 1, ( "should never happen" ) ); + return( POLARSSL_ERR_SSL_INTERNAL_ERROR ); + } if( ssl->in_msglen == 0 ) { From f5f25b3a0db2fd54f423166f673475b27769e0f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 27 Nov 2014 14:04:56 +0100 Subject: [PATCH 3/9] Add test for ctr_drbg_update() input sanitizing --- tests/suites/test_suite_ctr_drbg.function | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/suites/test_suite_ctr_drbg.function b/tests/suites/test_suite_ctr_drbg.function index fd9f087f29..a36bab245f 100644 --- a/tests/suites/test_suite_ctr_drbg.function +++ b/tests/suites/test_suite_ctr_drbg.function @@ -141,6 +141,10 @@ void ctr_drbg_entropy_usage( ) } TEST_ASSERT( last_idx == test_offset_idx ); + /* Call update with too much data (sizeof entropy > MAX(_SEED)_INPUT) + * (just make sure it doesn't cause memory corruption) */ + ctr_drbg_update( &ctx, entropy, sizeof( entropy ) ); + /* Now enable PR, so the next few calls should all reseed */ ctr_drbg_set_prediction_resistance( &ctx, CTR_DRBG_PR_ON ); TEST_ASSERT( ctr_drbg_random( &ctx, out, sizeof( out ) ) == 0 ); From 5cb4b310571d9ff273e95e99379c95ac387c0ce6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 25 Nov 2014 17:41:50 +0100 Subject: [PATCH 4/9] Fix missing bound check --- ChangeLog | 4 ++++ include/polarssl/ctr_drbg.h | 4 ++++ library/ctr_drbg.c | 8 ++++++++ 3 files changed, 16 insertions(+) diff --git a/ChangeLog b/ChangeLog index 89c87e0575..8370738b49 100644 --- a/ChangeLog +++ b/ChangeLog @@ -9,6 +9,10 @@ Features * Add support for Extended Master Secret (draft-ietf-tls-session-hash) * Add support for Encrypt-then-MAC (RFC 7366) +Bugfix + * Stack buffer overflow if ctr_drbg_update() is called with too large + add_len (found by Jean-Philippe Aumasson) (not triggerable remotely). + = PolarSSL 1.3.9 released 2014-10-20 Security * Lowest common hash was selected from signature_algorithms extension in diff --git a/include/polarssl/ctr_drbg.h b/include/polarssl/ctr_drbg.h index bebbfe9311..de696dc237 100644 --- a/include/polarssl/ctr_drbg.h +++ b/include/polarssl/ctr_drbg.h @@ -188,6 +188,10 @@ int ctr_drbg_reseed( ctr_drbg_context *ctx, * \param ctx CTR_DRBG context * \param additional Additional data to update state with * \param add_len Length of additional data + * + * \note If add_len is greater than CTR_DRBG_MAX_SEED_INPUT, + * only the first CTR_DRBG_MAX_SEED_INPUT bytes are used, + * the remaining ones are silently discarded. */ void ctr_drbg_update( ctr_drbg_context *ctx, const unsigned char *additional, size_t add_len ); diff --git a/library/ctr_drbg.c b/library/ctr_drbg.c index 96ee4f1625..91e061567b 100644 --- a/library/ctr_drbg.c +++ b/library/ctr_drbg.c @@ -137,6 +137,9 @@ static int block_cipher_df( unsigned char *output, int i, j; size_t buf_len, use_len; + if( data_len > CTR_DRBG_MAX_SEED_INPUT ) + return( POLARSSL_ERR_CTR_DRBG_INPUT_TOO_BIG ); + memset( buf, 0, CTR_DRBG_MAX_SEED_INPUT + CTR_DRBG_BLOCKSIZE + 16 ); aes_init( &aes_ctx ); @@ -256,6 +259,11 @@ void ctr_drbg_update( ctr_drbg_context *ctx, if( add_len > 0 ) { + /* MAX_INPUT would be more logical here, but we have to match + * block_cipher_df()'s limits since we can't propagate errors */ + if( add_len > CTR_DRBG_MAX_SEED_INPUT ) + add_len = CTR_DRBG_MAX_SEED_INPUT; + block_cipher_df( add_input, additional, add_len ); ctr_drbg_update_internal( ctx, add_input ); } From 5ba1d52f96abf46cde14898ae60df32d01a2a32b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 27 Nov 2014 11:33:55 +0100 Subject: [PATCH 5/9] Add memory_buffer_alloc_self_test() --- include/polarssl/memory_buffer_alloc.h | 9 ++ library/memory_buffer_alloc.c | 126 +++++++++++++++++++++++++ programs/test/selftest.c | 13 ++- 3 files changed, 145 insertions(+), 3 deletions(-) diff --git a/include/polarssl/memory_buffer_alloc.h b/include/polarssl/memory_buffer_alloc.h index c449752587..a1b4937f57 100644 --- a/include/polarssl/memory_buffer_alloc.h +++ b/include/polarssl/memory_buffer_alloc.h @@ -115,6 +115,15 @@ void memory_buffer_alloc_status( void ); */ int memory_buffer_alloc_verify( void ); +#if defined(POLARSSL_SELF_TEST) +/** + * \brief Checkup routine + * + * \return 0 if successful, or 1 if a test failed + */ +int memory_buffer_alloc_self_test( int verbose ); +#endif + #ifdef __cplusplus } #endif diff --git a/library/memory_buffer_alloc.c b/library/memory_buffer_alloc.c index 00ac3f1385..4f96018e30 100644 --- a/library/memory_buffer_alloc.c +++ b/library/memory_buffer_alloc.c @@ -586,4 +586,130 @@ void memory_buffer_alloc_free() polarssl_zeroize( &heap, sizeof(buffer_alloc_ctx) ); } +#if defined(POLARSSL_SELF_TEST) +static int check_pointer( void *p ) +{ + if( p == NULL ) + return( -1 ); + + if( (size_t) p % POLARSSL_MEMORY_ALIGN_MULTIPLE != 0 ) + return( -1 ); + + return( 0 ); +} + +static int check_all_free( ) +{ + if( heap.current_alloc_size != 0 || + heap.first != heap.first_free || + (void *) heap.first != (void *) heap.buf ) + { + return( -1 ); + } + + return( 0 ); +} + +#define TEST_ASSERT( condition ) \ + if( ! (condition) ) \ + { \ + if( verbose != 0 ) \ + polarssl_printf( "failed\n" ); \ + \ + ret = 1; \ + goto cleanup; \ + } + +int memory_buffer_alloc_self_test( int verbose ) +{ + int ret = 0; + unsigned char buf[1024]; + unsigned char *p, *q, *r; + + if( verbose != 0 ) + polarssl_printf( " MBA test #1 (basic alloc-free cycle): " ); + + memory_buffer_alloc_init( buf, sizeof( buf ) ); + + p = polarssl_malloc( 1 ); + q = polarssl_malloc( 128 ); + r = polarssl_malloc( 16 ); + + TEST_ASSERT( check_pointer( p ) == 0 && + check_pointer( q ) == 0 && + check_pointer( r ) == 0 ); + + polarssl_free( r ); + polarssl_free( q ); + polarssl_free( p ); + + TEST_ASSERT( check_all_free( ) == 0 ); + + memory_buffer_alloc_free( ); + + if( verbose != 0 ) + polarssl_printf( "passed\n" ); + + if( verbose != 0 ) + polarssl_printf( " MBA test #2 (buf not aligned): " ); + + memory_buffer_alloc_init( buf + 1, sizeof( buf ) - 1 ); + + p = polarssl_malloc( 1 ); + q = polarssl_malloc( 128 ); + r = polarssl_malloc( 16 ); + + TEST_ASSERT( check_pointer( p ) == 0 && + check_pointer( q ) == 0 && + check_pointer( r ) == 0 ); + + polarssl_free( r ); + polarssl_free( q ); + polarssl_free( p ); + + TEST_ASSERT( check_all_free( ) == 0 ); + + memory_buffer_alloc_free( ); + + if( verbose != 0 ) + polarssl_printf( "passed\n" ); + + if( verbose != 0 ) + polarssl_printf( " MBA test #3 (full): " ); + + memory_buffer_alloc_init( buf, sizeof( buf ) ); + + p = polarssl_malloc( sizeof( buf ) - sizeof( memory_header ) ); + + TEST_ASSERT( check_pointer( p ) == 0 ); + TEST_ASSERT( polarssl_malloc( 1 ) == NULL ); + + polarssl_free( p ); + + p = polarssl_malloc( sizeof( buf ) - 2 * sizeof( memory_header ) - 16 ); + q = polarssl_malloc( 16 ); + + TEST_ASSERT( check_pointer( p ) == 0 && check_pointer( q ) == 0 ); + TEST_ASSERT( polarssl_malloc( 1 ) == NULL ); + + polarssl_free( q ); + + TEST_ASSERT( polarssl_malloc( 17 ) == NULL ); + + polarssl_free( p ); + + TEST_ASSERT( check_all_free( ) == 0 ); + + memory_buffer_alloc_free( ); + + if( verbose != 0 ) + polarssl_printf( "passed\n" ); + +cleanup: + memory_buffer_alloc_free( ); + + return( ret ); +} +#endif /* POLARSSL_SELF_TEST */ + #endif /* POLARSSL_MEMORY_BUFFER_ALLOC_C */ diff --git a/programs/test/selftest.c b/programs/test/selftest.c index edf3d52d4b..3e68e36060 100644 --- a/programs/test/selftest.c +++ b/programs/test/selftest.c @@ -226,16 +226,23 @@ int main( int argc, char *argv[] ) #if defined(POLARSSL_MEMORY_BUFFER_ALLOC_C) && defined(POLARSSL_MEMORY_DEBUG) memory_buffer_alloc_status(); #endif + } +#if defined(POLARSSL_MEMORY_BUFFER_ALLOC_C) + memory_buffer_alloc_free(); + + if( ( ret = memory_buffer_alloc_self_test( v ) ) != 0 ) + return( ret ); +#endif + + if( v != 0 ) + { printf( " [ All tests passed ]\n\n" ); #if defined(_WIN32) printf( " Press Enter to exit this program.\n" ); fflush( stdout ); getchar(); #endif } -#if defined(POLARSSL_MEMORY_BUFFER_ALLOC_C) - memory_buffer_alloc_free(); -#endif return( ret ); } From 765bb31d242eaa7106010461503b72170c5fc784 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 27 Nov 2014 11:55:27 +0100 Subject: [PATCH 6/9] Add test_suite_memory_buffer_alloc --- tests/CMakeLists.txt | 1 + tests/Makefile | 5 +++++ tests/suites/main_test.function | 6 ++++-- tests/suites/test_suite_memory_buffer_alloc.data | 2 ++ .../test_suite_memory_buffer_alloc.function | 16 ++++++++++++++++ 5 files changed, 28 insertions(+), 2 deletions(-) create mode 100644 tests/suites/test_suite_memory_buffer_alloc.data create mode 100644 tests/suites/test_suite_memory_buffer_alloc.function diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 2e4d9d4a9b..ab6c59f1d1 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -77,6 +77,7 @@ add_test_suite(hmac_drbg hmac_drbg.pr) add_test_suite(hmac_shax) add_test_suite(md) add_test_suite(mdx) +add_test_suite(memory_buffer_alloc) add_test_suite(mpi) add_test_suite(pbkdf2) add_test_suite(pem) diff --git a/tests/Makefile b/tests/Makefile index c37b790fa2..37e1aacc3e 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -59,6 +59,7 @@ APPS = test_suite_aes.ecb test_suite_aes.cbc \ test_suite_hmac_drbg.nopr \ test_suite_hmac_drbg.pr \ test_suite_md test_suite_mdx \ + test_suite_memory_buffer_alloc \ test_suite_mpi test_suite_pbkdf2 \ test_suite_pem \ test_suite_pkcs1_v21 test_suite_pkcs5 \ @@ -336,6 +337,10 @@ test_suite_mdx: test_suite_mdx.c $(DEP) echo " CC $@.c" $(CC) $(CFLAGS) $(OFLAGS) $@.c $(LDFLAGS) -o $@ +test_suite_memory_buffer_alloc: test_suite_memory_buffer_alloc.c $(DEP) + echo " CC $@.c" + $(CC) $(CFLAGS) $(OFLAGS) $@.c $(LDFLAGS) -o $@ + test_suite_mpi: test_suite_mpi.c $(DEP) echo " CC $@.c" $(CC) $(CFLAGS) $(OFLAGS) $@.c $(LDFLAGS) -o $@ diff --git a/tests/suites/main_test.function b/tests/suites/main_test.function index 318ca9aedd..fae56f4252 100644 --- a/tests/suites/main_test.function +++ b/tests/suites/main_test.function @@ -211,7 +211,8 @@ int main() char buf[5000]; char *params[50]; -#if defined(POLARSSL_MEMORY_BUFFER_ALLOC_C) +#if defined(POLARSSL_MEMORY_BUFFER_ALLOC_C) && \ + !defined(TEST_SUITE_MEMORY_BUFFER_ALLOC) unsigned char alloc_buf[1000000]; memory_buffer_alloc_init( alloc_buf, sizeof(alloc_buf) ); #endif @@ -298,7 +299,8 @@ int main() fprintf( stdout, " (%d / %d tests (%d skipped))\n", total_tests - total_errors, total_tests, total_skipped ); -#if defined(POLARSSL_MEMORY_BUFFER_ALLOC_C) +#if defined(POLARSSL_MEMORY_BUFFER_ALLOC_C) && \ + !defined(TEST_SUITE_MEMORY_BUFFER_ALLOC) #if defined(POLARSSL_MEMORY_DEBUG) memory_buffer_alloc_status(); #endif diff --git a/tests/suites/test_suite_memory_buffer_alloc.data b/tests/suites/test_suite_memory_buffer_alloc.data new file mode 100644 index 0000000000..2542d4d869 --- /dev/null +++ b/tests/suites/test_suite_memory_buffer_alloc.data @@ -0,0 +1,2 @@ +Memory buffer alloc self test +memory_buffer_alloc_self_test: diff --git a/tests/suites/test_suite_memory_buffer_alloc.function b/tests/suites/test_suite_memory_buffer_alloc.function new file mode 100644 index 0000000000..88c36abc55 --- /dev/null +++ b/tests/suites/test_suite_memory_buffer_alloc.function @@ -0,0 +1,16 @@ +/* BEGIN_HEADER */ +#include +#define TEST_SUITE_MEMORY_BUFFER_ALLOC +/* END_HEADER */ + +/* BEGIN_DEPENDENCIES + * depends_on:POLARSSL_MEMORY_BUFFER_ALLOC_C + * END_DEPENDENCIES + */ + +/* BEGIN_CASE depends_on:POLARSSL_SELF_TEST */ +void memory_buffer_alloc_self_test( ) +{ + TEST_ASSERT( memory_buffer_alloc_self_test( 0 ) == 0 ); +} +/* END_CASE */ From 547ff6618fe75fb04cb5e3deb10163e50d63117c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 26 Nov 2014 15:42:16 +0100 Subject: [PATCH 7/9] Fix NULL dereference in buffer-based allocator --- ChangeLog | 6 ++++++ library/memory_buffer_alloc.c | 3 ++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 8370738b49..a1e9837f51 100644 --- a/ChangeLog +++ b/ChangeLog @@ -9,6 +9,12 @@ Features * Add support for Extended Master Secret (draft-ietf-tls-session-hash) * Add support for Encrypt-then-MAC (RFC 7366) +Security + * NULL pointer dereference in the buffer-based allocator when the buffer is + full and polarssl_free() is called (found by Jean-Philippe Aumasson) + (only possible if POLARSSL_MEMORY_BUFFER_ALLOC_C is enabled, which it is + not by default). + Bugfix * Stack buffer overflow if ctr_drbg_update() is called with too large add_len (found by Jean-Philippe Aumasson) (not triggerable remotely). diff --git a/library/memory_buffer_alloc.c b/library/memory_buffer_alloc.c index 4f96018e30..9cae251d56 100644 --- a/library/memory_buffer_alloc.c +++ b/library/memory_buffer_alloc.c @@ -484,7 +484,8 @@ static void buffer_alloc_free( void *ptr ) if( old == NULL ) { hdr->next_free = heap.first_free; - heap.first_free->prev_free = hdr; + if( heap.first_free != NULL ) + heap.first_free->prev_free = hdr; heap.first_free = hdr; } From 5dd28ea432cb8e04a2918c256d4d53f2bd417d23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 27 Nov 2014 13:57:42 +0100 Subject: [PATCH 8/9] Fix len miscalculation in buffer-based allocator --- ChangeLog | 3 +++ library/memory_buffer_alloc.c | 13 ++++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index a1e9837f51..85b5652432 100644 --- a/ChangeLog +++ b/ChangeLog @@ -18,6 +18,9 @@ Security Bugfix * Stack buffer overflow if ctr_drbg_update() is called with too large add_len (found by Jean-Philippe Aumasson) (not triggerable remotely). + * Possible buffer overflow of length at most POLARSSL_MEMORY_ALIGN_MULTIPLE + if memory_buffer_alloc_init() was called with buf not aligned and len not + a multiple of POLARSSL_MEMORY_ALIGN_MULTIPLE. = PolarSSL 1.3.9 released 2014-10-20 Security diff --git a/library/memory_buffer_alloc.c b/library/memory_buffer_alloc.c index 9cae251d56..4a5be479f8 100644 --- a/library/memory_buffer_alloc.c +++ b/library/memory_buffer_alloc.c @@ -563,9 +563,11 @@ int memory_buffer_alloc_init( unsigned char *buf, size_t len ) if( (size_t) buf % POLARSSL_MEMORY_ALIGN_MULTIPLE ) { + /* Adjust len first since buf is used in the computation */ + len -= POLARSSL_MEMORY_ALIGN_MULTIPLE + - (size_t) buf % POLARSSL_MEMORY_ALIGN_MULTIPLE; buf += POLARSSL_MEMORY_ALIGN_MULTIPLE - (size_t) buf % POLARSSL_MEMORY_ALIGN_MULTIPLE; - len -= (size_t) buf % POLARSSL_MEMORY_ALIGN_MULTIPLE; } heap.buf = buf; @@ -623,9 +625,9 @@ static int check_all_free( ) int memory_buffer_alloc_self_test( int verbose ) { - int ret = 0; unsigned char buf[1024]; - unsigned char *p, *q, *r; + unsigned char *p, *q, *r, *end; + int ret = 0; if( verbose != 0 ) polarssl_printf( " MBA test #1 (basic alloc-free cycle): " ); @@ -646,6 +648,9 @@ int memory_buffer_alloc_self_test( int verbose ) TEST_ASSERT( check_all_free( ) == 0 ); + /* Memorize end to compare with the next test */ + end = heap.buf + heap.len; + memory_buffer_alloc_free( ); if( verbose != 0 ) @@ -656,6 +661,8 @@ int memory_buffer_alloc_self_test( int verbose ) memory_buffer_alloc_init( buf + 1, sizeof( buf ) - 1 ); + TEST_ASSERT( heap.buf + heap.len == end ); + p = polarssl_malloc( 1 ); q = polarssl_malloc( 128 ); r = polarssl_malloc( 16 ); From 8b9bcecaae91b64e78498557b6270efe0138072a Mon Sep 17 00:00:00 2001 From: Paul Bakker Date: Tue, 13 Jan 2015 15:59:55 +0100 Subject: [PATCH 9/9] Stop assuming chars are signed --- programs/ssl/ssl_client2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 82877c616b..cbca810c77 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -137,7 +137,7 @@ struct options const char *alpn_string; /* ALPN supported protocols */ int fallback; /* is this a fallback connection? */ int extended_ms; /* negotiate extended master secret? */ - char etm; /* negotiate encrypt then mac? ? */ + int etm; /* negotiate encrypt then mac? */ } opt; static void my_debug( void *ctx, int level, const char *str )