From f2342b7ac3c3fe5914235a692c22db1dae316af4 Mon Sep 17 00:00:00 2001
From: Matt Caswell <matt@openssl.org>
Date: Wed, 9 Nov 2016 14:43:05 +0000
Subject: [PATCH] Address some supported_versions review comments

Added some TODOs, refactored a couple of things and added a SSL_IS_TLS13()
macro.

Reviewed-by: Rich Salz <rsalz@openssl.org>
---
 ssl/ssl_locl.h           | 4 ++++
 ssl/statem/statem_clnt.c | 7 +++----
 ssl/statem/statem_lib.c  | 5 +++++
 ssl/statem/statem_srvr.c | 5 +++--
 ssl/t1_lib.c             | 7 ++++++-
 5 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 31e1fd52dd..63b001ffee 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -349,6 +349,10 @@
 
 /* Check if an SSL structure is using DTLS */
 # define SSL_IS_DTLS(s)  (s->method->ssl3_enc->enc_flags & SSL_ENC_FLAG_DTLS)
+
+/* Check if we are using TLSv1.3 */
+# define SSL_IS_TLS13(s) (!SSL_IS_DTLS(s) && (s)->version >= TLS1_3_VERSION)
+
 /* See if we need explicit IV */
 # define SSL_USE_EXPLICIT_IV(s)  \
                 (s->method->ssl3_enc->enc_flags & SSL_ENC_FLAG_EXPLICIT_IV)
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 004383c413..e0d53fe33b 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -703,6 +703,7 @@ int tls_construct_client_hello(SSL *s, WPACKET *pkt)
     SSL_COMP *comp;
 #endif
     SSL_SESSION *sess = s->session;
+    int client_version;
 
     if (!WPACKET_set_max_size(pkt, SSL3_RT_MAX_PLAIN_LENGTH)) {
         /* Should not happen */
@@ -783,10 +784,8 @@ int tls_construct_client_hello(SSL *s, WPACKET *pkt)
      * For TLS 1.3 we always set the ClientHello version to 1.2 and rely on the
      * supported_versions extension for the real supported versions.
      */
-    if (!WPACKET_put_bytes_u16(pkt,
-                               (!SSL_IS_DTLS(s)
-                                   && s->client_version >= TLS1_3_VERSION)
-                               ? TLS1_2_VERSION : s->client_version)
+    client_version = SSL_IS_TLS13(s) ? TLS1_2_VERSION : s->client_version;
+    if (!WPACKET_put_bytes_u16(pkt, client_version)
             || !WPACKET_memcpy(pkt, s->s3->client_random, SSL3_RANDOM_SIZE)) {
         SSLerr(SSL_F_TLS_CONSTRUCT_CLIENT_HELLO, ERR_R_INTERNAL_ERROR);
         return 0;
diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index 15dc6fd35b..46ffb47e10 100644
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -1044,6 +1044,11 @@ int ssl_choose_server_version(SSL *s, CLIENTHELLO_MSG *hello)
             /* TODO(TLS1.3): Remove this before release */
             if (candidate_vers == TLS1_3_VERSION_DRAFT)
                 candidate_vers = TLS1_3_VERSION;
+            /*
+             * TODO(TLS1.3): There is some discussion on the TLS list about
+             * wheter to ignore versions <TLS1.2 in supported_versions. At the
+             * moment we honour them if present. To be reviewed later
+             */
             if ((int)candidate_vers > s->client_version)
                 s->client_version = candidate_vers;
             if (version_cmp(s, candidate_vers, best_vers) <= 0)
diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index a33362d57f..ba3457d2e0 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -1546,10 +1546,11 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt)
 {
     int compm, al = SSL_AD_INTERNAL_ERROR;
     size_t sl, len;
+    int version;
 
     /* TODO(TLS1.3): Remove the DRAFT conditional before release */
-    if (!WPACKET_put_bytes_u16(pkt, (s->version == TLS1_3_VERSION)
-                                    ? TLS1_3_VERSION_DRAFT : s->version)
+    version = SSL_IS_TLS13(s) ? TLS1_3_VERSION_DRAFT : s->version;
+    if (!WPACKET_put_bytes_u16(pkt, version)
                /*
                 * Random stuff. Filling of the server_random takes place in
                 * tls_process_client_hello()
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index de941b7f0d..e79c37eee0 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1371,7 +1371,7 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al)
         return 0;
     }
 
-    if (!SSL_IS_DTLS(s) && s->version >= TLS1_3_VERSION) {
+    if (SSL_IS_TLS13(s)) {
         int min_version, max_version, reason, currv;
         if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_supported_versions)
                 || !WPACKET_start_sub_packet_u16(pkt)
@@ -1384,6 +1384,11 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al)
             SSLerr(SSL_F_SSL_ADD_CLIENTHELLO_TLSEXT, reason);
             return 0;
         }
+        /*
+         * TODO(TLS1.3): There is some discussion on the TLS list as to wheter
+         * we should include versions <TLS1.2. For the moment we do. To be
+         * reviewed later.
+         */
         for (currv = max_version; currv >= min_version; currv--) {
             /* TODO(TLS1.3): Remove this first if clause prior to release!! */
             if (currv == TLS1_3_VERSION) {
-- 
2.25.1