From 80b34c0f56228353c174f9ff739d0755c62d76cf Mon Sep 17 00:00:00 2001 From: Jon Maloy Date: Fri, 16 Feb 2024 10:48:05 -0500 Subject: [PATCH 10/15] NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45235 Unit Tests RH-Author: Jon Maloy RH-MergeRequest: 56: Pixiefail issues in NetworkPkg package RH-Jira: RHEL-21840 RHEL-21844 RHEL-21846 RHEL-21848 RHEL-21850 RHEL-21852 RH-Acked-by: Gerd Hoffmann RH-Acked-by: Oliver Steffen RH-Commit: [10/15] 5dbf3f771506ff9a0c28827c568d04e825572658 JIRA: https://issues.redhat.com/browse/RHEL-21852 CVE: CVE-2022-45235 Upstream: Merged commit ff2986358f75d8f58ef08a66fe673539c9c48f41 Author: Doug Flick Date: Fri Jan 26 05:54:56 2024 +0800 NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45235 Unit Tests 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 Cc: Zachary Clark-williams Signed-off-by: Doug Flick [MSFT] Reviewed-by: Saloni Kasbekar Signed-off-by: Jon Maloy --- NetworkPkg/Test/NetworkPkgHostTest.dsc | 5 +- .../GoogleTest/PxeBcDhcp6GoogleTest.cpp | 278 +++++++++++++++++- .../GoogleTest/PxeBcDhcp6GoogleTest.h | 18 ++ 3 files changed, 298 insertions(+), 3 deletions(-) diff --git a/NetworkPkg/Test/NetworkPkgHostTest.dsc b/NetworkPkg/Test/NetworkPkgHostTest.dsc index c8a991e5c1..1010a80a15 100644 --- a/NetworkPkg/Test/NetworkPkgHostTest.dsc +++ b/NetworkPkg/Test/NetworkPkgHostTest.dsc @@ -26,7 +26,10 @@ # NetworkPkg/Dhcp6Dxe/GoogleTest/Dhcp6DxeGoogleTest.inf NetworkPkg/Ip6Dxe/GoogleTest/Ip6DxeGoogleTest.inf - NetworkPkg/UefiPxeBcDxe/GoogleTest/UefiPxeBcDxeGoogleTest.inf + NetworkPkg/UefiPxeBcDxe/GoogleTest/UefiPxeBcDxeGoogleTest.inf { + + 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.cpp b/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.cpp index 8260eeee50..bd423ebadf 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 +#include +#include +#include extern "C" { #include @@ -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 + ); +} diff --git a/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.h b/NetworkPkg/UefiPxeBcDxe/GoogleTest/PxeBcDhcp6GoogleTest.h index b17c314791..0d825e4425 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_ -- 2.39.3