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>
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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;
}
}
