From 51ccad3f40e5f000da8364b1bb4bddd41657c96e Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Wed, 20 Jul 2022 15:15:32 +0100 Subject: [PATCH] Resolve a TODO(RECLAYER) in the SSLv3 code We remove some code outside of the record layer which is no longer relevant since its functions are now performed by the new record layer code. This removes a TODO(RECLAYER) as a result. Reviewed-by: Hugo Landau Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/18132) --- ssl/record/ssl3_record.c | 155 +++++++++++---------------------------- 1 file changed, 43 insertions(+), 112 deletions(-) diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c index fac317a4ac6..5b3d48464c3 100644 --- a/ssl/record/ssl3_record.c +++ b/ssl/record/ssl3_record.c @@ -7,6 +7,7 @@ * https://www.openssl.org/source/license.html */ +#include #include "../ssl_local.h" #include #include @@ -611,21 +612,6 @@ int tls1_enc(SSL_CONNECTION *s, SSL3_RECORD *recs, size_t n_recs, int sending, return 1; } -/* - * TODO(RECLAYER): Remove me: now declared in - * ssl/record/methods/recmethod_local.h - */ -char ssl3_cbc_record_digest_supported(const EVP_MD_CTX *ctx); -int ssl3_cbc_digest_record(const EVP_MD *md, - unsigned char *md_out, - size_t *md_out_size, - const unsigned char *header, - const unsigned char *data, - size_t data_size, - size_t data_plus_mac_plus_padding_size, - const unsigned char *mac_secret, - size_t mac_secret_length, char is_sslv3); - int n_ssl3_mac(SSL_CONNECTION *sc, SSL3_RECORD *rec, unsigned char *md, int sending) { @@ -635,16 +621,18 @@ int n_ssl3_mac(SSL_CONNECTION *sc, SSL3_RECORD *rec, unsigned char *md, size_t md_size; size_t npad; int t; + unsigned int md_size_u; + EVP_MD_CTX *md_ctx; - if (sending) { - mac_sec = &(sc->s3.write_mac_secret[0]); - seq = RECORD_LAYER_get_write_sequence(&sc->rlayer); - hash = sc->write_hash; - } else { - mac_sec = &(sc->s3.read_mac_secret[0]); - seq = RECORD_LAYER_get_read_sequence(&sc->rlayer); - hash = sc->read_hash; - } + /* + * All read record layer operations should have been moved to the new + * record layer code + */ + assert(sending); + + mac_sec = &(sc->s3.write_mac_secret[0]); + seq = RECORD_LAYER_get_write_sequence(&sc->rlayer); + hash = sc->write_hash; t = EVP_MD_CTX_get_size(hash); if (t < 0) @@ -652,77 +640,34 @@ int n_ssl3_mac(SSL_CONNECTION *sc, SSL3_RECORD *rec, unsigned char *md, md_size = t; npad = (48 / md_size) * md_size; - if (!sending - && EVP_CIPHER_CTX_get_mode(sc->enc_read_ctx) == EVP_CIPH_CBC_MODE - && ssl3_cbc_record_digest_supported(hash)) { -#ifdef OPENSSL_NO_DEPRECATED_3_0 - return 0; -#else - /* - * This is a CBC-encrypted record. We must avoid leaking any - * timing-side channel information about how many blocks of data we - * are hashing because that gives an attacker a timing-oracle. - */ - - /*- - * npad is, at most, 48 bytes and that's with MD5: - * 16 + 48 + 8 (sequence bytes) + 1 + 2 = 75. - * - * With SHA-1 (the largest hash speced for SSLv3) the hash size - * goes up 4, but npad goes down by 8, resulting in a smaller - * total size. - */ - unsigned char header[75]; - size_t j = 0; - memcpy(header + j, mac_sec, md_size); - j += md_size; - memcpy(header + j, ssl3_pad_1, npad); - j += npad; - memcpy(header + j, seq, 8); - j += 8; - header[j++] = rec->type; - header[j++] = (unsigned char)(rec->length >> 8); - header[j++] = (unsigned char)(rec->length & 0xff); - - /* Final param == is SSLv3 */ - if (ssl3_cbc_digest_record(EVP_MD_CTX_get0_md(hash), - md, &md_size, - header, rec->input, - rec->length, rec->orig_len, - mac_sec, md_size, 1) <= 0) - return 0; -#endif - } else { - unsigned int md_size_u; - /* Chop the digest off the end :-) */ - EVP_MD_CTX *md_ctx = EVP_MD_CTX_new(); - - if (md_ctx == NULL) - return 0; + /* Chop the digest off the end :-) */ + md_ctx = EVP_MD_CTX_new(); - rec_char = rec->type; - p = md; - s2n(rec->length, p); - if (EVP_MD_CTX_copy_ex(md_ctx, hash) <= 0 - || EVP_DigestUpdate(md_ctx, mac_sec, md_size) <= 0 - || EVP_DigestUpdate(md_ctx, ssl3_pad_1, npad) <= 0 - || EVP_DigestUpdate(md_ctx, seq, 8) <= 0 - || EVP_DigestUpdate(md_ctx, &rec_char, 1) <= 0 - || EVP_DigestUpdate(md_ctx, md, 2) <= 0 - || EVP_DigestUpdate(md_ctx, rec->input, rec->length) <= 0 - || EVP_DigestFinal_ex(md_ctx, md, NULL) <= 0 - || EVP_MD_CTX_copy_ex(md_ctx, hash) <= 0 - || EVP_DigestUpdate(md_ctx, mac_sec, md_size) <= 0 - || EVP_DigestUpdate(md_ctx, ssl3_pad_2, npad) <= 0 - || EVP_DigestUpdate(md_ctx, md, md_size) <= 0 - || EVP_DigestFinal_ex(md_ctx, md, &md_size_u) <= 0) { - EVP_MD_CTX_free(md_ctx); - return 0; - } + if (md_ctx == NULL) + return 0; + rec_char = rec->type; + p = md; + s2n(rec->length, p); + if (EVP_MD_CTX_copy_ex(md_ctx, hash) <= 0 + || EVP_DigestUpdate(md_ctx, mac_sec, md_size) <= 0 + || EVP_DigestUpdate(md_ctx, ssl3_pad_1, npad) <= 0 + || EVP_DigestUpdate(md_ctx, seq, 8) <= 0 + || EVP_DigestUpdate(md_ctx, &rec_char, 1) <= 0 + || EVP_DigestUpdate(md_ctx, md, 2) <= 0 + || EVP_DigestUpdate(md_ctx, rec->input, rec->length) <= 0 + || EVP_DigestFinal_ex(md_ctx, md, NULL) <= 0 + || EVP_MD_CTX_copy_ex(md_ctx, hash) <= 0 + || EVP_DigestUpdate(md_ctx, mac_sec, md_size) <= 0 + || EVP_DigestUpdate(md_ctx, ssl3_pad_2, npad) <= 0 + || EVP_DigestUpdate(md_ctx, md, md_size) <= 0 + || EVP_DigestFinal_ex(md_ctx, md, &md_size_u) <= 0) { EVP_MD_CTX_free(md_ctx); + return 0; } + EVP_MD_CTX_free(md_ctx); + ssl3_record_sequence_update(seq); return 1; } @@ -743,13 +688,14 @@ int tls1_mac_old(SSL_CONNECTION *sc, SSL3_RECORD *rec, unsigned char *md, int t; int ret = 0; - if (sending) { - seq = RECORD_LAYER_get_write_sequence(&sc->rlayer); - hash = sc->write_hash; - } else { - seq = RECORD_LAYER_get_read_sequence(&sc->rlayer); - hash = sc->read_hash; - } + /* + * All read record layer calls should have been moved to the new record + * layer. + */ + assert(sending); + + seq = RECORD_LAYER_get_write_sequence(&sc->rlayer); + hash = sc->write_hash; t = EVP_MD_CTX_get_size(hash); if (!ossl_assert(t >= 0)) @@ -789,21 +735,6 @@ int tls1_mac_old(SSL_CONNECTION *sc, SSL3_RECORD *rec, unsigned char *md, header[11] = (unsigned char)(rec->length >> 8); header[12] = (unsigned char)(rec->length & 0xff); - if (!sending && !SSL_READ_ETM(sc) - && EVP_CIPHER_CTX_get_mode(sc->enc_read_ctx) == EVP_CIPH_CBC_MODE - && ssl3_cbc_record_digest_supported(mac_ctx)) { - OSSL_PARAM tls_hmac_params[2], *p = tls_hmac_params; - - *p++ = OSSL_PARAM_construct_size_t(OSSL_MAC_PARAM_TLS_DATA_SIZE, - &rec->orig_len); - *p++ = OSSL_PARAM_construct_end(); - - if (!EVP_PKEY_CTX_set_params(EVP_MD_CTX_get_pkey_ctx(mac_ctx), - tls_hmac_params)) { - goto end; - } - } - if (EVP_DigestSignUpdate(mac_ctx, header, sizeof(header)) <= 0 || EVP_DigestSignUpdate(mac_ctx, rec->input, rec->length) <= 0 || EVP_DigestSignFinal(mac_ctx, md, &md_size) <= 0) { -- 2.47.2