Allow different protocol version when trying to reuse a session

We now send the highest supported version by the client, even if the session
uses an older version.

This fixes 2 problems:
- When you try to reuse a session but the other side doesn't reuse it and
  uses a different protocol version the connection will fail.
- When you're trying to reuse a session with an old version you might be
  stuck trying to reuse the old version while both sides support a newer
  version

Signed-off-by: Kurt Roeckx <kurt@roeckx.be>
Reviewed-by: Viktor Dukhovni <viktor@openssl.org>

GH: #852, MR: #2452

6 files changed, 86 insertions, 167 deletions

diff --git a/ssl/methods.c b/ssl/methods.c
index d66edff988..e576502c63 100644
--- a/ssl/methods.c
+++ b/ssl/methods.c
@@ -116,59 +116,34 @@
  * TLS/SSLv3 methods
  */
 
-static const SSL_METHOD *tls1_get_method(int ver)
-{
-    if (ver == TLS_ANY_VERSION)
-        return TLS_method();
-#ifndef OPENSSL_NO_TLS1_2
-    if (ver == TLS1_2_VERSION)
-        return tlsv1_2_method();
-#endif
-#ifndef OPENSSL_NO_TLS1_1
-    if (ver == TLS1_1_VERSION)
-        return tlsv1_1_method();
-#endif
-#ifndef OPENSSL_NO_TLS1
-    if (ver == TLS1_VERSION)
-        return tlsv1_method();
-#endif
-#ifndef OPENSSL_NO_SSL3
-    if (ver == SSL3_VERSION)
-        return (sslv3_method());
-    else
-#endif
-    return NULL;
-}
-
 IMPLEMENT_tls_meth_func(TLS_ANY_VERSION, 0, 0,
                         TLS_method,
                         ossl_statem_accept,
-                        ossl_statem_connect, tls1_get_method, TLSv1_2_enc_data)
+                        ossl_statem_connect, TLSv1_2_enc_data)
 
 #ifndef OPENSSL_NO_TLS1_2_METHOD
 IMPLEMENT_tls_meth_func(TLS1_2_VERSION, 0, SSL_OP_NO_TLSv1_2,
                         tlsv1_2_method,
                         ossl_statem_accept,
-                        ossl_statem_connect, tls1_get_method, TLSv1_2_enc_data)
+                        ossl_statem_connect, TLSv1_2_enc_data)
 #endif
 
 #ifndef OPENSSL_NO_TLS1_1_METHOD
 IMPLEMENT_tls_meth_func(TLS1_1_VERSION, SSL_METHOD_NO_SUITEB, SSL_OP_NO_TLSv1_1,
                         tlsv1_1_method,
                         ossl_statem_accept,
-                        ossl_statem_connect, tls1_get_method, TLSv1_1_enc_data)
+                        ossl_statem_connect, TLSv1_1_enc_data)
 #endif
 
 #ifndef OPENSSL_NO_TLS1_METHOD
 IMPLEMENT_tls_meth_func(TLS1_VERSION, SSL_METHOD_NO_SUITEB, SSL_OP_NO_TLSv1,
                         tlsv1_method,
                         ossl_statem_accept,
-                        ossl_statem_connect, tls1_get_method, TLSv1_enc_data)
+                        ossl_statem_connect, TLSv1_enc_data)
 #endif
 
 #ifndef OPENSSL_NO_SSL3_METHOD
-IMPLEMENT_ssl3_meth_func(sslv3_method, ossl_statem_accept, ossl_statem_connect,
-                         tls1_get_method)
+IMPLEMENT_ssl3_meth_func(sslv3_method, ossl_statem_accept, ossl_statem_connect)
 #endif
 
 
