Fix NULL Pointer Read in Stateless Retry Scenario (on Server) (#1951)

microsoft · Sep 20, 2021 · 6fa64ff · 6fa64ff
1 parent e92b41a
commit 6fa64ff
Show file tree

Hide file tree

Showing 12 changed files with 128 additions and 115 deletions.
diff --git a/src/core/connection.c b/src/core/connection.c
@@ -4451,30 +4451,7 @@ QuicConnRecvPostProcessing(
                 "First usage of SrcCid: %s",
                 QuicCidBufToStr(Packet->DestCid, Packet->DestCidLen).Buffer);
             SourceCid->CID.UsedByPeer = TRUE;
-            if (SourceCid->CID.IsInitial) {
-                if (QuicConnIsServer(Connection) && SourceCid->Link.Next != NULL) {
-                    QUIC_CID_HASH_ENTRY* NextSourceCid =
-                        QUIC_CONTAINING_RECORD(
-                            SourceCid->Link.Next,
-                            QUIC_CID_HASH_ENTRY,
-                            Link);
-                    if (NextSourceCid->CID.IsInitial) {
-                        //
-                        // The client has started using our new initial CID. We
-                        // can discard the old (client chosen) one now.
-                        //
-                        SourceCid->Link.Next = NextSourceCid->Link.Next;
-                        QUIC_DBG_ASSERT(!NextSourceCid->CID.IsInLookupTable);
-                        QuicTraceEvent(
-                            ConnSourceCidRemoved,
-                            "[conn][%p] (SeqNum=%llu) Removed Source CID: %!CID!",
-                            Connection,
-                            NextSourceCid->CID.SequenceNumber,
-                            CLOG_BYTEARRAY(NextSourceCid->CID.Length, NextSourceCid->CID.Data));
-                        QUIC_FREE(NextSourceCid, QUIC_POOL_CIDHASH);
-                    }
-                }
-            } else {
+            if (!SourceCid->CID.IsInitial) {
                 PeerUpdatedCid = TRUE;
             }
         }

diff --git a/src/core/crypto.c b/src/core/crypto.c
@@ -1383,6 +1383,30 @@ QuicCryptoProcessTlsCompletion(
                 "Handshake confirmed (server)");
             QuicSendSetSendFlag(&Connection->Send, QUIC_CONN_SEND_FLAG_HANDSHAKE_DONE);
             QuicCryptoHandshakeConfirmed(&Connection->Crypto);
+
+            //
+            // Take this opportinuty to clean up the client chosen initial CID.
+            // It will be the second one in the list.
+            //
+            CXPLAT_DBG_ASSERT(Connection->SourceCids.Next != NULL);
+            CXPLAT_DBG_ASSERT(Connection->SourceCids.Next->Next != NULL);
+            CXPLAT_DBG_ASSERT(Connection->SourceCids.Next->Next != NULL);
+            CXPLAT_DBG_ASSERT(Connection->SourceCids.Next->Next->Next == NULL);
+            QUIC_CID_HASH_ENTRY* InitialSourceCid =
+                CXPLAT_CONTAINING_RECORD(
+                    Connection->SourceCids.Next->Next,
+                    QUIC_CID_HASH_ENTRY,
+                    Link);
+            CXPLAT_DBG_ASSERT(InitialSourceCid->CID.IsInitial);
+            Connection->SourceCids.Next->Next = Connection->SourceCids.Next->Next->Next;
+            CXPLAT_DBG_ASSERT(!InitialSourceCid->CID.IsInLookupTable);
+            QuicTraceEvent(
+                ConnSourceCidRemoved,
+                "[conn][%p] (SeqNum=%llu) Removed Source CID: %!CID!",
+                Connection,
+                InitialSourceCid->CID.SequenceNumber,
+                CASTED_CLOG_BYTEARRAY(InitialSourceCid->CID.Length, InitialSourceCid->CID.Data));
+            CXPLAT_FREE(InitialSourceCid, QUIC_POOL_CIDHASH);
         }
 
         //

diff --git a/src/generated/linux/connection.c.clog.h b/src/generated/linux/connection.c.clog.h
@@ -3375,31 +3375,6 @@ tracepoint(CLOG_CONNECTION_C, ConnPacketRecv , arg2, arg3, arg4, arg5);\
 
 
 
