Implement session id TLSv1.3 middlebox compatibility mode

Clients will send a "fake" session id and servers must echo it back.

Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/4701)

11 files changed, 117 insertions, 38 deletions

diff --git a/apps/apps.h b/apps/apps.h
index bb89eaecf6..321f6444da 100644
--- a/apps/apps.h
+++ b/apps/apps.h
@@ -208,7 +208,7 @@ int set_cert_times(X509 *x, const char *startdate, const char *enddate,
         OPT_S_STRICT, OPT_S_SIGALGS, OPT_S_CLIENTSIGALGS, OPT_S_GROUPS, \
         OPT_S_CURVES, OPT_S_NAMEDCURVE, OPT_S_CIPHER, \
         OPT_S_RECORD_PADDING, OPT_S_DEBUGBROKE, OPT_S_COMP, \
-        OPT_S_NO_RENEGOTIATION, OPT_S__LAST
+        OPT_S_NO_RENEGOTIATION, OPT_S_NO_MIDDLEBOX, OPT_S__LAST
 
 # define OPT_S_OPTIONS \
         {"no_ssl3", OPT_S_NOSSL3, '-',"Just disable SSLv3" }, \
@@ -253,7 +253,8 @@ int set_cert_times(X509 *x, const char *startdate, const char *enddate,
         {"record_padding", OPT_S_RECORD_PADDING, 's', \
             "Block size to pad TLS 1.3 records to."}, \
         {"debug_broken_protocol", OPT_S_DEBUGBROKE, '-', \
-            "Perform all sorts of protocol violations for testing purposes"}
+            "Perform all sorts of protocol violations for testing purposes"}, \
+        {"no_middlebox", OPT_S_NO_MIDDLEBOX, '-', "Disable TLSv1.3 middlebox compat mode" }
 
 
 # define OPT_S_CASES \
@@ -283,7 +284,8 @@ int set_cert_times(X509 *x, const char *startdate, const char *enddate,
         case OPT_S_CIPHER: \
         case OPT_S_RECORD_PADDING: \
         case OPT_S_NO_RENEGOTIATION: \
-        case OPT_S_DEBUGBROKE
+        case OPT_S_DEBUGBROKE: \
+        case OPT_S_NO_MIDDLEBOX
 
 #define IS_NO_PROT_FLAG(o) \
  (o == OPT_S_NOSSL3 || o == OPT_S_NOTLS1 || o == OPT_S_NOTLS1_1 \
diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt
index 5c4ca2d6d0..4410314bc1 100644
--- a/crypto/err/openssl.txt
+++ b/crypto/err/openssl.txt
@@ -2463,6 +2463,7 @@ SSL_R_INVALID_MAX_EARLY_DATA:174:invalid max early data
 SSL_R_INVALID_NULL_CMD_NAME:385:invalid null cmd name
 SSL_R_INVALID_SEQUENCE_NUMBER:402:invalid sequence number
 SSL_R_INVALID_SERVERINFO_DATA:388:invalid serverinfo data
+SSL_R_INVALID_SESSION_ID:232:invalid session id
 SSL_R_INVALID_SRP_USERNAME:357:invalid srp username
 SSL_R_INVALID_STATUS_RESPONSE:328:invalid status response
 SSL_R_INVALID_TICKET_KEYS_LENGTH:325:invalid ticket keys length
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index a5251b59cc..48779fa6d2 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -338,9 +338,17 @@ typedef int (*SSL_verify_cb)(int preverify_ok, X509_STORE_CTX *x509_ctx);
 # define SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION        0x00040000U
 /* Disable encrypt-then-mac */
 # define SSL_OP_NO_ENCRYPT_THEN_MAC                      0x00080000U
+
+/*
+ * Enable TLSv1.3 Compatibility mode. This is on by default. A future version
+ * of OpenSSL may have this disabled by default.
+ */
+# define SSL_OP_ENABLE_MIDDLEBOX_COMPAT                  0x00100000U
+
 /* Prioritize Chacha20Poly1305 when client does.
  * Modifies SSL_OP_CIPHER_SERVER_PREFERENCE */
 # define SSL_OP_PRIORITIZE_CHACHA                        0x00200000U
+
 /*
  * Set on servers to choose the cipher according to the server's preferences
  */
diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h
index 364b19809e..3199ab0ff4 100644
--- a/include/openssl/sslerr.h
+++ b/include/openssl/sslerr.h
@@ -543,6 +543,7 @@ int ERR_load_SSL_strings(void);
 # define SSL_R_INVALID_NULL_CMD_NAME                      385
 # define SSL_R_INVALID_SEQUENCE_NUMBER                    402
 # define SSL_R_INVALID_SERVERINFO_DATA                    388
+# define SSL_R_INVALID_SESSION_ID                         232
 # define SSL_R_INVALID_SRP_USERNAME                       357
 # define SSL_R_INVALID_STATUS_RESPONSE                    328
 # define SSL_R_INVALID_TICKET_KEYS_LENGTH                 325
diff --git a/ssl/ssl_conf.c b/ssl/ssl_conf.c
index fe090ae40d..0f53a47043 100644
--- a/ssl/ssl_conf.c
+++ b/ssl/ssl_conf.c
@@ -369,7 +369,8 @@ static int cmd_Options(SSL_CONF_CTX *cctx, const char *value)
         SSL_FLAG_TBL_INV("EncryptThenMac", SSL_OP_NO_ENCRYPT_THEN_MAC),
         SSL_FLAG_TBL("NoRenegotiation", SSL_OP_NO_RENEGOTIATION),
         SSL_FLAG_TBL("AllowNoDHEKEX", SSL_OP_ALLOW_NO_DHE_KEX),
-        SSL_FLAG_TBL("PrioritizeChaCha", SSL_OP_PRIORITIZE_CHACHA)
+        SSL_FLAG_TBL("PrioritizeChaCha", SSL_OP_PRIORITIZE_CHACHA),
+        SSL_FLAG_TBL("MiddleboxCompat", SSL_OP_ENABLE_MIDDLEBOX_COMPAT)
     };
     if (value == NULL)
         return -3;
@@ -591,6 +592,7 @@ static const ssl_conf_cmd_tbl ssl_conf_cmds[] = {
     SSL_CONF_CMD_SWITCH("allow_no_dhe_kex", 0),
     SSL_CONF_CMD_SWITCH("prioritize_chacha", SSL_CONF_FLAG_SERVER),
     SSL_CONF_CMD_SWITCH("strict", 0),
+    SSL_CONF_CMD_SWITCH("no_middlebox", SSL_CONF_FLAG_CLIENT),
     SSL_CONF_CMD_STRING(SignatureAlgorithms, "sigalgs", 0),
     SSL_CONF_CMD_STRING(ClientSignatureAlgorithms, "client_sigalgs", 0),
     SSL_CONF_CMD_STRING(Curves, "curves", 0),
@@ -665,6 +667,8 @@ static const ssl_switch_tbl ssl_cmd_switches[] = {
     /* chacha reprioritization */
     {SSL_OP_PRIORITIZE_CHACHA, 0},
     {SSL_CERT_FLAG_TLS_STRICT, SSL_TFLAG_CERT}, /* strict */
+    /* no_middlebox */
+    {SSL_OP_ENABLE_MIDDLEBOX_COMPAT, SSL_TFLAG_INV},
 };
 
 static int ssl_conf_cmd_skip_prefix(SSL_CONF_CTX *cctx, const char **pcmd)
diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index d608daff07..fa02b21b9d 100644
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -871,6 +871,7 @@ static const ERR_STRING_DATA SSL_str_reasons[] = {
     "invalid sequence number"},
     {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_INVALID_SERVERINFO_DATA),
     "invalid serverinfo data"},
+    {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_INVALID_SESSION_ID), "invalid session id"},
     {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_INVALID_SRP_USERNAME),
     "invalid srp username"},
     {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_INVALID_STATUS_RESPONSE),
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 20073184eb..bba0291421 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -2894,9 +2894,11 @@ SSL_CTX *SSL_CTX_new(const SSL_METHOD *meth)
      * Disable compression by default to prevent CRIME. Applications can
      * re-enable compression by configuring
      * SSL_CTX_clear_options(ctx, SSL_OP_NO_COMPRESSION);
