Signal error in HttpDecoder on empty PUSH_PROMISE frame.
Currently on an empty, invalid PUSH_PROMISE frame HttpDecoder transitions from
STATE_READING_FRAME_LENGTH directly to STATE_FINISH_PARSING, skipping
STATE_READING_FRAME_PAYLOAD, which results in calling
Visitor::OnPushPromiseFrameEnd() without calling
Visitor::OnPushPromiseFrameStart(). This is wrong and can cause QuicSpdyStream
to crash.
This was caught by ClusterFuzz at https://crbug.com/1001823.
Also add tests for other empty frames, and sanity DCHECKs in QuicSpdyStream.
gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled
gfe2_reloadable_flag_quic_enable_version_99.
PiperOrigin-RevId: 270386637
Change-Id: I0c1944d1df300136d27367679e3128dd45e9bfd3
diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc
index c9519fe..e1fa36e 100644
--- a/quic/core/http/http_decoder.cc
+++ b/quic/core/http/http_decoder.cc
@@ -153,6 +153,10 @@
continue_processing = visitor_->OnSettingsFrameStart(header_length);
break;
case static_cast<uint64_t>(HttpFrameType::PUSH_PROMISE):
+ if (current_frame_length_ == 0) {
+ RaiseError(QUIC_INVALID_FRAME_DATA, "Corrupt PUSH_PROMISE frame.");
+ return false;
+ }
break;
case static_cast<uint64_t>(HttpFrameType::GOAWAY):
break;
diff --git a/quic/core/http/http_decoder_test.cc b/quic/core/http/http_decoder_test.cc
index 6847ecb..3abc8be 100644
--- a/quic/core/http/http_decoder_test.cc
+++ b/quic/core/http/http_decoder_test.cc
@@ -919,7 +919,78 @@
EXPECT_EQ(test_data.error_message, decoder.error_detail());
}
}
-} // namespace test
+}
+
+TEST_F(HttpDecoderTest, EmptyCancelPushFrame) {
+ std::string input = QuicTextUtils::HexDecode(
+ "03" // type (CANCEL_PUSH)
+ "00"); // frame length
+
+ EXPECT_CALL(visitor_, OnError(&decoder_));
+ EXPECT_EQ(input.size(), ProcessInput(input));
+ EXPECT_EQ(QUIC_INVALID_FRAME_DATA, decoder_.error());
+ EXPECT_EQ("Unable to read push_id", decoder_.error_detail());
+}
+
+TEST_F(HttpDecoderTest, EmptySettingsFrame) {
+ std::string input = QuicTextUtils::HexDecode(
+ "04" // type (SETTINGS)
+ "00"); // frame length
+
+ EXPECT_CALL(visitor_, OnSettingsFrameStart(2));
+
+ SettingsFrame empty_frame;
+ EXPECT_CALL(visitor_, OnSettingsFrame(empty_frame));
+
+ EXPECT_EQ(input.size(), ProcessInput(input));
+ EXPECT_EQ(QUIC_NO_ERROR, decoder_.error());
+ EXPECT_EQ("", decoder_.error_detail());
+}
+
+// Regression test for https://crbug.com/1001823.
+TEST_F(HttpDecoderTest, EmptyPushPromiseFrame) {
+ std::string input = QuicTextUtils::HexDecode(
+ "05" // type (PUSH_PROMISE)
+ "00"); // frame length
+
+ EXPECT_CALL(visitor_, OnError(&decoder_));
+ EXPECT_EQ(input.size(), ProcessInput(input));
+ EXPECT_EQ(QUIC_INVALID_FRAME_DATA, decoder_.error());
+ EXPECT_EQ("Corrupt PUSH_PROMISE frame.", decoder_.error_detail());
+}
+
+TEST_F(HttpDecoderTest, EmptyGoAwayFrame) {
+ std::string input = QuicTextUtils::HexDecode(
+ "07" // type (GOAWAY)
+ "00"); // frame length
+
+ EXPECT_CALL(visitor_, OnError(&decoder_));
+ EXPECT_EQ(input.size(), ProcessInput(input));
+ EXPECT_EQ(QUIC_INVALID_FRAME_DATA, decoder_.error());
+ EXPECT_EQ("Unable to read GOAWAY stream_id", decoder_.error_detail());
+}
+
+TEST_F(HttpDecoderTest, EmptyMaxPushIdFrame) {
+ std::string input = QuicTextUtils::HexDecode(
+ "0d" // type (MAX_PUSH_ID)
+ "00"); // frame length
+
+ EXPECT_CALL(visitor_, OnError(&decoder_));
+ EXPECT_EQ(input.size(), ProcessInput(input));
+ EXPECT_EQ(QUIC_INVALID_FRAME_DATA, decoder_.error());
+ EXPECT_EQ("Unable to read push_id", decoder_.error_detail());
+}
+
+TEST_F(HttpDecoderTest, EmptyDuplicatePushFrame) {
+ std::string input = QuicTextUtils::HexDecode(
+ "0e" // type (DUPLICATE_PUSH)
+ "00"); // frame length
+
+ EXPECT_CALL(visitor_, OnError(&decoder_));
+ EXPECT_EQ(input.size(), ProcessInput(input));
+ EXPECT_EQ(QUIC_INVALID_FRAME_DATA, decoder_.error());
+ EXPECT_EQ("Unable to read push_id", decoder_.error_detail());
+}
} // namespace test
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc
index a407d5c..a089345 100644
--- a/quic/core/http/quic_spdy_stream.cc
+++ b/quic/core/http/quic_spdy_stream.cc
@@ -127,7 +127,7 @@
bool OnPushPromiseFrameStart(PushId push_id,
QuicByteCount header_length,
QuicByteCount push_id_length) override {
- if (!VersionHasStreamType(stream_->transport_version())) {
+ if (!VersionUsesQpack(stream_->transport_version())) {
CloseConnectionOnWrongFrame("Push Promise");
return false;
}
@@ -880,6 +880,7 @@
bool QuicSpdyStream::OnHeadersFramePayload(QuicStringPiece payload) {
DCHECK(VersionUsesQpack(transport_version()));
+ DCHECK(qpack_decoded_headers_accumulator_);
if (headers_decompressed_) {
trailers_payload_length_ += payload.length();
@@ -904,6 +905,7 @@
bool QuicSpdyStream::OnHeadersFrameEnd() {
DCHECK(VersionUsesQpack(transport_version()));
+ DCHECK(qpack_decoded_headers_accumulator_);
auto result = qpack_decoded_headers_accumulator_->EndHeaderBlock();
