Address some supported_versions review comments
authorMatt Caswell <matt@openssl.org>
Wed, 9 Nov 2016 14:43:05 +0000 (14:43 +0000)
committerMatt Caswell <matt@openssl.org>
Wed, 9 Nov 2016 16:03:09 +0000 (16:03 +0000)
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
ssl/statem/statem_clnt.c
ssl/statem/statem_lib.c
ssl/statem/statem_srvr.c
ssl/t1_lib.c

index 31e1fd5..63b001f 100644 (file)
 
 /* 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)
index 004383c..e0d53fe 100644 (file)
@@ -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;
index 15dc6fd..46ffb47 100644 (file)
@@ -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)
index a33362d..ba3457d 100644 (file)
@@ -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()
index de941b7..e79c37e 100644 (file)
@@ -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) {