Rewrite session ticket TLS extension handling using CBB/CBS and the new

extension framework.

ok jsing@ beck@

4 files changed, 447 insertions, 66 deletions

diff --git a/lib/libssl/ssl_tlsext.c b/lib/libssl/ssl_tlsext.c
index c050224c708..1813d46f41f 100644
--- a/lib/libssl/ssl_tlsext.c
+++ b/lib/libssl/ssl_tlsext.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_tlsext.c,v 1.6 2017/08/11 20:14:13 doug Exp $ */
+/* $OpenBSD: ssl_tlsext.c,v 1.7 2017/08/12 21:17:03 doug Exp $ */
 /*
  * Copyright (c) 2016, 2017 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -492,6 +492,131 @@ tlsext_sni_serverhello_parse(SSL *s, CBS *cbs, int *alert)
 	return 1;
 }
 
+/*
+ * SessionTicket extension - RFC 5077 section 3.2
+ */
+int
+tlsext_sessionticket_clienthello_needs(SSL *s)
+{
+	/*
+	 * Send session ticket extension when enabled and not overridden.
+	 *
+	 * When renegotiating, send an empty session ticket to indicate support.
+	 */
+	if ((SSL_get_options(s) & SSL_OP_NO_TICKET) != 0)
+		return 0;
+
+	if (s->internal->new_session)
+		return 1;
+
+	if (s->internal->tlsext_session_ticket != NULL &&
+	    s->internal->tlsext_session_ticket->data == NULL)
+		return 0;
+
+	return 1;
+}
+
+int
+tlsext_sessionticket_clienthello_build(SSL *s, CBB *cbb)
+{
+	/*
+	 * Signal that we support session tickets by sending an empty
+	 * extension when renegotiating or no session found.
+	 */
+	if (s->internal->new_session || s->session == NULL)
+		return 1;
+
+	if (s->session->tlsext_tick != NULL) {
+		/* Attempt to resume with an existing session ticket */
+		if (!CBB_add_bytes(cbb, s->session->tlsext_tick,
+		    s->session->tlsext_ticklen))
+			return 0;
+
+	} else if (s->internal->tlsext_session_ticket != NULL) {
+		/*
+		 * Attempt to resume with a custom provided session ticket set
+		 * by SSL_set_session_ticket_ext().
+		 */
+		if (s->internal->tlsext_session_ticket->length > 0) {
+			size_t ticklen = s->internal->tlsext_session_ticket->length;
+
+			if ((s->session->tlsext_tick = malloc(ticklen)) == NULL)
+				return 0;
+			memcpy(s->session->tlsext_tick,
+			    s->internal->tlsext_session_ticket->data,
+			    ticklen);
+			s->session->tlsext_ticklen = ticklen;
+
+			if (!CBB_add_bytes(cbb, s->session->tlsext_tick,
+			    s->session->tlsext_ticklen))
+				return 0;
+		}
+	}
+
+	if (!CBB_flush(cbb))
+		return 0;
+
+	return 1;
+}
+
+int
+tlsext_sessionticket_clienthello_parse(SSL *s, CBS *cbs, int *alert)
+{
+	if (s->internal->tls_session_ticket_ext_cb) {
+		if (!s->internal->tls_session_ticket_ext_cb(s, CBS_data(cbs),
+		    (int)CBS_len(cbs),
+		    s->internal->tls_session_ticket_ext_cb_arg)) {
+			*alert = TLS1_AD_INTERNAL_ERROR;
+			return 0;
+		}
+	}
+
+	/* We need to signal that this was processed fully */
+	if (!CBS_skip(cbs, CBS_len(cbs))) {
+		*alert = TLS1_AD_INTERNAL_ERROR;
+		return 0;
+	}
+
+	return 1;
+}
+
+int
+tlsext_sessionticket_serverhello_needs(SSL *s)
+{
+	return (s->internal->tlsext_ticket_expected &&
+	    !(SSL_get_options(s) & SSL_OP_NO_TICKET));
+}
+
+int
+tlsext_sessionticket_serverhello_build(SSL *s, CBB *cbb)
+{
+	/* Empty ticket */
+
+	return 1;
+}
+
+int
+tlsext_sessionticket_serverhello_parse(SSL *s, CBS *cbs, int *alert)
+{
+	if (s->internal->tls_session_ticket_ext_cb) {
+		if (!s->internal->tls_session_ticket_ext_cb(s, CBS_data(cbs),
+		    (int)CBS_len(cbs),
+		    s->internal->tls_session_ticket_ext_cb_arg)) {
+			*alert = TLS1_AD_INTERNAL_ERROR;
+			return 0;
+		}
+	}
+
+	if ((SSL_get_options(s) & SSL_OP_NO_TICKET) != 0 || CBS_len(cbs) > 0) {
+		*alert = TLS1_AD_UNSUPPORTED_EXTENSION;
+		return 0;
+	}
+
+	s->internal->tlsext_ticket_expected = 1;
+
+	return 1;
+}
+
 struct tls_extension {
 	uint16_t type;
 	int (*clienthello_needs)(SSL *s);
@@ -539,6 +664,15 @@ static struct tls_extension tls_extensions[] = {
 		.serverhello_build = tlsext_ec_serverhello_build,
 		.serverhello_parse = tlsext_ec_serverhello_parse,
 	},
+	{
+		.type = TLSEXT_TYPE_session_ticket,
+		.clienthello_needs = tlsext_sessionticket_clienthello_needs,
+		.clienthello_build = tlsext_sessionticket_clienthello_build,
+		.clienthello_parse = tlsext_sessionticket_clienthello_parse,
+		.serverhello_needs = tlsext_sessionticket_serverhello_needs,
+		.serverhello_build = tlsext_sessionticket_serverhello_build,
+		.serverhello_parse = tlsext_sessionticket_serverhello_parse,
+	},
 };
 
 #define N_TLS_EXTENSIONS (sizeof(tls_extensions) / sizeof(*tls_extensions))
