Sanity tests of inputs to ssl_version_cmp

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

diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index a10d350f9224beadd3842eff744d2244d3035552..8f85ef9d3369467d6a0b06f0fcbeafda1b22d228 100644 (file)
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -4351,8 +4351,9 @@ const SSL_CIPHER *ssl3_choose_cipher(SSL_CONNECTION *s, STACK_OF(SSL_CIPHER) *cl
         maxversion = SSL_CONNECTION_IS_DTLS(s) ? c->max_dtls : c->max_tls;
 
         /* Skip ciphers not supported by the protocol version */
-        if (ssl_version_cmp(s, s->version, minversion) < 0
-            || ssl_version_cmp(s, s->version, maxversion) > 0)
+        if (minversion <= 0 || maxversion <= 0
+                || ssl_version_cmp(s, s->version, minversion) < 0
+                || ssl_version_cmp(s, s->version, maxversion) > 0)
             continue;
 
         /*

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index b88251623a47571e6887f5597d12cc35236f8a5c..00a4359bc995d52835113b32356446e5d5e2aa90 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -4125,7 +4125,8 @@ int ssl_cipher_list_to_bytes(SSL_CONNECTION *s, STACK_OF(SSL_CIPHER) *sk,
             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
+            if (maxproto > 0 && minproto > 0
+                    && ssl_version_cmp(s, maxproto, s->s3.tmp.max_ver) >= 0
                     && ssl_version_cmp(s, minproto, s->s3.tmp.max_ver) <= 0)
                 maxverok = 1;
         }

diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index 98c3b45a058ad7e6950c6501adbf0126e4aa00ab..88b252a32359252958ec0f02eec5cceac6ab74e3 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -157,12 +157,13 @@ 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 negotiated_minversion;
-        int md5sha1_needed_maxversion = SSL_CONNECTION_IS_DTLS(s)
-                                        ? DTLS1_VERSION : TLS1_1_VERSION;
+        const int version1_2 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_2_VERSION
+                                                         : TLS1_2_VERSION;
+        const int version1_1 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_VERSION
+                                                         : TLS1_1_VERSION;
 
         /* We don't have MD5-SHA1 - do we need it? */
-        if (ssl_version_cmp(s, ver_max, md5sha1_needed_maxversion) <= 0) {
+        if (ssl_version_cmp(s, ver_max, version1_1) <= 0) {
             SSLfatal_data(s, SSL_AD_HANDSHAKE_FAILURE,
                           SSL_R_NO_SUITABLE_DIGEST_ALGORITHM,
                           "The max supported SSL/TLS version needs the"
@@ -175,10 +176,8 @@ int tls_setup_handshake(SSL_CONNECTION *s)
         ok = 1;
 
         /* Don't allow TLSv1.1 or below to be negotiated */
-        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 (ssl_version_cmp(s, ver_min, version1_2) < 0)
+                ok = SSL_set_min_proto_version(ssl, version1_2);
         if (!ok) {
             /* Shouldn't happen */
             SSLfatal(s, SSL_AD_HANDSHAKE_FAILURE, ERR_R_INTERNAL_ERROR);
@@ -203,7 +202,8 @@ int tls_setup_handshake(SSL_CONNECTION *s)
             int cipher_maxprotover = SSL_CONNECTION_IS_DTLS(s)
                                      ? c->max_dtls : c->max_tls;
 
-            if (ssl_version_cmp(s, ver_max, cipher_minprotover) >= 0
+            if (cipher_minprotover > 0 && cipher_maxprotover > 0
+                    && ssl_version_cmp(s, ver_max, cipher_minprotover) >= 0
                     && ssl_version_cmp(s, ver_max, cipher_maxprotover) <= 0) {
                 ok = 1;
                 break;
@@ -1798,6 +1798,9 @@ int ssl_version_cmp(const SSL_CONNECTION *s, int versiona, int versionb)
 {
     int dtls = SSL_CONNECTION_IS_DTLS(s);
 
+    if (!ossl_assert(versiona > 0) || !ossl_assert(versionb > 0))
+        return 0;
+
     if (versiona == versionb)
         return 0;
     if (!dtls)
@@ -2159,6 +2162,9 @@ int ssl_choose_server_version(SSL_CONNECTION *s, CLIENTHELLO_MSG *hello,
     const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION
                                                      : TLS1_3_VERSION;
 
+    if (client_version <= 0)
+        return SSL_R_WRONG_SSL_VERSION;
+
     s->client_version = client_version;
 
     switch (server_version) {
@@ -2223,7 +2229,9 @@ 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 (ssl_version_cmp(s, candidate_vers, best_vers) <= 0)
+            if (candidate_vers <= 0
+                    || (best_vers != 0
+                        && ssl_version_cmp(s, candidate_vers, best_vers) <= 0))
                 continue;
             if (ssl_version_supported(s, candidate_vers, &best_method))
                 best_vers = candidate_vers;

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 96570e3dd90945e25f741a1e3470495348edad0e..8e0795553b292e5ff7828e058bb99a386c90deeb 100644 (file)
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -2236,8 +2236,9 @@ int ssl_cipher_disabled(const SSL_CONNECTION *s, const SSL_CIPHER *c,
             && (c->algorithm_mkey & (SSL_kECDHE | SSL_kECDHEPSK)) != 0)
         minversion = SSL3_VERSION;
 
-    if (ssl_version_cmp(s, minversion, s->s3.tmp.max_ver) > 0
-        || ssl_version_cmp(s, maxversion, s->s3.tmp.min_ver) < 0)
+    if (minversion <= 0 || maxversion <= 0
+            || 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);