CVE-2021-24029: Reachable Assertion (CWE-617)
value: A packet of death scenario is possible in mvfst via a specially crafted message during a QUIC session, which causes a crash via a failed assertion. Per QUIC specification, this particular message should be treated as a connection error. This issue affects mvfst versions prior to commit a67083ff4b8dcbb7ee2839da6338032030d712b0 and proxygen versions prior to v2021.03.15.00.
CWE description: The product contains an assert() or similar statement that can be triggered by an attacker, which leads to an application exit or other behavior that is more severe than necessary.
CVSS v3 base score: 7.5
Published: 2021-03-15T22:15Z
Repository: https://github.com/facebookincubator/mvfst
Language: C++
Files changed: quic/server/state/ServerStateMachine.cpp, quic/server/test/QuicServerTransportTest.cpp
Commit date: 2021-03-03 07:26:26 -0800
Commit message: Close connection if we derive an extra 1-rtt write cipher

Summary: Fixes CVE-2021-24029

Reviewed By: mjoras, lnicco

Differential Revision: D26613890

fbshipit-source-id: 19bb2be2c731808144e1a074ece313fba11f1945

Diff: diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp
index a5f42f8e..eaa609e5 100644
--- a/quic/server/state/ServerStateMachine.cpp
+++ b/quic/server/state/ServerStateMachine.cpp
@@ -309,11 +309,14 @@ void updateHandshakeState(QuicServerConnectionState& conn) {
   if (oneRttWriteCipher) {
     if (conn.qLogger) {
       conn.qLogger->addTransportStateUpdate(kDerivedOneRttWriteCipher);
     }
     QUIC_TRACE(fst_trace, conn, "derived 1-rtt write cipher");
-    CHECK(!conn.oneRttWriteCipher.get());
+    if (conn.oneRttWriteCipher) {
+      throw QuicTransportException(
+          "Duplicate 1-rtt write cipher", TransportErrorCode::CRYPTO_ERROR);
+    }
     conn.oneRttWriteCipher = std::move(oneRttWriteCipher);
 
     updatePacingOnKeyEstablished(conn);
 
     // We negotiate the transport parameters whenever we have the 1-RTT write
diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp
index 07e22bcf..404a78b8 100644
--- a/quic/server/test/QuicServerTransportTest.cpp
+++ b/quic/server/test/QuicServerTransportTest.cpp
@@ -4263,10 +4263,25 @@ TEST_P(QuicServerTransportHandshakeTest, TestD6DStartCallback) {
   // CHLO should be enough to trigger probing
   recvClientHello();
   server->removeObserver(mockObserver.get());
 }
 
+TEST_F(QuicUnencryptedServerTransportTest, DuplicateOneRttWriteCipher) {
+  setupClientReadCodec();
+  recvClientHello();
+  recvClientFinished();
+  loopForWrites();
+  try {
+    recvClientHello();
+    recvClientFinished();
+    FAIL();
+  } catch (const std::runtime_error& ex) {
+    EXPECT_THAT(ex.what(), HasSubstr("Crypto error"));
+  }
+  EXPECT_TRUE(server->isClosed());
+}
+
 TEST_F(QuicServerTransportTest, TestRegisterAndHandleTransportKnobParams) {
   int flag = 0;
   server->registerKnobParamHandler(
       199, [&](QuicServerConnectionState* /* server_conn */, uint64_t val) {
         EXPECT_EQ(val, 10);

Vulnerable code:     CHECK(!conn.oneRttWriteCipher.get());
Fixed code:     if (conn.oneRttWriteCipher) {
      throw QuicTransportException(
          "Duplicate 1-rtt write cipher", TransportErrorCode::CRYPTO_ERROR);
    }
TEST_F(QuicUnencryptedServerTransportTest, DuplicateOneRttWriteCipher) {
  setupClientReadCodec();
  recvClientHello();
  recvClientFinished();
  loopForWrites();
  try {
    recvClientHello();
    recvClientFinished();
    FAIL();
  } catch (const std::runtime_error& ex) {
    EXPECT_THAT(ex.what(), HasSubstr("Crypto error"));
  }
  EXPECT_TRUE(server->isClosed());
}