Re: [edk2-devel] [PATCH v2 13/15] NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45235 Patch

["Saloni Kasbekar" <saloni.kasbekar@intel.com>]

Reviewed-by: Saloni Kasbekar <saloni.kasbekar@intel.com>

-----Original Message-----
From: Douglas Flick [MSFT] <doug.edk2@gmail.com> 
Sent: Thursday, January 25, 2024 1:55 PM
To: devel@edk2.groups.io
Cc: Doug Flick <dougflick@microsoft.com>; Kasbekar, Saloni <saloni.kasbekar@intel.com>; Clark-williams, Zachary <zachary.clark-williams@intel.com>; Doug Flick [MSFT] <doug.edk2@gmail.com>
Subject: [PATCH v2 13/15] NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45235 Patch

From: Doug Flick <dougflick@microsoft.com>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4540

Bug Details:
PixieFail Bug #7
CVE-2023-45235
CVSS 8.3 : CVSS:3.1/AV:A/AC:L/PR:N/UI:N/S:U/C:H/I:L/A:H
CWE-119 Improper Restriction of Operations within the Bounds of  a Memory Buffer

Buffer overflow when handling Server ID option from a DHCPv6 proxy Advertise message

Change Overview:

Performs two checks

1. Checks that the length of the duid is accurate
> + //
> + // Check that the minimum and maximum requirements are met // if 
> + ((OpLen < PXEBC_MIN_SIZE_OF_DUID) ||
(OpLen > PXEBC_MAX_SIZE_OF_DUID)) {
> +  Status = EFI_INVALID_PARAMETER;
> +  goto ON_ERROR;
> + }

2. Ensures that the amount of data written to the buffer is tracked and never exceeds that
> + //
> + // Check that the option length is valid.
> + //
> + if ((DiscoverLen + OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN)
 > DiscoverLenNeeded) {
> +     Status = EFI_OUT_OF_RESOURCES;
> +     goto ON_ERROR;
> + }

Additional code clean up and fix for memory leak in case Option was NULL

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>
---
 NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.h | 17 ++++++  NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c | 77 ++++++++++++++++++++++------
 2 files changed, 78 insertions(+), 16 deletions(-)

diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.h b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.h
index c86f6d391b80..6357d27faefd 100644
--- a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.h
+++ b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.h
@@ -34,6 +34,23 @@
 #define PXEBC_ADDR_START_DELIMITER        '[' #define PXEBC_ADDR_END_DELIMITER          ']' +//+// A DUID consists of a 2-octet type code represented in network byte+// order, followed by a variable number of octets that make up the+// actual identifier.  The length of the DUID (not including the type+// code) is at least 1 octet and at most 128 octets.+//+#define PXEBC_MIN_SIZE_OF_DUID  (sizeof(UINT16) + 1)+#define PXEBC_MAX_SIZE_OF_DUID  (sizeof(UINT16) + 128)++//+// This define represents the combineds code and length field from+// https://datatracker.ietf.org/doc/html/rfc3315#section-22.1+//+#define PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN  \+      (sizeof (((EFI_DHCP6_PACKET_OPTION *)0)->OpCode) + \+      sizeof (((EFI_DHCP6_PACKET_OPTION *)0)->OpLen))+ #define GET_NEXT_DHCP6_OPTION(Opt) \   (EFI_DHCP6_PACKET_OPTION *) ((UINT8 *) (Opt) + \   sizeof (EFI_DHCP6_PACKET_OPTION) + (NTOHS ((Opt)->OpLen)) - 1)diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c
