From 4ada436a1946cbb24db5ab4ca082b69c1bc10f37 Mon Sep 17 00:00:00 2001 From: Matt Caswell 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 Reviewed-by: Tomas Mraz (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 must be in protocol-list format, described below. The length of B is specified in -B. +B. Setting B 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 used by a server to select which protocol to use for the incoming connection. When B @@ -73,9 +74,16 @@ B and B, B, B list that matches an item in the B, B list is selected, and returned in B, B. The B value will point into either B or -B, so it should be copied immediately. If no match is found, the first -item in B, B is returned in B, B. This -function can also be used in the NPN callback. +B, 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, B is returned in B, B and +B is returned. This can be useful when implementating +the NPN callback. In the ALPN case, the value returned in B and B +must be ignored if B has been returned from +SSL_select_next_proto(). SSL_CTX_set_next_proto_select_cb() sets a callback B 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. The server's advertised protocols are provided in B and B. The callback can assume that B is syntactically valid. The client must -select a protocol. It is fatal to the connection if this callback returns -a value other than B. The B 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 or if the zero length protocol is selected. The B +parameter is the pointer set via SSL_CTX_set_next_proto_select_cb(). SSL_CTX_set_next_protos_advertised_cb() sets a callback B 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, B is returned in -B, B. +B, B (or B and 0 in the case where the first entry in +B 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