From 045d1e6e2535e7b44acc3283df15a73be3e203d8 Mon Sep 17 00:00:00 2001 From: Hani Damlaj Date: Fri, 18 Feb 2022 14:50:45 -0800 Subject: [PATCH] Close Transport Upon Receiving Client Initial With Malformed DstCid Summary: - as title :) Reviewed By: mjoras Differential Revision: D34124710 fbshipit-source-id: 3cee590d38abf395a09ca3a1d8632a5c4d8e3b64 --- quic/server/QuicServerWorker.cpp | 6 ++- quic/server/state/ServerStateMachine.cpp | 9 +++- quic/server/test/QuicServerTransportTest.cpp | 46 +++++++++++++++++--- 3 files changed, 52 insertions(+), 9 deletions(-) diff --git a/quic/server/QuicServerWorker.cpp b/quic/server/QuicServerWorker.cpp index 5473bd73a..cb195286b 100644 --- a/quic/server/QuicServerWorker.cpp +++ b/quic/server/QuicServerWorker.cpp @@ -1219,7 +1219,11 @@ void QuicServerWorker::onConnectionUnbound( const std::vector& connectionIdData) noexcept { VLOG(4) << "Removing from sourceAddressMap_ address=" << source.first; - if (transport->getConnectionsStats().totalBytesSent == 0) { + auto& localConnectionError = transport->getState()->localConnectionError; + if (transport->getConnectionsStats().totalBytesSent == 0 && + !(localConnectionError && localConnectionError->code.asLocalErrorCode() && + *localConnectionError->code.asLocalErrorCode() == + LocalErrorCode::CONNECTION_ABANDONED)) { QUIC_STATS(statsCallback_, onConnectionCloseZeroBytesWritten); } diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index 347d160f0..39cd8421f 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/quic/server/state/ServerStateMachine.cpp @@ -664,8 +664,10 @@ void onServerReadDataFromOpen( readData.networkData.data->computeChainDataLength() == 0) { return; } + + bool firstPacketFromPeer = false; if (!conn.readCodec) { - // First packet from the peer + firstPacketFromPeer = true; folly::io::Cursor cursor(readData.networkData.data.get()); auto initialByte = cursor.readBE(); auto parsedLongHeader = parseLongHeaderInvariant(initialByte, cursor); @@ -803,6 +805,11 @@ void onServerReadDataFromOpen( if (conn.qLogger) { conn.qLogger->addPacketDrop(packetSize, kCipherUnavailable); } + if (firstPacketFromPeer) { + throw QuicInternalException( + "Failed to decrypt first packet from peer", + LocalErrorCode::CONNECTION_ABANDONED); + } break; } case CodecResult::Type::REGULAR_PACKET: diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp index cb2326d9e..7aa40b622 100644 --- a/quic/server/test/QuicServerTransportTest.cpp +++ b/quic/server/test/QuicServerTransportTest.cpp @@ -2943,12 +2943,11 @@ TEST_F(QuicUnencryptedServerTransportTest, TestUnencryptedAck) { auto qLogger = std::make_shared(VantagePoint::Server); server->getNonConstConn().qLogger = qLogger; AckBlocks acks = {{1, 2}}; - auto expected = IOBuf::copyBuffer("hello"); PacketNum nextPacketNum = clientNextInitialPacketNum++; LongHeader header( LongHeader::Types::Initial, *clientConnectionId, - server->getConn().serverConnectionId.value_or(getTestConnectionId(1)), + *initialDestinationConnectionId, nextPacketNum, QuicVersion::MVFST); RegularQuicPacketBuilder builder( @@ -2962,7 +2961,7 @@ TEST_F(QuicUnencryptedServerTransportTest, TestUnencryptedAck) { *getInitialCipher(), *getInitialHeaderCipher(), nextPacketNum); - EXPECT_NO_THROW(deliverData(std::move(packet))); + EXPECT_THROW(deliverData(std::move(packet)), std::runtime_error); std::vector indices = getQLogEventIndices(QLogEventType::PacketDrop, qLogger); EXPECT_EQ(indices.size(), 1); @@ -3001,8 +3000,7 @@ TEST_F(QuicUnencryptedServerTransportTest, TestBadCleartextEncryption) { *aead, *getInitialHeaderCipher(), nextPacket); - EXPECT_CALL(*quicStats_, onPacketDropped(_)); - deliverData(std::move(packetData)); + EXPECT_THROW(deliverData(std::move(packetData)), std::runtime_error); // If crypto data was processed, we would have generated some writes. EXPECT_NE(server->getConn().readCodec, nullptr); EXPECT_TRUE(server->getConn().cryptoState->initialStream.writeBuffer.empty()); @@ -3231,6 +3229,40 @@ TEST_F(QuicUnencryptedServerTransportTest, TestNoAckOnlyCryptoInitial) { } } +TEST_F(QuicUnencryptedServerTransportTest, TestCorruptedDstCidInitialTest) { + auto chlo = folly::IOBuf::copyBuffer("CHLO"); + auto nextPacketNum = clientNextInitialPacketNum++; + + /* + * generate ciphers based off of initialDestinationConnectionId and include a + * different DstCid in the packet header. + */ + auto aead = getInitialCipher(); + auto headerCipher = getInitialHeaderCipher(); + auto corruptedDstCid = getTestConnectionId(1); + + EXPECT_TRUE(initialDestinationConnectionId); + EXPECT_TRUE(clientConnectionId); + EXPECT_NE(*initialDestinationConnectionId, corruptedDstCid); + + auto initialPacket = packetToBufCleartext( + createInitialCryptoPacket( + *clientConnectionId, + corruptedDstCid, + nextPacketNum, + QuicVersion::MVFST, + *chlo, + *aead, + 0 /* largestAcked */), + *aead, + *headerCipher, + nextPacketNum); + + EXPECT_CALL(routingCallback, onConnectionUnbound(_, _, _)).Times(1); + + EXPECT_THROW(deliverData(initialPacket->clone(), true), std::runtime_error); +} + TEST_F(QuicUnencryptedServerTransportTest, TestWriteHandshakeAndZeroRtt) { getFakeHandshakeLayer()->allowZeroRttKeys(); // This should trigger derivation of keys. @@ -3488,9 +3520,9 @@ TEST_F(QuicUnencryptedServerTransportTest, TestGarbageData) { auto headerCipher = getInitialHeaderCipher(); auto packet = createCryptoPacket( *clientConnectionId, - *clientConnectionId, + *initialDestinationConnectionId, nextPacket, - QuicVersion::QUIC_DRAFT, + QuicVersion::MVFST, ProtectionType::Initial, *IOBuf::copyBuffer("CHLO"), *aead,