Unhide oghttp2 RFC 9113 Section 8.1 trailer validation logic from open source. PiperOrigin-RevId: 976388482
diff --git a/quiche/http2/adapter/oghttp2_session.cc b/quiche/http2/adapter/oghttp2_session.cc index 8d3b95f..ad2f348 100644 --- a/quiche/http2/adapter/oghttp2_session.cc +++ b/quiche/http2/adapter/oghttp2_session.cc
@@ -318,6 +318,26 @@ SetResult(OnHeaderResult::HEADER_HTTP_MESSAGING); return; } + // RFC 9113 Section 8.1: + // "Trailer fields are carried in a field block that also terminates the + // stream. That is, trailer fields comprise a sequence starting with a HEADERS + // frame, followed by zero or more CONTINUATION frames, where the HEADERS + // frame bears an END_STREAM flag. Trailers MUST NOT include pseudo-header + // fields (Section 8.3). An endpoint that receives pseudo-header fields in + // trailers MUST treat the request or response as malformed (Section 8.1.1). + // + // An endpoint that receives a HEADERS frame without the END_STREAM flag set + // after receiving the HEADERS frame that opens a request or after receiving a + // final (non-informational) status code MUST treat the corresponding request + // or response as malformed (Section 8.1.1)." + if ((type_ == HeaderType::REQUEST_TRAILER || + type_ == HeaderType::RESPONSE_TRAILER) && + !frame_contains_fin_) { + QUICHE_CODE_COUNT(http2_trailers_no_end_stream); + QUICHE_VLOG(1) << "Trailers must contain END_STREAM"; + SetResult(OnHeaderResult::HEADER_HTTP_MESSAGING); + return; + } if (frame_contains_fin_ && IsResponse(type_) && StatusIs1xx(status_header())) { QUICHE_VLOG(1) << "Unexpected end of stream without final headers";
diff --git a/quiche/http2/adapter/oghttp2_session_test.cc b/quiche/http2/adapter/oghttp2_session_test.cc index d6926f1..bd7798f 100644 --- a/quiche/http2/adapter/oghttp2_session_test.cc +++ b/quiche/http2/adapter/oghttp2_session_test.cc
@@ -1690,6 +1690,168 @@ EXPECT_EQ(settings_frame.size(), result); } +TEST(OgHttp2SessionTest, ClientReceivesTrailersWithoutEndStream) { + TestVisitor visitor; + OgHttp2Session::Options options; + options.perspective = Perspective::kClient; + OgHttp2Session session(visitor, options); + + // Send client preface. + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); + int send_result = session.Send(); + EXPECT_EQ(0, send_result); + visitor.Clear(); + + // Submit a request. + int stream_id = + session.SubmitRequest(ToHeaders({{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/"}}), + true, nullptr); + ASSERT_EQ(stream_id, 1); + + // Send the request HEADERS. + // 0x5 is END_STREAM | END_HEADERS. + EXPECT_CALL(visitor, OnBeforeFrameSent(HEADERS, stream_id, _, 0x5)); + EXPECT_CALL(visitor, OnFrameSent(HEADERS, stream_id, _, 0x5, 0)); + send_result = session.Send(); + EXPECT_EQ(0, send_result); + visitor.Clear(); + + // Server sends preface, response headers, and then invalid trailers. + const std::string frames = TestFrameSequence() + .ServerPreface() + .Headers(1, {{":status", "200"}}, + /*fin=*/false) + // Trailers without END_STREAM (fin=false) + .Headers(1, {{"final-status", "a-ok"}}, + /*fin=*/false) + .Serialize(); + + testing::InSequence s; + + // Server preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + + // Response headers (4 is END_HEADERS) + EXPECT_CALL(visitor, OnFrameHeader(stream_id, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(stream_id)); + EXPECT_CALL(visitor, OnHeaderForStream(stream_id, ":status", "200")); + EXPECT_CALL(visitor, OnEndHeadersForStream(stream_id)); + + // Trailers (4 is END_HEADERS) + EXPECT_CALL(visitor, OnFrameHeader(stream_id, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(stream_id)); + EXPECT_CALL(visitor, OnHeaderForStream(stream_id, "final-status", "a-ok")); + + // The fix should trigger OnInvalidFrame because of the missing END_STREAM. + EXPECT_CALL( + visitor, + OnInvalidFrame(stream_id, + Http2VisitorInterface::InvalidFrameError::kHttpMessaging)); + + const int64_t result = session.ProcessBytes(frames); + EXPECT_EQ(frames.size(), static_cast<size_t>(result)); + + // Session should want to write SETTINGS ACK and RST_STREAM. + EXPECT_TRUE(session.want_write()); + + // We expect SETTINGS ACK. + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + + // We expect RST_STREAM due to the invalid trailers. + EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, stream_id, _, 0x0)); + EXPECT_CALL(visitor, + OnFrameSent(RST_STREAM, stream_id, _, 0x0, + static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); + EXPECT_CALL(visitor, + OnCloseStream(stream_id, Http2ErrorCode::HTTP2_NO_ERROR)); + + send_result = session.Send(); + EXPECT_EQ(0, send_result); +} + +// Regression test for receiving a second HEADERS frame without +// END_STREAM on an existing stream on server side. +// In current implementation, it is misinterpreted by +// OgHttp2Session::OnHeaders as an attempt to open a new stream. Since the +// stream ID has already been processed, it immediately triggers a +// ConnectionError::kInvalidNewStreamId (connection error) before our +// validation logic in OnHeaderBlockEnd can run. This still safely prevents +// the UAF/crash, albeit via a connection teardown instead of a stream reset. +TEST(OgHttp2SessionTest, ServerReceivesTrailersWithoutEndStream) { + TestVisitor visitor; + OgHttp2Session::Options options; + options.perspective = Perspective::kServer; + OgHttp2Session session(visitor, options); + + // Send server preface (initial settings). + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); + int send_result = session.Send(); + EXPECT_EQ(0, send_result); + visitor.Clear(); + + // Client sends preface, request headers, and then invalid trailers. + const std::string frames = TestFrameSequence() + .ClientPreface() + .Headers(1, + {{":method", "POST"}, + {":scheme", "https"}, + {":authority", "example.com"}, + {":path", "/"}}, + /*fin=*/false) + // Trailers without END_STREAM (fin=false) + .Headers(1, {{"customer-header", "value"}}, + /*fin=*/false) + .Serialize(); + + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + + // Request headers (4 is END_HEADERS) + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":method", "POST")); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":scheme", "https")); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":authority", "example.com")); + EXPECT_CALL(visitor, OnHeaderForStream(1, ":path", "/")); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + + // Trailers (4 is END_HEADERS) + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); + + // It fails immediately in OnHeaders because fin is false on an existing + // stream, which is interpreted as a new stream with an invalid (already used) + // ID. + EXPECT_CALL(visitor, + OnConnectionError( + Http2VisitorInterface::ConnectionError::kInvalidNewStreamId)); + + const int64_t result = session.ProcessBytes(frames); + EXPECT_GT(result, 0); + + // Session should want to write GOAWAY. + EXPECT_TRUE(session.want_write()); + + // We expect GOAWAY. + EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0)); + EXPECT_CALL(visitor, + OnFrameSent(GOAWAY, 0, _, 0x0, 1)); // 1 is PROTOCOL_ERROR + + send_result = session.Send(); + EXPECT_EQ(0, send_result); +} + } // namespace test } // namespace adapter } // namespace http2