kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Fix crash as server when resuming with SNI.

Browse Source 
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>
This commit is contained in:
Adam Langley 2014-09-02 13:52:56 -07:00 committed by Adam Langley
parent 04dbb7f1d1
commit ed8270a55c
2 changed files with 47 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
include/openssl/ssl.h
View File

@ -1375,12 +1375,9 @@ struct ssl_st
void *arg); void *arg);
void *tlsext_debug_arg; void *tlsext_debug_arg;
char *tlsext_hostname; char *tlsext_hostname;
int servername_done; /* no further mod of servername /* should_ack_sni is true if the SNI extension should be acked. This is
0 : call the servername extension callback. * only used by a server. */
1 : prepare 2, allow last ack just after in server callback. char should_ack_sni;
2 : don't call servername callback, no ack in server hello
*/
/* RFC4507 session ticket expected to be received or sent */ /* RFC4507 session ticket expected to be received or sent */
int tlsext_ticket_expected; int tlsext_ticket_expected;
size_t tlsext_ecpointformatlist_length; size_t tlsext_ecpointformatlist_length;

43
ssl/t1_lib.c
View File

@ -1212,7 +1212,7 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf, unsigned c
ret+=2; ret+=2;
if (ret>=limit) return NULL; /* this really never occurs, but ... */ 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; 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; CBS extensions;
size_t i; size_t i;
s->servername_done = 0; s->should_ack_sni = 0;
s->s3->next_proto_neg_seen = 0; s->s3->next_proto_neg_seen = 0;
s->s3->tmp.certificate_status_expected = 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) if (type == TLSEXT_TYPE_server_name)
{ {
CBS server_name_list; CBS server_name_list;
char have_seen_host_name = 0;
if (!CBS_get_u16_length_prefixed(&extension, &server_name_list) || if (!CBS_get_u16_length_prefixed(&extension, &server_name_list) ||
CBS_len(&server_name_list) < 1 || CBS_len(&server_name_list) < 1 ||
@ -1532,17 +1533,17 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CBS *cbs, int *out_alert)
if (name_type != TLSEXT_NAMETYPE_host_name) if (name_type != TLSEXT_NAMETYPE_host_name)
continue; continue;
if (!s->hit) if (have_seen_host_name)
{ {
if (s->session->tlsext_hostname) /* The ServerNameList MUST NOT contain
{ * more than one name of the same
/* The ServerNameList MUST NOT * name_type. */
contain more than one name of
the same name_type. */
*out_alert = SSL_AD_DECODE_ERROR; *out_alert = SSL_AD_DECODE_ERROR;
return 0; return 0;
} }
have_seen_host_name = 1;
if (!CBS_get_u16_length_prefixed(&server_name_list, &host_name) || if (!CBS_get_u16_length_prefixed(&server_name_list, &host_name) ||
CBS_len(&host_name) < 1) CBS_len(&host_name) < 1)
{ {
@ -1557,20 +1558,24 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CBS *cbs, int *out_alert)
return 0; return 0;
} }
if (!s->hit)
{
assert(s->session->tlsext_hostname == NULL);
if (s->session->tlsext_hostname)
{
/* This should be impossible. */
*out_alert = SSL_AD_DECODE_ERROR;
return 0;
}
/* Copy the hostname as a string. */ /* Copy the hostname as a string. */
if (!CBS_strdup(&host_name, &s->session->tlsext_hostname)) if (!CBS_strdup(&host_name, &s->session->tlsext_hostname))
{ {
*out_alert = SSL_AD_INTERNAL_ERROR; *out_alert = SSL_AD_INTERNAL_ERROR;
return 0; return 0;
} }
s->servername_done = 1;
} s->should_ack_sni = 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;
} }
} }
} }
@ -2150,7 +2155,9 @@ static int ssl_check_clienthello_tlsext(SSL *s)
return 1; return 1;
case SSL_TLSEXT_ERR_NOACK: case SSL_TLSEXT_ERR_NOACK:
s->servername_done=0; s->should_ack_sni = 0;
return 1;
default: default:
return 1; return 1;
} }
@ -2207,8 +2214,6 @@ static int ssl_check_serverhello_tlsext(SSL *s)
ssl3_send_alert(s,SSL3_AL_WARNING,al); ssl3_send_alert(s,SSL3_AL_WARNING,al);
return 1; return 1;
case SSL_TLSEXT_ERR_NOACK:
s->servername_done=0;
default: default:
return 1; return 1;
} }