From 1ba8e911ec9aec71b131070a15f0032d5900f051 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Tue, 19 Jun 2018 15:57:50 +1000 Subject: [PATCH 1/6] CBC mode: Allow zero-length message fragments (100% padding) Fixes https://github.com/ARMmbed/mbedtls/issues/1632 --- ChangeLog | 5 +++++ library/ssl_tls.c | 14 +++++++------- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/ChangeLog b/ChangeLog index 9ea55e1fd..acf4b4b47 100644 --- a/ChangeLog +++ b/ChangeLog @@ -15,6 +15,11 @@ Bugfix by Brendan Shanks. Part of a fix for #992. * Fix compilation error when MBEDTLS_ARC4_C is disabled and MBEDTLS_CIPHER_NULL_CIPHER is enabled. Found by TrinityTonic in #1719. + * Fix decryption of zero length messages (all padding) in some circumstances: + DTLS 1.0 and 1.2, and CBC ciphersuites using encrypt-then-MAC. Most often + seen when communicating with OpenSSL using TLS 1.0. Reported by @kFYatek + (#1632) and by Conor Murphy on the forum. Fix contributed by Espressif + Systems. Changes * Change the shebang line in Perl scripts to look up perl in the PATH. diff --git a/library/ssl_tls.c b/library/ssl_tls.c index a50a669dc..d2dfa00d2 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1902,27 +1902,27 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) * and fake check up to 256 bytes of padding */ size_t pad_count = 0, real_count = 1; - size_t padding_idx = ssl->in_msglen - padlen - 1; + size_t padding_idx = ssl->in_msglen - padlen; /* * Padding is guaranteed to be incorrect if: - * 1. padlen >= ssl->in_msglen + * 1. padlen > ssl->in_msglen * - * 2. padding_idx >= MBEDTLS_SSL_MAX_CONTENT_LEN + + * 2. padding_idx > MBEDTLS_SSL_MAX_CONTENT_LEN + * ssl->transform_in->maclen * * In both cases we reset padding_idx to a safe value (0) to * prevent out-of-buffer reads. */ - correct &= ( ssl->in_msglen >= padlen + 1 ); - correct &= ( padding_idx < MBEDTLS_SSL_MAX_CONTENT_LEN + + correct &= ( padlen <= ssl->in_msglen ); + correct &= ( padding_idx <= MBEDTLS_SSL_MAX_CONTENT_LEN + ssl->transform_in->maclen ); padding_idx *= correct; - for( i = 1; i <= 256; i++ ) + for( i = 0; i < 256; i++ ) { - real_count &= ( i <= padlen ); + real_count &= ( i < padlen ); pad_count += real_count * ( ssl->in_msg[padding_idx + i] == padlen - 1 ); } From b91cb6e1e6dc116343e9b11d9373d7e81cd04a7d Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Tue, 19 Jun 2018 15:58:22 +1000 Subject: [PATCH 2/6] TLSv1.2: Treat zero-length fragments as invalid, unless they are application data TLS v1.2 explicitly disallows other kinds of zero length fragments (earlier standards don't mention zero-length fragments at all). --- library/ssl_tls.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index d2dfa00d2..34d6bd5b8 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2055,6 +2055,16 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) if( ssl->in_msglen == 0 ) { +#if defined(MBEDTLS_SSL_PROTO_TLS1_2) + if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_3 + && ssl->in_msgtype != MBEDTLS_SSL_MSG_APPLICATION_DATA ) + { + /* TLS v1.2 explicitly disallows zero-length messages which are not application data */ + MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid zero-length message type: %d", ssl->in_msgtype ) ); + return( MBEDTLS_ERR_SSL_INVALID_RECORD ); + } +#endif /* MBEDTLS_SSL_PROTO_TLS1_2 */ + ssl->nb_zero++; /* From 8946b0dd30b5f0c16c1ebd42fa0cab307284f9ae Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 20 Jun 2018 15:43:50 +1000 Subject: [PATCH 3/6] Check for invalid short Alert messages (Short Change Cipher Spec & Handshake messages are already checked for.) --- ChangeLog | 2 ++ library/ssl_tls.c | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/ChangeLog b/ChangeLog index acf4b4b47..d2ce6ef48 100644 --- a/ChangeLog +++ b/ChangeLog @@ -20,6 +20,8 @@ Bugfix seen when communicating with OpenSSL using TLS 1.0. Reported by @kFYatek (#1632) and by Conor Murphy on the forum. Fix contributed by Espressif Systems. + * Fail when receiving a TLS alert message with an invalid length, or invalid + zero-length messages when using TLS 1.2. Contributed by Espressif Systems. Changes * Change the shebang line in Perl scripts to look up perl in the PATH. diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 34d6bd5b8..bcdc8d1bd 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -4137,6 +4137,16 @@ int mbedtls_ssl_handle_message_type( mbedtls_ssl_context *ssl ) if( ssl->in_msgtype == MBEDTLS_SSL_MSG_ALERT ) { + if( ssl->in_msglen != 2 ) + { + /* Note: Standard allows for more than one 2 byte alert + to be packed in a single message, but Mbed TLS doesn't + currently support this. */ + MBEDTLS_SSL_DEBUG_MSG( 1, ( "invalid alert message, len: %d", + ssl->in_msglen ) ); + return( MBEDTLS_ERR_SSL_INVALID_RECORD ); + } + MBEDTLS_SSL_DEBUG_MSG( 2, ( "got an alert message, type: [%d:%d]", ssl->in_msg[0], ssl->in_msg[1] ) ); From 46a6d5cbe551dfa2ae8d074c5d49623b413ffe95 Mon Sep 17 00:00:00 2001 From: Andres Amaya Garcia Date: Wed, 4 Jul 2018 09:29:34 +0100 Subject: [PATCH 4/6] Fix ssl_client2 to send 0-length app data --- programs/ssl/ssl_client2.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/programs/ssl/ssl_client2.c b/programs/ssl/ssl_client2.c index 8e2feb1a1..81514321f 100644 --- a/programs/ssl/ssl_client2.c +++ b/programs/ssl/ssl_client2.c @@ -246,7 +246,11 @@ int main( void ) " server_port=%%d default: 4433\n" \ " request_page=%%s default: \".\"\n" \ " request_size=%%d default: about 34 (basic request)\n" \ - " (minimum: 0, max: " MAX_REQUEST_SIZE_STR " )\n" \ + " (minimum: 0, max: " MAX_REQUEST_SIZE_STR ")\n" \ + " If 0, in the first exchange only an empty\n" \ + " application data message is sent followed by\n" \ + " a second non-empty message before attempting\n" \ + " to read a response from the server\n" \ " debug_level=%%d default: 0 (disabled)\n" \ " nbio=%%d default: 0 (blocking I/O)\n" \ " options: 1 (non-blocking), 2 (added delays)\n" \ @@ -1528,10 +1532,13 @@ send_request: if( opt.transport == MBEDTLS_SSL_TRANSPORT_STREAM ) { - for( written = 0, frags = 0; written < len; written += ret, frags++ ) + written = 0; + frags = 0; + + do { - while( ( ret = mbedtls_ssl_write( &ssl, buf + written, len - written ) ) - <= 0 ) + while( ( ret = mbedtls_ssl_write( &ssl, buf + written, + len - written ) ) < 0 ) { if( ret != MBEDTLS_ERR_SSL_WANT_READ && ret != MBEDTLS_ERR_SSL_WANT_WRITE ) @@ -1540,7 +1547,11 @@ send_request: goto exit; } } + + frags++; + written += ret; } + while( written < len ); } else /* Not stream, so datagram */ { @@ -1567,6 +1578,13 @@ send_request: buf[written] = '\0'; mbedtls_printf( " %d bytes written in %d fragments\n\n%s\n", written, frags, (char *) buf ); + /* Send a non-empty request if request_size == 0 */ + if ( len == 0 ) + { + opt.request_size = DFL_REQUEST_SIZE; + goto send_request; + } + /* * 7. Read the HTTP response */ From 8e346dc7932901a75b51851bd7f1a834ec3c6d14 Mon Sep 17 00:00:00 2001 From: Andres Amaya Garcia Date: Wed, 4 Jul 2018 10:01:39 +0100 Subject: [PATCH 5/6] Add ChangeLog entry for empty app data fix --- ChangeLog | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ChangeLog b/ChangeLog index d2ce6ef48..cf9b51591 100644 --- a/ChangeLog +++ b/ChangeLog @@ -22,6 +22,9 @@ Bugfix Systems. * Fail when receiving a TLS alert message with an invalid length, or invalid zero-length messages when using TLS 1.2. Contributed by Espressif Systems. + * Fix ssl_client2 example to send application data with 0-length content + when the request_size argument is set to 0 as stated in the documentation. + Fixes #1833. Changes * Change the shebang line in Perl scripts to look up perl in the PATH. From 14783c47e7f79114c0ae63982f3514e16abfb811 Mon Sep 17 00:00:00 2001 From: Andres Amaya Garcia Date: Tue, 10 Jul 2018 20:08:04 +0100 Subject: [PATCH 6/6] Add test for empty app data records to ssl-opt.sh --- tests/ssl-opt.sh | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 8fde32f16..a70ace3dd 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -1151,6 +1151,38 @@ run_test "Fallback SCSV: enabled, max version, openssl client" \ -s "received FALLBACK_SCSV" \ -S "inapropriate fallback" +# Test sending and receiving empty application data records + +run_test "Encrypt then MAC: empty application data record" \ + "$P_SRV auth_mode=none debug_level=4 etm=1" \ + "$P_CLI auth_mode=none etm=1 request_size=0 force_ciphersuite=TLS-ECDHE-RSA-WITH-AES-256-CBC-SHA" \ + 0 \ + -S "0000: 0f 0f 0f 0f 0f 0f 0f 0f 0f 0f 0f 0f 0f 0f 0f 0f" \ + -s "dumping 'input payload after decrypt' (0 bytes)" \ + -c "0 bytes written in 1 fragments" + +run_test "Default, no Encrypt then MAC: empty application data record" \ + "$P_SRV auth_mode=none debug_level=4 etm=0" \ + "$P_CLI auth_mode=none etm=0 request_size=0" \ + 0 \ + -s "dumping 'input payload after decrypt' (0 bytes)" \ + -c "0 bytes written in 1 fragments" + +run_test "Encrypt then MAC, DTLS: empty application data record" \ + "$P_SRV auth_mode=none debug_level=4 etm=1 dtls=1" \ + "$P_CLI auth_mode=none etm=1 request_size=0 force_ciphersuite=TLS-ECDHE-RSA-WITH-AES-256-CBC-SHA dtls=1" \ + 0 \ + -S "0000: 0f 0f 0f 0f 0f 0f 0f 0f 0f 0f 0f 0f 0f 0f 0f 0f" \ + -s "dumping 'input payload after decrypt' (0 bytes)" \ + -c "0 bytes written in 1 fragments" + +run_test "Default, no Encrypt then MAC, DTLS: empty application data record" \ + "$P_SRV auth_mode=none debug_level=4 etm=0 dtls=1" \ + "$P_CLI auth_mode=none etm=0 request_size=0 dtls=1" \ + 0 \ + -s "dumping 'input payload after decrypt' (0 bytes)" \ + -c "0 bytes written in 1 fragments" + ## ClientHello generated with ## "openssl s_client -CAfile tests/data_files/test-ca.crt -tls1_1 -connect localhost:4433 -cipher ..." ## then manually twiddling the ciphersuite list.