From 5acf9f42c2e853c660d1bc1c64da9d6e6dfab1e9 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 24 May 2021 14:23:38 -0400 Subject: [PATCH] Replace hs->needs_psk_binder with an output parameter. May not be strictly necessary, but similarly easier to reason about when we need to interweave multiple ClientHellos. Bug: 275 Change-Id: I9f85787860f3e8ce1653331ce52343d5bf5def23 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47992 Reviewed-by: Adam Langley --- ssl/handshake.cc | 1 - ssl/handshake_client.cc | 6 ++++-- ssl/internal.h | 11 +++++------ ssl/t1_lib.cc | 14 +++++++++----- 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/ssl/handshake.cc b/ssl/handshake.cc index 9133e162d..1994465b0 100644 --- a/ssl/handshake.cc +++ b/ssl/handshake.cc @@ -129,7 +129,6 @@ SSL_HANDSHAKE::SSL_HANDSHAKE(SSL *ssl_arg) ech_present(false), ech_is_inner_present(false), scts_requested(false), - needs_psk_binder(false), handshake_finalized(false), accept_psk_mode(false), cert_request(false), diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index 710b036da..e2c3dd96f 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc @@ -310,10 +310,12 @@ bool ssl_write_client_hello(SSL_HANDSHAKE *hs) { size_t header_len = SSL_is_dtls(ssl) ? DTLS1_HM_HEADER_LENGTH : SSL3_HM_HEADER_LENGTH; + bool needs_psk_binder; if (!ssl_write_client_cipher_list(hs, &body) || !CBB_add_u8(&body, 1 /* one compression method */) || !CBB_add_u8(&body, 0 /* null compression */) || - !ssl_add_clienthello_tlsext(hs, &body, header_len + CBB_len(&body))) { + !ssl_add_clienthello_tlsext(hs, &body, &needs_psk_binder, + header_len + CBB_len(&body))) { return false; } @@ -324,7 +326,7 @@ bool ssl_write_client_hello(SSL_HANDSHAKE *hs) { // Now that the length prefixes have been computed, fill in the placeholder // PSK binder. - if (hs->needs_psk_binder && + if (needs_psk_binder && !tls13_write_psk_binder(hs, MakeSpan(msg))) { return false; } diff --git a/ssl/internal.h b/ssl/internal.h index 8d7e493fa..250beb1b9 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -1866,10 +1866,6 @@ struct SSL_HANDSHAKE { // scts_requested is true if the SCT extension is in the ClientHello. bool scts_requested : 1; - // needs_psk_binder is true if the ClientHello has a placeholder PSK binder to - // be filled in. - bool needs_psk_binder : 1; - // handshake_finalized is true once the handshake has completed, at which // point accessors should use the established state. bool handshake_finalized : 1; @@ -3167,8 +3163,11 @@ bool tls1_set_curves_list(Array *out_group_ids, const char *curves); // ssl_add_clienthello_tlsext writes ClientHello extensions to |out|. It returns // true on success and false on failure. The |header_len| argument is the length // of the ClientHello written so far and is used to compute the padding length. -// (It does not include the record header.) -bool ssl_add_clienthello_tlsext(SSL_HANDSHAKE *hs, CBB *out, size_t header_len); +// (It does not include the record header.) On success, if +// |*out_needs_psk_binder| is true, the last ClientHello extension was the +// pre_shared_key extension and needs a PSK binder filled in. +bool ssl_add_clienthello_tlsext(SSL_HANDSHAKE *hs, CBB *out, + bool *out_needs_psk_binder, size_t header_len); bool ssl_add_serverhello_tlsext(SSL_HANDSHAKE *hs, CBB *out); bool ssl_parse_clienthello_tlsext(SSL_HANDSHAKE *hs, diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc index bd246f3d6..d46241e01 100644 --- a/ssl/t1_lib.cc +++ b/ssl/t1_lib.cc @@ -1944,9 +1944,11 @@ static size_t ext_pre_shared_key_clienthello_length(SSL_HANDSHAKE *hs) { return 15 + ssl->session->ticket.size() + binder_len; } -static bool ext_pre_shared_key_add_clienthello(SSL_HANDSHAKE *hs, CBB *out) { +static bool ext_pre_shared_key_add_clienthello(const SSL_HANDSHAKE *hs, + CBB *out, + bool *out_needs_binder) { const SSL *const ssl = hs->ssl; - hs->needs_psk_binder = false; + *out_needs_binder = false; if (hs->max_version < TLS1_3_VERSION || ssl->session == nullptr || ssl_session_protocol_version(ssl->session.get()) < TLS1_3_VERSION) { return true; @@ -1984,7 +1986,7 @@ static bool ext_pre_shared_key_add_clienthello(SSL_HANDSHAKE *hs, CBB *out) { return false; } - hs->needs_psk_binder = true; + *out_needs_binder = true; return CBB_flush(out); } @@ -3242,9 +3244,10 @@ static const struct tls_extension *tls_extension_find(uint32_t *out_index, } bool ssl_add_clienthello_tlsext(SSL_HANDSHAKE *hs, CBB *out, - size_t header_len) { + bool *out_needs_psk_binder, size_t header_len) { SSL *const ssl = hs->ssl; CBB extensions; + *out_needs_psk_binder = false; if (!CBB_add_u16_length_prefixed(out, &extensions)) { OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); return false; @@ -3355,7 +3358,8 @@ bool ssl_add_clienthello_tlsext(SSL_HANDSHAKE *hs, CBB *out, } // The PSK extension must be last, including after the padding. - if (!ext_pre_shared_key_add_clienthello(hs, &extensions)) { + if (!ext_pre_shared_key_add_clienthello(hs, &extensions, + out_needs_psk_binder)) { OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); return false; }