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 |  230 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 207 insertions(+), 23 deletions(-)

--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -160,6 +160,173 @@
 # include <openssl/engine.h>
 #endif
 
+#include <stdint.h>
+
+/* CVE-2024-5535 fix code */
+
+typedef struct {
+    /* Pointer to where we are currently reading from */
+    const unsigned char *curr;
+    /* Number of bytes remaining */
+    size_t remaining;
+} PACKET;
+
+/* Internal unchecked shorthand; don't use outside this file. */
+static inline void packet_forward(PACKET *pkt, size_t len)
+{
+    pkt->curr += len;
+    pkt->remaining -= len;
+}
+
+/*
+ * Returns the number of bytes remaining to be read in the PACKET
+ */
+static inline size_t PACKET_remaining(const PACKET *pkt)
+{
+    return pkt->remaining;
+}
+
+/*
+ * Returns a pointer to the first byte after the packet data.
+ * Useful for integrating with non-PACKET parsing code.
+ * Specifically, we use PACKET_end() to verify that a d2i_... call
+ * has consumed the entire packet contents.
+ */
+static inline const unsigned char *PACKET_end(const PACKET *pkt)
+{
+    return pkt->curr + pkt->remaining;
+}
+
+/*
+ * Returns a pointer to the PACKET's current position.
+ * For use in non-PACKETized APIs.
+ */
+static inline const unsigned char *PACKET_data(const PACKET *pkt)
+{
+    return pkt->curr;
+}
+
+/* Peek ahead at 1 byte from |pkt| and store the value in |*data| */
+static inline int PACKET_peek_1(const PACKET *pkt,
+                                unsigned int *data)
+{
+    if (!PACKET_remaining(pkt))
+        return 0;
+
+    *data = *pkt->curr;
+
+    return 1;
+}
+
+/* Get 1 byte from |pkt| and store the value in |*data| */
+static inline int PACKET_get_1(PACKET *pkt, unsigned int *data)
+{
+    if (!PACKET_peek_1(pkt, data))
+        return 0;
+
+    packet_forward(pkt, 1);
+
+    return 1;
+}
+
+/*
+ * Peek ahead at |len| bytes from the |pkt| and store a pointer to them in
+ * |*data|. This just points at the underlying buffer that |pkt| is using. The
+ * caller should not free this data directly (it will be freed when the
+ * underlying buffer gets freed
+ */
+static inline int PACKET_peek_bytes(const PACKET *pkt,
+                                    const unsigned char **data,
+                                    size_t len)
+{
+    if (PACKET_remaining(pkt) < len)
+        return 0;
+
+    *data = pkt->curr;
+
+    return 1;
+}
+
+/*
+ * Read |len| bytes from the |pkt| and store a pointer to them in |*data|. This
+ * just points at the underlying buffer that |pkt| is using. The caller should
+ * not free this data directly (it will be freed when the underlying buffer gets
+ * freed
+ */
+static inline int PACKET_get_bytes(PACKET *pkt,
+                                   const unsigned char **data,
+                                   size_t len)
+{
+    if (!PACKET_peek_bytes(pkt, data, len))
+        return 0;
+
+    packet_forward(pkt, len);
+
+    return 1;
+}
+
+
+/*
+ * Initialise a PACKET with |len| bytes held in |buf|. This does not make a
+ * copy of the data so |buf| must be present for the whole time that the PACKET
+ * is being used.
+ */
+static inline int PACKET_buf_init(PACKET *pkt,
+                                  const unsigned char *buf,
+                                  size_t len)
+{
+    /* Sanity check for negative values. */
+    if (len > (size_t)(SIZE_MAX / 2))
+        return 0;
+
+    pkt->curr = buf;
+    pkt->remaining = len;
+    return 1;
+}
+
+/*
+ * Reads a variable-length vector prefixed with a one-byte length, and stores
+ * the contents in |subpkt|. |pkt| can equal |subpkt|.
+ * Data is not copied: the |subpkt| packet will share its underlying buffer with
+ * the original |pkt|, so data wrapped by |pkt| must outlive the |subpkt|.
+ * Upon failure, the original |pkt| and |subpkt| are not modified.
+ */
+static inline int PACKET_get_length_prefixed_1(PACKET *pkt,
+                                                    PACKET *subpkt)
+{
+    unsigned int length;
+    const unsigned char *data;
+    PACKET tmp = *pkt;
+    if (!PACKET_get_1(&tmp, &length) ||
+        !PACKET_get_bytes(&tmp, &data, (size_t)length)) {
+        return 0;
+    }
+
+    *pkt = tmp;
+    subpkt->curr = data;
+    subpkt->remaining = length;
+
+    return 1;
+}
+
+/*
+ * Returns 1 if the packet has length |num| and its contents equal the |num|
+ * bytes read from |ptr|. Returns 0 otherwise (lengths or contents not equal).
+ * If lengths are equal, performs the comparison in constant time.
+ */
+static inline int PACKET_equal(const PACKET *pkt, const void *ptr,
+                                           size_t num)
+{
+    if (PACKET_remaining(pkt) != num)
+        return 0;
+    return CRYPTO_memcmp(pkt->curr, ptr, num) == 0;
+}
+
+
+/* End of CVE-2024-5535 fix code */
+
+
+
 const char *SSL_version_str = OPENSSL_VERSION_TEXT;
 
 SSL3_ENC_METHOD ssl3_undef_enc_method = {
@@ -1659,37 +1826,54 @@ int SSL_select_next_proto(unsigned char
                           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++;
+        /* Ignore spurious trailing bytes in the server list */
         }
-        i += server[i];
-        i++;
     }
 
-    /* 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
