openssl-1_1/openssl-CVE-2024-5535.patch

From 4ada436a1946cbb24db5ab4ca082b69c1bc10f37 Mon Sep 17 00:00:00 2001
From: Matt Caswell <matt@openssl.org>
Date: Fri, 31 May 2024 11:14:33 +0100
Subject: [PATCH] Fix SSL_select_next_proto

Ensure that the provided client list is non-NULL and starts with a valid
entry. When called from the ALPN callback the client list should already
have been validated by OpenSSL so this should not cause a problem. When
called from the NPN callback the client list is locally configured and
will not have already been validated. Therefore SSL_select_next_proto
should not assume that it is correctly formatted.

We implement stricter checking of the client protocol list. We also do the
same for the server list while we are about it.

CVE-2024-5535

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/24718)
---
 ssl/ssl_lib.c | 63 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 23 deletions(-)

Index: openssl-1.1.1w/ssl/ssl_lib.c
===================================================================
--- openssl-1.1.1w.orig/ssl/ssl_lib.c
+++ openssl-1.1.1w/ssl/ssl_lib.c
@@ -2761,37 +2761,54 @@ int SSL_select_next_proto(unsigned char
                           unsigned int server_len,
                           const unsigned char *client, unsigned int client_len)
 {
-    unsigned int i, j;
-    const unsigned char *result;
-    int status = OPENSSL_NPN_UNSUPPORTED;
+    PACKET cpkt, csubpkt, spkt, ssubpkt;
+
+    if (!PACKET_buf_init(&cpkt, client, client_len)
+            || !PACKET_get_length_prefixed_1(&cpkt, &csubpkt)
+            || PACKET_remaining(&csubpkt) == 0) {
+        *out = NULL;
+        *outlen = 0;
+        return OPENSSL_NPN_NO_OVERLAP;
+    }
+
+    /*
+     * Set the default opportunistic protocol. Will be overwritten if we find
+     * a match.
+     */
+    *out = (unsigned char *)PACKET_data(&csubpkt);
+    *outlen = (unsigned char)PACKET_remaining(&csubpkt);
 
     /*
      * For each protocol in server preference order, see if we support it.
      */
-    for (i = 0; i < server_len;) {
-        for (j = 0; j < client_len;) {
-            if (server[i] == client[j] &&
-                memcmp(&server[i + 1], &client[j + 1], server[i]) == 0) {
-                /* We found a match */
-                result = &server[i];
-                status = OPENSSL_NPN_NEGOTIATED;
-                goto found;
+    if (PACKET_buf_init(&spkt, server, server_len)) {
+        while (PACKET_get_length_prefixed_1(&spkt, &ssubpkt)) {
+            if (PACKET_remaining(&ssubpkt) == 0)
+                continue; /* Invalid - ignore it */
+            if (PACKET_buf_init(&cpkt, client, client_len)) {
+                while (PACKET_get_length_prefixed_1(&cpkt, &csubpkt)) {
+                    if (PACKET_equal(&csubpkt, PACKET_data(&ssubpkt),
+                                     PACKET_remaining(&ssubpkt))) {
+                        /* We found a match */
+                        *out = (unsigned char *)PACKET_data(&ssubpkt);
+                        *outlen = (unsigned char)PACKET_remaining(&ssubpkt);
+                        return OPENSSL_NPN_NEGOTIATED;
+                    }
+                }
+                /* Ignore spurious trailing bytes in the client list */
+            } else {
+                /* This should never happen */
+                return OPENSSL_NPN_NO_OVERLAP;
             }
-            j += client[j];
-            j++;
         }
-        i += server[i];
-        i++;
+        /* Ignore spurious trailing bytes in the server list */
     }
 
-    /* There's no overlap between our protocols and the server's list. */
-    result = client;
-    status = OPENSSL_NPN_NO_OVERLAP;
-
- found:
-    *out = (unsigned char *)result + 1;
-    *outlen = result[0];
-    return status;
+    /*
+     * There's no overlap between our protocols and the server's list. We use
+     * the default opportunistic protocol selected earlier
+     */
+    return OPENSSL_NPN_NO_OVERLAP;
 }
 
 #ifndef OPENSSL_NO_NEXTPROTONEG
Index: openssl-1.1.1w/ssl/statem/extensions_clnt.c
===================================================================
--- openssl-1.1.1w.orig/ssl/statem/extensions_clnt.c
+++ openssl-1.1.1w/ssl/statem/extensions_clnt.c
@@ -1599,7 +1599,8 @@ int tls_parse_stoc_npn(SSL *s, PACKET *p
                                   PACKET_data(pkt),
                                   PACKET_remaining(pkt),
                                   s->ctx->ext.npn_select_cb_arg) !=
-             SSL_TLSEXT_ERR_OK) {
+                                  SSL_TLSEXT_ERR_OK
+            || selected_len == 0) {
         SSLfatal(s, SSL_AD_HANDSHAKE_FAILURE, SSL_F_TLS_PARSE_STOC_NPN,
                  SSL_R_BAD_EXTENSION);
         return 0;
@@ -1630,6 +1631,8 @@ int tls_parse_stoc_alpn(SSL *s, PACKET *
                         size_t chainidx)
 {
     size_t len;
+    PACKET confpkt, protpkt;
+    int valid = 0;
 
     /* We must have requested it. */
     if (!s->s3->alpn_sent) {
@@ -1650,6 +1653,30 @@ int tls_parse_stoc_alpn(SSL *s, PACKET *
                  SSL_R_BAD_EXTENSION);
         return 0;
     }
+
+    /* It must be a protocol that we sent */
+    if (!PACKET_buf_init(&confpkt, s->ext.alpn, s->ext.alpn_len)) {
+        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PARSE_STOC_ALPN,
+                 ERR_R_INTERNAL_ERROR);
+        return 0;
+    }
+    while (PACKET_get_length_prefixed_1(&confpkt, &protpkt)) {
+        if (PACKET_remaining(&protpkt) != len)
+            continue;
+        if (memcmp(PACKET_data(pkt), PACKET_data(&protpkt), len) == 0) {
+            /* Valid protocol found */
+            valid = 1;
+            break;
+        }
+    }
+
+    if (!valid) {
+        /* The protocol sent from the server does not match one we advertised */
+        SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_F_TLS_PARSE_STOC_ALPN,
+                 SSL_R_BAD_EXTENSION);
+        return 0;
+    }
+
     OPENSSL_free(s->s3->alpn_selected);
     s->s3->alpn_selected = OPENSSL_malloc(len);
     if (s->s3->alpn_selected == NULL) {
Index: openssl-1.1.1w/doc/man3/SSL_CTX_set_alpn_select_cb.pod
===================================================================
--- openssl-1.1.1w.orig/doc/man3/SSL_CTX_set_alpn_select_cb.pod
+++ openssl-1.1.1w/doc/man3/SSL_CTX_set_alpn_select_cb.pod
@@ -52,7 +52,8 @@ SSL_select_next_proto, SSL_get0_alpn_sel
 SSL_CTX_set_alpn_protos() and SSL_set_alpn_protos() are used by the client to
 set the list of protocols available to be negotiated. The B<protos> must be in
 protocol-list format, described below. The length of B<protos> is specified in
-B<protos_len>.
+B<protos_len>. Setting B<protos_len> to 0 clears any existing list of ALPN
+protocols and no ALPN extension will be sent to the server.
 
 SSL_CTX_set_alpn_select_cb() sets the application callback B<cb> used by a
 server to select which protocol to use for the incoming connection. When B<cb>
@@ -73,9 +74,16 @@ B<server_len> and B<client>, B<client_le
 described below. The first item in the B<server>, B<server_len> list that
 matches an item in the B<client>, B<client_len> list is selected, and returned
 in B<out>, B<outlen>. The B<out> value will point into either B<server> or
-B<client>, so it should be copied immediately. If no match is found, the first
-item in B<client>, B<client_len> is returned in B<out>, B<outlen>. This
-function can also be used in the NPN callback.
+B<client>, so it should be copied immediately. The client list must include at
+least one valid (nonempty) protocol entry in the list.
+
+The SSL_select_next_proto() helper function can be useful from either the ALPN
+callback or the NPN callback (described below). If no match is found, the first
+item in B<client>, B<client_len> is returned in B<out>, B<outlen> and
+B<OPENSSL_NPN_NO_OVERLAP> is returned. This can be useful when implementating
+the NPN callback. In the ALPN case, the value returned in B<out> and B<outlen>
+must be ignored if B<OPENSSL_NPN_NO_OVERLAP> has been returned from
+SSL_select_next_proto().
 
 SSL_CTX_set_next_proto_select_cb() sets a callback B<cb> that is called when a
 client needs to select a protocol from the server's provided list, and a
@@ -85,9 +93,10 @@ must be set to point to the selected pro
 The length of the protocol name must be written into B<outlen>. The
 server's advertised protocols are provided in B<in> and B<inlen>. The
 callback can assume that B<in> is syntactically valid. The client must
-select a protocol. It is fatal to the connection if this callback returns
-a value other than B<SSL_TLSEXT_ERR_OK>. The B<arg> parameter is the pointer
-set via SSL_CTX_set_next_proto_select_cb().
+select a protocol (although it may be an empty, zero length protocol). It is
+fatal to the connection if this callback returns a value other than
+B<SSL_TLSEXT_ERR_OK> or if the zero length protocol is selected. The B<arg>
+parameter is the pointer set via SSL_CTX_set_next_proto_select_cb().
 
 SSL_CTX_set_next_protos_advertised_cb() sets a callback B<cb> that is called
 when a TLS server needs a list of supported protocols for Next Protocol
@@ -149,7 +158,8 @@ A match was found and is returned in B<o
 =item OPENSSL_NPN_NO_OVERLAP
 
 No match was found. The first item in B<client>, B<client_len> is returned in
-B<out>, B<outlen>.
+B<out>, B<outlen> (or B<NULL> and 0 in the case where the first entry in
+B<client> is invalid).
 
 =back
 
Index: openssl-1.1.1w/ssl/statem/extensions_srvr.c
===================================================================
--- openssl-1.1.1w.orig/ssl/statem/extensions_srvr.c
+++ openssl-1.1.1w/ssl/statem/extensions_srvr.c
@@ -1558,9 +1558,10 @@ EXT_RETURN tls_construct_stoc_next_proto
             return EXT_RETURN_FAIL;
         }
         s->s3->npn_seen = 1;
+        return EXT_RETURN_SENT;
     }
 
-    return EXT_RETURN_SENT;
+    return EXT_RETURN_NOT_SENT;
 }
 #endif