Re: [edk2-devel] [PATCH v2 03/15] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45230 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: Douglas Flick [MSFT] <doug.edk2@gmail.com>; Kasbekar, Saloni <saloni.kasbekar@intel.com>; Clark-williams, Zachary <zachary.clark-williams@intel.com>
Subject: [PATCH v2 03/15] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45230 Unit Tests

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

Confirms that reported issue...

"Buffer overflow in the DHCPv6 client via a long Server ID option"

..has been corrected by the provided patch.

Tests the following functions to ensure they appropriately handle untrusted data (either too long or too small) to prevent a buffer
overflow:

Dhcp6AppendOption
Dhcp6AppendETOption
Dhcp6AppendIaOption

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/Dhcp6DxeGoogleTest.inf         |  43 ++
 .../GoogleTest/Dhcp6DxeGoogleTest.cpp         |  20 +
 .../Dhcp6Dxe/GoogleTest/Dhcp6IoGoogleTest.cpp | 478 ++++++++++++++++++
 4 files changed, 542 insertions(+)
 create mode 100644 NetworkPkg/Dhcp6Dxe/GoogleTest/Dhcp6DxeGoogleTest.inf
 create mode 100644 NetworkPkg/Dhcp6Dxe/GoogleTest/Dhcp6DxeGoogleTest.cpp
 create mode 100644 NetworkPkg/Dhcp6Dxe/GoogleTest/Dhcp6IoGoogleTest.cpp

diff --git a/NetworkPkg/Test/NetworkPkgHostTest.dsc b/NetworkPkg/Test/NetworkPkgHostTest.dsc
index 1aeca5c5b353..20bc90b1728d 100644
--- a/NetworkPkg/Test/NetworkPkgHostTest.dsc
+++ b/NetworkPkg/Test/NetworkPkgHostTest.dsc
@@ -24,6 +24,7 @@ [Components]
   #   # Build HOST_APPLICATION that tests NetworkPkg   #+  NetworkPkg/Dhcp6Dxe/GoogleTest/Dhcp6DxeGoogleTest.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/Dhcp6Dxe/GoogleTest/Dhcp6DxeGoogleTest.inf b/NetworkPkg/Dhcp6Dxe/GoogleTest/Dhcp6DxeGoogleTest.inf
