zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

dcsctp: Don't deliver skipped messages

Browse Source 
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}
This commit is contained in:
Victor Boivie
2022-03-08 23:05:10 +01:00
committed by WebRTC LUCI CQ
parent d7031692e3
commit 584b4df92d
4 changed files with 31 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
net/dcsctp/packet/chunk/forward_tsn_chunk.cc
View File

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

3
net/dcsctp/packet/chunk/forward_tsn_chunk_test.cc
View File

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

12
net/dcsctp/rx/reassembly_queue.cc
View File

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

16
net/dcsctp/rx/reassembly_queue_test.cc
View File

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