Modify how QUIC 0-RTT go/no-go decision is made. The previous implementation was too strict in its byte-for-byte equality check including Transport Parameters, because the Transport Parameters contain a field that QUIC requires be different on each connection. This change still has BoringSSL do a byte-for-byte check, but now it is only done over the quic_early_data_context. An additional requirement is imposed that the quic_early_data_context must be set for early data capable tickets to be issued. Bug: 295 Change-Id: I5145c10752b41908b6807c3a3c967653b0c13f37 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/41427 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index a61488e..5a5cbdc 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h
@@ -3153,12 +3153,15 @@ // QUIC may impose similar restrictions, for example HTTP/3's restrictions on // SETTINGS frames. // -// BoringSSL imposes a stricter check on the server to enforce these -// restrictions. BoringSSL requires that the transport parameters and -// application protocol state be a byte-for-byte match between the connection -// where the ticket was issued and the connection where it is used for 0-RTT. If -// there is a mismatch, BoringSSL will reject early data (but not reject the -// resumption attempt). +// BoringSSL implements this check by doing a byte-for-byte comparison of an +// opaque context passed in by the server. This context must be the same on the +// connection where the ticket was issued and the connection where that ticket +// is used for 0-RTT. If there is a mismatch, or the context was not set, +// BoringSSL will reject early data (but not reject the resumption attempt). +// This context is set via |SSL_set_quic_early_data_context| and should cover +// both transport parameters and any application state. +// |SSL_set_quic_early_data_context| must be called on the server with a +// non-empty context if the server is to support 0-RTT in QUIC. // // BoringSSL does not perform any client-side checks on the transport // parameters received from a server that also accepted early data. It is up to @@ -3166,12 +3169,6 @@ // limits, and to close the QUIC connection if that is not the case. The same // holds for any application protocol state remembered for 0-RTT, e.g. HTTP/3 // SETTINGS. -// -// The transport parameter check happens automatically with -// |SSL_set_quic_transport_params|. QUIC servers must set application state via -// |SSL_set_quic_early_data_context| to configure the application protocol -// check. No other mechanisms are provided to have BoringSSL reject early data -// because of QUIC transport or application protocol restrictions. // ssl_encryption_level_t represents a specific QUIC encryption level used to // transmit handshake messages. @@ -3321,8 +3318,12 @@ // SSL_set_quic_early_data_context configures a context string in QUIC servers // for accepting early data. If a resumption connection offers early data, the // server will check if the value matches that of the connection which minted -// the ticket. If not, resumption still succeeds but early data is rejected. For -// HTTP/3, this should be the serialized server SETTINGS frame. +// the ticket. If not, resumption still succeeds but early data is rejected. +// This should include all QUIC Transport Parameters except ones specified that +// the client MUST NOT remember. This should also include any application +// protocol-specific state. For HTTP/3, this should be the serialized server +// SETTINGS frame and the QUIC Transport Parameters (except the stateless reset +// token). // // This function may be called before |SSL_do_handshake| or during server // certificate selection. It returns 1 on success and 0 on failure.
diff --git a/ssl/internal.h b/ssl/internal.h index e1b0925..b1c8bd1 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -2740,11 +2740,6 @@ bool jdk11_workaround : 1; }; -// Computes a SHA-256 hash of the transport parameters and early data context -// for QUIC, putting the hash in |SHA256_DIGEST_LENGTH| bytes at |hash_out|. -bool compute_quic_early_data_hash(const SSL_CONFIG *config, - uint8_t hash_out[SHA256_DIGEST_LENGTH]); - // From RFC 8446, used in determining PSK modes. #define SSL_PSK_DHE_KE 0x1 @@ -3559,9 +3554,9 @@ // is_quic indicates whether this session was created using QUIC. bool is_quic : 1; - // quic_early_data_hash is used to determine whether early data must be + // quic_early_data_context is used to determine whether early data must be // rejected when performing a QUIC handshake. - bssl::Array<uint8_t> quic_early_data_hash; + bssl::Array<uint8_t> quic_early_data_context; private: ~ssl_session_st();
diff --git a/ssl/ssl_asn1.cc b/ssl/ssl_asn1.cc index 7401d09..e6274f1 100644 --- a/ssl/ssl_asn1.cc +++ b/ssl/ssl_asn1.cc
@@ -192,7 +192,7 @@ CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 26; static const unsigned kIsQuicTag = CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 27; -static const unsigned kQuicEarlyDataHashTag = +static const unsigned kQuicEarlyDataContextTag = CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 28; static int SSL_SESSION_to_bytes_full(const SSL_SESSION *in, CBB *cbb, @@ -402,10 +402,10 @@ } } - if (!in->quic_early_data_hash.empty()) { - if (!CBB_add_asn1(&session, &child, kQuicEarlyDataHashTag) || - !CBB_add_asn1_octet_string(&child, in->quic_early_data_hash.data(), - in->quic_early_data_hash.size())) { + if (!in->quic_early_data_context.empty()) { + if (!CBB_add_asn1(&session, &child, kQuicEarlyDataContextTag) || + !CBB_add_asn1_octet_string(&child, in->quic_early_data_context.data(), + in->quic_early_data_context.size())) { OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); return 0; } @@ -752,8 +752,8 @@ kEarlyALPNTag) || !CBS_get_optional_asn1_bool(&session, &is_quic, kIsQuicTag, /*default_value=*/false) || - !SSL_SESSION_parse_octet_string(&session, &ret->quic_early_data_hash, - kQuicEarlyDataHashTag) || + !SSL_SESSION_parse_octet_string(&session, &ret->quic_early_data_context, + kQuicEarlyDataContextTag) || CBS_len(&session) != 0) { OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_SSL_SESSION); return nullptr;
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc index fa994e8..4c6d045 100644 --- a/ssl/ssl_session.cc +++ b/ssl/ssl_session.cc
@@ -269,8 +269,8 @@ return nullptr; } - if (!new_session->quic_early_data_hash.CopyFrom( - session->quic_early_data_hash)) { + if (!new_session->quic_early_data_context.CopyFrom( + session->quic_early_data_context)) { return nullptr; } } @@ -349,25 +349,6 @@ session->cipher); } -bool compute_quic_early_data_hash(const SSL_CONFIG *config, - uint8_t hash_out[SHA256_DIGEST_LENGTH]) { - ScopedEVP_MD_CTX hash_ctx; - uint32_t transport_param_len = config->quic_transport_params.size(); - uint32_t context_len = config->quic_early_data_context.size(); - if (!EVP_DigestInit(hash_ctx.get(), EVP_sha256()) || - !EVP_DigestUpdate(hash_ctx.get(), &transport_param_len, - sizeof(transport_param_len)) || - !EVP_DigestUpdate(hash_ctx.get(), config->quic_transport_params.data(), - config->quic_transport_params.size()) || - !EVP_DigestUpdate(hash_ctx.get(), &context_len, sizeof(context_len)) || - !EVP_DigestUpdate(hash_ctx.get(), config->quic_early_data_context.data(), - config->quic_early_data_context.size()) || - !EVP_DigestFinal(hash_ctx.get(), hash_out, nullptr)) { - return false; - } - return true; -} - int ssl_get_new_session(SSL_HANDSHAKE *hs, int is_server) { SSL *const ssl = hs->ssl; if (ssl->mode & SSL_MODE_NO_SESSION_CREATION) { @@ -384,9 +365,8 @@ session->ssl_version = ssl->version; session->is_quic = ssl->quic_method != nullptr; if (is_server && ssl->enable_early_data && session->is_quic) { - if (!session->quic_early_data_hash.Init(SHA256_DIGEST_LENGTH) || - !compute_quic_early_data_hash(hs->config, - session->quic_early_data_hash.data())) { + if (!session->quic_early_data_context.CopyFrom( + hs->config->quic_early_data_context)) { return 0; } }
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index fc7976e..73f0b9d 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc
@@ -5433,73 +5433,68 @@ bssl::UniquePtr<SSL_SESSION> session = CreateClientSessionForQUIC(); ASSERT_TRUE(session); - for (bool change_transport_params : {false, true}) { - SCOPED_TRACE(change_transport_params); - for (bool change_context : {false, true}) { - if (!change_transport_params && !change_context) { - continue; - } - SCOPED_TRACE(change_context); + ASSERT_TRUE(CreateClientAndServer()); + static const uint8_t new_context[] = {4}; + ASSERT_TRUE(SSL_set_quic_early_data_context(server_.get(), new_context, + sizeof(new_context))); + SSL_set_session(client_.get(), session.get()); - ASSERT_TRUE(CreateClientAndServer()); - static const uint8_t new_transport_params[] = {3}; - static const uint8_t new_context[] = {4}; - if (change_transport_params) { - ASSERT_TRUE(SSL_set_quic_transport_params( - server_.get(), new_transport_params, sizeof(new_transport_params))); - } - if (change_context) { - ASSERT_TRUE(SSL_set_quic_early_data_context(server_.get(), new_context, - sizeof(new_context))); - } - SSL_set_session(client_.get(), session.get()); + // The client handshake should return immediately into the early data + // state. + ASSERT_EQ(SSL_do_handshake(client_.get()), 1); + EXPECT_TRUE(SSL_in_early_data(client_.get())); + // The transport should have keys for sending 0-RTT data. + EXPECT_TRUE(transport_->client()->HasWriteSecret(ssl_encryption_early_data)); - // The client handshake should return immediately into the early data - // state. - ASSERT_EQ(SSL_do_handshake(client_.get()), 1); - EXPECT_TRUE(SSL_in_early_data(client_.get())); - // The transport should have keys for sending 0-RTT data. - EXPECT_TRUE( - transport_->client()->HasWriteSecret(ssl_encryption_early_data)); + // The server will consume the ClientHello, but it will not accept 0-RTT. + ASSERT_TRUE(ProvideHandshakeData(server_.get())); + ASSERT_EQ(SSL_do_handshake(server_.get()), -1); + EXPECT_EQ(SSL_ERROR_WANT_READ, SSL_get_error(server_.get(), -1)); + EXPECT_FALSE(SSL_in_early_data(server_.get())); + EXPECT_FALSE(transport_->server()->HasReadSecret(ssl_encryption_early_data)); - // The server will consume the ClientHello, but it will not accept 0-RTT. - ASSERT_TRUE(ProvideHandshakeData(server_.get())); - ASSERT_EQ(SSL_do_handshake(server_.get()), -1); - EXPECT_EQ(SSL_ERROR_WANT_READ, SSL_get_error(server_.get(), -1)); - EXPECT_FALSE(SSL_in_early_data(server_.get())); - EXPECT_FALSE( - transport_->server()->HasReadSecret(ssl_encryption_early_data)); - - // The client consumes the server response and signals 0-RTT rejection. - for (;;) { - ASSERT_TRUE(ProvideHandshakeData(client_.get())); - ASSERT_EQ(-1, SSL_do_handshake(client_.get())); - int err = SSL_get_error(client_.get(), -1); - if (err == SSL_ERROR_EARLY_DATA_REJECTED) { - break; - } - ASSERT_EQ(SSL_ERROR_WANT_READ, err); - } - - // As in TLS over TCP, 0-RTT rejection is sticky. - ASSERT_EQ(-1, SSL_do_handshake(client_.get())); - ASSERT_EQ(SSL_ERROR_EARLY_DATA_REJECTED, - SSL_get_error(client_.get(), -1)); - - // Finish up the client and server handshakes. - SSL_reset_early_data_reject(client_.get()); - ASSERT_TRUE(CompleteHandshakesForQUIC()); - - // Both sides can now exchange 1-RTT data. - ExpectHandshakeSuccess(); - EXPECT_TRUE(SSL_session_reused(client_.get())); - EXPECT_TRUE(SSL_session_reused(server_.get())); - EXPECT_FALSE(SSL_in_early_data(client_.get())); - EXPECT_FALSE(SSL_in_early_data(server_.get())); - EXPECT_FALSE(SSL_early_data_accepted(client_.get())); - EXPECT_FALSE(SSL_early_data_accepted(server_.get())); + // The client consumes the server response and signals 0-RTT rejection. + for (;;) { + ASSERT_TRUE(ProvideHandshakeData(client_.get())); + ASSERT_EQ(-1, SSL_do_handshake(client_.get())); + int err = SSL_get_error(client_.get(), -1); + if (err == SSL_ERROR_EARLY_DATA_REJECTED) { + break; } + ASSERT_EQ(SSL_ERROR_WANT_READ, err); } + + // As in TLS over TCP, 0-RTT rejection is sticky. + ASSERT_EQ(-1, SSL_do_handshake(client_.get())); + ASSERT_EQ(SSL_ERROR_EARLY_DATA_REJECTED, SSL_get_error(client_.get(), -1)); + + // Finish up the client and server handshakes. + SSL_reset_early_data_reject(client_.get()); + ASSERT_TRUE(CompleteHandshakesForQUIC()); + + // Both sides can now exchange 1-RTT data. + ExpectHandshakeSuccess(); + EXPECT_TRUE(SSL_session_reused(client_.get())); + EXPECT_TRUE(SSL_session_reused(server_.get())); + EXPECT_FALSE(SSL_in_early_data(client_.get())); + EXPECT_FALSE(SSL_in_early_data(server_.get())); + EXPECT_FALSE(SSL_early_data_accepted(client_.get())); + EXPECT_FALSE(SSL_early_data_accepted(server_.get())); +} + +TEST_F(QUICMethodTest, NoZeroRTTTicketWithoutEarlyDataContext) { + server_quic_early_data_context_ = {}; + const SSL_QUIC_METHOD quic_method = DefaultQUICMethod(); + + SSL_CTX_set_session_cache_mode(client_ctx_.get(), SSL_SESS_CACHE_BOTH); + SSL_CTX_set_early_data_enabled(client_ctx_.get(), 1); + SSL_CTX_set_early_data_enabled(server_ctx_.get(), 1); + ASSERT_TRUE(SSL_CTX_set_quic_method(client_ctx_.get(), &quic_method)); + ASSERT_TRUE(SSL_CTX_set_quic_method(server_ctx_.get(), &quic_method)); + + bssl::UniquePtr<SSL_SESSION> session = CreateClientSessionForQUIC(); + ASSERT_TRUE(session); + EXPECT_FALSE(SSL_SESSION_early_data_capable(session.get())); } TEST_F(QUICMethodTest, ZeroRTTReject) {
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index 683a2ca..33f821e 100644 --- a/ssl/tls13_server.cc +++ b/ssl/tls13_server.cc
@@ -127,7 +127,10 @@ return false; } session->ticket_age_add_valid = true; - if (ssl->enable_early_data) { + bool enable_early_data = + ssl->enable_early_data && + (!ssl->quic_method || !ssl->config->quic_early_data_context.empty()); + if (enable_early_data) { // QUIC does not use the max_early_data_size parameter and always sets it // to a fixed value. See draft-ietf-quic-tls-22, section 4.5. session->ticket_max_early_data = @@ -152,7 +155,7 @@ return false; } - if (ssl->enable_early_data) { + if (enable_early_data) { CBB early_data; if (!CBB_add_u16(&extensions, TLSEXT_TYPE_early_data) || !CBB_add_u16_length_prefixed(&extensions, &early_data) || @@ -314,13 +317,13 @@ if (!session->is_quic) { return true; } - if (session->quic_early_data_hash.size() != SHA256_DIGEST_LENGTH) { - return false; - } - uint8_t early_data_hash[SHA256_DIGEST_LENGTH]; - if (!compute_quic_early_data_hash(config, early_data_hash) || - CRYPTO_memcmp(session->quic_early_data_hash.data(), early_data_hash, - SHA256_DIGEST_LENGTH) != 0) { + + if (session->quic_early_data_context.empty() || + config->quic_early_data_context.size() != + session->quic_early_data_context.size() || + CRYPTO_memcmp(config->quic_early_data_context.data(), + session->quic_early_data_context.data(), + session->quic_early_data_context.size()) != 0) { return false; } return true;