Complete the TLS extension handling rewrite for the server-side.

This removes ssl_parse_clienthello_tlsext() and allows the CBS to be
passed all the way through from ssl3_get_client_hello(). The renegotation
check gets pulled up into ssl3_get_client_hello() which is where other
such checks exist.

The TLS extension parsing now also ensures that we do not get duplicates
of any known extensions (the old pre-rewrite code only did this for some
extensions).

ok inoguchi@

5 files changed, 86 insertions, 98 deletions

diff --git a/lib/libssl/ssl_locl.h b/lib/libssl/ssl_locl.h
index f6e922e99cb..d2a99afaa49 100644
--- a/lib/libssl/ssl_locl.h
+++ b/lib/libssl/ssl_locl.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_locl.h,v 1.201 2017/10/12 16:06:32 jsing Exp $ */
+/* $OpenBSD: ssl_locl.h,v 1.202 2018/01/27 15:30:05 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -164,6 +164,9 @@
 
 __BEGIN_HIDDEN_DECLS
 
+#define CTASSERT(x)	extern char  _ctassert[(x) ? 1 : -1 ]   \
+			    __attribute__((__unused__))
+
 #define l2n(l,c)	(*((c)++)=(unsigned char)(((l)>>24)&0xff), \
 			 *((c)++)=(unsigned char)(((l)>>16)&0xff), \
 			 *((c)++)=(unsigned char)(((l)>> 8)&0xff), \
@@ -1275,8 +1278,6 @@ uint16_t tls1_ec_nid2curve_id(const int nid);
 int tls1_check_curve(SSL *s, const uint16_t curve_id);
 int tls1_get_shared_curve(SSL *s);
 
-int ssl_parse_clienthello_tlsext(SSL *s, unsigned char **data,
-    unsigned char *d, int n, int *al);
 int ssl_parse_serverhello_tlsext(SSL *s, unsigned char **data,
     size_t n, int *al);
 int ssl_check_clienthello_tlsext_early(SSL *s);
diff --git a/lib/libssl/ssl_srvr.c b/lib/libssl/ssl_srvr.c
index 5d741cdc811..6450623d4a3 100644
--- a/lib/libssl/ssl_srvr.c
+++ b/lib/libssl/ssl_srvr.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_srvr.c,v 1.26 2017/10/12 15:52:50 jsing Exp $ */
+/* $OpenBSD: ssl_srvr.c,v 1.27 2018/01/27 15:30:05 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -813,7 +813,6 @@ ssl3_get_client_hello(SSL *s)
 	int i, j, ok, al, ret = -1, cookie_valid = 0;
 	long n;
 	unsigned long id;
-	unsigned char *p, *d;
 	SSL_CIPHER *c;
 	STACK_OF(SSL_CIPHER) *ciphers = NULL;
 	unsigned long alg_k;
@@ -843,8 +842,7 @@ ssl3_get_client_hello(SSL *s)
 	if (n < 0)
 		goto err;
 
-	d = p = (unsigned char *)s->internal->init_msg;
-	end = d + n;
+	end = (unsigned char *)s->internal->init_msg + n;
 
 	CBS_init(&cbs, s->internal->init_msg, n);
 
@@ -1038,14 +1036,17 @@ ssl3_get_client_hello(SSL *s)
 		goto f_err;
 	}
 
-	p = (unsigned char *)CBS_data(&cbs);
-
-	/* TLS extensions*/
-	if (!ssl_parse_clienthello_tlsext(s, &p, d, n, &al)) {
-		/* 'al' set by ssl_parse_clienthello_tlsext */
+	if (!tlsext_clienthello_parse(s, &cbs, &al)) {
 		SSLerror(s, SSL_R_PARSE_TLSEXT);
 		goto f_err;
 	}
+
+	if (!S3I(s)->renegotiate_seen && s->internal->renegotiate) {
+		al = SSL_AD_HANDSHAKE_FAILURE;
+		SSLerror(s, SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED);
+		goto f_err;
+	}
+
 	if (ssl_check_clienthello_tlsext_early(s) <= 0) {
 		SSLerror(s, SSL_R_CLIENTHELLO_TLSEXT);
 		goto err;
diff --git a/lib/libssl/ssl_tlsext.c b/lib/libssl/ssl_tlsext.c
index d0764af3c01..0e3ef7da154 100644
--- a/lib/libssl/ssl_tlsext.c
+++ b/lib/libssl/ssl_tlsext.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_tlsext.c,v 1.19 2018/01/27 15:17:13 jsing Exp $ */
+/* $OpenBSD: ssl_tlsext.c,v 1.20 2018/01/27 15:30:05 jsing Exp $ */
 /*
  * Copyright (c) 2016, 2017 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -1292,6 +1292,35 @@ static struct tls_extension tls_extensions[] = {
 
 #define N_TLS_EXTENSIONS (sizeof(tls_extensions) / sizeof(*tls_extensions))
 
+/* Ensure that extensions fit in a uint32_t bitmask. */
+CTASSERT(N_TLS_EXTENSIONS <= (sizeof(uint32_t) * 8));
+
+static struct tls_extension *
+tls_extension_find(uint16_t type, size_t *tls_extensions_idx)
+{
+	size_t i;
+
+	for (i = 0; i < N_TLS_EXTENSIONS; i++) {
+		if (tls_extensions[i].type == type) {
+			*tls_extensions_idx = i;
+			return &tls_extensions[i];
+		}
+	}
+
+	return NULL;
+}
+
+static void
+tlsext_clienthello_reset_state(SSL *s)
+{
+	s->internal->servername_done = 0;
+	s->tlsext_status_type = -1;
+	S3I(s)->renegotiate_seen = 0;
+	free(S3I(s)->alpn_selected);
+	S3I(s)->alpn_selected = NULL;
+	s->internal->srtp_profile = NULL;
+}
+
 int
 tlsext_clienthello_build(SSL *s, CBB *cbb)
 {
@@ -1329,28 +1358,55 @@ tlsext_clienthello_build(SSL *s, CBB *cbb)
 }
 
 int
-tlsext_clienthello_parse_one(SSL *s, CBS *cbs, uint16_t type, int *alert)
+tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert)
 {
+	CBS extensions, extension_data;
 	struct tls_extension *tlsext;
-	size_t i;
+	uint32_t extensions_seen = 0;
+	uint16_t type;
+	size_t idx;
 
-	for (i = 0; i < N_TLS_EXTENSIONS; i++) {
-		tlsext = &tls_extensions[i];
+	/* XXX - this possibly should be done by the caller... */
+	tlsext_clienthello_reset_state(s);
 
-		if (tlsext->type != type)
+	/* An empty extensions block is valid. */
+	if (CBS_len(cbs) == 0)
+		return 1;
+
+	*alert = SSL_AD_DECODE_ERROR;
+
+	if (!CBS_get_u16_length_prefixed(cbs, &extensions))
+		return 0;
+
+	while (CBS_len(&extensions) > 0) {
+		if (!CBS_get_u16(&extensions, &type))
+			return 0;
+		if (!CBS_get_u16_length_prefixed(&extensions, &extension_data))
+			return 0;
+
+		if (s->internal->tlsext_debug_cb != NULL)
+			s->internal->tlsext_debug_cb(s, 0, type,
+			    (unsigned char *)CBS_data(&extension_data),
+			    CBS_len(&extension_data),
+			    s->internal->tlsext_debug_arg);
+
+		/* Unknown extensions are ignored. */
+		if ((tlsext = tls_extension_find(type, &idx)) == NULL)
 			continue;
-		if (!tlsext->clienthello_parse(s, cbs, alert))
+
+		/* Check for duplicate extensions. */
+		if ((extensions_seen & (1 << idx)) != 0)
 			return 0;
-		if (CBS_len(cbs) != 0) {
-			*alert = SSL_AD_DECODE_ERROR;
+		extensions_seen |= (1 << idx);
+
+		if (!tlsext->clienthello_parse(s, &extension_data, alert))
 			return 0;
-		}
 
-		return 1;
+		if (CBS_len(&extension_data) != 0)
+			return 0;
 	}
 
-	/* Not found. */
-	return 2;
+	return 1;
 }
 
 int
diff --git a/lib/libssl/ssl_tlsext.h b/lib/libssl/ssl_tlsext.h
index 7c6250a7f7d..1af2e6cb3b1 100644
--- a/lib/libssl/ssl_tlsext.h
+++ b/lib/libssl/ssl_tlsext.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_tlsext.h,v 1.10 2017/08/27 02:58:04 doug Exp $ */
+/* $OpenBSD: ssl_tlsext.h,v 1.11 2018/01/27 15:30:05 jsing Exp $ */
 /*
  * Copyright (c) 2016, 2017 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -82,8 +82,7 @@ int tlsext_srtp_serverhello_parse(SSL *s, CBS *cbs, int *alert);
 #endif
 
 int tlsext_clienthello_build(SSL *s, CBB *cbb);
-int tlsext_clienthello_parse_one(SSL *s, CBS *cbs, uint16_t tlsext_type,
-    int *alert);
+int tlsext_clienthello_parse(SSL *s, CBS *cbs, int *alert);
 
 int tlsext_serverhello_build(SSL *s, CBB *cbb);
 int tlsext_serverhello_parse_one(SSL *s, CBS *cbs, uint16_t tlsext_type,
diff --git a/lib/libssl/t1_lib.c b/lib/libssl/t1_lib.c
index 1cef08d0946..fbd79431db6 100644
--- a/lib/libssl/t1_lib.c
+++ b/lib/libssl/t1_lib.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: t1_lib.c,v 1.139 2017/10/11 17:35:00 jsing Exp $ */
+/* $OpenBSD: t1_lib.c,v 1.140 2018/01/27 15:30:05 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -662,75 +662,6 @@ tls12_get_req_sig_algs(SSL *s, unsigned char **sigalgs, size_t *sigalgs_len)
 }
 
 int
-ssl_parse_clienthello_tlsext(SSL *s, unsigned char **p, unsigned char *d,
-    int n, int *al)
-{
-	unsigned short type;
-	unsigned short size;
-	unsigned short len;
-	unsigned char *data = *p;
-	unsigned char *end = d + n;
-	CBS cbs;
-
-	s->internal->servername_done = 0;
-	s->tlsext_status_type = -1;
-	S3I(s)->renegotiate_seen = 0;
-	free(S3I(s)->alpn_selected);
-	S3I(s)->alpn_selected = NULL;
-	s->internal->srtp_profile = NULL;
-
-	if (data == end)
-		goto ri_check;
-
-	if (end - data < 2)
-		goto err;
-	n2s(data, len);
-
-	if (end - data != len)
-		goto err;
-
-	while (end - data >= 4) {
-		n2s(data, type);
-		n2s(data, size);
-
-		if (end - data < size)
-			goto err;
-
-		if (s->internal->tlsext_debug_cb)
-			s->internal->tlsext_debug_cb(s, 0, type, data, size,
-			    s->internal->tlsext_debug_arg);
-
-		CBS_init(&cbs, data, size);
-		if (!tlsext_clienthello_parse_one(s, &cbs, type, al))
-			return 0;
-
-		data += size;
-	}
-
-	/* Spurious data on the end */
-	if (data != end)
-		goto err;
-
-	*p = data;
-
-ri_check:
-
-	/* Need RI if renegotiating */
-
-	if (!S3I(s)->renegotiate_seen && s->internal->renegotiate) {
-		*al = SSL_AD_HANDSHAKE_FAILURE;
-		SSLerror(s, SSL_R_UNSAFE_LEGACY_RENEGOTIATION_DISABLED);
-		return 0;
-	}
-
-	return 1;
-
-err:
-	*al = SSL_AD_DECODE_ERROR;
-	return 0;
-}
-
-int
 ssl_parse_serverhello_tlsext(SSL *s, unsigned char **p, size_t n, int *al)
 {
 	unsigned short type;