-#ifndef _clog_6_ARGS_TRACE_ConnSourceCidRemoved
-
-
-
-/*----------------------------------------------------------
-// Decoder Ring for ConnSourceCidRemoved
-// [conn][%p] (SeqNum=%llu) Removed Source CID: %!CID!
-// QuicTraceEvent(
-                            ConnSourceCidRemoved,
-                            "[conn][%p] (SeqNum=%llu) Removed Source CID: %!CID!",
-                            Connection,
-                            NextSourceCid->CID.SequenceNumber,
-                            CLOG_BYTEARRAY(NextSourceCid->CID.Length, NextSourceCid->CID.Data));
-// arg2 = arg2 = Connection
-// arg3 = arg3 = NextSourceCid->CID.SequenceNumber
-// arg4 = arg4 = CLOG_BYTEARRAY(NextSourceCid->CID.Length, NextSourceCid->CID.Data)
-----------------------------------------------------------*/
-#define _clog_6_ARGS_TRACE_ConnSourceCidRemoved(uniqueId, encoded_arg_string, arg2, arg3, arg4, arg4_len)\
-tracepoint(CLOG_CONNECTION_C, ConnSourceCidRemoved , arg2, arg3, arg4_len, arg4);\
-
-#endif
-
-
-
-
 #ifndef _clog_5_ARGS_TRACE_ConnRemoteAddrAdded
 
 

