From 435d88d70813825533c8789faa71c6287e0d43c9 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Fri, 23 Sep 2022 16:53:23 +0100 Subject: [PATCH] Use the configured max_send_fragment value in the write record layer Reviewed-by: Hugo Landau Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/19343) --- ssl/record/methods/dtls_meth.c | 8 +++--- ssl/record/methods/ktls_meth.c | 13 ++------- ssl/record/methods/recmethod_local.h | 6 +++- ssl/record/methods/tls_common.c | 42 ++++++++++++++++++---------- ssl/record/rec_layer_s3.c | 13 +++++++-- ssl/record/recordmethod.h | 7 +++++ ssl/ssl_lib.c | 1 + ssl/statem/extensions.c | 12 +++++--- 8 files changed, 67 insertions(+), 35 deletions(-) diff --git a/ssl/record/methods/dtls_meth.c b/ssl/record/methods/dtls_meth.c index 7dcf984aed..3349d16cff 100644 --- a/ssl/record/methods/dtls_meth.c +++ b/ssl/record/methods/dtls_meth.c @@ -253,7 +253,7 @@ static int dtls_process_record(OSSL_RECORD_LAYER *rl, DTLS_BITMAP *bitmap) * Check if the received packet overflows the current Max Fragment * Length setting. */ - if (rl->max_frag_len > 0 && rr->length > rl->max_frag_len) { + if (rr->length > rl->max_frag_len) { RLAYERfatal(rl, SSL_AD_RECORD_OVERFLOW, SSL_R_DATA_LENGTH_TOO_LONG); goto end; } @@ -482,8 +482,7 @@ int dtls_get_more_records(OSSL_RECORD_LAYER *rl) * If received packet overflows maximum possible fragment length then * silently discard it */ - if (rl->max_frag_len > 0 - && rr->length > rl->max_frag_len + SSL3_RT_MAX_ENCRYPTED_OVERHEAD) { + if (rr->length > rl->max_frag_len + SSL3_RT_MAX_ENCRYPTED_OVERHEAD) { /* record too long, silently discard it */ rr->length = 0; rl->packet_length = 0; @@ -713,5 +712,6 @@ const OSSL_RECORD_METHOD ossl_dtls_record_method = { dtls_set_in_init, tls_get_state, tls_set_options, - tls_get_compression + tls_get_compression, + tls_set_max_frag_len }; diff --git a/ssl/record/methods/ktls_meth.c b/ssl/record/methods/ktls_meth.c index 1f44810d1d..dbf42b4d46 100644 --- a/ssl/record/methods/ktls_meth.c +++ b/ssl/record/methods/ktls_meth.c @@ -307,16 +307,8 @@ static int ktls_set_crypto_state(OSSL_RECORD_LAYER *rl, int level, return OSSL_RECORD_RETURN_NON_FATAL_ERR; /* ktls supports only the maximum fragment size */ - if (rl->max_frag_len > 0 && rl->max_frag_len != SSL3_RT_MAX_PLAIN_LENGTH) + if (rl->max_frag_len != SSL3_RT_MAX_PLAIN_LENGTH) return OSSL_RECORD_RETURN_NON_FATAL_ERR; -#if 0 - /* - * TODO(RECLAYER): We will need to reintroduce the check of the send - * fragment for KTLS once we do the record write side implementation - */ - if (ssl_get_max_send_fragment(s) != SSL3_RT_MAX_PLAIN_LENGTH) - return OSSL_RECORD_RETURN_NON_FATAL_ERR; -#endif /* check that cipher is supported */ if (!ktls_int_check_supported_cipher(rl, ciph, md, taglen)) @@ -476,5 +468,6 @@ const OSSL_RECORD_METHOD ossl_ktls_record_method = { NULL, tls_get_state, tls_set_options, - tls_get_compression + tls_get_compression, + tls_set_max_frag_len }; diff --git a/ssl/record/methods/recmethod_local.h b/ssl/record/methods/recmethod_local.h index 27461ff645..eda50050e2 100644 --- a/ssl/record/methods/recmethod_local.h +++ b/ssl/record/methods/recmethod_local.h @@ -173,7 +173,10 @@ struct ossl_record_layer_st /* Set to 1 if this is the first handshake. 0 otherwise */ int is_first_handshake; - /* The negotiated maximum fragment length */ + /* + * The smaller of the configured and negotiated maximum fragment length + * or SSL3_RT_MAX_PLAIN_LENGTH if none + */ unsigned int max_frag_len; /* The maxium amount of early data we can receive/send */ @@ -329,6 +332,7 @@ void tls_get_state(OSSL_RECORD_LAYER *rl, const char **shortstr, const char **longstr); int tls_set_options(OSSL_RECORD_LAYER *rl, const OSSL_PARAM *options); const COMP_METHOD *tls_get_compression(OSSL_RECORD_LAYER *rl); +void tls_set_max_frag_len(OSSL_RECORD_LAYER *rl, size_t max_frag_len); int tls_setup_read_buffer(OSSL_RECORD_LAYER *rl); int tls_setup_write_buffer(OSSL_RECORD_LAYER *rl, size_t numwpipes, size_t firstlen, size_t nextlen); diff --git a/ssl/record/methods/tls_common.c b/ssl/record/methods/tls_common.c index 1c44d3f688..50132d7771 100644 --- a/ssl/record/methods/tls_common.c +++ b/ssl/record/methods/tls_common.c @@ -144,8 +144,6 @@ int tls_setup_write_buffer(OSSL_RECORD_LAYER *rl, size_t numwpipes, size_t align = 0, headerlen; SSL3_BUFFER *wb; size_t currpipe; - /* TODO(RECLAYER): Remove me */ - SSL_CONNECTION *s = (SSL_CONNECTION *)rl->cbarg; size_t defltlen = 0; if (firstlen == 0 || (numwpipes > 1 && nextlen == 0)) { @@ -158,8 +156,8 @@ int tls_setup_write_buffer(OSSL_RECORD_LAYER *rl, size_t numwpipes, align = SSL3_ALIGN_PAYLOAD - 1; #endif - defltlen = ssl_get_max_send_fragment(s) - + SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD + headerlen + align; + defltlen = rl->max_frag_len + SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD + + headerlen + align; #ifndef OPENSSL_NO_COMP if (tls_allow_compression(rl)) defltlen += SSL3_RT_MAX_COMPRESSED_OVERHEAD; @@ -237,8 +235,8 @@ int tls_setup_read_buffer(OSSL_RECORD_LAYER *rl) #endif if (b->buf == NULL) { - len = SSL3_RT_MAX_PLAIN_LENGTH - + SSL3_RT_MAX_ENCRYPTED_OVERHEAD + headerlen + align; + len = rl->max_frag_len + + SSL3_RT_MAX_ENCRYPTED_OVERHEAD + headerlen + align; #ifndef OPENSSL_NO_COMP if (tls_allow_compression(rl)) len += SSL3_RT_MAX_COMPRESSED_OVERHEAD; @@ -905,7 +903,7 @@ int tls_get_more_records(OSSL_RECORD_LAYER *rl) * Max Fragment Length setting. * Note: rl->max_frag_len > 0 and KTLS are mutually exclusive. */ - if (rl->max_frag_len > 0 && thisrr->length > rl->max_frag_len) { + if (thisrr->length > rl->max_frag_len) { RLAYERfatal(rl, SSL_AD_RECORD_OVERFLOW, SSL_R_DATA_LENGTH_TOO_LONG); goto end; } @@ -1214,6 +1212,12 @@ tls_int_new_record_layer(OSSL_LIB_CTX *libctx, const char *propq, int vers, if (rl == NULL) return OSSL_RECORD_RETURN_FATAL; + /* + * Default the value for max_frag_len. This may be overridden by the + * settings + */ + rl->max_frag_len = SSL3_RT_MAX_PLAIN_LENGTH; + /* Loop through all the settings since they must all be understood */ if (settings != NULL) { for (p = settings; p->key != NULL; p++) { @@ -1512,8 +1516,6 @@ int tls_write_records_default(OSSL_RECORD_LAYER *rl, size_t len, wpinited = 0; size_t j, prefix = 0; int using_ktls; - /* TODO(RECLAYER): REMOVE ME */ - SSL_CONNECTION *s = rl->cbarg; OSSL_RECORD_TEMPLATE prefixtempl; OSSL_RECORD_TEMPLATE *thistempl; @@ -1688,7 +1690,7 @@ int tls_write_records_default(OSSL_RECORD_LAYER *rl, && rl->enc_ctx != NULL && (!rl->allow_plain_alerts || thistempl->type != SSL3_RT_ALERT)) { - size_t rlen, max_send_fragment; + size_t rlen; if (!WPACKET_put_bytes_u8(thispkt, thistempl->type)) { RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); @@ -1697,11 +1699,10 @@ int tls_write_records_default(OSSL_RECORD_LAYER *rl, SSL3_RECORD_add_length(thiswr, 1); /* Add TLS1.3 padding */ - max_send_fragment = ssl_get_max_send_fragment(s); rlen = SSL3_RECORD_get_length(thiswr); - if (rlen < max_send_fragment) { + if (rlen < rl->max_frag_len) { size_t padding = 0; - size_t max_padding = max_send_fragment - rlen; + size_t max_padding = rl->max_frag_len - rlen; if (rl->padding != NULL) { padding = rl->padding(rl->cbarg, thistempl->type, rlen); @@ -2063,6 +2064,18 @@ const COMP_METHOD *tls_get_compression(OSSL_RECORD_LAYER *rl) #endif } +void tls_set_max_frag_len(OSSL_RECORD_LAYER *rl, size_t max_frag_len) +{ + rl->max_frag_len = max_frag_len; + /* + * We don't need to adjust buffer sizes. Write buffer sizes are + * automatically checked anyway. We should only be changing the read buffer + * size during the handshake, so we will create a new buffer when we create + * the new record layer. We can't change the existing buffer because it may + * already have data in it. + */ +} + const OSSL_RECORD_METHOD ossl_tls_record_method = { tls_new_record_layer, tls_free, @@ -2086,5 +2099,6 @@ const OSSL_RECORD_METHOD ossl_tls_record_method = { NULL, tls_get_state, tls_set_options, - tls_get_compression + tls_get_compression, + tls_set_max_frag_len }; diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 7d81a02111..d77acde232 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -1135,7 +1135,9 @@ int ssl_set_new_record_layer(SSL_CONNECTION *s, int version, SSL_CTX *sctx = SSL_CONNECTION_GET_CTX(s); const OSSL_RECORD_METHOD *meth; int use_etm, stream_mac = 0, tlstree = 0; - unsigned int maxfrag = SSL3_RT_MAX_PLAIN_LENGTH; + unsigned int maxfrag = (direction == OSSL_RECORD_DIRECTION_WRITE) + ? ssl_get_max_send_fragment(s) + : SSL3_RT_MAX_PLAIN_LENGTH; int use_early_data = 0; uint32_t max_early_data; COMP_METHOD *compm = (comp == NULL) ? NULL : comp->method; @@ -1205,9 +1207,16 @@ int ssl_set_new_record_layer(SSL_CONNECTION *s, int version, *set++ = OSSL_PARAM_construct_int(OSSL_LIBSSL_RECORD_LAYER_PARAM_TLSTREE, &tlstree); - if (s->session != NULL && USE_MAX_FRAGMENT_LENGTH_EXT(s->session)) + /* + * We only need to do this for the read side. The write side should already + * have the correct value due to the ssl_get_max_send_fragment() call above + */ + if (direction == OSSL_RECORD_DIRECTION_READ + && s->session != NULL + && USE_MAX_FRAGMENT_LENGTH_EXT(s->session)) maxfrag = GET_MAX_FRAGMENT_LENGTH(s->session); + if (maxfrag != SSL3_RT_MAX_PLAIN_LENGTH) *set++ = OSSL_PARAM_construct_uint(OSSL_LIBSSL_RECORD_LAYER_PARAM_MAX_FRAG_LEN, &maxfrag); diff --git a/ssl/record/recordmethod.h b/ssl/record/recordmethod.h index 6c84737a7c..a60b2470fa 100644 --- a/ssl/record/recordmethod.h +++ b/ssl/record/recordmethod.h @@ -302,6 +302,13 @@ struct ossl_record_method_st { int (*set_options)(OSSL_RECORD_LAYER *rl, const OSSL_PARAM *options); const COMP_METHOD *(*get_compression)(OSSL_RECORD_LAYER *rl); + + /* + * Set the maximum fragment length to be used for the record layer. This + * will override any previous value supplied for the "max_frag_len" + * setting during construction of the record layer. + */ + void (*set_max_frag_len)(OSSL_RECORD_LAYER *rl, size_t max_frag_len); }; diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 8d252d82bc..fb43b9b369 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -2789,6 +2789,7 @@ long SSL_ctrl(SSL *s, int cmd, long larg, void *parg) sc->max_send_fragment = larg; if (sc->max_send_fragment < sc->split_send_fragment) sc->split_send_fragment = sc->max_send_fragment; + sc->rlayer.wrlmethod->set_max_frag_len(sc->rlayer.wrl, larg); return 1; case SSL_CTRL_SET_SPLIT_SEND_FRAGMENT: if ((size_t)larg > sc->max_send_fragment || larg == 0) diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 47ad5110ab..880189d998 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -1707,14 +1707,18 @@ static int final_maxfragmentlen(SSL_CONNECTION *s, unsigned int context, return 0; } - /* Current SSL buffer is lower than requested MFL */ - if (s->session && USE_MAX_FRAGMENT_LENGTH_EXT(s->session) - && s->max_send_fragment < GET_MAX_FRAGMENT_LENGTH(s->session)) + if (s->session && USE_MAX_FRAGMENT_LENGTH_EXT(s->session)) { + s->rlayer.rrlmethod->set_max_frag_len(s->rlayer.rrl, + GET_MAX_FRAGMENT_LENGTH(s->session)); + s->rlayer.wrlmethod->set_max_frag_len(s->rlayer.wrl, + ssl_get_max_send_fragment(s)); /* trigger a larger buffer reallocation */ - if (!ssl3_setup_buffers(s)) { + /* TODO(RECLAYER): Remove me when DTLS moved to write record layer */ + if (SSL_CONNECTION_IS_DTLS(s) && !ssl3_setup_buffers(s)) { /* SSLfatal() already called */ return 0; } + } return 1; }