@@ -176,41 +151,18 @@ IMPLEMENT_ssl3_meth_func(sslv3_method, ossl_statem_accept, ossl_statem_connect,
  * TLS/SSLv3 server methods
  */
 
-static const SSL_METHOD *tls1_get_server_method(int ver)
-{
-    if (ver == TLS_ANY_VERSION)
-        return TLS_server_method();
-#ifndef OPENSSL_NO_TLS1_2
-    if (ver == TLS1_2_VERSION)
-        return tlsv1_2_server_method();
-#endif
-#ifndef OPENSSL_NO_TLS1_1
-    if (ver == TLS1_1_VERSION)
-        return tlsv1_1_server_method();
-#endif
-#ifndef OPENSSL_NO_TLS1
-    if (ver == TLS1_VERSION)
-        return tlsv1_server_method();
-#endif
-#ifndef OPENSSL_NO_SSL3
-    if (ver == SSL3_VERSION)
-        return (sslv3_server_method());
-#endif
-    return NULL;
-}
-
 IMPLEMENT_tls_meth_func(TLS_ANY_VERSION, 0, 0,
                         TLS_server_method,
                         ossl_statem_accept,
                         ssl_undefined_function,
-                        tls1_get_server_method, TLSv1_2_enc_data)
+                        TLSv1_2_enc_data)
 
 #ifndef OPENSSL_NO_TLS1_2_METHOD
 IMPLEMENT_tls_meth_func(TLS1_2_VERSION, 0, SSL_OP_NO_TLSv1_2,
                         tlsv1_2_server_method,
                         ossl_statem_accept,
                         ssl_undefined_function,
-                        tls1_get_server_method, TLSv1_2_enc_data)
+                        TLSv1_2_enc_data)
 #endif
 
 #ifndef OPENSSL_NO_TLS1_1_METHOD
@@ -218,7 +170,7 @@ IMPLEMENT_tls_meth_func(TLS1_1_VERSION, SSL_METHOD_NO_SUITEB, SSL_OP_NO_TLSv1_1,
                         tlsv1_1_server_method,
                         ossl_statem_accept,
                         ssl_undefined_function,
-                        tls1_get_server_method, TLSv1_1_enc_data)
+                        TLSv1_1_enc_data)
 #endif
 
 #ifndef OPENSSL_NO_TLS1_METHOD
@@ -226,13 +178,13 @@ IMPLEMENT_tls_meth_func(TLS1_VERSION, SSL_METHOD_NO_SUITEB, SSL_OP_NO_TLSv1,
                         tlsv1_server_method,
                         ossl_statem_accept,
                         ssl_undefined_function,
-                        tls1_get_server_method, TLSv1_enc_data)
+                        TLSv1_enc_data)
 #endif
 
 #ifndef OPENSSL_NO_SSL3_METHOD
 IMPLEMENT_ssl3_meth_func(sslv3_server_method,
                          ossl_statem_accept,
-                         ssl_undefined_function, tls1_get_server_method)
+                         ssl_undefined_function)
 #endif
 
 
@@ -240,41 +192,18 @@ IMPLEMENT_ssl3_meth_func(sslv3_server_method,
  * TLS/SSLv3 client methods
  */
 
-static const SSL_METHOD *tls1_get_client_method(int ver)
-{
-    if (ver == TLS_ANY_VERSION)
-        return TLS_client_method();
-#ifndef OPENSSL_NO_TLS1_2
-    if (ver == TLS1_2_VERSION)
-        return tlsv1_2_client_method();
-#endif
-#ifndef OPENSSL_NO_TLS1_1
-    if (ver == TLS1_1_VERSION)
-        return tlsv1_1_client_method();
-#endif
-#ifndef OPENSSL_NO_TLS1
-    if (ver == TLS1_VERSION)
-        return tlsv1_client_method();
-#endif
-#ifndef OPENSSL_NO_SSL3
-    if (ver == SSL3_VERSION)
-        return (sslv3_client_method());
-#endif
-    return NULL;
-}
-
 IMPLEMENT_tls_meth_func(TLS_ANY_VERSION, 0, 0,
                         TLS_client_method,
                         ssl_undefined_function,
                         ossl_statem_connect,
-                        tls1_get_client_method, TLSv1_2_enc_data)
+                        TLSv1_2_enc_data)
 
 #ifndef OPENSSL_NO_TLS1_2_METHOD
 IMPLEMENT_tls_meth_func(TLS1_2_VERSION, 0, SSL_OP_NO_TLSv1_2,
                         tlsv1_2_client_method,
                         ssl_undefined_function,
                         ossl_statem_connect,
-                        tls1_get_client_method, TLSv1_2_enc_data)
+                        TLSv1_2_enc_data)
 #endif
 
 #ifndef OPENSSL_NO_TLS1_1_METHOD
