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 <hlandau@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(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 d56184f81622965b5e5956a0232f7618ccf104fc..01bfd477d9210c862bc505861fc427afb4422758 100644 (file)
--- 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 <assert.h>
 #include <openssl/bio.h>
 #include <openssl/ssl.h>
 #include <openssl/err.h>
@@ -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 1da4f6a1263ba8eddd11a921ecf0d7b622c06e21..c8951d45dbfdb2662266a8b1da2ca0e68b99af59 100644 (file)
--- 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 a5d8fd3670776498796a7f3e540d0b8152c401b6..75080e653eafa283b9ddf4b45b88e75dc150ab74 100644 (file)
--- 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 a6dd59fc3226ee158cec1b149ff3bb82e1215c90..887a834eb9bc9a207b2cb3bb449cf9a74ff265a2 100644 (file)
--- 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 01ff53bec0d0be737677fb09bce495deeadaa6fe..64e23f3a9b719d0b585d77a5dbe8a65f3629d110 100644 (file)
--- 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 3a1f3e81054cbb5bc2fbcd2692df7487a5720b5e..0695664c973f1efd0474af685623b159d96bb108 100644 (file)
--- 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 3d00386129b398ccad30d01c38e816cbd8aff9e3..40bc5e88fa0862204daeea69bc56056795f327ca 100644 (file)
--- 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 bfc8c1cac9bc97e297680539d0ec535cfb8fd13b..86b30b47da19a63fe5021f447f16d8a6c9e38639 100644 (file)
--- 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 */