NetworkPkg TcpDxe: SECURITY PATCH CVE-2023-45236

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4541
REF: https://www.rfc-editor.org/rfc/rfc1948.txt
REF: https://www.rfc-editor.org/rfc/rfc6528.txt
REF: https://www.rfc-editor.org/rfc/rfc9293.txt

Bug Overview:
PixieFail Bug #8
CVE-2023-45236
CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:C/C:L/I:N/A:N
CWE-200 Exposure of Sensitive Information to an Unauthorized Actor

Updates TCP ISN generation to use a cryptographic hash of the
connection's identifying parameters and a secret key.
This prevents an attacker from guessing the ISN used for some other
connection.

This is follows the guidance in RFC 1948, RFC 6528, and RFC 9293.

RFC: 9293 Section 3.4.1.  Initial Sequence Number Selection

   A TCP implementation MUST use the above type of "clock" for clock-
   driven selection of initial sequence numbers (MUST-8), and SHOULD
   generate its initial sequence numbers with the expression:

   ISN = M + F(localip, localport, remoteip, remoteport, secretkey)

   where M is the 4 microsecond timer, and F() is a pseudorandom
   function (PRF) of the connection's identifying parameters ("localip,
   localport, remoteip, remoteport") and a secret key ("secretkey")
   (SHLD-1).  F() MUST NOT be computable from the outside (MUST-9), or
   an attacker could still guess at sequence numbers from the ISN used
   for some other connection.  The PRF could be implemented as a
   cryptographic hash of the concatenation of the TCP connection
   parameters and some secret data.  For discussion of the selection of
   a specific hash algorithm and management of the secret key data,
   please see Section 3 of [42].

   For each connection there is a send sequence number and a receive
   sequence number.  The initial send sequence number (ISS) is chosen by
   the data sending TCP peer, and the initial receive sequence number
   (IRS) is learned during the connection-establishing procedure.

   For a connection to be established or initialized, the two TCP peers
   must synchronize on each other's initial sequence numbers.  This is
   done in an exchange of connection-establishing segments carrying a
   control bit called "SYN" (for synchronize) and the initial sequence
   numbers.  As a shorthand, segments carrying the SYN bit are also
   called "SYNs".  Hence, the solution requires a suitable mechanism for
   picking an initial sequence number and a slightly involved handshake
   to exchange the ISNs.

Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>

Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>
Reviewed-by: Saloni Kasbekar <saloni.kasbekar@intel.com>

NetworkPkg/SecurityFixes.yaml

@@ -122,6 +122,28 @@ CVE_2023_45235:
     - http://www.openwall.com/lists/oss-security/2024/01/16/2
     - http://packetstormsecurity.com/files/176574/PixieFail-Proof-Of-Concepts.html
     - https://blog.quarkslab.com/pixiefail-nine-vulnerabilities-in-tianocores-edk-ii-ipv6-network-stack.html
+CVE_2023_45236:
+  commit_titles:
+    - "NetworkPkg: TcpDxe: SECURITY PATCH CVE-2023-45236 Patch"
+  cve: CVE-2023-45236
+  date_reported: 2023-08-28 13:56 UTC
+  description: "Bug 08 - edk2/NetworkPkg: Predictable TCP Initial Sequence Numbers"
+  note:
+  files_impacted:
+    - NetworkPkg/Include/Library/NetLib.h
+    - NetworkPkg/TcpDxe/TcpDriver.c
+    - NetworkPkg/TcpDxe/TcpDxe.inf
+    - NetworkPkg/TcpDxe/TcpFunc.h
+    - NetworkPkg/TcpDxe/TcpInput.c
+    - NetworkPkg/TcpDxe/TcpMain.h
+    - NetworkPkg/TcpDxe/TcpMisc.c
+    - NetworkPkg/TcpDxe/TcpTimer.c
+  links:
+    - https://bugzilla.tianocore.org/show_bug.cgi?id=4541
+    - https://nvd.nist.gov/vuln/detail/CVE-2023-45236
+    - http://www.openwall.com/lists/oss-security/2024/01/16/2
+    - http://packetstormsecurity.com/files/176574/PixieFail-Proof-Of-Concepts.html
+    - https://blog.quarkslab.com/pixiefail-nine-vulnerabilities-in-tianocores-edk-ii-ipv6-network-stack.html
 CVE_2023_45237:
   commit_titles:
     - "NetworkPkg:: SECURITY PATCH CVE 2023-45237"

NetworkPkg/TcpDxe/TcpDriver.c

@@ -83,6 +83,12 @@ EFI_SERVICE_BINDING_PROTOCOL  gTcpServiceBinding = {
   TcpServiceBindingDestroyChild
 };
 
