From: Matt Caswell Date: Fri, 26 Aug 2022 16:34:40 +0000 (+0100) Subject: Move logic for figuring out the record version out of record layer X-Git-Tag: openssl-3.2.0-alpha1~2033 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1d3676778c280ef05044c4c9e696a4f8096530ea;p=thirdparty%2Fopenssl.git Move logic for figuring out the record version out of record layer This calculation is based on lots of information from state machine and elsewhere that the record layer cannot access. In reality it is sufficient to simply tell the record layer what version to use. Reviewed-by: Hugo Landau Reviewed-by: Richard Levitte (Merged from https://github.com/openssl/openssl/pull/19198) --- diff --git a/ssl/record/methods/tls_common.c b/ssl/record/methods/tls_common.c index d56184f8162..01bfd477d92 100644 --- a/ssl/record/methods/tls_common.c +++ b/ssl/record/methods/tls_common.c @@ -7,6 +7,7 @@ * https://www.openssl.org/source/license.html */ +#include #include #include #include @@ -1467,6 +1468,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, * http://www.openssl.org/~bodo/tls-cbc.txt) */ prefixtempl.buf = NULL; + prefixtempl.version = templates[0].version; prefixtempl.buflen = 0; prefixtempl.type = SSL3_RT_APPLICATION_DATA; wpinited = 1; @@ -1554,8 +1556,6 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, /* Clear our SSL3_RECORD structures */ memset(wr, 0, sizeof(wr)); for (j = 0; j < numtempl + prefix; j++) { - unsigned int version = (s->version == TLS1_3_VERSION) ? TLS1_2_VERSION - : s->version; unsigned char *compressdata = NULL; size_t maxcomplen; unsigned int rectype; @@ -1578,17 +1578,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, rectype = thistempl->type; SSL3_RECORD_set_type(thiswr, rectype); - - /* - * Some servers hang if initial client hello is larger than 256 bytes - * and record version number > TLS 1.0 - */ - if (SSL_get_state(ssl) == TLS_ST_CW_CLNT_HELLO - && !s->renegotiate - && TLS1_get_version(ssl) > TLS1_VERSION - && s->hello_retry_request == SSL_HRR_NONE) - version = TLS1_VERSION; - SSL3_RECORD_set_rec_version(thiswr, version); + SSL3_RECORD_set_rec_version(thiswr, thistempl->version); maxcomplen = thistempl->buflen; if (s->compress != NULL) @@ -1600,7 +1590,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, */ if (!using_ktls && (!WPACKET_put_bytes_u8(thispkt, rectype) - || !WPACKET_put_bytes_u16(thispkt, version) + || !WPACKET_put_bytes_u16(thispkt, thistempl->version) || !WPACKET_start_sub_packet_u16(thispkt) || (eivlen > 0 && !WPACKET_allocate_bytes(thispkt, eivlen, NULL)) @@ -1916,7 +1906,6 @@ int tls_retry_write_records(OSSL_RECORD_LAYER *rl) if (rl->nextwbuf == rl->numwpipes && (rl->mode & SSL_MODE_RELEASE_BUFFERS) != 0) tls_release_write_buffer(rl); - return 1; } else if (i <= 0) { if (SSL_CONNECTION_IS_DTLS(s)) { diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 1da4f6a1263..c8951d45dbf 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -204,6 +204,7 @@ int ssl3_write_bytes(SSL *ssl, int type, const void *buf_, size_t len, int i; SSL_CONNECTION *s = SSL_CONNECTION_FROM_SSL_ONLY(ssl); OSSL_RECORD_TEMPLATE tmpls[SSL_MAX_PIPELINES]; + unsigned int recversion; if (s == NULL) return -1; @@ -479,6 +480,18 @@ int ssl3_write_bytes(SSL *ssl, int type, const void *buf_, size_t len, return -1; } + /* + * Some servers hang if initial client hello is larger than 256 bytes + * and record version number > TLS 1.0 + */ + /* TODO(RECLAYER): Does this also need to be in the DTLS equivalent code? */ + recversion = (s->version == TLS1_3_VERSION) ? TLS1_2_VERSION : s->version; + if (SSL_get_state(ssl) == TLS_ST_CW_CLNT_HELLO + && !s->renegotiate + && TLS1_get_version(ssl) > TLS1_VERSION + && s->hello_retry_request == SSL_HRR_NONE) + recversion = TLS1_VERSION; + for (;;) { size_t tmppipelen, remain; size_t numpipes, j, lensofar = 0; @@ -497,6 +510,7 @@ int ssl3_write_bytes(SSL *ssl, int type, const void *buf_, size_t len, */ for (j = 0; j < numpipes; j++) { tmpls[j].type = type; + tmpls[j].version = recversion; tmpls[j].buf = &(buf[tot]) + (j * max_send_fragment); tmpls[j].buflen = max_send_fragment; } @@ -514,6 +528,7 @@ int ssl3_write_bytes(SSL *ssl, int type, const void *buf_, size_t len, tmppipelen++; for (j = 0; j < numpipes; j++) { tmpls[j].type = type; + tmpls[j].version = recversion; tmpls[j].buf = &(buf[tot]) + lensofar; tmpls[j].buflen = tmppipelen; lensofar += tmppipelen; @@ -1422,3 +1437,14 @@ int ssl_set_new_record_layer(SSL_CONNECTION *s, int version, return ssl_post_record_layer_select(s, direction); } + +int ssl_set_record_protocol_version(SSL_CONNECTION *s, int vers) +{ + if (!ossl_assert(s->rlayer.rrlmethod != NULL) + || !ossl_assert(s->rlayer.wrlmethod != NULL)) + return 0; + s->rlayer.rrlmethod->set_protocol_version(s->rlayer.rrl, s->version); + s->rlayer.wrlmethod->set_protocol_version(s->rlayer.wrl, s->version); + + return 1; +} diff --git a/ssl/record/record.h b/ssl/record/record.h index a5d8fd36707..75080e653ea 100644 --- a/ssl/record/record.h +++ b/ssl/record/record.h @@ -260,6 +260,7 @@ int ssl_set_new_record_layer(SSL_CONNECTION *s, int version, int direction, const EVP_CIPHER *ciph, size_t taglen, int mactype, const EVP_MD *md, const SSL_COMP *comp); +int ssl_set_record_protocol_version(SSL_CONNECTION *s, int vers); # define OSSL_FUNC_RLAYER_SKIP_EARLY_DATA 1 OSSL_CORE_MAKE_FUNC(int, rlayer_skip_early_data, (void *cbarg)) diff --git a/ssl/record/recordmethod.h b/ssl/record/recordmethod.h index a6dd59fc322..887a834eb9b 100644 --- a/ssl/record/recordmethod.h +++ b/ssl/record/recordmethod.h @@ -64,6 +64,7 @@ typedef struct ossl_record_layer_st OSSL_RECORD_LAYER; */ struct ossl_record_template_st { int type; + unsigned int version; const unsigned char *buf; size_t buflen; }; diff --git a/ssl/s3_msg.c b/ssl/s3_msg.c index 01ff53bec0d..64e23f3a9b7 100644 --- a/ssl/s3_msg.c +++ b/ssl/s3_msg.c @@ -93,6 +93,14 @@ int ssl3_dispatch_alert(SSL *s) } templ.type = SSL3_RT_ALERT; + templ.version = (sc->version == TLS1_3_VERSION) ? TLS1_2_VERSION + : sc->version; + if (SSL_get_state(s) == TLS_ST_CW_CLNT_HELLO + && !sc->renegotiate + && TLS1_get_version(s) > TLS1_VERSION + && sc->hello_retry_request == SSL_HRR_NONE) { + templ.version = TLS1_VERSION; + } templ.buf = &sc->s3.send_alert[0]; templ.buflen = 2; diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c index 3a1f3e81054..0695664c973 100644 --- a/ssl/statem/extensions_clnt.c +++ b/ssl/statem/extensions_clnt.c @@ -1770,7 +1770,10 @@ int tls_parse_stoc_supported_versions(SSL_CONNECTION *s, PACKET *pkt, /* We just set it here. We validate it in ssl_choose_client_version */ s->version = version; - s->rlayer.rrlmethod->set_protocol_version(s->rlayer.rrl, version); + if (!ssl_set_record_protocol_version(s, version)) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + return 0; + } return 1; } diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 3d00386129b..40bc5e88fa0 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -1417,7 +1417,10 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL_CONNECTION *s, PACKET *pkt) } s->hello_retry_request = SSL_HRR_PENDING; /* Tell the record layer that we know we're going to get TLSv1.3 */ - s->rlayer.rrlmethod->set_protocol_version(s->rlayer.rrl, s->version); + if (!ssl_set_record_protocol_version(s, s->version)) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + goto err; + } hrr = 1; if (!PACKET_forward(pkt, SSL3_RANDOM_SIZE)) { SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_R_LENGTH_MISMATCH); diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index bfc8c1cac9b..86b30b47da1 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -1873,8 +1873,7 @@ int ssl_choose_server_version(SSL_CONNECTION *s, CLIENTHELLO_MSG *hello, check_for_downgrade(s, best_vers, dgrd); s->version = best_vers; ssl->method = best_method; - if (!s->rlayer.rrlmethod->set_protocol_version(s->rlayer.rrl, - best_vers)) + if (!ssl_set_record_protocol_version(s, best_vers)) return ERR_R_INTERNAL_ERROR; return 0; @@ -1904,8 +1903,7 @@ int ssl_choose_server_version(SSL_CONNECTION *s, CLIENTHELLO_MSG *hello, check_for_downgrade(s, vent->version, dgrd); s->version = vent->version; ssl->method = method; - if (!s->rlayer.rrlmethod->set_protocol_version(s->rlayer.rrl, - s->version)) + if (!ssl_set_record_protocol_version(s, s->version)) return ERR_R_INTERNAL_ERROR; return 0; @@ -1967,8 +1965,7 @@ int ssl_choose_client_version(SSL_CONNECTION *s, int version, * versions they don't want. If not, then easy to fix, just return * ssl_method_error(s, s->method) */ - if (!s->rlayer.rrlmethod->set_protocol_version(s->rlayer.rrl, - s->version)) { + if (!ssl_set_record_protocol_version(s, s->version)) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); return 0; } @@ -2032,8 +2029,7 @@ int ssl_choose_client_version(SSL_CONNECTION *s, int version, continue; ssl->method = vent->cmeth(); - if (!s->rlayer.rrlmethod->set_protocol_version(s->rlayer.rrl, - s->version)) { + if (!ssl_set_record_protocol_version(s, s->version)) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); return 0; } @@ -2202,7 +2198,8 @@ int ssl_set_client_hello_version(SSL_CONNECTION *s) * we read the ServerHello. So we need to tell the record layer * about this immediately. */ - s->rlayer.rrlmethod->set_protocol_version(s->rlayer.rrl, ver_max); + if (!ssl_set_record_protocol_version(s, ver_max)) + return 0; } } else if (ver_max > TLS1_2_VERSION) { /* TLS1.3 always uses TLS1.2 in the legacy_version field */