From 4841ce49a015237096e280fbacd1f4e2555df744 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 15 Dec 2014 03:59:02 -0500 Subject: [PATCH] Fix EVP_Cipher error-handling. Turns out the EVP_CIPH_FLAG_CUSTOM_CIPHER ciphers (i.e. legacy EVP_CIPHER AES-GCM) have a completely different return value setup than the normal ones which are the standard one/zero. (Except that they never return zero; see TODO.) Fix checks in ssl/ and remove remnants of EVP_CIPH_FLAG_CUSTOM_CIPHER in ssl/ as we're using EVP_AEAD now. See CHANGES entry added in upstream's 3da0ca796cae6625bd26418afe0a1dc47bf5a77f. Change-Id: Ia4d0ff59b03c35fab3a08141c60b9534cb7172e2 Reviewed-on: https://boringssl-review.googlesource.com/2606 Reviewed-by: Adam Langley --- include/openssl/cipher.h | 18 ++++++++++++++---- ssl/s3_enc.c | 2 +- ssl/t1_enc.c | 7 ++----- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/include/openssl/cipher.h b/include/openssl/cipher.h index 021c6821..9c498e8c 100644 --- a/include/openssl/cipher.h +++ b/include/openssl/cipher.h @@ -80,11 +80,14 @@ OPENSSL_EXPORT const EVP_CIPHER *EVP_des_ede3_cbc(void); OPENSSL_EXPORT const EVP_CIPHER *EVP_aes_128_ecb(void); OPENSSL_EXPORT const EVP_CIPHER *EVP_aes_128_cbc(void); OPENSSL_EXPORT const EVP_CIPHER *EVP_aes_128_ctr(void); -OPENSSL_EXPORT const EVP_CIPHER *EVP_aes_128_gcm(void); OPENSSL_EXPORT const EVP_CIPHER *EVP_aes_256_ecb(void); OPENSSL_EXPORT const EVP_CIPHER *EVP_aes_256_cbc(void); OPENSSL_EXPORT const EVP_CIPHER *EVP_aes_256_ctr(void); + +/* Deprecated AES-GCM implementations that set |EVP_CIPH_FLAG_CUSTOM_CIPHER|. + * Use |EVP_aead_aes_128_gcm| and |EVP_aead_aes_256_gcm| instead. */ +OPENSSL_EXPORT const EVP_CIPHER *EVP_aes_128_gcm(void); OPENSSL_EXPORT const EVP_CIPHER *EVP_aes_256_gcm(void); /* EVP_enc_null returns a 'cipher' that passes plaintext through as @@ -190,10 +193,17 @@ OPENSSL_EXPORT int EVP_DecryptFinal_ex(EVP_CIPHER_CTX *ctx, unsigned char *out, int *out_len); /* EVP_Cipher performs a one-shot encryption/decryption operation. No partial - * blocks etc are maintained between calls. It returns the number of bytes - * written or -1 on error. + * blocks etc are maintained between calls. It returns one on success and zero + * otherwise, unless |EVP_CIPHER_flags| has |EVP_CIPH_FLAG_CUSTOM_CIPHER| + * set. Then it returns the number of bytes written or -1 on error. + * + * WARNING: this differs from the usual return value convention when using + * |EVP_CIPH_FLAG_CUSTOM_CIPHER|. * - * WARNING: this differs from the usual return value convention. */ + * TODO(davidben): The normal ciphers currently never fail, even if, e.g., + * |in_len| is not a multiple of the block size for CBC-mode decryption. The + * input just gets rounded up while the output gets truncated. This should + * either be officially documented or fail. */ OPENSSL_EXPORT int EVP_Cipher(EVP_CIPHER_CTX *ctx, uint8_t *out, const uint8_t *in, size_t in_len); diff --git a/ssl/s3_enc.c b/ssl/s3_enc.c index 69d274d4..d4789251 100644 --- a/ssl/s3_enc.c +++ b/ssl/s3_enc.c @@ -469,7 +469,7 @@ int ssl3_enc(SSL *s, int send) /* otherwise, rec->length >= bs */ } - if (EVP_Cipher(ds,rec->data, rec->input, l) < 0) + if (!EVP_Cipher(ds, rec->data, rec->input, l)) return -1; if (EVP_MD_CTX_md(s->read_hash) != NULL) diff --git a/ssl/t1_enc.c b/ssl/t1_enc.c index af8dc831..f5a4b9f6 100644 --- a/ssl/t1_enc.c +++ b/ssl/t1_enc.c @@ -941,11 +941,8 @@ int tls1_enc(SSL *s, int send) return 0; } - i = EVP_Cipher(ds,rec->data,rec->input,l); - if ((EVP_CIPHER_flags(ds->cipher)&EVP_CIPH_FLAG_CUSTOM_CIPHER) - ?(i<0) - :(i==0)) - return -1; /* AEAD can fail to verify MAC */ + if (!EVP_Cipher(ds, rec->data, rec->input, l)) + return -1; ret = 1; if (EVP_MD_CTX_md(s->read_hash) != NULL)