From 43612b6bc742dc4ac242b8e5a10a9f34da29d7d5 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 7 Oct 2016 00:41:50 -0400 Subject: [PATCH] Move peer_supported_group_list to SSL_HANDSHAKE. Now not only the pointers but also the list itself is released after the handshake completes. Change-Id: I8b568147d2d4949b3b0efe58a93905f77a5a4481 Reviewed-on: https://boringssl-review.googlesource.com/11528 Reviewed-by: Adam Langley --- include/openssl/ssl.h | 6 ------ ssl/internal.h | 6 ++++++ ssl/s3_both.c | 1 + ssl/s3_lib.c | 1 - ssl/t1_lib.c | 28 +++++++++++----------------- 5 files changed, 18 insertions(+), 24 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 7b9875f1..dde36f9c 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -4344,12 +4344,6 @@ typedef struct ssl3_state_st { uint8_t new_key_len; uint8_t new_fixed_iv_len; - /* Server-only: peer_supported_group_list contains the supported group IDs - * advertised by the peer. This is only set on the server's end. The server - * does not advertise this extension to the client. */ - uint16_t *peer_supported_group_list; - size_t peer_supported_group_list_len; - /* extended_master_secret indicates whether the extended master secret * computation is used in this handshake. Note that this is different from * whether it was used for the current session. If this is a resumption diff --git a/ssl/internal.h b/ssl/internal.h index 1efc4869..591e6ab0 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -943,6 +943,12 @@ struct ssl_handshake_st { /* num_peer_sigalgs is the number of entries in |peer_sigalgs|. */ size_t num_peer_sigalgs; + /* peer_supported_group_list contains the supported group IDs advertised by + * the peer. This is only set on the server's end. The server does not + * advertise this extension to the client. */ + uint16_t *peer_supported_group_list; + size_t peer_supported_group_list_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 5092a3b8..3ed37935 100644 --- a/ssl/s3_both.c +++ b/ssl/s3_both.c @@ -153,6 +153,7 @@ void ssl_handshake_free(SSL_HANDSHAKE *hs) { OPENSSL_free(hs->key_share_bytes); OPENSSL_free(hs->public_key); OPENSSL_free(hs->peer_sigalgs); + OPENSSL_free(hs->peer_supported_group_list); OPENSSL_free(hs->peer_psk_identity_hint); sk_X509_NAME_pop_free(hs->ca_names, X509_NAME_free); OPENSSL_free(hs->certificate_types); diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index 472c9cb4..1515f94d 100644 --- a/ssl/s3_lib.c +++ b/ssl/s3_lib.c @@ -207,7 +207,6 @@ void ssl3_free(SSL *ssl) { OPENSSL_free(ssl->s3->tmp.peer_key); OPENSSL_free(ssl->s3->tmp.server_params); - OPENSSL_free(ssl->s3->tmp.peer_supported_group_list); SSL_SESSION_free(ssl->s3->new_session); SSL_SESSION_free(ssl->s3->established_session); ssl3_free_handshake_buffer(ssl); diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index a134c3fe..06595d2a 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -341,11 +341,11 @@ int tls1_get_shared_group(SSL *ssl, uint16_t *out_group_id) { if (ssl->options & SSL_OP_CIPHER_SERVER_PREFERENCE) { pref = groups; pref_len = groups_len; - supp = ssl->s3->tmp.peer_supported_group_list; - supp_len = ssl->s3->tmp.peer_supported_group_list_len; + supp = ssl->s3->hs->peer_supported_group_list; + supp_len = ssl->s3->hs->peer_supported_group_list_len; } else { - pref = ssl->s3->tmp.peer_supported_group_list; - pref_len = ssl->s3->tmp.peer_supported_group_list_len; + pref = ssl->s3->hs->peer_supported_group_list; + pref_len = ssl->s3->hs->peer_supported_group_list_len; supp = groups; supp_len = groups_len; } @@ -2259,12 +2259,6 @@ static int ext_supported_versions_add_clienthello(SSL *ssl, CBB *out) { * https://tools.ietf.org/html/rfc4492#section-5.1.2 * https://tools.ietf.org/html/draft-ietf-tls-tls13-12#section-6.3.2.2 */ -static void ext_supported_groups_init(SSL *ssl) { - OPENSSL_free(ssl->s3->tmp.peer_supported_group_list); - ssl->s3->tmp.peer_supported_group_list = NULL; - ssl->s3->tmp.peer_supported_group_list_len = 0; -} - static int ext_supported_groups_add_clienthello(SSL *ssl, CBB *out) { if (!ssl_any_ec_cipher_suites_enabled(ssl)) { return 1; @@ -2318,9 +2312,9 @@ static int ext_supported_groups_parse_clienthello(SSL *ssl, uint8_t *out_alert, return 0; } - ssl->s3->tmp.peer_supported_group_list = OPENSSL_malloc( + ssl->s3->hs->peer_supported_group_list = OPENSSL_malloc( CBS_len(&supported_group_list)); - if (ssl->s3->tmp.peer_supported_group_list == NULL) { + if (ssl->s3->hs->peer_supported_group_list == NULL) { *out_alert = SSL_AD_INTERNAL_ERROR; return 0; } @@ -2328,19 +2322,19 @@ static int ext_supported_groups_parse_clienthello(SSL *ssl, uint8_t *out_alert, const size_t num_groups = CBS_len(&supported_group_list) / 2; for (size_t i = 0; i < num_groups; i++) { if (!CBS_get_u16(&supported_group_list, - &ssl->s3->tmp.peer_supported_group_list[i])) { + &ssl->s3->hs->peer_supported_group_list[i])) { goto err; } } assert(CBS_len(&supported_group_list) == 0); - ssl->s3->tmp.peer_supported_group_list_len = num_groups; + ssl->s3->hs->peer_supported_group_list_len = num_groups; return 1; err: - OPENSSL_free(ssl->s3->tmp.peer_supported_group_list); - ssl->s3->tmp.peer_supported_group_list = NULL; + OPENSSL_free(ssl->s3->hs->peer_supported_group_list); + ssl->s3->hs->peer_supported_group_list = NULL; *out_alert = SSL_AD_INTERNAL_ERROR; return 0; } @@ -2479,7 +2473,7 @@ static const struct tls_extension kExtensions[] = { * https://crbug.com/363583. */ { TLSEXT_TYPE_supported_groups, - ext_supported_groups_init, + NULL, ext_supported_groups_add_clienthello, ext_supported_groups_parse_serverhello, ext_supported_groups_parse_clienthello,