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 <agl@google.com>
diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c
index 0b9e25c..ce05b89 100644
--- a/ssl/d1_lib.c
+++ b/ssl/d1_lib.c
@@ -162,7 +162,13 @@
}
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 @@
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 @@
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 c5a062b..a894222 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 e783c8f..ef4e024 100644
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -640,15 +640,20 @@
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 @@
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 2ab1cf7..5a25d7b 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 76f7408..0118aa5 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -190,6 +190,12 @@
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 @@
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 @@
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 283e89f..0364097 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -559,7 +559,6 @@
/* 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_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_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);
