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 0ebafc669c667d882e25b53b7254f37d671312e0..52a31baaea5f608208278f3750342d80566ad2e2 100644 (file)
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -4500,8 +4500,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 e2191c74550939a24bc197bf1bf9a166fc115fcc..1753010ea9f511126794e78503b14794b8e1f398 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -4201,7 +4201,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 f1bd0fd2fbecc78828ca160cd6ec132a7a9052d2..0126140b6cda96e2ced9676a01afe05604eb22f3 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -158,12 +158,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"
@@ -176,10 +177,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);
@@ -204,7 +203,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;
@@ -1814,6 +1814,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)
@@ -2175,6 +2178,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) {
@@ -2239,7 +2245,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 86a52aaf6934cd97230c2a248031ed96c17758c8..f49d7cff9de4e63ed68782c6f0e54e3fc14c4b2a 100644 (file)
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -2931,8 +2931,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);