Reject forbidden frame types and incomplete frames in ALPS.
https://vasilvv.github.io/httpbis-alps/draft-vvv-httpbis-alps.html#name-use-of-alps-in-http-3
PiperOrigin-RevId: 359360218
Change-Id: Ice2650cb934ba776f38ab9465c5d4d3e3294fdb6
diff --git a/quic/core/http/http_decoder.h b/quic/core/http/http_decoder.h
index 65e9f2e..fe955c3 100644
--- a/quic/core/http/http_decoder.h
+++ b/quic/core/http/http_decoder.h
@@ -150,6 +150,9 @@
const std::string& error_detail() const { return error_detail_; }
+ // Returns true if input data processed so far ends on a frame boundary.
+ bool AtFrameBoundary() const { return state_ == STATE_READING_FRAME_TYPE; }
+
private:
friend test::HttpDecoderPeer;
diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc
index 94e2bfc..91530d3 100644
--- a/quic/core/http/quic_spdy_session.cc
+++ b/quic/core/http/quic_spdy_session.cc
@@ -98,51 +98,74 @@
// HttpDecoder::Visitor implementation.
void OnError(HttpDecoder* /*decoder*/) override {}
bool OnCancelPushFrame(const CancelPushFrame& /*frame*/) override {
- return true;
+ error_detail_ = "CANCEL_PUSH frame forbidden";
+ return false;
}
bool OnMaxPushIdFrame(const MaxPushIdFrame& /*frame*/) override {
- return true;
+ error_detail_ = "MAX_PUSH_ID frame forbidden";
+ return false;
}
- bool OnGoAwayFrame(const GoAwayFrame& /*frame*/) override { return true; }
+ bool OnGoAwayFrame(const GoAwayFrame& /*frame*/) override {
+ error_detail_ = "GOAWAY frame forbidden";
+ return false;
+ }
bool OnSettingsFrameStart(QuicByteCount /*header_length*/) override {
return true;
}
bool OnSettingsFrame(const SettingsFrame& /*frame*/) override { return true; }
bool OnDataFrameStart(QuicByteCount /*header_length*/, QuicByteCount
/*payload_length*/) override {
- return true;
+ error_detail_ = "DATA frame forbidden";
+ return false;
}
bool OnDataFramePayload(absl::string_view /*payload*/) override {
- return true;
+ QUICHE_NOTREACHED();
+ return false;
}
- bool OnDataFrameEnd() override { return true; }
+ bool OnDataFrameEnd() override {
+ QUICHE_NOTREACHED();
+ return false;
+ }
bool OnHeadersFrameStart(QuicByteCount /*header_length*/,
QuicByteCount /*payload_length*/) override {
- return true;
+ error_detail_ = "HEADERS frame forbidden";
+ return false;
}
bool OnHeadersFramePayload(absl::string_view /*payload*/) override {
- return true;
+ QUICHE_NOTREACHED();
+ return false;
}
- bool OnHeadersFrameEnd() override { return true; }
+ bool OnHeadersFrameEnd() override {
+ QUICHE_NOTREACHED();
+ return false;
+ }
bool OnPushPromiseFrameStart(QuicByteCount /*header_length*/) override {
- return true;
+ error_detail_ = "PUSH_PROMISE frame forbidden";
+ return false;
}
bool OnPushPromiseFramePushId(
PushId /*push_id*/,
QuicByteCount
/*push_id_length*/,
QuicByteCount /*header_block_length*/) override {
- return true;
+ QUICHE_NOTREACHED();
+ return false;
}
bool OnPushPromiseFramePayload(absl::string_view /*payload*/) override {
- return true;
+ QUICHE_NOTREACHED();
+ return false;
}
- bool OnPushPromiseFrameEnd() override { return true; }
+ bool OnPushPromiseFrameEnd() override {
+ QUICHE_NOTREACHED();
+ return false;
+ }
bool OnPriorityUpdateFrameStart(QuicByteCount /*header_length*/) override {
- return true;
+ error_detail_ = "PRIORITY_UPDATE frame forbidden";
+ return false;
}
bool OnPriorityUpdateFrame(const PriorityUpdateFrame& /*frame*/) override {
- return true;
+ QUICHE_NOTREACHED();
+ return false;
}
bool OnAcceptChFrameStart(QuicByteCount /*header_length*/) override {
return true;
@@ -162,8 +185,13 @@
}
bool OnUnknownFrameEnd() override { return true; }
+ const absl::optional<std::string>& error_detail() const {
+ return error_detail_;
+ }
+
private:
QuicSpdySession* const session_;
+ absl::optional<std::string> error_detail_;
};
} // namespace
@@ -1053,10 +1081,18 @@
AlpsFrameDecoder alps_frame_decoder(this);
HttpDecoder decoder(&alps_frame_decoder);
decoder.ProcessInput(reinterpret_cast<const char*>(alps_data), alps_length);
+ if (alps_frame_decoder.error_detail()) {
+ return alps_frame_decoder.error_detail();
+ }
+
if (decoder.error() != QUIC_NO_ERROR) {
return decoder.error_detail();
}
+ if (!decoder.AtFrameBoundary()) {
+ return "incomplete HTTP/3 frame";
+ }
+
return absl::nullopt;
}
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc
index db263b0..c2ab731 100644
--- a/quic/core/http/quic_spdy_session_test.cc
+++ b/quic/core/http/quic_spdy_session_test.cc
@@ -3314,11 +3314,14 @@
session_.OnStreamFrame(data3);
}
-TEST_P(QuicSpdySessionTestClient, OnAlpsData) {
+TEST_P(QuicSpdySessionTestClient, AcceptChViaAlps) {
+ if (!VersionUsesHttp3(transport_version())) {
+ return;
+ }
+
StrictMock<MockHttp3DebugVisitor> debug_visitor;
session_.set_debug_visitor(&debug_visitor);
- AcceptChFrame accept_ch_frame{{{"foo", "bar"}}};
std::string serialized_accept_ch_frame = absl::HexStringToBytes(
"4089" // type (ACCEPT_CH)
"08" // length
@@ -3328,12 +3331,48 @@
"626172"); // value "bar"
if (GetQuicReloadableFlag(quic_parse_accept_ch_frame)) {
- EXPECT_CALL(debug_visitor, OnAcceptChFrameReceivedViaAlps(accept_ch_frame));
+ AcceptChFrame expected_accept_ch_frame{{{"foo", "bar"}}};
+ EXPECT_CALL(debug_visitor,
+ OnAcceptChFrameReceivedViaAlps(expected_accept_ch_frame));
}
- session_.OnAlpsData(
+ auto error = session_.OnAlpsData(
reinterpret_cast<const uint8_t*>(serialized_accept_ch_frame.data()),
serialized_accept_ch_frame.size());
+ EXPECT_FALSE(error);
+}
+
+TEST_P(QuicSpdySessionTestClient, AlpsForbiddenFrame) {
+ if (!VersionUsesHttp3(transport_version())) {
+ return;
+ }
+
+ std::string forbidden_frame = absl::HexStringToBytes(
+ "00" // type (DATA)
+ "03" // length
+ "66666f"); // "foo"
+
+ auto error = session_.OnAlpsData(
+ reinterpret_cast<const uint8_t*>(forbidden_frame.data()),
+ forbidden_frame.size());
+ ASSERT_TRUE(error);
+ EXPECT_EQ("DATA frame forbidden", error.value());
+}
+
+TEST_P(QuicSpdySessionTestClient, AlpsIncompleteFrame) {
+ if (!VersionUsesHttp3(transport_version())) {
+ return;
+ }
+
+ std::string incomplete_frame = absl::HexStringToBytes(
+ "04" // type (SETTINGS)
+ "03"); // non-zero length but empty payload
+
+ auto error = session_.OnAlpsData(
+ reinterpret_cast<const uint8_t*>(incomplete_frame.data()),
+ incomplete_frame.size());
+ ASSERT_TRUE(error);
+ EXPECT_EQ("incomplete HTTP/3 frame", error.value());
}
} // namespace
