From 0c7c03ee087f10d9ddda8f38d816763211f8b925 Mon Sep 17 00:00:00 2001 From: adttil <13380049+adttil@user.noreply.gitee.com> Date: Sat, 24 Feb 2024 14:28:35 +0800 Subject: [PATCH 12/13] NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45235 Patch Bug Details: PixieFail Bug #7 CVE-2023-45235 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 handling Server ID option from a DHCPv6 proxy Advertise message Change Overview: Performs two checks 1. Checks that the length of the duid is accurate > + // > + // Check that the minimum and maximum requirements are met > + // > + if ((OpLen < PXEBC_MIN_SIZE_OF_DUID) || (OpLen > PXEBC_MAX_SIZE_OF_DUID)) { > + Status = EFI_INVALID_PARAMETER; > + goto ON_ERROR; > + } 2. Ensures that the amount of data written to the buffer is tracked and never exceeds that > + // > + // Check that the option length is valid. > + // > + if ((DiscoverLen + OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN) > DiscoverLenNeeded) { > + Status = EFI_OUT_OF_RESOURCES; > + goto ON_ERROR; > + } Additional code clean up and fix for memory leak in case Option was NULL Signed-off-by: Doug Flick [MSFT] ref: https://github.com/tianocore/edk2/commit/fac297724e6cc343430cd0104e55cd7a96d1151e Signed-off-by: yexiao yexiao7@huawei.com --- NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c | 76 ++++++++++++++++++++++------ NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.h | 17 +++++++ 2 files changed, 77 insertions(+), 16 deletions(-) diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c index 7a20ba56..578a2615 100644 --- a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c +++ b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c @@ -883,6 +883,7 @@ PxeBcRequestBootService ( EFI_STATUS Status; EFI_DHCP6_PACKET *IndexOffer; UINT8 *Option; + UINTN DiscoverLenNeeded; PxeBc = &Private->PxeBc; Request = Private->Dhcp6Request; @@ -895,7 +896,8 @@ PxeBcRequestBootService ( return EFI_DEVICE_ERROR; } - Discover = AllocateZeroPool (sizeof (EFI_PXE_BASE_CODE_DHCPV6_PACKET)); + DiscoverLenNeeded = sizeof (EFI_PXE_BASE_CODE_DHCPV6_PACKET); + Discover = AllocateZeroPool (DiscoverLenNeeded); if (Discover == NULL) { return EFI_OUT_OF_RESOURCES; } @@ -920,16 +922,33 @@ PxeBcRequestBootService ( DHCP6_OPT_SERVER_ID ); if (Option == NULL) { - return EFI_NOT_FOUND; + Status = EFI_NOT_FOUND; + goto ON_ERROR; } // // Add Server ID Option. // OpLen = NTOHS (((EFI_DHCP6_PACKET_OPTION *) Option)->OpLen); - CopyMem (DiscoverOpt, Option, OpLen + 4); - DiscoverOpt += (OpLen + 4); - DiscoverLen += (OpLen + 4); + // + // Check that the minimum and maximum requirements are met + // + if ((OpLen < PXEBC_MIN_SIZE_OF_DUID) || (OpLen > PXEBC_MAX_SIZE_OF_DUID)) { + Status = EFI_INVALID_PARAMETER; + goto ON_ERROR; + } + + // + // Check that the option length is valid. + // + if ((DiscoverLen + OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN) > DiscoverLenNeeded) { + Status = EFI_OUT_OF_RESOURCES; + goto ON_ERROR; + } + + CopyMem (DiscoverOpt, Option, OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN); + DiscoverOpt += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN); + DiscoverLen += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN); } while (RequestLen < Request->Length) { @@ -939,15 +958,23 @@ PxeBcRequestBootService ( OpCode != EFI_DHCP6_IA_TYPE_TA && OpCode != DHCP6_OPT_SERVER_ID ) { + // + // Check that the option length is valid. + // + if (DiscoverLen + OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN > DiscoverLenNeeded) { + Status = EFI_OUT_OF_RESOURCES; + goto ON_ERROR; + } + // // Copy all the options except IA option and Server ID // - CopyMem (DiscoverOpt, RequestOpt, OpLen + 4); - DiscoverOpt += (OpLen + 4); - DiscoverLen += (OpLen + 4); + CopyMem (DiscoverOpt, RequestOpt, OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN); + DiscoverOpt += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN); + DiscoverLen += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN); } - RequestOpt += (OpLen + 4); - RequestLen += (OpLen + 4); + RequestOpt += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN); + RequestLen += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN); } // @@ -2135,6 +2162,7 @@ PxeBcDhcp6Discover ( UINT16 OpLen; UINT32 Xid; EFI_STATUS Status; + UINTN DiscoverLenNeeded; PxeBc = &Private->PxeBc; Mode = PxeBc->Mode; @@ -2150,7 +2178,8 @@ PxeBcDhcp6Discover ( return EFI_DEVICE_ERROR; } - Discover = AllocateZeroPool (sizeof (EFI_PXE_BASE_CODE_DHCPV6_PACKET)); + DiscoverLenNeeded = sizeof (EFI_PXE_BASE_CODE_DHCPV6_PACKET); + Discover = AllocateZeroPool (DiscoverLenNeeded); if (Discover == NULL) { return EFI_OUT_OF_RESOURCES; } @@ -2166,20 +2195,35 @@ PxeBcDhcp6Discover ( DiscoverLen = sizeof (EFI_DHCP6_HEADER); RequestLen = DiscoverLen; + // + // The request packet is generated by the UEFI network stack. In the DHCP4 DORA and DHCP6 SARR sequence, + // the first (discover in DHCP4 and solicit in DHCP6) and third (request in both DHCP4 and DHCP6) are + // generated by the DHCP client (the UEFI network stack in this case). By the time this function executes, + // the DHCP sequence already has been executed once (see UEFI Specification Figures 24.2 and 24.3), with + // Private->Dhcp6Request being a cached copy of the DHCP6 request packet that UEFI network stack previously + // generated and sent. + // + // Therefore while this code looks like it could overflow, in practice it's not possible. + // while (RequestLen < Request->Length) { OpCode = NTOHS (((EFI_DHCP6_PACKET_OPTION *) RequestOpt)->OpCode); OpLen = NTOHS (((EFI_DHCP6_PACKET_OPTION *) RequestOpt)->OpLen); if (OpCode != EFI_DHCP6_IA_TYPE_NA && OpCode != EFI_DHCP6_IA_TYPE_TA) { + if (DiscoverLen + OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN > DiscoverLenNeeded) { + Status = EFI_OUT_OF_RESOURCES; + goto ON_ERROR; + } + // // Copy all the options except IA option. // - CopyMem (DiscoverOpt, RequestOpt, OpLen + 4); - DiscoverOpt += (OpLen + 4); - DiscoverLen += (OpLen + 4); + CopyMem (DiscoverOpt, RequestOpt, OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN); + DiscoverOpt += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN); + DiscoverLen += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN); } - RequestOpt += (OpLen + 4); - RequestLen += (OpLen + 4); + RequestOpt += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN); + RequestLen += (OpLen + PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN); } Status = PxeBc->UdpWrite ( diff --git a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.h b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.h index ae4be775..fda5f502 100644 --- a/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.h +++ b/NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.h @@ -35,6 +35,23 @@ #define PXEBC_ADDR_START_DELIMITER '[' #define PXEBC_ADDR_END_DELIMITER ']' +// +// A DUID consists of a 2-octet type code represented in network byte +// order, followed by a variable number of octets that make up the +// actual identifier. The length of the DUID (not including the type +// code) is at least 1 octet and at most 128 octets. +// +#define PXEBC_MIN_SIZE_OF_DUID (sizeof(UINT16) + 1) +#define PXEBC_MAX_SIZE_OF_DUID (sizeof(UINT16) + 128) + +// +// This define represents the combineds code and length field from +// https://datatracker.ietf.org/doc/html/rfc3315#section-22.1 +// +#define PXEBC_COMBINED_SIZE_OF_OPT_CODE_AND_LEN \ + (sizeof (((EFI_DHCP6_PACKET_OPTION *)0)->OpCode) + \ + sizeof (((EFI_DHCP6_PACKET_OPTION *)0)->OpLen)) + #define GET_NEXT_DHCP6_OPTION(Opt) \ (EFI_DHCP6_PACKET_OPTION *) ((UINT8 *) (Opt) + \ sizeof (EFI_DHCP6_PACKET_OPTION) + (NTOHS ((Opt)->OpLen)) - 1) -- 2.33.0