Move session version consistency check

Make sure the session version consistency check is inside
ssl_get_prev_session(). Also fixes a bug where an inconsistent version can
cause a seg fault in TLSv1.3.

Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2259)

diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c
index c42ef1e135024d96f5969fb30a953a2d40d76b97..c28a5e1b3f34b7076e17ae40056715ab64530ba6 100644 (file)
--- a/ssl/ssl_sess.c
+++ b/ssl/ssl_sess.c
@@ -556,6 +556,10 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello)
 
     /* Now ret is non-NULL and we own one of its reference counts. */
 
+    /* Check TLS version consistency */
+    if (ret->ssl_version != s->version)
+        goto err;
+
     if (ret->sid_ctx_length != s->sid_ctx_length
         || memcmp(ret->sid_ctx, s->sid_ctx, ret->sid_ctx_length)) {
         /*
@@ -606,23 +610,6 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello)
         goto err;
     }
 
-    /*
-     * TODO(TLS1.3): This is temporary, because TLSv1.3 resumption is completely
-     * different. For now though we're still using the old resumption logic, so
-     * to avoid test failures we need this. Remove this code!
-     * 
-     * Check TLS version consistency. We can't resume <=TLSv1.2 session if we
-     * have negotiated TLSv1.3, and vice versa.
-     */
-    if (!SSL_IS_DTLS(s)
-            && ((ret->ssl_version <= TLS1_2_VERSION
-                 && s->version >=TLS1_3_VERSION)
-                || (ret->ssl_version >= TLS1_3_VERSION
-                    && s->version <= TLS1_2_VERSION))) {
-        /* Continue but do not resume */
-        goto err;
-    }
-
     /* Check extended master secret extension consistency */
     if (ret->flags & SSL_SESS_FLAG_EXTMS) {
         /* If old session includes extms, but new does not: abort handshake */
@@ -651,6 +638,9 @@ int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello)
  err:
     if (ret != NULL) {
         SSL_SESSION_free(ret);
+        /* In TLSv1.3 we already set s->session, so better NULL it out */
+        if (SSL_IS_TLS13(s))
+            s->session = NULL;
 
         if (!try_session_cache) {
             /*

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index f9659e2d9d81dc6d7b3754890d9ed00866b7427c..9292284bdc723a98c5b861faab3108b757c069ed 100644 (file)
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -1476,16 +1476,7 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt)
             goto err;
     } else {
         i = ssl_get_prev_session(s, &clienthello);
-        /*
-         * Only resume if the session's version matches the negotiated
-         * version.
-         * RFC 5246 does not provide much useful advice on resumption
-         * with a different protocol version. It doesn't forbid it but
-         * the sanity of such behaviour would be questionable.
-         * In practice, clients do not accept a version mismatch and
-         * will abort the handshake with an error.
-         */
-        if (i == 1 && s->version == s->session->ssl_version) {
+        if (i == 1) {
             /* previous session */
             s->hit = 1;
         } else if (i == -1) {