From 584b4df92d3332eb97fb5142e9a48c77cdb7a3b5 Mon Sep 17 00:00:00 2001
From: Victor Boivie <boivie@webrtc.org>
Date: Tue, 8 Mar 2022 23:05:10 +0100
Subject: [PATCH] dcsctp: Don't deliver skipped messages

If a FORWARD-TSN contains an ordered skipped stream with a large TSN
but with a too small SSN, it can result in messages being assembled
that should've been skipped. Typically:

Receive DATA, ordered, complete, TSN=10, SID=1, SSN=0
  - will be delivered.
Receive DATA, ordered, complete, TSN=43, SID=1, SSN=7
  - will stay in queue, due to missing SSN=1,2,3,4,5,6.
Receive FORWARD-TSN, TSN=44, SSN=6
  - is invalid, as the SSN should've been 7 or higher.

However, as the TSN isn't used for removing messages in ordered streams,
but just the SSN, the SSN=7 isn't removed but instead will be delivered
as it's the next following SSN after 6. This will trigger internal
consistency checks as a chunk with TSN=43 will be delivered when the
current cumulative TSN is set to 44, which is greater.

This was found when fuzzing, and can only be provoked by a client that
is intentionally misbehaving. Before this fix, there was no harm done,
but it failed consistency checks which fuzzers have enabled. When
bug 13799 was fixed (in a previous commit), this allowed the fuzzers to
find it faster.

Bug: webrtc:13799
Change-Id: I830ef189476e227e1dbe08157d34f96ad6453e30
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/254240
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36157}
---
 net/dcsctp/packet/chunk/forward_tsn_chunk.cc     |  3 +++
 .../packet/chunk/forward_tsn_chunk_test.cc       |  3 ++-
 net/dcsctp/rx/reassembly_queue.cc                | 12 ++++++++++--
 net/dcsctp/rx/reassembly_queue_test.cc           | 16 ++++++++++++++++
 4 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/net/dcsctp/packet/chunk/forward_tsn_chunk.cc b/net/dcsctp/packet/chunk/forward_tsn_chunk.cc
index f01505094d..e432114c50 100644
--- a/net/dcsctp/packet/chunk/forward_tsn_chunk.cc
+++ b/net/dcsctp/packet/chunk/forward_tsn_chunk.cc
@@ -87,6 +87,9 @@ void ForwardTsnChunk::SerializeTo(std::vector<uint8_t>& out) const {
 std::string ForwardTsnChunk::ToString() const {
   rtc::StringBuilder sb;
   sb << "FORWARD-TSN, new_cumulative_tsn=" << *new_cumulative_tsn();
+  for (const auto& skipped : skipped_streams()) {
+    sb << ", skip " << skipped.stream_id.value() << ":" << *skipped.ssn;
+  }
   return sb.str();
 }
 }  // namespace dcsctp
diff --git a/net/dcsctp/packet/chunk/forward_tsn_chunk_test.cc b/net/dcsctp/packet/chunk/forward_tsn_chunk_test.cc
index 9420c1f2ef..51f97f2396 100644
--- a/net/dcsctp/packet/chunk/forward_tsn_chunk_test.cc
+++ b/net/dcsctp/packet/chunk/forward_tsn_chunk_test.cc
@@ -56,7 +56,8 @@ TEST(ForwardTsnChunkTest, SerializeAndDeserialize) {
       ElementsAre(ForwardTsnChunk::SkippedStream(StreamID(1), SSN(23)),
                   ForwardTsnChunk::SkippedStream(StreamID(42), SSN(99))));
 
-  EXPECT_EQ(deserialized.ToString(), "FORWARD-TSN, new_cumulative_tsn=123");
+  EXPECT_EQ(deserialized.ToString(),
+            "FORWARD-TSN, new_cumulative_tsn=123, skip 1:23, skip 42:99");
 }
 
 }  // namespace
diff --git a/net/dcsctp/rx/reassembly_queue.cc b/net/dcsctp/rx/reassembly_queue.cc
index 5791d6805c..cbf198b136 100644
--- a/net/dcsctp/rx/reassembly_queue.cc
+++ b/net/dcsctp/rx/reassembly_queue.cc
@@ -210,8 +210,16 @@ void ReassemblyQueue::AddReassembledMessage(
                        << ", payload=" << message.payload().size() << " bytes";
 
   for (const UnwrappedTSN tsn : tsns) {
-    // Update watermark, or insert into delivered_tsns_
-    if (tsn == last_assembled_tsn_watermark_.next_value()) {
+    if (tsn <= last_assembled_tsn_watermark_) {
+      // This can be provoked by a misbehaving peer by sending FORWARD-TSN with
+      // invalid SSNs, allowing ordered messages to stay in the queue that
+      // should've been discarded.
+      RTC_DLOG(LS_VERBOSE)
+          << log_prefix_
+          << "Message is built from fragments already seen - skipping";
+      return;
+    } else if (tsn == last_assembled_tsn_watermark_.next_value()) {
+      // Update watermark, or insert into delivered_tsns_
       last_assembled_tsn_watermark_.Increment();
     } else {
       delivered_tsns_.insert(tsn);
diff --git a/net/dcsctp/rx/reassembly_queue_test.cc b/net/dcsctp/rx/reassembly_queue_test.cc
index d1e3bf6413..bc1b776837 100644
--- a/net/dcsctp/rx/reassembly_queue_test.cc
+++ b/net/dcsctp/rx/reassembly_queue_test.cc
@@ -389,5 +389,21 @@ TEST_F(ReassemblyQueueTest, HandoverAfterHavingAssembedOneMessage) {
   reasm2.Add(TSN(11), gen_.Ordered({1, 2, 3, 4}, "BE"));
   EXPECT_THAT(reasm2.FlushMessages(), SizeIs(1));
 }
+
+TEST_F(ReassemblyQueueTest, HandleInconsistentForwardTSN) {
+  // Found when fuzzing.
+  ReassemblyQueue reasm("log: ", TSN(10), kBufferSize);
+  // Add TSN=43, SSN=7. Can't be reassembled as previous SSNs aren't known.
+  reasm.Add(TSN(43), Data(kStreamID, SSN(7), MID(0), FSN(0), kPPID,
+                          std::vector<uint8_t>(10), Data::IsBeginning(true),
+                          Data::IsEnd(true), IsUnordered(false)));
+
+  // Invalid, as TSN=44 have to have SSN>=7, but peer says 6.
+  reasm.Handle(ForwardTsnChunk(
+      TSN(44), {ForwardTsnChunk::SkippedStream(kStreamID, SSN(6))}));
+
+  // Don't assemble SSN=7, as that TSN is skipped.
+  EXPECT_FALSE(reasm.HasMessages());
+}
 }  // namespace
 }  // namespace dcsctp