@@ -282,7 +211,7 @@ IMPLEMENT_tls_meth_func(TLS1_1_VERSION, SSL_METHOD_NO_SUITEB, SSL_OP_NO_TLSv1_1,
                         tlsv1_1_client_method,
                         ssl_undefined_function,
                         ossl_statem_connect,
-                        tls1_get_client_method, TLSv1_1_enc_data)
+                        TLSv1_1_enc_data)
 #endif
 
 #ifndef OPENSSL_NO_TLS1_METHOD
@@ -290,41 +219,26 @@ IMPLEMENT_tls_meth_func(TLS1_VERSION, SSL_METHOD_NO_SUITEB, SSL_OP_NO_TLSv1,
                         tlsv1_client_method,
                         ssl_undefined_function,
                         ossl_statem_connect,
-                        tls1_get_client_method, TLSv1_enc_data)
+                        TLSv1_enc_data)
 #endif
 
 #ifndef OPENSSL_NO_SSL3_METHOD
 IMPLEMENT_ssl3_meth_func(sslv3_client_method,
                          ssl_undefined_function,
-                         ossl_statem_connect, tls1_get_client_method)
+                         ossl_statem_connect)
 #endif
 
 
 /*
  * DTLS methods
  */
-static const SSL_METHOD *dtls1_get_method(int ver)
-{
-    if (ver == DTLS_ANY_VERSION)
-        return DTLS_method();
-#ifndef OPENSSL_NO_DTLS1
-    else if (ver == DTLS1_VERSION)
-        return dtlsv1_method();
-#endif
-#ifndef OPENSSL_NO_DTLS1_2
-    else if (ver == DTLS1_2_VERSION)
-        return dtlsv1_2_method();
-#endif
-    else
-        return NULL;
-}
 
 #ifndef OPENSSL_NO_DTLS1_METHOD
 IMPLEMENT_dtls1_meth_func(DTLS1_VERSION, SSL_METHOD_NO_SUITEB, SSL_OP_NO_DTLSv1,
                           dtlsv1_method,
                           ossl_statem_accept,
                           ossl_statem_connect,
-                          dtls1_get_method, DTLSv1_enc_data)
+                          DTLSv1_enc_data)
 #endif
 
 #ifndef OPENSSL_NO_DTLS1_2_METHOD
@@ -332,41 +246,25 @@ IMPLEMENT_dtls1_meth_func(DTLS1_2_VERSION, 0, SSL_OP_NO_DTLSv1_2,
                           dtlsv1_2_method,
                           ossl_statem_accept,
                           ossl_statem_connect,
-                          dtls1_get_method, DTLSv1_2_enc_data)
+                          DTLSv1_2_enc_data)
 #endif
 
 IMPLEMENT_dtls1_meth_func(DTLS_ANY_VERSION, 0, 0,
                           DTLS_method,
                           ossl_statem_accept,
                           ossl_statem_connect,
-                          dtls1_get_method, DTLSv1_2_enc_data)
+                          DTLSv1_2_enc_data)
 
 /*
  * DTLS server methods
  */
 
