Enforce that SNI hostnames be correct as per rfc 6066 and 5980.

Correct SNI alerts to differentiate between illegal parameter
and an unknown name.

ok tb@`

3 files changed, 159 insertions, 18 deletions

diff --git a/src/lib/libssl/ssl_tlsext.c b/src/lib/libssl/ssl_tlsext.c
index f5343c81ab..2184e65a2c 100644
--- a/src/lib/libssl/ssl_tlsext.c
+++ b/src/lib/libssl/ssl_tlsext.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_tlsext.c,v 1.71 2020/05/23 08:47:19 tb Exp $ */
+/* $OpenBSD: ssl_tlsext.c,v 1.72 2020/05/23 17:13:24 beck Exp $ */
 /*
  * Copyright (c) 2016, 2017, 2019 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -18,6 +18,7 @@
  */
 
 #include <openssl/ocsp.h>
+#include <ctype.h>
 
 #include "ssl_locl.h"
 
@@ -672,6 +673,53 @@ tlsext_sni_client_build(SSL *s, CBB *cbb)
         return 1;
 }
 
+/*
+ * Does the CBS contain only of a hostname consisting of RFC 5890
+ * compliant A-labels? (see RFC 6066 section 3). Not a complete check
+ * since we don't parse punycode to verify its validity but limits to
+ * correct structure and character set.
+ */
+int
+tlsext_sni_is_valid_hostname(CBS *cbs)
+{
+        uint8_t prev, c = 0;
+        int component = 0;
+        CBS hostname;
+
+        if (CBS_len(cbs) > TLSEXT_MAXLEN_host_name)
+                return 0;
+
+        CBS_dup(cbs, &hostname);
+        while(CBS_len(&hostname) > 0) {
+                prev = c;
+                if (!CBS_get_u8(&hostname, &c))
+                        return 0;
+                /* Everything has to be ASCII, with no NUL byte. */
+                if (!isascii(c) || c == '\0')
+                        return 0;
+                /* It must be alphanumeric, a '-', or a '.' */
+                if (!(isalnum(c) || c == '-' || c == '.'))
+                        return 0;
+                /* '-' and '.' must not start a component or be at the end. */
+                if (component == 0 || CBS_len(&hostname) == 0) {
+                        if (c == '-' || c == '.')
+                                return 0;
+                }
+                if (c == '.') {
+                        /* Components can not end with a dash. */
+                        if (prev == '-')
+                                return 0;
+                        /* Start new component */
+                        component = 0;
+                        continue;
+                }
+                /* Components must be 63 chars or less. */
+                if (++component > 63)
+                        return 0;
+        }
+        return 1;
+}
+
 int
 tlsext_sni_server_parse(SSL *s, CBS *cbs, int *alert)
 {
@@ -681,52 +729,66 @@ tlsext_sni_server_parse(SSL *s, CBS *cbs, int *alert)
         if (!CBS_get_u16_length_prefixed(cbs, &server_name_list))
                 goto err;
 
-        /*
-         * RFC 6066 section 3 forbids multiple host names with the same type.
-         * Additionally, only one type (host_name) is specified.
-         */
         if (!CBS_get_u8(&server_name_list, &name_type))
                 goto err;
-        if (name_type != TLSEXT_NAMETYPE_host_name)
+        /*
+         * RFC 6066 section 3, only one type (host_name) is specified.
+         * We do not tolerate unknown types, neither does BoringSSL.
+         * other implementations appear more tolerant.
+         */
+        if (name_type != TLSEXT_NAMETYPE_host_name) {
+                *alert = SSL3_AD_ILLEGAL_PARAMETER;
                 goto err;
+        }
+
 
         if (!CBS_get_u16_length_prefixed(&server_name_list, &host_name))
                 goto err;
-        if (CBS_len(&host_name) == 0 ||
-            CBS_len(&host_name) > TLSEXT_MAXLEN_host_name ||
-            CBS_contains_zero_byte(&host_name)) {
-                *alert = TLS1_AD_UNRECOGNIZED_NAME;
-                return 0;
+        /*
+         * RFC 6066 section 3 specifies a host name must be at least 1 byte
+         * so 0 length is a decode error.
+         */
+        if (CBS_len(&host_name) == 0)
+                goto err;
+
+        if (!tlsext_sni_is_valid_hostname(&host_name)) {
+                *alert = SSL3_AD_ILLEGAL_PARAMETER;
+                goto err;
         }
 
         if (s->internal->hit || S3I(s)->hs_tls13.hrr) {
                 if (s->session->tlsext_hostname == NULL) {
                         *alert = TLS1_AD_UNRECOGNIZED_NAME;
-                        return 0;
+                        goto err;
                 }
                 if (!CBS_mem_equal(&host_name, s->session->tlsext_hostname,
                     strlen(s->session->tlsext_hostname))) {
                         *alert = TLS1_AD_UNRECOGNIZED_NAME;
-                        return 0;
+                        goto err;
                 }
         } else {
                 if (s->session->tlsext_hostname != NULL)
                         goto err;
                 if (!CBS_strdup(&host_name, &s->session->tlsext_hostname)) {
                         *alert = TLS1_AD_INTERNAL_ERROR;
-                        return 0;
+                        goto err;
                 }
         }
 
-        if (CBS_len(&server_name_list) != 0)
+        /*
+         * RFC 6066 section 3 forbids multiple host names with the same type,
+         * therefore we allow only one entry.
+         */
+        if (CBS_len(&server_name_list) != 0) {
+                *alert = SSL3_AD_ILLEGAL_PARAMETER;
                 goto err;
+        }
         if (CBS_len(cbs) != 0)
                 goto err;
 
         return 1;
 
  err:
-        *alert = SSL_AD_DECODE_ERROR;
         return 0;
 }
 
diff --git a/src/lib/libssl/ssl_tlsext.h b/src/lib/libssl/ssl_tlsext.h
index aa40f6b1a6..15e0257e63 100644
--- a/src/lib/libssl/ssl_tlsext.h
+++ b/src/lib/libssl/ssl_tlsext.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_tlsext.h,v 1.22 2020/01/25 12:58:27 jsing Exp $ */
+/* $OpenBSD: ssl_tlsext.h,v 1.23 2020/05/23 17:13:24 beck Exp $ */
 /*
  * Copyright (c) 2016, 2017 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -58,6 +58,7 @@ int tlsext_sni_client_parse(SSL *s, CBS *cbs, int *alert);
 int tlsext_sni_server_needs(SSL *s);
 int tlsext_sni_server_build(SSL *s, CBB *cbb);
 int tlsext_sni_server_parse(SSL *s, CBS *cbs, int *alert);
+int tlsext_sni_is_valid_hostname(CBS *cbs);
 
 int tlsext_supportedgroups_client_needs(SSL *s);
 int tlsext_supportedgroups_client_build(SSL *s, CBB *cbb);
diff --git a/src/regress/lib/libssl/tlsext/tlsexttest.c b/src/regress/lib/libssl/tlsext/tlsexttest.c
index 3d03c2c0d3..5cf39e63ae 100644
--- a/src/regress/lib/libssl/tlsext/tlsexttest.c
+++ b/src/regress/lib/libssl/tlsext/tlsexttest.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tlsexttest.c,v 1.36 2020/05/11 18:20:01 jsing Exp $ */
+/* $OpenBSD: tlsexttest.c,v 1.37 2020/05/23 17:13:24 beck Exp $ */
 /*
  * Copyright (c) 2017 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -3544,6 +3544,81 @@ done:
         return (failure);
 }
 
+unsigned char *valid_hostnames[] = {
+        "openbsd.org",
+        "op3nbsd.org",
+        "org",
+        "3openbsd.com",
+        "3-0penb-d.c-m",
+        "a",
+        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.com",
+        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa."
+        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa."
+        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa."
+        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+        NULL,
+};
+
+static int
+test_tlsext_valid_hostnames(void) {
+        int i, failure = 0;
+        for (i = 0; valid_hostnames[i] != NULL; i++) {
+                CBS cbs;
+                CBS_init(&cbs, valid_hostnames[i], strlen(valid_hostnames[i]));
+                if (!tlsext_sni_is_valid_hostname(&cbs)) {
+                        fprintf(stderr, "FAIL: %s\n", valid_hostnames[i]);
+                        FAIL("Valid hostname rejected");
+                        failure = 1;
+                        goto done;
+                }
+        }
+ done:
+        return failure;
+}
+
+unsigned char *invalid_hostnames[] = {
+        "openbsd.org.",
+        "openbsd..org",
+        "openbsd.org-",
+        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.com",
+        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa."
+        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa."
+        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa."
+        "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.a",
+        "-p3nbsd.org",
+        "openbs-.org",
+        "openbsd\n.org",
+        "open_bsd.org",
+        "open\178bsd.org",
+        "open\255bsd.org",
+        NULL,
+};
+
+static int
+test_tlsext_invalid_hostnames(void) {
+        int i, failure = 0;
+        CBS cbs;
+        for (i = 0; invalid_hostnames[i] != NULL; i++) {
+                CBS_init(&cbs, invalid_hostnames[i],
+                    strlen(invalid_hostnames[i]));
+                if (tlsext_sni_is_valid_hostname(&cbs)) {
+                        fprintf(stderr, "%s\n", invalid_hostnames[i]);
+                        FAIL("Invalid hostname accepted");
+                        failure = 1;
+                        goto done;
+                }
+        }
+        CBS_init(&cbs, valid_hostnames[0],
+            strlen(valid_hostnames[0]) + 1);
+        if (tlsext_sni_is_valid_hostname(&cbs)) {
+                FAIL("hostname with NUL byte accepted");
+                failure = 1;
+                goto done;
+        }
+ done:
+        return failure;
+}
+
 
 int
 main(int argc, char **argv)
@@ -3595,5 +3670,8 @@ main(int argc, char **argv)
         failed |= test_tlsext_clienthello_build();
         failed |= test_tlsext_serverhello_build();
 
+        failed |= test_tlsext_valid_hostnames();
+        failed |= test_tlsext_invalid_hostnames();
+
         return (failed);
 }