From a4c8ff01903c56277d4218341e9a68864bc6aea9 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 8 Oct 2016 02:49:01 -0400 Subject: [PATCH] Move TLS 1.2 key exchange fields to SSL_HANDSHAKE. SSL_HANDSHAKE is dropped after the handshake, so I've removed the logic around smaller sizes. It's much simpler when we can use CBS_stow and CBB_finish without extra bounds-checking. Change-Id: Idafaa5d69e171aed9a8759f3d44e52cb01c40f39 Reviewed-on: https://boringssl-review.googlesource.com/11567 Reviewed-by: Adam Langley --- include/openssl/ssl.h | 12 ------------ ssl/handshake_client.c | 42 ++++++++++++++---------------------------- ssl/handshake_server.c | 42 +++++++++++++++++++----------------------- ssl/internal.h | 11 ++++++++++- ssl/s3_both.c | 2 ++ ssl/s3_lib.c | 3 --- 6 files changed, 45 insertions(+), 67 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index dde36f9c..34eac9af 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -4355,18 +4355,6 @@ typedef struct ssl3_state_st { /* peer_signature_algorithm is the signature algorithm used to authenticate * the peer, or zero if not applicable. */ uint16_t peer_signature_algorithm; - - /* ecdh_ctx is the current ECDH instance. */ - SSL_ECDH_CTX ecdh_ctx; - - /* peer_key is the peer's ECDH key. */ - uint8_t *peer_key; - uint16_t peer_key_len; - - /* server_params stores the ServerKeyExchange parameters to be signed while - * the signature is being computed. */ - uint8_t *server_params; - uint32_t server_params_len; } tmp; /* new_session is the new mutable session being established by the current diff --git a/ssl/handshake_client.c b/ssl/handshake_client.c index 5bde5676..a917e024 100644 --- a/ssl/handshake_client.c +++ b/ssl/handshake_client.c @@ -1213,18 +1213,13 @@ static int ssl3_get_server_key_exchange(SSL *ssl) { goto err; } - SSL_ECDH_CTX_init_for_dhe(&ssl->s3->tmp.ecdh_ctx, dh); + SSL_ECDH_CTX_init_for_dhe(&ssl->s3->hs->ecdh_ctx, dh); dh = NULL; /* Save the peer public key for later. */ - size_t peer_key_len; - if (!CBS_stow(&dh_Ys, &ssl->s3->tmp.peer_key, &peer_key_len)) { + if (!CBS_stow(&dh_Ys, &ssl->s3->hs->peer_key, &ssl->s3->hs->peer_key_len)) { goto err; } - /* |dh_Ys| was initialized with CBS_get_u16_length_prefixed, so peer_key_len - * fits in a uint16_t. */ - assert(sizeof(ssl->s3->tmp.peer_key_len) == 2 && peer_key_len <= 0xffff); - ssl->s3->tmp.peer_key_len = (uint16_t)peer_key_len; } else if (alg_k & SSL_kECDHE) { /* Parse the server parameters. */ uint8_t group_type; @@ -1248,17 +1243,12 @@ static int ssl3_get_server_key_exchange(SSL *ssl) { } /* Initialize ECDH and save the peer public key for later. */ - size_t peer_key_len; - if (!SSL_ECDH_CTX_init(&ssl->s3->tmp.ecdh_ctx, group_id) || - !CBS_stow(&point, &ssl->s3->tmp.peer_key, &peer_key_len)) { + if (!SSL_ECDH_CTX_init(&ssl->s3->hs->ecdh_ctx, group_id) || + !CBS_stow(&point, &ssl->s3->hs->peer_key, &ssl->s3->hs->peer_key_len)) { goto err; } - /* |point| was initialized with CBS_get_u8_length_prefixed, so peer_key_len - * fits in a uint16_t. */ - assert(sizeof(ssl->s3->tmp.peer_key_len) == 2 && peer_key_len <= 0xffff); - ssl->s3->tmp.peer_key_len = (uint16_t)peer_key_len; } else if (alg_k & SSL_kCECPQ1) { - SSL_ECDH_CTX_init_for_cecpq1(&ssl->s3->tmp.ecdh_ctx); + SSL_ECDH_CTX_init_for_cecpq1(&ssl->s3->hs->ecdh_ctx); CBS key; if (!CBS_get_u16_length_prefixed(&server_key_exchange, &key)) { al = SSL_AD_DECODE_ERROR; @@ -1266,14 +1256,9 @@ static int ssl3_get_server_key_exchange(SSL *ssl) { goto f_err; } - size_t peer_key_len; - if (!CBS_stow(&key, &ssl->s3->tmp.peer_key, &peer_key_len)) { + if (!CBS_stow(&key, &ssl->s3->hs->peer_key, &ssl->s3->hs->peer_key_len)) { goto err; } - /* |key| was initialized with CBS_get_u16_length_prefixed, so peer_key_len - * fits in a uint16_t. */ - assert(sizeof(ssl->s3->tmp.peer_key_len) == 2 && peer_key_len <= 0xffff); - ssl->s3->tmp.peer_key_len = (uint16_t)peer_key_len; } else if (!(alg_k & SSL_kPSK)) { al = SSL_AD_UNEXPECTED_MESSAGE; OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_MESSAGE); @@ -1620,15 +1605,15 @@ static int ssl3_send_client_key_exchange(SSL *ssl) { } else if (alg_k & (SSL_kECDHE|SSL_kDHE|SSL_kCECPQ1)) { /* Generate a keypair and serialize the public half. */ CBB child; - if (!SSL_ECDH_CTX_add_key(&ssl->s3->tmp.ecdh_ctx, &body, &child)) { + if (!SSL_ECDH_CTX_add_key(&ssl->s3->hs->ecdh_ctx, &body, &child)) { goto err; } /* Compute the premaster. */ uint8_t alert; - if (!SSL_ECDH_CTX_accept(&ssl->s3->tmp.ecdh_ctx, &child, &pms, &pms_len, - &alert, ssl->s3->tmp.peer_key, - ssl->s3->tmp.peer_key_len)) { + if (!SSL_ECDH_CTX_accept(&ssl->s3->hs->ecdh_ctx, &child, &pms, &pms_len, + &alert, ssl->s3->hs->peer_key, + ssl->s3->hs->peer_key_len)) { ssl3_send_alert(ssl, SSL3_AL_FATAL, alert); goto err; } @@ -1637,9 +1622,10 @@ static int ssl3_send_client_key_exchange(SSL *ssl) { } /* The key exchange state may now be discarded. */ - SSL_ECDH_CTX_cleanup(&ssl->s3->tmp.ecdh_ctx); - OPENSSL_free(ssl->s3->tmp.peer_key); - ssl->s3->tmp.peer_key = NULL; + SSL_ECDH_CTX_cleanup(&ssl->s3->hs->ecdh_ctx); + OPENSSL_free(ssl->s3->hs->peer_key); + ssl->s3->hs->peer_key = NULL; + ssl->s3->hs->peer_key_len = 0; } else if (alg_k & SSL_kPSK) { /* For plain PSK, other_secret is a block of 0s with the same length as * the pre-shared key. */ diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c index 99df871e..83e09d30 100644 --- a/ssl/handshake_server.c +++ b/ssl/handshake_server.c @@ -1050,13 +1050,13 @@ static int ssl3_send_server_key_exchange(SSL *ssl) { goto err; } - SSL_ECDH_CTX_init_for_dhe(&ssl->s3->tmp.ecdh_ctx, dh); + SSL_ECDH_CTX_init_for_dhe(&ssl->s3->hs->ecdh_ctx, dh); if (!CBB_add_u16_length_prefixed(&cbb, &child) || !BN_bn2cbb_padded(&child, BN_num_bytes(params->p), params->p) || !CBB_add_u16_length_prefixed(&cbb, &child) || !BN_bn2cbb_padded(&child, BN_num_bytes(params->g), params->g) || !CBB_add_u16_length_prefixed(&cbb, &child) || - !SSL_ECDH_CTX_offer(&ssl->s3->tmp.ecdh_ctx, &child)) { + !SSL_ECDH_CTX_offer(&ssl->s3->hs->ecdh_ctx, &child)) { goto err; } } else if (alg_k & SSL_kECDHE) { @@ -1070,39 +1070,35 @@ static int ssl3_send_server_key_exchange(SSL *ssl) { ssl->s3->new_session->key_exchange_info = group_id; /* Set up ECDH, generate a key, and emit the public half. */ - if (!SSL_ECDH_CTX_init(&ssl->s3->tmp.ecdh_ctx, group_id) || + if (!SSL_ECDH_CTX_init(&ssl->s3->hs->ecdh_ctx, group_id) || !CBB_add_u8(&cbb, NAMED_CURVE_TYPE) || !CBB_add_u16(&cbb, group_id) || !CBB_add_u8_length_prefixed(&cbb, &child) || - !SSL_ECDH_CTX_offer(&ssl->s3->tmp.ecdh_ctx, &child)) { + !SSL_ECDH_CTX_offer(&ssl->s3->hs->ecdh_ctx, &child)) { goto err; } } else if (alg_k & SSL_kCECPQ1) { - SSL_ECDH_CTX_init_for_cecpq1(&ssl->s3->tmp.ecdh_ctx); + SSL_ECDH_CTX_init_for_cecpq1(&ssl->s3->hs->ecdh_ctx); if (!CBB_add_u16_length_prefixed(&cbb, &child) || - !SSL_ECDH_CTX_offer(&ssl->s3->tmp.ecdh_ctx, &child)) { + !SSL_ECDH_CTX_offer(&ssl->s3->hs->ecdh_ctx, &child)) { goto err; } } else { assert(alg_k & SSL_kPSK); } - size_t len; - if (!CBB_finish(&cbb, &ssl->s3->tmp.server_params, &len) || - len > 0xffffffffu) { - OPENSSL_free(ssl->s3->tmp.server_params); - ssl->s3->tmp.server_params = NULL; + if (!CBB_finish(&cbb, &ssl->s3->hs->server_params, + &ssl->s3->hs->server_params_len)) { goto err; } - ssl->s3->tmp.server_params_len = (uint32_t)len; } /* Assemble the message. */ CBB body; if (!ssl->method->init_message(ssl, &cbb, &body, SSL3_MT_SERVER_KEY_EXCHANGE) || - !CBB_add_bytes(&body, ssl->s3->tmp.server_params, - ssl->s3->tmp.server_params_len)) { + !CBB_add_bytes(&body, ssl->s3->hs->server_params, + ssl->s3->hs->server_params_len)) { goto err; } @@ -1141,11 +1137,11 @@ static int ssl3_send_server_key_exchange(SSL *ssl) { uint8_t *transcript_data; size_t transcript_len; if (!CBB_init(&transcript, - 2*SSL3_RANDOM_SIZE + ssl->s3->tmp.server_params_len) || + 2*SSL3_RANDOM_SIZE + ssl->s3->hs->server_params_len) || !CBB_add_bytes(&transcript, ssl->s3->client_random, SSL3_RANDOM_SIZE) || !CBB_add_bytes(&transcript, ssl->s3->server_random, SSL3_RANDOM_SIZE) || - !CBB_add_bytes(&transcript, ssl->s3->tmp.server_params, - ssl->s3->tmp.server_params_len) || + !CBB_add_bytes(&transcript, ssl->s3->hs->server_params, + ssl->s3->hs->server_params_len) || !CBB_finish(&transcript, &transcript_data, &transcript_len)) { CBB_cleanup(&transcript); OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); @@ -1181,9 +1177,9 @@ static int ssl3_send_server_key_exchange(SSL *ssl) { goto err; } - OPENSSL_free(ssl->s3->tmp.server_params); - ssl->s3->tmp.server_params = NULL; - ssl->s3->tmp.server_params_len = 0; + OPENSSL_free(ssl->s3->hs->server_params); + ssl->s3->hs->server_params = NULL; + ssl->s3->hs->server_params_len = 0; ssl->state = SSL3_ST_SW_KEY_EXCH_C; return ssl->method->write_message(ssl); @@ -1571,7 +1567,7 @@ static int ssl3_get_client_key_exchange(SSL *ssl) { } else if (alg_k & (SSL_kECDHE|SSL_kDHE|SSL_kCECPQ1)) { /* Parse the ClientKeyExchange. */ CBS peer_key; - if (!SSL_ECDH_CTX_get_key(&ssl->s3->tmp.ecdh_ctx, &client_key_exchange, + if (!SSL_ECDH_CTX_get_key(&ssl->s3->hs->ecdh_ctx, &client_key_exchange, &peer_key) || CBS_len(&client_key_exchange) != 0) { al = SSL_AD_DECODE_ERROR; @@ -1581,7 +1577,7 @@ static int ssl3_get_client_key_exchange(SSL *ssl) { /* Compute the premaster. */ uint8_t alert; - if (!SSL_ECDH_CTX_finish(&ssl->s3->tmp.ecdh_ctx, &premaster_secret, + if (!SSL_ECDH_CTX_finish(&ssl->s3->hs->ecdh_ctx, &premaster_secret, &premaster_secret_len, &alert, CBS_data(&peer_key), CBS_len(&peer_key))) { al = alert; @@ -1589,7 +1585,7 @@ static int ssl3_get_client_key_exchange(SSL *ssl) { } /* The key exchange state may now be discarded. */ - SSL_ECDH_CTX_cleanup(&ssl->s3->tmp.ecdh_ctx); + SSL_ECDH_CTX_cleanup(&ssl->s3->hs->ecdh_ctx); } else if (alg_k & SSL_kPSK) { /* For plain PSK, other_secret is a block of 0s with the same length as the * pre-shared key. */ diff --git a/ssl/internal.h b/ssl/internal.h index 591e6ab0..0243ac49 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -919,7 +919,7 @@ struct ssl_handshake_st { uint16_t received; } custom_extensions; - /* ecdh_ctx is the active client ECDH offer in TLS 1.3. */ + /* ecdh_ctx is the current ECDH instance. */ SSL_ECDH_CTX ecdh_ctx; /* retry_group is the group ID selected by the server in HelloRetryRequest in @@ -949,6 +949,15 @@ struct ssl_handshake_st { uint16_t *peer_supported_group_list; size_t peer_supported_group_list_len; + /* peer_key is the peer's ECDH key for a TLS 1.2 client. */ + uint8_t *peer_key; + size_t peer_key_len; + + /* server_params, in TLS 1.2, stores the ServerKeyExchange parameters to be + * signed while the signature is being computed. */ + uint8_t *server_params; + size_t server_params_len; + /* session_tickets_sent, in TLS 1.3, is the number of tickets the server has * sent. */ uint8_t session_tickets_sent; diff --git a/ssl/s3_both.c b/ssl/s3_both.c index 3ed37935..e0d97dc6 100644 --- a/ssl/s3_both.c +++ b/ssl/s3_both.c @@ -150,6 +150,8 @@ void ssl_handshake_free(SSL_HANDSHAKE *hs) { OPENSSL_cleanse(hs->secret, sizeof(hs->secret)); OPENSSL_cleanse(hs->traffic_secret_0, sizeof(hs->traffic_secret_0)); SSL_ECDH_CTX_cleanup(&hs->ecdh_ctx); + OPENSSL_free(hs->peer_key); + OPENSSL_free(hs->server_params); OPENSSL_free(hs->key_share_bytes); OPENSSL_free(hs->public_key); OPENSSL_free(hs->peer_sigalgs); diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index 1515f94d..69d3a9dc 100644 --- a/ssl/s3_lib.c +++ b/ssl/s3_lib.c @@ -203,9 +203,6 @@ void ssl3_free(SSL *ssl) { ssl3_cleanup_key_block(ssl); ssl_read_buffer_clear(ssl); ssl_write_buffer_clear(ssl); - SSL_ECDH_CTX_cleanup(&ssl->s3->tmp.ecdh_ctx); - OPENSSL_free(ssl->s3->tmp.peer_key); - OPENSSL_free(ssl->s3->tmp.server_params); SSL_SESSION_free(ssl->s3->new_session); SSL_SESSION_free(ssl->s3->established_session);