From 6db7d0153abdaaf73afa518f6bb643e4f423a3a8 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Wed, 28 Jun 2017 15:18:30 +0100 Subject: [PATCH] Fix undefined behaviour in e_aes_cbc_hmac_sha256.c and e_aes_cbc_hmac_sha1.c In TLS mode of operation the padding value "pad" is obtained along with the maximum possible padding value "maxpad". If pad > maxpad then the data is invalid. However we must continue anyway because this is constant time code. We calculate the payload length like this: inp_len = len - (SHA_DIGEST_LENGTH + pad + 1); However if pad is invalid then inp_len ends up -ve (actually large +ve because it is a size_t). Later we do this: /* verify HMAC */ out += inp_len; len -= inp_len; This ends up with "out" pointing before the buffer which is undefined behaviour. Next we calculate "p" like this: unsigned char *p = out + len - 1 - maxpad - SHA256_DIGEST_LENGTH; Because of the "out + len" term the -ve inp_len value is cancelled out so "p" points to valid memory (although technically the pointer arithmetic is undefined behaviour again). We only ever then dereference "p" and never "out" directly so there is never an invalid read based on the bad pointer - so there is no security issue. This commit fixes the undefined behaviour by ensuring we use maxpad in place of pad, if the supplied pad is invalid. With thanks to Brian Carpenter for reporting this issue. Reviewed-by: Rich Salz (Merged from https://github.com/openssl/openssl/pull/3832) (cherry picked from commit 335d0a4646981c9d96b62811bcfd69a96a1a67d9) --- crypto/evp/e_aes_cbc_hmac_sha1.c | 10 +++++++++- crypto/evp/e_aes_cbc_hmac_sha256.c | 10 +++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/crypto/evp/e_aes_cbc_hmac_sha1.c b/crypto/evp/e_aes_cbc_hmac_sha1.c index 8e71e591ea..fe76dedaa8 100644 --- a/crypto/evp/e_aes_cbc_hmac_sha1.c +++ b/crypto/evp/e_aes_cbc_hmac_sha1.c @@ -528,7 +528,15 @@ static int aesni_cbc_hmac_sha1_cipher(EVP_CIPHER_CTX *ctx, unsigned char *out, maxpad |= (255 - maxpad) >> (sizeof(maxpad) * 8 - 8); maxpad &= 255; - ret &= constant_time_ge(maxpad, pad); + mask = constant_time_ge(maxpad, pad); + ret &= mask; + /* + * If pad is invalid then we will fail the above test but we must + * continue anyway because we are in constant time code. However, + * we'll use the maxpad value instead of the supplied pad to make + * sure we perform well defined pointer arithmetic. + */ + pad = constant_time_select(mask, pad, maxpad); inp_len = len - (SHA_DIGEST_LENGTH + pad + 1); mask = (0 - ((inp_len - len) >> (sizeof(inp_len) * 8 - 1))); diff --git a/crypto/evp/e_aes_cbc_hmac_sha256.c b/crypto/evp/e_aes_cbc_hmac_sha256.c index f0be05ca46..ce527178ad 100644 --- a/crypto/evp/e_aes_cbc_hmac_sha256.c +++ b/crypto/evp/e_aes_cbc_hmac_sha256.c @@ -538,7 +538,15 @@ static int aesni_cbc_hmac_sha256_cipher(EVP_CIPHER_CTX *ctx, maxpad |= (255 - maxpad) >> (sizeof(maxpad) * 8 - 8); maxpad &= 255; - ret &= constant_time_ge(maxpad, pad); + mask = constant_time_ge(maxpad, pad); + ret &= mask; + /* + * If pad is invalid then we will fail the above test but we must + * continue anyway because we are in constant time code. However, + * we'll use the maxpad value instead of the supplied pad to make + * sure we perform well defined pointer arithmetic. + */ + pad = constant_time_select(mask, pad, maxpad); inp_len = len - (SHA256_DIGEST_LENGTH + pad + 1); mask = (0 - ((inp_len - len) >> (sizeof(inp_len) * 8 - 1))); -- 2.25.1