+//
+// This is the handle for the Hash2ServiceBinding Protocol instance this driver produces
+// if the platform does not provide one.
+//
+EFI_HANDLE  mHash2ServiceHandle = NULL;
+
 /**
   Create and start the heartbeat timer for the TCP driver.
 
@@ -165,6 +171,23 @@ TcpDriverEntryPoint (
   EFI_STATUS  Status;
   UINT32      Random;
 
+  //
+  // Initialize the Secret used for hashing TCP sequence numbers
+  //
+  // Normally this should be regenerated periodically, but since
+  // this is only used for UEFI networking and not a general purpose
+  // operating system, it is not necessary to regenerate it.
+  //
+  Status = PseudoRandomU32 (&mTcpGlobalSecret);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "%a failed to generate random number: %r\n", __func__, Status));
+    return Status;
+  }
+
+  //
+  // Get a random number used to generate a random port number
+  // Intentionally not linking this to mTcpGlobalSecret to avoid leaking information about the secret
+  //
   Status = PseudoRandomU32 (&Random);
   if (EFI_ERROR (Status)) {
     DEBUG ((DEBUG_ERROR, "%a Failed to generate random number: %r\n", __func__, Status));
@@ -207,9 +230,8 @@ TcpDriverEntryPoint (
   }
 
   //
-  // Initialize ISS and random port.
+  // Initialize the random port.
   //
-  mTcpGlobalIss   = Random % mTcpGlobalIss;
   mTcp4RandomPort = (UINT16)(TCP_PORT_KNOWN + (Random % TCP_PORT_KNOWN));
   mTcp6RandomPort = mTcp4RandomPort;
 
@@ -224,6 +246,8 @@ TcpDriverEntryPoint (
   @param[in]  IpVersion          IP_VERSION_4 or IP_VERSION_6.
 
   @retval EFI_OUT_OF_RESOURCES   Failed to allocate some resources.
+  @retval EFI_UNSUPPORTED        Service Binding Protocols are unavailable.
+  @retval EFI_ALREADY_STARTED    The TCP driver is already started on the controller.
   @retval EFI_SUCCESS            A new IP6 service binding private was created.
 
 **/