-     * or by using the SSL_CONF library.
+     * or by using the SSL_CONF library. Similarly we also enable TLSv1.3
+     * middlebox compatibility by default. This may be disabled by default in
+     * a later OpenSSL version.
      */
-    ret->options |= SSL_OP_NO_COMPRESSION;
+    ret->options |= SSL_OP_NO_COMPRESSION | SSL_OP_ENABLE_MIDDLEBOX_COMPAT;
 
     ret->ext.status_type = TLSEXT_STATUSTYPE_nothing;
 
diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index ed6b9a8fd4..c2d63fa100 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -1132,6 +1132,12 @@ struct ssl_st {
     size_t psksession_id_len;
     /* Default generate session ID callback. */
     GEN_SESSION_CB generate_session_id;
+    /*
+     * The temporary TLSv1.3 session id. This isn't really a session id at all
+     * but is a random value sent in the legacy session id field.
+     */
+    unsigned char tmp_session_id[SSL_MAX_SSL_SESSION_ID_LENGTH];
+    size_t tmp_session_id_len;
     /* Used in SSL3 */
     /*
      * 0 don't care about verify failure.
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 3c628bdd99..473da7a730 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -1028,6 +1028,7 @@ int tls_construct_client_hello(SSL *s, WPACKET *pkt)
     SSL_COMP *comp;
 #endif
     SSL_SESSION *sess = s->session;
+    unsigned char *session_id;
 
     if (!WPACKET_set_max_size(pkt, SSL3_RT_MAX_PLAIN_LENGTH)) {
         /* Should not happen */
@@ -1047,7 +1048,7 @@ int tls_construct_client_hello(SSL *s, WPACKET *pkt)
     if (sess == NULL
             || !ssl_version_supported(s, sess->ssl_version)
             || !SSL_SESSION_is_resumable(sess)) {
-        if (!ssl_get_new_session(s, 0)) {
+        if (!s->hello_retry_request && !ssl_get_new_session(s, 0)) {
             /* SSLfatal() already called */
             return 0;
         }
@@ -1121,13 +1122,34 @@ int tls_construct_client_hello(SSL *s, WPACKET *pkt)
     }
 
     /* Session ID */
-    if (s->new_session || s->session->ssl_version == TLS1_3_VERSION)
-        sess_id_len = 0;
-    else
+    session_id = s->session->session_id;
+    if (s->new_session || s->session->ssl_version == TLS1_3_VERSION) {
+        if (s->version == TLS1_3_VERSION
+                && (s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) != 0) {
+            sess_id_len = sizeof(s->tmp_session_id);
+            s->tmp_session_id_len = sess_id_len;
+            session_id = s->tmp_session_id;
+            if (!s->hello_retry_request
+                    && ssl_randbytes(s, s->tmp_session_id,
+                                     sess_id_len) <= 0) {
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR,
+                         SSL_F_TLS_CONSTRUCT_CLIENT_HELLO,
+                         ERR_R_INTERNAL_ERROR);
+                return 0;
+            }
+        } else {
+            sess_id_len = 0;
+        }
+    } else {
         sess_id_len = s->session->session_id_length;
+        if (s->version == TLS1_3_VERSION) {
+            s->tmp_session_id_len = sess_id_len;
+            memcpy(s->tmp_session_id, s->session->session_id, sess_id_len);
+        }
+    }
     if (sess_id_len > sizeof(s->session->session_id)
             || !WPACKET_start_sub_packet_u8(pkt)
-            || (sess_id_len != 0 && !WPACKET_memcpy(pkt, s->session->session_id,
+            || (sess_id_len != 0 && !WPACKET_memcpy(pkt, session_id,
                                                     sess_id_len))
             || !WPACKET_close(pkt)) {
         SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CLIENT_HELLO,
@@ -1393,25 +1415,35 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt)
         goto err;
     }
 
-    /*
-     * In TLSv1.3 a ServerHello message signals a key change so the end of the
-     * message must be on a record boundary.
-     */
-    if (SSL_IS_TLS13(s) && RECORD_LAYER_processed_read_pending(&s->rlayer)) {
-        SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_F_TLS_PROCESS_SERVER_HELLO,
-                 SSL_R_NOT_ON_RECORD_BOUNDARY);
-        goto err;
-    }
-
-    if (SSL_IS_TLS13(s) && compression != 0) {
-        SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_F_TLS_PROCESS_SERVER_HELLO,
-               SSL_R_INVALID_COMPRESSION_ALGORITHM);
-        goto err;
-    }
-
     s->hit = 0;
 
     if (SSL_IS_TLS13(s)) {
+        /*
+         * In TLSv1.3 a ServerHello message signals a key change so the end of
+         * the message must be on a record boundary.
+         */
+        if (RECORD_LAYER_processed_read_pending(&s->rlayer)) {
+            SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE,
+                     SSL_F_TLS_PROCESS_SERVER_HELLO,
+                     SSL_R_NOT_ON_RECORD_BOUNDARY);
+            goto err;
+        }
+
+        if (compression != 0) {
+            SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER,
+                     SSL_F_TLS_PROCESS_SERVER_HELLO,
+                     SSL_R_INVALID_COMPRESSION_ALGORITHM);
+            goto err;
+        }
+
+        if (session_id_len != s->tmp_session_id_len
+                || memcmp(PACKET_data(&session_id), s->tmp_session_id,
+                          session_id_len) != 0) {
+            SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER,
+                     SSL_F_TLS_PROCESS_SERVER_HELLO, SSL_R_INVALID_SESSION_ID);
+            goto err;
+        }
+
         /* This will set s->hit if we are resuming */
         if (!tls_parse_extension(s, TLSEXT_IDX_psk,
                                  SSL_EXT_TLS1_3_SERVER_HELLO,
@@ -1493,11 +1525,19 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt)
         }
 
         s->session->ssl_version = s->version;
