From 3f26a49eb6de454cd81ae5e58e621084cd7d645c Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 9 Aug 2016 23:36:43 -0400 Subject: [PATCH] Fix up EVP_tls_cbc_remove_padding's calling convention. The old one was rather confusing. Switch to returning 1/0 for whether the padding is publicly invalid and then add an output argument which returns a constant_time_eq-style boolean. Change-Id: Ieba89d352faf80e9bcea993b716f4b2df5439d4b Reviewed-on: https://boringssl-review.googlesource.com/10222 Commit-Queue: David Benjamin Commit-Queue: Adam Langley Reviewed-by: Adam Langley CQ-Verified: CQ bot account: commit-bot@chromium.org --- crypto/cipher/e_tls.c | 24 +++++++++++------------- crypto/cipher/internal.h | 16 ++++++++-------- crypto/cipher/tls_cbc.c | 6 +++--- 3 files changed, 22 insertions(+), 24 deletions(-) diff --git a/crypto/cipher/e_tls.c b/crypto/cipher/e_tls.c index b87b0d6e..b562a535 100644 --- a/crypto/cipher/e_tls.c +++ b/crypto/cipher/e_tls.c @@ -264,20 +264,18 @@ static int aead_tls_open(const EVP_AEAD_CTX *ctx, uint8_t *out, /* Remove CBC padding. Code from here on is timing-sensitive with respect to * |padding_ok| and |data_plus_mac_len| for CBC ciphers. */ - int padding_ok; - unsigned data_plus_mac_len, data_len; + unsigned padding_ok, data_plus_mac_len, data_len; if (EVP_CIPHER_CTX_mode(&tls_ctx->cipher_ctx) == EVP_CIPH_CBC_MODE) { - padding_ok = EVP_tls_cbc_remove_padding( - &data_plus_mac_len, out, total, - EVP_CIPHER_CTX_block_size(&tls_ctx->cipher_ctx), - (unsigned)HMAC_size(&tls_ctx->hmac_ctx)); - /* Publicly invalid. This can be rejected in non-constant time. */ - if (padding_ok == 0) { + if (!EVP_tls_cbc_remove_padding( + &padding_ok, &data_plus_mac_len, out, total, + EVP_CIPHER_CTX_block_size(&tls_ctx->cipher_ctx), + (unsigned)HMAC_size(&tls_ctx->hmac_ctx))) { + /* Publicly invalid. This can be rejected in non-constant time. */ OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_BAD_DECRYPT); return 0; } } else { - padding_ok = 1; + padding_ok = ~0u; data_plus_mac_len = total; /* |data_plus_mac_len| = |total| = |in_len| at this point. |in_len| has * already been checked against the MAC size at the top of the function. */ @@ -285,9 +283,9 @@ static int aead_tls_open(const EVP_AEAD_CTX *ctx, uint8_t *out, } data_len = data_plus_mac_len - HMAC_size(&tls_ctx->hmac_ctx); - /* At this point, |padding_ok| is 1 or -1. If 1, the padding is valid and the - * first |data_plus_mac_size| bytes after |out| are the plaintext and - * MAC. Either way, |data_plus_mac_size| is large enough to extract a MAC. */ + /* At this point, if the padding is valid, the first |data_plus_mac_len| bytes + * after |out| are the plaintext and MAC. Otherwise, |data_plus_mac_len| is + * still large enough to extract a MAC, but it will be irrelevant. */ /* To allow for CBC mode which changes cipher length, |ad| doesn't include the * length for legacy ciphers. */ @@ -338,7 +336,7 @@ static int aead_tls_open(const EVP_AEAD_CTX *ctx, uint8_t *out, * EVP_tls_cbc_remove_padding. */ unsigned good = constant_time_eq_int(CRYPTO_memcmp(record_mac, mac, mac_len), 0); - good &= constant_time_eq_int(padding_ok, 1); + good &= padding_ok; if (!good) { OPENSSL_PUT_ERROR(CIPHER, CIPHER_R_BAD_DECRYPT); return 0; diff --git a/crypto/cipher/internal.h b/crypto/cipher/internal.h index 72ac1189..dba39120 100644 --- a/crypto/cipher/internal.h +++ b/crypto/cipher/internal.h @@ -104,15 +104,15 @@ struct evp_aead_st { /* EVP_tls_cbc_get_padding determines the padding from the decrypted, TLS, CBC * record in |in|. This decrypted record should not include any "decrypted" - * explicit IV. It sets |*out_len| to the length with the padding removed or - * |in_len| if invalid. + * explicit IV. If the record is publicly invalid, it returns zero. Otherwise, + * it returns one and sets |*out_padding_ok| to all ones (0xfff..f) if the + * padding is valid and zero otherwise. It then sets |*out_len| to the length + * with the padding removed or |in_len| if invalid. * - * block_size: the block size of the cipher used to encrypt the record. - * returns: - * 0: (in non-constant time) if the record is publicly invalid. - * 1: if the padding was valid - * -1: otherwise. */ -int EVP_tls_cbc_remove_padding(unsigned *out_len, + * If the function returns one, it runs in time independent of the contents of + * |in|. It is also guaranteed that |*out_len| >= |mac_size|, satisfying + * |EVP_tls_cbc_copy_mac|'s precondition. */ +int EVP_tls_cbc_remove_padding(unsigned *out_padding_ok, unsigned *out_len, const uint8_t *in, unsigned in_len, unsigned block_size, unsigned mac_size); diff --git a/crypto/cipher/tls_cbc.c b/crypto/cipher/tls_cbc.c index fbc93fa6..a0dc509d 100644 --- a/crypto/cipher/tls_cbc.c +++ b/crypto/cipher/tls_cbc.c @@ -73,7 +73,7 @@ * supported by TLS.) */ #define MAX_HASH_BLOCK_SIZE 128 -int EVP_tls_cbc_remove_padding(unsigned *out_len, +int EVP_tls_cbc_remove_padding(unsigned *out_padding_ok, unsigned *out_len, const uint8_t *in, unsigned in_len, unsigned block_size, unsigned mac_size) { unsigned padding_length, good, to_check, i; @@ -119,8 +119,8 @@ int EVP_tls_cbc_remove_padding(unsigned *out_len, * bad padding would give POODLE's padding oracle. */ padding_length = good & (padding_length + 1); *out_len = in_len - padding_length; - - return constant_time_select_int(good, 1, -1); + *out_padding_ok = good; + return 1; } /* If CBC_MAC_ROTATE_IN_PLACE is defined then EVP_tls_cbc_copy_mac is performed