From 4f428e86d8bc68f95446eef96129c6ad98b57104 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Fri, 14 Oct 2022 15:30:55 +0100 Subject: [PATCH] Ensure the record layer is responsible for calculating record overheads Don't calculate the potential record layer expansion outside of the record layer. We move some code that was doing that into the record layer. Reviewed-by: Richard Levitte Reviewed-by: Tomas Mraz Reviewed-by: Hugo Landau (Merged from https://github.com/openssl/openssl/pull/19424) --- ssl/record/methods/dtls_meth.c | 32 +++++++++++++++++++++++++++++++- ssl/record/methods/ktls_meth.c | 3 ++- ssl/record/methods/tls_common.c | 3 ++- ssl/record/recordmethod.h | 6 ++++++ ssl/statem/statem_dtls.c | 26 +++++--------------------- 5 files changed, 46 insertions(+), 24 deletions(-) diff --git a/ssl/record/methods/dtls_meth.c b/ssl/record/methods/dtls_meth.c index e6c71ed1e7..d219d7d02e 100644 --- a/ssl/record/methods/dtls_meth.c +++ b/ssl/record/methods/dtls_meth.c @@ -823,6 +823,35 @@ int dtls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, return ret; } +static size_t dtls_get_max_record_overhead(OSSL_RECORD_LAYER *rl) +{ + size_t blocksize, mac_size; + + /* + * TODO(RECLAYER): Review this. This is what the existing code did. + * I suspect it's not quite right. What about IV? AEAD Tag? Compression + * expansion? + */ + if (rl->md_ctx != NULL) { + if (rl->enc_ctx != NULL + && (EVP_CIPHER_get_flags(EVP_CIPHER_CTX_get0_cipher(rl->enc_ctx)) & + EVP_CIPH_FLAG_AEAD_CIPHER) != 0) + mac_size = 0; + else + mac_size = EVP_MD_CTX_get_size(rl->md_ctx); + } else { + mac_size = 0; + } + + if (rl->enc_ctx != NULL && + (EVP_CIPHER_CTX_get_mode(rl->enc_ctx) == EVP_CIPH_CBC_MODE)) + blocksize = 2 * EVP_CIPHER_CTX_get_block_size(rl->enc_ctx); + else + blocksize = 0; + + return DTLS1_RT_HEADER_LENGTH + mac_size + blocksize; +} + const OSSL_RECORD_METHOD ossl_dtls_record_method = { dtls_new_record_layer, dtls_free, @@ -847,5 +876,6 @@ const OSSL_RECORD_METHOD ossl_dtls_record_method = { tls_get_state, tls_set_options, tls_get_compression, - tls_set_max_frag_len + tls_set_max_frag_len, + dtls_get_max_record_overhead }; diff --git a/ssl/record/methods/ktls_meth.c b/ssl/record/methods/ktls_meth.c index 14db12ad5b..ba8e2a4523 100644 --- a/ssl/record/methods/ktls_meth.c +++ b/ssl/record/methods/ktls_meth.c @@ -578,5 +578,6 @@ const OSSL_RECORD_METHOD ossl_ktls_record_method = { tls_get_state, tls_set_options, tls_get_compression, - tls_set_max_frag_len + tls_set_max_frag_len, + NULL }; diff --git a/ssl/record/methods/tls_common.c b/ssl/record/methods/tls_common.c index 238684a77b..fe2620a8ea 100644 --- a/ssl/record/methods/tls_common.c +++ b/ssl/record/methods/tls_common.c @@ -2037,5 +2037,6 @@ const OSSL_RECORD_METHOD ossl_tls_record_method = { tls_get_state, tls_set_options, tls_get_compression, - tls_set_max_frag_len + tls_set_max_frag_len, + NULL }; diff --git a/ssl/record/recordmethod.h b/ssl/record/recordmethod.h index a60b2470fa..36ba59e3c2 100644 --- a/ssl/record/recordmethod.h +++ b/ssl/record/recordmethod.h @@ -309,6 +309,12 @@ struct ossl_record_method_st { * setting during construction of the record layer. */ void (*set_max_frag_len)(OSSL_RECORD_LAYER *rl, size_t max_frag_len); + + /* + * The maximum expansion in bytes that the record layer might add while + * writing a record + */ + size_t (*get_max_record_overhead)(OSSL_RECORD_LAYER *rl); }; diff --git a/ssl/statem/statem_dtls.c b/ssl/statem/statem_dtls.c index b673c860ab..2e71014ef8 100644 --- a/ssl/statem/statem_dtls.c +++ b/ssl/statem/statem_dtls.c @@ -116,7 +116,7 @@ int dtls1_do_write(SSL_CONNECTION *s, int type) size_t written; size_t curr_mtu; int retry = 1; - size_t len, frag_off, mac_size, blocksize, used_len; + size_t len, frag_off, overhead, used_len; SSL *ssl = SSL_CONNECTION_GET_SSL(s); if (!dtls1_query_mtu(s)) @@ -132,21 +132,7 @@ int dtls1_do_write(SSL_CONNECTION *s, int type) return -1; } - if (s->write_hash) { - if (s->enc_write_ctx - && (EVP_CIPHER_get_flags(EVP_CIPHER_CTX_get0_cipher(s->enc_write_ctx)) & - EVP_CIPH_FLAG_AEAD_CIPHER) != 0) - mac_size = 0; - else - mac_size = EVP_MD_CTX_get_size(s->write_hash); - } else - mac_size = 0; - - if (s->enc_write_ctx && - (EVP_CIPHER_CTX_get_mode(s->enc_write_ctx) == EVP_CIPH_CBC_MODE)) - blocksize = 2 * EVP_CIPHER_CTX_get_block_size(s->enc_write_ctx); - else - blocksize = 0; + overhead = s->rlayer.wrlmethod->get_max_record_overhead(s->rlayer.wrl); frag_off = 0; s->rwstate = SSL_NOTHING; @@ -187,8 +173,7 @@ int dtls1_do_write(SSL_CONNECTION *s, int type) } } - used_len = BIO_wpending(s->wbio) + DTLS1_RT_HEADER_LENGTH - + mac_size + blocksize; + used_len = BIO_wpending(s->wbio) + overhead; if (s->d1->mtu > used_len) curr_mtu = s->d1->mtu - used_len; else @@ -203,9 +188,8 @@ int dtls1_do_write(SSL_CONNECTION *s, int type) s->rwstate = SSL_WRITING; return ret; } - used_len = DTLS1_RT_HEADER_LENGTH + mac_size + blocksize; - if (s->d1->mtu > used_len + DTLS1_HM_HEADER_LENGTH) { - curr_mtu = s->d1->mtu - used_len; + if (s->d1->mtu > overhead + DTLS1_HM_HEADER_LENGTH) { + curr_mtu = s->d1->mtu - overhead; } else { /* Shouldn't happen */ return -1; -- 2.34.1