admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

dcsctp: Avoid integer overflow in HEARTBEAT-ACK

Browse Source 
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}
This commit is contained in:
Victor Boivie 2021-09-24 11:50:29 +02:00 committed by WebRTC LUCI CQ
parent 29b4049abc
commit f75f9c2b29
2 changed files with 27 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
net/dcsctp/socket/heartbeat_handler.cc
View File

@ -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

23
net/dcsctp/socket/heartbeat_handler_test.cc
View File

@ -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<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));