Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CHERRY-PICK] Backports PxeFail Vulnerability Patches to Release/202208 #911

Draft
wants to merge 19 commits into
base: release/202208
Choose a base branch
from
Draft

[CHERRY-PICK] Backports PxeFail Vulnerability Patches to Release/202208 #911

wants to merge 19 commits into from
+3,717 −353

Conversation

Flickdm
Copy link
Member

@Flickdm Flickdm commented Jun 13, 2024 •
edited
Loading

Description

This represents the first half of the security patches for PxeFail that are the easiest for a platform to consume without breaking changes.

Covers the following CVEs:

Links:
https://github.com/quarkslab/pixiefail
https://blog.quarkslab.com/pixiefail-nine-vulnerabilities-in-tianocores-edk-ii-ipv6-network-stack.html

These patches were taken with one manual merge conflict in a google test.

To recreate cherry-pick from edk2 use

git cherry-pick f31453e8d6542461d92d835e0b79fec8b039174d^..1d0b95f6457d225c5108302a9da74b4ed7aa5a38
git cherry-pick 1c440a5eceedc64e892877eeac0f1a4938f5abbb^..5fd3078a2e08f607dc86a16c1b184b6e30a34a49
  • Impacts functionality?
  • Impacts security?
  • Patches the PXE specific related vulnerabilities with breaking changes to patch the others following up in a subsequent PR
  • Breaking change?
  • Includes tests?
  • Yes but without changes to backport Google Test they will not run
  • Includes documentation?

How This Was Tested

TODO

Integration Instructions

N/A

@Flickdm Flickdm added impact:breaking-change Requires integration attention impact:security Has a security impact labels Jun 13, 2024
@Flickdm Flickdm force-pushed the backport/security_fixes/networkpkg/pxefail branch from 3bf060e to 98a0e4b Compare June 17, 2024 04:49
Doug Flick via groups.io and others added 19 commits June 16, 2024 21:56
@Flickdm
NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45230 Patch
4df3aa2 
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4535

Bug Details:
PixieFail Bug #2
CVE-2023-45230
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

Changes Overview:
> -UINT8 *
> +EFI_STATUS
>  Dhcp6AppendOption (
> -  IN OUT UINT8   *Buf,
> -  IN     UINT16  OptType,
> -  IN     UINT16  OptLen,
> -  IN     UINT8   *Data
> +  IN OUT EFI_DHCP6_PACKET  *Packet,
> +  IN OUT UINT8             **PacketCursor,
> +  IN     UINT16            OptType,
> +  IN     UINT16            OptLen,
> +  IN     UINT8             *Data
>    );

Dhcp6AppendOption() and variants can return errors now.  All callsites
are adapted accordingly.

It gets passed in EFI_DHCP6_PACKET as additional parameter ...

> +  //
> +  // Verify the PacketCursor is within the packet
> +  //
> +  if (  (*PacketCursor < Packet->Dhcp6.Option)
> +     || (*PacketCursor >= Packet->Dhcp6.Option +
 (Packet->Size - sizeof (EFI_DHCP6_HEADER))))
> +  {
> +    return EFI_INVALID_PARAMETER;
> +  }

... so it can look at Packet->Size when checking buffer space.
Also to allow Packet->Length updates.

Lots of checks added.

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>

Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
@Flickdm
NetworkPkg: : Add Unit tests to CI and create Host Test DSC
d9c2632 
Adds Host Based testing to the NetworkPkg

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>

Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
@Flickdm
NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45230 Unit Tests
f05b002 
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 <[email protected]>
Cc: Zachary Clark-williams <[email protected]>

Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
@Flickdm
NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Patch
bd1fffb 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4534

Bug Details:
PixieFail Bug #1
CVE-2023-45229
CVSS 6.5 : CVSS:3.1/AV:A/AC:L/PR:N/UI:N/S:U/C:H/I:N/A:N
CWE-125 Out-of-bounds Read

Change Overview:

Introduce Dhcp6SeekInnerOptionSafe which performs checks before seeking
the Inner Option from a DHCP6 Option.