-        s->session->session_id_length = session_id_len;
-        /* session_id_len could be 0 */
-        if (session_id_len > 0)
-            memcpy(s->session->session_id, PACKET_data(&session_id),
-                   session_id_len);
+        /*
+         * In TLSv1.2 and below we save the session id we were sent so we can
+         * resume it later. In TLSv1.3 the session id we were sent is just an
+         * echo of what we originally sent in the ClientHello and should not be
+         * used for resumption.
+         */
+        if (!SSL_IS_TLS13(s)) {
+            s->session->session_id_length = session_id_len;
+            /* session_id_len could be 0 */
+            if (session_id_len > 0)
+                memcpy(s->session->session_id, PACKET_data(&session_id),
+                       session_id_len);
+        }
     }
 
     /* Session version and negotiated protocol version should match */
diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index e91bba4c0f..36083096b9 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -1686,6 +1686,12 @@ static int tls_early_post_process_client_hello(SSL *s)
         }
     }
 
+    if (SSL_IS_TLS13(s)) {
+        memcpy(s->tmp_session_id, s->clienthello->session_id,
+               s->clienthello->session_id_len);
+        s->tmp_session_id_len = s->clienthello->session_id_len;
+    }
+
     /*
      * If it is a hit, check that the cipher is in the list. In TLSv1.3 we check
      * ciphersuite compatibility with the session as part of resumption.
@@ -2192,6 +2198,7 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt)
     int compm;
     size_t sl, len;
     int version;
+    unsigned char *session_id;
 
     version = SSL_IS_TLS13(s) ? TLS1_2_VERSION : s->version;
     if (!WPACKET_put_bytes_u16(pkt, version)
@@ -2217,6 +2224,8 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt)
      *   session ID.
      * - However, if we want the new session to be single-use,
      *   we send back a 0-length session ID.
+     * - In TLSv1.3 we echo back the session id sent to us by the client
+     *   regardless
      * s->hit is non-zero in either case of session reuse,
      * so the following won't overwrite an ID that we're supposed
      * to send back.
@@ -2226,17 +2235,20 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt)
          && !s->hit))
         s->session->session_id_length = 0;
 
-    sl = s->session->session_id_length;
+    if (SSL_IS_TLS13(s)) {
+        sl = s->tmp_session_id_len;
+        session_id = s->tmp_session_id;
+    } else {
+        sl = s->session->session_id_length;
+        session_id = s->session->session_id;
+    }
+
     if (sl > sizeof(s->session->session_id)) {
         SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_SERVER_HELLO,
                  ERR_R_INTERNAL_ERROR);
         return 0;
     }
 
-    /* Never send a session_id back in the ServerHello */
-    if (SSL_IS_TLS13(s))
-        sl = 0;
-
     /* set up the compression method */
 #ifdef OPENSSL_NO_COMP
     compm = 0;