index 2b2d372889a3..7fd1281c1184 100644
--- a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c
+++ b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c
@@ -887,6 +887,7 @@ PxeBcRequestBootService (
   EFI_STATUS                       Status;   EFI_DHCP6_PACKET                 *IndexOffer;   UINT8                            *Option;+  UINTN                            DiscoverLenNeeded;    PxeBc      = &Private->PxeBc;   Request    = Private->Dhcp6Request;@@ -899,7 +900,8 @@ PxeBcRequestBootService (
     return EFI_DEVICE_ERROR;   } -  Discover = AllocateZeroPool (sizeof (EFI_PXE_BASE_CODE_DHCPV6_PACKET));+  DiscoverLenNeeded = sizeof (EFI_PXE_BASE_CODE_DHCPV6_PACKET);+  Discover          = AllocateZeroPool (DiscoverLenNeeded);   if (Discover == NULL) {     return EFI_OUT_OF_RESOURCES;   }@@ -924,16 +926,34 @@ PxeBcRequestBootService (
                DHCP6_OPT_SERVER_ID                );     if (Option == NULL) {-      return EFI_NOT_FOUND;+      Status = EFI_NOT_FOUND;+      goto ON_ERROR;     }      //     // Add Server ID Option.     //     OpLen = NTOHS (((EFI_DHCP6_PACKET_OPTION *)Option)->OpLen);-    CopyMem (DiscoverOpt, Option, OpLen + 4);-    DiscoverOpt += (OpLen + 4);-    DiscoverLen += (OpLen + 4);++    //+    // Check that the minimum and maximum requirements are met+    //+    if ((OpLen < PXEBC_MIN_SIZE_OF_DUID) || (OpLen > PXEBC_MAX_SIZE_OF_DUID)) {+      Status = EFI_INVALID_PARAMETER;+      goto ON_ERROR;+    }++    //+    // Check that the option length is valid.+    //+    if ((DiscoverLen + OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN) > DiscoverLenNeeded) {+      Status = EFI_OUT_OF_RESOURCES;+      goto ON_ERROR;+    }++    CopyMem (DiscoverOpt, Option, OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN);+    DiscoverOpt += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN);+    DiscoverLen += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN);   }    while (RequestLen < Request->Length) {@@ -944,16 +964,24 @@ PxeBcRequestBootService (
         (OpCode != DHCP6_OPT_SERVER_ID)         )     {+      //+      // Check that the option length is valid.+      //+      if (DiscoverLen + OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN > DiscoverLenNeeded) {+        Status = EFI_OUT_OF_RESOURCES;+        goto ON_ERROR;+      }+       //       // Copy all the options except IA option and Server ID       //-      CopyMem (DiscoverOpt, RequestOpt, OpLen + 4);-      DiscoverOpt += (OpLen + 4);-      DiscoverLen += (OpLen + 4);+      CopyMem (DiscoverOpt, RequestOpt, OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN);+      DiscoverOpt += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN);+      DiscoverLen += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN);     } -    RequestOpt += (OpLen + 4);-    RequestLen += (OpLen + 4);+    RequestOpt += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN);+    RequestLen += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN);   }    //@@ -2154,6 +2182,7 @@ PxeBcDhcp6Discover (
   UINT16                           OpLen;   UINT32                           Xid;   EFI_STATUS                       Status;+  UINTN                            DiscoverLenNeeded;    PxeBc    = &Private->PxeBc;   Mode     = PxeBc->Mode;@@ -2169,7 +2198,8 @@ PxeBcDhcp6Discover (
     return EFI_DEVICE_ERROR;   } -  Discover = AllocateZeroPool (sizeof (EFI_PXE_BASE_CODE_DHCPV6_PACKET));+  DiscoverLenNeeded = sizeof (EFI_PXE_BASE_CODE_DHCPV6_PACKET);+  Discover          = AllocateZeroPool (DiscoverLenNeeded);   if (Discover == NULL) {     return EFI_OUT_OF_RESOURCES;   }@@ -2185,22 +2215,37 @@ PxeBcDhcp6Discover (
   DiscoverLen             = sizeof (EFI_DHCP6_HEADER);   RequestLen              = DiscoverLen; +  //+  // The request packet is generated by the UEFI network stack. In the DHCP4 DORA and DHCP6 SARR sequence,+  // the first (discover in DHCP4 and solicit in DHCP6) and third (request in both DHCP4 and DHCP6) are+  // generated by the DHCP client (the UEFI network stack in this case). By the time this function executes,+  // the DHCP sequence already has been executed once (see UEFI Specification Figures 24.2 and 24.3), with+  // Private->Dhcp6Request being a cached copy of the DHCP6 request packet that UEFI network stack previously+  // generated and sent.+  //+  // Therefore while this code looks like it could overflow, in practice it's not possible.+  //   while (RequestLen < Request->Length) {     OpCode = NTOHS (((EFI_DHCP6_PACKET_OPTION *)RequestOpt)->OpCode);     OpLen  = NTOHS (((EFI_DHCP6_PACKET_OPTION *)RequestOpt)->OpLen);     if ((OpCode != EFI_DHCP6_IA_TYPE_NA) &&         (OpCode != EFI_DHCP6_IA_TYPE_TA))     {+      if (DiscoverLen + OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN > DiscoverLenNeeded) {+        Status = EFI_OUT_OF_RESOURCES;+        goto ON_ERROR;+      }+       //       // Copy all the options except IA option.       //-      CopyMem (DiscoverOpt, RequestOpt, OpLen + 4);-      DiscoverOpt += (OpLen + 4);-      DiscoverLen += (OpLen + 4);+      CopyMem (DiscoverOpt, RequestOpt, OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN);+      DiscoverOpt += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN);+      DiscoverLen += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN);     } -    RequestOpt += (OpLen + 4);-    RequestLen += (OpLen + 4);+    RequestOpt += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN);+    RequestLen += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN);   }    Status = PxeBc->UdpWrite (-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114967): https://edk2.groups.io/g/devel/message/114967
Mute This Topic: https://groups.io/mt/103964991/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-