Validate header field value characters after reading headers in QuicSpdyStream instead of as part of QPACK decoding. The new stream header validation would reset the stream (instead of closing connection), which is consistent with how other types of invalid headers are handled.
Protected by FLAGS_quic_reloadable_flag_quic_valid_header_field_value_at_spdy_stream.
PiperOrigin-RevId: 462481911
diff --git a/quiche/quic/core/http/quic_spdy_stream.cc b/quiche/quic/core/http/quic_spdy_stream.cc
index 69dd05f..ed8ff04 100644
--- a/quiche/quic/core/http/quic_spdy_stream.cc
+++ b/quiche/quic/core/http/quic_spdy_stream.cc
@@ -623,6 +623,12 @@
bool header_too_large = VersionUsesHttp3(transport_version())
? header_list_size_limit_exceeded_
: header_list.empty();
+ if (!AreHeaderFieldValuesValid(header_list)) {
+ OnInvalidHeaders();
+ QUIC_RELOADABLE_FLAG_COUNT_N(
+ quic_validate_header_field_value_at_spdy_stream, 2, 2);
+ return;
+ }
// Validate request headers if it did not exceed size limit. If it did,
// OnHeadersTooLarge() should have already handled it previously.
if (!header_too_large && !AreHeadersValid(header_list)) {
@@ -1571,6 +1577,33 @@
return true;
}
+bool QuicSpdyStream::AreHeaderFieldValuesValid(
+ const QuicHeaderList& header_list) const {
+ if (!GetQuicReloadableFlag(quic_validate_header_field_value_at_spdy_stream) ||
+ !VersionUsesHttp3(transport_version())) {
+ return true;
+ }
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_validate_header_field_value_at_spdy_stream,
+ 1, 2);
+ // According to https://www.rfc-editor.org/rfc/rfc9114.html#section-10.3
+ // "[...] HTTP/3 can transport field values that are not valid. While most
+ // values that can be encoded will not alter field parsing, carriage return
+ // (ASCII 0x0d), line feed (ASCII 0x0a), and the null character (ASCII 0x00)
+ // might be exploited by an attacker if they are translated verbatim. Any
+ // request or response that contains a character not permitted in a field
+ // value MUST be treated as malformed.
+ // [...]"
+ for (const std::pair<std::string, std::string>& pair : header_list) {
+ const std::string& value = pair.second;
+ for (const auto c : value) {
+ if (c == '\0' || c == '\n' || c == '\r') {
+ return false;
+ }
+ }
+ }
+ return true;
+}
+
void QuicSpdyStream::OnInvalidHeaders() { Reset(QUIC_BAD_APPLICATION_PAYLOAD); }
#undef ENDPOINT // undef for jumbo builds
diff --git a/quiche/quic/core/http/quic_spdy_stream.h b/quiche/quic/core/http/quic_spdy_stream.h
index ffc2e18..62b0eea 100644
--- a/quiche/quic/core/http/quic_spdy_stream.h
+++ b/quiche/quic/core/http/quic_spdy_stream.h
@@ -318,6 +318,10 @@
void OnWriteSideInDataRecvdState() override;
virtual bool AreHeadersValid(const QuicHeaderList& header_list) const;
+ // TODO(b/202433856) Merge AreHeaderFieldValueValid into AreHeadersValid once
+ // all flags guarding the behavior of AreHeadersValid has been rolled out.
+ virtual bool AreHeaderFieldValuesValid(
+ const QuicHeaderList& header_list) const;
// Reset stream upon invalid request headers.
virtual void OnInvalidHeaders();
diff --git a/quiche/quic/core/qpack/qpack_decoder_test.cc b/quiche/quic/core/qpack/qpack_decoder_test.cc
index 36bc6f9..ecf3a17 100644
--- a/quiche/quic/core/qpack/qpack_decoder_test.cc
+++ b/quiche/quic/core/qpack/qpack_decoder_test.cc
@@ -223,9 +223,14 @@
}
TEST_P(QpackDecoderTest, LineFeedInValue) {
- EXPECT_CALL(handler_,
- OnDecodingErrorDetected(QUIC_INVALID_CHARACTER_IN_FIELD_VALUE,
- "Invalid character in field value."));
+ if (!GetQuicReloadableFlag(quic_validate_header_field_value_at_spdy_stream)) {
+ EXPECT_CALL(handler_,
+ OnDecodingErrorDetected(QUIC_INVALID_CHARACTER_IN_FIELD_VALUE,
+ "Invalid character in field value."));
+ } else {
+ EXPECT_CALL(handler_, OnHeaderDecoded(Eq("foo"), Eq("ba\nr")));
+ EXPECT_CALL(handler_, OnDecodingCompleted());
+ }
DecodeHeaderBlock(absl::HexStringToBytes("000023666f6f0462610a72"));
}
diff --git a/quiche/quic/core/qpack/qpack_progressive_decoder.cc b/quiche/quic/core/qpack/qpack_progressive_decoder.cc
index 7258128..feea62a 100644
--- a/quiche/quic/core/qpack/qpack_progressive_decoder.cc
+++ b/quiche/quic/core/qpack/qpack_progressive_decoder.cc
@@ -342,21 +342,24 @@
bool QpackProgressiveDecoder::OnHeaderDecoded(bool value_from_static_table,
absl::string_view name,
absl::string_view value) {
- // Skip test for static table entries as they are all known to be valid.
- if (!value_from_static_table) {
- // According to Section 10.3 of
- // https://quicwg.org/base-drafts/draft-ietf-quic-http.html,
- // "[...] HTTP/3 can transport field values that are not valid. While most
- // values that can be encoded will not alter field parsing, carriage return
- // (CR, ASCII 0x0d), line feed (LF, ASCII 0x0a), and the zero character
- // (NUL, ASCII 0x00) might be exploited by an attacker if they are
- // translated verbatim. Any request or response that contains a character
- // not permitted in a field value MUST be treated as malformed [...]"
- for (const auto c : value) {
- if (c == '\0' || c == '\n' || c == '\r') {
- OnError(QUIC_INVALID_CHARACTER_IN_FIELD_VALUE,
- "Invalid character in field value.");
- return false;
+ if (!GetQuicReloadableFlag(quic_validate_header_field_value_at_spdy_stream)) {
+ // Skip test for static table entries as they are all known to be valid.
+ if (!value_from_static_table) {
+ // According to Section 10.3 of
+ // https://quicwg.org/base-drafts/draft-ietf-quic-http.html,
+ // "[...] HTTP/3 can transport field values that are not valid. While most
+ // values that can be encoded will not alter field parsing, carriage
+ // return (CR, ASCII 0x0d), line feed (LF, ASCII 0x0a), and the zero
+ // character (NUL, ASCII 0x00) might be exploited by an attacker if they
+ // are translated verbatim. Any request or response that contains a
+ // character not permitted in a field value MUST be treated as malformed
+ // [...]"
+ for (const auto c : value) {
+ if (c == '\0' || c == '\n' || c == '\r') {
+ OnError(QUIC_INVALID_CHARACTER_IN_FIELD_VALUE,
+ "Invalid character in field value.");
+ return false;
+ }
}
}
}
diff --git a/quiche/quic/core/quic_flags_list.h b/quiche/quic/core/quic_flags_list.h
index d867aeb..96b5862 100644
--- a/quiche/quic/core/quic_flags_list.h
+++ b/quiche/quic/core/quic_flags_list.h
@@ -97,6 +97,8 @@
QUIC_FLAG(quic_reloadable_flag_quic_connection_migration_use_new_cid_v2, true)
// If true, uses conservative cwnd gain and pacing gain when cwnd gets bootstrapped.
QUIC_FLAG(quic_reloadable_flag_quic_conservative_cwnd_and_pacing_gains, false)
+// If true, validate header field character at spdy stream instead of qpack for IETF QUIC.
+QUIC_FLAG(quic_reloadable_flag_quic_validate_header_field_value_at_spdy_stream, false)
// Store original QUIC connection IDs in the dispatcher\'s map
QUIC_FLAG(quic_restart_flag_quic_map_original_connection_ids, false)
// When the flag is true, exit STARTUP after the same number of loss events as PROBE_UP.
diff --git a/quiche/quic/tools/quic_simple_server_stream_test.cc b/quiche/quic/tools/quic_simple_server_stream_test.cc
index f9aef38..1aa0a7f 100644
--- a/quiche/quic/tools/quic_simple_server_stream_test.cc
+++ b/quiche/quic/tools/quic_simple_server_stream_test.cc
@@ -19,6 +19,7 @@
#include "quiche/quic/core/quic_types.h"
#include "quiche/quic/core/quic_utils.h"
#include "quiche/quic/platform/api/quic_expect_bug.h"
+#include "quiche/quic/platform/api/quic_flags.h"
#include "quiche/quic/platform/api/quic_socket_address.h"
#include "quiche/quic/platform/api/quic_test.h"
#include "quiche/quic/test_tools/crypto_test_utils.h"
@@ -643,6 +644,12 @@
// \000 is a way to write the null byte when followed by a literal digit.
header_list_.OnHeader("content-length", absl::string_view("11\00012", 5));
+ if (GetQuicReloadableFlag(quic_validate_header_field_value_at_spdy_stream) &&
+ session_.version().UsesHttp3()) {
+ EXPECT_CALL(session_,
+ MaybeSendStopSendingFrame(_, QuicResetStreamError::FromInternal(
+ QUIC_STREAM_NO_ERROR)));
+ }
EXPECT_CALL(*stream_, WriteHeadersMock(false));
EXPECT_CALL(session_, WritevData(_, _, _, _, _, _))
.WillRepeatedly(
@@ -659,6 +666,12 @@
// \000 is a way to write the null byte when followed by a literal digit.
header_list_.OnHeader("content-length", absl::string_view("\00012", 3));
+ if (GetQuicReloadableFlag(quic_validate_header_field_value_at_spdy_stream) &&
+ session_.version().UsesHttp3()) {
+ EXPECT_CALL(session_,
+ MaybeSendStopSendingFrame(_, QuicResetStreamError::FromInternal(
+ QUIC_STREAM_NO_ERROR)));
+ }
EXPECT_CALL(*stream_, WriteHeadersMock(false));
EXPECT_CALL(session_, WritevData(_, _, _, _, _, _))
.WillRepeatedly(
@@ -670,17 +683,35 @@
EXPECT_TRUE(stream_->write_side_closed());
}
-TEST_P(QuicSimpleServerStreamTest, ValidMultipleContentLength) {
+TEST_P(QuicSimpleServerStreamTest, InvalidMultipleContentLengthII) {
spdy::Http2HeaderBlock request_headers;
// \000 is a way to write the null byte when followed by a literal digit.
header_list_.OnHeader("content-length", absl::string_view("11\00011", 5));
+ if (GetQuicReloadableFlag(quic_validate_header_field_value_at_spdy_stream) &&
+ session_.version().UsesHttp3()) {
+ EXPECT_CALL(session_,
+ MaybeSendStopSendingFrame(_, QuicResetStreamError::FromInternal(
+ QUIC_STREAM_NO_ERROR)));
+ EXPECT_CALL(*stream_, WriteHeadersMock(false));
+ EXPECT_CALL(session_, WritevData(_, _, _, _, _, _))
+ .WillRepeatedly(
+ Invoke(&session_, &MockQuicSimpleServerSession::ConsumeData));
+ }
+
stream_->OnStreamHeaderList(false, kFakeFrameLen, header_list_);
- EXPECT_EQ(11, stream_->content_length());
- EXPECT_FALSE(QuicStreamPeer::read_side_closed(stream_));
- EXPECT_FALSE(stream_->reading_stopped());
- EXPECT_FALSE(stream_->write_side_closed());
+ if (GetQuicReloadableFlag(quic_validate_header_field_value_at_spdy_stream) &&
+ session_.version().UsesHttp3()) {
+ EXPECT_TRUE(QuicStreamPeer::read_side_closed(stream_));
+ EXPECT_TRUE(stream_->reading_stopped());
+ EXPECT_TRUE(stream_->write_side_closed());
+ } else {
+ EXPECT_EQ(11, stream_->content_length());
+ EXPECT_FALSE(QuicStreamPeer::read_side_closed(stream_));
+ EXPECT_FALSE(stream_->reading_stopped());
+ EXPECT_FALSE(stream_->write_side_closed());
+ }
}
TEST_P(QuicSimpleServerStreamTest,