Close connection when an IETF frame of unexpected type is received at the corresponding encryption level.
Protected by FLAGS_quic_reloadable_flag_quic_reject_unexpected_ietf_frame_types.
PiperOrigin-RevId: 360703267
Change-Id: Iba465aee2e8b709757f2c77d9b8bfd860ef89bf4
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 60eccb8..4407411 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -1174,9 +1174,16 @@
EncryptionLevel level) {
QuicPacketHeader header = ConstructPacketHeader(number, level);
QuicFrames frames;
- frames.push_back(QuicFrame(frame1_));
- if (has_stop_waiting) {
- frames.push_back(QuicFrame(stop_waiting_));
+ if (GetQuicReloadableFlag(quic_reject_unexpected_ietf_frame_types) &&
+ VersionHasIetfQuicFrames(version().transport_version) &&
+ (level == ENCRYPTION_INITIAL || level == ENCRYPTION_HANDSHAKE)) {
+ frames.push_back(QuicFrame(QuicPingFrame()));
+ frames.push_back(QuicFrame(QuicPaddingFrame(100)));
+ } else {
+ frames.push_back(QuicFrame(frame1_));
+ if (has_stop_waiting) {
+ frames.push_back(QuicFrame(stop_waiting_));
+ }
}
return ConstructPacket(header, frames);
}
@@ -2785,7 +2792,9 @@
TEST_P(QuicConnectionTest, RejectUnencryptedStreamData) {
// EXPECT_QUIC_BUG tests are expensive so only run one instance of them.
- if (!IsDefaultTestConfiguration()) {
+ if (!IsDefaultTestConfiguration() ||
+ (GetQuicReloadableFlag(quic_reject_unexpected_ietf_frame_types) &&
+ VersionHasIetfQuicFrames(version().transport_version))) {
return;
}
@@ -2939,28 +2948,6 @@
}
}
-TEST_P(QuicConnectionTest,
- AckFrequencyFrameOutsideApplicationDataNumberSpaceIsIgnored) {
- if (!GetParam().version.HasIetfQuicFrames()) {
- return;
- }
- connection_.set_can_receive_ack_frequency_frame();
-
- QuicAckFrequencyFrame ack_frequency_frame;
- ack_frequency_frame.packet_tolerance = 3;
- ProcessFramePacketAtLevel(1, QuicFrame(&ack_frequency_frame),
- ENCRYPTION_HANDSHAKE);
-
- // Expect 30 acks, every 2nd (instead of 3rd) packet including the first
- // packet with AckFrequencyFrame.
- EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(30);
- EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(60);
- // Receives packets 2 - 61.
- for (size_t i = 2; i <= 61; ++i) {
- ProcessDataPacket(i);
- }
-}
-
TEST_P(QuicConnectionTest, AckDecimationReducesAcks) {
const size_t kMinRttMs = 40;
RttStats* rtt_stats = const_cast<RttStats*>(manager_->GetRttStats());
@@ -7757,7 +7744,13 @@
EXPECT_CALL(visitor_,
OnConnectionClosed(_, ConnectionCloseSource::FROM_SELF));
ForceProcessFramePacket(QuicFrame(frame1_));
- TestConnectionCloseQuicErrorCode(QUIC_MAYBE_CORRUPTED_MEMORY);
+ if (GetQuicReloadableFlag(quic_reject_unexpected_ietf_frame_types) &&
+ VersionHasIetfQuicFrames(version().transport_version)) {
+ // INITIAL packet should not contain STREAM frame.
+ TestConnectionCloseQuicErrorCode(IETF_QUIC_PROTOCOL_VIOLATION);
+ } else {
+ TestConnectionCloseQuicErrorCode(QUIC_MAYBE_CORRUPTED_MEMORY);
+ }
}
TEST_P(QuicConnectionTest, ClientReceivesRejOnNonCryptoStream) {
@@ -7774,7 +7767,13 @@
EXPECT_CALL(visitor_,
OnConnectionClosed(_, ConnectionCloseSource::FROM_SELF));
ForceProcessFramePacket(QuicFrame(frame1_));
- TestConnectionCloseQuicErrorCode(QUIC_MAYBE_CORRUPTED_MEMORY);
+ if (GetQuicReloadableFlag(quic_reject_unexpected_ietf_frame_types) &&
+ VersionHasIetfQuicFrames(version().transport_version)) {
+ // INITIAL packet should not contain STREAM frame.
+ TestConnectionCloseQuicErrorCode(IETF_QUIC_PROTOCOL_VIOLATION);
+ } else {
+ TestConnectionCloseQuicErrorCode(QUIC_MAYBE_CORRUPTED_MEMORY);
+ }
}
TEST_P(QuicConnectionTest, CloseConnectionOnPacketTooLarge) {
@@ -9174,23 +9173,23 @@
// Receives packet 1000 in initial data.
ProcessCryptoPacketAtLevel(1000, ENCRYPTION_INITIAL);
EXPECT_TRUE(connection_.HasPendingAcks());
- peer_framer_.SetEncrypter(ENCRYPTION_ZERO_RTT,
+ peer_framer_.SetEncrypter(ENCRYPTION_FORWARD_SECURE,
std::make_unique<TaggingEncrypter>(0x02));
- SetDecrypter(ENCRYPTION_ZERO_RTT,
+ SetDecrypter(ENCRYPTION_FORWARD_SECURE,
std::make_unique<StrictTaggingDecrypter>(0x02));
connection_.SetEncrypter(ENCRYPTION_INITIAL,
std::make_unique<TaggingEncrypter>(0x02));
// Receives packet 1000 in application data.
- ProcessDataPacketAtLevel(1000, false, ENCRYPTION_ZERO_RTT);
+ ProcessDataPacketAtLevel(1000, false, ENCRYPTION_FORWARD_SECURE);
EXPECT_TRUE(connection_.HasPendingAcks());
- connection_.SendApplicationDataAtLevel(ENCRYPTION_ZERO_RTT, 5, "data", 0,
- NO_FIN);
+ connection_.SendApplicationDataAtLevel(ENCRYPTION_FORWARD_SECURE, 5, "data",
+ 0, NO_FIN);
// Verify application data ACK gets bundled with outgoing data.
EXPECT_EQ(2u, writer_->frame_count());
// Make sure ACK alarm is still set because initial data is not ACKed.
EXPECT_TRUE(connection_.HasPendingAcks());
// Receive packet 1001 in application data.
- ProcessDataPacketAtLevel(1001, false, ENCRYPTION_ZERO_RTT);
+ ProcessDataPacketAtLevel(1001, false, ENCRYPTION_FORWARD_SECURE);
clock_.AdvanceTime(DefaultRetransmissionTime());
// Simulates ACK alarm fires and verify two ACKs are flushed.
EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(2);
@@ -9199,13 +9198,9 @@
connection_.GetAckAlarm()->Fire();
EXPECT_FALSE(connection_.HasPendingAcks());
// Receives more packets in application data.
- ProcessDataPacketAtLevel(1002, false, ENCRYPTION_ZERO_RTT);
+ ProcessDataPacketAtLevel(1002, false, ENCRYPTION_FORWARD_SECURE);
EXPECT_TRUE(connection_.HasPendingAcks());
- peer_framer_.SetEncrypter(ENCRYPTION_FORWARD_SECURE,
- std::make_unique<TaggingEncrypter>(0x02));
- SetDecrypter(ENCRYPTION_FORWARD_SECURE,
- std::make_unique<StrictTaggingDecrypter>(0x02));
// Verify zero rtt and forward secure packets get acked in the same packet.
EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)).Times(1);
ProcessDataPacket(1003);
@@ -11049,7 +11044,10 @@
std::make_unique<TaggingEncrypter>(0x01));
connection_.SetDefaultEncryptionLevel(ENCRYPTION_HANDSHAKE);
// Verify all ENCRYPTION_HANDSHAKE packets get processed.
- EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(6);
+ if (!GetQuicReloadableFlag(quic_reject_unexpected_ietf_frame_types) ||
+ !VersionHasIetfQuicFrames(version().transport_version)) {
+ EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(6);
+ }
connection_.GetProcessUndecryptablePacketsAlarm()->Fire();
EXPECT_EQ(1u, QuicConnectionPeer::NumUndecryptablePackets(&connection_));
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h
index 708ae5c..0e9fa10 100644
--- a/quic/core/quic_flags_list.h
+++ b/quic/core/quic_flags_list.h
@@ -48,6 +48,7 @@
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_h3_datagram, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_pass_path_response_to_validator, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_preempt_stream_data_with_handshake_packet, false)
+QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_reject_unexpected_ietf_frame_types, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_require_handshake_confirmation, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_send_path_response, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_send_timestamps, false)
diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc
index fdccef9..24034fd 100644
--- a/quic/core/quic_framer.cc
+++ b/quic/core/quic_framer.cc
@@ -1899,7 +1899,7 @@
// Handle the payload.
if (VersionHasIetfQuicFrames(version_.transport_version)) {
current_received_frame_type_ = 0;
- if (!ProcessIetfFrameData(&reader, *header)) {
+ if (!ProcessIetfFrameData(&reader, *header, decrypted_level)) {
current_received_frame_type_ = 0;
QUICHE_DCHECK_NE(QUIC_NO_ERROR,
error_); // ProcessIetfFrameData sets the error.
@@ -3098,8 +3098,33 @@
return true;
}
+// static
+bool QuicFramer::IsIetfFrameTypeExpectedForEncryptionLevel(
+ uint64_t frame_type,
+ EncryptionLevel level) {
+ switch (level) {
+ case ENCRYPTION_INITIAL:
+ case ENCRYPTION_HANDSHAKE:
+ return frame_type == IETF_CRYPTO || frame_type == IETF_ACK ||
+ frame_type == IETF_PING || frame_type == IETF_PADDING ||
+ frame_type == IETF_CONNECTION_CLOSE;
+ case ENCRYPTION_ZERO_RTT:
+ return !(frame_type == IETF_ACK || frame_type == IETF_CRYPTO ||
+ frame_type == IETF_HANDSHAKE_DONE ||
+ frame_type == IETF_NEW_TOKEN ||
+ frame_type == IETF_PATH_RESPONSE ||
+ frame_type == IETF_RETIRE_CONNECTION_ID);
+ case ENCRYPTION_FORWARD_SECURE:
+ return true;
+ default:
+ QUIC_BUG << "Unknown encryption level: " << level;
+ }
+ return false;
+}
+
bool QuicFramer::ProcessIetfFrameData(QuicDataReader* reader,
- const QuicPacketHeader& header) {
+ const QuicPacketHeader& header,
+ EncryptionLevel decrypted_level) {
QUICHE_DCHECK(VersionHasIetfQuicFrames(version_.transport_version))
<< "Attempt to process frames as IETF frames but version ("
<< version_.transport_version << ") does not support IETF Framing.";
@@ -3118,6 +3143,19 @@
set_detailed_error("Unable to read frame type.");
return RaiseError(QUIC_INVALID_FRAME_DATA);
}
+ if (reject_unexpected_ietf_frame_types_) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_reject_unexpected_ietf_frame_types, 1,
+ 2);
+ if (!IsIetfFrameTypeExpectedForEncryptionLevel(frame_type,
+ decrypted_level)) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_reject_unexpected_ietf_frame_types, 2,
+ 2);
+ set_detailed_error(absl::StrCat("IETF frame type ", frame_type,
+ " is unexpected at encryption level ",
+ decrypted_level));
+ return RaiseError(IETF_QUIC_PROTOCOL_VIOLATION);
+ }
+ }
current_received_frame_type_ = frame_type;
// Is now the number of bytes into which the frame type was encoded.
diff --git a/quic/core/quic_framer.h b/quic/core/quic_framer.h
index 571278d..0307594 100644
--- a/quic/core/quic_framer.h
+++ b/quic/core/quic_framer.h
@@ -826,8 +826,13 @@
QuicPacketNumber base_packet_number,
uint64_t* packet_number);
bool ProcessFrameData(QuicDataReader* reader, const QuicPacketHeader& header);
+
+ static bool IsIetfFrameTypeExpectedForEncryptionLevel(uint64_t frame_type,
+ EncryptionLevel level);
+
bool ProcessIetfFrameData(QuicDataReader* reader,
- const QuicPacketHeader& header);
+ const QuicPacketHeader& header,
+ EncryptionLevel decrypted_level);
bool ProcessStreamFrame(QuicDataReader* reader,
uint8_t frame_type,
QuicStreamFrame* frame);
@@ -1158,6 +1163,9 @@
// Indicates whether received RETRY packets should be dropped.
bool drop_incoming_retry_packets_ = false;
+ bool reject_unexpected_ietf_frame_types_ =
+ GetQuicReloadableFlag(quic_reject_unexpected_ietf_frame_types);
+
// The length in bytes of the last packet number written to an IETF-framed
// packet.
size_t last_written_packet_number_length_;
diff --git a/quic/core/quic_framer_test.cc b/quic/core/quic_framer_test.cc
index 12b779a..dafe1f7 100644
--- a/quic/core/quic_framer_test.cc
+++ b/quic/core/quic_framer_test.cc
@@ -15164,6 +15164,47 @@
EXPECT_EQ(1, visitor_.decrypted_first_packet_in_key_phase_count_);
}
+TEST_P(QuicFramerTest, ErrorWhenUnexpectedFrameTypeEncountered) {
+ if (!GetQuicReloadableFlag(quic_reject_unexpected_ietf_frame_types) ||
+ !VersionHasIetfQuicFrames(framer_.transport_version()) ||
+ !QuicVersionHasLongHeaderLengths(framer_.transport_version()) ||
+ !framer_.version().HasLongHeaderLengths()) {
+ return;
+ }
+ SetDecrypterLevel(ENCRYPTION_ZERO_RTT);
+ // clang-format off
+ unsigned char 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
+ 0x08,
+ // source connection ID
+ 0xFE, 0xDC, 0xBA, 0x98, 0x76, 0x54, 0x32, 0x11,
+ // long header packet length
+ 0x05,
+ // packet number
+ 0x12, 0x34, 0x56, 0x00,
+ // unexpected ietf ack frame type in 0-RTT packet
+ 0x02,
+ };
+ // clang-format on
+
+ QuicEncryptedPacket encrypted(AsChars(packet), ABSL_ARRAYSIZE(packet), false);
+
+ EXPECT_FALSE(framer_.ProcessPacket(encrypted));
+
+ EXPECT_THAT(framer_.error(), IsError(IETF_QUIC_PROTOCOL_VIOLATION));
+ EXPECT_EQ("IETF frame type 2 is unexpected at encryption level 2",
+ framer_.detailed_error());
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/core/quic_packet_creator_test.cc b/quic/core/quic_packet_creator_test.cc
index 8ff44c1..11c9800 100644
--- a/quic/core/quic_packet_creator_test.cc
+++ b/quic/core/quic_packet_creator_test.cc
@@ -299,7 +299,9 @@
for (int i = ENCRYPTION_INITIAL; i < NUM_ENCRYPTION_LEVELS; ++i) {
EncryptionLevel level = static_cast<EncryptionLevel>(i);
creator_.set_encryption_level(level);
- frames_.push_back(QuicFrame(new QuicAckFrame(InitAckFrame(1))));
+ if (level != ENCRYPTION_ZERO_RTT) {
+ frames_.push_back(QuicFrame(new QuicAckFrame(InitAckFrame(1))));
+ }
QuicStreamId stream_id = QuicUtils::GetFirstBidirectionalStreamId(
client_framer_.transport_version(), Perspective::IS_CLIENT);
if (level != ENCRYPTION_INITIAL && level != ENCRYPTION_HANDSHAKE) {
@@ -308,7 +310,9 @@
}
SerializedPacket serialized = SerializeAllFrames(frames_);
EXPECT_EQ(level, serialized.encryption_level);
- delete frames_[0].ack_frame;
+ if (level != ENCRYPTION_ZERO_RTT) {
+ delete frames_[0].ack_frame;
+ }
frames_.clear();
{
@@ -318,13 +322,15 @@
EXPECT_CALL(framer_visitor_, OnUnauthenticatedHeader(_));
EXPECT_CALL(framer_visitor_, OnDecryptedPacket(_, _));
EXPECT_CALL(framer_visitor_, OnPacketHeader(_));
- EXPECT_CALL(framer_visitor_, OnAckFrameStart(_, _))
- .WillOnce(Return(true));
- EXPECT_CALL(framer_visitor_,
- OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)))
- .WillOnce(Return(true));
- EXPECT_CALL(framer_visitor_, OnAckFrameEnd(QuicPacketNumber(1)))
- .WillOnce(Return(true));
+ if (level != ENCRYPTION_ZERO_RTT) {
+ EXPECT_CALL(framer_visitor_, OnAckFrameStart(_, _))
+ .WillOnce(Return(true));
+ EXPECT_CALL(framer_visitor_,
+ OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)))
+ .WillOnce(Return(true));
+ EXPECT_CALL(framer_visitor_, OnAckFrameEnd(QuicPacketNumber(1)))
+ .WillOnce(Return(true));
+ }
if (level != ENCRYPTION_INITIAL && level != ENCRYPTION_HANDSHAKE) {
EXPECT_CALL(framer_visitor_, OnStreamFrame(_));
}
@@ -2118,7 +2124,9 @@
EncryptionLevel level = static_cast<EncryptionLevel>(i);
creator_.set_encryption_level(level);
QuicAckFrame ack_frame(InitAckFrame(1));
- frames_.push_back(QuicFrame(&ack_frame));
+ if (level != ENCRYPTION_ZERO_RTT) {
+ frames_.push_back(QuicFrame(&ack_frame));
+ }
if (level != ENCRYPTION_INITIAL && level != ENCRYPTION_HANDSHAKE) {
frames_.push_back(
QuicFrame(QuicStreamFrame(1, false, 0u, absl::string_view())));
@@ -2156,20 +2164,26 @@
EXPECT_CALL(framer_visitor_, OnUnauthenticatedHeader(_));
EXPECT_CALL(framer_visitor_, OnDecryptedPacket(_, _));
EXPECT_CALL(framer_visitor_, OnPacketHeader(_));
- EXPECT_CALL(framer_visitor_, OnAckFrameStart(_, _)).WillOnce(Return(true));
- EXPECT_CALL(framer_visitor_,
- OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)))
- .WillOnce(Return(true));
- EXPECT_CALL(framer_visitor_, OnAckFrameEnd(_)).WillOnce(Return(true));
+ if (i != ENCRYPTION_ZERO_RTT) {
+ EXPECT_CALL(framer_visitor_, OnAckFrameStart(_, _))
+ .WillOnce(Return(true));
+ EXPECT_CALL(framer_visitor_,
+ OnAckRange(QuicPacketNumber(1), QuicPacketNumber(2)))
+ .WillOnce(Return(true));
+ EXPECT_CALL(framer_visitor_, OnAckFrameEnd(_)).WillOnce(Return(true));
+ }
if (i == ENCRYPTION_INITIAL) {
// Verify padding is added.
EXPECT_CALL(framer_visitor_, OnPaddingFrame(_));
- } else {
+ } else if (i != ENCRYPTION_ZERO_RTT) {
EXPECT_CALL(framer_visitor_, OnPaddingFrame(_)).Times(testing::AtMost(1));
}
if (i != ENCRYPTION_INITIAL && i != ENCRYPTION_HANDSHAKE) {
EXPECT_CALL(framer_visitor_, OnStreamFrame(_));
}
+ if (i == ENCRYPTION_ZERO_RTT) {
+ EXPECT_CALL(framer_visitor_, OnPaddingFrame(_));
+ }
EXPECT_CALL(framer_visitor_, OnPacketComplete());
server_framer_.ProcessPacket(*packets[i]);
