Fix SSL_select_next_proto()

SSL_select_next_proto() is already quite broken by its design: const in,
non-const out, with the intention of pointing somewhere inside of the two
input pointers. A length returned in an unsigned char (because, you know,
the individual protocols are encoded in Pascal strings). Can't signal
uailure either. It also has an unreachable public return code.

Also, due to originally catering to NPN, this function opportunistically
selects a protocol from the second input (client) parameters, which makes
little sense for ALPN since that means the server falls back to a protocol
it doesn't (want to) support. If there's no overlap, it's the callback's
job to signal error to its caller for ALPN.

As if that wasn't enough misdesign and bugs, the one we're concerned with
here wasn't reported to us twice in ten years is that if you pass this API
a zero-length (or a sufficiently malformed client protocol list), it would
return a pointer pointing somewhere into the heap instead into one of the
two input pointers. This pointer could then be interpreted as a Pascal
string, resulting in an information disclosure of up to 255 bytes from the
heap to the peer, or a crash.

This can only happen for NPN (where it does happen in old python and node).

A long time ago jsing removed NPN support from LibreSSL, because it had
an utter garbage implementation and because it was practically unused.
First it was already replaced by the somewhat less bad ALPN, and the only
users were the always same language bindings that tend to use every feature
they shouldn't use. There were a lot of complaints due to failing test
cases in there, but in the end the decision turned out to be the right
one: the consequence is that LibreSSL isn't vulnerable to CVE-2024-5535.

Still, there is a bug here to fix. It is completely straightforward to
do so. Rewrite this mess using CBS, preserving the current behavior.
Also, we do not follow BoringSSL's renaming of the variables. It would
result in confusing code in almost all alpn callbacks I've seen in the
wild. The only exception is the accidental example of Qt.

ok jsing

1 files changed, 54 insertions, 29 deletions

diff --git a/src/lib/libssl/ssl_lib.c b/src/lib/libssl/ssl_lib.c
index d1b552d94f..406567b535 100644
--- a/src/lib/libssl/ssl_lib.c
+++ b/src/lib/libssl/ssl_lib.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_lib.c,v 1.323 2024/04/15 16:00:05 tb Exp $ */
+/* $OpenBSD: ssl_lib.c,v 1.324 2024/06/28 14:46:19 tb Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1785,45 +1785,70 @@ LSSL_ALIAS(SSL_get_servername_type);
  * It returns either:
  * OPENSSL_NPN_NEGOTIATED if a common protocol was found, or
  * OPENSSL_NPN_NO_OVERLAP if the fallback case was reached.
+ *
+ * XXX - the out argument points into server_list or client list and should
+ * therefore really be const. We can't fix that without breaking the callers.
  */
 int
 SSL_select_next_proto(unsigned char **out, unsigned char *outlen,
-    const unsigned char *server, unsigned int server_len,
-    const unsigned char *client, unsigned int client_len)
+    const unsigned char *server_list, unsigned int server_list_len,
+    const unsigned char *client_list, unsigned int client_list_len)
 {
-        unsigned int             i, j;
-        const unsigned char     *result;
-        int                      status = OPENSSL_NPN_UNSUPPORTED;
+        CBS client, client_proto, server, server_proto;
+
+        *out = NULL;
+        *outlen = 0;
+
+        /* First check that the client list is well-formed. */
+        CBS_init(&client, client_list, client_list_len);
+        if (!tlsext_alpn_check_format(&client))
+                goto err;
+
+        /*
+         * Use first client protocol as fallback. This is one way of doing NPN's
+         * "opportunistic" protocol selection (see security considerations in
+         * draft-agl-tls-nextprotoneg-04), and it is the documented behavior of
+         * this API. For ALPN it's the callback's responsibility to fail on
+         * OPENSSL_NPN_NO_OVERLAP.
+         */
+
+        if (!CBS_get_u8_length_prefixed(&client, &client_proto))
+                goto err;
+
+        *out = (unsigned char *)CBS_data(&client_proto);
+        *outlen = CBS_len(&client_proto);
+
+        /* Now check that the server list is well-formed. */
+        CBS_init(&server, server_list, server_list_len);
+        if (!tlsext_alpn_check_format(&server))
+                goto err;
 
         /*
-         * For each protocol in server preference order,
-         * see if we support it.
+         * Walk the server list and select the first protocol that appears in
+         * the client list.
          */
-        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;
+        while (CBS_len(&server) > 0) {
+                if (!CBS_get_u8_length_prefixed(&server, &server_proto))
+                        goto err;
+
+                CBS_init(&client, client_list, client_list_len);
+
+                while (CBS_len(&client) > 0) {
+                        if (!CBS_get_u8_length_prefixed(&client, &client_proto))
+                                goto err;
+
+                        if (CBS_mem_equal(&client_proto,
+                            CBS_data(&server_proto), CBS_len(&server_proto))) {
+                                *out = (unsigned char *)CBS_data(&server_proto);
+                                *outlen = CBS_len(&server_proto);
+
+                                return OPENSSL_NPN_NEGOTIATED;
                         }
-                        j += client[j];
-                        j++;
                 }
-                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);
+ err:
+        return OPENSSL_NPN_NO_OVERLAP;
 }
 LSSL_ALIAS(SSL_select_next_proto);