Re: [edk2-devel] [PATCH v2 14/15] NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45235 Unit Tests

["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 14/15] NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45235 Unit Tests

From: Doug Flick <dougflick@microsoft.com>

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

Unit tests to confirm that the bug..

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

..has been patched.

This patch contains unit tests for the following functions:
PxeBcRequestBootService
PxeBcDhcp6Discover

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/Test/NetworkPkgHostTest.dsc        |   5 +-
 .../GoogleTest/PxeBcDhcp6GoogleTest.h         |  18 ++
 .../GoogleTest/PxeBcDhcp6GoogleTest.cpp       | 278 +++++++++++++++++-
 3 files changed, 298 insertions(+), 3 deletions(-)

diff --git a/NetworkPkg/Test/NetworkPkgHostTest.dsc b/NetworkPkg/Test/NetworkPkgHostTest.dsc
index a0273c431025..fa301a7a52ab 100644
--- a/NetworkPkg/Test/NetworkPkgHostTest.dsc
+++ b/NetworkPkg/Test/NetworkPkgHostTest.dsc
@@ -27,7 +27,10 @@ [Components]
   #   NetworkPkg/Dhcp6Dxe/GoogleTest/Dhcp6DxeGoogleTest.inf   NetworkPkg/Ip6Dxe/GoogleTest/Ip6DxeGoogleTest.inf-  NetworkPkg/UefiPxeBcDxe/GoogleTest/UefiPxeBcDxeGoogleTest.inf+  NetworkPkg/UefiPxeBcDxe/GoogleTest/UefiPxeBcDxeGoogleTest.inf {+    <LibraryClasses>+      UefiRuntimeServicesTableLib|MdePkg/Test/Mock/Library/GoogleTest/MockUefiRuntimeServicesTableLib/MockUefiRuntimeServicesTableLib.inf+  }  # Despite these library classes being listed in [LibraryClasses] below, they are not needed for the host-based unit tests. [LibraryClasses]diff --git a/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.h b/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.h
index b17c314791c8..0d825e44250a 100644
--- a/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.h
+++ b/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.h
@@ -47,4 +47,22 @@ PxeBcCacheDnsServerAddresses (
   IN PXEBC_DHCP6_PACKET_CACHE  *Cache6   ); +/**+  Build and send out the request packet for the bootfile, and parse the reply.++  @param[in]  Private               The pointer to PxeBc private data.+  @param[in]  Index                 PxeBc option boot item type.++  @retval     EFI_SUCCESS           Successfully discovered the boot file.+  @retval     EFI_OUT_OF_RESOURCES  Failed to allocate resources.+  @retval     EFI_NOT_FOUND         Can't get the PXE reply packet.+  @retval     Others                Failed to discover the boot file.++**/+EFI_STATUS+PxeBcRequestBootService (+  IN  PXEBC_PRIVATE_DATA  *Private,+  IN  UINT32              Index+  );+ #endif // PXE_BC_DHCP6_GOOGLE_TEST_H_diff --git a/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.cpp b/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.cpp
index 8260eeee50dc..bd423ebadfce 100644
--- a/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.cpp
+++ b/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.cpp
@@ -4,7 +4,9 @@
   Copyright (c) Microsoft Corporation   SPDX-License-Identifier: BSD-2-Clause-Patent **/-#include <gtest/gtest.h>+#include <Library/GoogleTestLib.h>+#include <GoogleTest/Library/MockUefiLib.h>+#include <GoogleTest/Library/MockUefiRuntimeServicesTableLib.h>  extern "C" {   #include <Uefi.h>@@ -19,7 +21,8 @@ extern "C" {
 // Definitions /////////////////////////////////////////////////////////////////////////////// -#define PACKET_SIZE  (1500)+#define PACKET_SIZE            (1500)+#define REQUEST_OPTION_LENGTH  (120)  typedef struct {   UINT16    OptionCode;   // The option code for DHCP6_OPT_SERVER_ID (e.g., 0x03)@@ -76,6 +79,26 @@ MockConfigure (
 }  // Needed by PxeBcSupport+EFI_STATUS+PxeBcDns6 (+  IN PXEBC_PRIVATE_DATA  *Private,+  IN     CHAR16          *HostName,+  OUT EFI_IPv6_ADDRESS   *IpAddress+  )+{+  return EFI_SUCCESS;+}++UINT32+PxeBcBuildDhcp6Options (+  IN  PXEBC_PRIVATE_DATA       *Private,+  OUT EFI_DHCP6_PACKET_OPTION  **OptList,+  IN  UINT8                    *Buffer+  )+{+  return EFI_SUCCESS;+}+ EFI_STATUS EFIAPI QueueDpc (@@ -159,6 +182,10 @@ TEST_F (PxeBcHandleDhcp6OfferTest, BasicUsageTest) {
   ASSERT_EQ (PxeBcHandleDhcp6Offer (&(PxeBcHandleDhcp6OfferTest::Private)), EFI_DEVICE_ERROR); } +///////////////////////////////////////////////////////////////////////////////+// PxeBcCacheDnsServerAddresses Tests+///////////////////////////////////////////////////////////////////////////////+ class PxeBcCacheDnsServerAddressesTest : public ::testing::Test { public:   PXEBC_PRIVATE_DATA Private = { 0 };@@ -298,3 +325,250 @@ TEST_F (PxeBcCacheDnsServerAddressesTest, MultipleDnsEntries) {
     FreePool (Private.DnsServer);   } }++///////////////////////////////////////////////////////////////////////////////+// PxeBcRequestBootServiceTest Test Cases+///////////////////////////////////////////////////////////////////////////////++class PxeBcRequestBootServiceTest : public ::testing::Test {+public:+  PXEBC_PRIVATE_DATA Private = { 0 };+  EFI_UDP6_PROTOCOL Udp6Read;++protected:+  // Add any setup code if needed+  virtual void+  SetUp (+    )+  {+    Private.Dhcp6Request = (EFI_DHCP6_PACKET *)AllocateZeroPool (PACKET_SIZE);++    // Need to setup the EFI_PXE_BASE_CODE_PROTOCOL+    // The function under test really only needs the following:+    //  UdpWrite+    //  UdpRead++    Private.PxeBc.UdpWrite = (EFI_PXE_BASE_CODE_UDP_WRITE)MockUdpWrite;+    Private.PxeBc.UdpRead  = (EFI_PXE_BASE_CODE_UDP_READ)MockUdpRead;++    // Need to setup EFI_UDP6_PROTOCOL+    // The function under test really only needs the following:+    //  Configure++    Udp6Read.Configure = (EFI_UDP6_CONFIGURE)MockConfigure;+    Private.Udp6Read   = &Udp6Read;+  }++  // Add any cleanup code if needed+  virtual void+  TearDown (+    )+  {+    if (Private.Dhcp6Request != NULL) {+      FreePool (Private.Dhcp6Request);+    }++    // Clean up any resources or variables+  }+};++TEST_F (PxeBcRequestBootServiceTest, ServerDiscoverBasicUsageTest) {+  PxeBcRequestBootServiceTest::Private.OfferBuffer[0].Dhcp6.OfferType = PxeOfferTypeProxyBinl;++  DHCP6_OPTION_SERVER_ID  Server = { 0 };++  Server.OptionCode =  HTONS (DHCP6_OPT_SERVER_ID);+  Server.OptionLen  = HTONS (16); // valid length+  UINT8  Index = 0;++  EFI_DHCP6_PACKET  *Packet = (EFI_DHCP6_PACKET *)&Private.OfferBuffer[Index].Dhcp6.Packet.Offer;++  UINT8  *Cursor = (UINT8 *)(Packet->Dhcp6.Option);++  CopyMem (Cursor, &Server, sizeof (Server));+  Cursor += sizeof (Server);++  // Update the packet length+  Packet->Length = (UINT16)(Cursor - (UINT8 *)Packet);+  Packet->Size   = PACKET_SIZE;++  ASSERT_EQ (PxeBcRequestBootService (&(PxeBcRequestBootServiceTest::Private), Index), EFI_SUCCESS);+}++TEST_F (PxeBcRequestBootServiceTest, AttemptDiscoverOverFlowExpectFailure) {+  PxeBcRequestBootServiceTest::Private.OfferBuffer[0].Dhcp6.OfferType = PxeOfferTypeProxyBinl;++  DHCP6_OPTION_SERVER_ID  Server = { 0 };++  Server.OptionCode =  HTONS (DHCP6_OPT_SERVER_ID);+  Server.OptionLen  = HTONS (1500); // This length would overflow without a check+  UINT8  Index = 0;++  EFI_DHCP6_PACKET  *Packet = (EFI_DHCP6_PACKET *)&Private.OfferBuffer[Index].Dhcp6.Packet.Offer;++  UINT8  *Cursor = (UINT8 *)(Packet->Dhcp6.Option);++  CopyMem (Cursor, &Server, sizeof (Server));+  Cursor += sizeof (Server);++  // Update the packet length+  Packet->Length = (UINT16)(Cursor - (UINT8 *)Packet);+  Packet->Size   = PACKET_SIZE;++  // This is going to be stopped by the duid overflow check+  ASSERT_EQ (PxeBcRequestBootService (&(PxeBcRequestBootServiceTest::Private), Index), EFI_INVALID_PARAMETER);+}++TEST_F (PxeBcRequestBootServiceTest, RequestBasicUsageTest) {+  EFI_DHCP6_PACKET_OPTION  RequestOpt = { 0 }; // the data section doesn't really matter++  RequestOpt.OpCode = HTONS (0x1337);+  RequestOpt.OpLen  = 0; // valid length++  UINT8  Index = 0;++  EFI_DHCP6_PACKET  *Packet = (EFI_DHCP6_PACKET *)&Private.Dhcp6Request[Index];++  UINT8  *Cursor = (UINT8 *)(Packet->Dhcp6.Option);++  CopyMem (Cursor, &RequestOpt, sizeof (RequestOpt));+  Cursor += sizeof (RequestOpt);++  // Update the packet length+  Packet->Length = (UINT16)(Cursor - (UINT8 *)Packet);+  Packet->Size   = PACKET_SIZE;++  ASSERT_EQ (PxeBcRequestBootService (&(PxeBcRequestBootServiceTest::Private), Index), EFI_SUCCESS);+}++TEST_F (PxeBcRequestBootServiceTest, AttemptRequestOverFlowExpectFailure) {+  EFI_DHCP6_PACKET_OPTION  RequestOpt = { 0 }; // the data section doesn't really matter++  RequestOpt.OpCode = HTONS (0x1337);+  RequestOpt.OpLen  = 1500; // this length would overflow without a check++  UINT8  Index = 0;++  EFI_DHCP6_PACKET  *Packet = (EFI_DHCP6_PACKET *)&Private.Dhcp6Request[Index];++  UINT8  *Cursor = (UINT8 *)(Packet->Dhcp6.Option);++  CopyMem (Cursor, &RequestOpt, sizeof (RequestOpt));+  Cursor += sizeof (RequestOpt);++  // Update the packet length+  Packet->Length = (UINT16)(Cursor - (UINT8 *)Packet);+  Packet->Size   = PACKET_SIZE;++  ASSERT_EQ (PxeBcRequestBootService (&(PxeBcRequestBootServiceTest::Private), Index), EFI_OUT_OF_RESOURCES);+}++///////////////////////////////////////////////////////////////////////////////+// PxeBcDhcp6Discover Test+///////////////////////////////////////////////////////////////////////////////++class PxeBcDhcp6DiscoverTest : public ::testing::Test {+public:+  PXEBC_PRIVATE_DATA Private = { 0 };+  EFI_UDP6_PROTOCOL Udp6Read;++protected:+  MockUefiRuntimeServicesTableLib RtServicesMock;++  // Add any setup code if needed+  virtual void+  SetUp (+    )+  {+    Private.Dhcp6Request = (EFI_DHCP6_PACKET *)AllocateZeroPool (PACKET_SIZE);++    // Need to setup the EFI_PXE_BASE_CODE_PROTOCOL+    // The function under test really only needs the following:+    //  UdpWrite+    //  UdpRead++    Private.PxeBc.UdpWrite = (EFI_PXE_BASE_CODE_UDP_WRITE)MockUdpWrite;+    Private.PxeBc.UdpRead  = (EFI_PXE_BASE_CODE_UDP_READ)MockUdpRead;++    // Need to setup EFI_UDP6_PROTOCOL+    // The function under test really only needs the following:+    //  Configure++    Udp6Read.Configure = (EFI_UDP6_CONFIGURE)MockConfigure;+    Private.Udp6Read   = &Udp6Read;+  }++  // Add any cleanup code if needed+  virtual void+  TearDown (+    )+  {+    if (Private.Dhcp6Request != NULL) {+      FreePool (Private.Dhcp6Request);+    }++    // Clean up any resources or variables+  }+};++// Test Description+// This will cause an overflow by an untrusted packet during the option parsing+TEST_F (PxeBcDhcp6DiscoverTest, BasicOverflowTest) {+  EFI_IPv6_ADDRESS         DestIp     = { 0 };+  EFI_DHCP6_PACKET_OPTION  RequestOpt = { 0 }; // the data section doesn't really matter++  RequestOpt.OpCode = HTONS (0x1337);+  RequestOpt.OpLen  = HTONS (0xFFFF); // overflow++  UINT8  *Cursor = (UINT8 *)(Private.Dhcp6Request->Dhcp6.Option);++  CopyMem (Cursor, &RequestOpt, sizeof (RequestOpt));+  Cursor += sizeof (RequestOpt);++  Private.Dhcp6Request->Length = (UINT16)(Cursor - (UINT8 *)Private.Dhcp6Request);++  EXPECT_CALL (RtServicesMock, gRT_GetTime)+    .WillOnce (::testing::Return (0));++  ASSERT_EQ (+    PxeBcDhcp6Discover (+      &(PxeBcDhcp6DiscoverTest::Private),+      0,+      NULL,+      FALSE,+      (EFI_IP_ADDRESS *)&DestIp+      ),+    EFI_OUT_OF_RESOURCES+    );+}++// Test Description+// This will test that we can handle a packet with a valid option length+TEST_F (PxeBcDhcp6DiscoverTest, BasicUsageTest) {+  EFI_IPv6_ADDRESS         DestIp     = { 0 };+  EFI_DHCP6_PACKET_OPTION  RequestOpt = { 0 }; // the data section doesn't really matter++  RequestOpt.OpCode = HTONS (0x1337);+  RequestOpt.OpLen  = HTONS (0x30);++  UINT8  *Cursor = (UINT8 *)(Private.Dhcp6Request->Dhcp6.Option);++  CopyMem (Cursor, &RequestOpt, sizeof (RequestOpt));+  Cursor += sizeof (RequestOpt);++  Private.Dhcp6Request->Length = (UINT16)(Cursor - (UINT8 *)Private.Dhcp6Request);++  EXPECT_CALL (RtServicesMock, gRT_GetTime)+    .WillOnce (::testing::Return (0));++  ASSERT_EQ (+    PxeBcDhcp6Discover (+      &(PxeBcDhcp6DiscoverTest::Private),+      0,+      NULL,+      FALSE,+      (EFI_IP_ADDRESS *)&DestIp+      ),+    EFI_SUCCESS+    );+}-- 
2.43.0



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