From 62fd16283a4ac2b27449f8e4ac2d9af75d169083 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 11 Jan 2015 13:30:01 -0500 Subject: [PATCH] Implement SSL_clear with ssl_new and ssl_free. State on s3 gets freed in both ssl3_clear and ssl3_free. Considate to just ssl3_free. This replaces the (SSL,ssl,ssl3)_clear calls in (SSL,ssl,ssl3)_new with the state that was initialized. This results in a little code duplication between SSL_new and SSL_clear because state is on the wrong object. I've just left TODOs for now; some of it will need disentangling. We're far from it, but going forward, separate state between s and s->s3 as: - s contains configuration state, DTLS or TLS. It is initialized from SSL_CTX, configurable directly afterwards, and preserved across SSL_clear calls. (Including when it's implicitly set as part of a handshake callback.) - Connection state hangs off s->s3 (TLS) and s->d1 (DTLS). It is reset across SSL_clear. This should happen naturally out of a ssl_free/ssl_new pair. The goal is to avoid needing separate initialize and reset code for anything; the point any particular state is reset is the point its owning context is destroyed and recreated. Change-Id: I5d779010778109f8c339c07433a0777feaf94d1f Reviewed-on: https://boringssl-review.googlesource.com/2822 Reviewed-by: Adam Langley --- ssl/d1_lib.c | 47 ++++++------------------- ssl/d1_meth.c | 1 - ssl/s3_lib.c | 95 ++++---------------------------------------------- ssl/s3_meth.c | 1 - ssl/ssl_lib.c | 39 +++++++++++++++++++-- ssl/ssl_locl.h | 3 -- 6 files changed, 55 insertions(+), 131 deletions(-) diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c index 0b9e25c0..ce05b891 100644 --- a/ssl/d1_lib.c +++ b/ssl/d1_lib.c @@ -162,7 +162,13 @@ int dtls1_new(SSL *s) { } s->d1 = d1; - s->method->ssl_clear(s); + + /* Set the version to the highest version for DTLS. This controls the initial + * state of |s->enc_method| and what the API reports as the version prior to + * negotiation. + * + * TODO(davidben): This is fragile and confusing. */ + s->version = DTLS1_2_VERSION; return 1; } @@ -214,6 +220,10 @@ static void dtls1_clear_queues(SSL *s) { void dtls1_free(SSL *s) { ssl3_free(s); + if (s == NULL || s->d1 == NULL) { + return; + } + dtls1_clear_queues(s); pqueue_free(s->d1->unprocessed_rcds.q); @@ -226,41 +236,6 @@ void dtls1_free(SSL *s) { s->d1 = NULL; } -void dtls1_clear(SSL *s) { - pqueue unprocessed_rcds; - pqueue processed_rcds; - pqueue buffered_messages; - pqueue sent_messages; - pqueue buffered_app_data; - unsigned int mtu; - - if (s->d1) { - unprocessed_rcds = s->d1->unprocessed_rcds.q; - processed_rcds = s->d1->processed_rcds.q; - buffered_messages = s->d1->buffered_messages; - sent_messages = s->d1->sent_messages; - buffered_app_data = s->d1->buffered_app_data.q; - mtu = s->d1->mtu; - - dtls1_clear_queues(s); - - memset(s->d1, 0, sizeof(*(s->d1))); - - if (SSL_get_options(s) & SSL_OP_NO_QUERY_MTU) { - s->d1->mtu = mtu; - } - - s->d1->unprocessed_rcds.q = unprocessed_rcds; - s->d1->processed_rcds.q = processed_rcds; - s->d1->buffered_messages = buffered_messages; - s->d1->sent_messages = sent_messages; - s->d1->buffered_app_data.q = buffered_app_data; - } - - ssl3_clear(s); - s->version = DTLS1_2_VERSION; -} - long dtls1_ctrl(SSL *s, int cmd, long larg, void *parg) { int ret = 0; diff --git a/ssl/d1_meth.c b/ssl/d1_meth.c index c5a062b1..a894222c 100644 --- a/ssl/d1_meth.c +++ b/ssl/d1_meth.c @@ -60,7 +60,6 @@ static const SSL_PROTOCOL_METHOD DTLS_protocol_method = { dtls1_new, - dtls1_clear, dtls1_free, dtls1_accept, dtls1_connect, diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index e783c8f0..ef4e024f 100644 --- a/ssl/s3_lib.c +++ b/ssl/s3_lib.c @@ -640,15 +640,20 @@ int ssl3_new(SSL *s) { memset(s3->wrec.seq_num, 0, sizeof(s3->wrec.seq_num)); s->s3 = s3; - s->method->ssl_clear(s); + /* Set the version to the highest supported version for TLS. This controls the + * initial state of |s->enc_method| and what the API reports as the version + * prior to negotiation. + * + * TODO(davidben): This is fragile and confusing. */ + s->version = TLS1_2_VERSION; return 1; err: return 0; } void ssl3_free(SSL *s) { - if (s == NULL) { + if (s == NULL || s->s3 == NULL) { return; } @@ -699,92 +704,6 @@ void ssl3_free(SSL *s) { s->s3 = NULL; } -void ssl3_clear(SSL *s) { - uint8_t *rp, *wp; - size_t rlen, wlen; - int init_extra; - - /* TODO(davidben): Can this just call ssl3_free + ssl3_new. rbuf, wbuf, and - * init_extra are preserved, but this may not serve anything more than saving - * a malloc. */ - - if (s->s3->sniff_buffer != NULL) { - BUF_MEM_free(s->s3->sniff_buffer); - } - s->s3->sniff_buffer = NULL; - - ssl3_cleanup_key_block(s); - if (s->s3->tmp.ca_names != NULL) { - sk_X509_NAME_pop_free(s->s3->tmp.ca_names, X509_NAME_free); - } - s->s3->tmp.ca_names = NULL; - if (s->s3->tmp.certificate_types != NULL) { - OPENSSL_free(s->s3->tmp.certificate_types); - } - s->s3->tmp.certificate_types = NULL; - if (s->s3->tmp.peer_ecpointformatlist) { - OPENSSL_free(s->s3->tmp.peer_ecpointformatlist); - } - s->s3->tmp.peer_ecpointformatlist = NULL; - if (s->s3->tmp.peer_ellipticcurvelist) { - OPENSSL_free(s->s3->tmp.peer_ellipticcurvelist); - } - s->s3->tmp.peer_ellipticcurvelist = NULL; - if (s->s3->tmp.peer_psk_identity_hint) { - OPENSSL_free(s->s3->tmp.peer_psk_identity_hint); - } - s->s3->tmp.peer_psk_identity_hint = NULL; - - if (s->s3->tmp.dh != NULL) { - DH_free(s->s3->tmp.dh); - s->s3->tmp.dh = NULL; - } - if (s->s3->tmp.ecdh != NULL) { - EC_KEY_free(s->s3->tmp.ecdh); - s->s3->tmp.ecdh = NULL; - } - rp = s->s3->rbuf.buf; - wp = s->s3->wbuf.buf; - rlen = s->s3->rbuf.len; - wlen = s->s3->wbuf.len; - init_extra = s->s3->init_extra; - if (s->s3->handshake_buffer) { - BIO_free(s->s3->handshake_buffer); - s->s3->handshake_buffer = NULL; - } - if (s->s3->handshake_dgst) { - ssl3_free_digest_list(s); - } - - if (s->s3->alpn_selected) { - OPENSSL_free(s->s3->alpn_selected); - s->s3->alpn_selected = NULL; - } - memset(s->s3, 0, sizeof *s->s3); - s->s3->rbuf.buf = rp; - s->s3->wbuf.buf = wp; - s->s3->rbuf.len = rlen; - s->s3->wbuf.len = wlen; - s->s3->init_extra = init_extra; - - ssl_free_wbio_buffer(s); - - s->packet_length = 0; - s->s3->renegotiate = 0; - s->s3->total_renegotiations = 0; - s->s3->num_renegotiations = 0; - s->s3->in_read_app_data = 0; - s->version = TLS1_2_VERSION; - - if (s->next_proto_negotiated) { - OPENSSL_free(s->next_proto_negotiated); - s->next_proto_negotiated = NULL; - s->next_proto_negotiated_len = 0; - } - - s->s3->tlsext_channel_id_valid = 0; -} - static int ssl3_set_req_cert_type(CERT *c, const uint8_t *p, size_t len); long ssl3_ctrl(SSL *s, int cmd, long larg, void *parg) { diff --git a/ssl/s3_meth.c b/ssl/s3_meth.c index 2ab1cf79..5a25d7b4 100644 --- a/ssl/s3_meth.c +++ b/ssl/s3_meth.c @@ -59,7 +59,6 @@ static const SSL_PROTOCOL_METHOD TLS_protocol_method = { ssl3_new, - ssl3_clear, ssl3_free, ssl3_accept, ssl3_connect, diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 76f74084..0118aa55 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -190,6 +190,12 @@ int SSL_clear(SSL *s) { assert(s->state == 0); } + /* TODO(davidben): Some state on |s| is reset both in |SSL_new| and + * |SSL_clear| because it is per-connection state rather than configuration + * state. Per-connection state should be on |s->s3| and |s->d1| so it is + * naturally reset at the right points between |SSL_new|, |SSL_clear|, and + * |ssl3_new|. */ + s->rwstate = SSL_NOTHING; s->rstate = SSL_ST_READ_HEADER; @@ -198,11 +204,39 @@ int SSL_clear(SSL *s) { s->init_buf = NULL; } + s->packet = NULL; + s->packet_length = 0; + ssl_clear_cipher_ctx(s); ssl_clear_hash_ctx(&s->read_hash); ssl_clear_hash_ctx(&s->write_hash); - s->method->ssl_clear(s); + if (s->next_proto_negotiated) { + OPENSSL_free(s->next_proto_negotiated); + s->next_proto_negotiated = NULL; + s->next_proto_negotiated_len = 0; + } + + /* The s->d1->mtu is simultaneously configuration (preserved across + * clear) and connection-specific state (gets reset). + * + * TODO(davidben): Avoid this. */ + unsigned mtu = 0; + if (s->d1 != NULL) { + mtu = s->d1->mtu; + } + + s->method->ssl_free(s); + if (!s->method->ssl_new(s)) { + return 0; + } + s->enc_method = ssl3_get_enc_method(s->version); + assert(s->enc_method != NULL); + + if (SSL_IS_DTLS(s) && (SSL_get_options(s) & SSL_OP_NO_QUERY_MTU)) { + s->d1->mtu = mtu; + } + s->client_version = s->version; return 1; @@ -315,7 +349,8 @@ SSL *SSL_new(SSL_CTX *ctx) { s->references = 1; - SSL_clear(s); + s->rwstate = SSL_NOTHING; + s->rstate = SSL_ST_READ_HEADER; CRYPTO_new_ex_data(CRYPTO_EX_INDEX_SSL, s, &s->ex_data); diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 283e89f8..0364097e 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -559,7 +559,6 @@ struct ssl_method_st { /* Used to hold functions for SSLv2 or SSLv3/TLSv1 functions */ struct ssl_protocol_method_st { int (*ssl_new)(SSL *s); - void (*ssl_clear)(SSL *s); void (*ssl_free)(SSL *s); int (*ssl_accept)(SSL *s); int (*ssl_connect)(SSL *s); @@ -792,7 +791,6 @@ int ssl3_read(SSL *s, void *buf, int len); int ssl3_peek(SSL *s, void *buf, int len); int ssl3_write(SSL *s, const void *buf, int len); int ssl3_shutdown(SSL *s); -void ssl3_clear(SSL *s); long ssl3_ctrl(SSL *s, int cmd, long larg, void *parg); long ssl3_ctx_ctrl(SSL_CTX *s, int cmd, long larg, void *parg); long ssl3_callback_ctrl(SSL *s, int cmd, void (*fp)(void)); @@ -877,7 +875,6 @@ int dtls1_new(SSL *s); int dtls1_accept(SSL *s); int dtls1_connect(SSL *s); void dtls1_free(SSL *s); -void dtls1_clear(SSL *s); long dtls1_ctrl(SSL *s, int cmd, long larg, void *parg); int dtls1_shutdown(SSL *s);