-static const SSL_METHOD *dtls1_get_server_method(int ver)
-{
-    if (ver == DTLS_ANY_VERSION)
-        return DTLS_server_method();
-#ifndef OPENSSL_NO_DTLS1
-    else if (ver == DTLS1_VERSION)
-        return dtlsv1_server_method();
-#endif
-#ifndef OPENSSL_NO_DTLS1_2
-    else if (ver == DTLS1_2_VERSION)
-        return dtlsv1_2_server_method();
-#endif
-    else
-        return NULL;
-}
-
 #ifndef OPENSSL_NO_DTLS1_METHOD
 IMPLEMENT_dtls1_meth_func(DTLS1_VERSION, SSL_METHOD_NO_SUITEB, SSL_OP_NO_DTLSv1,
                           dtlsv1_server_method,
                           ossl_statem_accept,
                           ssl_undefined_function,
-                          dtls1_get_server_method, DTLSv1_enc_data)
+                          DTLSv1_enc_data)
 #endif
 
 #ifndef OPENSSL_NO_DTLS1_2_METHOD
@@ -374,42 +272,26 @@ IMPLEMENT_dtls1_meth_func(DTLS1_2_VERSION, 0, SSL_OP_NO_DTLSv1_2,
                           dtlsv1_2_server_method,
                           ossl_statem_accept,
                           ssl_undefined_function,
-                          dtls1_get_server_method, DTLSv1_2_enc_data)
+                          DTLSv1_2_enc_data)
 #endif
 
 IMPLEMENT_dtls1_meth_func(DTLS_ANY_VERSION, 0, 0,
                           DTLS_server_method,
                           ossl_statem_accept,
                           ssl_undefined_function,
-                          dtls1_get_server_method, DTLSv1_2_enc_data)
+                          DTLSv1_2_enc_data)
 
 
 /*
  * DTLS client methods
  */
 
-static const SSL_METHOD *dtls1_get_client_method(int ver)
-{
-    if (ver == DTLS_ANY_VERSION)
-        return DTLS_client_method();
-#ifndef OPENSSL_NO_DTLS1
-    else if (ver == DTLS1_VERSION || ver == DTLS1_BAD_VER)
-        return dtlsv1_client_method();
-#endif
-#ifndef OPENSSL_NO_DTLS1_2
-    else if (ver == DTLS1_2_VERSION)
-        return dtlsv1_2_client_method();
-#endif
-    else
-        return NULL;
-}
-
 #ifndef OPENSSL_NO_DTLS1_METHOD
 IMPLEMENT_dtls1_meth_func(DTLS1_VERSION, SSL_METHOD_NO_SUITEB, SSL_OP_NO_DTLSv1,
                           dtlsv1_client_method,
                           ssl_undefined_function,
                           ossl_statem_connect,
-                          dtls1_get_client_method, DTLSv1_enc_data)
+                          DTLSv1_enc_data)
 #endif
 
 #ifndef OPENSSL_NO_DTLS1_2_METHOD
@@ -417,14 +299,14 @@ IMPLEMENT_dtls1_meth_func(DTLS1_2_VERSION, 0, SSL_OP_NO_DTLSv1_2,
                           dtlsv1_2_client_method,
                           ssl_undefined_function,
                           ossl_statem_connect,
-                          dtls1_get_client_method, DTLSv1_2_enc_data)
+                          DTLSv1_2_enc_data)
 #endif
 
 IMPLEMENT_dtls1_meth_func(DTLS_ANY_VERSION, 0, 0,
                           DTLS_client_method,
                           ssl_undefined_function,
                           ossl_statem_connect,
-                          dtls1_get_client_method, DTLSv1_2_enc_data)
+                          DTLSv1_2_enc_data)
 
 #if OPENSSL_API_COMPAT < 0x10100000L
 
diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index 88f6c73cfe..d0cadc60f0 100644
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -634,6 +634,8 @@ static ERR_STRING_DATA SSL_str_reasons[] = {
      "ssl session id context too long"},
     {ERR_REASON(SSL_R_SSL_SESSION_ID_HAS_BAD_LENGTH),
      "ssl session id has bad length"},
