Make HttpDecoder::ParsePriorityFrame() more robust.
1. As https://crbug.com/981291 and https://crbug.com/981646 demonstrate,
ParsePriorityFrame() used crashes on certain input. Fix that.
2. Signal QUIC_INVALID_FRAME_DATA instead of QUIC_INTERNAL_ERROR, since
handling invalid input is not an internal bug but an error on the
sender's part.
3. Signal error if PRIORITY frame has superfluous payload.
4. Add tests.
gfe-relnote: n/a, change in QUIC v99-only class
PiperOrigin-RevId: 258409280
Change-Id: If2d5a14c222955ebaa4650b0723be180330c8d1a
diff --git a/quic/core/http/http_decoder.cc b/quic/core/http/http_decoder.cc
index 1c4c083..3388b2c 100644
--- a/quic/core/http/http_decoder.cc
+++ b/quic/core/http/http_decoder.cc
@@ -449,27 +449,42 @@
bool HttpDecoder::ParsePriorityFrame(QuicDataReader* reader,
PriorityFrame* frame) {
uint8_t flags;
- bool success = reader->ReadUInt8(&flags);
- DCHECK(success);
+ if (!reader->ReadUInt8(&flags)) {
+ // TODO(b/124216424): Use HTTP_MALFORMED_FRAME.
+ RaiseError(QUIC_INVALID_FRAME_DATA, "Unable to read PRIORITY frame flags.");
+ return false;
+ }
frame->prioritized_type =
static_cast<PriorityElementType>(ExtractBits(flags, 2, 6));
frame->dependency_type =
static_cast<PriorityElementType>(ExtractBits(flags, 2, 4));
+ // TODO(b/137662729): Update bitmask for exclusive flag.
frame->exclusive = flags % 2 == 1;
- // TODO(bnc): Handle partial delivery.
+ // TODO(b/137359636): Handle partial delivery.
if (frame->prioritized_type != ROOT_OF_TREE &&
!reader->ReadVarInt62(&frame->prioritized_element_id)) {
- RaiseError(QUIC_INTERNAL_ERROR, "Unable to read prioritized_element_id");
+ // TODO(b/124216424): Use HTTP_MALFORMED_FRAME.
+ RaiseError(QUIC_INVALID_FRAME_DATA,
+ "Unable to read prioritized_element_id.");
return false;
}
if (frame->dependency_type != ROOT_OF_TREE &&
!reader->ReadVarInt62(&frame->element_dependency_id)) {
- RaiseError(QUIC_INTERNAL_ERROR, "Unable to read element_dependency_id");
+ // TODO(b/124216424): Use HTTP_MALFORMED_FRAME.
+ RaiseError(QUIC_INVALID_FRAME_DATA,
+ "Unable to read element_dependency_id.");
return false;
}
if (!reader->ReadUInt8(&frame->weight)) {
- RaiseError(QUIC_INTERNAL_ERROR, "Unable to read priority frame weight");
+ // TODO(b/124216424): Use HTTP_MALFORMED_FRAME.
+ RaiseError(QUIC_INVALID_FRAME_DATA,
+ "Unable to read priority frame weight.");
+ return false;
+ }
+ if (!reader->IsDoneReading()) {
+ // TODO(b/124216424): Use HTTP_MALFORMED_FRAME.
+ RaiseError(QUIC_INVALID_FRAME_DATA, "Superfluous data in priority frame.");
return false;
}
return true;
diff --git a/quic/core/http/http_decoder_test.cc b/quic/core/http/http_decoder_test.cc
index 9229db0..735f322 100644
--- a/quic/core/http/http_decoder_test.cc
+++ b/quic/core/http/http_decoder_test.cc
@@ -411,6 +411,52 @@
EXPECT_EQ("", decoder_.error_detail());
}
+// Regression test for https://crbug.com/981291 and https://crbug.com/981646.
+TEST_F(HttpDecoderTest, CorruptPriorityFrame) {
+ const char* const payload1 =
+ "\x01" // request stream, request stream, exclusive
+ "\x03" // prioritized_element_id
+ "\x04" // element_dependency_id
+ "\xFF" // weight
+ "\xFF"; // superfluous data
+ const char* const payload2 =
+ "\xf1" // root of tree, root of tree, exclusive
+ "\xFF" // weight
+ "\xFF"; // superfluous data
+ struct {
+ const char* const payload;
+ size_t payload_length;
+ const char* const error_message;
+ } kTestData[] = {
+ {payload1, 0, "Unable to read PRIORITY frame flags."},
+ {payload1, 1, "Unable to read prioritized_element_id."},
+ {payload1, 2, "Unable to read element_dependency_id."},
+ {payload1, 3, "Unable to read priority frame weight."},
+ {payload1, 5, "Superfluous data in priority frame."},
+ {payload2, 0, "Unable to read PRIORITY frame flags."},
+ {payload2, 1, "Unable to read priority frame weight."},
+ {payload2, 3, "Superfluous data in priority frame."},
+ };
+
+ for (const auto& test_data : kTestData) {
+ std::string input;
+ input.push_back(2u); // type PRIORITY
+ input.push_back(test_data.payload_length);
+ size_t header_length = input.size();
+ input.append(test_data.payload, test_data.payload_length);
+
+ HttpDecoder decoder(&visitor_);
+ EXPECT_CALL(visitor_, OnPriorityFrameStart(Http3FrameLengths(
+ header_length, test_data.payload_length)));
+
+ QuicByteCount processed_bytes =
+ decoder.ProcessInput(input.data(), input.size());
+ EXPECT_EQ(input.size(), processed_bytes);
+ EXPECT_EQ(QUIC_INVALID_FRAME_DATA, decoder.error());
+ EXPECT_EQ(test_data.error_message, decoder.error_detail());
+ }
+}
+
TEST_F(HttpDecoderTest, SettingsFrame) {
InSequence s;
std::string input(