diff --git a/src/generated/linux/connection.c.clog.h.lttng.h b/src/generated/linux/connection.c.clog.h.lttng.h
@@ -1800,35 +1800,6 @@ TRACEPOINT_EVENT(CLOG_CONNECTION_C, ConnPacketRecv,
 
 
 
-/*----------------------------------------------------------
-// Decoder Ring for ConnSourceCidRemoved
-// [conn][%p] (SeqNum=%llu) Removed Source CID: %!CID!
-// QuicTraceEvent(
-                            ConnSourceCidRemoved,
-                            "[conn][%p] (SeqNum=%llu) Removed Source CID: %!CID!",
-                            Connection,
-                            NextSourceCid->CID.SequenceNumber,
-                            CLOG_BYTEARRAY(NextSourceCid->CID.Length, NextSourceCid->CID.Data));
-// arg2 = arg2 = Connection
-// arg3 = arg3 = NextSourceCid->CID.SequenceNumber
-// arg4 = arg4 = CLOG_BYTEARRAY(NextSourceCid->CID.Length, NextSourceCid->CID.Data)
-----------------------------------------------------------*/
-TRACEPOINT_EVENT(CLOG_CONNECTION_C, ConnSourceCidRemoved,
-    TP_ARGS(
-        const void *, arg2,
-        unsigned long long, arg3,
-        unsigned int, arg4_len,
-        const void *, arg4), 
-    TP_FIELDS(
-        ctf_integer_hex(uint64_t, arg2, arg2)
-        ctf_integer(uint64_t, arg3, arg3)
-        ctf_integer(unsigned int, arg4_len, arg4_len)
-        ctf_sequence(char, arg4, arg4, unsigned int, arg4_len)
-    )
-)
-
-
-
 /*----------------------------------------------------------
 // Decoder Ring for ConnLocalAddrRemoved
 // [conn][%p] Removed Local IP: %!ADDR!

diff --git a/src/generated/linux/crypto.c.clog.h b/src/generated/linux/crypto.c.clog.h
@@ -619,6 +619,31 @@ tracepoint(CLOG_CRYPTO_C, ConnHandshakeComplete , arg2);\
 
 
 
+#ifndef _clog_6_ARGS_TRACE_ConnSourceCidRemoved
+
+
+
+/*----------------------------------------------------------
+// Decoder Ring for ConnSourceCidRemoved
+// [conn][%p] (SeqNum=%llu) Removed Source CID: %!CID!
+// QuicTraceEvent(
+                ConnSourceCidRemoved,
+                "[conn][%p] (SeqNum=%llu) Removed Source CID: %!CID!",
+                Connection,
+                InitialSourceCid->CID.SequenceNumber,
+                CASTED_CLOG_BYTEARRAY(InitialSourceCid->CID.Length, InitialSourceCid->CID.Data));
+// arg2 = arg2 = Connection
+// arg3 = arg3 = InitialSourceCid->CID.SequenceNumber
+// arg4 = arg4 = CASTED_CLOG_BYTEARRAY(InitialSourceCid->CID.Length, InitialSourceCid->CID.Data)
+----------------------------------------------------------*/
+#define _clog_6_ARGS_TRACE_ConnSourceCidRemoved(uniqueId, encoded_arg_string, arg2, arg3, arg4, arg4_len)\
+tracepoint(CLOG_CRYPTO_C, ConnSourceCidRemoved , arg2, arg3, arg4_len, arg4);\
+
+#endif
+
+
+
+
 #ifndef _clog_4_ARGS_TRACE_AllocFailure
 
 

diff --git a/src/generated/linux/crypto.c.clog.h.lttng.h b/src/generated/linux/crypto.c.clog.h.lttng.h
@@ -531,6 +531,35 @@ TRACEPOINT_EVENT(CLOG_CRYPTO_C, ConnHandshakeComplete,
 
 
 
+/*----------------------------------------------------------
+// Decoder Ring for ConnSourceCidRemoved
+// [conn][%p] (SeqNum=%llu) Removed Source CID: %!CID!
+// QuicTraceEvent(
+                ConnSourceCidRemoved,
+                "[conn][%p] (SeqNum=%llu) Removed Source CID: %!CID!",
+                Connection,
+                InitialSourceCid->CID.SequenceNumber,
+                CASTED_CLOG_BYTEARRAY(InitialSourceCid->CID.Length, InitialSourceCid->CID.Data));
+// arg2 = arg2 = Connection
+// arg3 = arg3 = InitialSourceCid->CID.SequenceNumber
+// arg4 = arg4 = CASTED_CLOG_BYTEARRAY(InitialSourceCid->CID.Length, InitialSourceCid->CID.Data)
+----------------------------------------------------------*/
+TRACEPOINT_EVENT(CLOG_CRYPTO_C, ConnSourceCidRemoved,
+    TP_ARGS(
+        const void *, arg2,
+        unsigned long long, arg3,
+        unsigned int, arg4_len,
+        const void *, arg4), 
+    TP_FIELDS(
+        ctf_integer_hex(uint64_t, arg2, arg2)
+        ctf_integer(uint64_t, arg3, arg3)
+        ctf_integer(unsigned int, arg4_len, arg4_len)
+        ctf_sequence(char, arg4, arg4, unsigned int, arg4_len)
+    )
+)
+
+
+
 /*----------------------------------------------------------
 // Decoder Ring for ConnNewPacketKeys
 // [conn][%p] New packet keys created successfully.

diff --git a/src/manifest/clog.sidecar b/src/manifest/clog.sidecar
@@ -1698,26 +1698,6 @@
       ],
       "macroName": "QuicTraceEvent"
     },
-    "ConnSourceCidRemoved": {
-      "ModuleProperites": {},
-      "TraceString": "[conn][%p] (SeqNum=%llu) Removed Source CID: %!CID!",
-      "UniqueId": "ConnSourceCidRemoved",
-      "splitArgs": [
-        {
-          "DefinationEncoding": "p",
-          "MacroVariableName": "arg2"
-        },
-        {
-          "DefinationEncoding": "llu",
-          "MacroVariableName": "arg3"
-        },
-        {
-          "DefinationEncoding": "!CID!",
-          "MacroVariableName": "arg4"
-        }
-      ],
-      "macroName": "QuicTraceEvent"
-    },
     "ConnLocalAddrRemoved": {
       "ModuleProperites": {},
       "TraceString": "[conn][%p] Removed Local IP: %!ADDR!",
@@ -1910,6 +1890,26 @@
       ],
       "macroName": "QuicTraceEvent"
     },
+    "ConnSourceCidRemoved": {
+      "ModuleProperites": {},
+      "TraceString": "[conn][%p] (SeqNum=%llu) Removed Source CID: %!CID!",
+      "UniqueId": "ConnSourceCidRemoved",
+      "splitArgs": [
+        {
+          "DefinationEncoding": "p",
+          "MacroVariableName": "arg2"
+        },
+        {
+          "DefinationEncoding": "llu",
+          "MacroVariableName": "arg3"
+        },
+        {
+          "DefinationEncoding": "!CID!",
+          "MacroVariableName": "arg4"
+        }
+      ],
+      "macroName": "QuicTraceEvent"
+    },
     "IgnoreCryptoFrame": {
       "ModuleProperites": {},
       "TraceString": "[conn][%p] Ignoring received crypto after cleanup",
@@ -10379,10 +10379,6 @@
         "UniquenessHash": "af137df5-1055-7424-de16-65895d8d276b",
         "TraceID": "ConnPacketRecv"
       },
-      {
-        "UniquenessHash": "65f1ff8d-810a-4f24-b2f6-817daf9275ef",
-        "TraceID": "ConnSourceCidRemoved"
-      },
       {
         "UniquenessHash": "23378f35-794d-1493-0f86-0c338e1bf37c",
         "TraceID": "ConnLocalAddrRemoved"
@@ -10411,6 +10407,10 @@
         "UniquenessHash": "11964356-1a57-bc4d-0637-9300c14e0cb9",
         "TraceID": "ConnOutFlowBlocked"
       },
+      {
+        "UniquenessHash": "65f1ff8d-810a-4f24-b2f6-817daf9275ef",
+        "TraceID": "ConnSourceCidRemoved"
+      },
       {
         "UniquenessHash": "60d753ab-6710-fe16-e47d-37f046f5973c",
         "TraceID": "IgnoreCryptoFrame"

diff --git a/src/test/MsQuicTests.h b/src/test/MsQuicTests.h
@@ -60,13 +60,19 @@ void QuicTestBindConnectionExplicit(_In_ int Family);
 // Handshake Tests
 //
 
+typedef enum QUIC_TEST_ASYNC_CONFIG_MODE {
+    QUIC_TEST_ASYNC_CONFIG_DISABLED,
+    QUIC_TEST_ASYNC_CONFIG_ENABLED,
+    QUIC_TEST_ASYNC_CONFIG_DELAYED,
+} QUIC_TEST_ASYNC_CONFIG_MODE;
+
 void
 QuicTestConnect(
     _In_ int Family,
     _In_ bool ServerStatelessRetry,
     _In_ bool ClientUsesOldVersion,
     _In_ bool MultipleALPNs,
-    _In_ bool AsyncConfiguration,
+    _In_ QUIC_TEST_ASYNC_CONFIG_MODE AsyncConfiguration,
     _In_ bool MultiPacketClientInitial,
     _In_ bool SessionResumption,
     _In_ uint8_t RandomLossPercentage // 0 to 100

diff --git a/src/test/bin/quic_gtest.cpp b/src/test/bin/quic_gtest.cpp
@@ -310,7 +310,7 @@ TEST_P(WithHandshakeArgs1, Connect) {
             (uint8_t)GetParam().ServerStatelessRetry,
             0,  // ClientUsesOldVersion
             (uint8_t)GetParam().MultipleALPNs,
-            0,  // AsyncConfiguration
+            QUIC_TEST_ASYNC_CONFIG_DISABLED,
             (uint8_t)GetParam().MultiPacketClientInitial,
             (uint8_t)GetParam().SessionResumption,
             0   // RandomLossPercentage
@@ -322,7 +322,7 @@ TEST_P(WithHandshakeArgs1, Connect) {
             GetParam().ServerStatelessRetry,
             false,  // ClientUsesOldVersion
             GetParam().MultipleALPNs,
-            false,  // AsyncConfiguration
+            QUIC_TEST_ASYNC_CONFIG_DISABLED,
             GetParam().MultiPacketClientInitial,
             GetParam().SessionResumption,
             0);     // RandomLossPercentage
@@ -337,7 +337,7 @@ TEST_P(WithHandshakeArgs2, OldVersion) {
             (uint8_t)GetParam().ServerStatelessRetry,
             1,  // ClientUsesOldVersion
             0,  // MultipleALPNs
-            0,  // AsyncConfiguration
+            QUIC_TEST_ASYNC_CONFIG_DISABLED,
             0,  // MultiPacketClientInitial
             0,  // SessionResumption
             0   // RandomLossPercentage
@@ -349,7 +349,7 @@ TEST_P(WithHandshakeArgs2, OldVersion) {
             GetParam().ServerStatelessRetry,
             false,  // ClientUsesOldVersion
             false,  // MultipleALPNs
-            false,  // AsyncConfiguration
+            QUIC_TEST_ASYNC_CONFIG_DISABLED,
             false,  // MultiPacketClientInitial
             false,  // SessionResumption
             0);     // RandomLossPercentage
@@ -364,7 +364,7 @@ TEST_P(WithHandshakeArgs3, AsyncSecurityConfig) {
             (uint8_t)GetParam().ServerStatelessRetry,
             0,  // ClientUsesOldVersion
             (uint8_t)GetParam().MultipleALPNs,
-            1,  // AsyncConfiguration
+            GetParam().DelayedAsyncConfig ? (uint8_t)QUIC_TEST_ASYNC_CONFIG_DELAYED : (uint8_t)QUIC_TEST_ASYNC_CONFIG_ENABLED,
             0,  // MultiPacketClientInitial
             0,  // SessionResumption
             0   // RandomLossPercentage
@@ -376,7 +376,7 @@ TEST_P(WithHandshakeArgs3, AsyncSecurityConfig) {
             GetParam().ServerStatelessRetry,
             false,  // ClientUsesOldVersion
             GetParam().MultipleALPNs,
-            true,   // AsyncConfiguration
+            GetParam().DelayedAsyncConfig ? QUIC_TEST_ASYNC_CONFIG_DELAYED : QUIC_TEST_ASYNC_CONFIG_ENABLED,
             false,  // MultiPacketClientInitial
             false,  // SessionResumption
             0);     // RandomLossPercentage
@@ -401,7 +401,7 @@ TEST_P(WithHandshakeArgs4, RandomLoss) {
             (uint8_t)GetParam().ServerStatelessRetry,
             0,  // ClientUsesOldVersion
             0,  // MultipleALPNs
-            0,  // AsyncConfiguration
+            QUIC_TEST_ASYNC_CONFIG_DISABLED,
             (uint8_t)GetParam().MultiPacketClientInitial,
             (uint8_t)GetParam().SessionResumption,
             GetParam().RandomLossPercentage
@@ -413,7 +413,7 @@ TEST_P(WithHandshakeArgs4, RandomLoss) {
             GetParam().ServerStatelessRetry,
             false,  // ClientUsesOldVersion
             false,  // MultipleALPNs,
-            false,  // AsyncConfiguration
+            QUIC_TEST_ASYNC_CONFIG_DISABLED,
             GetParam().MultiPacketClientInitial,
             GetParam().SessionResumption,
             GetParam().RandomLossPercentage);

diff --git a/src/test/bin/quic_gtest.h b/src/test/bin/quic_gtest.h
@@ -102,12 +102,14 @@ struct HandshakeArgs3 {
     int Family;
     bool ServerStatelessRetry;
     bool MultipleALPNs;
+    bool DelayedAsyncConfig;
     static ::std::vector<HandshakeArgs3> Generate() {
         ::std::vector<HandshakeArgs3> list;
         for (int Family : { 4, 6})
         for (bool ServerStatelessRetry : { false, true })
         for (bool MultipleALPNs : { false, true })
-            list.push_back({ Family, ServerStatelessRetry, MultipleALPNs });
+        for (bool DelayedAsyncConfig : { false, true })
+            list.push_back({ Family, ServerStatelessRetry, MultipleALPNs, DelayedAsyncConfig });
         return list;
     }
 };
@@ -116,7 +118,8 @@ std::ostream& operator << (std::ostream& o, const HandshakeArgs3& args) {
     return o <<
         (args.Family == 4 ? "v4" : "v6") << "/" <<
         (args.ServerStatelessRetry ? "Retry" : "NoRetry") << "/" <<
-        (args.MultipleALPNs ? "MultipleALPNs" : "SingleALPN");
+        (args.MultipleALPNs ? "MultipleALPNs" : "SingleALPN") << "/" <<
+        (args.DelayedAsyncConfig ? "DelayedAsyncConfig" : "AsyncConfig");
 }
 
 class WithHandshakeArgs3 : public testing::Test,