Re: [edk2-devel] [PATCH v2 10/15] NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45234 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 10/15] NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45234 Patch

From: Doug Flick <dougflick@microsoft.com>

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

Bug Details:
PixieFail Bug #6
CVE-2023-45234
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 processing DNS Servers option in a DHCPv6 Advertise message

Change Overview:

Introduces a function to cache the Dns Server and perform sanitizing on the incoming DnsServerLen to ensure that the length is valid

> + EFI_STATUS
> + PxeBcCacheDnsServerAddresses (
> +  IN PXEBC_PRIVATE_DATA        *Private,
> +  IN PXEBC_DHCP6_PACKET_CACHE  *Cache6
> +  )

Additional code cleanup

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.c | 71 +++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 6 deletions(-)

diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c
index 425e0cf8061d..2b2d372889a3 100644
--- a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c
+++ b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c
@@ -3,6 +3,7 @@
    (C) Copyright 2014 Hewlett-Packard Development Company, L.P.<BR>   Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>+  Copyright (c) Microsoft Corporation    SPDX-License-Identifier: BSD-2-Clause-Patent @@ -1312,6 +1313,65 @@ PxeBcSelectDhcp6Offer (
   } } +/**+  Cache the DHCPv6 DNS Server addresses++  @param[in] Private               The pointer to PXEBC_PRIVATE_DATA.+  @param[in] Cache6                The pointer to PXEBC_DHCP6_PACKET_CACHE.++  @retval    EFI_SUCCESS           Cache the DHCPv6 DNS Server address successfully.+  @retval    EFI_OUT_OF_RESOURCES  Failed to allocate resources.+  @retval    EFI_DEVICE_ERROR      The DNS Server Address Length provided by a untrusted+                                   option is not a multiple of 16 bytes (sizeof (EFI_IPv6_ADDRESS)).+**/+EFI_STATUS+PxeBcCacheDnsServerAddresses (+  IN PXEBC_PRIVATE_DATA        *Private,+  IN PXEBC_DHCP6_PACKET_CACHE  *Cache6+  )+{+  UINT16  DnsServerLen;++  DnsServerLen = NTOHS (Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER]->OpLen);+  //+  // Make sure that the number is nonzero+  //+  if (DnsServerLen == 0) {+    return EFI_DEVICE_ERROR;+  }++  //+  // Make sure the DnsServerlen is a multiple of EFI_IPv6_ADDRESS (16)+  //+  if (DnsServerLen % sizeof (EFI_IPv6_ADDRESS) != 0) {+    return EFI_DEVICE_ERROR;+  }++  //+  // This code is currently written to only support a single DNS Server instead+  // of multiple such as is spec defined (RFC3646, Section 3). The proper behavior+  // would be to allocate the full space requested, CopyMem all of the data,+  // and then add a DnsServerCount field to Private and update additional code+  // that depends on this.+  //+  // To support multiple DNS servers the `AllocationSize` would need to be changed to DnsServerLen+  //+  // This is tracked in https://bugzilla.tianocore.org/show_bug.cgi?id=1886+  //+  Private->DnsServer = AllocateZeroPool (sizeof (EFI_IPv6_ADDRESS));+  if (Private->DnsServer == NULL) {+    return EFI_OUT_OF_RESOURCES;+  }++  //+  // Intentionally only copy over the first server address.+  // To support multiple DNS servers, the `Length` would need to be changed to DnsServerLen+  //+  CopyMem (Private->DnsServer, Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER]->Data, sizeof (EFI_IPv6_ADDRESS));++  return EFI_SUCCESS;+}+ /**   Handle the DHCPv6 offer packet. @@ -1335,6 +1395,7 @@ PxeBcHandleDhcp6Offer (
   UINT32                    SelectIndex;   UINT32                    Index; +  ASSERT (Private != NULL);   ASSERT (Private->SelectIndex > 0);   SelectIndex = (UINT32)(Private->SelectIndex - 1);   ASSERT (SelectIndex < PXEBC_OFFER_MAX_NUM);@@ -1342,15 +1403,13 @@ PxeBcHandleDhcp6Offer (
   Status = EFI_SUCCESS;    //-  // First try to cache DNS server address if DHCP6 offer provides.+  // First try to cache DNS server addresses if DHCP6 offer provides.   //   if (Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER] != NULL) {-    Private->DnsServer = AllocateZeroPool (NTOHS (Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER]->OpLen));-    if (Private->DnsServer == NULL) {-      return EFI_OUT_OF_RESOURCES;+    Status = PxeBcCacheDnsServerAddresses (Private, Cache6);+    if (EFI_ERROR (Status)) {+      return Status;     }--    CopyMem (Private->DnsServer, Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER]->Data, sizeof (EFI_IPv6_ADDRESS));   }    if (Cache6->OfferType == PxeOfferTypeDhcpBinl) {-- 
2.43.0



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