>
> EFI_STATUS
> Dhcp6SeekInnerOptionSafe (
>  IN  UINT16  IaType,
>  IN  UINT8   *Option,
>  IN  UINT32  OptionLen,
>  OUT UINT8   **IaInnerOpt,
>  OUT UINT16  *IaInnerLen
>  );
>

Lots of code cleanup to improve code readability.

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>

Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
@Flickdm
NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Unit Tests
fd84d36 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4534

These tests confirm that the report bug...

"Out-of-bounds read when processing IA_NA/IA_TA options in a
DHCPv6 Advertise message"

..has been patched.

The following functions are tested to confirm an out of bounds read is
patched and that the correct statuses are returned:

Dhcp6SeekInnerOptionSafe
Dhcp6SeekStsOption

TCBZ4534
CVE-2023-45229
CVSS 6.5 : CVSS:3.1/AV:A/AC:L/PR:N/UI:N/S:U/C:H/I:N/A:N
CWE-125 Out-of-bounds Read

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>

Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
@Flickdm
NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45231 Patch
6093423 
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4536

Bug Overview:
PixieFail Bug #3
CVE-2023-45231
CVSS 6.5 : CVSS:3.1/AV:A/AC:L/PR:N/UI:N/S:U/C:H/I:N/A:N
CWE-125 Out-of-bounds Read

Out-of-bounds read when handling a ND Redirect message with truncated
options

Change Overview:

Adds a check to prevent truncated options from being parsed
+  //
+  // Cannot process truncated options.
+  // Cannot process options with a length of 0 as there is no Type
field.
+  //
+  if (OptionLen < sizeof (IP6_OPTION_HEADER)) {
+    return FALSE;
+  }

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>

Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
@Flickdm
NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45231 Unit Tests
75b7a24 
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4536

Validates that the patch for...

Out-of-bounds read when handling a ND Redirect message with truncated
options

.. has been fixed

Tests the following function to ensure that an out of bounds read does
not occur
Ip6OptionValidation

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>

Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
@Flickdm
NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45232 Patch
00f9db0 
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4537
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4538

Bug Details:
PixieFail Bug #4
CVE-2023-45232
CVSS 7.5 : CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H
CWE-835 Loop with Unreachable Exit Condition ('Infinite Loop')

Infinite loop when parsing unknown options in the Destination Options
header

PixieFail Bug #5
CVE-2023-45233
CVSS 7.5 : CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H
CWE-835 Loop with Unreachable Exit Condition ('Infinite Loop')

Infinite loop when parsing a PadN option in the Destination Options
header

Change Overview:

Most importantly this change corrects the following incorrect math
and cleans up the code.

>   // It is a PadN option
>   //
> - Offset = (UINT8)(Offset + *(Option + Offset + 1) + 2);
> + OptDataLen = ((EFI_IP6_OPTION *)(Option + Offset))->Length;
> + Offset     = IP6_NEXT_OPTION_OFFSET (Offset, OptDataLen);

> case Ip6OptionSkip:
> - Offset = (UINT8)(Offset + *(Option + Offset + 1));
>   OptDataLen = ((EFI_IP6_OPTION *)(Option + Offset))->Length;
>   Offset     = IP6_NEXT_OPTION_OFFSET (Offset, OptDataLen);

Additionally, this change also corrects incorrect math where the calling
function was calculating the HDR EXT optionLen as a uint8 instead of a
uint16

> - OptionLen = (UINT8)((*Option + 1) * 8 - 2);
> + OptionLen = IP6_HDR_EXT_LEN (*Option) -
IP6_COMBINED_SIZE_OF_NEXT_HDR_AND_LEN;

Additionally this check adds additional logic to santize the incoming
data

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>

Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
@Flickdm
NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45232 Unit Tests
26373cd 
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4537
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4538

Unit tests to confirm that..

Infinite loop when parsing unknown options in the Destination Options
header

and

Infinite loop when parsing a PadN option in the Destination Options
header

... have been patched

This patch tests the following functions:
Ip6IsOptionValid

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>

Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
@Flickdm
NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45234 Patch
a11b486 
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4539

Bug Details:
PixieFail Bug #6
CVE-2023-45234
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 processing DNS Servers option in a DHCPv6
Advertise message

Change Overview:

Introduces a function to cache the Dns Server and perform sanitizing
on the incoming DnsServerLen to ensure that the length is valid

> + EFI_STATUS
> + PxeBcCacheDnsServerAddresses (
> +  IN PXEBC_PRIVATE_DATA        *Private,
> +  IN PXEBC_DHCP6_PACKET_CACHE  *Cache6
> +  )

Additional code cleanup

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>

Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
@Flickdm
NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45234 Unit Tests
8ad5f21 
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 <[email protected]>
Cc: Zachary Clark-williams <[email protected]>

Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
@Flickdm
MdePkg: Test: Add gRT_GetTime Google Test Mock
351d5ea 
This adds support for GetTime Google Test Mock

Cc: Michael D Kinney <[email protected]>
Cc: Liming Gao <[email protected]>
Cc: Zhiguang Liu <[email protected]>

Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Michael D Kinney <[email protected]>
@Flickdm
NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45235 Patch
8e12db1 
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4540

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

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>

Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
@Flickdm
NetworkPkg: UefiPxeBcDxe: SECURITY PATCH CVE-2023-45235 Unit Tests
93822e9 
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 <[email protected]>
Cc: Zachary Clark-williams <[email protected]>

Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
@Flickdm
NetworkPkg: : Adds a SecurityFix.yaml file
74eea86 
This creates / adds a security file that tracks the security fixes
found in this package and can be used to find the fixes that were
applied.

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>

Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
@Flickdm
NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch
f51893e 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4673
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4534

This was not part of the Quarkslab bugs however the same pattern
as CVE-2023-45229 exists in Dhcp6UpdateIaInfo.

This patch replaces the code in question with the safe function
created to patch CVE-2023-45229

>
>   if (EFI_ERROR (
>        Dhcp6SeekInnerOptionSafe (
>          Instance->Config->IaDescriptor.Type,
>          Option,
>          OptionLen,
>          &IaInnerOpt,
>          &IaInnerLen
>          )
>        ))
>  {
>    return EFI_DEVICE_ERROR;
>  }
>

Additionally corrects incorrect usage of macro to read the status

> - StsCode = NTOHS (ReadUnaligned16 ((UINT16 *)DHCP6_OFFSET_OF_OPT_LEN
 (Option)));
> + StsCode = NTOHS (ReadUnaligned16 ((UINT16 *)
DHCP6_OFFSET_OF_STATUS_CODE (Option));

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>
Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
Reviewed-by: Leif Lindholm <[email protected]>
@Flickdm
NetworkPkg: Dhcp6Dxe: Removes duplicate check and replaces with macro
e76ff64 
Removes duplicate check after merge

>
>  //
>  // Verify the PacketCursor is within the packet
>  //
>  if (  (*PacketCursor < Packet->Dhcp6.Option)
>     || (*PacketCursor >= Packet->Dhcp6.Option + (Packet->Size -
sizeof (EFI_DHCP6_HEADER))))
>  {
>    return EFI_INVALID_PARAMETER;
>  }
>

Converts the check to a macro and replaces all instances of the check
with the macro

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>
Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
Reviewed-by: Leif Lindholm <[email protected]>
@Flickdm
NetworkPkg: Dhcp6Dxe: Packet-Length is not updated before appending
d1c54ff 
In order for Dhcp6AppendIaAddrOption (..) to safely append the IA
Address option, the Packet-Length field must be updated before appending
the option.

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>
Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
Reviewed-by: Leif Lindholm <[email protected]>
@Flickdm
NetworkPkg: : Updating SecurityFixes.yaml
b1e73fa 
This captures the related security change for Dhcp6Dxe that is related
to CVE-2023-45229

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>
Signed-off-by: Doug Flick [MSFT] <[email protected]>
Reviewed-by: Saloni Kasbekar <[email protected]>
Reviewed-by: Leif Lindholm <[email protected]>
@Flickdm Flickdm force-pushed the backport/security_fixes/networkpkg/pxefail branch from 98a0e4b to b1e73fa Compare June 17, 2024 05:19
@Flickdm Flickdm closed this Jul 18, 2024
@Flickdm Flickdm reopened this Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
impact:breaking-change Requires integration attention impact:security Has a security impact
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@Flickdm