+    {ERR_REASON(SSL_R_SSL_SESSION_VERSION_MISMATCH),
+     "ssl session version mismatch"},
     {ERR_REASON(SSL_R_TLS_CLIENT_CERT_REQ_WITH_ANON_CIPHER),
      "tls client cert req with anon cipher"},
     {ERR_REASON(SSL_R_TLS_HEARTBEAT_PEER_DOESNT_ACCEPT),
diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 64f4ae9373..4a2b52d19e 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -542,7 +542,6 @@ struct ssl_method_st {
     int (*ssl_pending) (const SSL *s);
     int (*num_ciphers) (void);
     const SSL_CIPHER *(*get_cipher) (unsigned ncipher);
-    const struct ssl_method_st *(*get_ssl_method) (int version);
     long (*get_timeout) (void);
     const struct ssl3_enc_method *ssl3_enc; /* Extra SSLv3/TLS stuff */
     int (*ssl_version) (void);
@@ -1761,7 +1760,7 @@ extern const SSL3_ENC_METHOD DTLSv1_2_enc_data;
 #define SSL_METHOD_NO_SUITEB    (1U<<1)
 
 # define IMPLEMENT_tls_meth_func(version, flags, mask, func_name, s_accept, \
-                                 s_connect, s_get_meth, enc_data) \
+                                 s_connect, enc_data) \
 const SSL_METHOD *func_name(void)  \
         { \
         static const SSL_METHOD func_name##_data= { \
@@ -1789,7 +1788,6 @@ const SSL_METHOD *func_name(void)  \
                 ssl3_pending, \
                 ssl3_num_ciphers, \
                 ssl3_get_cipher, \
-                s_get_meth, \
                 tls1_default_timeout, \
                 &enc_data, \
                 ssl_undefined_void_function, \
@@ -1799,7 +1797,7 @@ const SSL_METHOD *func_name(void)  \
         return &func_name##_data; \
         }
 
-# define IMPLEMENT_ssl3_meth_func(func_name, s_accept, s_connect, s_get_meth) \
+# define IMPLEMENT_ssl3_meth_func(func_name, s_accept, s_connect) \
 const SSL_METHOD *func_name(void)  \
         { \
         static const SSL_METHOD func_name##_data= { \
@@ -1827,7 +1825,6 @@ const SSL_METHOD *func_name(void)  \
                 ssl3_pending, \
                 ssl3_num_ciphers, \
                 ssl3_get_cipher, \
-                s_get_meth, \
                 ssl3_default_timeout, \
                 &SSLv3_enc_data, \
                 ssl_undefined_void_function, \
@@ -1838,7 +1835,7 @@ const SSL_METHOD *func_name(void)  \
         }
 
 # define IMPLEMENT_dtls1_meth_func(version, flags, mask, func_name, s_accept, \
