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

From: Doug Flick <dougflick@microsoft.com>

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

Unit tests to that the bug..

Buffer overflow when processing DNS Servers option in a DHCPv6 Advertise message

..has been patched

This contains tests for the following functions:
PxeBcHandleDhcp6Offer
PxeBcCacheDnsServerAddresses

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        |   1 +
 .../GoogleTest/UefiPxeBcDxeGoogleTest.inf     |  48 +++
 .../GoogleTest/PxeBcDhcp6GoogleTest.h         |  50 +++
 .../GoogleTest/PxeBcDhcp6GoogleTest.cpp       | 300 ++++++++++++++++++
 .../GoogleTest/UefiPxeBcDxeGoogleTest.cpp     |  19 ++
 5 files changed, 418 insertions(+)
 create mode 100644 NetworkPkg/UefiPxeBcDxe/GoogleTest/UefiPxeBcDxeGoogleTest.inf
 create mode 100644 NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.h
 create mode 100644 NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.cpp
 create mode 100644 NetworkPkg/UefiPxeBcDxe/GoogleTest/UefiPxeBcDxeGoogleTest.cpp

diff --git a/NetworkPkg/Test/NetworkPkgHostTest.dsc b/NetworkPkg/Test/NetworkPkgHostTest.dsc
index 7fa7b0f9d5be..a0273c431025 100644
--- a/NetworkPkg/Test/NetworkPkgHostTest.dsc
+++ b/NetworkPkg/Test/NetworkPkgHostTest.dsc
@@ -27,6 +27,7 @@ [Components]
   #   NetworkPkg/Dhcp6Dxe/GoogleTest/Dhcp6DxeGoogleTest.inf   NetworkPkg/Ip6Dxe/GoogleTest/Ip6DxeGoogleTest.inf+  NetworkPkg/UefiPxeBcDxe/GoogleTest/UefiPxeBcDxeGoogleTest.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/UefiPxeBcDxeGoogleTest.inf b/NetworkPkg/UefiPxeBcDxe/GoogleTest/UefiPxeBcDxeGoogleTest.inf
