From: tb Date: Fri, 28 Jun 2024 14:46:19 +0000 (+0000) Subject: Fix SSL_select_next_proto() X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=0a52d54c17a97c9e091b44cc815c8a172176da08;p=openbsd 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 --- diff --git a/lib/libssl/ssl_lib.c b/lib/libssl/ssl_lib.c index d1b552d94fc..406567b535a 100644 --- a/lib/libssl/ssl_lib.c +++ b/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);