Fix crash as server when resuming with SNI.

Thanks to Denis Denisov for noting that |host_name| could be used while
uninitialised in the resumption case.

While in the area, this change also renames |servername_done| to
something more reasonable and removes a documented value that was never
used. Additionally, the SNI ack was only sent when not resuming so
calculating whether it should be sent when processing ClientHello
extensions (which is after s->hit has been set) is superfluous.

Lastly, since SNI is only acked by servers, there's no need to worry
about the SNI callback returning NOACK in the client case.

Change-Id: Ie4ecfc347bd7afaf93b12526ff9311cc45da4df6
Reviewed-on: https://boringssl-review.googlesource.com/1700
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>

2 files changed, 47 insertions, 45 deletions

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 70afbd4..900fae7 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -1375,12 +1375,9 @@ struct ssl_st
 					void *arg);
 	void *tlsext_debug_arg;
 	char *tlsext_hostname;
-	int servername_done;   /* no further mod of servername 
-	                          0 : call the servername extension callback.
-	                          1 : prepare 2, allow last ack just after in server callback.
-	                          2 : don't call servername callback, no ack in server hello
-	                       */
-
+	/* should_ack_sni is true if the SNI extension should be acked. This is
+	 * only used by a server. */
+	char should_ack_sni;
 	/* RFC4507 session ticket expected to be received or sent */
 	int tlsext_ticket_expected;
 	size_t tlsext_ecpointformatlist_length;
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 8fdd813..1af521f 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1212,7 +1212,7 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf, unsigned c
 	ret+=2;
 	if (ret>=limit) return NULL; /* this really never occurs, but ... */
 
-	if (!s->hit && s->servername_done == 1 && s->session->tlsext_hostname != NULL)
+	if (!s->hit && s->should_ack_sni && s->session->tlsext_hostname != NULL)
 		{ 
 		if ((long)(limit - ret - 4) < 0) return NULL; 
 
@@ -1418,7 +1418,7 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CBS *cbs, int *out_alert)
 	CBS extensions;
 	size_t i;
 
-	s->servername_done = 0;
+	s->should_ack_sni = 0;
 	s->s3->next_proto_neg_seen = 0;
 	s->s3->tmp.certificate_status_expected = 0;
 
@@ -1506,6 +1506,7 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CBS *cbs, int *out_alert)
 		if (type == TLSEXT_TYPE_server_name)
 			{
 			CBS server_name_list;
+			char have_seen_host_name = 0;
 
 			if (!CBS_get_u16_length_prefixed(&extension, &server_name_list) ||
 				CBS_len(&server_name_list) < 1 ||
@@ -1532,45 +1533,49 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CBS *cbs, int *out_alert)
 				if (name_type != TLSEXT_NAMETYPE_host_name)
 					continue;
 
+				if (have_seen_host_name)
+					{
+					/* The ServerNameList MUST NOT contain
+					 * more than one name of the same
+					 * name_type. */
+					*out_alert = SSL_AD_DECODE_ERROR;
+					return 0;
+					}
+
+				have_seen_host_name = 1;
+
+				if (!CBS_get_u16_length_prefixed(&server_name_list, &host_name) ||
+					CBS_len(&host_name) < 1)
+					{
+					*out_alert = SSL_AD_DECODE_ERROR;
+					return 0;
+					}
+
+				if (CBS_len(&host_name) > TLSEXT_MAXLEN_host_name ||
+					CBS_contains_zero_byte(&host_name))
+					{
+					*out_alert = SSL_AD_UNRECOGNIZED_NAME;
+					return 0;
+					}
+
 				if (!s->hit)
 					{
+					assert(s->session->tlsext_hostname == NULL);
 					if (s->session->tlsext_hostname)
 						{
-						/* The ServerNameList MUST NOT
-						   contain more than one name of
-						   the same name_type. */
+						/* This should be impossible. */
 						*out_alert = SSL_AD_DECODE_ERROR;
 						return 0;
 						}
 
-					if (!CBS_get_u16_length_prefixed(&server_name_list, &host_name) ||
-						CBS_len(&host_name) < 1)
-						{
-						*out_alert = SSL_AD_DECODE_ERROR;
-						return 0;
-						}
-
-					if (CBS_len(&host_name) > TLSEXT_MAXLEN_host_name ||
-						CBS_contains_zero_byte(&host_name))
-						{
-						*out_alert = SSL_AD_UNRECOGNIZED_NAME;
-						return 0;
-						}
-
 					/* Copy the hostname as a string. */
 					if (!CBS_strdup(&host_name, &s->session->tlsext_hostname))
 						{
 						*out_alert = SSL_AD_INTERNAL_ERROR;
 						return 0;
 						}
-					s->servername_done = 1;
-					}
-				else
-					{
-					s->servername_done = s->session->tlsext_hostname
-						&& strlen(s->session->tlsext_hostname) == CBS_len(&host_name)
-						&& strncmp(s->session->tlsext_hostname,
-							(char *)CBS_data(&host_name), CBS_len(&host_name)) == 0;
+
+					s->should_ack_sni = 1;
 					}
 				}
 			}
@@ -2147,12 +2152,14 @@ static int ssl_check_clienthello_tlsext(SSL *s)
 
 		case SSL_TLSEXT_ERR_ALERT_WARNING:
 			ssl3_send_alert(s,SSL3_AL_WARNING,al);
-			return 1; 
-					
+			return 1;
+
 		case SSL_TLSEXT_ERR_NOACK:
-			s->servername_done=0;
-			default:
-		return 1;
+			s->should_ack_sni = 0;
+			return 1;
+
+		default:
+			return 1;
 		}
 	}
 
@@ -2200,17 +2207,15 @@ static int ssl_check_serverhello_tlsext(SSL *s)
 	switch (ret)
 		{
 		case SSL_TLSEXT_ERR_ALERT_FATAL:
-			ssl3_send_alert(s,SSL3_AL_FATAL,al); 
+			ssl3_send_alert(s,SSL3_AL_FATAL,al);
 			return -1;
 
 		case SSL_TLSEXT_ERR_ALERT_WARNING:
 			ssl3_send_alert(s,SSL3_AL_WARNING,al);
-			return 1; 
-					
-		case SSL_TLSEXT_ERR_NOACK:
-			s->servername_done=0;
-			default:
-		return 1;
+			return 1;
+
+		default:
+			return 1;
 		}
 	}