From 8a5f659c66ebc53b522d7155f034588b676045e3 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 12 Jan 2024 11:25:53 -0800 Subject: [chaotic-good] Fix fuzzer bug (#35537) We were getting errors due to insane amounts of padding: enforce limits, fix b/319533934. Closes #35537 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35537 from ctiller:fff 9f5f31ef27b46631b78edbae1d9e445115fbb25f PiperOrigin-RevId: 597899598 --- .../chaotic_good/chaotic_good_transport.h | 7 ++-- .../ext/transport/chaotic_good/client_transport.cc | 3 +- src/core/ext/transport/chaotic_good/frame.cc | 40 ++++++++++++++++------ src/core/ext/transport/chaotic_good/frame.h | 22 ++++++++---- .../ext/transport/chaotic_good/frame_header.cc | 4 --- src/core/ext/transport/chaotic_good/frame_header.h | 2 +- .../ext/transport/chaotic_good/server_transport.cc | 6 ++-- test/core/transport/chaotic_good/frame_fuzzer.cc | 14 ++++---- .../frame_fuzzer_corpus/testcase-5162634043785216 | 2 ++ test/core/transport/chaotic_good/frame_test.cc | 4 ++- 10 files changed, 69 insertions(+), 35 deletions(-) create mode 100644 test/core/transport/chaotic_good/frame_fuzzer_corpus/testcase-5162634043785216 diff --git a/src/core/ext/transport/chaotic_good/chaotic_good_transport.h b/src/core/ext/transport/chaotic_good/chaotic_good_transport.h index 1096486bbe..5dd8355363 100644 --- a/src/core/ext/transport/chaotic_good/chaotic_good_transport.h +++ b/src/core/ext/transport/chaotic_good/chaotic_good_transport.h @@ -49,7 +49,7 @@ class ChaoticGoodTransport { // Resolves to StatusOr>. auto ReadFrameBytes() { return TrySeq( - control_endpoint_->ReadSlice(FrameHeader::frame_header_size_), + control_endpoint_->ReadSlice(FrameHeader::kFrameHeaderSize), [this](Slice read_buffer) { auto frame_header = FrameHeader::Parse(reinterpret_cast( @@ -88,9 +88,10 @@ class ChaoticGoodTransport { } absl::Status DeserializeFrame(FrameHeader header, BufferPair buffers, - Arena* arena, FrameInterface& frame) { + Arena* arena, FrameInterface& frame, + FrameLimits limits) { return frame.Deserialize(&parser_, header, bitgen_, arena, - std::move(buffers)); + std::move(buffers), limits); } // Skip a frame, but correctly handle any hpack state updates. diff --git a/src/core/ext/transport/chaotic_good/client_transport.cc b/src/core/ext/transport/chaotic_good/client_transport.cc index d4075bcad1..b0d5f6b3b2 100644 --- a/src/core/ext/transport/chaotic_good/client_transport.cc +++ b/src/core/ext/transport/chaotic_good/client_transport.cc @@ -135,7 +135,8 @@ auto ChaoticGoodClientTransport::TransportReadLoop() { absl::Status deserialize_status; if (call_handler.has_value()) { deserialize_status = transport_.DeserializeFrame( - frame_header, std::move(buffers), call_handler->arena(), frame); + frame_header, std::move(buffers), call_handler->arena(), frame, + FrameLimits{1024 * 1024 * 1024, aligned_bytes_ - 1}); } else { // Stream not found, skip the frame. transport_.SkipFrame(frame_header, std::move(buffers)); diff --git a/src/core/ext/transport/chaotic_good/frame.cc b/src/core/ext/transport/chaotic_good/frame.cc index d2c0b3a4fc..cff5cd4680 100644 --- a/src/core/ext/transport/chaotic_good/frame.cc +++ b/src/core/ext/transport/chaotic_good/frame.cc @@ -47,8 +47,8 @@ const uint8_t kZeros[64] = {}; namespace { const NoDestruct kZeroSlice{[] { // Frame header size is fixed to 24 bytes. - auto slice = GRPC_SLICE_MALLOC(FrameHeader::frame_header_size_); - memset(GRPC_SLICE_START_PTR(slice), 0, FrameHeader::frame_header_size_); + auto slice = GRPC_SLICE_MALLOC(FrameHeader::kFrameHeaderSize); + memset(GRPC_SLICE_START_PTR(slice), 0, FrameHeader::kFrameHeaderSize); return slice; }()}; @@ -81,7 +81,7 @@ class FrameSerializer { SliceBuffer& AddTrailers() { header_.flags.set(2); header_.header_length = - output_.control.Length() - FrameHeader::frame_header_size_; + output_.control.Length() - FrameHeader::kFrameHeaderSize; return output_.control; } @@ -91,13 +91,13 @@ class FrameSerializer { // Header length is already known in AddTrailers(). header_.trailer_length = output_.control.Length() - header_.header_length - - FrameHeader::frame_header_size_; + FrameHeader::kFrameHeaderSize; } else { if (header_.flags.is_set(0)) { // Calculate frame header length in Finish() since AddTrailers() isn't // called. header_.header_length = - output_.control.Length() - FrameHeader::frame_header_size_; + output_.control.Length() - FrameHeader::kFrameHeaderSize; } } header_.Serialize( @@ -175,9 +175,23 @@ absl::StatusOr> ReadMetadata( } } // namespace +absl::Status FrameLimits::ValidateMessage(const FrameHeader& header) { + if (header.message_length > max_message_size) { + return absl::InvalidArgumentError( + absl::StrCat("Message length ", header.message_length, + " exceeds maximum allowed ", max_message_size)); + } + if (header.message_padding > max_padding) { + return absl::InvalidArgumentError( + absl::StrCat("Message padding ", header.message_padding, + " exceeds maximum allowed ", max_padding)); + } + return absl::OkStatus(); +} + absl::Status SettingsFrame::Deserialize(HPackParser*, const FrameHeader& header, absl::BitGenRef, Arena*, - BufferPair buffers) { + BufferPair buffers, FrameLimits) { if (header.type != FrameType::kSettings) { return absl::InvalidArgumentError("Expected settings frame"); } @@ -201,8 +215,8 @@ std::string SettingsFrame::ToString() const { return "SettingsFrame{}"; } absl::Status ClientFragmentFrame::Deserialize(HPackParser* parser, const FrameHeader& header, absl::BitGenRef bitsrc, - Arena* arena, - BufferPair buffers) { + Arena* arena, BufferPair buffers, + FrameLimits limits) { if (header.stream_id == 0) { return absl::InvalidArgumentError("Expected non-zero stream id"); } @@ -224,6 +238,8 @@ absl::Status ClientFragmentFrame::Deserialize(HPackParser* parser, "Unexpected non-zero header length", header.header_length)); } if (header.flags.is_set(1)) { + auto r = limits.ValidateMessage(header); + if (!r.ok()) return r; message = FragmentMessage{Arena::MakePooled(std::move(buffers.data), 0), header.message_padding, header.message_length}; @@ -279,8 +295,8 @@ std::string ClientFragmentFrame::ToString() const { absl::Status ServerFragmentFrame::Deserialize(HPackParser* parser, const FrameHeader& header, absl::BitGenRef bitsrc, - Arena* arena, - BufferPair buffers) { + Arena* arena, BufferPair buffers, + FrameLimits limits) { if (header.stream_id == 0) { return absl::InvalidArgumentError("Expected non-zero stream id"); } @@ -299,6 +315,8 @@ absl::Status ServerFragmentFrame::Deserialize(HPackParser* parser, "Unexpected non-zero header length", header.header_length)); } if (header.flags.is_set(1)) { + auto r = limits.ValidateMessage(header); + if (!r.ok()) return r; message.emplace(Arena::MakePooled(std::move(buffers.data), 0), header.message_padding, header.message_length); } else if (buffers.data.Length() != 0) { @@ -347,7 +365,7 @@ std::string ServerFragmentFrame::ToString() const { absl::Status CancelFrame::Deserialize(HPackParser*, const FrameHeader& header, absl::BitGenRef, Arena*, - BufferPair buffers) { + BufferPair buffers, FrameLimits) { if (header.type != FrameType::kCancel) { return absl::InvalidArgumentError("Expected cancel frame"); } diff --git a/src/core/ext/transport/chaotic_good/frame.h b/src/core/ext/transport/chaotic_good/frame.h index e7ccd6ee22..0246fa7de1 100644 --- a/src/core/ext/transport/chaotic_good/frame.h +++ b/src/core/ext/transport/chaotic_good/frame.h @@ -42,12 +42,19 @@ struct BufferPair { SliceBuffer data; }; +struct FrameLimits { + size_t max_message_size = 1024 * 1024 * 1024; + size_t max_padding = 63; + + absl::Status ValidateMessage(const FrameHeader& header); +}; + class FrameInterface { public: virtual absl::Status Deserialize(HPackParser* parser, const FrameHeader& header, absl::BitGenRef bitsrc, Arena* arena, - BufferPair buffers) = 0; + BufferPair buffers, FrameLimits limits) = 0; virtual BufferPair Serialize(HPackCompressor* encoder) const = 0; virtual std::string ToString() const = 0; @@ -68,7 +75,7 @@ class FrameInterface { struct SettingsFrame final : public FrameInterface { absl::Status Deserialize(HPackParser* parser, const FrameHeader& header, absl::BitGenRef bitsrc, Arena* arena, - BufferPair buffers) override; + BufferPair buffers, FrameLimits limits) override; BufferPair Serialize(HPackCompressor* encoder) const override; std::string ToString() const override; @@ -91,14 +98,17 @@ struct FragmentMessage { } bool operator==(const FragmentMessage& other) const { - return EqVal(*message, *other.message) && length == other.length; + if (length != other.length) return false; + if (message == nullptr && other.message == nullptr) return true; + if (message == nullptr || other.message == nullptr) return false; + return EqVal(*message, *other.message); } }; struct ClientFragmentFrame final : public FrameInterface { absl::Status Deserialize(HPackParser* parser, const FrameHeader& header, absl::BitGenRef bitsrc, Arena* arena, - BufferPair buffers) override; + BufferPair buffers, FrameLimits limits) override; BufferPair Serialize(HPackCompressor* encoder) const override; std::string ToString() const override; @@ -116,7 +126,7 @@ struct ClientFragmentFrame final : public FrameInterface { struct ServerFragmentFrame final : public FrameInterface { absl::Status Deserialize(HPackParser* parser, const FrameHeader& header, absl::BitGenRef bitsrc, Arena* arena, - BufferPair buffers) override; + BufferPair buffers, FrameLimits limits) override; BufferPair Serialize(HPackCompressor* encoder) const override; std::string ToString() const override; @@ -134,7 +144,7 @@ struct ServerFragmentFrame final : public FrameInterface { struct CancelFrame final : public FrameInterface { absl::Status Deserialize(HPackParser* parser, const FrameHeader& header, absl::BitGenRef bitsrc, Arena* arena, - BufferPair buffers) override; + BufferPair buffers, FrameLimits limits) override; BufferPair Serialize(HPackCompressor* encoder) const override; std::string ToString() const override; diff --git a/src/core/ext/transport/chaotic_good/frame_header.cc b/src/core/ext/transport/chaotic_good/frame_header.cc index 6b1ec01849..c565beec78 100644 --- a/src/core/ext/transport/chaotic_good/frame_header.cc +++ b/src/core/ext/transport/chaotic_good/frame_header.cc @@ -70,10 +70,6 @@ absl::StatusOr FrameHeader::Parse(const uint8_t* data) { } header.message_length = ReadLittleEndianUint32(data + 12); header.message_padding = ReadLittleEndianUint32(data + 16); - if (header.flags.is_set(1) && header.message_length <= 0) { - return absl::InvalidArgumentError( - absl::StrCat("Invalid message length: ", header.message_length)); - } header.trailer_length = ReadLittleEndianUint32(data + 20); return header; } diff --git a/src/core/ext/transport/chaotic_good/frame_header.h b/src/core/ext/transport/chaotic_good/frame_header.h index 773b44f26e..62f3088f58 100644 --- a/src/core/ext/transport/chaotic_good/frame_header.h +++ b/src/core/ext/transport/chaotic_good/frame_header.h @@ -60,7 +60,7 @@ struct FrameHeader { trailer_length == h.trailer_length; } // Frame header size is fixed to 24 bytes. - static constexpr size_t frame_header_size_ = 24; + static constexpr size_t kFrameHeaderSize = 24; }; } // namespace chaotic_good diff --git a/src/core/ext/transport/chaotic_good/server_transport.cc b/src/core/ext/transport/chaotic_good/server_transport.cc index 3d4387ac94..291031b7c6 100644 --- a/src/core/ext/transport/chaotic_good/server_transport.cc +++ b/src/core/ext/transport/chaotic_good/server_transport.cc @@ -165,7 +165,8 @@ auto ChaoticGoodServerTransport::DeserializeAndPushFragmentToNewCall( ClientFragmentFrame fragment_frame; ScopedArenaPtr arena(acceptor_->CreateArena()); absl::Status status = transport_.DeserializeFrame( - frame_header, std::move(buffers), arena.get(), fragment_frame); + frame_header, std::move(buffers), arena.get(), fragment_frame, + FrameLimits{1024 * 1024 * 1024, aligned_bytes_ - 1}); absl::optional call_initiator; if (status.ok()) { auto create_call_result = @@ -193,7 +194,8 @@ auto ChaoticGoodServerTransport::DeserializeAndPushFragmentToExistingCall( if (call_initiator.has_value()) arena = call_initiator->arena(); ClientFragmentFrame fragment_frame; absl::Status status = transport_.DeserializeFrame( - frame_header, std::move(buffers), arena, fragment_frame); + frame_header, std::move(buffers), arena, fragment_frame, + FrameLimits{1024 * 1024 * 1024, aligned_bytes_ - 1}); return MaybePushFragmentIntoCall(std::move(call_initiator), std::move(status), std::move(fragment_frame)); } diff --git a/test/core/transport/chaotic_good/frame_fuzzer.cc b/test/core/transport/chaotic_good/frame_fuzzer.cc index 0348156077..b5a09ab1d8 100644 --- a/test/core/transport/chaotic_good/frame_fuzzer.cc +++ b/test/core/transport/chaotic_good/frame_fuzzer.cc @@ -49,6 +49,8 @@ struct DeterministicBitGen : public std::numeric_limits { uint64_t operator()() { return 42; } }; +FrameLimits FuzzerFrameLimits() { return FrameLimits{1024 * 1024 * 1024, 63}; } + template void AssertRoundTrips(const T& input, FrameType expected_frame_type) { HPackCompressor hpack_compressor; @@ -69,9 +71,9 @@ void AssertRoundTrips(const T& input, FrameType expected_frame_type) { T output; HPackParser hpack_parser; DeterministicBitGen bitgen; - auto deser = - output.Deserialize(&hpack_parser, header.value(), absl::BitGenRef(bitgen), - GetContext(), std::move(serialized)); + auto deser = output.Deserialize(&hpack_parser, header.value(), + absl::BitGenRef(bitgen), GetContext(), + std::move(serialized), FuzzerFrameLimits()); GPR_ASSERT(deser.ok()); GPR_ASSERT(output == input); } @@ -82,9 +84,9 @@ void FinishParseAndChecks(const FrameHeader& header, BufferPair buffers) { ExecCtx exec_ctx; // Initialized to get this_cpu() info in global_stat(). HPackParser hpack_parser; DeterministicBitGen bitgen; - auto deser = - parsed.Deserialize(&hpack_parser, header, absl::BitGenRef(bitgen), - GetContext(), std::move(buffers)); + auto deser = parsed.Deserialize(&hpack_parser, header, + absl::BitGenRef(bitgen), GetContext(), + std::move(buffers), FuzzerFrameLimits()); if (!deser.ok()) return; gpr_log(GPR_INFO, "Read frame: %s", parsed.ToString().c_str()); AssertRoundTrips(parsed, header.type); diff --git a/test/core/transport/chaotic_good/frame_fuzzer_corpus/testcase-5162634043785216 b/test/core/transport/chaotic_good/frame_fuzzer_corpus/testcase-5162634043785216 new file mode 100644 index 0000000000..3652070e75 --- /dev/null +++ b/test/core/transport/chaotic_good/frame_fuzzer_corpus/testcase-5162634043785216 @@ -0,0 +1,2 @@ +control: "\200\002\000\000\ny\0022\000\000\000\000\033\000\000\000;E\\000\\ny]0" +data: "\200\002\000\000\ny\0022\000\000\000\000\033\000\000\000;E\\000\\ny]0" diff --git a/test/core/transport/chaotic_good/frame_test.cc b/test/core/transport/chaotic_good/frame_test.cc index 15153a09b8..e29df6536c 100644 --- a/test/core/transport/chaotic_good/frame_test.cc +++ b/test/core/transport/chaotic_good/frame_test.cc @@ -28,6 +28,8 @@ namespace grpc_core { namespace chaotic_good { namespace { +FrameLimits TestFrameLimits() { return FrameLimits{1024 * 1024 * 1024, 63}; } + template void AssertRoundTrips(const T& input, FrameType expected_frame_type) { HPackCompressor hpack_compressor; @@ -50,7 +52,7 @@ void AssertRoundTrips(const T& input, FrameType expected_frame_type) { ScopedArenaPtr arena = MakeScopedArena(1024, &allocator); auto deser = output.Deserialize(&hpack_parser, header.value(), absl::BitGenRef(bitgen), - arena.get(), std::move(serialized)); + arena.get(), std::move(serialized), TestFrameLimits()); GPR_ASSERT(deser.ok()); GPR_ASSERT(output == input); } -- cgit v1.2.3