From 294ab9730c570213b496cfc2fc14b3c0bfcd4bcc Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 19 Feb 2025 17:04:07 -0500 Subject: [PATCH] Fix legacy_version in DTLS 1.3 HelloRetryRequest Sergey Sukhanov noticed we were setting legacy_version to TLS 1.2 instead of DTLS 1.2. Bug: 323561277 Change-Id: I07e0fd8e5ac8f027ba8c46b39a7e06b700d1f5c7 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/76587 Reviewed-by: Bob Beck Commit-Queue: Bob Beck Auto-Submit: David Benjamin --- ssl/test/runner/conn.go | 4 ++-- ssl/test/runner/handshake_messages.go | 7 ++++++- ssl/tls13_server.cc | 10 ++++------ 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go index a3ca81736..b75e48b6b 100644 --- a/ssl/test/runner/conn.go +++ b/ssl/test/runner/conn.go @@ -1518,8 +1518,8 @@ func (c *Conn) readHandshake() (any, error) { if data[0] == typeServerHello && len(data) >= 38 { vers := uint16(data[4])<<8 | uint16(data[5]) - if vers == VersionTLS12 && bytes.Equal(data[6:38], tls13HelloRetryRequest) { - m = new(helloRetryRequestMsg) + if (vers == VersionDTLS12 || vers == VersionTLS12) && bytes.Equal(data[6:38], tls13HelloRetryRequest) { + m = &helloRetryRequestMsg{isDTLS: c.isDTLS} } } diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go index 523258167..3bffb5a5e 100644 --- a/ssl/test/runner/handshake_messages.go +++ b/ssl/test/runner/handshake_messages.go @@ -1940,6 +1940,11 @@ func (m *helloRetryRequestMsg) marshal() []byte { } func (m *helloRetryRequestMsg) unmarshal(data []byte) bool { + expectedLegacyVers := uint16(VersionTLS12) + if m.isDTLS { + expectedLegacyVers = VersionDTLS12 + } + m.raw = data reader := cryptobyte.String(data[4:]) var legacyVers uint16 @@ -1947,7 +1952,7 @@ func (m *helloRetryRequestMsg) unmarshal(data []byte) bool { var compressionMethod byte var extensions cryptobyte.String if !reader.ReadUint16(&legacyVers) || - legacyVers != VersionTLS12 || + legacyVers != expectedLegacyVers || !reader.ReadBytes(&random, 32) || !readUint8LengthPrefixedBytes(&reader, &m.sessionID) || !reader.ReadUint16(&m.cipherSuite) || diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index e34ecf4b4..3e83e5fdc 100644 --- a/ssl/tls13_server.cc +++ b/ssl/tls13_server.cc @@ -711,7 +711,8 @@ static enum ssl_hs_wait_t do_send_hello_retry_request(SSL_HANDSHAKE *hs) { ScopedCBB cbb; CBB body, session_id, extensions; if (!ssl->method->init_message(ssl, cbb.get(), &body, SSL3_MT_SERVER_HELLO) || - !CBB_add_u16(&body, TLS1_2_VERSION) || + !CBB_add_u16(&body, + SSL_is_dtls(ssl) ? DTLS1_2_VERSION : TLS1_2_VERSION) || !CBB_add_bytes(&body, kHelloRetryRequest, SSL3_RANDOM_SIZE) || !CBB_add_u8_length_prefixed(&body, &session_id) || !CBB_add_bytes(&session_id, hs->session_id.data(), @@ -907,15 +908,12 @@ static enum ssl_hs_wait_t do_send_server_hello(SSL_HANDSHAKE *hs) { } } - uint16_t server_hello_version = TLS1_2_VERSION; - if (SSL_is_dtls(ssl)) { - server_hello_version = DTLS1_2_VERSION; - } Array server_hello; ScopedCBB cbb; CBB body, extensions, session_id; if (!ssl->method->init_message(ssl, cbb.get(), &body, SSL3_MT_SERVER_HELLO) || - !CBB_add_u16(&body, server_hello_version) || + !CBB_add_u16(&body, + SSL_is_dtls(ssl) ? DTLS1_2_VERSION : TLS1_2_VERSION) || !CBB_add_bytes(&body, ssl->s3->server_random, sizeof(ssl->s3->server_random)) || !CBB_add_u8_length_prefixed(&body, &session_id) ||