Simplify ssl protocol version comparisons.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/23163)

diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index e8ec98c221540be7d5537302d7fb3bfe9ab6c80c..d1497b115bc0c91a111cf70d5860406716b9a8d3 100644 (file)
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -4287,15 +4287,15 @@ const SSL_CIPHER *ssl3_choose_cipher(SSL_CONNECTION *s, STACK_OF(SSL_CIPHER) *cl
     }
 
     for (i = 0; i < sk_SSL_CIPHER_num(prio); i++) {
+        int minversion, maxversion;
+
         c = sk_SSL_CIPHER_value(prio, i);
+        minversion = SSL_CONNECTION_IS_DTLS(s) ? c->min_dtls : c->min_tls;
+        maxversion = SSL_CONNECTION_IS_DTLS(s) ? c->max_dtls : c->max_tls;
 
         /* Skip ciphers not supported by the protocol version */
-        if (!SSL_CONNECTION_IS_DTLS(s) &&
-            ((s->version < c->min_tls) || (s->version > c->max_tls)))
-            continue;
-        if (SSL_CONNECTION_IS_DTLS(s) &&
-            (DTLS_VERSION_LT(s->version, c->min_dtls) ||
-             DTLS_VERSION_GT(s->version, c->max_dtls)))
+        if (ssl_version_cmp(s, s->version, minversion) < 0
+            || ssl_version_cmp(s, s->version, maxversion) > 0)
             continue;
 
         /*

diff --git a/ssl/ssl_local.h b/ssl/ssl_local.h
index 164ec57b2e18c3b53f85ff014c4fae22fe06e539..4e99ada22cdf249d2eccbecc3e021e85302e877a 100644 (file)
--- a/ssl/ssl_local.h
+++ b/ssl/ssl_local.h
@@ -2629,6 +2629,7 @@ __owur int ssl3_handshake_write(SSL_CONNECTION *s);
 
 __owur int ssl_allow_compression(SSL_CONNECTION *s);
 
+__owur int ssl_version_cmp(const SSL_CONNECTION *s, int versiona, int versionb);
 __owur int ssl_version_supported(const SSL_CONNECTION *s, int version,
                                  const SSL_METHOD **meth);
 

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index caf9d7d11c0126fec1bb8f8a4d12da42595aeea6..922f8a1119e18323cb51b2e87797a9f309efce49 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -4119,15 +4119,12 @@ int ssl_cipher_list_to_bytes(SSL_CONNECTION *s, STACK_OF(SSL_CIPHER) *sk,
 
         /* Sanity check that the maximum version we offer has ciphers enabled */
         if (!maxverok) {
-            if (SSL_CONNECTION_IS_DTLS(s)) {
-                if (DTLS_VERSION_GE(c->max_dtls, s->s3.tmp.max_ver)
-                        && DTLS_VERSION_LE(c->min_dtls, s->s3.tmp.max_ver))
-                    maxverok = 1;
-            } else {
-                if (c->max_tls >= s->s3.tmp.max_ver
-                        && c->min_tls <= s->s3.tmp.max_ver)
-                    maxverok = 1;
-            }
+            int minproto = SSL_CONNECTION_IS_DTLS(s) ? c->min_dtls : c->min_tls;
+            int maxproto = SSL_CONNECTION_IS_DTLS(s) ? c->max_dtls : c->max_tls;
+
+            if (ssl_version_cmp(s, maxproto, s->s3.tmp.max_ver) >= 0
+                    && ssl_version_cmp(s, minproto, s->s3.tmp.max_ver) <= 0)
+                maxverok = 1;
         }
 
         totlen += len;

diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index 5693a1269df2c7f1f40220684cf32145f02f8ebf..16b5f590a0b1bceee82e7bd4b511606d2683cc7f 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -156,17 +156,12 @@ int tls_setup_handshake(SSL_CONNECTION *s)
 
     /* Sanity check that we have MD5-SHA1 if we need it */
     if (sctx->ssl_digest_methods[SSL_MD_MD5_SHA1_IDX] == NULL) {
-        int md5sha1_needed = 0;
+        int negotiated_minversion;
+        int md5sha1_needed_maxversion = SSL_CONNECTION_IS_DTLS(s)
+                                        ? DTLS1_VERSION : TLS1_1_VERSION;
 
         /* We don't have MD5-SHA1 - do we need it? */
-        if (SSL_CONNECTION_IS_DTLS(s)) {
-            if (DTLS_VERSION_LE(ver_max, DTLS1_VERSION))
-                md5sha1_needed = 1;
-        } else {
-            if (ver_max <= TLS1_1_VERSION)
-                md5sha1_needed = 1;
-        }
-        if (md5sha1_needed) {
+        if (ssl_version_cmp(s, ver_max, md5sha1_needed_maxversion) <= 0) {
             SSLfatal_data(s, SSL_AD_HANDSHAKE_FAILURE,
                           SSL_R_NO_SUITABLE_DIGEST_ALGORITHM,
                           "The max supported SSL/TLS version needs the"
@@ -177,14 +172,12 @@ int tls_setup_handshake(SSL_CONNECTION *s)
         }
 
         ok = 1;
+
         /* Don't allow TLSv1.1 or below to be negotiated */
-        if (SSL_CONNECTION_IS_DTLS(s)) {
-            if (DTLS_VERSION_LT(ver_min, DTLS1_2_VERSION))
-                ok = SSL_set_min_proto_version(ssl, DTLS1_2_VERSION);
-        } else {
-            if (ver_min < TLS1_2_VERSION)
-                ok = SSL_set_min_proto_version(ssl, TLS1_2_VERSION);
-        }
+        negotiated_minversion = SSL_CONNECTION_IS_DTLS(s) ?
+                                DTLS1_2_VERSION : TLS1_2_VERSION;
+        if (ssl_version_cmp(s, ver_min, negotiated_minversion) < 0)
+                ok = SSL_set_min_proto_version(ssl, negotiated_minversion);
         if (!ok) {
             /* Shouldn't happen */
             SSLfatal(s, SSL_AD_HANDSHAKE_FAILURE, ERR_R_INTERNAL_ERROR);
@@ -204,16 +197,16 @@ int tls_setup_handshake(SSL_CONNECTION *s)
          */
         for (i = 0; i < sk_SSL_CIPHER_num(ciphers); i++) {
             const SSL_CIPHER *c = sk_SSL_CIPHER_value(ciphers, i);
+            int cipher_minprotover = SSL_CONNECTION_IS_DTLS(s)
+                                     ? c->min_dtls : c->min_tls;
+            int cipher_maxprotover = SSL_CONNECTION_IS_DTLS(s)
+                                     ? c->max_dtls : c->max_tls;
 
-            if (SSL_CONNECTION_IS_DTLS(s)) {
-                if (DTLS_VERSION_GE(ver_max, c->min_dtls) &&
-                        DTLS_VERSION_LE(ver_max, c->max_dtls))
-                    ok = 1;
-            } else if (ver_max >= c->min_tls && ver_max <= c->max_tls) {
+            if (ssl_version_cmp(s, ver_max, cipher_minprotover) >= 0
+                    && ssl_version_cmp(s, ver_max, cipher_maxprotover) <= 0) {
                 ok = 1;
-            }
-            if (ok)
                 break;
+            }
         }
         if (!ok) {
             SSLfatal_data(s, SSL_AD_HANDSHAKE_FAILURE,
@@ -1788,15 +1781,23 @@ int ssl_allow_compression(SSL_CONNECTION *s)
     return ssl_security(s, SSL_SECOP_COMPRESSION, 0, 0, NULL);
 }
 
-static int version_cmp(const SSL_CONNECTION *s, int a, int b)
+/*
+ * SSL/TLS/DTLS version comparison
+ *
+ * Returns
+ *      0 if versiona is equal to versionb
+ *      1 if versiona is greater than versionb
+ *     -1 if versiona is less than versionb
+ */
+int ssl_version_cmp(const SSL_CONNECTION *s, int versiona, int versionb)
 {
     int dtls = SSL_CONNECTION_IS_DTLS(s);
 
-    if (a == b)
+    if (versiona == versionb)
         return 0;
     if (!dtls)
-        return a < b ? -1 : 1;
-    return DTLS_VERSION_LT(a, b) ? -1 : 1;
+        return versiona < versionb ? -1 : 1;
+    return DTLS_VERSION_LT(versiona, versionb) ? -1 : 1;
 }
 
 typedef struct {
@@ -1873,12 +1874,12 @@ static int ssl_method_error(const SSL_CONNECTION *s, const SSL_METHOD *method)
     int version = method->version;
 
     if ((s->min_proto_version != 0 &&
-         version_cmp(s, version, s->min_proto_version) < 0) ||
+        ssl_version_cmp(s, version, s->min_proto_version) < 0) ||
         ssl_security(s, SSL_SECOP_VERSION, 0, version, NULL) == 0)
         return SSL_R_VERSION_TOO_LOW;
 
     if (s->max_proto_version != 0 &&
-        version_cmp(s, version, s->max_proto_version) > 0)
+        ssl_version_cmp(s, version, s->max_proto_version) > 0)
         return SSL_R_VERSION_TOO_HIGH;
 
     if ((s->options & method->mask) != 0)
@@ -1966,7 +1967,7 @@ int ssl_version_supported(const SSL_CONNECTION *s, int version,
     switch (SSL_CONNECTION_GET_SSL(s)->method->version) {
     default:
         /* Version should match method version for non-ANY method */
-        return version_cmp(s, version, s->version) == 0;
+        return ssl_version_cmp(s, version, s->version) == 0;
     case TLS_ANY_VERSION:
         table = tls_version_table;
         break;
@@ -1976,10 +1977,10 @@ int ssl_version_supported(const SSL_CONNECTION *s, int version,
     }
 
     for (vent = table;
-         vent->version != 0 && version_cmp(s, version, vent->version) <= 0;
+         vent->version != 0 && ssl_version_cmp(s, version, vent->version) <= 0;
          ++vent) {
         if (vent->cmeth != NULL
-                && version_cmp(s, version, vent->version) == 0
+                && ssl_version_cmp(s, version, vent->version) == 0
                 && ssl_method_error(s, vent->cmeth()) == 0
                 && (!s->server
                     || version != TLS1_3_VERSION
@@ -2153,7 +2154,7 @@ int ssl_choose_server_version(SSL_CONNECTION *s, CLIENTHELLO_MSG *hello,
     switch (server_version) {
     default:
         if (!SSL_CONNECTION_IS_TLS13(s)) {
-            if (version_cmp(s, client_version, s->version) < 0)
+            if (ssl_version_cmp(s, client_version, s->version) < 0)
                 return SSL_R_WRONG_SSL_VERSION;
             *dgrd = DOWNGRADE_NONE;
             /*
@@ -2210,7 +2211,7 @@ int ssl_choose_server_version(SSL_CONNECTION *s, CLIENTHELLO_MSG *hello,
             return SSL_R_BAD_LEGACY_VERSION;
 
         while (PACKET_get_net_2(&versionslist, &candidate_vers)) {
-            if (version_cmp(s, candidate_vers, best_vers) <= 0)
+            if (ssl_version_cmp(s, candidate_vers, best_vers) <= 0)
                 continue;
             if (ssl_version_supported(s, candidate_vers, &best_method))
                 best_vers = candidate_vers;
@@ -2245,7 +2246,7 @@ int ssl_choose_server_version(SSL_CONNECTION *s, CLIENTHELLO_MSG *hello,
      * If the supported versions extension isn't present, then the highest
      * version we can negotiate is TLSv1.2
      */
-    if (version_cmp(s, client_version, TLS1_3_VERSION) >= 0)
+    if (ssl_version_cmp(s, client_version, TLS1_3_VERSION) >= 0)
         client_version = TLS1_2_VERSION;
 
     /*
@@ -2256,7 +2257,7 @@ int ssl_choose_server_version(SSL_CONNECTION *s, CLIENTHELLO_MSG *hello,
         const SSL_METHOD *method;
 
         if (vent->smeth == NULL ||
-            version_cmp(s, client_version, vent->version) < 0)
+            ssl_version_cmp(s, client_version, vent->version) < 0)
             continue;
         method = vent->smeth();
         if (ssl_method_error(s, method) == 0) {
@@ -2344,13 +2345,8 @@ int ssl_choose_client_version(SSL_CONNECTION *s, int version,
         SSLfatal(s, SSL_AD_PROTOCOL_VERSION, ret);
         return 0;
     }
-    if (SSL_CONNECTION_IS_DTLS(s) ? DTLS_VERSION_LT(s->version, ver_min)
-                                  : s->version < ver_min) {
-        s->version = origv;
-        SSLfatal(s, SSL_AD_PROTOCOL_VERSION, SSL_R_UNSUPPORTED_PROTOCOL);
-        return 0;
-    } else if (SSL_CONNECTION_IS_DTLS(s) ? DTLS_VERSION_GT(s->version, ver_max)
-                                         : s->version > ver_max) {
+    if (ssl_version_cmp(s, s->version, ver_min) < 0
+        || ssl_version_cmp(s, s->version, ver_max) > 0) {
         s->version = origv;
         SSLfatal(s, SSL_AD_PROTOCOL_VERSION, SSL_R_UNSUPPORTED_PROTOCOL);
         return 0;

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 236c1b49a782526b09ffd76451c3993352bfd9ea..de82d2b33a1e621874822311d380fe7229ccf969 100644 (file)
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -849,6 +849,7 @@ int tls_valid_group(SSL_CONNECTION *s, uint16_t group_id,
     const TLS_GROUP_INFO *ginfo = tls1_group_id_lookup(SSL_CONNECTION_GET_CTX(s),
                                                        group_id);
     int ret;
+    int group_minversion, group_maxversion;
 
     if (okfortls13 != NULL)
         *okfortls13 = 0;
@@ -856,27 +857,22 @@ int tls_valid_group(SSL_CONNECTION *s, uint16_t group_id,
     if (ginfo == NULL)
         return 0;
 
-    if (SSL_CONNECTION_IS_DTLS(s)) {
-        if (ginfo->mindtls < 0 || ginfo->maxdtls < 0)
-            return 0;
-        if (ginfo->maxdtls == 0)
-            ret = 1;
-        else
-            ret = DTLS_VERSION_LE(minversion, ginfo->maxdtls);
-        if (ginfo->mindtls > 0)
-            ret &= DTLS_VERSION_GE(maxversion, ginfo->mindtls);
-    } else {
-        if (ginfo->mintls < 0 || ginfo->maxtls < 0)
-            return 0;
-        if (ginfo->maxtls == 0)
-            ret = 1;
-        else
-            ret = (minversion <= ginfo->maxtls);
-        if (ginfo->mintls > 0)
-            ret &= (maxversion >= ginfo->mintls);
+    group_minversion = SSL_CONNECTION_IS_DTLS(s) ? ginfo->mindtls : ginfo->mintls;
+    group_maxversion = SSL_CONNECTION_IS_DTLS(s) ? ginfo->maxdtls : ginfo->maxtls;
+
+    if (group_minversion < 0 || group_maxversion < 0)
+        return 0;
+    if (group_maxversion == 0)
+        ret = 1;
+    else
+        ret = (ssl_version_cmp(s, minversion, group_maxversion) <= 0);
+    if (group_minversion > 0)
+        ret &= (ssl_version_cmp(s, maxversion, group_minversion) >= 0);
+
+    if (!SSL_CONNECTION_IS_DTLS(s)) {
         if (ret && okfortls13 != NULL && maxversion == TLS1_3_VERSION)
-            *okfortls13 = (ginfo->maxtls == 0)
-                          || (ginfo->maxtls >= TLS1_3_VERSION);
+            *okfortls13 = (group_maxversion == 0)
+                          || (group_maxversion >= TLS1_3_VERSION);
     }
     ret &= !isec
            || strcmp(ginfo->algorithm, "EC") == 0
@@ -962,6 +958,7 @@ uint16_t tls1_shared_group(SSL_CONNECTION *s, int nmatch)
     for (k = 0, i = 0; i < num_pref; i++) {
         uint16_t id = pref[i];
         const TLS_GROUP_INFO *inf;
+        int minversion, maxversion;
 
         if (!tls1_in_list(id, supp, num_supp)
                 || !tls_group_allowed(s, id, SSL_SECOP_CURVE_SHARED))
@@ -969,20 +966,17 @@ uint16_t tls1_shared_group(SSL_CONNECTION *s, int nmatch)
         inf = tls1_group_id_lookup(ctx, id);
         if (!ossl_assert(inf != NULL))
             return 0;
-        if (SSL_CONNECTION_IS_DTLS(s)) {
-            if (inf->maxdtls == -1)
-                continue;
-            if ((inf->mindtls != 0 && DTLS_VERSION_LT(s->version, inf->mindtls))
-                    || (inf->maxdtls != 0
-                        && DTLS_VERSION_GT(s->version, inf->maxdtls)))
-                continue;
-        } else {
-            if (inf->maxtls == -1)
-                continue;
-            if ((inf->mintls != 0 && s->version < inf->mintls)
-                    || (inf->maxtls != 0 && s->version > inf->maxtls))
-                continue;
-        }
+
+        minversion = SSL_CONNECTION_IS_DTLS(s)
+                         ? inf->mindtls : inf->mintls;
+        maxversion = SSL_CONNECTION_IS_DTLS(s)
+                         ? inf->maxdtls : inf->maxtls;
+        if (maxversion == -1)
+            continue;
+        if ((minversion != 0 && ssl_version_cmp(s, s->version, minversion) < 0)
+            || (maxversion != 0
+                && ssl_version_cmp(s, s->version, maxversion) > 0))
+            continue;
 
         if (nmatch == k)
             return id;
@@ -2060,6 +2054,9 @@ int ssl_set_client_disabled(SSL_CONNECTION *s)
 int ssl_cipher_disabled(const SSL_CONNECTION *s, const SSL_CIPHER *c,
                         int op, int ecdhe)
 {
+    int minversion = SSL_CONNECTION_IS_DTLS(s) ? c->min_dtls : c->min_tls;
+    int maxversion = SSL_CONNECTION_IS_DTLS(s) ? c->max_dtls : c->max_tls;
+
     if (c->algorithm_mkey & s->s3.tmp.mask_k
         || c->algorithm_auth & s->s3.tmp.mask_a)
         return 1;
@@ -2077,23 +2074,17 @@ int ssl_cipher_disabled(const SSL_CONNECTION *s, const SSL_CIPHER *c,
             return 1;
         }
 
-    if (!SSL_CONNECTION_IS_DTLS(s)) {
-        int min_tls = c->min_tls;
-
-        /*
-         * For historical reasons we will allow ECHDE to be selected by a server
-         * in SSLv3 if we are a client
-         */
-        if (min_tls == TLS1_VERSION && ecdhe
-                && (c->algorithm_mkey & (SSL_kECDHE | SSL_kECDHEPSK)) != 0)
-            min_tls = SSL3_VERSION;
+    /*
+     * For historical reasons we will allow ECHDE to be selected by a server
+     * in SSLv3 if we are a client
+     */
+    if (minversion == TLS1_VERSION
+            && ecdhe
+            && (c->algorithm_mkey & (SSL_kECDHE | SSL_kECDHEPSK)) != 0)
+        minversion = SSL3_VERSION;
 
-        if ((min_tls > s->s3.tmp.max_ver) || (c->max_tls < s->s3.tmp.min_ver))
-            return 1;
-    }
-    if (SSL_CONNECTION_IS_DTLS(s)
-            && (DTLS_VERSION_GT(c->min_dtls, s->s3.tmp.max_ver)
-                || DTLS_VERSION_LT(c->max_dtls, s->s3.tmp.min_ver)))
+    if (ssl_version_cmp(s, minversion, s->s3.tmp.max_ver) > 0
+        || ssl_version_cmp(s, maxversion, s->s3.tmp.min_ver) < 0)
         return 1;
 
     return !ssl_security(s, op, c->strength_bits, 0, (void *)c);