Avoid relying on SSL_get_session's behavior during the handshake.
Mid-renegotiation, there are a lot of sets of TLS parameters flying
around. We need to be clear which one we want for each operation. There
were a few parts of TLS 1.2 which were relying on SSL_get_session to
abstract between the resumption session and a new session.
Implement that separately as ssl_handshake_session, so we're free to
avoid SSL_get_session returning an incomplete session mid-renegotiation.
This doesn't fixed the linked Chromium bug, but it is necessary to do
so. (I'm trying to separate the SSL_get_session change from the
dependencies within the library.)
Update-Note: SSL_generate_key_block will now fail mid-handshake. It is
ambiguous which key block to use and, in some cases, we may not even be
able to compute the right key block.
Bug: chromium:1010748
Change-Id: I30c8a683bb506310e37adbd05a28e3b8de6e6836
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/41865
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 8e11ef2..f804928 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -3604,11 +3604,13 @@
const uint8_t **out_write_iv,
size_t *out_iv_len);
-// SSL_get_key_block_len returns the length of |ssl|'s key block.
+// SSL_get_key_block_len returns the length of |ssl|'s key block. It is an error
+// to call this function during a handshake.
OPENSSL_EXPORT size_t SSL_get_key_block_len(const SSL *ssl);
// SSL_generate_key_block generates |out_len| bytes of key material for |ssl|'s
-// current connection state.
+// current connection state. It is an error to call this function during a
+// handshake.
OPENSSL_EXPORT int SSL_generate_key_block(const SSL *ssl, uint8_t *out,
size_t out_len);
diff --git a/ssl/handoff.cc b/ssl/handoff.cc
index fdbac18..977fcc5 100644
--- a/ssl/handoff.cc
+++ b/ssl/handoff.cc
@@ -632,7 +632,7 @@
case handback_after_session_resumption:
// The write keys are installed after server Finished, but the client
// keys must wait for ChangeCipherSpec.
- if (!tls1_configure_aead(ssl, evp_aead_seal, &key_block, session->cipher,
+ if (!tls1_configure_aead(ssl, evp_aead_seal, &key_block, session,
write_iv)) {
return false;
}
@@ -642,9 +642,9 @@
break;
case handback_after_handshake:
// The handshake is complete, so both keys are installed.
- if (!tls1_configure_aead(ssl, evp_aead_seal, &key_block, session->cipher,
+ if (!tls1_configure_aead(ssl, evp_aead_seal, &key_block, session,
write_iv) ||
- !tls1_configure_aead(ssl, evp_aead_open, &key_block, session->cipher,
+ !tls1_configure_aead(ssl, evp_aead_open, &key_block, session,
read_iv)) {
return false;
}
diff --git a/ssl/handshake.cc b/ssl/handshake.cc
index 27fc3af..7bceb1d 100644
--- a/ssl/handshake.cc
+++ b/ssl/handshake.cc
@@ -441,7 +441,7 @@
uint8_t finished[EVP_MAX_MD_SIZE];
size_t finished_len;
if (!hs->transcript.GetFinishedMAC(finished, &finished_len,
- SSL_get_session(ssl), !ssl->server) ||
+ ssl_handshake_session(hs), !ssl->server) ||
!ssl_hash_message(hs, msg)) {
return ssl_hs_error;
}
@@ -484,7 +484,7 @@
bool ssl_send_finished(SSL_HANDSHAKE *hs) {
SSL *const ssl = hs->ssl;
- const SSL_SESSION *session = SSL_get_session(ssl);
+ const SSL_SESSION *session = ssl_handshake_session(hs);
uint8_t finished[EVP_MAX_MD_SIZE];
size_t finished_len;
@@ -541,6 +541,13 @@
return true;
}
+const SSL_SESSION *ssl_handshake_session(const SSL_HANDSHAKE *hs) {
+ if (hs->new_session) {
+ return hs->new_session.get();
+ }
+ return hs->ssl->session.get();
+}
+
int ssl_run_handshake(SSL_HANDSHAKE *hs, bool *out_early_return) {
SSL *const ssl = hs->ssl;
for (;;) {
diff --git a/ssl/internal.h b/ssl/internal.h
index 182b02f..2000409 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1940,6 +1940,11 @@
bool ssl_send_finished(SSL_HANDSHAKE *hs);
bool ssl_output_cert_chain(SSL_HANDSHAKE *hs);
+// ssl_handshake_session returns the |SSL_SESSION| corresponding to the current
+// handshake. Note, in TLS 1.2 resumptions, this session is immutable.
+const SSL_SESSION *ssl_handshake_session(const SSL_HANDSHAKE *hs);
+
+
// SSLKEYLOGFILE functions.
// ssl_log_secret logs |secret| with label |label|, if logging is enabled for
@@ -2921,13 +2926,14 @@
// determined by |direction|) using the keys generated by the TLS KDF. The
// |key_block_cache| argument is used to store the generated key block, if
// empty. Otherwise it's assumed that the key block is already contained within
-// it. Returns one on success or zero on error.
-int tls1_configure_aead(SSL *ssl, evp_aead_direction_t direction,
- Array<uint8_t> *key_block_cache,
- const SSL_CIPHER *cipher,
- Span<const uint8_t> iv_override);
+// it. It returns true on success or false on error.
+bool tls1_configure_aead(SSL *ssl, evp_aead_direction_t direction,
+ Array<uint8_t> *key_block_cache,
+ const SSL_SESSION *session,
+ Span<const uint8_t> iv_override);
-int tls1_change_cipher_state(SSL_HANDSHAKE *hs, evp_aead_direction_t direction);
+bool tls1_change_cipher_state(SSL_HANDSHAKE *hs,
+ evp_aead_direction_t direction);
int tls1_generate_master_secret(SSL_HANDSHAKE *hs, uint8_t *out,
Span<const uint8_t> premaster);
diff --git a/ssl/t1_enc.cc b/ssl/t1_enc.cc
index 73b6544..d8b6ea2 100644
--- a/ssl/t1_enc.cc
+++ b/ssl/t1_enc.cc
@@ -189,21 +189,36 @@
return true;
}
-int tls1_configure_aead(SSL *ssl, evp_aead_direction_t direction,
- Array<uint8_t> *key_block_cache,
- const SSL_CIPHER *cipher,
- Span<const uint8_t> iv_override) {
+static bool generate_key_block(const SSL *ssl, Span<uint8_t> out,
+ const SSL_SESSION *session) {
+ auto master_key =
+ MakeConstSpan(session->master_key, session->master_key_length);
+ static const char kLabel[] = "key expansion";
+ auto label = MakeConstSpan(kLabel, sizeof(kLabel) - 1);
+
+ const EVP_MD *digest = ssl_session_get_digest(session);
+ // Note this function assumes that |session|'s key material corresponds to
+ // |ssl->s3->client_random| and |ssl->s3->server_random|.
+ return tls1_prf(digest, out, master_key, label, ssl->s3->server_random,
+ ssl->s3->client_random);
+}
+
+bool tls1_configure_aead(SSL *ssl, evp_aead_direction_t direction,
+ Array<uint8_t> *key_block_cache,
+ const SSL_SESSION *session,
+ Span<const uint8_t> iv_override) {
size_t mac_secret_len, key_len, iv_len;
- if (!get_key_block_lengths(ssl, &mac_secret_len, &key_len, &iv_len, cipher)) {
- return 0;
+ if (!get_key_block_lengths(ssl, &mac_secret_len, &key_len, &iv_len,
+ session->cipher)) {
+ return false;
}
// Ensure that |key_block_cache| is set up.
const size_t key_block_size = 2 * (mac_secret_len + key_len + iv_len);
if (key_block_cache->empty()) {
if (!key_block_cache->Init(key_block_size) ||
- !SSL_generate_key_block(ssl, key_block_cache->data(), key_block_size)) {
- return 0;
+ !generate_key_block(ssl, MakeSpan(*key_block_cache), session)) {
+ return false;
}
}
assert(key_block_cache->size() == key_block_size);
@@ -224,15 +239,16 @@
if (!iv_override.empty()) {
if (iv_override.size() != iv_len) {
- return 0;
+ return false;
}
iv = iv_override;
}
- UniquePtr<SSLAEADContext> aead_ctx = SSLAEADContext::Create(
- direction, ssl->version, SSL_is_dtls(ssl), cipher, key, mac_secret, iv);
+ UniquePtr<SSLAEADContext> aead_ctx =
+ SSLAEADContext::Create(direction, ssl->version, SSL_is_dtls(ssl),
+ session->cipher, key, mac_secret, iv);
if (!aead_ctx) {
- return 0;
+ return false;
}
if (direction == evp_aead_open) {
@@ -246,10 +262,10 @@
/*secret_for_quic=*/{});
}
-int tls1_change_cipher_state(SSL_HANDSHAKE *hs,
- evp_aead_direction_t direction) {
+bool tls1_change_cipher_state(SSL_HANDSHAKE *hs,
+ evp_aead_direction_t direction) {
return tls1_configure_aead(hs->ssl, direction, &hs->key_block,
- hs->new_cipher, {});
+ ssl_handshake_session(hs), {});
}
int tls1_generate_master_secret(SSL_HANDSHAKE *hs, uint8_t *out,
@@ -286,6 +302,11 @@
using namespace bssl;
size_t SSL_get_key_block_len(const SSL *ssl) {
+ // See |SSL_generate_key_block|.
+ if (SSL_in_init(ssl)) {
+ return 0;
+ }
+
size_t mac_secret_len, key_len, fixed_iv_len;
if (!get_key_block_lengths(ssl, &mac_secret_len, &key_len, &fixed_iv_len,
SSL_get_current_cipher(ssl))) {
@@ -297,16 +318,16 @@
}
int SSL_generate_key_block(const SSL *ssl, uint8_t *out, size_t out_len) {
- const SSL_SESSION *session = SSL_get_session(ssl);
- auto out_span = MakeSpan(out, out_len);
- auto master_key =
- MakeConstSpan(session->master_key, session->master_key_length);
- static const char kLabel[] = "key expansion";
- auto label = MakeConstSpan(kLabel, sizeof(kLabel) - 1);
+ // Which cipher state to use is ambiguous during a handshake. In particular,
+ // there are points where read and write states are from different epochs.
+ // During a handshake, before ChangeCipherSpec, the encryption states may not
+ // match |ssl->s3->client_random| and |ssl->s3->server_random|.
+ if (SSL_in_init(ssl)) {
+ OPENSSL_PUT_ERROR(SSL, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED);
+ return 0;
+ }
- const EVP_MD *digest = ssl_session_get_digest(session);
- return tls1_prf(digest, out_span, master_key, label, ssl->s3->server_random,
- ssl->s3->client_random);
+ return generate_key_block(ssl, MakeSpan(out, out_len), SSL_get_session(ssl));
}
int SSL_export_keying_material(SSL *ssl, uint8_t *out, size_t out_len,