diff --git a/lib/libssl/ssl_tlsext.h b/lib/libssl/ssl_tlsext.h
index 38f8ffaa658..1e701e941ac 100644
--- a/lib/libssl/ssl_tlsext.h
+++ b/lib/libssl/ssl_tlsext.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_tlsext.h,v 1.5 2017/08/11 20:14:13 doug Exp $ */
+/* $OpenBSD: ssl_tlsext.h,v 1.6 2017/08/12 21:17:03 doug Exp $ */
 /*
  * Copyright (c) 2016, 2017 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -44,6 +44,12 @@ int tlsext_ecpf_serverhello_needs(SSL *s);
 int tlsext_ecpf_serverhello_build(SSL *s, CBB *cbb);
 int tlsext_ecpf_serverhello_parse(SSL *s, CBS *cbs, int *alert);
 
+int tlsext_sessionticket_clienthello_needs(SSL *s);
+int tlsext_sessionticket_clienthello_build(SSL *s, CBB *cbb);
+int tlsext_sessionticket_clienthello_parse(SSL *s, CBS *cbs, int *alert);
+int tlsext_sessionticket_serverhello_needs(SSL *s);
+int tlsext_sessionticket_serverhello_build(SSL *s, CBB *cbb);
+int tlsext_sessionticket_serverhello_parse(SSL *s, CBS *cbs, int *alert);
 
 int tlsext_clienthello_build(SSL *s, CBB *cbb);
 int tlsext_clienthello_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 911e8d3f4e2..63d401c3371 100644
--- a/lib/libssl/t1_lib.c
+++ b/lib/libssl/t1_lib.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: t1_lib.c,v 1.128 2017/08/12 21:03:08 jsing Exp $ */
+/* $OpenBSD: t1_lib.c,v 1.129 2017/08/12 21:17:03 doug Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -690,39 +690,6 @@ ssl_add_clienthello_tlsext(SSL *s, unsigned char *p, unsigned char *limit)
 		return NULL;
 	ret += len;
 
-	if (!(SSL_get_options(s) & SSL_OP_NO_TICKET)) {
-		int ticklen;
-		if (!s->internal->new_session && s->session && s->session->tlsext_tick)
-			ticklen = s->session->tlsext_ticklen;
-		else if (s->session && s->internal->tlsext_session_ticket &&
-		    s->internal->tlsext_session_ticket->data) {
-			ticklen = s->internal->tlsext_session_ticket->length;
-			s->session->tlsext_tick = malloc(ticklen);
-			if (!s->session->tlsext_tick)
-				return NULL;
-			memcpy(s->session->tlsext_tick,
-			    s->internal->tlsext_session_ticket->data, ticklen);
-			s->session->tlsext_ticklen = ticklen;
-		} else
-			ticklen = 0;
-		if (ticklen == 0 && s->internal->tlsext_session_ticket &&
-		    s->internal->tlsext_session_ticket->data == NULL)
-			goto skip_ext;
-		/* Check for enough room 2 for extension type, 2 for len
- 		 * rest for ticket
-  		 */
-		if ((size_t)(limit - ret) < 4 + ticklen)
-			return NULL;
-		s2n(TLSEXT_TYPE_session_ticket, ret);
-
-		s2n(ticklen, ret);
-		if (ticklen) {
-			memcpy(ret, s->session->tlsext_tick, ticklen);
-			ret += ticklen;
-		}
-	}
-skip_ext:
-
 	if (TLS1_get_client_version(s) >= TLS1_2_VERSION) {
 		if ((size_t)(limit - ret) < sizeof(tls12_sigalgs) + 6)
 			return NULL;
@@ -884,15 +851,6 @@ ssl_add_serverhello_tlsext(SSL *s, unsigned char *p, unsigned char *limit)
 	 * extension.
 	 */
 
-	if (s->internal->tlsext_ticket_expected &&
-	    !(SSL_get_options(s) & SSL_OP_NO_TICKET)) {
-		if ((size_t)(limit - ret) < 4)
-			return NULL;
-
-		s2n(TLSEXT_TYPE_session_ticket, ret);
-		s2n(0, ret);
-	}
-
 	if (s->internal->tlsext_status_expected) {
 		if ((size_t)(limit - ret) < 4)
 			return NULL;
@@ -1068,13 +1026,7 @@ ssl_parse_clienthello_tlsext(SSL *s, unsigned char **p, unsigned char *d,
 		if (!tlsext_clienthello_parse_one(s, &cbs, type, al))
 			return 0;
 
-		if (type == TLSEXT_TYPE_session_ticket) {
-			if (s->internal->tls_session_ticket_ext_cb &&
-			    !s->internal->tls_session_ticket_ext_cb(s, data, size, s->internal->tls_session_ticket_ext_cb_arg)) {
-				*al = TLS1_AD_INTERNAL_ERROR;
-				return 0;
-			}
-		} else if (type == TLSEXT_TYPE_signature_algorithms) {
+		if (type == TLSEXT_TYPE_signature_algorithms) {
 			int dsize;
 			if (sigalg_seen || size < 2) {
 				*al = SSL_AD_DECODE_ERROR;
@@ -1277,19 +1229,7 @@ ssl_parse_serverhello_tlsext(SSL *s, unsigned char **p, size_t n, int *al)
 		if (!tlsext_serverhello_parse_one(s, &cbs, type, al))
 			return 0;
 
-		if (type == TLSEXT_TYPE_session_ticket) {
-			if (s->internal->tls_session_ticket_ext_cb &&
-			    !s->internal->tls_session_ticket_ext_cb(s, data, size, s->internal->tls_session_ticket_ext_cb_arg)) {
-				*al = TLS1_AD_INTERNAL_ERROR;
-				return 0;
-			}
-			if ((SSL_get_options(s) & SSL_OP_NO_TICKET) || (size > 0)) {
-				*al = TLS1_AD_UNSUPPORTED_EXTENSION;
-				return 0;
-			}
-			s->internal->tlsext_ticket_expected = 1;
-		}
-		else if (type == TLSEXT_TYPE_status_request &&
+		if (type == TLSEXT_TYPE_status_request &&
 		    s->version != DTLS1_VERSION) {
 			/* MUST be empty and only sent if we've requested
 			 * a status request message.
diff --git a/regress/lib/libssl/tlsext/tlsexttest.c b/regress/lib/libssl/tlsext/tlsexttest.c
index 880142202ed..1267f50a494 100644
--- a/regress/lib/libssl/tlsext/tlsexttest.c
+++ b/regress/lib/libssl/tlsext/tlsexttest.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tlsexttest.c,v 1.9 2017/08/12 19:09:37 beck Exp $ */
+/* $OpenBSD: tlsexttest.c,v 1.10 2017/08/12 21:17:03 doug Exp $ */
 /*
  * Copyright (c) 2017 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -1298,6 +1298,304 @@ test_tlsext_sni_serverhello(void)
 	return (failure);
 }
 
+/*
+ * Session ticket - RFC 5077 since no known implementations use 4507.
+ *
+ * Session tickets can be length 0 (special case) to 2^16-1.
+ *
+ * The state is encrypted by the server so it is opaque to the client.
+ */
+static uint8_t tlsext_sessionticket_hello_min[1];
+static uint8_t tlsext_sessionticket_hello_max[65535];
+
+static int
+test_tlsext_sessionticket_clienthello(void)
+{
+	unsigned char *data = NULL;
+	SSL_CTX *ssl_ctx = NULL;
+	SSL *ssl = NULL;
+	int failure;
+	CBB cbb;
+	size_t dlen;
+	uint8_t dummy[1234];
+
+	failure = 1;
+
+	CBB_init(&cbb, 0);
+
+	/* Create fake session tickets with random data. */
+	arc4random_buf(tlsext_sessionticket_hello_min,
+	    sizeof(tlsext_sessionticket_hello_min));
+	arc4random_buf(tlsext_sessionticket_hello_max,
+	    sizeof(tlsext_sessionticket_hello_max));
+
+	if ((ssl_ctx = SSL_CTX_new(TLS_client_method())) == NULL)
+		errx(1, "failed to create SSL_CTX");
+	if ((ssl = SSL_new(ssl_ctx)) == NULL)
+		errx(1, "failed to create SSL");
+
+	/* Should need a ticket by default. */
+	if (!tlsext_sessionticket_clienthello_needs(ssl)) {
+		FAIL("clienthello should need Sessionticket for default "
+		    "ciphers\n");
+		goto err;
+	}
+
+	/* Test disabling tickets. */
+	if ((SSL_set_options(ssl, SSL_OP_NO_TICKET) & SSL_OP_NO_TICKET) == 0) {
+		FAIL("Cannot disable tickets in the TLS connection");
+		return 0;
+	}
+	if (tlsext_sessionticket_clienthello_needs(ssl)) {
+		FAIL("clienthello should not need SessionTicket if it was disabled");
+		goto err;
+	}
+
+	/* Test re-enabling tickets. */
+	if ((SSL_clear_options(ssl, SSL_OP_NO_TICKET) & SSL_OP_NO_TICKET) != 0) {
+		FAIL("Cannot re-enable tickets in the TLS connection");
+		return 0;
+	}
+	if (!tlsext_sessionticket_clienthello_needs(ssl)) {
+		FAIL("clienthello should need SessionTicket if it was disabled");
+		goto err;
+	}
+
+	/* Since we don't have a session, we should build an empty ticket. */
+	if (!tlsext_sessionticket_clienthello_build(ssl, &cbb)) {
+		FAIL("Cannot build a ticket");
+		goto err;
+	}
+	if (!CBB_finish(&cbb, &data, &dlen)) {
+		FAIL("Cannot finish CBB");
+		goto err;
+	}
+	if (dlen != 0) {
+		FAIL("Expected 0 length but found %zu\n", dlen);
+		goto err;
+	}
+
+	CBB_cleanup(&cbb);
+	CBB_init(&cbb, 0);
+	free(data);
+	data = NULL;
+
+	/* With a new session (but no ticket), we should still have 0 length */
+	if ((ssl->session = SSL_SESSION_new()) == NULL)
+		errx(1, "failed to create session");
+	if (!tlsext_sessionticket_clienthello_needs(ssl)) {
+		FAIL("Should still want a session ticket with a new session");
+		goto err;
+	}
+	if (!tlsext_sessionticket_clienthello_build(ssl, &cbb)) {
+		FAIL("Cannot build a ticket");
+		goto err;
+	}
+	if (!CBB_finish(&cbb, &data, &dlen)) {
+		FAIL("Cannot finish CBB");
+		goto err;
+	}
+	if (dlen != 0) {
+		FAIL("Expected 0 length but found %zu\n", dlen);
+		goto err;
+	}
+
+	CBB_cleanup(&cbb);
+	CBB_init(&cbb, 0);
+	free(data);
+	data = NULL;
+
+	/* With a new session (and ticket), we should use that ticket */
+	SSL_SESSION_free(ssl->session);
+	if ((ssl->session = SSL_SESSION_new()) == NULL)
+		errx(1, "failed to create session");
+
+	arc4random_buf(&dummy, sizeof(dummy));
+	if ((ssl->session->tlsext_tick = malloc(sizeof(dummy))) == NULL) {
+		errx(1, "failed to malloc");
+	}
+	memcpy(ssl->session->tlsext_tick, dummy, sizeof(dummy));
+	ssl->session->tlsext_ticklen = sizeof(dummy);
+
+	if (!tlsext_sessionticket_clienthello_needs(ssl)) {
+		FAIL("Should still want a session ticket with a new session");
+		goto err;
+	}
+	if (!tlsext_sessionticket_clienthello_build(ssl, &cbb)) {
+		FAIL("Cannot build a ticket");
+		goto err;
+	}
+	if (!CBB_finish(&cbb, &data, &dlen)) {
+		FAIL("Cannot finish CBB");
+		goto err;
+	}
+	if (dlen != sizeof(dummy)) {
+		FAIL("Expected %zu length but found %zu\n", sizeof(dummy), dlen);
+		goto err;
+	}
+	if (memcmp(data, dummy, dlen) != 0) {
+		FAIL("serverhello SNI differs:\n");
+		compare_data(data, dlen,
+		    dummy, sizeof(dummy));
+		goto err;
+	}
+
+	CBB_cleanup(&cbb);
+	CBB_init(&cbb, 0);
+	free(data);
+	data = NULL;
+	free(ssl->session->tlsext_tick);
+	ssl->session->tlsext_tick = NULL;
+	ssl->session->tlsext_ticklen = 0;
+
+	/*
+	 * Send in NULL to disable session tickets at runtime without going
+	 * through SSL_set_options().
+	 */
+	if (!SSL_set_session_ticket_ext(ssl, NULL, 0)) {
+		FAIL("Could not set a NULL custom ticket");
+		goto err;
+	}
+	/* Should not need a ticket in this case */
+	if (tlsext_sessionticket_clienthello_needs(ssl)) {
+		FAIL("Should not want to use session tickets with a NULL custom");
+		goto err;
+	}
+
+	/*
+	 * If you want to remove the tlsext_session_ticket behavior, you have
+	 * to do it manually.
+	 */
+	free(ssl->internal->tlsext_session_ticket);
+	ssl->internal->tlsext_session_ticket = NULL;
+
+	if (!tlsext_sessionticket_clienthello_needs(ssl)) {
+		FAIL("Should need a session ticket again when the custom one is removed");
+		goto err;
+	}
+
+	/* Test a custom session ticket (not recommended in practice) */
+	if (!SSL_set_session_ticket_ext(ssl, tlsext_sessionticket_hello_max,
+	    sizeof(tlsext_sessionticket_hello_max))) {
+		FAIL("Should be able to set a custom ticket");
+		goto err;
+	}
+	if (!tlsext_sessionticket_clienthello_needs(ssl)) {
+		FAIL("Should need a session ticket again when the custom one is not empty");
+		goto err;
+	}
+	if (!tlsext_sessionticket_clienthello_build(ssl, &cbb)) {
+		FAIL("Cannot build a ticket with a max length random payload");
+		goto err;
+	}
+	if (!CBB_finish(&cbb, &data, &dlen)) {
+		FAIL("Cannot finish CBB");
+		goto err;
+	}
+	if (dlen != sizeof(tlsext_sessionticket_hello_max)) {
+		FAIL("Expected %zu length but found %zu\n",
+		    sizeof(tlsext_sessionticket_hello_max), dlen);
+		goto err;
+	}
+	if (memcmp(data, tlsext_sessionticket_hello_max,
+	    sizeof(tlsext_sessionticket_hello_max)) != 0) {
+		FAIL("Expected to get what we passed in");
+		compare_data(data, dlen,
+		    tlsext_sessionticket_hello_max,
+		    sizeof(tlsext_sessionticket_hello_max));
+		goto err;
+	}
+
+	failure = 0;
+
+ err:
+	CBB_cleanup(&cbb);
+	SSL_CTX_free(ssl_ctx);
+	SSL_free(ssl);
+	free(data);
+
+	return (failure);
+}
+
+
+static int
+test_tlsext_sessionticket_serverhello(void)
+{
+	SSL_CTX *ssl_ctx = NULL;
+	SSL *ssl = NULL;
+	int failure;
+	uint8_t *data;
+	size_t dlen;
+	CBB cbb;
+
+	CBB_init(&cbb, 0);
+
+	failure = 1;
+
+	if ((ssl_ctx = SSL_CTX_new(TLS_server_method())) == NULL)
+		errx(1, "failed to create SSL_CTX");
+	if ((ssl = SSL_new(ssl_ctx)) == NULL)
+		errx(1, "failed to create SSL");
+
+	/*
+	 * By default, should not need a session ticket since the ticket
+	 * is not yet expected.
+	 */
+	if (tlsext_sessionticket_serverhello_needs(ssl)) {
+		FAIL("serverhello should not need SessionTicket by default\n");
+		goto err;
+	}
+
+	/* Test disabling tickets. */
+	if ((SSL_set_options(ssl, SSL_OP_NO_TICKET) & SSL_OP_NO_TICKET) == 0) {
+		FAIL("Cannot disable tickets in the TLS connection");
+		return 0;
+	}
+	if (tlsext_sessionticket_serverhello_needs(ssl)) {
+		FAIL("serverhello should not need SessionTicket if it was disabled");
+		goto err;
+	}
+
+	/* Test re-enabling tickets. */
+	if ((SSL_clear_options(ssl, SSL_OP_NO_TICKET) & SSL_OP_NO_TICKET) != 0) {
+		FAIL("Cannot re-enable tickets in the TLS connection");
+		return 0;
+	}
+	if (tlsext_sessionticket_serverhello_needs(ssl)) {
+		FAIL("serverhello should not need SessionTicket yet");
+		goto err;
+	}
+
+	/* Set expected to require it. */
+	ssl->internal->tlsext_ticket_expected = 1;
+	if (!tlsext_sessionticket_serverhello_needs(ssl)) {
+		FAIL("serverhello should now be required for SessionTicket");
+		goto err;
+	}
+
+	/* server hello's session ticket should always be 0 length payload. */
+	if (!tlsext_sessionticket_serverhello_build(ssl, &cbb)) {
+		FAIL("Cannot build a ticket with a max length random payload");
+		goto err;
+	}
+	if (!CBB_finish(&cbb, &data, &dlen)) {
+		FAIL("Cannot finish CBB");
+		goto err;
+	}
+	if (dlen != 0) {
+		FAIL("Expected 0 length but found %zu\n", dlen);
+		goto err;
+	}
+
+	failure = 0;
+
+ err:
+	SSL_CTX_free(ssl_ctx);
+	SSL_free(ssl);
+
+	return (failure);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -1317,5 +1615,8 @@ main(int argc, char **argv)
 	failed |= test_tlsext_sni_clienthello();
 	failed |= test_tlsext_sni_serverhello();
 
+	failed |= test_tlsext_sessionticket_clienthello();
+	failed |= test_tlsext_sessionticket_serverhello();
+
 	return (failed);
 }