Add tests for invalid-sized version_info transport parameter. (#3076)

* Add tests for invalid-sized version_info transport parameter. * Fix OpenSSL to read server TP as soon as its ready. * Add new compile flag, only run test on debug builds.
microsoft · Sep 29, 2022 · 8bb5728 · 8bb5728
1 parent 4c4d065
commit 8bb5728
Show file tree

Hide file tree

Showing 17 changed files with 327 additions and 66 deletions.
diff --git a/src/core/connection.c b/src/core/connection.c
@@ -2344,7 +2344,11 @@ QuicConnGenerateLocalTransportParameters(
         LocalTP->CibirOffset = Connection->CibirId[1];
     }
 
-    if (Connection->Settings.VersionNegotiationExtEnabled) {
+    if (Connection->Settings.VersionNegotiationExtEnabled
+#if QUIC_TEST_DISABLE_VNE_TP_GENERATION
+        && !Connection->State.DisableVneTp
+#endif
+        ) {
         uint32_t VersionInfoLength = 0;
         LocalTP->VersionInfo =
             QuicVersionNegotiationExtEncodeVersionInfo(Connection, &VersionInfoLength);
@@ -5383,10 +5387,19 @@ QuicConnRecvDatagramBatch(
         CXPLAT_ECN_TYPE ECN = CXPLAT_ECN_FROM_TOS(Datagrams[i]->TypeOfService);
         Packet = CxPlatDataPathRecvDataToRecvPacket(Datagrams[i]);
         CXPLAT_DBG_ASSERT(Packet->PacketId != 0);
-        if (QuicConnRecvPrepareDecrypt(
-                Connection, Packet, HpMask + i * CXPLAT_HP_SAMPLE_LENGTH) &&
-            QuicConnRecvDecryptAndAuthenticate(Connection, Path, Packet) &&
-            QuicConnRecvFrames(Connection, Path, Packet, ECN)) {
+        if (!QuicConnRecvPrepareDecrypt(
+                Connection, Packet, HpMask + i * CXPLAT_HP_SAMPLE_LENGTH) ||
+            !QuicConnRecvDecryptAndAuthenticate(Connection, Path, Packet)) {
+            if (Connection->State.CompatibleVerNegotiationAttempted &&
+                !Connection->State.CompatibleVerNegotiationCompleted) {
+                //
+                // The packet which initiated compatible version negotation failed
+                // decryption, so undo the version change.
+                //
+                Connection->Stats.QuicVersion = Connection->OriginalQuicVersion;
+                Connection->State.CompatibleVerNegotiationAttempted = FALSE;
+            }
+        } else if (QuicConnRecvFrames(Connection, Path, Packet, ECN)) {
 
             QuicConnRecvPostProcessing(Connection, &Path, Packet);
             RecvState->ResetIdleTimeout |= Packet->CompletelyValid;
@@ -5407,17 +5420,6 @@ QuicConnRecvDatagramBatch(
                     Path->SpinBit = !Packet->SH->SpinBit;
                 }
             }
-
-        } else {
-            if (Connection->State.CompatibleVerNegotiationAttempted &&
-                !Connection->State.CompatibleVerNegotiationCompleted) {
-                //
-                // The packet which initiated compatible version negotation failed
-                // decryption, so undo the version change.
-                //
-                Connection->Stats.QuicVersion = Connection->OriginalQuicVersion;
-                Connection->State.CompatibleVerNegotiationAttempted = FALSE;
-            }
         }
     }
 }
@@ -6561,7 +6563,7 @@ QuicConnParamSet(
         QuicTraceLogConnVerbose(
             TestTPSet,
             Connection,
-            "Setting Test Transport Parameter (type %hu, %hu bytes)",
+            "Setting Test Transport Parameter (type %x, %hu bytes)",
             Connection->TestTransportParameter.Type,
             Connection->TestTransportParameter.Length);
 
@@ -6579,6 +6581,19 @@ QuicConnParamSet(
         Status = QUIC_STATUS_SUCCESS;
         break;
 
+#if QUIC_TEST_DISABLE_VNE_TP_GENERATION
+    case QUIC_PARAM_CONN_DISABLE_VNE_TP_GENERATION:
+
+        if (BufferLength != sizeof(BOOLEAN) || Buffer == NULL) {
+            Status = QUIC_STATUS_INVALID_PARAMETER;
+            break;
+        }
+
+        Connection->State.DisableVneTp = *(BOOLEAN*)Buffer;
+        Status = QUIC_STATUS_SUCCESS;
+        break;
+#endif
+
     default:
         Status = QUIC_STATUS_INVALID_PARAMETER;
         break;

diff --git a/src/core/connection.h b/src/core/connection.h
@@ -186,6 +186,14 @@ typedef union QUIC_CONNECTION_STATE {
         //
         BOOLEAN IsVerifying : 1;
 #endif
+
+#if QUIC_TEST_DISABLE_VNE_TP_GENERATION
+        //
+        // Whether to disable automatic generation of VNE transport parameter.
+        // Only used for testing, and thus only enabled for debug builds.
+        //
+        BOOLEAN DisableVneTp : 1;
+#endif
     };
 } QUIC_CONNECTION_STATE;
 

diff --git a/src/core/crypto.c b/src/core/crypto.c
@@ -1798,7 +1798,7 @@ QuicCryptoProcessData(
 
             Status =
                 QuicConnProcessPeerTransportParameters(Connection, FALSE);
-            if (Status == QUIC_STATUS_VER_NEG_ERROR) {
+            if (QUIC_FAILED(Status)) {
                 //
                 // Communicate error up the stack to perform Incompatible
                 // Version Negotiation.

diff --git a/src/core/crypto_tls.c b/src/core/crypto_tls.c
@@ -1743,34 +1743,27 @@ QuicCryptoTlsDecodeTransportParameters( // NOLINT(readability-function-size, goo
             break;
 
         case QUIC_TP_ID_VERSION_NEGOTIATION_EXT:
-            if (Length < MIN_VERSION_INFO_LENGTH) {
-                QuicTraceEvent(
-                    ConnErrorStatus,
-                    "[conn][%p] ERROR, %u, %s.",
-                    Connection,
-                    Length,
-                    "Invalid length of QUIC_TP_ID_VERSION_NEGOTIATION_EXT");
-                goto Exit;
-            }
-            TransportParams->VersionInfo = CXPLAT_ALLOC_NONPAGED(Length, QUIC_POOL_VERSION_INFO);
-            if (TransportParams->VersionInfo == NULL) {
-                QuicTraceEvent(
-                    AllocFailure,
-                    "Allocation of '%s' failed. (%llu bytes)",
-                    IsServerTP ?
-                        "Received Client Version Negotiation Info" :
-                        "Received Server Version Negotiation Info",
-                    Length);
-            } else {
-                TransportParams->Flags |= QUIC_TP_FLAG_VERSION_NEGOTIATION;
+            if (Length > 0) {
+                TransportParams->VersionInfo = CXPLAT_ALLOC_NONPAGED(Length, QUIC_POOL_VERSION_INFO);
+                if (TransportParams->VersionInfo == NULL) {
+                    QuicTraceEvent(
+                        AllocFailure,
+                        "Allocation of '%s' failed. (%llu bytes)",
+                        "Version Negotiation Info",
+                        Length);
+                    break;
+                }
                 CxPlatCopyMemory((uint8_t*)TransportParams->VersionInfo, TPBuf + Offset, Length);
-                TransportParams->VersionInfoLength = Length;
-                QuicTraceLogConnVerbose(
-                    DecodeTPVersionNegotiationInfo,
-                    Connection,
-                    "TP: Version Negotiation Info (%hu bytes)",
-                    Length);
+            } else {
+                TransportParams->VersionInfo = NULL;
             }
+            TransportParams->Flags |= QUIC_TP_FLAG_VERSION_NEGOTIATION;
+            TransportParams->VersionInfoLength = Length;
+            QuicTraceLogConnVerbose(
+                DecodeTPVersionNegotiationInfo,
+                Connection,
+                "TP: Version Negotiation Info (%hu bytes)",
+                Length);
             break;
 
         case QUIC_TP_ID_MIN_ACK_DELAY:
@@ -1891,8 +1884,10 @@ QuicCryptoTlsCleanupTransportParameters(
     )
 {
     if (TransportParams->Flags & QUIC_TP_FLAG_VERSION_NEGOTIATION) {
-        CXPLAT_FREE(TransportParams->VersionInfo, QUIC_POOL_VERSION_INFO);
-        TransportParams->VersionInfo = NULL;
+        if (TransportParams->VersionInfo != NULL) {
+            CXPLAT_FREE(TransportParams->VersionInfo, QUIC_POOL_VERSION_INFO);
+            TransportParams->VersionInfo = NULL;
+        }
         TransportParams->VersionInfoLength = 0;
         TransportParams->Flags &= ~QUIC_TP_FLAG_VERSION_NEGOTIATION;
     }

diff --git a/src/generated/linux/connection.c.clog.h b/src/generated/linux/connection.c.clog.h
@@ -1410,11 +1410,11 @@ tracepoint(CLOG_CONNECTION_C, ForceCidUpdate , arg1);\
 
 /*----------------------------------------------------------
 // Decoder Ring for TestTPSet
-// [conn][%p] Setting Test Transport Parameter (type %hu, %hu bytes)
+// [conn][%p] Setting Test Transport Parameter (type %x, %hu bytes)
 // QuicTraceLogConnVerbose(
             TestTPSet,
             Connection,
-            "Setting Test Transport Parameter (type %hu, %hu bytes)",
+            "Setting Test Transport Parameter (type %x, %hu bytes)",
             Connection->TestTransportParameter.Type,
             Connection->TestTransportParameter.Length);
 // arg1 = arg1 = Connection = arg1

diff --git a/src/generated/linux/connection.c.clog.h.lttng.h b/src/generated/linux/connection.c.clog.h.lttng.h
@@ -1550,11 +1550,11 @@ TRACEPOINT_EVENT(CLOG_CONNECTION_C, ForceCidUpdate,
 
 /*----------------------------------------------------------
 // Decoder Ring for TestTPSet
-// [conn][%p] Setting Test Transport Parameter (type %hu, %hu bytes)
+// [conn][%p] Setting Test Transport Parameter (type %x, %hu bytes)
 // QuicTraceLogConnVerbose(
             TestTPSet,
             Connection,
-            "Setting Test Transport Parameter (type %hu, %hu bytes)",
+            "Setting Test Transport Parameter (type %x, %hu bytes)",
             Connection->TestTransportParameter.Type,
             Connection->TestTransportParameter.Length);
 // arg1 = arg1 = Connection = arg1
@@ -1564,11 +1564,11 @@ TRACEPOINT_EVENT(CLOG_CONNECTION_C, ForceCidUpdate,
 TRACEPOINT_EVENT(CLOG_CONNECTION_C, TestTPSet,
     TP_ARGS(
         const void *, arg1,
-        unsigned short, arg3,
+        unsigned int, arg3,
         unsigned short, arg4), 
     TP_FIELDS(
         ctf_integer_hex(uint64_t, arg1, arg1)
-        ctf_integer(unsigned short, arg3, arg3)
+        ctf_integer(unsigned int, arg3, arg3)
         ctf_integer(unsigned short, arg4, arg4)
     )
 )

diff --git a/src/generated/linux/crypto_tls.c.clog.h b/src/generated/linux/crypto_tls.c.clog.h
@@ -1061,10 +1061,10 @@ tracepoint(CLOG_CRYPTO_TLS_C, DecodeTPDisable1RttEncryption , arg1);\
 // Decoder Ring for DecodeTPVersionNegotiationInfo
 // [conn][%p] TP: Version Negotiation Info (%hu bytes)
 // QuicTraceLogConnVerbose(
-                    DecodeTPVersionNegotiationInfo,
-                    Connection,
-                    "TP: Version Negotiation Info (%hu bytes)",
-                    Length);
+                DecodeTPVersionNegotiationInfo,
+                Connection,
+                "TP: Version Negotiation Info (%hu bytes)",
+                Length);
 // arg1 = arg1 = Connection = arg1
 // arg3 = arg3 = Length = arg3
 ----------------------------------------------------------*/

diff --git a/src/generated/linux/crypto_tls.c.clog.h.lttng.h b/src/generated/linux/crypto_tls.c.clog.h.lttng.h
@@ -1179,10 +1179,10 @@ TRACEPOINT_EVENT(CLOG_CRYPTO_TLS_C, DecodeTPDisable1RttEncryption,
 // Decoder Ring for DecodeTPVersionNegotiationInfo
 // [conn][%p] TP: Version Negotiation Info (%hu bytes)
 // QuicTraceLogConnVerbose(
-                    DecodeTPVersionNegotiationInfo,
-                    Connection,
-                    "TP: Version Negotiation Info (%hu bytes)",
-                    Length);
+                DecodeTPVersionNegotiationInfo,
+                Connection,
+                "TP: Version Negotiation Info (%hu bytes)",
+                Length);
 // arg1 = arg1 = Connection = arg1
 // arg3 = arg3 = Length = arg3
 ----------------------------------------------------------*/

diff --git a/src/inc/msquicp.h b/src/inc/msquicp.h
@@ -84,6 +84,12 @@ typedef struct QUIC_TEST_DATAPATH_HOOKS {
 // Allocation failures are currently only enabled on debug builds.
 //
 #define QUIC_TEST_ALLOC_FAILURES_ENABLED 1
+
+//
+// Enable support to disable automatic generation of the version
+// negotiation transport parameter.
+//
+#define QUIC_TEST_DISABLE_VNE_TP_GENERATION 1
 #endif
 
 typedef struct QUIC_PRIVATE_TRANSPORT_PARAMETER {
@@ -123,6 +129,7 @@ typedef struct QUIC_PRIVATE_TRANSPORT_PARAMETER {
 #define QUIC_PARAM_CONN_FORCE_CID_UPDATE                0x85000001  // No payload
 #define QUIC_PARAM_CONN_TEST_TRANSPORT_PARAMETER        0x85000002  // QUIC_PRIVATE_TRANSPORT_PARAMETER
 #define QUIC_PARAM_CONN_KEEP_ALIVE_PADDING              0x85000003  // uint16_t
+#define QUIC_PARAM_CONN_DISABLE_VNE_TP_GENERATION       0x85000004  // BOOLEAN
 
 #if defined(__cplusplus)
 }

diff --git a/src/manifest/clog.sidecar b/src/manifest/clog.sidecar
@@ -10585,15 +10585,15 @@
     },
     "TestTPSet": {
       "ModuleProperites": {},
-      "TraceString": "[conn][%p] Setting Test Transport Parameter (type %hu, %hu bytes)",
+      "TraceString": "[conn][%p] Setting Test Transport Parameter (type %x, %hu bytes)",
       "UniqueId": "TestTPSet",
       "splitArgs": [
         {
           "DefinationEncoding": "p",
           "MacroVariableName": "arg1"
         },
         {
-          "DefinationEncoding": "hu",
+          "DefinationEncoding": "x",
           "MacroVariableName": "arg3"
         },
         {
@@ -15091,9 +15091,9 @@
         "EncodingString": "[test]<--- %s"
       },
       {
-        "UniquenessHash": "ad3632a2-ac75-293d-854f-381b59368114",
+        "UniquenessHash": "d35927ed-5967-1f8a-2604-5660d1911c16",
         "TraceID": "TestTPSet",
-        "EncodingString": "[conn][%p] Setting Test Transport Parameter (type %hu, %hu bytes)"
+        "EncodingString": "[conn][%p] Setting Test Transport Parameter (type %x, %hu bytes)"
       },
       {
         "UniquenessHash": "db41669b-3e8f-05ca-4ac2-2c194cce7b51",

diff --git a/src/platform/tls_openssl.c b/src/platform/tls_openssl.c
@@ -107,6 +107,11 @@ typedef struct CXPLAT_TLS {
     //
     BOOLEAN PeerCertReceived : 1;
 
+    //
+    // Indicates whether the peer's TP has been received.
+    //
+    BOOLEAN PeerTPReceived : 1;
+
     //
     // The TLS extension type for the QUIC transport parameters.
     //
@@ -1933,6 +1938,25 @@ CxPlatTlsProcessData(
             switch (Err) {
             case SSL_ERROR_WANT_READ:
             case SSL_ERROR_WANT_WRITE:
+                //
+                // Best effort to get the server's transport params as early as possible.
+                //
+                if (!TlsContext->IsServer && TlsContext->PeerTPReceived == FALSE) {
+                    const uint8_t* TransportParams;
+                    size_t TransportParamLen;
+                    SSL_get_peer_quic_transport_params(
+                            TlsContext->Ssl, &TransportParams, &TransportParamLen);
+                    if (TransportParams != NULL && TransportParamLen != 0) {
+                        TlsContext->PeerTPReceived = TRUE;
+                        if (!TlsContext->SecConfig->Callbacks.ReceiveTP(
+                                TlsContext->Connection,
+                                (uint16_t)TransportParamLen,
+                                TransportParams)) {
+                            TlsContext->ResultFlags |= CXPLAT_TLS_RESULT_ERROR;
+                            goto Exit;
+                        }
+                    }
+                }
                 goto Exit;
 
             case SSL_ERROR_SSL: {
@@ -2054,7 +2078,10 @@ CxPlatTlsProcessData(
         if (TlsContext->IsServer) {
             TlsContext->State->ReadKey = QUIC_PACKET_KEY_1_RTT;
             TlsContext->ResultFlags |= CXPLAT_TLS_RESULT_READ_KEY_UPDATED;
-        } else {
+        } else if (!TlsContext->PeerTPReceived) {
+            //
+            // Last chance to get the server's transport params.
+            //
             const uint8_t* TransportParams;
             size_t TransportParamLen;
             SSL_get_peer_quic_transport_params(
@@ -2067,6 +2094,7 @@ CxPlatTlsProcessData(
                 TlsContext->ResultFlags |= CXPLAT_TLS_RESULT_ERROR;
                 goto Exit;
             }
+            TlsContext->PeerTPReceived = TRUE;
             if (!TlsContext->SecConfig->Callbacks.ReceiveTP(
                     TlsContext->Connection,
                     (uint16_t)TransportParamLen,

diff --git a/src/test/MsQuicTests.h b/src/test/MsQuicTests.h
@@ -285,6 +285,12 @@ QuicTestClientBlockedSourcePort(
     _In_ int Family
     );
 
+void
+QuicTestOddSizeVNTP(
+    _In_ bool TestServer,
+    _In_ uint16_t VNTPSize
+    );
+
 //
 // Post Handshake Tests
 //
@@ -1088,4 +1094,12 @@ typedef struct {
 #define IOCTL_QUIC_RUN_CHANGE_ALPN \
     QUIC_CTL_CODE(102, METHOD_BUFFERED, FILE_WRITE_DATA)
 
-#define QUIC_MAX_IOCTL_FUNC_CODE 102
+typedef struct {
+    BOOLEAN TestServer;
+    uint8_t VnTpSize;
+} QUIC_RUN_ODD_SIZE_VN_TP_PARAMS;
+
+#define IOCTL_QUIC_RUN_ODD_SIZE_VN_TP \
+    QUIC_CTL_CODE(103, METHOD_BUFFERED, FILE_WRITE_DATA)
+
+#define QUIC_MAX_IOCTL_FUNC_CODE 103