Don't initialize enc_method before version negotiation. Move it into ssl->s3 so it automatically behaves correctly on SSL_clear. ssl->version is still a mess though. Change-Id: I17a692a04a845886ec4f8de229fa6cf99fa7e24a Reviewed-on: https://boringssl-review.googlesource.com/6844 Reviewed-by: Adam Langley <alangley@gmail.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index f6ed6f4..1723ba9 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h
@@ -3757,10 +3757,6 @@ * TLS). */ const SSL_PROTOCOL_METHOD *method; - /* enc_method is the method table corresponding to the current protocol - * version. */ - const SSL3_ENC_METHOD *enc_method; - /* There are 2 BIO's even though they are normally both the same. This is so * data can be read and written to different handlers */ @@ -4010,6 +4006,10 @@ /* aead_write_ctx is the current write cipher state. */ SSL_AEAD_CTX *aead_write_ctx; + /* enc_method is the method table corresponding to the current protocol + * version. */ + const SSL3_ENC_METHOD *enc_method; + /* State pertaining to the pending handshake. * * TODO(davidben): State is current spread all over the place. Move
diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c index 7f08b06..81079f4 100644 --- a/ssl/d1_lib.c +++ b/ssl/d1_lib.c
@@ -110,11 +110,11 @@ ssl->d1 = d1; - /* Set the version to the highest version for DTLS. This controls the initial - * state of |ssl->enc_method| and what the API reports as the version prior to - * negotiation. + /* Set the version to the highest supported version. * - * TODO(davidben): This is fragile and confusing. */ + * TODO(davidben): Move this field into |s3|, have it store the normalized + * protocol version, and implement this pre-negotiation quirk in |SSL_version| + * at the API boundary rather than in internal state. */ ssl->version = DTLS1_2_VERSION; return 1; }
diff --git a/ssl/s3_both.c b/ssl/s3_both.c index 1a91e0b..725c4f5 100644 --- a/ssl/s3_both.c +++ b/ssl/s3_both.c
@@ -163,8 +163,8 @@ if (ssl->state == a) { p = ssl_handshake_start(ssl); - n = ssl->enc_method->final_finish_mac(ssl, ssl->server, - ssl->s3->tmp.finish_md); + n = ssl->s3->enc_method->final_finish_mac(ssl, ssl->server, + ssl->s3->tmp.finish_md); if (n == 0) { return 0; } @@ -208,7 +208,7 @@ return; } - ssl->s3->tmp.peer_finish_md_len = ssl->enc_method->final_finish_mac( + ssl->s3->tmp.peer_finish_md_len = ssl->s3->enc_method->final_finish_mac( ssl, !ssl->server, ssl->s3->tmp.peer_finish_md); } @@ -449,15 +449,15 @@ } *out_len = len; } else if (pkey_type == EVP_PKEY_RSA) { - if (ssl->enc_method->cert_verify_mac(ssl, NID_md5, out) == 0 || - ssl->enc_method->cert_verify_mac(ssl, NID_sha1, - out + MD5_DIGEST_LENGTH) == 0) { + if (ssl->s3->enc_method->cert_verify_mac(ssl, NID_md5, out) == 0 || + ssl->s3->enc_method->cert_verify_mac(ssl, NID_sha1, + out + MD5_DIGEST_LENGTH) == 0) { return 0; } *out_len = MD5_DIGEST_LENGTH + SHA_DIGEST_LENGTH; *out_md = EVP_md5_sha1(); } else if (pkey_type == EVP_PKEY_EC) { - if (ssl->enc_method->cert_verify_mac(ssl, NID_sha1, out) == 0) { + if (ssl->s3->enc_method->cert_verify_mac(ssl, NID_sha1, out) == 0) { return 0; } *out_len = SHA_DIGEST_LENGTH;
diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index ec7a50b..09e527a 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c
@@ -786,8 +786,8 @@ goto f_err; } ssl->version = server_version; - ssl->enc_method = ssl3_get_enc_method(server_version); - assert(ssl->enc_method != NULL); + ssl->s3->enc_method = ssl3_get_enc_method(server_version); + assert(ssl->s3->enc_method != NULL); /* At this point, the connection's version is known and ssl->version is * fixed. Begin enforcing the record-layer version. */ ssl->s3->have_version = 1;
diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index 0d4a821..fa25579 100644 --- a/ssl/s3_lib.c +++ b/ssl/s3_lib.c
@@ -197,11 +197,11 @@ ssl->s3 = s3; - /* Set the version to the highest supported version for TLS. This controls the - * initial state of |ssl->enc_method| and what the API reports as the version - * prior to negotiation. + /* Set the version to the highest supported version. * - * TODO(davidben): This is fragile and confusing. */ + * TODO(davidben): Move this field into |s3|, have it store the normalized + * protocol version, and implement this pre-negotiation quirk in |SSL_version| + * at the API boundary rather than in internal state. */ ssl->version = TLS1_2_VERSION; return 1; err:
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index bbe178f..ae709dc 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c
@@ -871,8 +871,8 @@ goto f_err; } ssl->version = version; - ssl->enc_method = ssl3_get_enc_method(version); - assert(ssl->enc_method != NULL); + ssl->s3->enc_method = ssl3_get_enc_method(version); + assert(ssl->s3->enc_method != NULL); /* At this point, the connection's version is known and |ssl->version| is * fixed. Begin enforcing the record-layer version. */ ssl->s3->have_version = 1;
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 8eba906..ff77749 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c
@@ -417,8 +417,6 @@ if (!ssl->method->ssl_new(ssl)) { goto err; } - ssl->enc_method = ssl3_get_enc_method(ssl->version); - assert(ssl->enc_method != NULL); ssl->rwstate = SSL_NOTHING; @@ -2630,8 +2628,6 @@ if (!ssl->method->ssl_new(ssl)) { return 0; } - ssl->enc_method = ssl3_get_enc_method(ssl->version); - assert(ssl->enc_method != NULL); if (SSL_IS_DTLS(ssl) && (SSL_get_options(ssl) & SSL_OP_NO_QUERY_MTU)) { ssl->d1->mtu = mtu;
diff --git a/ssl/t1_enc.c b/ssl/t1_enc.c index 39711d5..a64e9d8 100644 --- a/ssl/t1_enc.c +++ b/ssl/t1_enc.c
@@ -328,7 +328,7 @@ } int SSL_generate_key_block(const SSL *ssl, uint8_t *out, size_t out_len) { - return ssl->enc_method->prf( + return ssl->s3->enc_method->prf( ssl, out, out_len, ssl->session->master_key, ssl->session->master_key_length, TLS_MD_KEY_EXPANSION_CONST, TLS_MD_KEY_EXPANSION_CONST_SIZE, ssl->s3->server_random, SSL3_RANDOM_SIZE, @@ -478,9 +478,10 @@ } static const size_t kFinishedLen = 12; - if (!ssl->enc_method->prf(ssl, out, kFinishedLen, ssl->session->master_key, - ssl->session->master_key_length, label, label_len, - buf, digests_len, NULL, 0)) { + if (!ssl->s3->enc_method->prf(ssl, out, kFinishedLen, + ssl->session->master_key, + ssl->session->master_key_length, label, + label_len, buf, digests_len, NULL, 0)) { return 0; } @@ -497,19 +498,19 @@ return 0; } - if (!ssl->enc_method->prf(ssl, out, SSL3_MASTER_SECRET_SIZE, premaster, - premaster_len, - TLS_MD_EXTENDED_MASTER_SECRET_CONST, - TLS_MD_EXTENDED_MASTER_SECRET_CONST_SIZE, digests, - digests_len, NULL, 0)) { + if (!ssl->s3->enc_method->prf(ssl, out, SSL3_MASTER_SECRET_SIZE, premaster, + premaster_len, + TLS_MD_EXTENDED_MASTER_SECRET_CONST, + TLS_MD_EXTENDED_MASTER_SECRET_CONST_SIZE, + digests, digests_len, NULL, 0)) { return 0; } } else { - if (!ssl->enc_method->prf(ssl, out, SSL3_MASTER_SECRET_SIZE, premaster, - premaster_len, TLS_MD_MASTER_SECRET_CONST, - TLS_MD_MASTER_SECRET_CONST_SIZE, - ssl->s3->client_random, SSL3_RANDOM_SIZE, - ssl->s3->server_random, SSL3_RANDOM_SIZE)) { + if (!ssl->s3->enc_method->prf(ssl, out, SSL3_MASTER_SECRET_SIZE, premaster, + premaster_len, TLS_MD_MASTER_SECRET_CONST, + TLS_MD_MASTER_SECRET_CONST_SIZE, + ssl->s3->client_random, SSL3_RANDOM_SIZE, + ssl->s3->server_random, SSL3_RANDOM_SIZE)) { return 0; } } @@ -547,9 +548,10 @@ memcpy(seed + 2 * SSL3_RANDOM_SIZE + 2, context, context_len); } - int ret = ssl->enc_method->prf(ssl, out, out_len, ssl->session->master_key, - ssl->session->master_key_length, label, - label_len, seed, seed_len, NULL, 0); + int ret = + ssl->s3->enc_method->prf(ssl, out, out_len, ssl->session->master_key, + ssl->session->master_key_length, label, + label_len, seed, seed_len, NULL, 0); OPENSSL_free(seed); return ret; }