From 7b42030b5d4b85a662c10024043eeec5349b6adb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 28 Jun 2018 10:38:35 +0200 Subject: [PATCH] Add counter-measure to cache-based Lucky 13 The basis for the Lucky 13 family of attacks is for an attacker to be able to distinguish between (long) valid TLS-CBC padding and invalid TLS-CBC padding. Since our code sets padlen = 0 for invalid padding, the length of the input to the HMAC function, and the location where we read the MAC, give information about that. A local attacker could gain information about that by observing via a cache attack whether the bytes at the end of the record (at the location of would-be padding) have been read during MAC verification (computation + comparison). Let's make sure they're always read. --- ChangeLog | 8 ++++++++ library/ssl_tls.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 0acb2c625..e6a5368e6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -23,6 +23,14 @@ Security a cache attack targetting an internal MD/SHA buffer. Connections using GCM or CCM instead of CBC or using Encrypt-then-Mac (RFC 7366) were not affected. Found by Kenny Paterson, Eyal Ronen and Adi Shamir. + * Add a counter-measure against a vulnerability in TLS ciphersuites based + on CBC, in (D)TLS 1.0 to 1.2, that allowed a local attacker, able to + execute code on the local machine as well as manipulate network packets, + to partially recover the plaintext of messages under some conditions (see + previous entry) by using a cache attack targeting the SSL input record + buffer. Connections using GCM or CCM instead of CBC or using + Encrypt-then-Mac (RFC 7366) were not affected. Found by Kenny Paterson, + Eyal Ronen and Adi Shamir. API Changes * Extend the platform module with a util component that contains diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 4d50497cd..e362abb78 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1276,6 +1276,27 @@ static void ssl_mac( mbedtls_md_context_t *md_ctx, #define SSL_SOME_MODES_USE_MAC #endif +/* The function below is only used in the Lucky 13 counter-measure in + * ssl_decrypt_buf(). These are the defines that guard the call site. */ +#if defined(SSL_SOME_MODES_USE_MAC) && \ + ( defined(MBEDTLS_SSL_PROTO_TLS1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_1) || \ + defined(MBEDTLS_SSL_PROTO_TLS1_2) ) +/* This function makes sure every byte in the memory region is accessed + * (in ascending addresses order) */ +static void ssl_read_memory( unsigned char *p, size_t len ) +{ + unsigned char acc = 0; + volatile unsigned char force; + + for( ; len != 0; p++, len-- ) + acc ^= *p; + + force = acc; + (void) force; +} +#endif /* SSL_SOME_MODES_USE_MAC && ( TLS1 || TLS1_1 || TLS1_2 ) */ + /* * Encryption/decryption functions */ @@ -2011,6 +2032,20 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) * linking an extra division function in some builds). */ size_t j, extra_run = 0; + + /* + * The next two sizes are the minimum and maximum values of + * in_msglen over all padlen values. + * + * They're independent of padlen, since we previously did + * in_msglen -= padlen. + * + * Note that max_len + maclen is never more than the buffer + * length, as we previously did in_msglen -= maclen too. + */ + const size_t max_len = ssl->in_msglen + padlen; + const size_t min_len = ( max_len > 256 ) ? max_len - 256 : 0; + switch( ssl->transform_in->ciphersuite_info->mac ) { #if defined(MBEDTLS_MD5_C) || defined(MBEDTLS_SHA1_C) || \ @@ -2042,12 +2077,25 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_len, 2 ); mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_msg, ssl->in_msglen ); + /* Make sure we access everything even when padlen > 0. This + * makes the synchronisation requirements for just-in-time + * Prime+Probe attacks much tighter and hopefully impractical. */ + ssl_read_memory( ssl->in_msg + ssl->in_msglen, padlen ); mbedtls_md_hmac_finish( &ssl->transform_in->md_ctx_dec, mac_expect ); - /* Call mbedtls_md_process at least once due to cache attacks */ + + /* Call mbedtls_md_process at least once due to cache attacks + * that observe whether md_process() was called of not */ for( j = 0; j < extra_run + 1; j++ ) mbedtls_md_process( &ssl->transform_in->md_ctx_dec, ssl->in_msg ); mbedtls_md_hmac_reset( &ssl->transform_in->md_ctx_dec ); + + /* Make sure we access all the memory that could contain the MAC, + * before we check it in the next code block. This makes the + * synchronisation requirements for just-in-time Prime+Probe + * attacks much tighter and hopefully impractical. */ + ssl_read_memory( ssl->in_msg + min_len, + max_len - min_len + ssl->transform_in->maclen ); } else #endif /* MBEDTLS_SSL_PROTO_TLS1 || MBEDTLS_SSL_PROTO_TLS1_1 || \ @@ -2057,9 +2105,11 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } +#if defined(MBEDTLS_SSL_DEBUG_ALL) MBEDTLS_SSL_DEBUG_BUF( 4, "expected mac", mac_expect, ssl->transform_in->maclen ); MBEDTLS_SSL_DEBUG_BUF( 4, "message mac", ssl->in_msg + ssl->in_msglen, ssl->transform_in->maclen ); +#endif if( mbedtls_ssl_safer_memcmp( ssl->in_msg + ssl->in_msglen, mac_expect, ssl->transform_in->maclen ) != 0 )