public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Doug Flick via groups.io" <dougflick=microsoft.com@groups.io>
To: devel@edk2.groups.io
Cc: Doug Flick <dougflick@microsoft.com>,
	Saloni Kasbekar <saloni.kasbekar@intel.com>,
	Zachary Clark-williams <zachary.clark-williams@intel.com>,
	"Doug Flick [MSFT]" <doug.edk2@gmail.com>
Subject: [edk2-devel] [PATCH 12/14] NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45235 Patch
Date: Tue, 23 Jan 2024 19:33:35 -0800	[thread overview]
Message-ID: <2276136eefe1d8080cfcdb5cdac3cf297a58aa3a.1706062164.git.doug.edk2@gmail.com> (raw)
In-Reply-To: <cover.1706062164.git.doug.edk2@gmail.com>

From: Doug Flick <dougflick@microsoft.com>

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

SECURITY PATCH - Patch

TCBZ4540
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

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 (#114264): https://edk2.groups.io/g/devel/message/114264
Mute This Topic: https://groups.io/mt/103926743/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



  parent reply	other threads:[~2024-01-24  5:20 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-24  3:33 [edk2-devel] [PATCH 00/14] Security Patches for EDK II Network Stack Doug Flick via groups.io
2024-01-24  3:33 ` [edk2-devel] [PATCH 01/14] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45230 Patch Doug Flick via groups.io
2024-01-24 11:30   ` Gerd Hoffmann
2024-01-24  3:33 ` [edk2-devel] [PATCH 02/14] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45230 Unit Tests Doug Flick via groups.io
2024-01-24 11:39   ` Gerd Hoffmann
2024-01-24  3:33 ` [edk2-devel] [PATCH 03/14] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Patch Doug Flick via groups.io
2024-01-24 11:45   ` Gerd Hoffmann
2024-01-24  3:33 ` [edk2-devel] [PATCH 04/14] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Unit Tests Doug Flick via groups.io
2024-01-24  3:33 ` [edk2-devel] [PATCH 05/14] NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45231 - Patch Doug Flick via groups.io
2024-01-24 11:53   ` Gerd Hoffmann
2024-01-24  3:33 ` [edk2-devel] [PATCH 06/14] NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45231 - Unit Tests Doug Flick via groups.io
2024-01-24  3:33 ` [edk2-devel] [PATCH 07/14] NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45232 Patch Doug Flick via groups.io
2024-01-24 10:39   ` Pedro Falcato
2024-01-24 12:02     ` Gerd Hoffmann
2024-01-24  3:33 ` [edk2-devel] [PATCH 08/14] NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45232 Unit Tests Doug Flick via groups.io
2024-01-24  3:33 ` [edk2-devel] [PATCH 09/14] NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45234 Patch Doug Flick via groups.io
2024-01-24 12:09   ` Gerd Hoffmann
2024-01-24  3:33 ` [edk2-devel] [PATCH 10/14] NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45234 Unit Tests Doug Flick via groups.io
2024-01-24  3:33 ` [edk2-devel] [PATCH 11/14] MdePkg: Test: Add gRT_GetTime Google Test Mock Doug Flick via groups.io
2024-01-24 18:50   ` Michael D Kinney
2024-01-24  3:33 ` Doug Flick via groups.io [this message]
2024-01-24  3:33 ` [edk2-devel] [PATCH 13/14] NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45235 Unit Tests Doug Flick via groups.io
2024-01-24  3:33 ` [edk2-devel] [PATCH 14/14] NetworkPkg: : Adds a SecurityFix.yaml file Doug Flick via groups.io
2024-01-24 10:17 ` [edk2-devel] [PATCH 00/14] Security Patches for EDK II Network Stack Pedro Falcato

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2276136eefe1d8080cfcdb5cdac3cf297a58aa3a.1706062164.git.doug.edk2@gmail.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox