From be88529753897c29c677d1becb321f0072c0659c Mon Sep 17 00:00:00 2001 From: Ben Laurie Date: Mon, 28 Jan 2013 17:34:33 +0000 Subject: [PATCH] Update DTLS code to match CBC decoding in TLS. This change updates the DTLS code to match the constant-time CBC behaviour in the TLS. (cherry picked from commit 9f27de170d1b7bef3d46d41382dc4dafde8b3900) (cherry picked from commit 5e4ca556e970edb8a7f364fcb6ee6818a965a60b) Conflicts: ssl/d1_enc.c ssl/d1_pkt.c ssl/s3_pkt.c --- ssl/d1_enc.c | 13 ++++++-- ssl/d1_pkt.c | 83 +++++++++++++++++++++++++++++++--------------------- ssl/s3_enc.c | 4 --- ssl/s3_pkt.c | 17 +++++------ ssl/t1_enc.c | 4 --- 5 files changed, 67 insertions(+), 54 deletions(-) diff --git a/ssl/d1_enc.c b/ssl/d1_enc.c index 906a26364b..f0c446dedd 100644 --- a/ssl/d1_enc.c +++ b/ssl/d1_enc.c @@ -126,6 +126,14 @@ #include #endif +/* dtls1_enc encrypts/decrypts the record in |s->wrec| / |s->rrec|, respectively. + * + * Returns: + * 0: (in non-constant time) if the record is publically invalid (i.e. too + * short etc). + * 1: if the record's padding is valid / the encryption was successful. + * -1: if the record's padding/AEAD-authenticator is invalid or, if sending, + * an internal error occured. */ int dtls1_enc(SSL *s, int send) { SSL3_RECORD *rec; @@ -165,8 +173,7 @@ int dtls1_enc(SSL *s, int send) if (s->read_hash) { mac_size=EVP_MD_size(s->read_hash); - if (mac_size < 0) - return -1; + OPENSSL_assert(mac_size >= 0); } ds=s->enc_read_ctx; rec= &(s->s3->rrec); @@ -232,7 +239,7 @@ int dtls1_enc(SSL *s, int send) if (!send) { if (l == 0 || l%bs != 0) - return -1; + return 0; } EVP_Cipher(ds,rec->data,rec->input,l); diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c index d367524536..72c5d9d294 100644 --- a/ssl/d1_pkt.c +++ b/ssl/d1_pkt.c @@ -327,17 +327,12 @@ dtls1_get_buffered_record(SSL *s) static int dtls1_process_record(SSL *s) { - int al; - int clear=0; - int enc_err; + int i,al; + int enc_err; SSL_SESSION *sess; SSL3_RECORD *rr; unsigned int mac_size; unsigned char md[EVP_MAX_MD_SIZE]; - int decryption_failed_or_bad_record_mac = 0; - unsigned char *mac = NULL; - int i; - rr= &(s->s3->rrec); sess = s->session; @@ -370,12 +365,16 @@ dtls1_process_record(SSL *s) rr->orig_len=rr->length; enc_err = s->method->ssl3_enc->enc(s,0); - if (enc_err <= 0) + /* enc_err is: + * 0: (in non-constant time) if the record is publically invalid. + * 1: if the padding is valid + * -1: if the padding is invalid */ + if (enc_err == 0) { - /* To minimize information leaked via timing, we will always - * perform all computations before discarding the message. - */ - decryption_failed_or_bad_record_mac = 1; + /* For DTLS we simply ignore bad packets. */ + rr->length = 0; + s->packet_length = 0; + goto err; } #ifdef TLS_DEBUG @@ -385,41 +384,59 @@ printf("\n"); #endif /* r->length is now the compressed data plus mac */ -if ( (sess == NULL) || - (s->enc_read_ctx == NULL) || - (s->read_hash == NULL)) - clear=1; - - if (!clear) + if ((sess != NULL) && + (s->enc_read_ctx != NULL) && + (s->read_hash != NULL)) { + /* s->read_hash != NULL => mac_size != -1 */ + unsigned char *mac = NULL; + unsigned char mac_tmp[EVP_MAX_MD_SIZE]; mac_size=EVP_MD_size(s->read_hash); + OPENSSL_assert(mac_size <= EVP_MAX_MD_SIZE); - if (rr->length > SSL3_RT_MAX_COMPRESSED_LENGTH+mac_size) + /* orig_len is the length of the record before any padding was + * removed. This is public information, as is the MAC in use, + * therefore we can safely process the record in a different + * amount of time if it's too short to possibly contain a MAC. + */ + if (rr->orig_len < mac_size || + /* CBC records must have a padding length byte too. */ + (EVP_CIPHER_CTX_mode(s->enc_read_ctx) == EVP_CIPH_CBC_MODE && + rr->orig_len < mac_size+1)) { -#if 0 /* OK only for stream ciphers (then rr->length is visible from ciphertext anyway) */ - al=SSL_AD_RECORD_OVERFLOW; - SSLerr(SSL_F_DTLS1_PROCESS_RECORD,SSL_R_PRE_MAC_LENGTH_TOO_LONG); + al=SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_SSL3_GET_RECORD,SSL_R_LENGTH_TOO_SHORT); goto f_err; -#else - decryption_failed_or_bad_record_mac = 1; -#endif } - /* check the MAC for rr->input (it's in mac_size bytes at the tail) */ - if (rr->length >= mac_size) + + if (EVP_CIPHER_CTX_mode(s->enc_read_ctx) == EVP_CIPH_CBC_MODE) { + /* We update the length so that the TLS header bytes + * can be constructed correctly but we need to extract + * the MAC in constant time from within the record, + * without leaking the contents of the padding bytes. + * */ + mac = mac_tmp; + ssl3_cbc_copy_mac(mac_tmp, rr, mac_size); rr->length -= mac_size; - mac = &rr->data[rr->length]; } else - rr->length = 0; - i=s->method->ssl3_enc->mac(s,md,0); - if (i < 0 || mac == NULL || CRYPTO_memcmp(md,mac,mac_size) != 0) { - decryption_failed_or_bad_record_mac = 1; + /* In this case there's no padding, so |rec->orig_len| + * equals |rec->length| and we checked that there's + * enough bytes for |mac_size| above. */ + rr->length -= mac_size; + mac = &rr->data[rr->length]; } + + i=s->method->ssl3_enc->mac(s,md,0 /* not send */); + if (i < 0 || mac == NULL || CRYPTO_memcmp(md, mac, (size_t)mac_size) != 0) + enc_err = -1; + if (rr->length > SSL3_RT_MAX_COMPRESSED_LENGTH+mac_size) + enc_err = -1; } - if (decryption_failed_or_bad_record_mac) + if (enc_err < 0) { /* decryption failed, silently discard message */ rr->length = 0; diff --git a/ssl/s3_enc.c b/ssl/s3_enc.c index a7830a2d43..170953ca8d 100644 --- a/ssl/s3_enc.c +++ b/ssl/s3_enc.c @@ -498,11 +498,7 @@ int ssl3_enc(SSL *s, int send) if (!send) { if (l == 0 || l%bs != 0) - { - SSLerr(SSL_F_SSL3_ENC,SSL_R_BLOCK_CIPHER_PAD_IS_WRONG); - ssl3_send_alert(s,SSL3_AL_FATAL,SSL_AD_DECRYPTION_FAILED); return 0; - } /* otherwise, rec->length >= bs */ } diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c index e658edbb1d..9f5abd516d 100644 --- a/ssl/s3_pkt.c +++ b/ssl/s3_pkt.c @@ -247,7 +247,6 @@ static int ssl3_get_record(SSL *s) unsigned char md[EVP_MAX_MD_SIZE]; short version; unsigned mac_size; - int clear=0; size_t extra; rr= &(s->s3->rrec); @@ -361,8 +360,9 @@ again: * -1: if the padding is invalid */ if (enc_err == 0) { - /* SSLerr() and ssl3_send_alert() have been called */ - goto err; + al=SSL_AD_DECRYPTION_FAILED; + SSLerr(SSL_F_TLS1_ENC,SSL_R_BLOCK_CIPHER_PAD_IS_WRONG); + goto f_err; } #ifdef TLS_DEBUG @@ -372,14 +372,11 @@ printf("\n"); #endif /* r->length is now the compressed data plus mac */ - if ( (sess == NULL) || - (s->enc_read_ctx == NULL) || - (s->read_hash == NULL)) - clear=1; - - if (!clear) + if ((sess != NULL) && + (s->enc_read_ctx != NULL) && + (s->read_hash != NULL)) { - /* !clear => s->read_hash != NULL => mac_size != -1 */ + /* s->read_hash != NULL => mac_size != -1 */ unsigned char *mac = NULL; unsigned char mac_tmp[EVP_MAX_MD_SIZE]; mac_size=EVP_MD_size(s->read_hash); diff --git a/ssl/t1_enc.c b/ssl/t1_enc.c index 8968ed0ef7..d621293a74 100644 --- a/ssl/t1_enc.c +++ b/ssl/t1_enc.c @@ -619,11 +619,7 @@ int tls1_enc(SSL *s, int send) if (!send) { if (l == 0 || l%bs != 0) - { - SSLerr(SSL_F_TLS1_ENC,SSL_R_BLOCK_CIPHER_PAD_IS_WRONG); - ssl3_send_alert(s,SSL3_AL_FATAL,SSL_AD_DECRYPTION_FAILED); return 0; - } } EVP_Cipher(ds,rec->data,rec->input,l); -- 2.34.1