From f75f9c2b2958e6179415665865dec6466726a8ce Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Fri, 24 Sep 2021 11:50:29 +0200 Subject: [PATCH] dcsctp: Avoid integer overflow in HEARTBEAT-ACK When a HEARTBEAT is sent out, the current timestamp is stored in the parameter that will be returned by the HEARTBEAT-ACK. If the timestamp from the HEARTBEAT-ACK would be from the future (either by jumping clocks, bit errors, exploiting peer or a fuzzer), the measured RTT would become really large, and when it was calculated, it would result in an integer overflow; a wrapping subtraction. This isn't a problem, as RetransmissionTimeout::ObserveRTT method would discard measurements that were negative or too large, but it would trigger an UBSAN violation. Adding an additional check so that the subtraction doesn't ever wrap. Bug: chromium:1252515 Change-Id: I1f97b1e773a4639b8193a528716ebd6a27fcb740 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232904 Reviewed-by: Florent Castelli Commit-Queue: Victor Boivie Cr-Commit-Position: refs/heads/main@{#35095} --- net/dcsctp/socket/heartbeat_handler.cc | 7 ++++--- net/dcsctp/socket/heartbeat_handler_test.cc | 23 +++++++++++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/net/dcsctp/socket/heartbeat_handler.cc b/net/dcsctp/socket/heartbeat_handler.cc index 87baa147d9..8f41b9d925 100644 --- a/net/dcsctp/socket/heartbeat_handler.cc +++ b/net/dcsctp/socket/heartbeat_handler.cc @@ -153,9 +153,10 @@ void HeartbeatHandler::HandleHeartbeatAck(HeartbeatAckChunk chunk) { return; } - DurationMs duration(*ctx_->callbacks().TimeMillis() - *info->created_at()); - - ctx_->ObserveRTT(duration); + TimeMs now = ctx_->callbacks().TimeMillis(); + if (info->created_at() <= now) { + ctx_->ObserveRTT(now - info->created_at()); + } // https://tools.ietf.org/html/rfc4960#section-8.1 // "The counter shall be reset each time ... a HEARTBEAT ACK is received from diff --git a/net/dcsctp/socket/heartbeat_handler_test.cc b/net/dcsctp/socket/heartbeat_handler_test.cc index 2c5df9fd92..83a7355ac9 100644 --- a/net/dcsctp/socket/heartbeat_handler_test.cc +++ b/net/dcsctp/socket/heartbeat_handler_test.cc @@ -135,6 +135,29 @@ TEST_F(HeartbeatHandlerTest, SendsHeartbeatRequestsOnIdleChannel) { handler_.HandleHeartbeatAck(std::move(ack)); } +TEST_F(HeartbeatHandlerTest, DoesntObserveInvalidHeartbeats) { + AdvanceTime(options_.heartbeat_interval); + + // Grab the request, and make a response. + std::vector payload = callbacks_.ConsumeSentPacket(); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(payload)); + ASSERT_THAT(packet.descriptors(), SizeIs(1)); + + ASSERT_HAS_VALUE_AND_ASSIGN( + HeartbeatRequestChunk req, + HeartbeatRequestChunk::Parse(packet.descriptors()[0].data)); + + HeartbeatAckChunk ack(std::move(req).extract_parameters()); + + EXPECT_CALL(context_, ObserveRTT).Times(0); + + // Go backwards in time - which make the HEARTBEAT-ACK have an invalid + // timestamp in it, as it will be in the future. + callbacks_.AdvanceTime(DurationMs(-100)); + + handler_.HandleHeartbeatAck(std::move(ack)); +} + TEST_F(HeartbeatHandlerTest, IncreasesErrorIfNotAckedInTime) { DurationMs rto(105); EXPECT_CALL(context_, current_rto).WillOnce(Return(rto));