@@ -2247,7 +2259,7 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt)
         compm = s->s3->tmp.new_compression->id;
 #endif
 
-    if (!WPACKET_sub_memcpy_u8(pkt, s->session->session_id, sl)
+    if (!WPACKET_sub_memcpy_u8(pkt,     session_id, sl)
             || !s->method->put_cipher_by_char(s->s3->tmp.new_cipher, pkt, &len)
             || !WPACKET_put_bytes_u8(pkt, compm)
             || !tls_construct_extensions(s, pkt,
diff --git a/test/clienthellotest.c b/test/clienthellotest.c
index 8ba65ce4fb..88e0a1c66a 100644
--- a/test/clienthellotest.c
+++ b/test/clienthellotest.c
@@ -90,6 +90,8 @@ static int test_client_hello(int currtest)
     case TEST_ADD_PADDING:
     case TEST_PADDING_NOT_NEEDED:
         SSL_CTX_set_options(ctx, SSL_OP_TLSEXT_PADDING);
+        /* Make sure we get a consistent size across TLS versions */
+        SSL_CTX_clear_options(ctx, SSL_OP_ENABLE_MIDDLEBOX_COMPAT);
         /*
          * Add some dummy ALPN protocols so that the ClientHello is at least
          * F5_WORKAROUND_MIN_MSG_LEN bytes long - meaning padding will be