gfe-relnote: In QUIC, only do minimum validation of coalesced packets (only validate connection IDs). Protected by gfe2_reloadable_flag_quic_minimum_validation_of_coalesced_packets. Also do not consider unprocessable coalesced packet as QUIC_PEER_BUG, because garbage content could be padding outside QUIC packet.. PiperOrigin-RevId: 297414444 Change-Id: Ifc6359e62a561746012fc718aaf24a821a7cfedf
diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc index 280e464..6e7e589 100644 --- a/quic/core/quic_framer.cc +++ b/quic/core/quic_framer.cc
@@ -1580,23 +1580,39 @@ QuicPacketHeader coalesced_header; if (!ProcessIetfPacketHeader(&coalesced_reader, &coalesced_header)) { - QUIC_PEER_BUG << ENDPOINT - << "Failed to parse received coalesced header of length " - << coalesced_data_length << " with error: " << detailed_error_ - << ": " - << quiche::QuicheTextUtils::HexEncode(coalesced_data, - coalesced_data_length) - << " previous header was " << header; + // Some implementations pad their INITIAL packets by sending random invalid + // data after the INITIAL, and that is allowed by the specification. If we + // fail to parse a subsequent coalesced packet, simply ignore it. + QUIC_DLOG(INFO) << ENDPOINT + << "Failed to parse received coalesced header of length " + << coalesced_data_length + << " with error: " << detailed_error_ << ": " + << quiche::QuicheTextUtils::HexEncode(coalesced_data, + coalesced_data_length) + << " previous header was " << header; return; } - if (coalesced_header.destination_connection_id != - header.destination_connection_id || - (coalesced_header.form != IETF_QUIC_SHORT_HEADER_PACKET && - coalesced_header.version != header.version)) { - QUIC_PEER_BUG << ENDPOINT << "Received mismatched coalesced header " - << coalesced_header << " previous header was " << header; - return; + if (GetQuicReloadableFlag(quic_minimum_validation_of_coalesced_packets)) { + QUIC_RELOADABLE_FLAG_COUNT(quic_minimum_validation_of_coalesced_packets); + if (coalesced_header.destination_connection_id != + header.destination_connection_id) { + // Drop coalesced packets with mismatched connection IDs. + QUIC_DLOG(INFO) << ENDPOINT << "Received mismatched coalesced header " + << coalesced_header << " previous header was " << header; + QUIC_CODE_COUNT( + quic_received_coalesced_packets_with_mismatched_connection_id); + return; + } + } else { + if (coalesced_header.destination_connection_id != + header.destination_connection_id || + (coalesced_header.form != IETF_QUIC_SHORT_HEADER_PACKET && + coalesced_header.version != header.version)) { + QUIC_PEER_BUG << ENDPOINT << "Received mismatched coalesced header " + << coalesced_header << " previous header was " << header; + return; + } } QuicEncryptedPacket coalesced_packet(coalesced_data, coalesced_data_length,
diff --git a/quic/core/quic_framer_test.cc b/quic/core/quic_framer_test.cc index beac097..ec45837 100644 --- a/quic/core/quic_framer_test.cc +++ b/quic/core/quic_framer_test.cc
@@ -12282,6 +12282,272 @@ CheckStreamFrameData("HELLO_WORLD?", visitor_.stream_frames_[1].get()); } +TEST_P(QuicFramerTest, CoalescedPacketWithUdpPadding) { + if (!framer_.version().HasLongHeaderLengths()) { + return; + } + SetDecrypterLevel(ENCRYPTION_ZERO_RTT); + // clang-format off + unsigned char packet[] = { + // first coalesced packet + // public flags (long header with packet type ZERO_RTT_PROTECTED and + // 4-byte packet number) + 0xD3, + // version + QUIC_VERSION_BYTES, + // destination connection ID length + 0x08, + // destination connection ID + 0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10, + // source connection ID length + 0x00, + // long header packet length + 0x1E, + // packet number + 0x12, 0x34, 0x56, 0x78, + // frame type (stream frame with fin) + 0xFE, + // stream id + 0x02, 0x03, 0x04, + // offset + 0x3A, 0x98, 0xFE, 0xDC, 0x32, 0x10, 0x76, 0x54, + // data length + 0x00, 0x0c, + // data + 'h', 'e', 'l', 'l', + 'o', ' ', 'w', 'o', + 'r', 'l', 'd', '!', + // padding + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + }; + unsigned char packet99[] = { + // first coalesced packet + // public flags (long header with packet type ZERO_RTT_PROTECTED and + // 4-byte packet number) + 0xD3, + // version + QUIC_VERSION_BYTES, + // destination connection ID length + 0x08, + // destination connection ID + 0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10, + // source connection ID length + 0x00, + // long header packet length + 0x1E, + // packet number + 0x12, 0x34, 0x56, 0x78, + // frame type (IETF_STREAM frame with FIN, LEN, and OFFSET bits set) + 0x08 | 0x01 | 0x02 | 0x04, + // stream id + kVarInt62FourBytes + 0x00, 0x02, 0x03, 0x04, + // offset + kVarInt62EightBytes + 0x3A, 0x98, 0xFE, 0xDC, + 0x32, 0x10, 0x76, 0x54, + // data length + kVarInt62OneByte + 0x0c, + // data + 'h', 'e', 'l', 'l', + 'o', ' ', 'w', 'o', + 'r', 'l', 'd', '!', + // padding + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + }; + // clang-format on + + unsigned char* p = packet; + size_t p_length = QUICHE_ARRAYSIZE(packet); + if (framer_.version().HasIetfQuicFrames()) { + p = packet99; + p_length = QUICHE_ARRAYSIZE(packet99); + } + + QuicEncryptedPacket encrypted(AsChars(p), p_length, false); + EXPECT_TRUE(framer_.ProcessPacket(encrypted)); + + EXPECT_THAT(framer_.error(), IsQuicNoError()); + ASSERT_TRUE(visitor_.header_.get()); + + ASSERT_EQ(1u, visitor_.stream_frames_.size()); + EXPECT_EQ(0u, visitor_.ack_frames_.size()); + + // Stream ID should be the last 3 bytes of kStreamId. + EXPECT_EQ(0x00FFFFFF & kStreamId, visitor_.stream_frames_[0]->stream_id); + EXPECT_TRUE(visitor_.stream_frames_[0]->fin); + EXPECT_EQ(kStreamOffset, visitor_.stream_frames_[0]->offset); + CheckStreamFrameData("hello world!", visitor_.stream_frames_[0].get()); + + EXPECT_EQ(visitor_.coalesced_packets_.size(), 0u); +} + +TEST_P(QuicFramerTest, CoalescedPacketWithDifferentVersion) { + if (!QuicVersionHasLongHeaderLengths(framer_.transport_version())) { + return; + } + SetQuicReloadableFlag(quic_minimum_validation_of_coalesced_packets, true); + SetDecrypterLevel(ENCRYPTION_ZERO_RTT); + // clang-format off + unsigned char packet[] = { + // first coalesced packet + // public flags (long header with packet type ZERO_RTT_PROTECTED and + // 4-byte packet number) + 0xD3, + // version + QUIC_VERSION_BYTES, + // destination connection ID length + 0x08, + // destination connection ID + 0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10, + // source connection ID length + 0x00, + // long header packet length + 0x1E, + // packet number + 0x12, 0x34, 0x56, 0x78, + // frame type (stream frame with fin) + 0xFE, + // stream id + 0x02, 0x03, 0x04, + // offset + 0x3A, 0x98, 0xFE, 0xDC, 0x32, 0x10, 0x76, 0x54, + // data length + 0x00, 0x0c, + // data + 'h', 'e', 'l', 'l', + 'o', ' ', 'w', 'o', + 'r', 'l', 'd', '!', + // second coalesced packet + // public flags (long header with packet type ZERO_RTT_PROTECTED and + // 4-byte packet number) + 0xD3, + // garbage version + 'G', 'A', 'B', 'G', + // destination connection ID length + 0x08, + // destination connection ID + 0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10, + // source connection ID length + 0x00, + // long header packet length + 0x1E, + // packet number + 0x12, 0x34, 0x56, 0x79, + // frame type (stream frame with fin) + 0xFE, + // stream id + 0x02, 0x03, 0x04, + // offset + 0x3A, 0x98, 0xFE, 0xDC, 0x32, 0x10, 0x76, 0x54, + // data length + 0x00, 0x0c, + // data + 'H', 'E', 'L', 'L', + 'O', '_', 'W', 'O', + 'R', 'L', 'D', '?', + }; + unsigned char packet99[] = { + // first coalesced packet + // public flags (long header with packet type ZERO_RTT_PROTECTED and + // 4-byte packet number) + 0xD3, + // version + QUIC_VERSION_BYTES, + // destination connection ID length + 0x08, + // destination connection ID + 0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10, + // source connection ID length + 0x00, + // long header packet length + 0x1E, + // packet number + 0x12, 0x34, 0x56, 0x78, + // frame type (IETF_STREAM frame with FIN, LEN, and OFFSET bits set) + 0x08 | 0x01 | 0x02 | 0x04, + // stream id + kVarInt62FourBytes + 0x00, 0x02, 0x03, 0x04, + // offset + kVarInt62EightBytes + 0x3A, 0x98, 0xFE, 0xDC, + 0x32, 0x10, 0x76, 0x54, + // data length + kVarInt62OneByte + 0x0c, + // data + 'h', 'e', 'l', 'l', + 'o', ' ', 'w', 'o', + 'r', 'l', 'd', '!', + // second coalesced packet + // public flags (long header with packet type ZERO_RTT_PROTECTED and + // 4-byte packet number) + 0xD3, + // garbage version + 'G', 'A', 'B', 'G', + // destination connection ID length + 0x08, + // destination connection ID + 0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x10, + // source connection ID length + 0x00, + // long header packet length + 0x1E, + // packet number + 0x12, 0x34, 0x56, 0x79, + // frame type (IETF_STREAM frame with FIN, LEN, and OFFSET bits set) + 0x08 | 0x01 | 0x02 | 0x04, + // stream id + kVarInt62FourBytes + 0x00, 0x02, 0x03, 0x04, + // offset + kVarInt62EightBytes + 0x3A, 0x98, 0xFE, 0xDC, + 0x32, 0x10, 0x76, 0x54, + // data length + kVarInt62OneByte + 0x0c, + // data + 'H', 'E', 'L', 'L', + 'O', '_', 'W', 'O', + 'R', 'L', 'D', '?', + }; + // clang-format on + + unsigned char* p = packet; + size_t p_length = QUICHE_ARRAYSIZE(packet); + if (framer_.version().HasIetfQuicFrames()) { + p = packet99; + p_length = QUICHE_ARRAYSIZE(packet99); + } + + QuicEncryptedPacket encrypted(AsChars(p), p_length, false); + EXPECT_TRUE(framer_.ProcessPacket(encrypted)); + + EXPECT_THAT(framer_.error(), IsQuicNoError()); + ASSERT_TRUE(visitor_.header_.get()); + + ASSERT_EQ(1u, visitor_.stream_frames_.size()); + EXPECT_EQ(0u, visitor_.ack_frames_.size()); + + // Stream ID should be the last 3 bytes of kStreamId. + EXPECT_EQ(0x00FFFFFF & kStreamId, visitor_.stream_frames_[0]->stream_id); + EXPECT_TRUE(visitor_.stream_frames_[0]->fin); + EXPECT_EQ(kStreamOffset, visitor_.stream_frames_[0]->offset); + CheckStreamFrameData("hello world!", visitor_.stream_frames_[0].get()); + + ASSERT_EQ(visitor_.coalesced_packets_.size(), 1u); + EXPECT_TRUE(framer_.ProcessPacket(*visitor_.coalesced_packets_[0].get())); + + EXPECT_THAT(framer_.error(), IsQuicNoError()); + ASSERT_TRUE(visitor_.header_.get()); + + ASSERT_EQ(1u, visitor_.stream_frames_.size()); + // Verify version mismatch gets reported. + EXPECT_EQ(1, visitor_.version_mismatch_); +} + TEST_P(QuicFramerTest, UndecryptablePacketWithoutDecrypter) { QuicFramerPeer::SetPerspective(&framer_, Perspective::IS_CLIENT); @@ -12798,8 +13064,12 @@ QuicEncryptedPacket encrypted(AsChars(p), p_length, false); - EXPECT_QUIC_PEER_BUG(EXPECT_TRUE(framer_.ProcessPacket(encrypted)), - "Server: Received mismatched coalesced header.*"); + if (GetQuicReloadableFlag(quic_minimum_validation_of_coalesced_packets)) { + EXPECT_TRUE(framer_.ProcessPacket(encrypted)); + } else { + EXPECT_QUIC_PEER_BUG(EXPECT_TRUE(framer_.ProcessPacket(encrypted)), + "Server: Received mismatched coalesced header.*"); + } EXPECT_THAT(framer_.error(), IsQuicNoError()); ASSERT_TRUE(visitor_.header_.get()); @@ -12904,8 +13174,7 @@ QuicEncryptedPacket encrypted(AsChars(p), p_length, false); - EXPECT_QUIC_PEER_BUG(EXPECT_TRUE(framer_.ProcessPacket(encrypted)), - "Server: Failed to parse received coalesced header.*"); + EXPECT_TRUE(framer_.ProcessPacket(encrypted)); EXPECT_THAT(framer_.error(), IsQuicNoError()); ASSERT_TRUE(visitor_.header_.get()); @@ -12975,9 +13244,8 @@ false); // Make sure we discard the subsequent zeroes. - EXPECT_QUIC_PEER_BUG(EXPECT_TRUE(framer_.ProcessPacket(encrypted)), - "Server: (Failed to parse received|Received mismatched) " - "coalesced header.*"); + EXPECT_TRUE(framer_.ProcessPacket(encrypted)); + EXPECT_TRUE(visitor_.coalesced_packets_.empty()); } TEST_P(QuicFramerTest, ClientReceivesInvalidVersion) {