Splitting SSL session state.
To prevent configuration/established session confusion, the handshake
session state is separated into the configured session (ssl->session)
and the newly created session (ssl->s3->new_session). Upon conclusion of
the handshake, the finalized session is stored
in (ssl->s3->established_session). During the handshake, any requests
for the session (SSL_get_session) return a non-resumable session, to
prevent resumption of a partially filled session. Sessions should only
be cached upon the completion of the full handshake, using the resulting
established_session. The semantics of accessors on the session are
maintained mid-renego.
Change-Id: I4358aecb71fce4fe14a6746c5af1416a69935078
Reviewed-on: https://boringssl-review.googlesource.com/8612
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/handshake_client.c b/ssl/handshake_client.c
index 5fe706c..d78bc27 100644
--- a/ssl/handshake_client.c
+++ b/ssl/handshake_client.c
@@ -252,7 +252,7 @@
goto end;
}
- if (ssl->hit) {
+ if (ssl->session != NULL) {
ssl->state = SSL3_ST_CR_SESSION_TICKET_A;
} else {
ssl->state = SSL3_ST_CR_CERT_A;
@@ -411,7 +411,7 @@
}
ssl->state = SSL3_ST_CW_FLUSH;
- if (ssl->hit) {
+ if (ssl->session != NULL) {
ssl->s3->tmp.next_state = SSL_ST_OK;
} else {
/* This is a non-resumption handshake. If it involves ChannelID, then
@@ -474,7 +474,7 @@
}
ssl->method->received_flight(ssl);
- if (ssl->hit) {
+ if (ssl->session != NULL) {
ssl->state = SSL3_ST_CW_CHANGE;
} else {
ssl->state = SSL_ST_OK;
@@ -506,6 +506,29 @@
ssl3_cleanup_key_block(ssl);
ssl->method->release_current_message(ssl, 1 /* free_buffer */);
+ SSL_SESSION_free(ssl->s3->established_session);
+ if (ssl->session != NULL) {
+ ssl->s3->established_session = SSL_SESSION_up_ref(ssl->session);
+ } else {
+ /* We make a copy of the session in order to maintain the immutability
+ * of the new established_session due to False Start. The caller may
+ * have taken a reference to the temporary session. */
+ ssl->s3->established_session =
+ SSL_SESSION_dup(ssl->s3->new_session, 1 /* include ticket */);
+ if (ssl->s3->established_session == NULL) {
+ /* Do not stay in SSL_ST_OK, to avoid confusing |SSL_in_init|
+ * callers. */
+ ssl->state = SSL_ST_ERROR;
+ skip = 1;
+ ret = -1;
+ goto end;
+ }
+ ssl->s3->established_session->not_resumable = 0;
+
+ SSL_SESSION_free(ssl->s3->new_session);
+ ssl->s3->new_session = NULL;
+ }
+
/* Remove write buffering now. */
ssl_free_wbio_buffer(ssl);
@@ -526,6 +549,11 @@
ssl_do_info_callback(ssl, SSL_CB_HANDSHAKE_DONE, 1);
goto end;
+ case SSL_ST_ERROR:
+ OPENSSL_PUT_ERROR(SSL, SSL_R_SSL_HANDSHAKE_FAILURE);
+ ret = -1;
+ goto end;
+
default:
OPENSSL_PUT_ERROR(SSL, SSL_R_UNKNOWN_STATE);
ret = -1;
@@ -869,17 +897,16 @@
SSL_R_ATTEMPT_TO_REUSE_SESSION_IN_DIFFERENT_CONTEXT);
goto f_err;
}
- ssl->hit = 1;
} else {
/* The session wasn't resumed. Create a fresh SSL_SESSION to
* fill out. */
- ssl->hit = 0;
+ SSL_set_session(ssl, NULL);
if (!ssl_get_new_session(ssl, 0 /* client */)) {
goto f_err;
}
/* Note: session_id could be empty. */
- ssl->session->session_id_length = CBS_len(&session_id);
- memcpy(ssl->session->session_id, CBS_data(&session_id),
+ ssl->s3->new_session->session_id_length = CBS_len(&session_id);
+ memcpy(ssl->s3->new_session->session_id, CBS_data(&session_id),
CBS_len(&session_id));
}
@@ -908,7 +935,7 @@
goto f_err;
}
- if (ssl->hit) {
+ if (ssl->session != NULL) {
if (ssl->session->cipher != c) {
al = SSL_AD_ILLEGAL_PARAMETER;
OPENSSL_PUT_ERROR(SSL, SSL_R_OLD_SESSION_CIPHER_NOT_RETURNED);
@@ -920,7 +947,7 @@
goto f_err;
}
} else {
- ssl->session->cipher = c;
+ ssl->s3->new_session->cipher = c;
}
ssl->s3->tmp.new_cipher = c;
@@ -932,7 +959,8 @@
/* If doing a full handshake, the server may request a client certificate
* which requires hashing the handshake transcript. Otherwise, the handshake
* buffer may be released. */
- if (ssl->hit || !ssl_cipher_uses_certificate_auth(ssl->s3->tmp.new_cipher)) {
+ if (ssl->session != NULL ||
+ !ssl_cipher_uses_certificate_auth(ssl->s3->tmp.new_cipher)) {
ssl3_free_handshake_buffer(ssl);
}
@@ -957,7 +985,7 @@
goto f_err;
}
- if (ssl->hit &&
+ if (ssl->session != NULL &&
ssl->s3->tmp.extended_master_secret !=
ssl->session->extended_master_secret) {
al = SSL_AD_HANDSHAKE_FAILURE;
@@ -1007,13 +1035,13 @@
/* NOTE: Unlike the server half, the client's copy of |cert_chain| includes
* the leaf. */
- sk_X509_pop_free(ssl->session->cert_chain, X509_free);
- ssl->session->cert_chain = chain;
+ sk_X509_pop_free(ssl->s3->new_session->cert_chain, X509_free);
+ ssl->s3->new_session->cert_chain = chain;
- X509_free(ssl->session->peer);
- ssl->session->peer = X509_up_ref(leaf);
+ X509_free(ssl->s3->new_session->peer);
+ ssl->s3->new_session->peer = X509_up_ref(leaf);
- ssl->session->verify_result = ssl->verify_result;
+ ssl->s3->new_session->verify_result = ssl->verify_result;
return 1;
@@ -1050,8 +1078,8 @@
goto f_err;
}
- if (!CBS_stow(&ocsp_response, &ssl->session->ocsp_response,
- &ssl->session->ocsp_response_length)) {
+ if (!CBS_stow(&ocsp_response, &ssl->s3->new_session->ocsp_response,
+ &ssl->s3->new_session->ocsp_response_length)) {
al = SSL_AD_INTERNAL_ERROR;
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
goto f_err;
@@ -1064,7 +1092,7 @@
}
static int ssl3_verify_server_cert(SSL *ssl) {
- int ret = ssl_verify_cert_chain(ssl, ssl->session->cert_chain);
+ int ret = ssl_verify_cert_chain(ssl, ssl->s3->new_session->cert_chain);
if (ssl->verify_mode != SSL_VERIFY_NONE && ret <= 0) {
int al = ssl_verify_alarm_type(ssl->verify_result);
ssl3_send_alert(ssl, SSL3_AL_FATAL, al);
@@ -1173,11 +1201,11 @@
goto err;
}
- ssl->session->key_exchange_info = DH_num_bits(dh);
- if (ssl->session->key_exchange_info < 1024) {
+ ssl->s3->new_session->key_exchange_info = DH_num_bits(dh);
+ if (ssl->s3->new_session->key_exchange_info < 1024) {
OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_DH_P_LENGTH);
goto err;
- } else if (ssl->session->key_exchange_info > 4096) {
+ } else if (ssl->s3->new_session->key_exchange_info > 4096) {
/* Overly large DHE groups are prohibitively expensive, so enforce a limit
* to prevent a server from causing us to perform too expensive of a
* computation. */
@@ -1210,7 +1238,7 @@
OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
goto f_err;
}
- ssl->session->key_exchange_info = group_id;
+ ssl->s3->new_session->key_exchange_info = group_id;
/* Ensure the group is consistent with preferences. */
if (!tls1_check_group_id(ssl, group_id)) {
@@ -1261,7 +1289,7 @@
/* ServerKeyExchange should be signed by the server's public key. */
if (ssl_cipher_uses_certificate_auth(ssl->s3->tmp.new_cipher)) {
- pkey = X509_get_pubkey(ssl->session->peer);
+ pkey = X509_get_pubkey(ssl->s3->new_session->peer);
if (pkey == NULL) {
goto err;
}
@@ -1518,9 +1546,9 @@
}
assert(psk_len <= PSK_MAX_PSK_LEN);
- OPENSSL_free(ssl->session->psk_identity);
- ssl->session->psk_identity = BUF_strdup(identity);
- if (ssl->session->psk_identity == NULL) {
+ OPENSSL_free(ssl->s3->new_session->psk_identity);
+ ssl->s3->new_session->psk_identity = BUF_strdup(identity);
+ if (ssl->s3->new_session->psk_identity == NULL) {
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
goto err;
}
@@ -1544,7 +1572,7 @@
goto err;
}
- EVP_PKEY *pkey = X509_get_pubkey(ssl->session->peer);
+ EVP_PKEY *pkey = X509_get_pubkey(ssl->s3->new_session->peer);
if (pkey == NULL) {
goto err;
}
@@ -1654,12 +1682,14 @@
}
ssl->state = SSL3_ST_CW_KEY_EXCH_B;
- ssl->session->master_key_length =
- tls1_generate_master_secret(ssl, ssl->session->master_key, pms, pms_len);
- if (ssl->session->master_key_length == 0) {
+ ssl->s3->new_session->master_key_length =
+ tls1_generate_master_secret(ssl, ssl->s3->new_session->master_key, pms,
+ pms_len);
+ if (ssl->s3->new_session->master_key_length == 0) {
goto err;
}
- ssl->session->extended_master_secret = ssl->s3->tmp.extended_master_secret;
+ ssl->s3->new_session->extended_master_secret =
+ ssl->s3->tmp.extended_master_secret;
OPENSSL_cleanse(pms, pms_len);
OPENSSL_free(pms);
@@ -1879,7 +1909,6 @@
}
static int ssl3_get_new_session_ticket(SSL *ssl) {
- int al;
int ret = ssl->method->ssl_get_message(ssl, SSL3_MT_NEW_SESSION_TICKET,
ssl_hash_message);
if (ret <= 0) {
@@ -1892,9 +1921,9 @@
if (!CBS_get_u32(&new_session_ticket, &ticket_lifetime_hint) ||
!CBS_get_u16_length_prefixed(&new_session_ticket, &ticket) ||
CBS_len(&new_session_ticket) != 0) {
- al = SSL_AD_DECODE_ERROR;
+ ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
- goto f_err;
+ return -1;
}
if (CBS_len(&ticket) == 0) {
@@ -1906,46 +1935,47 @@
return 1;
}
- if (ssl->hit) {
+ int session_renewed = ssl->session != NULL;
+ SSL_SESSION *session = ssl->s3->new_session;
+ if (session_renewed) {
/* The server is sending a new ticket for an existing session. Sessions are
* immutable once established, so duplicate all but the ticket of the
* existing session. */
- uint8_t *bytes;
- size_t bytes_len;
- if (!SSL_SESSION_to_bytes_for_ticket(ssl->session, &bytes, &bytes_len)) {
- goto err;
- }
- SSL_SESSION *new_session = SSL_SESSION_from_bytes(bytes, bytes_len);
- OPENSSL_free(bytes);
- if (new_session == NULL) {
+ session = SSL_SESSION_dup(ssl->session,
+ 0 /* Don't duplicate session ticket */);
+ if (session == NULL) {
/* This should never happen. */
OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
goto err;
}
-
- SSL_SESSION_free(ssl->session);
- ssl->session = new_session;
}
- if (!CBS_stow(&ticket, &ssl->session->tlsext_tick,
- &ssl->session->tlsext_ticklen)) {
+ if (!CBS_stow(&ticket, &session->tlsext_tick, &session->tlsext_ticklen)) {
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
goto err;
}
- ssl->session->tlsext_tick_lifetime_hint = ticket_lifetime_hint;
+ session->tlsext_tick_lifetime_hint = ticket_lifetime_hint;
/* Generate a session ID for this session based on the session ticket. We use
* the session ID mechanism for detecting ticket resumption. This also fits in
* with assumptions elsewhere in OpenSSL.*/
- if (!EVP_Digest(CBS_data(&ticket), CBS_len(&ticket), ssl->session->session_id,
- &ssl->session->session_id_length, EVP_sha256(), NULL)) {
+ if (!EVP_Digest(CBS_data(&ticket), CBS_len(&ticket),
+ session->session_id, &session->session_id_length,
+ EVP_sha256(), NULL)) {
goto err;
}
+ if (session_renewed) {
+ session->not_resumable = 0;
+ SSL_SESSION_free(ssl->session);
+ ssl->session = session;
+ }
+
return 1;
-f_err:
- ssl3_send_alert(ssl, SSL3_AL_FATAL, al);
err:
+ if (session_renewed) {
+ SSL_SESSION_free(session);
+ }
return -1;
}