@@ -234,11 +258,13 @@ TcpCreateService (
   IN UINT8       IpVersion
   )
 {
-  EFI_STATUS        Status;
-  EFI_GUID          *IpServiceBindingGuid;
-  EFI_GUID          *TcpServiceBindingGuid;
-  TCP_SERVICE_DATA  *TcpServiceData;
-  IP_IO_OPEN_DATA   OpenData;
+  EFI_STATUS                    Status;
+  EFI_GUID                      *IpServiceBindingGuid;
+  EFI_GUID                      *TcpServiceBindingGuid;
+  TCP_SERVICE_DATA              *TcpServiceData;
+  IP_IO_OPEN_DATA               OpenData;
+  EFI_SERVICE_BINDING_PROTOCOL  *Hash2ServiceBinding;
+  EFI_HASH2_PROTOCOL            *Hash2Protocol;
 
   if (IpVersion == IP_VERSION_4) {
     IpServiceBindingGuid  = &gEfiIp4ServiceBindingProtocolGuid;
@@ -272,6 +298,33 @@ TcpCreateService (
     return EFI_UNSUPPORTED;
   }
 
+  Status = gBS->LocateProtocol (&gEfiHash2ProtocolGuid, NULL, (VOID **)&Hash2Protocol);
+  if (EFI_ERROR (Status)) {
+    //
+    // If we can't find the Hashing protocol, then we need to create one.
+    //
+
+    //
+    // Platform is expected to publish the hash service binding protocol to support TCP.
+    //
+    Status = gBS->LocateProtocol (
+                    &gEfiHash2ServiceBindingProtocolGuid,
+                    NULL,
+                    (VOID **)&Hash2ServiceBinding
+                    );
+    if (EFI_ERROR (Status) || (Hash2ServiceBinding == NULL) || (Hash2ServiceBinding->CreateChild == NULL)) {
+      return EFI_UNSUPPORTED;
+    }
+
+    //
+    // Create an instance of the hash protocol for this controller.
+    //
+    Status = Hash2ServiceBinding->CreateChild (Hash2ServiceBinding, &mHash2ServiceHandle);
+    if (EFI_ERROR (Status)) {
+      return EFI_UNSUPPORTED;
+    }
+  }
+
   //
   // Create the TCP service data.
   //
@@ -423,6 +476,7 @@ TcpDestroyService (
   EFI_STATUS                               Status;
   LIST_ENTRY                               *List;
   TCP_DESTROY_CHILD_IN_HANDLE_BUF_CONTEXT  Context;
+  EFI_SERVICE_BINDING_PROTOCOL             *Hash2ServiceBinding;
 
   ASSERT ((IpVersion == IP_VERSION_4) || (IpVersion == IP_VERSION_6));
 
@@ -439,6 +493,30 @@ TcpDestroyService (
     return EFI_SUCCESS;
   }
 
+  //
+  // Destroy the Hash2ServiceBinding instance if it is created by Tcp driver.
+  //
+  if (mHash2ServiceHandle != NULL) {
+    Status = gBS->LocateProtocol (
+                    &gEfiHash2ServiceBindingProtocolGuid,
+                    NULL,
+                    (VOID **)&Hash2ServiceBinding
+                    );
+    if (EFI_ERROR (Status) || (Hash2ServiceBinding == NULL) || (Hash2ServiceBinding->DestroyChild == NULL)) {
+      return EFI_UNSUPPORTED;
+    }
+
+    //
+    // Destroy the instance of the hashing protocol for this controller.
+    //
+    Status = Hash2ServiceBinding->DestroyChild (Hash2ServiceBinding, &mHash2ServiceHandle);
+    if (EFI_ERROR (Status)) {
+      return EFI_UNSUPPORTED;
+    }
+
+    mHash2ServiceHandle = NULL;
+  }
+
   Status = gBS->OpenProtocol (
                   NicHandle,
                   ServiceBindingGuid,

NetworkPkg/TcpDxe/TcpDxe.inf

@@ -6,6 +6,7 @@
 #  stack has been loaded in system. This driver supports both IPv4 and IPv6 network stack.
 #
 #  Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) Microsoft Corporation
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -68,7 +69,6 @@
   NetLib
   IpIoLib
 
-
 [Protocols]
   ## SOMETIMES_CONSUMES
   ## SOMETIMES_PRODUCES
@@ -81,6 +81,12 @@
   gEfiIp6ServiceBindingProtocolGuid             ## TO_START
   gEfiTcp6ProtocolGuid                          ## BY_START
   gEfiTcp6ServiceBindingProtocolGuid            ## BY_START
+  gEfiHash2ProtocolGuid                         ## BY_START
+  gEfiHash2ServiceBindingProtocolGuid           ## BY_START
+
+[Guids]
+  gEfiHashAlgorithmMD5Guid                      ## CONSUMES
+  gEfiHashAlgorithmSha256Guid                   ## CONSUMES
 
 [Depex]
   gEfiHash2ServiceBindingProtocolGuid

NetworkPkg/TcpDxe/TcpFunc.h

@@ -2,7 +2,7 @@
   Declaration of external functions shared in TCP driver.
 
   Copyright (c) 2009 - 2014, Intel Corporation. All rights reserved.<BR>
-
+  Copyright (c) Microsoft Corporation
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -36,8 +36,11 @@ VOID
 
   @param[in, out]  Tcb               Pointer to the TCP_CB of this TCP instance.
 
+  @retval EFI_SUCCESS             The operation completed successfully
+  @retval others                  The underlying functions failed and could not complete the operation
+
 **/
-VOID
+EFI_STATUS
 TcpInitTcbLocal (
   IN OUT TCP_CB  *Tcb
   );
@@ -128,17 +131,6 @@ TcpCloneTcb (
   IN TCP_CB  *Tcb
   );
 
-/**
-  Compute an ISS to be used by a new connection.
-
-  @return The result ISS.
-
-**/
-TCP_SEQNO
-TcpGetIss (
-  VOID
-  );
-
 /**
   Get the local mss.
 
@@ -202,8 +194,11 @@ TcpFormatNetbuf (
   @param[in, out]  Tcb          Pointer to the TCP_CB that wants to initiate a
                                 connection.
 
+  @retval EFI_SUCCESS             The operation completed successfully
+  @retval others                  The underlying functions failed and could not complete the operation
+
 **/
-VOID
+EFI_STATUS
 TcpOnAppConnect (
   IN OUT TCP_CB  *Tcb
   );

NetworkPkg/TcpDxe/TcpInput.c

@@ -724,6 +724,7 @@ TcpInput (
   TCP_SEQNO   Urg;
   UINT16      Checksum;
   INT32       Usable;
+  EFI_STATUS  Status;
 
   ASSERT ((Version == IP_VERSION_4) || (Version == IP_VERSION_6));
 
@@ -872,7 +873,17 @@ TcpInput (
       Tcb->LocalEnd.Port  = Head->DstPort;
       Tcb->RemoteEnd.Port = Head->SrcPort;
 
-      TcpInitTcbLocal (Tcb);
+      Status = TcpInitTcbLocal (Tcb);
+      if (EFI_ERROR (Status)) {
+        DEBUG (
+          (DEBUG_ERROR,
+           "TcpInput: discard a segment because failed to init local end for TCB %p\n",
+           Tcb)
+          );
+
+        goto DISCARD;
+      }
+
       TcpInitTcbPeer (Tcb, Seg, &Option);
 
       TcpSetState (Tcb, TCP_SYN_RCVD);

NetworkPkg/TcpDxe/TcpMain.h

@@ -3,7 +3,7 @@
   It is the common head file for all Tcp*.c in TCP driver.
 
   Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.<BR>
-
+  Copyright (c) Microsoft Corporation
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -13,6 +13,7 @@
 
 #include <Protocol/ServiceBinding.h>
 #include <Protocol/DriverBinding.h>
+#include <Protocol/Hash2.h>
 #include <Library/IpIoLib.h>
 #include <Library/DevicePathLib.h>
 #include <Library/PrintLib.h>
@@ -31,7 +32,7 @@ extern EFI_UNICODE_STRING_TABLE      *gTcpControllerNameTable;
 
 extern LIST_ENTRY  mTcpRunQue;
 extern LIST_ENTRY  mTcpListenQue;
-extern TCP_SEQNO   mTcpGlobalIss;
+extern TCP_SEQNO   mTcpGlobalSecret;
 extern UINT32      mTcpTick;
 
 ///
@@ -45,14 +46,6 @@ extern UINT32      mTcpTick;
 
 #define TCP_EXPIRE_TIME  65535
 
-///
-/// The implementation selects the initial send sequence number and the unit to
-/// be added when it is increased.
-///
-#define TCP_BASE_ISS         0x4d7e980b
-#define TCP_ISS_INCREMENT_1  2048
-#define TCP_ISS_INCREMENT_2  100
-
 typedef union {
   EFI_TCP4_CONFIG_DATA    Tcp4CfgData;
   EFI_TCP6_CONFIG_DATA    Tcp6CfgData;
@@ -774,4 +767,50 @@ Tcp6Poll (
   IN EFI_TCP6_PROTOCOL  *This
   );
 
+/**
+  Retrieves the Initial Sequence Number (ISN) for a TCP connection identified by local
+  and remote IP addresses and ports.
+
+  This method is based on https://datatracker.ietf.org/doc/html/rfc9293#section-3.4.1
+  Where the ISN is computed as follows:
+    ISN = TimeStamp + MD5(LocalIP, LocalPort, RemoteIP, RemotePort, Secret)
+
+  Otherwise:
+    ISN = M + F(localip, localport, remoteip, remoteport, secretkey)
+
+    "Here M is the 4 microsecond timer, and F() is a pseudorandom function (PRF) of the
+    connection's identifying parameters ("localip, localport, remoteip, remoteport")
+    and a secret key ("secretkey") (SHLD-1). F() MUST NOT be computable from the
+    outside (MUST-9), or an attacker could still guess at sequence numbers from the
+    ISN used for some other connection. The PRF could be implemented as a
+    cryptographic hash of the concatenation of the TCP connection parameters and some
+    secret data. For discussion of the selection of a specific hash algorithm and
+    management of the secret key data."
+
+  @param[in]       LocalIp        A pointer to the local IP address of the TCP connection.
+  @param[in]       LocalIpSize    The size, in bytes, of the LocalIp buffer.
+  @param[in]       LocalPort      The local port number of the TCP connection.
+  @param[in]       RemoteIp       A pointer to the remote IP address of the TCP connection.
+  @param[in]       RemoteIpSize   The size, in bytes, of the RemoteIp buffer.
+  @param[in]       RemotePort     The remote port number of the TCP connection.
+  @param[out]      Isn            A pointer to the variable that will receive the Initial
+                                  Sequence Number (ISN).
+
+  @retval EFI_SUCCESS             The operation completed successfully, and the ISN was
+                                  retrieved.
+  @retval EFI_INVALID_PARAMETER   One or more of the input parameters are invalid.
+  @retval EFI_UNSUPPORTED         The operation is not supported.
+
+**/
+EFI_STATUS
+TcpGetIsn (
+  IN UINT8       *LocalIp,
+  IN UINTN       LocalIpSize,
+  IN UINT16      LocalPort,
+  IN UINT8       *RemoteIp,
+  IN UINTN       RemoteIpSize,
+  IN UINT16      RemotePort,
+  OUT TCP_SEQNO  *Isn
+  );
+
 #endif