From 568bc23208441f4a7f302a26cce6061f9beee463 Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Sun, 20 Mar 2022 19:59:03 +0100 Subject: [PATCH] dcsctp: Don't reassemble already received chunks This is a solution to some problems that have been found locally when running the fuzzer for a long time. The fuzzer keeps on fuzzing, and has found a way to trigger a consistency check to fail when a client intentionally sends different messages - unordered and ordered - using the same TSNs. As the reassembly queue has different handling of ordered and unordered chunks due to how they are reassembled, it will not notice if it receives two different chunks with the same TSN. They will both go to their respective reassembly streams, as those are separate by design. The data tracker - which keeps track of all received DATA chunks as it needs to generate SACKs, has a global understanding of all received chunks. By having it indicate if this is a duplicate received chunk, the socket can avoid forwarding both chunks to the reassembly queue; only one chunk will get there. Bug: None Change-Id: I602a8552a9a4c853684fcf105309ec3d8073f2c2 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/256110 Reviewed-by: Harald Alvestrand Commit-Queue: Victor Boivie Cr-Commit-Position: refs/heads/main@{#36316} --- net/dcsctp/rx/data_tracker.cc | 6 +++- net/dcsctp/rx/data_tracker.h | 5 +-- net/dcsctp/rx/data_tracker_test.cc | 40 ++++++++++++++------- net/dcsctp/socket/dcsctp_socket.cc | 11 +++--- net/dcsctp/socket/dcsctp_socket_test.cc | 48 +++++++++++++++++++++++++ 5 files changed, 90 insertions(+), 20 deletions(-) diff --git a/net/dcsctp/rx/data_tracker.cc b/net/dcsctp/rx/data_tracker.cc index f31847b524..8faee9e7d2 100644 --- a/net/dcsctp/rx/data_tracker.cc +++ b/net/dcsctp/rx/data_tracker.cc @@ -123,8 +123,9 @@ bool DataTracker::IsTSNValid(TSN tsn) const { return true; } -void DataTracker::Observe(TSN tsn, +bool DataTracker::Observe(TSN tsn, AnyDataChunk::ImmediateAckFlag immediate_ack) { + bool is_duplicate = false; UnwrappedTSN unwrapped_tsn = tsn_unwrapper_.Unwrap(tsn); // IsTSNValid must be called prior to calling this method. @@ -143,6 +144,7 @@ void DataTracker::Observe(TSN tsn, // packet arrives with duplicate DATA chunk(s) bundled with new DATA chunks, // the endpoint MAY immediately send a SACK." UpdateAckState(AckState::kImmediate, "duplicate data"); + is_duplicate = true; } else { if (unwrapped_tsn == last_cumulative_acked_tsn_.next_value()) { last_cumulative_acked_tsn_ = unwrapped_tsn; @@ -167,6 +169,7 @@ void DataTracker::Observe(TSN tsn, // delay. If a packet arrives with duplicate DATA chunk(s) bundled with // new DATA chunks, the endpoint MAY immediately send a SACK." // No need to do this. SACKs are sent immediately on packet loss below. + is_duplicate = true; } } } @@ -208,6 +211,7 @@ void DataTracker::Observe(TSN tsn, } else if (ack_state_ == AckState::kDelayed) { UpdateAckState(AckState::kImmediate, "received DATA when already delayed"); } + return !is_duplicate; } void DataTracker::HandleForwardTsn(TSN new_cumulative_ack) { diff --git a/net/dcsctp/rx/data_tracker.h b/net/dcsctp/rx/data_tracker.h index fb8add82a2..603a237245 100644 --- a/net/dcsctp/rx/data_tracker.h +++ b/net/dcsctp/rx/data_tracker.h @@ -69,8 +69,9 @@ class DataTracker { // means that there is intentional packet loss. bool IsTSNValid(TSN tsn) const; - // Call for every incoming data chunk. - void Observe(TSN tsn, + // Call for every incoming data chunk. Returns `true` if `tsn` was seen for + // the first time, and `false` if it has been seen before (a duplicate `tsn`). + bool Observe(TSN tsn, AnyDataChunk::ImmediateAckFlag immediate_ack = AnyDataChunk::ImmediateAckFlag(false)); // Called at the end of processing an SCTP packet. diff --git a/net/dcsctp/rx/data_tracker_test.cc b/net/dcsctp/rx/data_tracker_test.cc index 3d8380a05c..43494734b6 100644 --- a/net/dcsctp/rx/data_tracker_test.cc +++ b/net/dcsctp/rx/data_tracker_test.cc @@ -48,9 +48,16 @@ class DataTrackerTest : public testing::Test { std::make_unique("log: ", timer_.get(), kInitialTSN)) { } - void Observer(std::initializer_list tsns) { + void Observer(std::initializer_list tsns, + bool expect_as_duplicate = false) { for (const uint32_t tsn : tsns) { - tracker_->Observe(TSN(tsn), AnyDataChunk::ImmediateAckFlag(false)); + if (expect_as_duplicate) { + EXPECT_FALSE( + tracker_->Observe(TSN(tsn), AnyDataChunk::ImmediateAckFlag(false))); + } else { + EXPECT_TRUE( + tracker_->Observe(TSN(tsn), AnyDataChunk::ImmediateAckFlag(false))); + } } } @@ -125,7 +132,7 @@ TEST_F(DataTrackerTest, AckAlreadyReceivedChunk) { EXPECT_THAT(sack1.gap_ack_blocks(), IsEmpty()); // Receive old chunk - Observer({8}); + Observer({8}, /*expect_as_duplicate=*/true); SackChunk sack2 = tracker_->CreateSelectiveAck(kArwnd); EXPECT_EQ(sack2.cumulative_tsn_ack(), TSN(11)); EXPECT_THAT(sack2.gap_ack_blocks(), IsEmpty()); @@ -145,7 +152,8 @@ TEST_F(DataTrackerTest, DoubleSendRetransmittedChunk) { EXPECT_THAT(sack2.gap_ack_blocks(), IsEmpty()); // Receive chunk 12 again. - Observer({12, 19, 20, 21}); + Observer({12}, /*expect_as_duplicate=*/true); + Observer({19, 20, 21}); SackChunk sack3 = tracker_->CreateSelectiveAck(kArwnd); EXPECT_EQ(sack3.cumulative_tsn_ack(), TSN(21)); EXPECT_THAT(sack3.gap_ack_blocks(), IsEmpty()); @@ -176,7 +184,8 @@ TEST_F(DataTrackerTest, ForwardTsnSkipsFromGapBlock) { TEST_F(DataTrackerTest, ExampleFromRFC3758) { tracker_->HandleForwardTsn(TSN(102)); - Observer({102, 104, 105, 107}); + Observer({102}, /*expect_as_duplicate=*/true); + Observer({104, 105, 107}); tracker_->HandleForwardTsn(TSN(103)); @@ -246,7 +255,8 @@ TEST_F(DataTrackerTest, WillNotAcceptInvalidTSNs) { } TEST_F(DataTrackerTest, ReportSingleDuplicateTsns) { - Observer({11, 12, 11}); + Observer({11, 12}); + Observer({11}, /*expect_as_duplicate=*/true); SackChunk sack = tracker_->CreateSelectiveAck(kArwnd); EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(12)); EXPECT_THAT(sack.gap_ack_blocks(), IsEmpty()); @@ -254,7 +264,9 @@ TEST_F(DataTrackerTest, ReportSingleDuplicateTsns) { } TEST_F(DataTrackerTest, ReportMultipleDuplicateTsns) { - Observer({11, 12, 13, 14, 12, 13, 12, 13, 15, 16}); + Observer({11, 12, 13, 14}); + Observer({12, 13, 12, 13}, /*expect_as_duplicate=*/true); + Observer({15, 16}); SackChunk sack = tracker_->CreateSelectiveAck(kArwnd); EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(16)); EXPECT_THAT(sack.gap_ack_blocks(), IsEmpty()); @@ -262,7 +274,9 @@ TEST_F(DataTrackerTest, ReportMultipleDuplicateTsns) { } TEST_F(DataTrackerTest, ReportDuplicateTsnsInGapAckBlocks) { - Observer({11, /*12,*/ 13, 14, 13, 14, 15, 16}); + Observer({11, /*12,*/ 13, 14}); + Observer({13, 14}, /*expect_as_duplicate=*/true); + Observer({15, 16}); SackChunk sack = tracker_->CreateSelectiveAck(kArwnd); EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(11)); EXPECT_THAT(sack.gap_ack_blocks(), ElementsAre(SackChunk::GapAckBlock(2, 5))); @@ -270,7 +284,9 @@ TEST_F(DataTrackerTest, ReportDuplicateTsnsInGapAckBlocks) { } TEST_F(DataTrackerTest, ClearsDuplicateTsnsAfterCreatingSack) { - Observer({11, 12, 13, 14, 12, 13, 12, 13, 15, 16}); + Observer({11, 12, 13, 14}); + Observer({12, 13, 12, 13}, /*expect_as_duplicate=*/true); + Observer({15, 16}); SackChunk sack1 = tracker_->CreateSelectiveAck(kArwnd); EXPECT_EQ(sack1.cumulative_tsn_ack(), TSN(16)); EXPECT_THAT(sack1.gap_ack_blocks(), IsEmpty()); @@ -380,7 +396,7 @@ TEST_F(DataTrackerTest, SendsSackOnDuplicateDataChunks) { tracker_->ObservePacketEnd(); EXPECT_TRUE(tracker_->ShouldSendAck()); EXPECT_FALSE(timer_->is_running()); - Observer({11}); + Observer({11}, /*expect_as_duplicate=*/true); tracker_->ObservePacketEnd(); EXPECT_TRUE(tracker_->ShouldSendAck()); EXPECT_FALSE(timer_->is_running()); @@ -394,7 +410,7 @@ TEST_F(DataTrackerTest, SendsSackOnDuplicateDataChunks) { EXPECT_TRUE(tracker_->ShouldSendAck()); EXPECT_FALSE(timer_->is_running()); // Duplicate again - Observer({12}); + Observer({12}, /*expect_as_duplicate=*/true); tracker_->ObservePacketEnd(); EXPECT_TRUE(tracker_->ShouldSendAck()); EXPECT_FALSE(timer_->is_running()); @@ -418,7 +434,7 @@ TEST_F(DataTrackerTest, GapAckBlockAddsAnother) { TEST_F(DataTrackerTest, GapAckBlockAddsDuplicate) { Observer({12}); - Observer({12}); + Observer({12}, /*expect_as_duplicate=*/true); SackChunk sack = tracker_->CreateSelectiveAck(kArwnd); EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(10)); EXPECT_THAT(sack.gap_ack_blocks(), ElementsAre(SackChunk::GapAckBlock(2, 2))); diff --git a/net/dcsctp/socket/dcsctp_socket.cc b/net/dcsctp/socket/dcsctp_socket.cc index b93584ed47..21b40d854f 100644 --- a/net/dcsctp/socket/dcsctp_socket.cc +++ b/net/dcsctp/socket/dcsctp_socket.cc @@ -1024,11 +1024,12 @@ void DcSctpSocket::HandleDataCommon(AnyDataChunk& chunk) { return; } - tcb_->data_tracker().Observe(tsn, immediate_ack); - tcb_->reassembly_queue().MaybeResetStreamsDeferred( - tcb_->data_tracker().last_cumulative_acked_tsn()); - tcb_->reassembly_queue().Add(tsn, std::move(data)); - DeliverReassembledMessages(); + if (tcb_->data_tracker().Observe(tsn, immediate_ack)) { + tcb_->reassembly_queue().MaybeResetStreamsDeferred( + tcb_->data_tracker().last_cumulative_acked_tsn()); + tcb_->reassembly_queue().Add(tsn, std::move(data)); + DeliverReassembledMessages(); + } } void DcSctpSocket::HandleInit(const CommonHeader& header, diff --git a/net/dcsctp/socket/dcsctp_socket_test.cc b/net/dcsctp/socket/dcsctp_socket_test.cc index d30043d332..848b7d6274 100644 --- a/net/dcsctp/socket/dcsctp_socket_test.cc +++ b/net/dcsctp/socket/dcsctp_socket_test.cc @@ -2214,5 +2214,53 @@ TEST_P(DcSctpSocketParametrizedTest, CanLoseFirstOrderedMessage) { MaybeHandoverSocketAndSendMessage(a, std::move(z)); } +TEST(DcSctpSocketTest, ReceiveBothUnorderedAndOrderedWithSameTSN) { + /* This issue was found by fuzzing. */ + SocketUnderTest a("A"); + SocketUnderTest z("Z"); + + a.socket.Connect(); + std::vector init_data = a.cb.ConsumeSentPacket(); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket init_packet, + SctpPacket::Parse(init_data)); + ASSERT_HAS_VALUE_AND_ASSIGN( + InitChunk init_chunk, + InitChunk::Parse(init_packet.descriptors()[0].data)); + z.socket.ReceivePacket(init_data); + a.socket.ReceivePacket(z.cb.ConsumeSentPacket()); + z.socket.ReceivePacket(a.cb.ConsumeSentPacket()); + a.socket.ReceivePacket(z.cb.ConsumeSentPacket()); + + // Receive a short unordered message with tsn=INITIAL_TSN+1 + TSN tsn = init_chunk.initial_tsn(); + AnyDataChunk::Options opts; + opts.is_beginning = Data::IsBeginning(true); + opts.is_end = Data::IsEnd(true); + opts.is_unordered = IsUnordered(true); + z.socket.ReceivePacket( + SctpPacket::Builder(z.socket.verification_tag(), z.options) + .Add(DataChunk(TSN(*tsn + 1), StreamID(1), SSN(0), PPID(53), + std::vector(10), opts)) + .Build()); + + // Now receive a longer _ordered_ message with [INITIAL_TSN, INITIAL_TSN+1]. + // This isn't allowed as it reuses TSN=53 with different properties, but it + // shouldn't cause any issues. + opts.is_unordered = IsUnordered(false); + opts.is_end = Data::IsEnd(false); + z.socket.ReceivePacket( + SctpPacket::Builder(z.socket.verification_tag(), z.options) + .Add(DataChunk(tsn, StreamID(1), SSN(0), PPID(53), + std::vector(10), opts)) + .Build()); + + opts.is_beginning = Data::IsBeginning(false); + opts.is_end = Data::IsEnd(true); + z.socket.ReceivePacket( + SctpPacket::Builder(z.socket.verification_tag(), z.options) + .Add(DataChunk(TSN(*tsn + 1), StreamID(1), SSN(0), PPID(53), + std::vector(10), opts)) + .Build()); +} } // namespace } // namespace dcsctp