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
