Close connection on incorrect order of HEADERS and DATA frames. While the HTTP/3 spec draft does not require this yet, this is the sane thing to do, and my proposal to change the spec has some support: https://github.com/quicwg/base-drafts/issues/2858. This fixes fuzzer-found bug https://crbug.com/978733. NO_BUG=Bug is https://crbug.com/978733 on Chromium bugtracker. gfe-relnote: n/a, change in QUIC v99-only code. PiperOrigin-RevId: 257169271 Change-Id: I10ed66e671f0dc03d6f24694d4c53ba3691e9c51
diff --git a/quic/core/http/end_to_end_test.cc b/quic/core/http/end_to_end_test.cc index 96e5aef..1f8d6cf 100644 --- a/quic/core/http/end_to_end_test.cc +++ b/quic/core/http/end_to_end_test.cc
@@ -173,6 +173,14 @@ return params; } +void WriteHeadersOnStream(QuicSpdyStream* stream) { + // Since QuicSpdyStream uses QuicHeaderList::empty() to detect too large + // headers, it also fails when receiving empty headers. + SpdyHeaderBlock headers; + headers["foo"] = "bar"; + stream->WriteHeaders(std::move(headers), /* fin = */ false, nullptr); +} + class ServerDelegate : public PacketDroppingTestWriter::Delegate { public: explicit ServerDelegate(QuicDispatcher* dispatcher) @@ -1960,6 +1968,7 @@ // Open a data stream to make sure the stream level flow control is updated. QuicSpdyClientStream* stream = client_->GetOrCreateStream(); + WriteHeadersOnStream(stream); stream->WriteOrBufferBody("hello", false); // Client should have the right values for server's receive window. @@ -2016,6 +2025,7 @@ // Open a data stream to make sure the stream level flow control is updated. QuicSpdyClientStream* stream = client_->GetOrCreateStream(); + WriteHeadersOnStream(stream); stream->WriteOrBufferBody("hello", false); // Client should have the right values for server's receive window. @@ -3564,6 +3574,7 @@ // Set a TTL which expires immediately. stream->MaybeSetTtl(QuicTime::Delta::FromMicroseconds(1)); + WriteHeadersOnStream(stream); // 1 MB body. std::string body(1024 * 1024, 'a'); stream->WriteOrBufferBody(body, true);
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc index c625410..96be207 100644 --- a/quic/core/http/quic_spdy_stream.cc +++ b/quic/core/http/quic_spdy_stream.cc
@@ -760,6 +760,14 @@ bool QuicSpdyStream::OnDataFrameStart(Http3FrameLengths frame_lengths) { DCHECK( VersionHasDataFrameHeader(session()->connection()->transport_version())); + if (!headers_decompressed_ || trailers_decompressed_) { + // TODO(b/124216424): Change error code to HTTP_UNEXPECTED_FRAME. + session()->connection()->CloseConnection( + QUIC_INVALID_HEADERS_STREAM_DATA, "Unexpected DATA frame received.", + ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); + return false; + } + body_buffer_.OnDataHeader(frame_lengths); return true; } @@ -841,6 +849,15 @@ DCHECK(VersionUsesQpack(spdy_session_->connection()->transport_version())); DCHECK(!qpack_decoded_headers_accumulator_); + if (trailers_decompressed_) { + // TODO(b/124216424): Change error code to HTTP_UNEXPECTED_FRAME. + session()->connection()->CloseConnection( + QUIC_INVALID_HEADERS_STREAM_DATA, + "HEADERS frame received after trailing HEADERS.", + ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); + return false; + } + if (headers_decompressed_) { trailers_length_ = frame_length; } else {
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc index ff81ebf..b2fbc70 100644 --- a/quic/core/http/quic_spdy_stream_test.cc +++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -37,6 +37,7 @@ using testing::AtLeast; using testing::ElementsAre; using testing::Invoke; +using testing::InvokeWithoutArgs; using testing::Pair; using testing::Return; using testing::StrictMock; @@ -2122,6 +2123,133 @@ stream_->OnStreamFrame(frame); } +// Close connection if a DATA frame is received before a HEADERS frame. +TEST_P(QuicSpdyStreamTest, DataBeforeHeaders) { + if (!VersionUsesQpack(GetParam().transport_version)) { + return; + } + + Initialize(kShouldProcessData); + + // Closing the connection is mocked out in tests. Instead, simply stop + // reading data at the stream level to prevent QuicSpdyStream from blowing up. + // TODO(b/124216424): Change error code to HTTP_UNEXPECTED_FRAME. + EXPECT_CALL( + *connection_, + CloseConnection(QUIC_INVALID_HEADERS_STREAM_DATA, + "Unexpected DATA frame received.", + ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET)) + .WillOnce(InvokeWithoutArgs([this]() { stream_->StopReading(); })); + + std::string data = DataFrame(kDataFramePayload); + stream_->OnStreamFrame(QuicStreamFrame(stream_->id(), false, 0, data)); +} + +// Close connection if a HEADERS frame is received after the trailing HEADERS. +TEST_P(QuicSpdyStreamTest, TrailersAfterTrailers) { + if (!VersionUsesQpack(GetParam().transport_version)) { + return; + } + + Initialize(kShouldProcessData); + + // Receive and consume headers, with single header field "foo: bar". + std::string headers = + HeadersFrame(QuicTextUtils::HexDecode("00002a94e703626172")); + QuicStreamOffset offset = 0; + stream_->OnStreamFrame( + QuicStreamFrame(stream_->id(), false, offset, headers)); + offset += headers.size(); + + EXPECT_THAT(stream_->header_list(), ElementsAre(Pair("foo", "bar"))); + stream_->ConsumeHeaderList(); + + // Receive data. It is consumed by TestStream. + std::string data = DataFrame(kDataFramePayload); + stream_->OnStreamFrame(QuicStreamFrame(stream_->id(), false, offset, data)); + offset += data.size(); + + EXPECT_EQ(kDataFramePayload, stream_->data()); + + // Receive and consume trailers, with single header field + // "custom-key: custom-value". + std::string trailers1 = HeadersFrame( + QuicTextUtils::HexDecode("00002f0125a849e95ba97d7f8925a849e95bb8e8b4bf")); + stream_->OnStreamFrame( + QuicStreamFrame(stream_->id(), false, offset, trailers1)); + offset += trailers1.size(); + + EXPECT_TRUE(stream_->trailers_decompressed()); + EXPECT_THAT(stream_->received_trailers(), + ElementsAre(Pair("custom-key", "custom-value"))); + + // Closing the connection is mocked out in tests. Instead, simply stop + // reading data at the stream level to prevent QuicSpdyStream from blowing up. + // TODO(b/124216424): Change error code to HTTP_UNEXPECTED_FRAME. + EXPECT_CALL( + *connection_, + CloseConnection(QUIC_INVALID_HEADERS_STREAM_DATA, + "HEADERS frame received after trailing HEADERS.", + ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET)) + .WillOnce(InvokeWithoutArgs([this]() { stream_->StopReading(); })); + + // Receive another HEADERS frame, with no header fields. + std::string trailers2 = HeadersFrame(QuicTextUtils::HexDecode("0000")); + stream_->OnStreamFrame( + QuicStreamFrame(stream_->id(), false, offset, trailers2)); +} + +// Regression test for https://crbug.com/978733. +// Close connection if a DATA frame is received after the trailing HEADERS. +TEST_P(QuicSpdyStreamTest, DataAfterTrailers) { + if (!VersionUsesQpack(GetParam().transport_version)) { + return; + } + + Initialize(kShouldProcessData); + + // Receive and consume headers, with single header field "foo: bar". + std::string headers = + HeadersFrame(QuicTextUtils::HexDecode("00002a94e703626172")); + QuicStreamOffset offset = 0; + stream_->OnStreamFrame( + QuicStreamFrame(stream_->id(), false, offset, headers)); + offset += headers.size(); + + EXPECT_THAT(stream_->header_list(), ElementsAre(Pair("foo", "bar"))); + stream_->ConsumeHeaderList(); + + // Receive data. It is consumed by TestStream. + std::string data1 = DataFrame(kDataFramePayload); + stream_->OnStreamFrame(QuicStreamFrame(stream_->id(), false, offset, data1)); + offset += data1.size(); + EXPECT_EQ(kDataFramePayload, stream_->data()); + + // Receive trailers, with single header field "custom-key: custom-value". + std::string trailers = HeadersFrame( + QuicTextUtils::HexDecode("00002f0125a849e95ba97d7f8925a849e95bb8e8b4bf")); + stream_->OnStreamFrame( + QuicStreamFrame(stream_->id(), false, offset, trailers)); + offset += trailers.size(); + + EXPECT_THAT(stream_->received_trailers(), + ElementsAre(Pair("custom-key", "custom-value"))); + + // Closing the connection is mocked out in tests. Instead, simply stop + // reading data at the stream level to prevent QuicSpdyStream from blowing up. + // TODO(b/124216424): Change error code to HTTP_UNEXPECTED_FRAME. + EXPECT_CALL( + *connection_, + CloseConnection(QUIC_INVALID_HEADERS_STREAM_DATA, + "Unexpected DATA frame received.", + ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET)) + .WillOnce(InvokeWithoutArgs([this]() { stream_->StopReading(); })); + + // Receive more data. + std::string data2 = DataFrame("This payload should not be proccessed."); + stream_->OnStreamFrame(QuicStreamFrame(stream_->id(), false, offset, data2)); +} + } // namespace } // namespace test } // namespace quic