From 81ea0bf538909a0aa3fe2f82726ebdda7dc94ab1 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 23 Nov 2014 04:20:17 -0500 Subject: [PATCH] Delay creating s->session until resumption is resolved. When not offering to resume a session, the client populates s->session with a fresh SSL_SESSION before the ServerHello is processed and, in DTLS_ANY_VERSION, before the version is even determined. Don't create a fresh SSL_SESSION until we know we are doing a full handshake. This brings ssl3_send_client_hello closer to ssl23_client_hello in behavior. It also fixes ssl_version in the client in DTLS_ANY_VERSION. SSLv23_client_method is largely unchanged. If no session is offered, s->session continues to be NULL until the ServerHello is received. The one difference is that s->session isn't populated until the entire ServerHello is received, rather than just the first half, in the case of a fragmented ServerHello. Apart from info_callback, no external hooks get called between those points, so this shouldn't expose new missing NULL checks. The other client methods change significantly to match SSLv23_client_method's behavior. For TLS, any exposed missing NULL checks are also in SSLv23_client_method (and version-specific methods are already weird), so that should be safe. For DTLS, I've verified that accesses in d1_*.c either handle NULL or are after the ServerHello. Change-Id: Idcae6bd242480e28a57dbba76ce67f1ac1ae1d1d Reviewed-on: https://boringssl-review.googlesource.com/2404 Reviewed-by: Adam Langley --- ssl/s23_clnt.c | 6 ------ ssl/s3_clnt.c | 39 ++++++++++++++++++++------------------- 2 files changed, 20 insertions(+), 25 deletions(-) diff --git a/ssl/s23_clnt.c b/ssl/s23_clnt.c index 7ca3a7ed..ec6609de 100644 --- a/ssl/s23_clnt.c +++ b/ssl/s23_clnt.c @@ -555,12 +555,6 @@ static int ssl23_get_server_hello(SSL *s) goto err; } s->init_num=0; - - /* If there was no session to resume, now that the final version is - * determined, insert a fresh one. */ - if (s->session == NULL && !ssl_get_new_session(s,0)) - goto err; - return(SSL_connect(s)); err: return(-1); diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index 6f47e95c..8d60d8fc 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -148,6 +148,7 @@ * OTHERWISE. */ +#include #include #include @@ -627,15 +628,6 @@ int ssl3_send_client_hello(SSL *s) buf=(unsigned char *)s->init_buf->data; if (s->state == SSL3_ST_CW_CLNT_HELLO_A) { - SSL_SESSION *sess = s->session; - if (sess == NULL || - sess->ssl_version != s->version || - !sess->session_id_length || - sess->not_resumable) - { - if (!ssl_get_new_session(s,0)) - goto err; - } if (s->method->version == DTLS_ANY_VERSION) { /* Determine which DTLS version to use */ @@ -666,6 +658,18 @@ int ssl3_send_client_hello(SSL *s) } s->client_version = s->version; } + + /* If the configured session was created at a version + * higher than our maximum version, drop it. */ + if (s->session && + (s->session->session_id_length == 0 || + s->session->not_resumable || + (!SSL_IS_DTLS(s) && s->session->ssl_version > s->version) || + (SSL_IS_DTLS(s) && s->session->ssl_version < s->version))) + { + SSL_set_session(s, NULL); + } + /* else use the pre-loaded session */ p=s->s3->client_random; @@ -727,7 +731,7 @@ int ssl3_send_client_hello(SSL *s) p+=SSL3_RANDOM_SIZE; /* Session ID */ - if (s->new_session) + if (s->new_session || s->session == NULL) i=0; else i=s->session->session_id_length; @@ -868,7 +872,8 @@ int ssl3_get_server_hello(SSL *s) /* Copy over the server random. */ memcpy(s->s3->server_random, CBS_data(&server_random), SSL3_RANDOM_SIZE); - if (CBS_len(&session_id) != 0 && + assert(s->session == NULL || s->session->session_id_length > 0); + if (s->session != NULL && CBS_mem_equal(&session_id, s->session->session_id, s->session->session_id_length)) { @@ -884,16 +889,12 @@ int ssl3_get_server_hello(SSL *s) } else { - /* The session wasn't resumed. */ + /* The session wasn't resumed. Create a fresh SSL_SESSION to + * fill out. */ s->hit = 0; - /* If we were trying for session-id reuse, make a new - * SSL_SESSION so we don't stuff up other people */ - if (s->session->session_id_length > 0) + if (!ssl_get_new_session(s, 0)) { - if (!ssl_get_new_session(s,0)) - { - goto f_err; - } + goto f_err; } /* Note: session_id could be empty. */ s->session->session_id_length = CBS_len(&session_id);