From 974d4d675cc6f3e1aa50b294ae12a5ba2acebd62 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Mon, 27 Apr 2015 11:07:06 +0100 Subject: [PATCH] Sanity check EVP_CTRL_AEAD_TLS_AAD The various implementations of EVP_CTRL_AEAD_TLS_AAD expect a buffer of at least 13 bytes long. Add sanity checks to ensure that the length is at least that. Also add a new constant (EVP_AEAD_TLS1_AAD_LEN) to evp.h to represent this length. Thanks to Kevin Wojtysiak (Int3 Solutions) and Paramjot Oberoi (Int3 Solutions) for reporting this issue. Reviewed-by: Andy Polyakov (cherry picked from commit c8269881093324b881b81472be037055571f73f3) Conflicts: ssl/record/ssl3_record.c Conflicts: apps/speed.c crypto/evp/e_aes_cbc_hmac_sha256.c crypto/evp/evp.h --- crypto/evp/e_aes.c | 2 +- crypto/evp/e_aes_cbc_hmac_sha1.c | 9 ++++++--- crypto/evp/e_rc4_hmac_md5.c | 7 ++++++- crypto/evp/evp.h | 3 +++ ssl/t1_enc.c | 7 +++++-- 5 files changed, 21 insertions(+), 7 deletions(-) diff --git a/crypto/evp/e_aes.c b/crypto/evp/e_aes.c index 245c18a693..bde480481d 100644 --- a/crypto/evp/e_aes.c +++ b/crypto/evp/e_aes.c @@ -753,7 +753,7 @@ static int aes_gcm_ctrl(EVP_CIPHER_CTX *c, int type, int arg, void *ptr) case EVP_CTRL_AEAD_TLS1_AAD: /* Save the AAD for later use */ - if (arg != 13) + if (arg != EVP_AEAD_TLS1_AAD_LEN) return 0; memcpy(c->buf, ptr, arg); gctx->tls_aad_len = arg; diff --git a/crypto/evp/e_aes_cbc_hmac_sha1.c b/crypto/evp/e_aes_cbc_hmac_sha1.c index 3f8a5ae391..d1f5928f62 100644 --- a/crypto/evp/e_aes_cbc_hmac_sha1.c +++ b/crypto/evp/e_aes_cbc_hmac_sha1.c @@ -503,7 +503,12 @@ static int aesni_cbc_hmac_sha1_ctrl(EVP_CIPHER_CTX *ctx, int type, int arg, case EVP_CTRL_AEAD_TLS1_AAD: { unsigned char *p = ptr; - unsigned int len = p[arg - 2] << 8 | p[arg - 1]; + unsigned int len; + + if (arg != EVP_AEAD_TLS1_AAD_LEN) + return -1; + + len = p[arg - 2] << 8 | p[arg - 1]; if (ctx->encrypt) { key->payload_length = len; @@ -520,8 +525,6 @@ static int aesni_cbc_hmac_sha1_ctrl(EVP_CIPHER_CTX *ctx, int type, int arg, AES_BLOCK_SIZE) & -AES_BLOCK_SIZE) - len); } else { - if (arg > 13) - arg = 13; memcpy(key->aux.tls_aad, ptr, arg); key->payload_length = arg; diff --git a/crypto/evp/e_rc4_hmac_md5.c b/crypto/evp/e_rc4_hmac_md5.c index 80735d345a..e6b0cdff43 100644 --- a/crypto/evp/e_rc4_hmac_md5.c +++ b/crypto/evp/e_rc4_hmac_md5.c @@ -258,7 +258,12 @@ static int rc4_hmac_md5_ctrl(EVP_CIPHER_CTX *ctx, int type, int arg, case EVP_CTRL_AEAD_TLS1_AAD: { unsigned char *p = ptr; - unsigned int len = p[arg - 2] << 8 | p[arg - 1]; + unsigned int len; + + if (arg != EVP_AEAD_TLS1_AAD_LEN) + return -1; + + len = p[arg - 2] << 8 | p[arg - 1]; if (!ctx->encrypt) { len -= MD5_DIGEST_LENGTH; diff --git a/crypto/evp/evp.h b/crypto/evp/evp.h index b00997b149..01bdeebfbc 100644 --- a/crypto/evp/evp.h +++ b/crypto/evp/evp.h @@ -409,6 +409,9 @@ struct evp_cipher_st { /* Set the GCM invocation field, decrypt only */ # define EVP_CTRL_GCM_SET_IV_INV 0x18 +/* RFC 5246 defines additional data to be 13 bytes in length */ +# define EVP_AEAD_TLS1_AAD_LEN 13 + /* GCM TLS constants */ /* Length of fixed part of IV derived from PRF */ # define EVP_GCM_TLS_FIXED_IV_LEN 4 diff --git a/ssl/t1_enc.c b/ssl/t1_enc.c index 2736238f8e..8f45294e55 100644 --- a/ssl/t1_enc.c +++ b/ssl/t1_enc.c @@ -785,7 +785,7 @@ int tls1_enc(SSL *s, int send) bs = EVP_CIPHER_block_size(ds->cipher); if (EVP_CIPHER_flags(ds->cipher) & EVP_CIPH_FLAG_AEAD_CIPHER) { - unsigned char buf[13], *seq; + unsigned char buf[EVP_AEAD_TLS1_AAD_LEN], *seq; seq = send ? s->s3->write_sequence : s->s3->read_sequence; @@ -809,7 +809,10 @@ int tls1_enc(SSL *s, int send) buf[10] = (unsigned char)(s->version); buf[11] = rec->length >> 8; buf[12] = rec->length & 0xff; - pad = EVP_CIPHER_CTX_ctrl(ds, EVP_CTRL_AEAD_TLS1_AAD, 13, buf); + pad = EVP_CIPHER_CTX_ctrl(ds, EVP_CTRL_AEAD_TLS1_AAD, + EVP_AEAD_TLS1_AAD_LEN, buf); + if (pad <= 0) + return -1; if (send) { l += pad; rec->length += pad; -- 2.25.1