new file mode 100644
index 000000000000..8e9119a37158
--- /dev/null
+++ b/NetworkPkg/Dhcp6Dxe/GoogleTest/Dhcp6DxeGoogleTest.inf
@@ -0,0 +1,43 @@
+## @file+# Unit test suite for the Dhcp6Dxe using Google Test+#+# Copyright (c) Microsoft Corporation.<BR>+# SPDX-License-Identifier: BSD-2-Clause-Patent+##+[Defines]+  INF_VERSION         = 0x00010017+  BASE_NAME           = Dhcp6DxeGoogleTest+  FILE_GUID           = 1D2A4C65-38C8-4C2F-BB60-B5FA49625AA9+  VERSION_STRING      = 1.0+  MODULE_TYPE         = HOST_APPLICATION+#+# The following information is for reference only and not required by the build tools.+#+#  VALID_ARCHITECTURES           = IA32 X64 AARCH64+#+[Sources]+  Dhcp6DxeGoogleTest.cpp+  Dhcp6IoGoogleTest.cpp+  ../Dhcp6Io.c+  ../Dhcp6Utility.c++[Packages]+  MdePkg/MdePkg.dec+  MdeModulePkg/MdeModulePkg.dec+  UnitTestFrameworkPkg/UnitTestFrameworkPkg.dec+  NetworkPkg/NetworkPkg.dec++[LibraryClasses]+  GoogleTestLib+  DebugLib+  NetLib+  PcdLib++[Protocols]+  gEfiDhcp6ServiceBindingProtocolGuid++[Pcd]+  gEfiNetworkPkgTokenSpaceGuid.PcdDhcp6UidType++[Guids]+  gZeroGuiddiff --git a/NetworkPkg/Dhcp6Dxe/GoogleTest/Dhcp6DxeGoogleTest.cpp b/NetworkPkg/Dhcp6Dxe/GoogleTest/Dhcp6DxeGoogleTest.cpp
new file mode 100644
index 000000000000..9aeced2f9156
--- /dev/null
+++ b/NetworkPkg/Dhcp6Dxe/GoogleTest/Dhcp6DxeGoogleTest.cpp
@@ -0,0 +1,20 @@
+/** @file+  Acts as the main entry point for the tests for the Dhcp6Dxe 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 ();+}diff --git a/NetworkPkg/Dhcp6Dxe/GoogleTest/Dhcp6IoGoogleTest.cpp b/NetworkPkg/Dhcp6Dxe/GoogleTest/Dhcp6IoGoogleTest.cpp
new file mode 100644
index 000000000000..7ee40e4af480
--- /dev/null
+++ b/NetworkPkg/Dhcp6Dxe/GoogleTest/Dhcp6IoGoogleTest.cpp
@@ -0,0 +1,478 @@
+/** @file+  Tests for Dhcp6Io.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 <Library/BaseMemoryLib.h>+  #include "../Dhcp6Impl.h"+  #include "../Dhcp6Utility.h"+}++////////////////////////////////////////////////////////////////////////+// Defines+////////////////////////////////////////////////////////////////////////++#define DHCP6_PACKET_MAX_LEN  1500++////////////////////////////////////////////////////////////////////////+////////////////////////////////////////////////////////////////////////+// Symbol Definitions+// These functions are not directly under test - but required to compile+////////////////////////////////////////////////////////////////////////++// This definition is used by this test but is also required to compile+// by Dhcp6Io.c+EFI_IPv6_ADDRESS  mAllDhcpRelayAndServersAddress = {+  { 0xFF, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 2 }+};++EFI_STATUS+EFIAPI+UdpIoSendDatagram (+  IN  UDP_IO           *UdpIo,+  IN  NET_BUF          *Packet,+  IN  UDP_END_POINT    *EndPoint OPTIONAL,+  IN  EFI_IP_ADDRESS   *Gateway  OPTIONAL,+  IN  UDP_IO_CALLBACK  CallBack,+  IN  VOID             *Context+  )+{+  return EFI_SUCCESS;+}++EFI_STATUS+EFIAPI+UdpIoRecvDatagram (+  IN  UDP_IO           *UdpIo,+  IN  UDP_IO_CALLBACK  CallBack,+  IN  VOID             *Context,+  IN  UINT32           HeadLen+  )+{+  return EFI_SUCCESS;+}++////////////////////////////////////////////////////////////////////////+// Dhcp6AppendOptionTest Tests+////////////////////////////////////////////////////////////////////////++class Dhcp6AppendOptionTest : public ::testing::Test {+public:+  UINT8 *Buffer = NULL;+  EFI_DHCP6_PACKET *Packet;++protected:+  // Add any setup code if needed+  virtual void+  SetUp (+    )+  {+    // Initialize any resources or variables+    Buffer = (UINT8 *)AllocateZeroPool (DHCP6_PACKET_MAX_LEN);+    ASSERT_NE (Buffer, (UINT8 *)NULL);++    Packet       = (EFI_DHCP6_PACKET *)Buffer;+    Packet->Size = DHCP6_PACKET_MAX_LEN;+  }++  // Add any cleanup code if needed+  virtual void+  TearDown (+    )+  {+    // Clean up any resources or variables+    if (Buffer != NULL) {+      FreePool (Buffer);+    }+  }+};++// Test Description:+// Attempt to append an option to a packet that is too small by a duid that is too large+TEST_F (Dhcp6AppendOptionTest, InvalidDataExpectBufferTooSmall) {+  UINT8           *Cursor;+  EFI_DHCP6_DUID  *UntrustedDuid;+  EFI_STATUS      Status;++  UntrustedDuid = (EFI_DHCP6_DUID *)AllocateZeroPool (sizeof (EFI_DHCP6_DUID));+  ASSERT_NE (UntrustedDuid, (EFI_DHCP6_DUID *)NULL);++  UntrustedDuid->Length = NTOHS (0xFFFF);++  Cursor = Dhcp6AppendOptionTest::Packet->Dhcp6.Option;++  Status = Dhcp6AppendOption (+             Dhcp6AppendOptionTest::Packet,+             &Cursor,+             HTONS (Dhcp6OptServerId),+             UntrustedDuid->Length,+             UntrustedDuid->Duid+             );++  ASSERT_EQ (Status, EFI_BUFFER_TOO_SMALL);+}++// Test Description:+// Attempt to append an option to a packet that is large enough+TEST_F (Dhcp6AppendOptionTest, ValidDataExpectSuccess) {+  UINT8           *Cursor;+  EFI_DHCP6_DUID  *UntrustedDuid;+  EFI_STATUS      Status;+  UINTN           OriginalLength;++  UINT8  Duid[6] = { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05 };++  Packet->Length = sizeof (EFI_DHCP6_HEADER);+  OriginalLength = Packet->Length;++  UntrustedDuid = (EFI_DHCP6_DUID *)AllocateZeroPool (sizeof (EFI_DHCP6_DUID));+  ASSERT_NE (UntrustedDuid, (EFI_DHCP6_DUID *)NULL);++  UntrustedDuid->Length = NTOHS (sizeof (Duid));+  CopyMem (UntrustedDuid->Duid, Duid, sizeof (Duid));++  Cursor = Dhcp6AppendOptionTest::Packet->Dhcp6.Option;++  Status = Dhcp6AppendOption (+             Dhcp6AppendOptionTest::Packet,+             &Cursor,+             HTONS (Dhcp6OptServerId),+             UntrustedDuid->Length,+             UntrustedDuid->Duid+             );++  ASSERT_EQ (Status, EFI_SUCCESS);++  // verify that the pointer to cursor moved by the expected amount+  ASSERT_EQ (Cursor, (UINT8 *)Dhcp6AppendOptionTest::Packet->Dhcp6.Option + sizeof (Duid) + 4);++  // verify that the length of the packet is now the expected amount+  ASSERT_EQ (Dhcp6AppendOptionTest::Packet->Length, OriginalLength + sizeof (Duid) + 4);+}++////////////////////////////////////////////////////////////////////////+// Dhcp6AppendETOption Tests+////////////////////////////////////////////////////////////////////////++class Dhcp6AppendETOptionTest : public ::testing::Test {+public:+  UINT8 *Buffer = NULL;+  EFI_DHCP6_PACKET *Packet;++protected:+  // Add any setup code if needed+  virtual void+  SetUp (+    )+  {+    // Initialize any resources or variables+    Buffer = (UINT8 *)AllocateZeroPool (DHCP6_PACKET_MAX_LEN);+    ASSERT_NE (Buffer, (UINT8 *)NULL);++    Packet         = (EFI_DHCP6_PACKET *)Buffer;+    Packet->Size   = DHCP6_PACKET_MAX_LEN;+    Packet->Length = sizeof (EFI_DHCP6_HEADER);+  }++  // Add any cleanup code if needed+  virtual void+  TearDown (+    )+  {+    // Clean up any resources or variables+    if (Buffer != NULL) {+      FreePool (Buffer);+    }+  }+};++// Test Description:+// Attempt to append an option to a packet that is too small by a duid that is too large+TEST_F (Dhcp6AppendETOptionTest, InvalidDataExpectBufferTooSmall) {+  UINT8           *Cursor;+  EFI_STATUS      Status;+  DHCP6_INSTANCE  Instance;+  UINT16          ElapsedTimeVal;+  UINT16          *ElapsedTime;++  Cursor      = Dhcp6AppendETOptionTest::Packet->Dhcp6.Option;+  ElapsedTime = &ElapsedTimeVal;++  Packet->Length = Packet->Size - 2;++  Status = Dhcp6AppendETOption (+             Dhcp6AppendETOptionTest::Packet,+             &Cursor,+             &Instance, // Instance is not used in this function+             &ElapsedTime+             );++  // verify that we error out because the packet is too small for the option header+  ASSERT_EQ (Status, EFI_BUFFER_TOO_SMALL);++  // reset the length+  Packet->Length = sizeof (EFI_DHCP6_HEADER);+}++// Test Description:+// Attempt to append an option to a packet that is large enough+TEST_F (Dhcp6AppendETOptionTest, ValidDataExpectSuccess) {+  UINT8           *Cursor;+  EFI_STATUS      Status;+  DHCP6_INSTANCE  Instance;+  UINT16          ElapsedTimeVal;+  UINT16          *ElapsedTime;+  UINTN           ExpectedSize;+  UINTN           OriginalLength;++  Cursor         = Dhcp6AppendETOptionTest::Packet->Dhcp6.Option;+  ElapsedTime    = &ElapsedTimeVal;+  ExpectedSize   = 6;+  OriginalLength = Packet->Length;++  Status = Dhcp6AppendETOption (+             Dhcp6AppendETOptionTest::Packet,+             &Cursor,+             &Instance, // Instance is not used in this function+             &ElapsedTime+             );++  // verify that the status is EFI_SUCCESS+  ASSERT_EQ (Status, EFI_SUCCESS);++  // verify that the pointer to cursor moved by the expected amount+  ASSERT_EQ (Cursor, (UINT8 *)Dhcp6AppendETOptionTest::Packet->Dhcp6.Option + ExpectedSize);++  // verify that the length of the packet is now the expected amount+  ASSERT_EQ (Dhcp6AppendETOptionTest::Packet->Length, OriginalLength + ExpectedSize);+}++////////////////////////////////////////////////////////////////////////+// Dhcp6AppendIaOption Tests+////////////////////////////////////////////////////////////////////////++class Dhcp6AppendIaOptionTest : public ::testing::Test {+public:+  UINT8 *Buffer = NULL;+  EFI_DHCP6_PACKET *Packet;+  EFI_DHCP6_IA *Ia;++protected:+  // Add any setup code if needed+  virtual void+  SetUp (+    )+  {+    // Initialize any resources or variables+    Buffer = (UINT8 *)AllocateZeroPool (DHCP6_PACKET_MAX_LEN);+    ASSERT_NE (Buffer, (UINT8 *)NULL);++    Packet       = (EFI_DHCP6_PACKET *)Buffer;+    Packet->Size = DHCP6_PACKET_MAX_LEN;++    Ia = (EFI_DHCP6_IA *)AllocateZeroPool (sizeof (EFI_DHCP6_IA) + sizeof (EFI_DHCP6_IA_ADDRESS) * 2);+    ASSERT_NE (Ia, (EFI_DHCP6_IA *)NULL);++    CopyMem (Ia->IaAddress, mAllDhcpRelayAndServersAddress.Addr, sizeof (EFI_IPv6_ADDRESS));+    CopyMem (Ia->IaAddress + 1, mAllDhcpRelayAndServersAddress.Addr, sizeof (EFI_IPv6_ADDRESS));++    Ia->IaAddressCount = 2;+  }++  // Add any cleanup code if needed+  virtual void+  TearDown (+    )+  {+    // Clean up any resources or variables+    if (Buffer != NULL) {+      FreePool (Buffer);+    }++    if (Ia != NULL) {+      FreePool (Ia);+    }+  }+};++// Test Description:+// Attempt to append an option to a packet that doesn't have enough space+// for the option header+TEST_F (Dhcp6AppendIaOptionTest, IaNaInvalidDataExpectBufferTooSmall) {+  UINT8       *Cursor;+  EFI_STATUS  Status;++  Packet->Length = Packet->Size - 2;++  Ia->Descriptor.Type = Dhcp6OptIana;+  Ia->Descriptor.IaId = 0x12345678;++  Cursor = Dhcp6AppendIaOptionTest::Packet->Dhcp6.Option;++  Status = Dhcp6AppendIaOption (+             Dhcp6AppendIaOptionTest::Packet,+             &Cursor,+             Ia,+             0x12345678,+             0x11111111,+             Dhcp6OptIana+             );++  // verify that we error out because the packet is too small for the option header+  ASSERT_EQ (Status, EFI_BUFFER_TOO_SMALL);++  // reset the length+  Packet->Length = sizeof (EFI_DHCP6_HEADER);+}++// Test Description:+// Attempt to append an option to a packet that doesn't have enough space+// for the option header+TEST_F (Dhcp6AppendIaOptionTest, IaTaInvalidDataExpectBufferTooSmall) {+  UINT8       *Cursor;+  EFI_STATUS  Status;++  // Use up nearly all the space in the packet+  Packet->Length = Packet->Size - 2;++  Ia->Descriptor.Type = Dhcp6OptIata;+  Ia->Descriptor.IaId = 0x12345678;++  Cursor = Dhcp6AppendIaOptionTest::Packet->Dhcp6.Option;++  Status = Dhcp6AppendIaOption (+             Dhcp6AppendIaOptionTest::Packet,+             &Cursor,+             Ia,+             0,+             0,+             Dhcp6OptIata+             );++  // verify that we error out because the packet is too small for the option header+  ASSERT_EQ (Status, EFI_BUFFER_TOO_SMALL);++  // reset the length+  Packet->Length = sizeof (EFI_DHCP6_HEADER);+}++TEST_F (Dhcp6AppendIaOptionTest, IaNaValidDataExpectSuccess) {+  UINT8       *Cursor;+  EFI_STATUS  Status;+  UINTN       ExpectedSize;+  UINTN       OriginalLength;++  //+  // 2 bytes for the option header type+  //+  ExpectedSize = 2;+  //+  // 2 bytes for the option header length+  //+  ExpectedSize += 2;+  //+  // 4 bytes for the IAID+  //+  ExpectedSize += 4;+  //+  // + 4 bytes for the T1+  //+  ExpectedSize += 4;+  //+  // + 4 bytes for the T2+  //+  ExpectedSize += 4;+  //+  // + (4 + sizeof (EFI_DHCP6_IA_ADDRESS)) * 2;+  //   + 2 bytes for the option header type+  //   + 2 bytes for the option header length+  //   + sizeof (EFI_DHCP6_IA_ADDRESS) for the IA Address+  //+  ExpectedSize += (4 + sizeof (EFI_DHCP6_IA_ADDRESS)) * 2;++  Cursor = Dhcp6AppendIaOptionTest::Packet->Dhcp6.Option;++  Packet->Length = sizeof (EFI_DHCP6_HEADER);+  OriginalLength = Packet->Length;++  Ia->Descriptor.Type = Dhcp6OptIana;+  Ia->Descriptor.IaId = 0x12345678;++  Status = Dhcp6AppendIaOption (+             Dhcp6AppendIaOptionTest::Packet,+             &Cursor,+             Ia,+             0x12345678,+             0x12345678,+             Dhcp6OptIana+             );++  // verify that the pointer to cursor moved by the expected amount+  ASSERT_EQ (Cursor, (UINT8 *)Dhcp6AppendIaOptionTest::Packet->Dhcp6.Option + ExpectedSize);++  // verify that the length of the packet is now the expected amount+  ASSERT_EQ (Dhcp6AppendIaOptionTest::Packet->Length, OriginalLength + ExpectedSize);++  // verify that the status is EFI_SUCCESS+  ASSERT_EQ (Status, EFI_SUCCESS);+}++TEST_F (Dhcp6AppendIaOptionTest, IaTaValidDataExpectSuccess) {+  UINT8       *Cursor;+  EFI_STATUS  Status;+  UINTN       ExpectedSize;+  UINTN       OriginalLength;++  //+  // 2 bytes for the option header type+  //+  ExpectedSize = 2;+  //+  // 2 bytes for the option header length+  //+  ExpectedSize += 2;+  //+  // 4 bytes for the IAID+  //+  ExpectedSize += 4;+  //+  // + (4 + sizeof (EFI_DHCP6_IA_ADDRESS)) * 2;+  //   + 2 bytes for the option header type+  //   + 2 bytes for the option header length+  //   + sizeof (EFI_DHCP6_IA_ADDRESS) for the IA Address+  //+  ExpectedSize += (4 + sizeof (EFI_DHCP6_IA_ADDRESS)) * 2;++  Cursor = Dhcp6AppendIaOptionTest::Packet->Dhcp6.Option;++  Packet->Length = sizeof (EFI_DHCP6_HEADER);+  OriginalLength = Packet->Length;++  Ia->Descriptor.Type = Dhcp6OptIata;+  Ia->Descriptor.IaId = 0x12345678;++  Status = Dhcp6AppendIaOption (+             Dhcp6AppendIaOptionTest::Packet,+             &Cursor,+             Ia,+             0,+             0,+             Dhcp6OptIata+             );++  // verify that the pointer to cursor moved by the expected amount+  ASSERT_EQ (Cursor, (UINT8 *)Dhcp6AppendIaOptionTest::Packet->Dhcp6.Option + ExpectedSize);++  // verify that the length of the packet is now the expected amount+  ASSERT_EQ (Dhcp6AppendIaOptionTest::Packet->Length, OriginalLength + ExpectedSize);++  // verify that the status is EFI_SUCCESS+  ASSERT_EQ (Status, EFI_SUCCESS);+}-- 
2.43.0



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