new file mode 100644
index 000000000000..301dcdf61109
--- /dev/null
+++ b/NetworkPkg/UefiPxeBcDxe/GoogleTest/UefiPxeBcDxeGoogleTest.inf
@@ -0,0 +1,48 @@
+## @file+# Unit test suite for the UefiPxeBcDxe using Google Test+#+# Copyright (c) Microsoft Corporation.<BR>+# SPDX-License-Identifier: BSD-2-Clause-Patent+##+[Defines]+INF_VERSION    = 0x00010005+BASE_NAME      = UefiPxeBcDxeGoogleTest+FILE_GUID      = 77D45C64-EC1E-4174-887B-886E89FD1EDF+MODULE_TYPE    = HOST_APPLICATION+VERSION_STRING = 1.0++#+# The following information is for reference only and not required by the build tools.+#+#  VALID_ARCHITECTURES           = IA32 X64+#++[Sources]+  UefiPxeBcDxeGoogleTest.cpp+  PxeBcDhcp6GoogleTest.cpp+  PxeBcDhcp6GoogleTest.h+  ../PxeBcDhcp6.c+  ../PxeBcSupport.c++[Packages]+  MdePkg/MdePkg.dec+  MdeModulePkg/MdeModulePkg.dec+  UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec+  NetworkPkg/NetworkPkg.dec++[LibraryClasses]+  GoogleTestLib+  DebugLib+  NetLib+  PcdLib++[Protocols]+  gEfiDhcp6ServiceBindingProtocolGuid+  gEfiDns6ServiceBindingProtocolGuid+  gEfiDns6ProtocolGuid++[Pcd]+  gEfiNetworkPkgTokenSpaceGuid.PcdDhcp6UidType++[Guids]+  gZeroGuiddiff --git a/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.h b/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.h
new file mode 100644
index 000000000000..b17c314791c8
--- /dev/null
+++ b/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.h
@@ -0,0 +1,50 @@
+/** @file+  This file exposes the internal interfaces which may be unit tested+  for the PxeBcDhcp6Dxe driver.++  Copyright (c) Microsoft Corporation.<BR>+  SPDX-License-Identifier: BSD-2-Clause-Patent+**/++#ifndef PXE_BC_DHCP6_GOOGLE_TEST_H_+#define PXE_BC_DHCP6_GOOGLE_TEST_H_++//+// Minimal includes needed to compile+//+#include <Uefi.h>+#include "../PxeBcImpl.h"++/**+  Handle the DHCPv6 offer packet.++  @param[in]  Private             The pointer to PXEBC_PRIVATE_DATA.++  @retval     EFI_SUCCESS           Handled the DHCPv6 offer packet successfully.+  @retval     EFI_NO_RESPONSE       No response to the following request packet.+  @retval     EFI_OUT_OF_RESOURCES  Failed to allocate resources.+  @retval     EFI_BUFFER_TOO_SMALL  Can't cache the offer pacet.++**/+EFI_STATUS+PxeBcHandleDhcp6Offer (+  IN PXEBC_PRIVATE_DATA  *Private+  );++/**+  Cache the DHCPv6 Server address++  @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 Server address successfully.+  @retval    EFI_OUT_OF_RESOURCES  Failed to allocate resources.+  @retval    EFI_DEVICE_ERROR      Failed to cache the DHCPv6 Server address.+**/+EFI_STATUS+PxeBcCacheDnsServerAddresses (+  IN PXEBC_PRIVATE_DATA        *Private,+  IN PXEBC_DHCP6_PACKET_CACHE  *Cache6+  );++#endif // PXE_BC_DHCP6_GOOGLE_TEST_H_diff --git a/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.cpp b/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.cpp
new file mode 100644
index 000000000000..8260eeee50dc
--- /dev/null
+++ b/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.cpp
@@ -0,0 +1,300 @@
+/** @file+  Host based unit test for PxeBcDhcp6.c.++  Copyright (c) Microsoft Corporation+  SPDX-License-Identifier: BSD-2-Clause-Patent+**/+#include <gtest/gtest.h>++extern "C" {+  #include <Uefi.h>+  #include <Library/BaseLib.h>+  #include <Library/DebugLib.h>+  #include "../PxeBcImpl.h"+  #include "../PxeBcDhcp6.h"+  #include "PxeBcDhcp6GoogleTest.h"+}++///////////////////////////////////////////////////////////////////////////////+// Definitions+///////////////////////////////////////////////////////////////////////////////++#define PACKET_SIZE  (1500)++typedef struct {+  UINT16    OptionCode;   // The option code for DHCP6_OPT_SERVER_ID (e.g., 0x03)+  UINT16    OptionLen;    // The length of the option (e.g., 16 bytes)+  UINT8     ServerId[16]; // The 16-byte DHCPv6 Server Identifier+} DHCP6_OPTION_SERVER_ID;++///////////////////////////////////////////////////////////////////////////////+/// Symbol Definitions+///////////////////////////////////////////////////////////////////////////////++EFI_STATUS+MockUdpWrite (+  IN EFI_PXE_BASE_CODE_PROTOCOL      *This,+  IN UINT16                          OpFlags,+  IN EFI_IP_ADDRESS                  *DestIp,+  IN EFI_PXE_BASE_CODE_UDP_PORT      *DestPort,+  IN EFI_IP_ADDRESS                  *GatewayIp   OPTIONAL,+  IN EFI_IP_ADDRESS                  *SrcIp       OPTIONAL,+  IN OUT EFI_PXE_BASE_CODE_UDP_PORT  *SrcPort     OPTIONAL,+  IN UINTN                           *HeaderSize  OPTIONAL,+  IN VOID                            *HeaderPtr   OPTIONAL,+  IN UINTN                           *BufferSize,+  IN VOID                            *BufferPtr+  )+{+  return EFI_SUCCESS;+}++EFI_STATUS+MockUdpRead (+  IN EFI_PXE_BASE_CODE_PROTOCOL      *This,+  IN UINT16                          OpFlags,+  IN OUT EFI_IP_ADDRESS              *DestIp      OPTIONAL,+  IN OUT EFI_PXE_BASE_CODE_UDP_PORT  *DestPort    OPTIONAL,+  IN OUT EFI_IP_ADDRESS              *SrcIp       OPTIONAL,+  IN OUT EFI_PXE_BASE_CODE_UDP_PORT  *SrcPort     OPTIONAL,+  IN UINTN                           *HeaderSize  OPTIONAL,+  IN VOID                            *HeaderPtr   OPTIONAL,+  IN OUT UINTN                       *BufferSize,+  IN VOID                            *BufferPtr+  )+{+  return EFI_SUCCESS;+}++EFI_STATUS+MockConfigure (+  IN EFI_UDP6_PROTOCOL     *This,+  IN EFI_UDP6_CONFIG_DATA  *UdpConfigData OPTIONAL+  )+{+  return EFI_SUCCESS;+}++// Needed by PxeBcSupport+EFI_STATUS+EFIAPI+QueueDpc (+  IN EFI_TPL            DpcTpl,+  IN EFI_DPC_PROCEDURE  DpcProcedure,+  IN VOID               *DpcContext    OPTIONAL+  )+{+  return EFI_SUCCESS;+}++///////////////////////////////////////////////////////////////////////////////+// PxeBcHandleDhcp6OfferTest Tests+///////////////////////////////////////////////////////////////////////////////++class PxeBcHandleDhcp6OfferTest : public ::testing::Test {+public:+  PXEBC_PRIVATE_DATA Private = { 0 };+  EFI_UDP6_PROTOCOL Udp6Read;+  EFI_PXE_BASE_CODE_MODE Mode = { 0 };++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;++    // Need to setup the EFI_PXE_BASE_CODE_MODE+    Private.PxeBc.Mode = &Mode;++    // for this test it doesn't really matter what the Dhcpv6 ack is set to+  }++  // Add any cleanup code if needed+  virtual void+  TearDown (+    )+  {+    if (Private.Dhcp6Request != NULL) {+      FreePool (Private.Dhcp6Request);+    }++    // Clean up any resources or variables+  }+};++// Note:+// Testing PxeBcHandleDhcp6Offer() is difficult because it depends on a+// properly setup Private structure. Attempting to properly test this function+// without a signficant refactor is a fools errand. Instead, we will test+// that we can prevent an overflow in the function.+TEST_F (PxeBcHandleDhcp6OfferTest, BasicUsageTest) {+  PXEBC_DHCP6_PACKET_CACHE  *Cache6 = NULL;+  EFI_DHCP6_PACKET_OPTION   Option  = { 0 };++  Private.SelectIndex = 1; // SelectIndex is 1-based+  Cache6              = &Private.OfferBuffer[Private.SelectIndex - 1].Dhcp6;++  Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER] = &Option;+  // Setup the DHCPv6 offer packet+  Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER]->OpCode = DHCP6_OPT_SERVER_ID;+  Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER]->OpLen  = NTOHS (1337);++  ASSERT_EQ (PxeBcHandleDhcp6Offer (&(PxeBcHandleDhcp6OfferTest::Private)), EFI_DEVICE_ERROR);+}++class PxeBcCacheDnsServerAddressesTest : public ::testing::Test {+public:+  PXEBC_PRIVATE_DATA Private = { 0 };++protected:+  // Add any setup code if needed+  virtual void+  SetUp (+    )+  {+  }++  // Add any cleanup code if needed+  virtual void+  TearDown (+    )+  {+  }+};++// Test Description+// Test that we cache the DNS server address from the DHCPv6 offer packet+TEST_F (PxeBcCacheDnsServerAddressesTest, BasicUsageTest) {+  UINT8                     SearchPattern[16] = { 0xDE, 0xAD, 0xBE, 0xEF, 0xDE, 0xAD, 0xBE, 0xEF, 0xDE, 0xAD, 0xBE, 0xEF, 0xDE, 0xAD, 0xBE, 0xEF };+  EFI_DHCP6_PACKET_OPTION   *Option;+  PXEBC_DHCP6_PACKET_CACHE  *Cache6 = NULL;++  Option = (EFI_DHCP6_PACKET_OPTION *)AllocateZeroPool (sizeof (EFI_DHCP6_PACKET_OPTION) + sizeof (SearchPattern));+  ASSERT_NE (Option, nullptr);++  Option->OpCode = DHCP6_OPT_SERVER_ID;+  Option->OpLen  = NTOHS (sizeof (SearchPattern));+  CopyMem (Option->Data, SearchPattern, sizeof (SearchPattern));++  Private.SelectIndex                         = 1; // SelectIndex is 1-based+  Cache6                                      = &Private.OfferBuffer[Private.SelectIndex - 1].Dhcp6;+  Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER] = Option;++  Private.DnsServer = nullptr;++  ASSERT_EQ (PxeBcCacheDnsServerAddresses (&(PxeBcCacheDnsServerAddressesTest::Private), Cache6), EFI_SUCCESS);+  ASSERT_NE (Private.DnsServer, nullptr);+  ASSERT_EQ (CompareMem (Private.DnsServer, SearchPattern, sizeof (SearchPattern)), 0);++  if (Private.DnsServer) {+    FreePool (Private.DnsServer);+  }++  if (Option) {+    FreePool (Option);+  }+}+// Test Description+// Test that we can prevent an overflow in the function+TEST_F (PxeBcCacheDnsServerAddressesTest, AttemptOverflowTest) {+  EFI_DHCP6_PACKET_OPTION   Option  = { 0 };+  PXEBC_DHCP6_PACKET_CACHE  *Cache6 = NULL;++  Private.SelectIndex                         = 1; // SelectIndex is 1-based+  Cache6                                      = &Private.OfferBuffer[Private.SelectIndex - 1].Dhcp6;+  Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER] = &Option;+  // Setup the DHCPv6 offer packet+  Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER]->OpCode = DHCP6_OPT_SERVER_ID;+  Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER]->OpLen  = NTOHS (1337);++  Private.DnsServer = NULL;++  ASSERT_EQ (PxeBcCacheDnsServerAddresses (&(PxeBcCacheDnsServerAddressesTest::Private), Cache6), EFI_DEVICE_ERROR);+  ASSERT_EQ (Private.DnsServer, nullptr);++  if (Private.DnsServer) {+    FreePool (Private.DnsServer);+  }+}++// Test Description+// Test that we can prevent an underflow in the function+TEST_F (PxeBcCacheDnsServerAddressesTest, AttemptUnderflowTest) {+  EFI_DHCP6_PACKET_OPTION   Option  = { 0 };+  PXEBC_DHCP6_PACKET_CACHE  *Cache6 = NULL;++  Private.SelectIndex                         = 1; // SelectIndex is 1-based+  Cache6                                      = &Private.OfferBuffer[Private.SelectIndex - 1].Dhcp6;+  Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER] = &Option;+  // Setup the DHCPv6 offer packet+  Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER]->OpCode = DHCP6_OPT_SERVER_ID;+  Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER]->OpLen  = NTOHS (2);++  Private.DnsServer = NULL;++  ASSERT_EQ (PxeBcCacheDnsServerAddresses (&(PxeBcCacheDnsServerAddressesTest::Private), Cache6), EFI_DEVICE_ERROR);+  ASSERT_EQ (Private.DnsServer, nullptr);++  if (Private.DnsServer) {+    FreePool (Private.DnsServer);+  }+}++// Test Description+// Test that we can handle recursive dns (multiple dns entries)+TEST_F (PxeBcCacheDnsServerAddressesTest, MultipleDnsEntries) {+  EFI_DHCP6_PACKET_OPTION   Option  = { 0 };+  PXEBC_DHCP6_PACKET_CACHE  *Cache6 = NULL;++  Private.SelectIndex                         = 1; // SelectIndex is 1-based+  Cache6                                      = &Private.OfferBuffer[Private.SelectIndex - 1].Dhcp6;+  Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER] = &Option;+  // Setup the DHCPv6 offer packet+  Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER]->OpCode = DHCP6_OPT_SERVER_ID;++  EFI_IPv6_ADDRESS  addresses[2] = {+    // 2001:db8:85a3::8a2e:370:7334+    { 0x20, 0x01, 0x0d, 0xb8, 0x85, 0xa3, 0x00, 0x00, 0x00, 0x00, 0x8a, 0x2e, 0x03, 0x70, 0x73, 0x34 },+    // fe80::d478:91c3:ecd7:4ff9+    { 0xfe, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xd4, 0x78, 0x91, 0xc3, 0xec, 0xd7, 0x4f, 0xf9 }+  };++  CopyMem (Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER]->Data, &addresses, sizeof (addresses));++  Cache6->OptList[PXEBC_DHCP6_IDX_DNS_SERVER]->OpLen = NTOHS (sizeof (addresses));++  Private.DnsServer = NULL;++  ASSERT_EQ (PxeBcCacheDnsServerAddresses (&(PxeBcCacheDnsServerAddressesTest::Private), Cache6), EFI_SUCCESS);++  ASSERT_NE (Private.DnsServer, nullptr);++  //+  // This is expected to fail until DnsServer supports multiple DNS servers+  //+  // This is tracked in https://bugzilla.tianocore.org/show_bug.cgi?id=1886+  //+  // Disabling:+  // ASSERT_EQ (CompareMem(Private.DnsServer, &addresses, sizeof(addresses)), 0);++  if (Private.DnsServer) {+    FreePool (Private.DnsServer);+  }+}diff --git a/NetworkPkg/UefiPxeBcDxe/GoogleTest/UefiPxeBcDxeGoogleTest.cpp b/NetworkPkg/UefiPxeBcDxe/GoogleTest/UefiPxeBcDxeGoogleTest.cpp
new file mode 100644
index 000000000000..cc4fdf525b62
--- /dev/null
+++ b/NetworkPkg/UefiPxeBcDxe/GoogleTest/UefiPxeBcDxeGoogleTest.cpp
@@ -0,0 +1,19 @@
+/** @file+  Acts as the main entry point for the tests for the UefiPxeBcDxe module.+  Copyright (c) Microsoft Corporation+  SPDX-License-Identifier: BSD-2-Clause-Patent+**/+#include <gtest/gtest.h>++////////////////////////////////////////////////////////////////////////////////+// Run the tests+////////////////////////////////////////////////////////////////////////////////+int+main (+  int   argc,+  char  *argv[]+  )+{+  testing::InitGoogleTest (&argc, argv);+  return RUN_ALL_TESTS ();+}-- 
2.43.0



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