From 0a971b5dc823fe509de85eb8c560bb1de99b6c85 Mon Sep 17 00:00:00 2001 From: Paul Bakker Date: Mon, 11 Mar 2013 16:08:06 +0100 Subject: [PATCH] Removed further timing differences during SSL message decryption in ssl_decrypt_buf() New padding checking is unbiased on correct or incorrect padding and has no branch prediction timing differences. The additional MAC checks further straighten out the timing differences. (cherry picked from commit e47b34bdc8507b63758402f69e7623d11dfb6984) Conflicts: ChangeLog library/ssl_tls.c --- ChangeLog | 4 +-- include/polarssl/md5.h | 3 ++ include/polarssl/sha1.h | 3 ++ include/polarssl/sha2.h | 3 ++ library/md5.c | 2 +- library/sha1.c | 2 +- library/sha2.c | 2 +- library/ssl_tls.c | 80 ++++++++++++++++++++++++++++------------- 8 files changed, 69 insertions(+), 30 deletions(-) diff --git a/ChangeLog b/ChangeLog index 5afc2a1dc..1257e6133 100644 --- a/ChangeLog +++ b/ChangeLog @@ -7,8 +7,8 @@ Changes disabled by default and can be enabled with POLARSSL_SSL_DEBUG_ALL Security - * Removed timing differences during SSL message decryption in - ssl_decrypt_buf() due to badly formatted padding + * Removed further timing differences during SSL message decryption in + ssl_decrypt_buf() = Version 1.1.5 released on 2013-01-16 Bugfix diff --git a/include/polarssl/md5.h b/include/polarssl/md5.h index 936e9c958..316786019 100644 --- a/include/polarssl/md5.h +++ b/include/polarssl/md5.h @@ -147,6 +147,9 @@ void md5_hmac( const unsigned char *key, size_t keylen, */ int md5_self_test( int verbose ); +/* Internal use */ +void md5_process( md5_context *ctx, const unsigned char data[64] ); + #ifdef __cplusplus } #endif diff --git a/include/polarssl/sha1.h b/include/polarssl/sha1.h index 0d5e67eb2..a512edd0f 100644 --- a/include/polarssl/sha1.h +++ b/include/polarssl/sha1.h @@ -145,6 +145,9 @@ void sha1_hmac( const unsigned char *key, size_t keylen, */ int sha1_self_test( int verbose ); +/* Internal use */ +void sha1_process( sha1_context *ctx, const unsigned char data[64] ); + #ifdef __cplusplus } #endif diff --git a/include/polarssl/sha2.h b/include/polarssl/sha2.h index 811b0fd61..da3970ce0 100644 --- a/include/polarssl/sha2.h +++ b/include/polarssl/sha2.h @@ -153,6 +153,9 @@ void sha2_hmac( const unsigned char *key, size_t keylen, */ int sha2_self_test( int verbose ); +/* Internal use */ +void sha2_process( sha2_context *ctx, const unsigned char data[64] ); + #ifdef __cplusplus } #endif diff --git a/library/md5.c b/library/md5.c index 7a449b246..2c660bb88 100644 --- a/library/md5.c +++ b/library/md5.c @@ -75,7 +75,7 @@ void md5_starts( md5_context *ctx ) ctx->state[3] = 0x10325476; } -static void md5_process( md5_context *ctx, const unsigned char data[64] ) +void md5_process( md5_context *ctx, const unsigned char data[64] ) { unsigned long X[16], A, B, C, D; diff --git a/library/sha1.c b/library/sha1.c index 72ca06338..cda40b4a0 100644 --- a/library/sha1.c +++ b/library/sha1.c @@ -76,7 +76,7 @@ void sha1_starts( sha1_context *ctx ) ctx->state[4] = 0xC3D2E1F0; } -static void sha1_process( sha1_context *ctx, const unsigned char data[64] ) +void sha1_process( sha1_context *ctx, const unsigned char data[64] ) { unsigned long temp, W[16], A, B, C, D, E; diff --git a/library/sha2.c b/library/sha2.c index 4b5e6962f..ec87d18f5 100644 --- a/library/sha2.c +++ b/library/sha2.c @@ -97,7 +97,7 @@ void sha2_starts( sha2_context *ctx, int is224 ) ctx->is224 = is224; } -static void sha2_process( sha2_context *ctx, const unsigned char data[64] ) +void sha2_process( sha2_context *ctx, const unsigned char data[64] ) { unsigned long temp1, temp2, W[64]; unsigned long A, B, C, D, E, F, G, H; diff --git a/library/ssl_tls.c b/library/ssl_tls.c index d7413d9a8..013483494 100644 --- a/library/ssl_tls.c +++ b/library/ssl_tls.c @@ -683,7 +683,7 @@ static int ssl_decrypt_buf( ssl_context *ssl ) unsigned char *dec_msg; unsigned char *dec_msg_result; size_t dec_msglen; - size_t minlen = 0, fake_padlen; + size_t minlen = 0; /* * Check immediate ciphertext sanity @@ -765,7 +765,6 @@ static int ssl_decrypt_buf( ssl_context *ssl ) } padlen = 1 + ssl->in_msg[ssl->in_msglen - 1]; - fake_padlen = 256 - padlen; if( ssl->in_msglen < ssl->maclen + padlen ) { @@ -774,7 +773,6 @@ static int ssl_decrypt_buf( ssl_context *ssl ) ssl->in_msglen, ssl->maclen, padlen ) ); #endif padlen = 0; - fake_padlen = 256; correct = 0; } @@ -796,26 +794,23 @@ static int ssl_decrypt_buf( ssl_context *ssl ) * TLSv1+: always check the padding up to the first failure * and fake check up to 256 bytes of padding */ + size_t pad_count = 0, fake_pad_count = 0; + size_t padding_idx = ssl->in_msglen - padlen - 1; + for( i = 1; i <= padlen; i++ ) - { - if( ssl->in_msg[ssl->in_msglen - i] != padlen - 1 ) - { - correct = 0; - fake_padlen = 256 - i; - padlen = 0; - } - } - for( i = 1; i <= fake_padlen; i++ ) - { - if( ssl->in_msg[i + 1] != fake_padlen - 1 ) - minlen = 0; - else - minlen = 1; - } + pad_count += ( ssl->in_msg[padding_idx + i] == padlen - 1 ); + + for( ; i <= 256; i++ ) + fake_pad_count += ( ssl->in_msg[padding_idx + i] == padlen - 1 ); + + correct &= ( pad_count == padlen ); /* Only 1 on correct padding */ + correct &= ( pad_count + fake_pad_count < 512 ); /* Always 1 */ + #if defined(POLARSSL_SSL_DEBUG_ALL) if( padlen > 0 && correct == 0) SSL_DEBUG_MSG( 1, ( "bad padding byte detected" ) ); #endif + padlen &= correct * 0x1FF; } } @@ -848,15 +843,48 @@ static int ssl_decrypt_buf( 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 + * + * 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) */ + int j, extra_run = 0; + extra_run = ( 13 + ssl->in_msglen + padlen + 8 ) / 64 - + ( 13 + ssl->in_msglen + 8 ) / 64; + + extra_run &= correct * 0xFF; + if( ssl->maclen == 16 ) - md5_hmac( ssl->mac_dec, 16, - ssl->in_ctr, ssl->in_msglen + 13, - ssl->in_msg + ssl->in_msglen ); - else - sha1_hmac( ssl->mac_dec, 20, - ssl->in_ctr, ssl->in_msglen + 13, - ssl->in_msg + ssl->in_msglen ); + { + md5_context ctx; + md5_hmac_starts( &ctx, ssl->mac_dec, 16 ); + md5_hmac_update( &ctx, ssl->in_ctr, ssl->in_msglen + 13 ); + md5_hmac_finish( &ctx, ssl->in_msg + ssl->in_msglen ); + + for( j = 0; j < extra_run; j++ ) + md5_process( &ctx, ssl->in_msg ); + } + else if( ssl->maclen == 20 ) + { + sha1_context ctx; + sha1_hmac_starts( &ctx, ssl->mac_dec, 20 ); + sha1_hmac_update( &ctx, ssl->in_ctr, ssl->in_msglen + 13 ); + sha1_hmac_finish( &ctx, ssl->in_msg + ssl->in_msglen ); + + for( j = 0; j < extra_run; j++ ) + sha1_process( &ctx, ssl->in_msg ); + } + else if( ssl->maclen != 0 ) + { + SSL_DEBUG_MSG( 1, ( "invalid MAC len: %d", + ssl->maclen ) ); + return( POLARSSL_ERR_SSL_FEATURE_UNAVAILABLE ); + } } SSL_DEBUG_BUF( 4, "message mac", tmp, ssl->maclen ); @@ -866,7 +894,9 @@ static int ssl_decrypt_buf( ssl_context *ssl ) if( memcmp( tmp, ssl->in_msg + ssl->in_msglen, ssl->maclen ) != 0 ) { +#if defined(POLARSSL_SSL_DEBUG_ALL) SSL_DEBUG_MSG( 1, ( "message mac does not match" ) ); +#endif correct = 0; }