From 20b4408fbd4c5663f73a12d05a31722a8f4a18ab Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 29 May 2018 14:06:49 +0200 Subject: [PATCH 01/10] Fix Lucky13 attack protection when using HMAC-SHA-384 As a protection against the Lucky Thirteen attack, the TLS code for CBC decryption in encrypt-then-MAC mode performs extra MAC calculations to compensate for variations in message size due to padding. The amount of extra MAC calculation to perform was based on the assumption that the bulk of the time is spent in processing 64-byte blocks, which is correct for most supported hashes but not for SHA-384. Correct the amount of extra work for SHA-384 (and SHA-512 which is currently not used in TLS, and MD2 although no one should care about that). --- library/ssl_tls.c | 62 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 54 insertions(+), 8 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index bc9dc77e1..6fdfb6349 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1985,20 +1985,66 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) { /* * Process MAC and always update for padlen afterwards to make - * total time independent of padlen - * - * extra_run compensates MAC check for padlen + * total time independent of padlen. * * Known timing attacks: * - Lucky Thirteen (http://www.isg.rhul.ac.uk/tls/TLStiming.pdf) * - * We use ( ( Lx + 8 ) / 64 ) to handle 'negative Lx' values - * correctly. (We round down instead of up, so -56 is the correct - * value for our calculations instead of -55) + * To compensate for different timings for the MAC calculation + * depending on how much padding was removed (which is determined + * by padlen), process extra_run more blocks through the hash + * function. + * + * The formula in the paper is + * extra_run = ceil( (L1-55) / 64 ) - ceil( (L2-55) / 64 ) + * where L1 is the size of the header plus the decrypted message + * plus CBC padding and L2 is the size of the header plus the + * decrypted message. This is for an underlying hash function + * with 64-byte blocks. + * We use ( (Lx+8) / 64 ) to handle 'negative Lx' values + * correctly. We round down instead of up, so -56 is the correct + * value for our calculations instead of -55. + * + * Repeat the formula rather than defining a block_size variable + * so that the code only uses division by a constant, not division + * by a variable. */ size_t j, extra_run = 0; - extra_run = ( 13 + ssl->in_msglen + padlen + 8 ) / 64 - - ( 13 + ssl->in_msglen + 8 ) / 64; + switch( ssl->transform_in->ciphersuite_info->mac ) + { +#if defined(MBEDTLS_MD2_C) + case MBEDTLS_MD_MD2: + /* no size prepended, 64-byte compression blocks */ + extra_run = ( 13 + ssl->in_msglen + padlen ) / 64 - + ( 13 + ssl->in_msglen ) / 64; + break; +#endif +#if defined(MBEDTLS_MD4_C) || defined(MBEDTLS_MD5_C) || \ + defined(MBEDTLS_SHA1_C) || defined(MBEDTLS_SHA224_C) || \ + defined(MBEDTLS_SHA256_C) || defined(MBEDTLS_RIPEMD160_C) + case MBEDTLS_MD_MD4: + case MBEDTLS_MD_MD5: + case MBEDTLS_MD_SHA1: + case MBEDTLS_MD_SHA224: + case MBEDTLS_MD_SHA256: + case MBEDTLS_MD_RIPEMD160: + /* 8 bytes of message size, 64-byte compression blocks */ + extra_run = ( 13 + ssl->in_msglen + padlen + 8 ) / 64 - + ( 13 + ssl->in_msglen + 8 ) / 64; + break; +#endif +#if defined(MBEDTLS_SHA384_C) || defined(MBEDTLS_SHA512_C) + case MBEDTLS_MD_SHA384: + case MBEDTLS_MD_SHA512: + /* 16 bytes of message size, 128-byte compression blocks */ + extra_run = ( 13 + ssl->in_msglen + padlen + 16 ) / 128 - + ( 13 + ssl->in_msglen + 16 ) / 128; + break; +#endif + default: + MBEDTLS_SSL_DEBUG_MSG( 1, ( "unsupported HMAC hash" ) ); + return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); + } extra_run &= correct * 0xFF; From 1bd9d58b21b5b19d70fd262f80351a8c48ea941b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 4 Jun 2018 11:58:44 +0200 Subject: [PATCH 02/10] Clarify comment about integer division by a variable --- library/ssl_tls.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 6fdfb6349..e1b8f9c5b 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2005,9 +2005,10 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) * correctly. We round down instead of up, so -56 is the correct * value for our calculations instead of -55. * - * Repeat the formula rather than defining a block_size variable - * so that the code only uses division by a constant, not division - * by a variable. + * Repeat the formula rather than defining a block_size variable. + * This avoids requiring division by a variable at runtime + * (which would be marginally less efficient and would require + * linking an extra division function in some builds). */ size_t j, extra_run = 0; switch( ssl->transform_in->ciphersuite_info->mac ) From a7fe25d5a53bd930a56b0980d214914cb7f6821b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 4 Jun 2018 12:01:18 +0200 Subject: [PATCH 03/10] Remove tests of #define's that don't exist --- library/ssl_tls.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index e1b8f9c5b..893429d78 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2021,9 +2021,9 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) break; #endif #if defined(MBEDTLS_MD4_C) || defined(MBEDTLS_MD5_C) || \ - defined(MBEDTLS_SHA1_C) || defined(MBEDTLS_SHA224_C) || \ - defined(MBEDTLS_SHA256_C) || defined(MBEDTLS_RIPEMD160_C) case MBEDTLS_MD_MD4: + defined(MBEDTLS_SHA1_C) || defined(MBEDTLS_SHA256_C) \ + defined(MBEDTLS_RIPEMD160_C) case MBEDTLS_MD_MD5: case MBEDTLS_MD_SHA1: case MBEDTLS_MD_SHA224: @@ -2034,7 +2034,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) ( 13 + ssl->in_msglen + 8 ) / 64; break; #endif -#if defined(MBEDTLS_SHA384_C) || defined(MBEDTLS_SHA512_C) +#if defined(MBEDTLS_SHA512_C) case MBEDTLS_MD_SHA384: case MBEDTLS_MD_SHA512: /* 16 bytes of message size, 128-byte compression blocks */ From 5c38984fa70bf4998bacea1251003d3dc61f915c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 4 Jun 2018 12:02:43 +0200 Subject: [PATCH 04/10] Use our habitual INTERNAL_ERROR debug message --- library/ssl_tls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 893429d78..8e855a120 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2043,7 +2043,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) break; #endif default: - MBEDTLS_SSL_DEBUG_MSG( 1, ( "unsupported HMAC hash" ) ); + MBEDTLS_SSL_DEBUG_MSG( 1, ( "should never happen" ) ); return( MBEDTLS_ERR_SSL_INTERNAL_ERROR ); } From d0e55a465779554d354343601161aa11f69353b6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 4 Jun 2018 12:03:30 +0200 Subject: [PATCH 05/10] ssl_decrypt_buf: remove code for hashes that aren't used in TLS --- library/ssl_tls.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index 8e855a120..4d50497cd 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -2013,22 +2013,11 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) size_t j, extra_run = 0; switch( ssl->transform_in->ciphersuite_info->mac ) { -#if defined(MBEDTLS_MD2_C) - case MBEDTLS_MD_MD2: - /* no size prepended, 64-byte compression blocks */ - extra_run = ( 13 + ssl->in_msglen + padlen ) / 64 - - ( 13 + ssl->in_msglen ) / 64; - break; -#endif -#if defined(MBEDTLS_MD4_C) || defined(MBEDTLS_MD5_C) || \ - case MBEDTLS_MD_MD4: - defined(MBEDTLS_SHA1_C) || defined(MBEDTLS_SHA256_C) \ - defined(MBEDTLS_RIPEMD160_C) +#if defined(MBEDTLS_MD5_C) || defined(MBEDTLS_SHA1_C) || \ + defined(MBEDTLS_SHA256_C) case MBEDTLS_MD_MD5: case MBEDTLS_MD_SHA1: - case MBEDTLS_MD_SHA224: case MBEDTLS_MD_SHA256: - case MBEDTLS_MD_RIPEMD160: /* 8 bytes of message size, 64-byte compression blocks */ extra_run = ( 13 + ssl->in_msglen + padlen + 8 ) / 64 - ( 13 + ssl->in_msglen + 8 ) / 64; @@ -2036,7 +2025,6 @@ static int ssl_decrypt_buf( mbedtls_ssl_context *ssl ) #endif #if defined(MBEDTLS_SHA512_C) case MBEDTLS_MD_SHA384: - case MBEDTLS_MD_SHA512: /* 16 bytes of message size, 128-byte compression blocks */ extra_run = ( 13 + ssl->in_msglen + padlen + 16 ) / 128 - ( 13 + ssl->in_msglen + 16 ) / 128; From 104d85865d1339225f1b706d841597a7430c7e85 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 27 Jun 2018 10:57:33 +0200 Subject: [PATCH 06/10] Add ChangeLog entry --- ChangeLog | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/ChangeLog b/ChangeLog index 348864c0e..19bdb79f1 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,21 @@ mbed TLS ChangeLog (Sorted per branch, date) = mbed TLS x.x.x branch released xxxx-xx-xx +Security + * Fix a vulnerability in TLS ciphersuites based on CBC and using SHA-384, + in (D)TLS 1.0 to 1.2, that allowed an active network attacker to + partially recover the plaintext of messages under some conditions by + exploiting timing measurements. With DTLS, the attacker could perform + this recovery by sending many messages in the same connection. With TLS + or if mbedtls_ssl_conf_dtls_badmac_limit() was used, the attack only + worked if the same secret (for example a HTTP Cookie) has been repeatedly + sent over connections manipulated by the attacker. Connections using GCM + or CCM instead of CBC, using hash sizes other than SHA-384, or using + Encrypt-then-Mac (RFC 7366) were not affected. The vulnerability was + caused by a miscalculation (for SHA-384) in a countermeasure to the + original Lucky 13 attack. Found by Kenny Paterson, Eyal Ronen and Adi + Shamir. + API Changes * Extend the platform module with a util component that contains functionality shared by multiple Mbed TLS modules. At this stage From 1cc1fb05999aea8067e11f5c4f4fdb32dbe91036 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Thu, 28 Jun 2018 12:10:27 +0200 Subject: [PATCH 07/10] Fix Lucky 13 cache attack on MD/SHA padding 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 gives information about that. Information about this length (modulo the MD/SHA block size) can be deduced from how much MD/SHA padding (this is distinct from TLS-CBC padding) is used. If MD/SHA padding is read from a (static) buffer, a local attacker could get information about how much is used via a cache attack targeting that buffer. Let's get rid of this buffer. Now the only buffer used is the internal MD/SHA one, which is always read fully by the process() function. --- ChangeLog | 7 ++++++ library/md5.c | 54 +++++++++++++++++++++++++++----------------- library/sha1.c | 51 +++++++++++++++++++++++++++--------------- library/sha256.c | 52 +++++++++++++++++++++++++++---------------- library/sha512.c | 58 ++++++++++++++++++++++++++++-------------------- 5 files changed, 141 insertions(+), 81 deletions(-) diff --git a/ChangeLog b/ChangeLog index 19bdb79f1..0acb2c625 100644 --- a/ChangeLog +++ b/ChangeLog @@ -16,6 +16,13 @@ Security caused by a miscalculation (for SHA-384) in a countermeasure to the original Lucky 13 attack. Found by Kenny Paterson, Eyal Ronen and Adi Shamir. + * Fix 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 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. API Changes * Extend the platform module with a util component that contains diff --git a/library/md5.c b/library/md5.c index 8238c2b81..2a740cda8 100644 --- a/library/md5.c +++ b/library/md5.c @@ -309,14 +309,6 @@ void mbedtls_md5_update( mbedtls_md5_context *ctx, } #endif -static const unsigned char md5_padding[64] = -{ - 0x80, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 -}; - /* * MD5 final digest */ @@ -324,26 +316,48 @@ int mbedtls_md5_finish_ret( mbedtls_md5_context *ctx, unsigned char output[16] ) { int ret; - uint32_t last, padn; + uint32_t used; uint32_t high, low; - unsigned char msglen[8]; + /* + * Add padding: 0x80 then 0x00 until 8 bytes remain for the length + */ + used = ctx->total[0] & 0x3F; + + ctx->buffer[used++] = 0x80; + + if( used <= 56 ) + { + /* Enough room for padding + length in current block */ + memset( ctx->buffer + used, 0, 56 - used ); + } + else + { + /* We'll need an extra block */ + memset( ctx->buffer + used, 0, 64 - used ); + + if( ( ret = mbedtls_internal_md5_process( ctx, ctx->buffer ) ) != 0 ) + return( ret ); + + memset( ctx->buffer, 0, 56 ); + } + + /* + * Add message length + */ high = ( ctx->total[0] >> 29 ) | ( ctx->total[1] << 3 ); low = ( ctx->total[0] << 3 ); - PUT_UINT32_LE( low, msglen, 0 ); - PUT_UINT32_LE( high, msglen, 4 ); + PUT_UINT32_LE( low, ctx->buffer, 56 ); + PUT_UINT32_LE( high, ctx->buffer, 60 ); - last = ctx->total[0] & 0x3F; - padn = ( last < 56 ) ? ( 56 - last ) : ( 120 - last ); - - if( ( ret = mbedtls_md5_update_ret( ctx, md5_padding, padn ) ) != 0 ) - return( ret ); - - if( ( ret = mbedtls_md5_update_ret( ctx, msglen, 8 ) ) != 0 ) - return( ret ); + if( ( ret = mbedtls_internal_md5_process( ctx, ctx->buffer ) ) != 0 ) + return( ret ); + /* + * Output final state + */ PUT_UINT32_LE( ctx->state[0], output, 0 ); PUT_UINT32_LE( ctx->state[1], output, 4 ); PUT_UINT32_LE( ctx->state[2], output, 8 ); diff --git a/library/sha1.c b/library/sha1.c index 1587de480..bab6087c4 100644 --- a/library/sha1.c +++ b/library/sha1.c @@ -342,14 +342,6 @@ void mbedtls_sha1_update( mbedtls_sha1_context *ctx, } #endif -static const unsigned char sha1_padding[64] = -{ - 0x80, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 -}; - /* * SHA-1 final digest */ @@ -357,25 +349,48 @@ int mbedtls_sha1_finish_ret( mbedtls_sha1_context *ctx, unsigned char output[20] ) { int ret; - uint32_t last, padn; + uint32_t used; uint32_t high, low; - unsigned char msglen[8]; + /* + * Add padding: 0x80 then 0x00 until 8 bytes remain for the length + */ + used = ctx->total[0] & 0x3F; + + ctx->buffer[used++] = 0x80; + + if( used <= 56 ) + { + /* Enough room for padding + length in current block */ + memset( ctx->buffer + used, 0, 56 - used ); + } + else + { + /* We'll need an extra block */ + memset( ctx->buffer + used, 0, 64 - used ); + + if( ( ret = mbedtls_internal_sha1_process( ctx, ctx->buffer ) ) != 0 ) + return( ret ); + + memset( ctx->buffer, 0, 56 ); + } + + /* + * Add message length + */ high = ( ctx->total[0] >> 29 ) | ( ctx->total[1] << 3 ); low = ( ctx->total[0] << 3 ); - PUT_UINT32_BE( high, msglen, 0 ); - PUT_UINT32_BE( low, msglen, 4 ); + PUT_UINT32_BE( high, ctx->buffer, 56 ); + PUT_UINT32_BE( low, ctx->buffer, 60 ); - last = ctx->total[0] & 0x3F; - padn = ( last < 56 ) ? ( 56 - last ) : ( 120 - last ); - - if( ( ret = mbedtls_sha1_update_ret( ctx, sha1_padding, padn ) ) != 0 ) - return( ret ); - if( ( ret = mbedtls_sha1_update_ret( ctx, msglen, 8 ) ) != 0 ) + if( ( ret = mbedtls_internal_sha1_process( ctx, ctx->buffer ) ) != 0 ) return( ret ); + /* + * Output final state + */ PUT_UINT32_BE( ctx->state[0], output, 0 ); PUT_UINT32_BE( ctx->state[1], output, 4 ); PUT_UINT32_BE( ctx->state[2], output, 8 ); diff --git a/library/sha256.c b/library/sha256.c index 695485d84..dbb4a8986 100644 --- a/library/sha256.c +++ b/library/sha256.c @@ -311,14 +311,6 @@ void mbedtls_sha256_update( mbedtls_sha256_context *ctx, } #endif -static const unsigned char sha256_padding[64] = -{ - 0x80, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 -}; - /* * SHA-256 final digest */ @@ -326,26 +318,48 @@ int mbedtls_sha256_finish_ret( mbedtls_sha256_context *ctx, unsigned char output[32] ) { int ret; - uint32_t last, padn; + uint32_t used; uint32_t high, low; - unsigned char msglen[8]; + /* + * Add padding: 0x80 then 0x00 until 8 bytes remain for the length + */ + used = ctx->total[0] & 0x3F; + + ctx->buffer[used++] = 0x80; + + if( used <= 56 ) + { + /* Enough room for padding + length in current block */ + memset( ctx->buffer + used, 0, 56 - used ); + } + else + { + /* We'll need an extra block */ + memset( ctx->buffer + used, 0, 64 - used ); + + if( ( ret = mbedtls_internal_sha256_process( ctx, ctx->buffer ) ) != 0 ) + return( ret ); + + memset( ctx->buffer, 0, 56 ); + } + + /* + * Add message length + */ high = ( ctx->total[0] >> 29 ) | ( ctx->total[1] << 3 ); low = ( ctx->total[0] << 3 ); - PUT_UINT32_BE( high, msglen, 0 ); - PUT_UINT32_BE( low, msglen, 4 ); + PUT_UINT32_BE( high, ctx->buffer, 56 ); + PUT_UINT32_BE( low, ctx->buffer, 60 ); - last = ctx->total[0] & 0x3F; - padn = ( last < 56 ) ? ( 56 - last ) : ( 120 - last ); - - if( ( ret = mbedtls_sha256_update_ret( ctx, sha256_padding, padn ) ) != 0 ) - return( ret ); - - if( ( ret = mbedtls_sha256_update_ret( ctx, msglen, 8 ) ) != 0 ) + if( ( ret = mbedtls_internal_sha256_process( ctx, ctx->buffer ) ) != 0 ) return( ret ); + /* + * Output final state + */ PUT_UINT32_BE( ctx->state[0], output, 0 ); PUT_UINT32_BE( ctx->state[1], output, 4 ); PUT_UINT32_BE( ctx->state[2], output, 8 ); diff --git a/library/sha512.c b/library/sha512.c index 6de94e99b..a9440e8af 100644 --- a/library/sha512.c +++ b/library/sha512.c @@ -341,18 +341,6 @@ void mbedtls_sha512_update( mbedtls_sha512_context *ctx, } #endif -static const unsigned char sha512_padding[128] = -{ - 0x80, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 -}; - /* * SHA-512 final digest */ @@ -360,26 +348,48 @@ int mbedtls_sha512_finish_ret( mbedtls_sha512_context *ctx, unsigned char output[64] ) { int ret; - size_t last, padn; + unsigned used; uint64_t high, low; - unsigned char msglen[16]; + /* + * Add padding: 0x80 then 0x00 until 16 bytes remain for the length + */ + used = ctx->total[0] & 0x7F; + + ctx->buffer[used++] = 0x80; + + if( used <= 112 ) + { + /* Enough room for padding + length in current block */ + memset( ctx->buffer + used, 0, 112 - used ); + } + else + { + /* We'll need an extra block */ + memset( ctx->buffer + used, 0, 128 - used ); + + if( ( ret = mbedtls_internal_sha512_process( ctx, ctx->buffer ) ) != 0 ) + return( ret ); + + memset( ctx->buffer, 0, 112 ); + } + + /* + * Add message length + */ high = ( ctx->total[0] >> 61 ) | ( ctx->total[1] << 3 ); low = ( ctx->total[0] << 3 ); - PUT_UINT64_BE( high, msglen, 0 ); - PUT_UINT64_BE( low, msglen, 8 ); + PUT_UINT64_BE( high, ctx->buffer, 112 ); + PUT_UINT64_BE( low, ctx->buffer, 120 ); - last = (size_t)( ctx->total[0] & 0x7F ); - padn = ( last < 112 ) ? ( 112 - last ) : ( 240 - last ); - - if( ( ret = mbedtls_sha512_update_ret( ctx, sha512_padding, padn ) ) != 0 ) - return( ret ); - - if( ( ret = mbedtls_sha512_update_ret( ctx, msglen, 16 ) ) != 0 ) - return( ret ); + if( ( ret = mbedtls_internal_sha512_process( ctx, ctx->buffer ) ) != 0 ) + return( ret ); + /* + * Output final state + */ PUT_UINT64_BE( ctx->state[0], output, 0 ); PUT_UINT64_BE( ctx->state[1], output, 8 ); PUT_UINT64_BE( ctx->state[2], output, 16 ); 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 08/10] 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 ) From 6a25cfae2a7bf34f206232168942bd2db0886742 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Tue, 10 Jul 2018 11:15:36 +0200 Subject: [PATCH 09/10] Avoid debug message that might leak length The length to the debug message could conceivably leak through the time it takes to print it, and that length would in turn reveal whether padding was correct or not. --- library/ssl_tls.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/ssl_tls.c b/library/ssl_tls.c index e362abb78..d66c9cfcc 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -1972,8 +1972,10 @@ 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, "raw buffer after decryption", ssl->in_msg, ssl->in_msglen ); +#endif /* * Authenticate if not done yet. From 830ce11ebaad029b06d06fcad1e39a67d1cd1b60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20P=C3=A9gouri=C3=A9-Gonnard?= Date: Wed, 11 Jul 2018 18:27:08 +0200 Subject: [PATCH 10/10] Clarify attack conditions in the ChangeLog. Referring to the previous entry could imply that the current one was limited to SHA-384 too, which it isn't. --- ChangeLog | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/ChangeLog b/ChangeLog index e6a5368e6..e4a05c79f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -19,10 +19,13 @@ Security * Fix 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 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. + plaintext of messages under some conditions by using a cache attack + targetting an internal MD/SHA buffer. With TLS or if + mbedtls_ssl_conf_dtls_badmac_limit() was used, the attack only worked if + the same secret (for example a HTTP Cookie) has been repeatedly sent over + connections manipulated by the attacker. 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,