-                                        s_connect, s_get_meth, enc_data) \
+                                        s_connect, enc_data) \
 const SSL_METHOD *func_name(void)  \
         { \
         static const SSL_METHOD func_name##_data= { \
@@ -1866,7 +1863,6 @@ const SSL_METHOD *func_name(void)  \
                 ssl3_pending, \
                 ssl3_num_ciphers, \
                 ssl3_get_cipher, \
-                s_get_meth, \
                 dtls1_default_timeout, \
                 &enc_data, \
                 ssl_undefined_void_function, \
@@ -1996,6 +1992,8 @@ __owur int ssl3_handshake_write(SSL *s);
 
 __owur int ssl_allow_compression(SSL *s);
 
+__owur int ssl_version_supported(const SSL *s, int version);
+
 __owur int ssl_set_client_hello_version(SSL *s);
 __owur int ssl_check_version_downgrade(SSL *s);
 __owur int ssl_set_version_bound(int method_version, int version, int *bound);
diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c
index 3f030a76eb..70e2683ee4 100644
--- a/ssl/ssl_sess.c
+++ b/ssl/ssl_sess.c
@@ -872,19 +872,9 @@ int SSL_SESSION_up_ref(SSL_SESSION *ss)
 int SSL_set_session(SSL *s, SSL_SESSION *session)
 {
     int ret = 0;
-    const SSL_METHOD *meth;
-
     if (session != NULL) {
-        meth = s->ctx->method->get_ssl_method(session->ssl_version);
-        if (meth == NULL)
-            meth = s->method->get_ssl_method(session->ssl_version);
-        if (meth == NULL) {
-            SSLerr(SSL_F_SSL_SET_SESSION, SSL_R_UNABLE_TO_FIND_SSL_METHOD);
-            return (0);
-        }
-
-        if (meth != s->method) {
-            if (!SSL_set_ssl_method(s, meth))
+        if (s->ctx->method != s->method) {
+            if (!SSL_set_ssl_method(s, s->ctx->method))
                 return (0);
         }
 
@@ -896,9 +886,8 @@ int SSL_set_session(SSL *s, SSL_SESSION *session)
     } else {
         SSL_SESSION_free(s->session);
         s->session = NULL;
-        meth = s->ctx->method;
-        if (meth != s->method) {
-            if (!SSL_set_ssl_method(s, meth))
+        if (s->ctx->method != s->method) {
+            if (!SSL_set_ssl_method(s, s->ctx->method))
                 return (0);
         }
         ret = 1;
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index bafb90a9dc..73f54bcb96 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -817,7 +817,8 @@ int tls_construct_client_hello(SSL *s)
         goto err;
     }
 
-    if ((sess == NULL) || (sess->ssl_version != s->version) ||
+    if ((sess == NULL) ||
+        !ssl_version_supported(s, sess->ssl_version) ||
         /*
          * In the case of EAP-FAST, we can have a pre-shared
          * "ticket" without a session ID.
@@ -1126,12 +1127,22 @@ 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 */
         memcpy(s->session->session_id, PACKET_data(&session_id),
                session_id_len);
     }
 
+    /* Session version and negotiated protocol version should match */
+    if (s->version != s->session->ssl_version) {
+        al = SSL_AD_PROTOCOL_VERSION;
+
+        SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO,
+               SSL_R_SSL_SESSION_VERSION_MISMATCH);
+        goto f_err;
+    }
+
     c = ssl_get_cipher_by_char(s, cipherchars);
     if (c == NULL) {
         /* unknown cipher */
diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index a24060e47f..8fcc23246e 100644
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -788,6 +788,44 @@ static int ssl_method_error(const SSL *s, const SSL_METHOD *method)
 }
 
 /*
+ * ssl_version_supported - Check that the specified `version` is supported by
+ * `SSL *` instance
+ *
+ * @s: The SSL handle for the candidate method
+ * @version: Protocol version to test against
+ *
+ * Returns 1 when supported, otherwise 0
+ */
+int ssl_version_supported(const SSL *s, int version)
+{
+    const version_info *vent;
+    const version_info *table;
+
+    switch (s->method->version) {
+    default:
+        /* Version should match method version for non-ANY method */
+        return version_cmp(s, version, s->version) == 0;
+    case TLS_ANY_VERSION:
+        table = tls_version_table;
+        break;
+    case DTLS_ANY_VERSION:
+        table = dtls_version_table;
+        break;
+    }
+
+    for (vent = table;
+         vent->version != 0 && version_cmp(s, version, vent->version) <= 0;
+         ++vent) {
+        if (vent->cmeth != NULL &&
+            version_cmp(s, version, vent->version) == 0 &&
+            ssl_method_error(s, vent->cmeth()) == 0) {
+            return 1;
+        }
+    }
+    return 0;
+}
+
+/*
  * ssl_check_version_downgrade - In response to RFC7507 SCSV version
  * fallback indication from a client check whether we're using the highest
  * supported protocol version.
@@ -976,7 +1014,6 @@ int ssl_choose_client_version(SSL *s, int version)
          * versions they don't want.  If not, then easy to fix, just return
          * ssl_method_error(s, s->method)
          */
-        s->session->ssl_version = s->version;
         return 0;
     case TLS_ANY_VERSION:
         table = tls_version_table;
@@ -999,7 +1036,7 @@ int ssl_choose_client_version(SSL *s, int version)
         if (err != 0)
             return err;
         s->method = method;
-        s->session->ssl_version = s->version = version;
+        s->version = version;
         return 0;
     }