From 2582de25902510cdb934c5ff59845fc26a7f2e28 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Tue, 27 Sep 2022 15:32:22 +0100 Subject: [PATCH] Move record padding out of tls_common.c Only tls13_meth.c needs to handle adding record padding. All other *_meth.c files can ignore it. Reviewed-by: Hugo Landau Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/19343) --- ssl/record/methods/ktls_meth.c | 3 +- ssl/record/methods/recmethod_local.h | 5 +++ ssl/record/methods/ssl3_meth.c | 3 +- ssl/record/methods/tls13_meth.c | 59 +++++++++++++++++++++++++++- ssl/record/methods/tls1_meth.c | 4 +- ssl/record/methods/tls_common.c | 55 +++----------------------- ssl/record/methods/tlsany_meth.c | 4 +- 7 files changed, 79 insertions(+), 54 deletions(-) diff --git a/ssl/record/methods/ktls_meth.c b/ssl/record/methods/ktls_meth.c index 60fd80465d7..f73334604ad 100644 --- a/ssl/record/methods/ktls_meth.c +++ b/ssl/record/methods/ktls_meth.c @@ -506,7 +506,8 @@ static struct record_functions_st ossl_ktls_funcs = { ktls_allocate_write_buffers, ktls_initialise_write_packets, NULL, - ktls_prepare_record_header + ktls_prepare_record_header, + NULL }; const OSSL_RECORD_METHOD ossl_ktls_record_method = { diff --git a/ssl/record/methods/recmethod_local.h b/ssl/record/methods/recmethod_local.h index d7c526c2d59..0a4c97a29d0 100644 --- a/ssl/record/methods/recmethod_local.h +++ b/ssl/record/methods/recmethod_local.h @@ -105,6 +105,11 @@ struct record_functions_st OSSL_RECORD_TEMPLATE *templ, unsigned int rectype, unsigned char **recdata); + + int (*add_record_padding)(OSSL_RECORD_LAYER *rl, + OSSL_RECORD_TEMPLATE *thistempl, + WPACKET *thispkt, + SSL3_RECORD *thiswr); }; struct ossl_record_layer_st diff --git a/ssl/record/methods/ssl3_meth.c b/ssl/record/methods/ssl3_meth.c index 6803cb5975c..544d4d07e04 100644 --- a/ssl/record/methods/ssl3_meth.c +++ b/ssl/record/methods/ssl3_meth.c @@ -315,5 +315,6 @@ struct record_functions_st ssl_3_0_funcs = { tls1_allocate_write_buffers, tls1_initialise_write_packets, NULL, - tls_prepare_record_header_default + tls_prepare_record_header_default, + NULL }; diff --git a/ssl/record/methods/tls13_meth.c b/ssl/record/methods/tls13_meth.c index b388ab86faf..4b0142391b6 100644 --- a/ssl/record/methods/tls13_meth.c +++ b/ssl/record/methods/tls13_meth.c @@ -253,6 +253,62 @@ static unsigned int tls13_get_record_type(OSSL_RECORD_LAYER *rl, return SSL3_RT_APPLICATION_DATA; } +static int tls13_add_record_padding(OSSL_RECORD_LAYER *rl, + OSSL_RECORD_TEMPLATE *thistempl, + WPACKET *thispkt, + SSL3_RECORD *thiswr) +{ + size_t rlen; + + /* Nothing to be done in the case of a plaintext alert */ + if (rl->allow_plain_alerts && thistempl->type != SSL3_RT_ALERT) + return 1; + + if (!WPACKET_put_bytes_u8(thispkt, thistempl->type)) { + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + return 0; + } + SSL3_RECORD_add_length(thiswr, 1); + + /* Add TLS1.3 padding */ + rlen = SSL3_RECORD_get_length(thiswr); + if (rlen < rl->max_frag_len) { + size_t padding = 0; + size_t max_padding = rl->max_frag_len - rlen; + + if (rl->padding != NULL) { + padding = rl->padding(rl->cbarg, thistempl->type, rlen); + } else if (rl->block_padding > 0) { + size_t mask = rl->block_padding - 1; + size_t remainder; + + /* optimize for power of 2 */ + if ((rl->block_padding & mask) == 0) + remainder = rlen & mask; + else + remainder = rlen % rl->block_padding; + /* don't want to add a block of padding if we don't have to */ + if (remainder == 0) + padding = 0; + else + padding = rl->block_padding - remainder; + } + if (padding > 0) { + /* do not allow the record to exceed max plaintext length */ + if (padding > max_padding) + padding = max_padding; + if (!WPACKET_memset(thispkt, 0, padding)) { + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, + ERR_R_INTERNAL_ERROR); + return 0; + } + SSL3_RECORD_add_length(thiswr, padding); + } + } + + return 1; +} + struct record_functions_st tls_1_3_funcs = { tls13_set_crypto_state, tls13_cipher, @@ -267,5 +323,6 @@ struct record_functions_st tls_1_3_funcs = { tls_allocate_write_buffers_default, tls_initialise_write_packets_default, tls13_get_record_type, - tls_prepare_record_header_default + tls_prepare_record_header_default, + tls13_add_record_padding }; diff --git a/ssl/record/methods/tls1_meth.c b/ssl/record/methods/tls1_meth.c index 56d015f71fa..b02c53d6494 100644 --- a/ssl/record/methods/tls1_meth.c +++ b/ssl/record/methods/tls1_meth.c @@ -656,7 +656,8 @@ struct record_functions_st tls_1_funcs = { tls1_allocate_write_buffers, tls1_initialise_write_packets, NULL, - tls_prepare_record_header_default + tls_prepare_record_header_default, + NULL }; struct record_functions_st dtls_1_funcs = { @@ -672,5 +673,6 @@ struct record_functions_st dtls_1_funcs = { NULL, NULL, NULL, + NULL, NULL }; diff --git a/ssl/record/methods/tls_common.c b/ssl/record/methods/tls_common.c index 7618caa2a6f..e3be4303367 100644 --- a/ssl/record/methods/tls_common.c +++ b/ssl/record/methods/tls_common.c @@ -1628,7 +1628,7 @@ int tls_write_records_default(OSSL_RECORD_LAYER *rl, if (!rl->funcs->prepare_record_header(rl, thispkt, thistempl, rectype, &compressdata)) { - RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + /* RLAYERfatal() already called */ goto err; } @@ -1658,54 +1658,11 @@ int tls_write_records_default(OSSL_RECORD_LAYER *rl, SSL3_RECORD_reset_input(&wr[j]); } - if (rl->version == TLS1_3_VERSION - && !using_ktls - && rl->enc_ctx != NULL - && (!rl->allow_plain_alerts - || thistempl->type != SSL3_RT_ALERT)) { - size_t rlen; - - if (!WPACKET_put_bytes_u8(thispkt, thistempl->type)) { - RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); - goto err; - } - SSL3_RECORD_add_length(thiswr, 1); - - /* Add TLS1.3 padding */ - rlen = SSL3_RECORD_get_length(thiswr); - if (rlen < rl->max_frag_len) { - size_t padding = 0; - size_t max_padding = rl->max_frag_len - rlen; - - if (rl->padding != NULL) { - padding = rl->padding(rl->cbarg, thistempl->type, rlen); - } else if (rl->block_padding > 0) { - size_t mask = rl->block_padding - 1; - size_t remainder; - - /* optimize for power of 2 */ - if ((rl->block_padding & mask) == 0) - remainder = rlen & mask; - else - remainder = rlen % rl->block_padding; - /* don't want to add a block of padding if we don't have to */ - if (remainder == 0) - padding = 0; - else - padding = rl->block_padding - remainder; - } - if (padding > 0) { - /* do not allow the record to exceed max plaintext length */ - if (padding > max_padding) - padding = max_padding; - if (!WPACKET_memset(thispkt, 0, padding)) { - RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, - ERR_R_INTERNAL_ERROR); - goto err; - } - SSL3_RECORD_add_length(thiswr, padding); - } - } + if (rl->funcs->add_record_padding != NULL + && !rl->funcs->add_record_padding(rl, thistempl, thispkt, + thiswr)) { + /* RLAYERfatal() already called */ + goto err; } /* diff --git a/ssl/record/methods/tlsany_meth.c b/ssl/record/methods/tlsany_meth.c index eaabcf20a94..8d4486547c1 100644 --- a/ssl/record/methods/tlsany_meth.c +++ b/ssl/record/methods/tlsany_meth.c @@ -148,7 +148,8 @@ struct record_functions_st tls_any_funcs = { tls_allocate_write_buffers_default, tls_initialise_write_packets_default, NULL, - tls_prepare_record_header_default + tls_prepare_record_header_default, + NULL }; static int dtls_any_set_protocol_version(OSSL_RECORD_LAYER *rl, int vers) @@ -174,5 +175,6 @@ struct record_functions_st dtls_any_funcs = { NULL, NULL, NULL, + NULL, NULL }; -- 2.47.3