Improve SNI hostname validation.

For some time now we've validated the hostname provided to the server in
the SNI extension. Per RFC 6066, an IP literal is invalid as a hostname -
the current code rejects IPv6 literals, but allows IPv4 literals through.

Improve this check to explicitly detect both IPv4 and IPv6 literals. Some
software has been historically known to include IP literals in SNI, so
rather than rejecting this outright (and failing with a decode error),
pretend that the SNI extension does not exist (such that we do not break
some older clients).

ok inoguchi@ tb@

2 files changed, 54 insertions, 9 deletions

diff --git a/lib/libssl/ssl_tlsext.c b/lib/libssl/ssl_tlsext.c
index d8143ce1be7..3da8ebc46c6 100644
--- a/lib/libssl/ssl_tlsext.c
+++ b/lib/libssl/ssl_tlsext.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_tlsext.c,v 1.100 2021/10/25 10:01:46 jsing Exp $ */
+/* $OpenBSD: ssl_tlsext.c,v 1.101 2021/11/01 16:37:17 jsing Exp $ */
 /*
  * Copyright (c) 2016, 2017, 2019 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -17,6 +17,11 @@
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
+#include <sys/socket.h>
+
+#include <arpa/inet.h>
+#include <netinet/in.h>
+
 #include <ctype.h>
 
 #include <openssl/ocsp.h>
@@ -673,6 +678,29 @@ tlsext_sni_client_build(SSL *s, uint16_t msg_type, CBB *cbb)
 	return 1;
 }
 
+static int
+tlsext_sni_is_ip_literal(CBS *cbs, int *is_ip)
+{
+	union {
+		struct in_addr ip4;
+		struct in6_addr ip6;
+	} addrbuf;
+	char *hostname = NULL;
+
+	*is_ip = 0;
+
+	if (!CBS_strdup(cbs, &hostname))
+		return 0;
+
+	if (inet_pton(AF_INET, hostname, &addrbuf) == 1 ||
+	    inet_pton(AF_INET6, hostname, &addrbuf) == 1)
+		*is_ip = 1;
+
+	free(hostname);
+
+	return 1;
+}
+
 /*
  * Validate that the CBS contains only a hostname consisting of RFC 5890
  * compliant A-labels (see RFC 6066 section 3). Not a complete check
@@ -680,7 +708,7 @@ tlsext_sni_client_build(SSL *s, uint16_t msg_type, CBB *cbb)
  * correct structure and character set.
  */
 int
-tlsext_sni_is_valid_hostname(CBS *cbs)
+tlsext_sni_is_valid_hostname(CBS *cbs, int *is_ip)
 {
 	uint8_t prev, c = 0;
 	int component = 0;
@@ -691,7 +719,13 @@ tlsext_sni_is_valid_hostname(CBS *cbs)
 	if (CBS_len(&hostname) > TLSEXT_MAXLEN_host_name)
 		return 0;
 
-	while(CBS_len(&hostname) > 0) {
+	/* An IP literal is invalid as a host name (RFC 6066 section 3). */
+	if (!tlsext_sni_is_ip_literal(&hostname, is_ip))
+		return 0;
+	if (*is_ip)
+		return 0;
+
+	while (CBS_len(&hostname) > 0) {
 		prev = c;
 		if (!CBS_get_u8(&hostname, &c))
 			return 0;
@@ -727,12 +761,14 @@ tlsext_sni_server_parse(SSL *s, uint16_t msg_type, CBS *cbs, int *alert)
 {
 	CBS server_name_list, host_name;
 	uint8_t name_type;
+	int is_ip;
 
 	if (!CBS_get_u16_length_prefixed(cbs, &server_name_list))
 		goto err;
 
 	if (!CBS_get_u8(&server_name_list, &name_type))
 		goto err;
+
 	/*
 	 * RFC 6066 section 3, only one type (host_name) is specified.
 	 * We do not tolerate unknown types, neither does BoringSSL.
@@ -743,17 +779,25 @@ tlsext_sni_server_parse(SSL *s, uint16_t msg_type, CBS *cbs, int *alert)
 		goto err;
 	}
 
-
-	if (!CBS_get_u16_length_prefixed(&server_name_list, &host_name))
-		goto err;
 	/*
 	 * RFC 6066 section 3 specifies a host name must be at least 1 byte
 	 * so 0 length is a decode error.
 	 */
+	if (!CBS_get_u16_length_prefixed(&server_name_list, &host_name))
+		goto err;
 	if (CBS_len(&host_name) < 1)
 		goto err;
 
-	if (!tlsext_sni_is_valid_hostname(&host_name)) {
+	if (!tlsext_sni_is_valid_hostname(&host_name, &is_ip)) {
+		/*
+		 * Various pieces of software have been known to set the SNI
+		 * host name to an IP address, even though that violates the
+		 * RFC. If this is the case, pretend the SNI extension does
+		 * not exist.
+		 */
+		if (is_ip)
+			goto done;
+
 		*alert = SSL_AD_ILLEGAL_PARAMETER;
 		goto err;
 	}
@@ -777,6 +821,7 @@ tlsext_sni_server_parse(SSL *s, uint16_t msg_type, CBS *cbs, int *alert)
 		}
 	}
 
+ done:
 	/*
 	 * RFC 6066 section 3 forbids multiple host names with the same type,
 	 * therefore we allow only one entry.
diff --git a/lib/libssl/ssl_tlsext.h b/lib/libssl/ssl_tlsext.h
index 8e0742aa2ca..b4c135fdf1c 100644
--- a/lib/libssl/ssl_tlsext.h
+++ b/lib/libssl/ssl_tlsext.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_tlsext.h,v 1.26 2020/10/11 01:13:04 guenther Exp $ */
+/* $OpenBSD: ssl_tlsext.h,v 1.27 2021/11/01 16:37:17 jsing Exp $ */
 /*
  * Copyright (c) 2016, 2017 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -60,7 +60,7 @@ int tlsext_sni_client_parse(SSL *s, uint16_t msg_type, CBS *cbs, int *alert);
 int tlsext_sni_server_needs(SSL *s, uint16_t msg_type);
 int tlsext_sni_server_build(SSL *s, uint16_t msg_type, CBB *cbb);
 int tlsext_sni_server_parse(SSL *s, uint16_t msg_type, CBS *cbs, int *alert);
-int tlsext_sni_is_valid_hostname(CBS *cbs);
+int tlsext_sni_is_valid_hostname(CBS *cbs, int *is_ip);
 
 int tlsext_supportedgroups_client_needs(SSL *s, uint16_t msg_type);
 int tlsext_supportedgroups_client_build(SSL *s, uint16_t msg_type, CBB *cbb);