kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Fix up EVP_tls_cbc_remove_padding's calling convention.

Browse Source 
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 <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
This commit is contained in:
David Benjamin 2016-08-09 23:36:43 -04:00 committed by CQ bot account: commit-bot@chromium.org
parent 65d74e4d76
commit 3f26a49eb6
3 changed files with 22 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

24
crypto/cipher/e_tls.c
View File

@ -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;

16
crypto/cipher/internal.h
View File

@ -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);

6
crypto/cipher/tls_cbc.c
View File

@ -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