From ce471c0da7f905a6a1c11b47e709a521f8a706af Mon Sep 17 00:00:00 2001 From: beck <> Date: Sat, 23 May 2020 17:13:24 +0000 Subject: 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@` --- src/lib/libssl/ssl_tlsext.c | 94 +++++++++++++++++++++++++----- src/lib/libssl/ssl_tlsext.h | 3 +- src/regress/lib/libssl/tlsext/tlsexttest.c | 80 ++++++++++++++++++++++++- 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 * Copyright (c) 2017 Doug Hogan @@ -18,6 +18,7 @@ */ #include +#include #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 * Copyright (c) 2017 Doug Hogan @@ -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 * Copyright (c) 2017 Doug Hogan @@ -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); } -- cgit v1.2.3-55-g6feb