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)

4 files changed, 26 insertions, 15 deletions

diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index d3a48842ca..5b1a3ec3f5 100644
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -4294,8 +4294,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 53b1a116b3..451e555d9d 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -4123,7 +4123,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 0de402f0e1..98ea730c23 100644
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -156,12 +156,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"
@@ -174,10 +175,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);
@@ -202,7 +201,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;
@@ -1791,6 +1791,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)
@@ -2152,6 +2155,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) {
@@ -2216,7 +2222,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 c9e784a08e..d52b4ffe85 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -2100,8 +2100,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);