From 0b0afaa81a784e824d3007e8f3503edaa7740179 Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Sun, 11 Apr 2021 23:35:44 +0200 Subject: [PATCH] dcsctp: Add Chunk Validators The SCTP RFCs aren't very strict in specifying when a chunk or parameter is invalid, so most chunks and/or parameters must be accepted but they may need some cleaning to avoid a lot of error handling deeper in the chunk handling code. Bug: webrtc:12614 Change-Id: I723f08cbdc26e1a1b78463b6137340e638089037 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/214966 Reviewed-by: Harald Alvestrand Commit-Queue: Victor Boivie Cr-Commit-Position: refs/heads/master@{#33788} --- net/dcsctp/packet/BUILD.gn | 15 ++ net/dcsctp/packet/chunk_validators.cc | 90 ++++++++++++ net/dcsctp/packet/chunk_validators.h | 33 +++++ net/dcsctp/packet/chunk_validators_test.cc | 161 +++++++++++++++++++++ 4 files changed, 299 insertions(+) create mode 100644 net/dcsctp/packet/chunk_validators.cc create mode 100644 net/dcsctp/packet/chunk_validators.h create mode 100644 net/dcsctp/packet/chunk_validators_test.cc diff --git a/net/dcsctp/packet/BUILD.gn b/net/dcsctp/packet/BUILD.gn index 13769b2abe..9893547b11 100644 --- a/net/dcsctp/packet/BUILD.gn +++ b/net/dcsctp/packet/BUILD.gn @@ -210,6 +210,19 @@ rtc_library("chunk") { absl_deps = [ "//third_party/abseil-cpp/absl/types:optional" ] } +rtc_library("chunk_validators") { + deps = [ + ":chunk", + "../../../rtc_base", + "../../../rtc_base:checks", + "../../../rtc_base:rtc_base_approved", + ] + sources = [ + "chunk_validators.cc", + "chunk_validators.h", + ] +} + rtc_library("sctp_packet") { deps = [ ":bounded_io", @@ -240,6 +253,7 @@ if (rtc_include_tests) { deps = [ ":bounded_io", ":chunk", + ":chunk_validators", ":crc32c", ":error_cause", ":parameter", @@ -271,6 +285,7 @@ if (rtc_include_tests) { "chunk/shutdown_ack_chunk_test.cc", "chunk/shutdown_chunk_test.cc", "chunk/shutdown_complete_chunk_test.cc", + "chunk_validators_test.cc", "crc32c_test.cc", "error_cause/cookie_received_while_shutting_down_cause_test.cc", "error_cause/invalid_mandatory_parameter_cause_test.cc", diff --git a/net/dcsctp/packet/chunk_validators.cc b/net/dcsctp/packet/chunk_validators.cc new file mode 100644 index 0000000000..b3467037c7 --- /dev/null +++ b/net/dcsctp/packet/chunk_validators.cc @@ -0,0 +1,90 @@ +/* + * Copyright (c) 2021 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ +#include "net/dcsctp/packet/chunk_validators.h" + +#include +#include +#include + +#include "net/dcsctp/packet/chunk/sack_chunk.h" +#include "rtc_base/logging.h" + +namespace dcsctp { + +SackChunk ChunkValidators::Clean(SackChunk&& sack) { + if (Validate(sack)) { + return std::move(sack); + } + + RTC_DLOG(LS_WARNING) << "Received SACK is malformed; cleaning it"; + + std::vector gap_ack_blocks; + gap_ack_blocks.reserve(sack.gap_ack_blocks().size()); + + // First: Only keep blocks that are sane + for (const SackChunk::GapAckBlock& gap_ack_block : sack.gap_ack_blocks()) { + if (gap_ack_block.end > gap_ack_block.start) { + gap_ack_blocks.emplace_back(gap_ack_block); + } + } + + // Not more than at most one remaining? Exit early. + if (gap_ack_blocks.size() <= 1) { + return SackChunk(sack.cumulative_tsn_ack(), sack.a_rwnd(), + std::move(gap_ack_blocks), + std::vector(sack.duplicate_tsns().begin(), + sack.duplicate_tsns().end())); + } + + // Sort the intervals by their start value, to aid in the merging below. + absl::c_sort(gap_ack_blocks, [&](const SackChunk::GapAckBlock& a, + const SackChunk::GapAckBlock& b) { + return a.start < b.start; + }); + + // Merge overlapping ranges. + std::vector merged; + merged.reserve(gap_ack_blocks.size()); + merged.push_back(gap_ack_blocks[0]); + + for (size_t i = 1; i < gap_ack_blocks.size(); ++i) { + if (merged.back().end + 1 >= gap_ack_blocks[i].start) { + merged.back().end = std::max(merged.back().end, gap_ack_blocks[i].end); + } else { + merged.push_back(gap_ack_blocks[i]); + } + } + + return SackChunk(sack.cumulative_tsn_ack(), sack.a_rwnd(), std::move(merged), + std::vector(sack.duplicate_tsns().begin(), + sack.duplicate_tsns().end())); +} + +bool ChunkValidators::Validate(const SackChunk& sack) { + if (sack.gap_ack_blocks().empty()) { + return true; + } + + // Ensure that gap-ack-blocks are sorted, has an "end" that is not before + // "start" and are non-overlapping and non-adjacent. + uint16_t prev_end = 0; + for (const SackChunk::GapAckBlock& gap_ack_block : sack.gap_ack_blocks()) { + if (gap_ack_block.end < gap_ack_block.start) { + return false; + } + if (gap_ack_block.start <= (prev_end + 1)) { + return false; + } + prev_end = gap_ack_block.end; + } + return true; +} + +} // namespace dcsctp diff --git a/net/dcsctp/packet/chunk_validators.h b/net/dcsctp/packet/chunk_validators.h new file mode 100644 index 0000000000..b11848a162 --- /dev/null +++ b/net/dcsctp/packet/chunk_validators.h @@ -0,0 +1,33 @@ +/* + * Copyright (c) 2021 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ +#ifndef NET_DCSCTP_PACKET_CHUNK_VALIDATORS_H_ +#define NET_DCSCTP_PACKET_CHUNK_VALIDATORS_H_ + +#include "net/dcsctp/packet/chunk/sack_chunk.h" + +namespace dcsctp { +// Validates and cleans SCTP chunks. +class ChunkValidators { + public: + // Given a SackChunk, will return `true` if it's valid, and `false` if not. + static bool Validate(const SackChunk& sack); + + // Given a SackChunk, it will return a cleaned and validated variant of it. + // RFC4960 doesn't say anything about validity of SACKs or if the Gap ACK + // blocks must be sorted, and non-overlapping. While they always are in + // well-behaving implementations, this can't be relied on. + // + // This method internally calls `Validate`, which means that you can always + // pass a SackChunk to this method (valid or not), and use the results. + static SackChunk Clean(SackChunk&& sack); +}; +} // namespace dcsctp + +#endif // NET_DCSCTP_PACKET_CHUNK_VALIDATORS_H_ diff --git a/net/dcsctp/packet/chunk_validators_test.cc b/net/dcsctp/packet/chunk_validators_test.cc new file mode 100644 index 0000000000..d59fd4ec48 --- /dev/null +++ b/net/dcsctp/packet/chunk_validators_test.cc @@ -0,0 +1,161 @@ +/* + * Copyright (c) 2021 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ +#include "net/dcsctp/packet/chunk_validators.h" + +#include + +#include "rtc_base/gunit.h" +#include "test/gmock.h" + +namespace dcsctp { +namespace { +using ::testing::ElementsAre; +using ::testing::IsEmpty; + +TEST(ChunkValidatorsTest, NoGapAckBlocksAreValid) { + SackChunk sack(TSN(123), /*a_rwnd=*/456, + /*gap_ack_blocks=*/{}, {}); + + EXPECT_TRUE(ChunkValidators::Validate(sack)); + + SackChunk clean = ChunkValidators::Clean(std::move(sack)); + EXPECT_THAT(clean.gap_ack_blocks(), IsEmpty()); +} + +TEST(ChunkValidatorsTest, OneValidAckBlock) { + SackChunk sack(TSN(123), /*a_rwnd=*/456, {SackChunk::GapAckBlock(2, 3)}, {}); + + EXPECT_TRUE(ChunkValidators::Validate(sack)); + + SackChunk clean = ChunkValidators::Clean(std::move(sack)); + EXPECT_THAT(clean.gap_ack_blocks(), + ElementsAre(SackChunk::GapAckBlock(2, 3))); +} + +TEST(ChunkValidatorsTest, TwoValidAckBlocks) { + SackChunk sack(TSN(123), /*a_rwnd=*/456, + {SackChunk::GapAckBlock(2, 3), SackChunk::GapAckBlock(5, 6)}, + {}); + + EXPECT_TRUE(ChunkValidators::Validate(sack)); + + SackChunk clean = ChunkValidators::Clean(std::move(sack)); + EXPECT_THAT( + clean.gap_ack_blocks(), + ElementsAre(SackChunk::GapAckBlock(2, 3), SackChunk::GapAckBlock(5, 6))); +} + +TEST(ChunkValidatorsTest, OneInvalidAckBlock) { + SackChunk sack(TSN(123), /*a_rwnd=*/456, {SackChunk::GapAckBlock(1, 2)}, {}); + + EXPECT_FALSE(ChunkValidators::Validate(sack)); + + // It's not strictly valid, but due to the renegable nature of gap ack blocks, + // the cum_ack_tsn can't simply be moved. + SackChunk clean = ChunkValidators::Clean(std::move(sack)); + EXPECT_THAT(clean.gap_ack_blocks(), + ElementsAre(SackChunk::GapAckBlock(1, 2))); +} + +TEST(ChunkValidatorsTest, RemovesInvalidGapAckBlockFromSack) { + SackChunk sack(TSN(123), /*a_rwnd=*/456, + {SackChunk::GapAckBlock(2, 3), SackChunk::GapAckBlock(6, 4)}, + {}); + + EXPECT_FALSE(ChunkValidators::Validate(sack)); + + SackChunk clean = ChunkValidators::Clean(std::move(sack)); + + EXPECT_THAT(clean.gap_ack_blocks(), + ElementsAre(SackChunk::GapAckBlock(2, 3))); +} + +TEST(ChunkValidatorsTest, SortsGapAckBlocksInOrder) { + SackChunk sack(TSN(123), /*a_rwnd=*/456, + {SackChunk::GapAckBlock(6, 7), SackChunk::GapAckBlock(3, 4)}, + {}); + + EXPECT_FALSE(ChunkValidators::Validate(sack)); + + SackChunk clean = ChunkValidators::Clean(std::move(sack)); + + EXPECT_THAT( + clean.gap_ack_blocks(), + ElementsAre(SackChunk::GapAckBlock(3, 4), SackChunk::GapAckBlock(6, 7))); +} + +TEST(ChunkValidatorsTest, MergesAdjacentBlocks) { + SackChunk sack(TSN(123), /*a_rwnd=*/456, + {SackChunk::GapAckBlock(3, 4), SackChunk::GapAckBlock(5, 6)}, + {}); + + EXPECT_FALSE(ChunkValidators::Validate(sack)); + + SackChunk clean = ChunkValidators::Clean(std::move(sack)); + + EXPECT_THAT(clean.gap_ack_blocks(), + ElementsAre(SackChunk::GapAckBlock(3, 6))); +} + +TEST(ChunkValidatorsTest, MergesOverlappingByOne) { + SackChunk sack(TSN(123), /*a_rwnd=*/456, + {SackChunk::GapAckBlock(3, 4), SackChunk::GapAckBlock(4, 5)}, + {}); + + SackChunk clean = ChunkValidators::Clean(std::move(sack)); + + EXPECT_FALSE(ChunkValidators::Validate(sack)); + + EXPECT_THAT(clean.gap_ack_blocks(), + ElementsAre(SackChunk::GapAckBlock(3, 5))); +} + +TEST(ChunkValidatorsTest, MergesOverlappingByMore) { + SackChunk sack(TSN(123), /*a_rwnd=*/456, + {SackChunk::GapAckBlock(3, 10), SackChunk::GapAckBlock(4, 5)}, + {}); + + EXPECT_FALSE(ChunkValidators::Validate(sack)); + + SackChunk clean = ChunkValidators::Clean(std::move(sack)); + + EXPECT_THAT(clean.gap_ack_blocks(), + ElementsAre(SackChunk::GapAckBlock(3, 10))); +} + +TEST(ChunkValidatorsTest, MergesBlocksStartingWithSameStartOffset) { + SackChunk sack(TSN(123), /*a_rwnd=*/456, + {SackChunk::GapAckBlock(3, 7), SackChunk::GapAckBlock(3, 5), + SackChunk::GapAckBlock(3, 9)}, + {}); + + EXPECT_FALSE(ChunkValidators::Validate(sack)); + + SackChunk clean = ChunkValidators::Clean(std::move(sack)); + + EXPECT_THAT(clean.gap_ack_blocks(), + ElementsAre(SackChunk::GapAckBlock(3, 9))); +} + +TEST(ChunkValidatorsTest, MergesBlocksPartiallyOverlapping) { + SackChunk sack(TSN(123), /*a_rwnd=*/456, + {SackChunk::GapAckBlock(3, 7), SackChunk::GapAckBlock(5, 9)}, + {}); + + EXPECT_FALSE(ChunkValidators::Validate(sack)); + + SackChunk clean = ChunkValidators::Clean(std::move(sack)); + + EXPECT_THAT(clean.gap_ack_blocks(), + ElementsAre(SackChunk::GapAckBlock(3, 9))); +} + +} // namespace +} // namespace dcsctp