diff --git a/src/ssl/dtls_record.cc b/src/ssl/dtls_record.cc index 911dcc658..f76969618 100644 --- a/src/ssl/dtls_record.cc +++ b/src/ssl/dtls_record.cc @@ -123,6 +123,8 @@ BSSL_NAMESPACE_BEGIN +static constexpr uint64_t kMaxSequenceNumber = (uint64_t{1} << 48) - 1; + bool DTLSReplayBitmap::ShouldDiscard(uint64_t seq_num) const { const size_t kWindowSize = map_.size(); @@ -179,25 +181,31 @@ static uint16_t reconstruct_epoch(uint8_t wire_epoch, uint16_t current_epoch) { uint64_t reconstruct_seqnum(uint16_t wire_seq, uint64_t seq_mask, uint64_t max_valid_seqnum) { + // Although DTLS 1.3 can support sequence numbers up to 2^64-1, we continue to + // enforce the DTLS 1.2 2^48-1 limit. With a minimal DTLS 1.3 record header (2 + // bytes), no payload, and 16 byte AEAD overhead, sending 2^48 records would + // require 5 petabytes. This allows us to continue to pack a DTLS record + // number into an 8-byte structure. + assert(max_valid_seqnum <= kMaxSequenceNumber); + assert(seq_mask == 0xff || seq_mask == 0xffff); + uint64_t max_seqnum_plus_one = max_valid_seqnum + 1; uint64_t diff = (wire_seq - max_seqnum_plus_one) & seq_mask; uint64_t step = seq_mask + 1; + // This addition cannot overflow. It is at most 2^48 + seq_mask. It, however, + // may exceed 2^48-1. uint64_t seqnum = max_seqnum_plus_one + diff; - // seqnum is computed as the addition of 3 non-negative values - // (max_valid_seqnum, 1, and diff). The values 1 and diff are small (relative - // to the size of a uint64_t), while max_valid_seqnum can span the range of - // all uint64_t values. If seqnum is less than max_valid_seqnum, then the - // addition overflowed. - bool overflowed = seqnum < max_valid_seqnum; + bool too_large = seqnum > kMaxSequenceNumber; // If the diff is larger than half the step size, then the closest seqnum // to max_seqnum_plus_one (in Z_{2^64}) is seqnum minus step instead of // seqnum. bool closer_is_less = diff > step / 2; // Subtracting step from seqnum will cause underflow if seqnum is too small. bool would_underflow = seqnum < step; - if (overflowed || (closer_is_less && !would_underflow)) { + if (too_large || (closer_is_less && !would_underflow)) { seqnum -= step; } + assert(seqnum <= kMaxSequenceNumber); return seqnum; } @@ -491,7 +499,6 @@ bool dtls_seal_record(SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out, const size_t record_header_len = dtls_record_header_write_len(ssl, epoch); // Ensure the sequence number update does not overflow. - const uint64_t kMaxSequenceNumber = (uint64_t{1} << 48) - 1; if (write_epoch->next_seq + 1 > kMaxSequenceNumber) { OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW); return false; diff --git a/src/ssl/internal.h b/src/ssl/internal.h index 29a674cb8..0f3bd8229 100644 --- a/src/ssl/internal.h +++ b/src/ssl/internal.h @@ -1222,6 +1222,9 @@ class DTLSReplayBitmap { // successfully deprotected in this epoch. This function returns the sequence // number that is numerically closest to one plus |max_valid_seqnum| that when // bitwise and-ed with |seq_mask| equals |wire_seq|. +// +// |max_valid_seqnum| must be most 2^48-1, in which case the output will also be +// at most 2^48-1. OPENSSL_EXPORT uint64_t reconstruct_seqnum(uint16_t wire_seq, uint64_t seq_mask, uint64_t max_valid_seqnum); diff --git a/src/ssl/ssl_internal_test.cc b/src/ssl/ssl_internal_test.cc index 2cca65a65..b6abafdfe 100644 --- a/src/ssl/ssl_internal_test.cc +++ b/src/ssl/ssl_internal_test.cc @@ -399,16 +399,13 @@ TEST(ReconstructSeqnumTest, Increment) { EXPECT_EQ(reconstruct_seqnum(0xffff, 0xffff, 0x2f000), 0x2ffffu); EXPECT_EQ(reconstruct_seqnum(0xfffe, 0xffff, 0x2f000), 0x2fffeu); - // Test that reconstruct_seqnum can return - // std::numeric_limits::max(). - EXPECT_EQ(reconstruct_seqnum(0xff, 0xff, 0xffffffffffffffff), - std::numeric_limits::max()); - EXPECT_EQ(reconstruct_seqnum(0xff, 0xff, 0xfffffffffffffffe), - std::numeric_limits::max()); - EXPECT_EQ(reconstruct_seqnum(0xffff, 0xffff, 0xffffffffffffffff), - std::numeric_limits::max()); - EXPECT_EQ(reconstruct_seqnum(0xffff, 0xffff, 0xfffffffffffffffe), - std::numeric_limits::max()); + // Test that reconstruct_seqnum can return the maximum sequence number, + // 2^48-1. + constexpr uint64_t kMaxSequence = (uint64_t{1} << 48) - 1; + EXPECT_EQ(reconstruct_seqnum(0xff, 0xff, kMaxSequence), kMaxSequence); + EXPECT_EQ(reconstruct_seqnum(0xff, 0xff, kMaxSequence - 1), kMaxSequence); + EXPECT_EQ(reconstruct_seqnum(0xffff, 0xffff, kMaxSequence), kMaxSequence); + EXPECT_EQ(reconstruct_seqnum(0xffff, 0xffff, kMaxSequence - 1), kMaxSequence); } TEST(ReconstructSeqnumTest, Decrement) { @@ -431,35 +428,35 @@ TEST(ReconstructSeqnumTest, Decrement) { EXPECT_EQ(reconstruct_seqnum(0xffff, 0xffff, 0x20000), 0x1ffffu); EXPECT_EQ(reconstruct_seqnum(0xfffe, 0xffff, 0x20000), 0x1fffeu); - // Test when the max seen sequence number is close to the uint64_t max value. - // In some cases, the closest numerical value in the integers will overflow - // a uint64_t. Instead of returning the closest value in Z_{2^64}, - // reconstruct_seqnum should return the closest integer less than 2^64, even - // if there is a closer value greater than 2^64. - EXPECT_EQ(reconstruct_seqnum(0, 0xff, 0xffffffffffffffff), - 0xffffffffffffff00u); - EXPECT_EQ(reconstruct_seqnum(0, 0xff, 0xfffffffffffffffe), - 0xffffffffffffff00u); - EXPECT_EQ(reconstruct_seqnum(1, 0xff, 0xffffffffffffffff), - 0xffffffffffffff01u); - EXPECT_EQ(reconstruct_seqnum(1, 0xff, 0xfffffffffffffffe), - 0xffffffffffffff01u); - EXPECT_EQ(reconstruct_seqnum(0xfe, 0xff, 0xffffffffffffffff), - 0xfffffffffffffffeu); - EXPECT_EQ(reconstruct_seqnum(0xfd, 0xff, 0xfffffffffffffffe), - 0xfffffffffffffffdu); - EXPECT_EQ(reconstruct_seqnum(0, 0xffff, 0xffffffffffffffff), - 0xffffffffffff0000u); - EXPECT_EQ(reconstruct_seqnum(0, 0xffff, 0xfffffffffffffffe), - 0xffffffffffff0000u); - EXPECT_EQ(reconstruct_seqnum(1, 0xffff, 0xffffffffffffffff), - 0xffffffffffff0001u); - EXPECT_EQ(reconstruct_seqnum(1, 0xffff, 0xfffffffffffffffe), - 0xffffffffffff0001u); - EXPECT_EQ(reconstruct_seqnum(0xfffe, 0xffff, 0xffffffffffffffff), - 0xfffffffffffffffeu); - EXPECT_EQ(reconstruct_seqnum(0xfffd, 0xffff, 0xfffffffffffffffe), - 0xfffffffffffffffdu); + constexpr uint64_t kMaxSequence = (uint64_t{1} << 48) - 1; + // kMaxSequence00 is kMaxSequence with the last byte replaced with 0x00. + constexpr uint64_t kMaxSequence00 = kMaxSequence - 0xff; + // kMaxSequence0000 is kMaxSequence with the last byte replaced with 0x0000. + constexpr uint64_t kMaxSequence0000 = kMaxSequence - 0xffff; + + // Test when the max seen sequence number is close to the 2^48-1 max value. + // In some cases, the closest numerical value in the integers will exceed the + // limit. In this case, reconstruct_seqnum should return the closest integer + // within range. + EXPECT_EQ(reconstruct_seqnum(0, 0xff, kMaxSequence), kMaxSequence00); + EXPECT_EQ(reconstruct_seqnum(0, 0xff, kMaxSequence - 1), kMaxSequence00); + EXPECT_EQ(reconstruct_seqnum(1, 0xff, kMaxSequence), kMaxSequence00 + 0x01); + EXPECT_EQ(reconstruct_seqnum(1, 0xff, kMaxSequence - 1), + kMaxSequence00 + 0x01); + EXPECT_EQ(reconstruct_seqnum(0xfe, 0xff, kMaxSequence), + kMaxSequence00 + 0xfe); + EXPECT_EQ(reconstruct_seqnum(0xfd, 0xff, kMaxSequence - 1), + kMaxSequence00 + 0xfd); + EXPECT_EQ(reconstruct_seqnum(0, 0xffff, kMaxSequence), kMaxSequence0000); + EXPECT_EQ(reconstruct_seqnum(0, 0xffff, kMaxSequence - 1), kMaxSequence0000); + EXPECT_EQ(reconstruct_seqnum(1, 0xffff, kMaxSequence), + kMaxSequence0000 + 0x0001); + EXPECT_EQ(reconstruct_seqnum(1, 0xffff, kMaxSequence - 1), + kMaxSequence0000 + 0x0001); + EXPECT_EQ(reconstruct_seqnum(0xfffe, 0xffff, kMaxSequence), + kMaxSequence0000 + 0xfffe); + EXPECT_EQ(reconstruct_seqnum(0xfffd, 0xffff, kMaxSequence - 1), + kMaxSequence0000 + 0xfffd); } TEST(ReconstructSeqnumTest, Halfway) { diff --git a/src/ssl/ssl_test.cc b/src/ssl/ssl_test.cc index dd295ccdc..eff071242 100644 --- a/src/ssl/ssl_test.cc +++ b/src/ssl/ssl_test.cc @@ -2773,6 +2773,11 @@ class SSLVersionTest : public ::testing::TestWithParam { uint16_t version() const { return GetParam().version; } + bool is_tls13() const { + return version() == TLS1_3_VERSION || + version() == DTLS1_3_EXPERIMENTAL_VERSION; + } + bool is_dtls() const { return GetParam().ssl_method == VersionParam::is_dtls; } @@ -3493,11 +3498,6 @@ static int SwitchSessionIDContextSNI(SSL *ssl, int *out_alert, void *arg) { } TEST_P(SSLVersionTest, SessionIDContext) { - if (version() == DTLS1_3_EXPERIMENTAL_VERSION) { - // TODO(crbug.com/42290594): Enable the rest of this test for DTLS 1.3 - // once it supports NewSessionTickets. - return; - } static const uint8_t kContext1[] = {1}; static const uint8_t kContext2[] = {2}; @@ -3634,11 +3634,6 @@ static bool GetServerTicketTime(long *out, const SSL_SESSION *session) { } TEST_P(SSLVersionTest, SessionTimeout) { - if (version() == DTLS1_3_EXPERIMENTAL_VERSION) { - // TODO(crbug.com/42290594): Enable the rest of this test for DTLS 1.3 - // once it supports NewSessionTickets. - return; - } for (bool server_test : {false, true}) { SCOPED_TRACE(server_test); @@ -3651,9 +3646,8 @@ TEST_P(SSLVersionTest, SessionTimeout) { // We are willing to use a longer lifetime for TLS 1.3 sessions as // resumptions still perform ECDHE. - const time_t timeout = version() == TLS1_3_VERSION - ? SSL_DEFAULT_SESSION_PSK_DHE_TIMEOUT - : SSL_DEFAULT_SESSION_TIMEOUT; + const time_t timeout = is_tls13() ? SSL_DEFAULT_SESSION_PSK_DHE_TIMEOUT + : SSL_DEFAULT_SESSION_TIMEOUT; // Both client and server must enforce session timeouts. We configure the // other side with a frozen clock so it never expires tickets. @@ -3713,7 +3707,7 @@ TEST_P(SSLVersionTest, SessionTimeout) { ASSERT_EQ(session_time, g_current_time.tv_sec); - if (version() == TLS1_3_VERSION) { + if (is_tls13()) { // Renewal incorporates fresh key material in TLS 1.3, so we extend the // lifetime TLS 1.3. g_current_time.tv_sec = new_start_time + timeout - 1; @@ -3775,11 +3769,6 @@ TEST_P(SSLVersionTest, DefaultTicketKeyInitialization) { } TEST_P(SSLVersionTest, DefaultTicketKeyRotation) { - if (version() == DTLS1_3_EXPERIMENTAL_VERSION) { - // TODO(crbug.com/42290594): Enable the rest of this test for DTLS 1.3 - // once it supports NewSessionTickets. - return; - } static const time_t kStartTime = 1001; g_current_time.tv_sec = kStartTime; @@ -4077,8 +4066,7 @@ TEST_P(SSLVersionTest, ALPNCipherAvailable) { TEST_P(SSLVersionTest, SSLClearSessionResumption) { // Skip this for TLS 1.3. TLS 1.3's ticket mechanism is incompatible with this // API pattern. - if (version() == TLS1_3_VERSION || - version() == DTLS1_3_EXPERIMENTAL_VERSION) { + if (is_tls13()) { return; } @@ -4483,11 +4471,6 @@ TEST_P(SSLVersionTest, GetServerName) { bssl::UniquePtr session = CreateClientSession(client_ctx_.get(), server_ctx_.get(), config); - if (version() == DTLS1_3_EXPERIMENTAL_VERSION) { - // TODO(crbug.com/42290594): Enable the rest of this test for DTLS 1.3 - // once it supports NewSessionTickets. - return; - } // If the client resumes a session with a different name, |SSL_get_servername| // must return the new name. ASSERT_TRUE(session); @@ -4500,11 +4483,6 @@ TEST_P(SSLVersionTest, GetServerName) { // Test that session cache mode bits are honored in the client session callback. TEST_P(SSLVersionTest, ClientSessionCacheMode) { - if (version() == DTLS1_3_EXPERIMENTAL_VERSION) { - // TODO(crbug.com/42290594): Enable the rest of this test for DTLS 1.3 - // once it supports NewSessionTickets. - return; - } SSL_CTX_set_session_cache_mode(client_ctx_.get(), SSL_SESS_CACHE_OFF); EXPECT_FALSE(CreateClientSession(client_ctx_.get(), server_ctx_.get())); @@ -5571,11 +5549,6 @@ TEST(SSLTest, NoCiphersAvailable) { } TEST_P(SSLVersionTest, SessionVersion) { - if (version() == DTLS1_3_EXPERIMENTAL_VERSION) { - // TODO(crbug.com/42290594): Enable the rest of this test for DTLS 1.3 - // once it supports NewSessionTickets. - return; - } SSL_CTX_set_session_cache_mode(client_ctx_.get(), SSL_SESS_CACHE_BOTH); SSL_CTX_set_session_cache_mode(server_ctx_.get(), SSL_SESS_CACHE_BOTH); @@ -5585,8 +5558,7 @@ TEST_P(SSLVersionTest, SessionVersion) { EXPECT_EQ(version(), SSL_SESSION_get_protocol_version(session.get())); // Sessions in TLS 1.3 and later should be single-use. - EXPECT_EQ(version() == TLS1_3_VERSION, - !!SSL_SESSION_should_be_single_use(session.get())); + EXPECT_EQ(is_tls13(), !!SSL_SESSION_should_be_single_use(session.get())); // Making fake sessions for testing works. session.reset(SSL_SESSION_new(client_ctx_.get())); @@ -6228,11 +6200,6 @@ TEST_P(SSLVersionTest, VerifyBeforeCertRequest) { // Test that ticket-based sessions on the client get fake session IDs. TEST_P(SSLVersionTest, FakeIDsForTickets) { - if (version() == DTLS1_3_EXPERIMENTAL_VERSION) { - // TODO(crbug.com/42290594): Enable the rest of this test for DTLS 1.3 - // once it supports NewSessionTickets. - return; - } SSL_CTX_set_session_cache_mode(client_ctx_.get(), SSL_SESS_CACHE_BOTH); SSL_CTX_set_session_cache_mode(server_ctx_.get(), SSL_SESS_CACHE_BOTH); @@ -6254,8 +6221,7 @@ TEST_P(SSLVersionTest, SessionCacheThreads) { SSL_CTX_set_session_cache_mode(client_ctx_.get(), SSL_SESS_CACHE_BOTH); SSL_CTX_set_session_cache_mode(server_ctx_.get(), SSL_SESS_CACHE_BOTH); - if (version() == TLS1_3_VERSION || - version() == DTLS1_3_EXPERIMENTAL_VERSION) { + if (is_tls13()) { // Our TLS 1.3 implementation does not support stateful resumption. ASSERT_FALSE(CreateClientSession(client_ctx_.get(), server_ctx_.get())); return; @@ -6364,11 +6330,6 @@ TEST_P(SSLVersionTest, SessionCacheThreads) { } TEST_P(SSLVersionTest, SessionTicketThreads) { - if (version() == DTLS1_3_EXPERIMENTAL_VERSION) { - // TODO(crbug.com/42290594): Enable the rest of this test for DTLS 1.3 - // once it supports NewSessionTickets. - return; - } for (bool renew_ticket : {false, true}) { SCOPED_TRACE(renew_ticket); ASSERT_NO_FATAL_FAILURE(ResetContexts()); @@ -6438,8 +6399,7 @@ TEST(SSLTest, GetCertificateThreads) { // performing stateful resumption will share an underlying SSL_SESSION object, // potentially across threads. TEST_P(SSLVersionTest, SessionPropertiesThreads) { - if (version() == TLS1_3_VERSION || - version() == DTLS1_3_EXPERIMENTAL_VERSION) { + if (is_tls13()) { // Our TLS 1.3 implementation does not support stateful resumption. ASSERT_FALSE(CreateClientSession(client_ctx_.get(), server_ctx_.get())); return; @@ -8041,11 +8001,6 @@ TEST_P(SSLVersionTest, DoubleSSLError) { } TEST_P(SSLVersionTest, SameKeyResume) { - if (version() == DTLS1_3_EXPERIMENTAL_VERSION) { - // TODO(crbug.com/42290594): Enable the rest of this test for DTLS 1.3 - // once it supports NewSessionTickets. - return; - } uint8_t key[48]; RAND_bytes(key, sizeof(key)); @@ -8083,11 +8038,6 @@ TEST_P(SSLVersionTest, SameKeyResume) { } TEST_P(SSLVersionTest, DifferentKeyNoResume) { - if (version() == DTLS1_3_EXPERIMENTAL_VERSION) { - // TODO(crbug.com/42290594): Enable the rest of this test for DTLS 1.3 - // once it supports NewSessionTickets. - return; - } uint8_t key1[48], key2[48]; RAND_bytes(key1, sizeof(key1)); RAND_bytes(key2, sizeof(key2)); @@ -8126,11 +8076,6 @@ TEST_P(SSLVersionTest, DifferentKeyNoResume) { } TEST_P(SSLVersionTest, UnrelatedServerNoResume) { - if (version() == DTLS1_3_EXPERIMENTAL_VERSION) { - // TODO(crbug.com/42290594): Enable the rest of this test for DTLS 1.3 - // once it supports NewSessionTickets. - return; - } bssl::UniquePtr server_ctx2 = CreateContext(); ASSERT_TRUE(server_ctx2); ASSERT_TRUE(UseCertAndKey(server_ctx2.get())); @@ -8168,11 +8113,6 @@ Span SessionIDOf(const SSL* ssl) { } TEST_P(SSLVersionTest, TicketSessionIDsMatch) { - if (version() == DTLS1_3_EXPERIMENTAL_VERSION) { - // TODO(crbug.com/42290594): Enable the rest of this test for DTLS 1.3 - // once it supports NewSessionTickets. - return; - } // This checks that the session IDs at client and server match after a ticket // resumption. It's unclear whether this should be true, but Envoy depends // on it in their tests so this will give an early signal if we break it. @@ -9648,8 +9588,7 @@ TEST_P(SSLVersionTest, KeyLog) { ASSERT_TRUE(Connect()); // Check that we logged the secrets we expected to log. - if (version() == TLS1_3_VERSION || - version() == DTLS1_3_EXPERIMENTAL_VERSION) { + if (is_tls13()) { EXPECT_THAT(client_log, ElementsAre(Key("CLIENT_HANDSHAKE_TRAFFIC_SECRET"), Key("CLIENT_TRAFFIC_SECRET_0"), Key("EXPORTER_SECRET"),