commit f75f9c2b2958e6179415665865dec6466726a8ce
author Victor Boivie <boivie@webrtc.org> Fri Sep 24 09:50:29 2021
committer WebRTC LUCI CQ <webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com> Mon Sep 27 10:52:46 2021
tree 4522ab098a11d9852d0f5da04403dc9d1e4730d2
parent 29b4049abc5928d995b51f8614dfb27dc7e3359e

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 <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35095}

net/dcsctp/socket/heartbeat_handler.cc

diff --git a/net/dcsctp/socket/heartbeat_handler.cc b/net/dcsctp/socket/heartbeat_handler.cc
index 87baa14..8f41b9d 100644
--- a/net/dcsctp/socket/heartbeat_handler.cc
+++ b/net/dcsctp/socket/heartbeat_handler.cc
@@ -153,9 +153,10 @@
     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

net/dcsctp/socket/heartbeat_handler_test.cc

diff --git a/net/dcsctp/socket/heartbeat_handler_test.cc b/net/dcsctp/socket/heartbeat_handler_test.cc
index 2c5df9f..83a7355 100644
--- a/net/dcsctp/socket/heartbeat_handler_test.cc
+++ b/net/dcsctp/socket/heartbeat_handler_test.cc
@@ -135,6 +135,29 @@
   handler_.HandleHeartbeatAck(std::move(ack));
 }
 
+TEST_F(HeartbeatHandlerTest, DoesntObserveInvalidHeartbeats) {
+  AdvanceTime(options_.heartbeat_interval);
+
+  // Grab the request, and make a response.
+  std::vector<uint8_t> 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));