Skip to content

Commit

Permalink
NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45235 Patch
Browse files Browse the repository at this point in the history 
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4540

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

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>
Reviewed-by: Saloni Kasbekar <saloni.kasbekar@intel.com>
Reviewed-by: Jeff Brasen <jbrasen@nvidia.com>
Tested-by: Jeff Brasen <jbrasen@nvidia.com>
  • Loading branch information
@Flickdm @jgarver
Flickdm authored and jgarver committed Feb 9, 2024
1 parent 029a646 commit fb8414e
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 16 deletions.
77 changes: 61 additions & 16 deletions NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -887,6 +887,7 @@ PxeBcRequestBootService (
EFI_STATUS Status;
EFI_DHCP6_PACKET *IndexOffer;
UINT8 *Option;
UINTN DiscoverLenNeeded;

PxeBc = &Private->PxeBc;
Request = Private->Dhcp6Request;
Expand All @@ -899,7 +900,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;
}
Expand All @@ -924,16 +926,34 @@ 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) {
Expand All @@ -944,16 +964,24 @@ PxeBcRequestBootService (
(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);
}

//
Expand Down Expand Up @@ -2154,6 +2182,7 @@ PxeBcDhcp6Discover (
UINT16 OpLen;
UINT32 Xid;
EFI_STATUS Status;
UINTN DiscoverLenNeeded;

PxeBc = &Private->PxeBc;
Mode = PxeBc->Mode;
Expand All @@ -2169,7 +2198,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;
}
Expand All @@ -2185,22 +2215,37 @@ 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 (
Expand Down
17 changes: 17 additions & 0 deletions NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,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)
Expand Down

0 comments on commit fb8414e

Please sign in to comment.