[edk2-devel] [PATCH 0/3] [edk2-stable202402] Corrects additional concern in NetworkPkg

[edk2-devel] [PATCH 0/3] [edk2-stable202402] Corrects additional concern in NetworkPkg

[Doug Flick via groups.io]

After talking with Micheal Kinney, I was advised to resend
these with edk2-stable202402, and CC Stewards.

These patches are time sensitive and need reviews.

This patch series corrects an additional security concern
found in Dhc6Dxe related to CVE-2023-45229. 

Additionally this fixes some issues on the mailing list
that were not pulled in before merging into Edk2.

Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>

Cc: Andrew Fish <afish@apple.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>

Doug Flick (3):
  [edk2-stable202402] NetworkPkg: Dhcp6Dxe: SECURITY PATCH
    CVE-2023-45229 Related Patch
  [edk2-stable202402] NetworkPkg: Dhcp6Dxe: Additional Code Cleanup
  [edk2-stable202402] NetworkPkg: : Updating SecurityFixes.yaml

 NetworkPkg/Dhcp6Dxe/Dhcp6Io.c      | 232 +++++++++++++++++------------
 NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c |  20 +--
 NetworkPkg/SecurityFixes.yaml      |   1 +
 3 files changed, 141 insertions(+), 112 deletions(-)

-- 
2.43.0


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

[edk2-devel] [PATCH 1/3] [edk2-stable202402] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch

[Doug Flick via groups.io]

From: Doug Flick <dougflick@microsoft.com>

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 <saloni.kasbekar@intel.com>
Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>

Cc: Andrew Fish <afish@apple.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>

Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>
---
 NetworkPkg/Dhcp6Dxe/Dhcp6Io.c | 232 ++++++++++++++++++++--------------
 1 file changed, 134 insertions(+), 98 deletions(-)

diff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c b/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
index 3b8feb4a2032..6000e885afaf 100644
--- a/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
+++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
@@ -510,6 +510,97 @@ Dhcp6CallbackUser (
   return Status;
 }
 
+/**
+  Seeks the Inner Options from a DHCP6 Option
+
+  @param[in]  IaType          The type of the IA option.
+  @param[in]  Option          The pointer to the DHCP6 Option.
+  @param[in]  OptionLen       The length of the DHCP6 Option.
+  @param[out] IaInnerOpt      The pointer to the IA inner option.
+  @param[out] IaInnerLen      The length of the IA inner option.
+
+  @retval EFI_SUCCESS         Seek the inner option successfully.
+  @retval EFI_DEVICE_ERROR    The OptionLen is invalid. On Error,
+                              the pointers are not modified
+**/
+EFI_STATUS
+Dhcp6SeekInnerOptionSafe (
+  IN  UINT16  IaType,
+  IN  UINT8   *Option,
+  IN  UINT32  OptionLen,
+  OUT UINT8   **IaInnerOpt,
+  OUT UINT16  *IaInnerLen
+  )
+{
+  UINT16  IaInnerLenTmp;
+  UINT8   *IaInnerOptTmp;
+
+  if (Option == NULL) {
+    ASSERT (Option != NULL);
+    return EFI_DEVICE_ERROR;
+  }
+
+  if (IaInnerOpt == NULL) {
+    ASSERT (IaInnerOpt != NULL);
+    return EFI_DEVICE_ERROR;
+  }
+
+  if (IaInnerLen == NULL) {
+    ASSERT (IaInnerLen != NULL);
+    return EFI_DEVICE_ERROR;
+  }
+
+  if (IaType == Dhcp6OptIana) {
+    //
+    // Verify we have a fully formed IA_NA
+    //
+    if (OptionLen < DHCP6_MIN_SIZE_OF_IA_NA) {
+      return EFI_DEVICE_ERROR;
+    }
+
+    //
+    // Get the IA Inner Option and Length
+    //
+    IaInnerOptTmp = DHCP6_OFFSET_OF_IA_NA_INNER_OPT (Option);
+
+    //
+    // Verify the IaInnerLen is valid.
+    //
+    IaInnerLenTmp = (UINT16)NTOHS (ReadUnaligned16 ((UINT16 *)DHCP6_OFFSET_OF_OPT_LEN (Option)));
+    if (IaInnerLenTmp < DHCP6_SIZE_OF_COMBINED_IAID_T1_T2) {
+      return EFI_DEVICE_ERROR;
+    }
+
+    IaInnerLenTmp -= DHCP6_SIZE_OF_COMBINED_IAID_T1_T2;
+  } else if (IaType == Dhcp6OptIata) {
+    //
+    // Verify the OptionLen is valid.
+    //
+    if (OptionLen < DHCP6_MIN_SIZE_OF_IA_TA) {
+      return EFI_DEVICE_ERROR;
+    }
+
+    IaInnerOptTmp = DHCP6_OFFSET_OF_IA_TA_INNER_OPT (Option);
+
+    //
+    // Verify the IaInnerLen is valid.
+    //
+    IaInnerLenTmp = (UINT16)NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option))));
+    if (IaInnerLenTmp < DHCP6_SIZE_OF_IAID) {
+      return EFI_DEVICE_ERROR;
+    }
+
+    IaInnerLenTmp -= DHCP6_SIZE_OF_IAID;
+  } else {
+    return EFI_DEVICE_ERROR;
+  }
+
+  *IaInnerOpt = IaInnerOptTmp;
+  *IaInnerLen = IaInnerLenTmp;
+
+  return EFI_SUCCESS;
+}
+
 /**
   Update Ia according to the new reply message.
 
@@ -528,13 +619,23 @@ Dhcp6UpdateIaInfo (
 {
   EFI_STATUS  Status;
   UINT8       *Option;
+  UINT32      OptionLen;
   UINT8       *IaInnerOpt;
   UINT16      IaInnerLen;
   UINT16      StsCode;
   UINT32      T1;
   UINT32      T2;
 
+  T1 = 0;
+  T2 = 0;
+
   ASSERT (Instance->Config != NULL);
+
+  // OptionLen is the length of the Options excluding the DHCP header.
+  // Length of the EFI_DHCP6_PACKET from the first byte of the Header field to the last
+  // byte of the Option[] field.
+  OptionLen = Packet->Length - sizeof (Packet->Dhcp6.Header);
+
   //
   // If the reply was received in response to a solicit with rapid commit option,
   // request, renew or rebind message, the client updates the information it has
@@ -549,13 +650,29 @@ Dhcp6UpdateIaInfo (
   //
   Option = Dhcp6SeekIaOption (
              Packet->Dhcp6.Option,
-             Packet->Length - sizeof (EFI_DHCP6_HEADER),
+             OptionLen,
              &Instance->Config->IaDescriptor
              );
   if (Option == NULL) {
     return EFI_DEVICE_ERROR;
   }
 
+  //
+  // Calculate the distance from Packet->Dhcp6.Option to the IA option.
+  //
+  // Packet->Size and Packet->Length are both UINT32 type, and Packet->Size is
+  // the size of the whole packet, including the DHCP header, and Packet->Length
+  // is the length of the DHCP message body, excluding the DHCP header.
+  //
+  // (*Option - Packet->Dhcp6.Option) is the number of bytes from the start of
+  // DHCP6 option area to the start of the IA option.
+  //
+  // Dhcp6SeekInnerOptionSafe() is searching starting from the start of the
+  // IA option to the end of the DHCP6 option area, thus subtract the space
+  // up until this option
+  //
+  OptionLen = OptionLen - (UINT32)(Option - Packet->Dhcp6.Option);
+
   //
   // The format of the IA_NA option is:
   //
@@ -591,32 +708,32 @@ Dhcp6UpdateIaInfo (
   //
 
   //
-  // sizeof (option-code + option-len + IaId)           = 8
-  // sizeof (option-code + option-len + IaId + T1)      = 12
-  // sizeof (option-code + option-len + IaId + T1 + T2) = 16
-  //
-  // The inner options still start with 2 bytes option-code and 2 bytes option-len.
+  // Seek the inner option
   //
+  if (EFI_ERROR (
+        Dhcp6SeekInnerOptionSafe (
+          Instance->Config->IaDescriptor.Type,
+          Option,
+          OptionLen,
+          &IaInnerOpt,
+          &IaInnerLen
+          )
+        ))
+  {
+    return EFI_DEVICE_ERROR;
+  }
+
   if (Instance->Config->IaDescriptor.Type == Dhcp6OptIana) {
     T1 = NTOHL (ReadUnaligned32 ((UINT32 *)(DHCP6_OFFSET_OF_IA_NA_T1 (Option))));
     T2 = NTOHL (ReadUnaligned32 ((UINT32 *)(DHCP6_OFFSET_OF_IA_NA_T2 (Option))));
     //
     // Refer to RFC3155 Chapter 22.4. If a client receives an IA_NA with T1 greater than T2,
     // and both T1 and T2 are greater than 0, the client discards the IA_NA option and processes
-    // the remainder of the message as though the server had not  included the invalid IA_NA option.
+    // the remainder of the message as though the server had not included the invalid IA_NA option.
     //
     if ((T1 > T2) && (T2 > 0)) {
       return EFI_DEVICE_ERROR;
     }
-
-    IaInnerOpt = DHCP6_OFFSET_OF_IA_NA_INNER_OPT (Option);
-    IaInnerLen = (UINT16)(NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option)))) - DHCP6_SIZE_OF_COMBINED_IAID_T1_T2);
-  } else {
-    T1 = 0;
-    T2 = 0;
-
-    IaInnerOpt = DHCP6_OFFSET_OF_IA_TA_INNER_OPT (Option);
-    IaInnerLen = (UINT16)(NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option)))) - DHCP6_SIZE_OF_IAID);
   }
 
   //
@@ -642,7 +759,7 @@ Dhcp6UpdateIaInfo (
   Option  = Dhcp6SeekOption (IaInnerOpt, IaInnerLen, Dhcp6OptStatusCode);
 
   if (Option != NULL) {
-    StsCode = NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option))));
+    StsCode = NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_STATUS_CODE (Option))));
     if (StsCode != Dhcp6StsSuccess) {
       return EFI_DEVICE_ERROR;
     }
@@ -662,87 +779,6 @@ Dhcp6UpdateIaInfo (
   return Status;
 }
 
-/**
-  Seeks the Inner Options from a DHCP6 Option
-
-  @param[in]  IaType          The type of the IA option.
-  @param[in]  Option          The pointer to the DHCP6 Option.
-  @param[in]  OptionLen       The length of the DHCP6 Option.
-  @param[out] IaInnerOpt      The pointer to the IA inner option.
-  @param[out] IaInnerLen      The length of the IA inner option.
-
-  @retval EFI_SUCCESS         Seek the inner option successfully.
-  @retval EFI_DEVICE_ERROR    The OptionLen is invalid. On Error,
-                              the pointers are not modified
-**/
-EFI_STATUS
-Dhcp6SeekInnerOptionSafe (
-  IN  UINT16  IaType,
-  IN  UINT8   *Option,
-  IN  UINT32  OptionLen,
-  OUT UINT8   **IaInnerOpt,
-  OUT UINT16  *IaInnerLen
-  )
-{
-  UINT16  IaInnerLenTmp;
-  UINT8   *IaInnerOptTmp;
-
-  if (Option == NULL) {
-    ASSERT (Option != NULL);
-    return EFI_DEVICE_ERROR;
-  }
-
-  if (IaInnerOpt == NULL) {
-    ASSERT (IaInnerOpt != NULL);
-    return EFI_DEVICE_ERROR;
-  }
-
-  if (IaInnerLen == NULL) {
-    ASSERT (IaInnerLen != NULL);
-    return EFI_DEVICE_ERROR;
-  }
-
-  if (IaType == Dhcp6OptIana) {
-    // Verify we have a fully formed IA_NA
-    if (OptionLen < DHCP6_MIN_SIZE_OF_IA_NA) {
-      return EFI_DEVICE_ERROR;
-    }
-
-    //
-    IaInnerOptTmp = DHCP6_OFFSET_OF_IA_NA_INNER_OPT (Option);
-
-    // Verify the IaInnerLen is valid.
-    IaInnerLenTmp = (UINT16)NTOHS (ReadUnaligned16 ((UINT16 *)DHCP6_OFFSET_OF_OPT_LEN (Option)));
-    if (IaInnerLenTmp < DHCP6_SIZE_OF_COMBINED_IAID_T1_T2) {
-      return EFI_DEVICE_ERROR;
-    }
-
-    IaInnerLenTmp -= DHCP6_SIZE_OF_COMBINED_IAID_T1_T2;
-  } else if (IaType == Dhcp6OptIata) {
-    // Verify the OptionLen is valid.
-    if (OptionLen < DHCP6_MIN_SIZE_OF_IA_TA) {
-      return EFI_DEVICE_ERROR;
-    }
-
-    IaInnerOptTmp = DHCP6_OFFSET_OF_IA_TA_INNER_OPT (Option);
-
-    // Verify the IaInnerLen is valid.
-    IaInnerLenTmp = (UINT16)NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option))));
-    if (IaInnerLenTmp < DHCP6_SIZE_OF_IAID) {
-      return EFI_DEVICE_ERROR;
-    }
-
-    IaInnerLenTmp -= DHCP6_SIZE_OF_IAID;
-  } else {
-    return EFI_DEVICE_ERROR;
-  }
-
-  *IaInnerOpt = IaInnerOptTmp;
-  *IaInnerLen = IaInnerLenTmp;
-
-  return EFI_SUCCESS;
-}
-
 /**
   Seek StatusCode Option in package. A Status Code option may appear in the
   options field of a DHCP message and/or in the options field of another option.
-- 
2.43.0



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

[edk2-devel] [PATCH 2/3] [edk2-stable202402] NetworkPkg: Dhcp6Dxe: Additional Code Cleanup

[Doug Flick via groups.io]

From: Doug Flick <dougflick@microsoft.com>

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;
>  }
>

Additionally Calculates the Packet->Length before appending

Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>

Cc: Andrew Fish <afish@apple.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>

Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>
---
 NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
index 705c665c519d..348602c4e1a8 100644
--- a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
+++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
@@ -657,15 +657,6 @@ Dhcp6AppendOption (
     return EFI_BUFFER_TOO_SMALL;
   }
 
-  //
-  // 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;
-  }
-
   WriteUnaligned16 ((UINT16 *)*PacketCursor, OptType);
   *PacketCursor += DHCP6_SIZE_OF_OPT_CODE;
   WriteUnaligned16 ((UINT16 *)*PacketCursor, OptLen);
@@ -929,6 +920,11 @@ Dhcp6AppendIaOption (
     *PacketCursor += sizeof (T2);
   }
 
+  //
+  // Update the packet length
+  //
+  Packet->Length += BytesNeeded;
+
   //
   // Fill all the addresses belong to the Ia
   //
@@ -945,11 +941,6 @@ Dhcp6AppendIaOption (
   //
   *Len = HTONS ((UINT16)(*PacketCursor - (UINT8 *)Len - 2));
 
-  //
-  // Update the packet length
-  //
-  Packet->Length += BytesNeeded;
-
   return EFI_SUCCESS;
 }
 
@@ -957,6 +948,7 @@ Dhcp6AppendIaOption (
   Append the appointed Elapsed time option to Buf, and move Buf to the end.
 
   @param[in, out] Packet        A pointer to the packet, on success Packet->Length
+                                will be updated.
   @param[in, out] PacketCursor  The pointer in the packet, on success PacketCursor
                                 will be moved to the end of the option.
   @param[in]      Instance      The pointer to the Dhcp6 instance.
-- 
2.43.0



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

[edk2-devel] [PATCH 3/3] [edk2-stable202402] NetworkPkg: : Updating SecurityFixes.yaml

[Doug Flick via groups.io]

From: Doug Flick <dougflick@microsoft.com>

This captures the related security change for Dhcp6Dxe that is related
to CVE-2023-45229

Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>

Cc: Andrew Fish <afish@apple.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>

Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>
---
 NetworkPkg/SecurityFixes.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/NetworkPkg/SecurityFixes.yaml b/NetworkPkg/SecurityFixes.yaml
index 7e900483fec5..fa42025e0d82 100644
--- a/NetworkPkg/SecurityFixes.yaml
+++ b/NetworkPkg/SecurityFixes.yaml
@@ -8,6 +8,7 @@ CVE_2023_45229:
   commit_titles:
     - "NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Patch"
     - "NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Unit Tests"
+    - "NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch"
   cve: CVE-2023-45229
   date_reported: 2023-08-28 13:56 UTC
   description: "Bug 01 - edk2/NetworkPkg: Out-of-bounds read when processing IA_NA/IA_TA options in a DHCPv6 Advertise message"
-- 
2.43.0



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

Re: [edk2-devel] [PATCH 1/3] [edk2-stable202402] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch

[Saloni Kasbekar]

Reviewed-by: Saloni Kasbekar <saloni.kasbekar@intel.com>

-----Original Message-----
From: Douglas Flick [MSFT] <doug.edk2@gmail.com> 
Sent: Friday, February 9, 2024 7:05 PM
To: devel@edk2.groups.io
Cc: Doug Flick <dougflick@microsoft.com>; Kasbekar, Saloni <saloni.kasbekar@intel.com>; Clark-williams, Zachary <zachary.clark-williams@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Doug Flick [MSFT] <doug.edk2@gmail.com>
Subject: [PATCH 1/3] [edk2-stable202402] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch

From: Doug Flick <dougflick@microsoft.com>

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 <saloni.kasbekar@intel.com>
Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>

Cc: Andrew Fish <afish@apple.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>

Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>
---
 NetworkPkg/Dhcp6Dxe/Dhcp6Io.c | 232 ++++++++++++++++++++--------------
 1 file changed, 134 insertions(+), 98 deletions(-)

diff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c b/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c index 3b8feb4a2032..6000e885afaf 100644
--- a/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
+++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
@@ -510,6 +510,97 @@ Dhcp6CallbackUser (
   return Status; } +/**+  Seeks the Inner Options from a DHCP6 Option++  @param[in]  IaType          The type of the IA option.+  @param[in]  Option          The pointer to the DHCP6 Option.+  @param[in]  OptionLen       The length of the DHCP6 Option.+  @param[out] IaInnerOpt      The pointer to the IA inner option.+  @param[out] IaInnerLen      The length of the IA inner option.++  @retval EFI_SUCCESS         Seek the inner option successfully.+  @retval EFI_DEVICE_ERROR    The OptionLen is invalid. On Error,+                              the pointers are not modified+**/+EFI_STATUS+Dhcp6SeekInnerOptionSafe (+  IN  UINT16  IaType,+  IN  UINT8   *Option,+  IN  UINT32  OptionLen,+  OUT UINT8   **IaInnerOpt,+  OUT UINT16  *IaInnerLen+  )+{+  UINT16  IaInnerLenTmp;+  UINT8   *IaInnerOptTmp;++  if (Option == NULL) {+    ASSERT (Option != NULL);+    return EFI_DEVICE_ERROR;+  }++  if (IaInnerOpt == NULL) {+    ASSERT (IaInnerOpt != NULL);+    return EFI_DEVICE_ERROR;+  }++  if (IaInnerLen == NULL) {+    ASSERT (IaInnerLen != NULL);+    return EFI_DEVICE_ERROR;+  }++  if (IaType == Dhcp6OptIana) {+    //+    // Verify we have a fully formed IA_NA+    //+    if (OptionLen < DHCP6_MIN_SIZE_OF_IA_NA) {+      return EFI_DEVICE_ERROR;+    }++    //+    // Get the IA Inner Option and Length+    //+    IaInnerOptTmp = DHCP6_OFFSET_OF_IA_NA_INNER_OPT (Option);++    //+    // Verify the IaInnerLen is valid.+    //+    IaInnerLenTmp = (UINT16)NTOHS (ReadUnaligned16 ((UINT16 *)DHCP6_OFFSET_OF_OPT_LEN (Option)));+    if (IaInnerLenTmp < DHCP6_SIZE_OF_COMBINED_IAID_T1_T2) {+      return EFI_DEVICE_ERROR;+    }++    IaInnerLenTmp -= DHCP6_SIZE_OF_COMBINED_IAID_T1_T2;+  } else if (IaType == Dhcp6OptIata) {+    //+    // Verify the OptionLen is valid.+    //+    if (OptionLen < DHCP6_MIN_SIZE_OF_IA_TA) {+      return EFI_DEVICE_ERROR;+    }++    IaInnerOptTmp = DHCP6_OFFSET_OF_IA_TA_INNER_OPT (Option);++    //+    // Verify the IaInnerLen is valid.+    //+    IaInnerLenTmp = (UINT16)NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option))));+    if (IaInnerLenTmp < DHCP6_SIZE_OF_IAID) {+      return EFI_DEVICE_ERROR;+    }++    IaInnerLenTmp -= DHCP6_SIZE_OF_IAID;+  } else {+    return EFI_DEVICE_ERROR;+  }++  *IaInnerOpt = IaInnerOptTmp;+  *IaInnerLen = IaInnerLenTmp;++  return EFI_SUCCESS;+}+ /**   Update Ia according to the new reply message. @@ -528,13 +619,23 @@ Dhcp6UpdateIaInfo (
 {   EFI_STATUS  Status;   UINT8       *Option;+  UINT32      OptionLen;   UINT8       *IaInnerOpt;   UINT16      IaInnerLen;   UINT16      StsCode;   UINT32      T1;   UINT32      T2; +  T1 = 0;+  T2 = 0;+   ASSERT (Instance->Config != NULL);++  // OptionLen is the length of the Options excluding the DHCP header.+  // Length of the EFI_DHCP6_PACKET from the first byte of the Header field to the last+  // byte of the Option[] field.+  OptionLen = Packet->Length - sizeof (Packet->Dhcp6.Header);+   //   // If the reply was received in response to a solicit with rapid commit option,   // request, renew or rebind message, the client updates the information it has@@ -549,13 +650,29 @@ Dhcp6UpdateIaInfo (
   //   Option = Dhcp6SeekIaOption (              Packet->Dhcp6.Option,-             Packet->Length - sizeof (EFI_DHCP6_HEADER),+             OptionLen,              &Instance->Config->IaDescriptor              );   if (Option == NULL) {     return EFI_DEVICE_ERROR;   } +  //+  // Calculate the distance from Packet->Dhcp6.Option to the IA option.+  //+  // Packet->Size and Packet->Length are both UINT32 type, and Packet->Size is+  // the size of the whole packet, including the DHCP header, and Packet->Length+  // is the length of the DHCP message body, excluding the DHCP header.+  //+  // (*Option - Packet->Dhcp6.Option) is the number of bytes from the start of+  // DHCP6 option area to the start of the IA option.+  //+  // Dhcp6SeekInnerOptionSafe() is searching starting from the start of the+  // IA option to the end of the DHCP6 option area, thus subtract the space+  // up until this option+  //+  OptionLen = OptionLen - (UINT32)(Option - Packet->Dhcp6.Option);+   //   // The format of the IA_NA option is:   //@@ -591,32 +708,32 @@ Dhcp6UpdateIaInfo (
   //    //-  // sizeof (option-code + option-len + IaId)           = 8-  // sizeof (option-code + option-len + IaId + T1)      = 12-  // sizeof (option-code + option-len + IaId + T1 + T2) = 16-  //-  // The inner options still start with 2 bytes option-code and 2 bytes option-len.+  // Seek the inner option   //+  if (EFI_ERROR (+        Dhcp6SeekInnerOptionSafe (+          Instance->Config->IaDescriptor.Type,+          Option,+          OptionLen,+          &IaInnerOpt,+          &IaInnerLen+          )+        ))+  {+    return EFI_DEVICE_ERROR;+  }+   if (Instance->Config->IaDescriptor.Type == Dhcp6OptIana) {     T1 = NTOHL (ReadUnaligned32 ((UINT32 *)(DHCP6_OFFSET_OF_IA_NA_T1 (Option))));     T2 = NTOHL (ReadUnaligned32 ((UINT32 *)(DHCP6_OFFSET_OF_IA_NA_T2 (Option))));     //     // Refer to RFC3155 Chapter 22.4. If a client receives an IA_NA with T1 greater than T2,     // and both T1 and T2 are greater than 0, the client discards the IA_NA option and processes-    // the remainder of the message as though the server had not  included the invalid IA_NA option.+    // the remainder of the message as though the server had not included the invalid IA_NA option.     //     if ((T1 > T2) && (T2 > 0)) {       return EFI_DEVICE_ERROR;     }--    IaInnerOpt = DHCP6_OFFSET_OF_IA_NA_INNER_OPT (Option);-    IaInnerLen = (UINT16)(NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option)))) - DHCP6_SIZE_OF_COMBINED_IAID_T1_T2);-  } else {-    T1 = 0;-    T2 = 0;--    IaInnerOpt = DHCP6_OFFSET_OF_IA_TA_INNER_OPT (Option);-    IaInnerLen = (UINT16)(NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option)))) - DHCP6_SIZE_OF_IAID);   }    //@@ -642,7 +759,7 @@ Dhcp6UpdateIaInfo (
   Option  = Dhcp6SeekOption (IaInnerOpt, IaInnerLen, Dhcp6OptStatusCode);    if (Option != NULL) {-    StsCode = NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option))));+    StsCode = NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_STATUS_CODE (Option))));     if (StsCode != Dhcp6StsSuccess) {       return EFI_DEVICE_ERROR;     }@@ -662,87 +779,6 @@ Dhcp6UpdateIaInfo (
   return Status; } -/**-  Seeks the Inner Options from a DHCP6 Option--  @param[in]  IaType          The type of the IA option.-  @param[in]  Option          The pointer to the DHCP6 Option.-  @param[in]  OptionLen       The length of the DHCP6 Option.-  @param[out] IaInnerOpt      The pointer to the IA inner option.-  @param[out] IaInnerLen      The length of the IA inner option.--  @retval EFI_SUCCESS         Seek the inner option successfully.-  @retval EFI_DEVICE_ERROR    The OptionLen is invalid. On Error,-                              the pointers are not modified-**/-EFI_STATUS-Dhcp6SeekInnerOptionSafe (-  IN  UINT16  IaType,-  IN  UINT8   *Option,-  IN  UINT32  OptionLen,-  OUT UINT8   **IaInnerOpt,-  OUT UINT16  *IaInnerLen-  )-{-  UINT16  IaInnerLenTmp;-  UINT8   *IaInnerOptTmp;--  if (Option == NULL) {-    ASSERT (Option != NULL);-    return EFI_DEVICE_ERROR;-  }--  if (IaInnerOpt == NULL) {-    ASSERT (IaInnerOpt != NULL);-    return EFI_DEVICE_ERROR;-  }--  if (IaInnerLen == NULL) {-    ASSERT (IaInnerLen != NULL);-    return EFI_DEVICE_ERROR;-  }--  if (IaType == Dhcp6OptIana) {-    // Verify we have a fully formed IA_NA-    if (OptionLen < DHCP6_MIN_SIZE_OF_IA_NA) {-      return EFI_DEVICE_ERROR;-    }--    //-    IaInnerOptTmp = DHCP6_OFFSET_OF_IA_NA_INNER_OPT (Option);--    // Verify the IaInnerLen is valid.-    IaInnerLenTmp = (UINT16)NTOHS (ReadUnaligned16 ((UINT16 *)DHCP6_OFFSET_OF_OPT_LEN (Option)));-    if (IaInnerLenTmp < DHCP6_SIZE_OF_COMBINED_IAID_T1_T2) {-      return EFI_DEVICE_ERROR;-    }--    IaInnerLenTmp -= DHCP6_SIZE_OF_COMBINED_IAID_T1_T2;-  } else if (IaType == Dhcp6OptIata) {-    // Verify the OptionLen is valid.-    if (OptionLen < DHCP6_MIN_SIZE_OF_IA_TA) {-      return EFI_DEVICE_ERROR;-    }--    IaInnerOptTmp = DHCP6_OFFSET_OF_IA_TA_INNER_OPT (Option);--    // Verify the IaInnerLen is valid.-    IaInnerLenTmp = (UINT16)NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option))));-    if (IaInnerLenTmp < DHCP6_SIZE_OF_IAID) {-      return EFI_DEVICE_ERROR;-    }--    IaInnerLenTmp -= DHCP6_SIZE_OF_IAID;-  } else {-    return EFI_DEVICE_ERROR;-  }--  *IaInnerOpt = IaInnerOptTmp;-  *IaInnerLen = IaInnerLenTmp;--  return EFI_SUCCESS;-}- /**   Seek StatusCode Option in package. A Status Code option may appear in the   options field of a DHCP message and/or in the options field of another option.-- 
2.43.0



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

Re: [edk2-devel] [PATCH 2/3] [edk2-stable202402] NetworkPkg: Dhcp6Dxe: Additional Code Cleanup

[Saloni Kasbekar]

Reviewed-by: Saloni Kasbekar <saloni.kasbekar@intel.com>

-----Original Message-----
From: Douglas Flick [MSFT] <doug.edk2@gmail.com> 
Sent: Friday, February 9, 2024 7:05 PM
To: devel@edk2.groups.io
Cc: Doug Flick <dougflick@microsoft.com>; Kasbekar, Saloni <saloni.kasbekar@intel.com>; Clark-williams, Zachary <zachary.clark-williams@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Doug Flick [MSFT] <doug.edk2@gmail.com>
Subject: [PATCH 2/3] [edk2-stable202402] NetworkPkg: Dhcp6Dxe: Additional Code Cleanup

From: Doug Flick <dougflick@microsoft.com>

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;
>  }
>

Additionally Calculates the Packet->Length before appending

Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>

Cc: Andrew Fish <afish@apple.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>

Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>
---
 NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
index 705c665c519d..348602c4e1a8 100644
--- a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
+++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
@@ -657,15 +657,6 @@ Dhcp6AppendOption (
     return EFI_BUFFER_TOO_SMALL;   } -  //-  // 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;-  }-   WriteUnaligned16 ((UINT16 *)*PacketCursor, OptType);   *PacketCursor += DHCP6_SIZE_OF_OPT_CODE;   WriteUnaligned16 ((UINT16 *)*PacketCursor, OptLen);@@ -929,6 +920,11 @@ Dhcp6AppendIaOption (
     *PacketCursor += sizeof (T2);   } +  //+  // Update the packet length+  //+  Packet->Length += BytesNeeded;+   //   // Fill all the addresses belong to the Ia   //@@ -945,11 +941,6 @@ Dhcp6AppendIaOption (
   //   *Len = HTONS ((UINT16)(*PacketCursor - (UINT8 *)Len - 2)); -  //-  // Update the packet length-  //-  Packet->Length += BytesNeeded;-   return EFI_SUCCESS; } @@ -957,6 +948,7 @@ Dhcp6AppendIaOption (
   Append the appointed Elapsed time option to Buf, and move Buf to the end.    @param[in, out] Packet        A pointer to the packet, on success Packet->Length+                                will be updated.   @param[in, out] PacketCursor  The pointer in the packet, on success PacketCursor                                 will be moved to the end of the option.   @param[in]      Instance      The pointer to the Dhcp6 instance.-- 
2.43.0



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

Re: [edk2-devel] [PATCH 3/3] [edk2-stable202402] NetworkPkg: : Updating SecurityFixes.yaml

[Saloni Kasbekar]

Reviewed-by: Saloni Kasbekar <saloni.kasbekar@intel.com>

-----Original Message-----
From: Douglas Flick [MSFT] <doug.edk2@gmail.com> 
Sent: Friday, February 9, 2024 7:05 PM
To: devel@edk2.groups.io
Cc: Doug Flick <dougflick@microsoft.com>; Kasbekar, Saloni <saloni.kasbekar@intel.com>; Clark-williams, Zachary <zachary.clark-williams@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Doug Flick [MSFT] <doug.edk2@gmail.com>
Subject: [PATCH 3/3] [edk2-stable202402] NetworkPkg: : Updating SecurityFixes.yaml

From: Doug Flick <dougflick@microsoft.com>

This captures the related security change for Dhcp6Dxe that is related to CVE-2023-45229

Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>

Cc: Andrew Fish <afish@apple.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>

Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>
---
 NetworkPkg/SecurityFixes.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/NetworkPkg/SecurityFixes.yaml b/NetworkPkg/SecurityFixes.yaml index 7e900483fec5..fa42025e0d82 100644
--- a/NetworkPkg/SecurityFixes.yaml
+++ b/NetworkPkg/SecurityFixes.yaml
@@ -8,6 +8,7 @@ CVE_2023_45229:
   commit_titles:     - "NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Patch"     - "NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Unit Tests"+    - "NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch"   cve: CVE-2023-45229   date_reported: 2023-08-28 13:56 UTC   description: "Bug 01 - edk2/NetworkPkg: Out-of-bounds read when processing IA_NA/IA_TA options in a DHCPv6 Advertise message"-- 
2.43.0



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

Re: [edk2-devel] [PATCH 0/3] [edk2-stable202402] Corrects additional concern in NetworkPkg

[Doug Flick via groups.io]

Additional details requested for why this change should go in this
release after the code freeze.

1. The additional security concern refers to  4673 – edk2/NetworkPkg:
Out-of-bounds read when processing IA_NA/IA_TA options in a DHCPv6
Advertise message (tianocore.org). Which was seen as:

"[...] this is very closely related to CVE-2023-45229, which is
already public, see no need to embargo. Making this BZ public.

CVE-2023-45229 was very recently mitigated under BZ 4518.  We Will fix
this as an enhancement to the patches for BZ 4518."

While no PoC exists for this bug of an abundance of caution, it's
critical that this "enhancement" make it into the release

2. Corrects an incorrect offset

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

3. Adds the additional commits to a security yaml file so downstream
consumer may find the commits and ensure they have the patches - or
have the commits so they can cherry pick them appropriately.



On Fri, Feb 9, 2024 at 6:04 PM Douglas Flick [MSFT] <doug.edk2@gmail.com> wrote:
>
> After talking with Micheal Kinney, I was advised to resend
> these with edk2-stable202402, and CC Stewards.
>
> These patches are time sensitive and need reviews.
>
> This patch series corrects an additional security concern
> found in Dhc6Dxe related to CVE-2023-45229.
>
> Additionally this fixes some issues on the mailing list
> that were not pulled in before merging into Edk2.
>
> Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
> Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>
>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>
> Doug Flick (3):
>   [edk2-stable202402] NetworkPkg: Dhcp6Dxe: SECURITY PATCH
>     CVE-2023-45229 Related Patch
>   [edk2-stable202402] NetworkPkg: Dhcp6Dxe: Additional Code Cleanup
>   [edk2-stable202402] NetworkPkg: : Updating SecurityFixes.yaml
>
>  NetworkPkg/Dhcp6Dxe/Dhcp6Io.c      | 232 +++++++++++++++++------------
>  NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c |  20 +--
>  NetworkPkg/SecurityFixes.yaml      |   1 +
>  3 files changed, 141 insertions(+), 112 deletions(-)
>
> --
> 2.43.0


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

Re: [edk2-devel] [PATCH 0/3] [edk2-stable202402] Corrects additional concern in NetworkPkg

[Saloni Kasbekar]

Hi Mike,

Could you please consider taking this for the edk2-stable202402 patches? While they weren't part of the bugs reported by Quarkslab, they follow a similar pattern and expose similar vulnerabilities to the bugs reported. My recommendation would be to pull in these changes for the stable release so that we have all the known vulnerabilities fixed as a part of the release.

Thanks,
Saloni

-----Original Message-----
From: Douglas Flick [MSFT] <doug.edk2@gmail.com> 
Sent: Friday, February 9, 2024 7:05 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>; Andrew Fish <afish@apple.com>; Leif Lindholm <quic_llindhol@quicinc.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: [PATCH 0/3] [edk2-stable202402] Corrects additional concern in NetworkPkg

After talking with Micheal Kinney, I was advised to resend these with edk2-stable202402, and CC Stewards.

These patches are time sensitive and need reviews.

This patch series corrects an additional security concern found in Dhc6Dxe related to CVE-2023-45229. 

Additionally this fixes some issues on the mailing list that were not pulled in before merging into Edk2.

Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>

Cc: Andrew Fish <afish@apple.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>

Doug Flick (3):
  [edk2-stable202402] NetworkPkg: Dhcp6Dxe: SECURITY PATCH
    CVE-2023-45229 Related Patch
  [edk2-stable202402] NetworkPkg: Dhcp6Dxe: Additional Code Cleanup
  [edk2-stable202402] NetworkPkg: : Updating SecurityFixes.yaml

 NetworkPkg/Dhcp6Dxe/Dhcp6Io.c      | 232 +++++++++++++++++------------
 NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c |  20 +--
 NetworkPkg/SecurityFixes.yaml      |   1 +
 3 files changed, 141 insertions(+), 112 deletions(-)

--
2.43.0


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

Re: [edk2-devel] [PATCH 1/3] [edk2-stable202402] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch

[Leif Lindholm]

On Fri, Feb 09, 2024 at 19:04:56 -0800, Douglas Flick [MSFT] wrote:
> From: Doug Flick <dougflick@microsoft.com>
> 
> 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;
> >  }
> >

Could you possibly introduce a declaration for
Dhcp6SeekInnerOptionSafe () instead of moving it? (Or break the move
out as a separate commit.)
That would significantly reduce the likelihood of mistakes during review.

/
    Leif

> 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 <saloni.kasbekar@intel.com>
> Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>
> 
> Cc: Andrew Fish <afish@apple.com>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> 
> Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>
> ---
>  NetworkPkg/Dhcp6Dxe/Dhcp6Io.c | 232 ++++++++++++++++++++--------------
>  1 file changed, 134 insertions(+), 98 deletions(-)
> 
> diff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c b/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
> index 3b8feb4a2032..6000e885afaf 100644
> --- a/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
> +++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
> @@ -510,6 +510,97 @@ Dhcp6CallbackUser (
>    return Status;
>  }
>  
> +/**
> +  Seeks the Inner Options from a DHCP6 Option
> +
> +  @param[in]  IaType          The type of the IA option.
> +  @param[in]  Option          The pointer to the DHCP6 Option.
> +  @param[in]  OptionLen       The length of the DHCP6 Option.
> +  @param[out] IaInnerOpt      The pointer to the IA inner option.
> +  @param[out] IaInnerLen      The length of the IA inner option.
> +
> +  @retval EFI_SUCCESS         Seek the inner option successfully.
> +  @retval EFI_DEVICE_ERROR    The OptionLen is invalid. On Error,
> +                              the pointers are not modified
> +**/
> +EFI_STATUS
> +Dhcp6SeekInnerOptionSafe (
> +  IN  UINT16  IaType,
> +  IN  UINT8   *Option,
> +  IN  UINT32  OptionLen,
> +  OUT UINT8   **IaInnerOpt,
> +  OUT UINT16  *IaInnerLen
> +  )
> +{
> +  UINT16  IaInnerLenTmp;
> +  UINT8   *IaInnerOptTmp;
> +
> +  if (Option == NULL) {
> +    ASSERT (Option != NULL);
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  if (IaInnerOpt == NULL) {
> +    ASSERT (IaInnerOpt != NULL);
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  if (IaInnerLen == NULL) {
> +    ASSERT (IaInnerLen != NULL);
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  if (IaType == Dhcp6OptIana) {
> +    //
> +    // Verify we have a fully formed IA_NA
> +    //
> +    if (OptionLen < DHCP6_MIN_SIZE_OF_IA_NA) {
> +      return EFI_DEVICE_ERROR;
> +    }
> +
> +    //
> +    // Get the IA Inner Option and Length
> +    //
> +    IaInnerOptTmp = DHCP6_OFFSET_OF_IA_NA_INNER_OPT (Option);
> +
> +    //
> +    // Verify the IaInnerLen is valid.
> +    //
> +    IaInnerLenTmp = (UINT16)NTOHS (ReadUnaligned16 ((UINT16 *)DHCP6_OFFSET_OF_OPT_LEN (Option)));
> +    if (IaInnerLenTmp < DHCP6_SIZE_OF_COMBINED_IAID_T1_T2) {
> +      return EFI_DEVICE_ERROR;
> +    }
> +
> +    IaInnerLenTmp -= DHCP6_SIZE_OF_COMBINED_IAID_T1_T2;
> +  } else if (IaType == Dhcp6OptIata) {
> +    //
> +    // Verify the OptionLen is valid.
> +    //
> +    if (OptionLen < DHCP6_MIN_SIZE_OF_IA_TA) {
> +      return EFI_DEVICE_ERROR;
> +    }
> +
> +    IaInnerOptTmp = DHCP6_OFFSET_OF_IA_TA_INNER_OPT (Option);
> +
> +    //
> +    // Verify the IaInnerLen is valid.
> +    //
> +    IaInnerLenTmp = (UINT16)NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option))));
> +    if (IaInnerLenTmp < DHCP6_SIZE_OF_IAID) {
> +      return EFI_DEVICE_ERROR;
> +    }
> +
> +    IaInnerLenTmp -= DHCP6_SIZE_OF_IAID;
> +  } else {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  *IaInnerOpt = IaInnerOptTmp;
> +  *IaInnerLen = IaInnerLenTmp;
> +
> +  return EFI_SUCCESS;
> +}
> +
>  /**
>    Update Ia according to the new reply message.
>  
> @@ -528,13 +619,23 @@ Dhcp6UpdateIaInfo (
>  {
>    EFI_STATUS  Status;
>    UINT8       *Option;
> +  UINT32      OptionLen;
>    UINT8       *IaInnerOpt;
>    UINT16      IaInnerLen;
>    UINT16      StsCode;
>    UINT32      T1;
>    UINT32      T2;
>  
> +  T1 = 0;
> +  T2 = 0;
> +
>    ASSERT (Instance->Config != NULL);
> +
> +  // OptionLen is the length of the Options excluding the DHCP header.
> +  // Length of the EFI_DHCP6_PACKET from the first byte of the Header field to the last
> +  // byte of the Option[] field.
> +  OptionLen = Packet->Length - sizeof (Packet->Dhcp6.Header);
> +
>    //
>    // If the reply was received in response to a solicit with rapid commit option,
>    // request, renew or rebind message, the client updates the information it has
> @@ -549,13 +650,29 @@ Dhcp6UpdateIaInfo (
>    //
>    Option = Dhcp6SeekIaOption (
>               Packet->Dhcp6.Option,
> -             Packet->Length - sizeof (EFI_DHCP6_HEADER),
> +             OptionLen,
>               &Instance->Config->IaDescriptor
>               );
>    if (Option == NULL) {
>      return EFI_DEVICE_ERROR;
>    }
>  
> +  //
> +  // Calculate the distance from Packet->Dhcp6.Option to the IA option.
> +  //
> +  // Packet->Size and Packet->Length are both UINT32 type, and Packet->Size is
> +  // the size of the whole packet, including the DHCP header, and Packet->Length
> +  // is the length of the DHCP message body, excluding the DHCP header.
> +  //
> +  // (*Option - Packet->Dhcp6.Option) is the number of bytes from the start of
> +  // DHCP6 option area to the start of the IA option.
> +  //
> +  // Dhcp6SeekInnerOptionSafe() is searching starting from the start of the
> +  // IA option to the end of the DHCP6 option area, thus subtract the space
> +  // up until this option
> +  //
> +  OptionLen = OptionLen - (UINT32)(Option - Packet->Dhcp6.Option);
> +
>    //
>    // The format of the IA_NA option is:
>    //
> @@ -591,32 +708,32 @@ Dhcp6UpdateIaInfo (
>    //
>  
>    //
> -  // sizeof (option-code + option-len + IaId)           = 8
> -  // sizeof (option-code + option-len + IaId + T1)      = 12
> -  // sizeof (option-code + option-len + IaId + T1 + T2) = 16
> -  //
> -  // The inner options still start with 2 bytes option-code and 2 bytes option-len.
> +  // Seek the inner option
>    //
> +  if (EFI_ERROR (
> +        Dhcp6SeekInnerOptionSafe (
> +          Instance->Config->IaDescriptor.Type,
> +          Option,
> +          OptionLen,
> +          &IaInnerOpt,
> +          &IaInnerLen
> +          )
> +        ))
> +  {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
>    if (Instance->Config->IaDescriptor.Type == Dhcp6OptIana) {
>      T1 = NTOHL (ReadUnaligned32 ((UINT32 *)(DHCP6_OFFSET_OF_IA_NA_T1 (Option))));
>      T2 = NTOHL (ReadUnaligned32 ((UINT32 *)(DHCP6_OFFSET_OF_IA_NA_T2 (Option))));
>      //
>      // Refer to RFC3155 Chapter 22.4. If a client receives an IA_NA with T1 greater than T2,
>      // and both T1 and T2 are greater than 0, the client discards the IA_NA option and processes
> -    // the remainder of the message as though the server had not  included the invalid IA_NA option.
> +    // the remainder of the message as though the server had not included the invalid IA_NA option.
>      //
>      if ((T1 > T2) && (T2 > 0)) {
>        return EFI_DEVICE_ERROR;
>      }
> -
> -    IaInnerOpt = DHCP6_OFFSET_OF_IA_NA_INNER_OPT (Option);
> -    IaInnerLen = (UINT16)(NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option)))) - DHCP6_SIZE_OF_COMBINED_IAID_T1_T2);
> -  } else {
> -    T1 = 0;
> -    T2 = 0;
> -
> -    IaInnerOpt = DHCP6_OFFSET_OF_IA_TA_INNER_OPT (Option);
> -    IaInnerLen = (UINT16)(NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option)))) - DHCP6_SIZE_OF_IAID);
>    }
>  
>    //
> @@ -642,7 +759,7 @@ Dhcp6UpdateIaInfo (
>    Option  = Dhcp6SeekOption (IaInnerOpt, IaInnerLen, Dhcp6OptStatusCode);
>  
>    if (Option != NULL) {
> -    StsCode = NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option))));
> +    StsCode = NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_STATUS_CODE (Option))));
>      if (StsCode != Dhcp6StsSuccess) {
>        return EFI_DEVICE_ERROR;
>      }
> @@ -662,87 +779,6 @@ Dhcp6UpdateIaInfo (
>    return Status;
>  }
>  
> -/**
> -  Seeks the Inner Options from a DHCP6 Option
> -
> -  @param[in]  IaType          The type of the IA option.
> -  @param[in]  Option          The pointer to the DHCP6 Option.
> -  @param[in]  OptionLen       The length of the DHCP6 Option.
> -  @param[out] IaInnerOpt      The pointer to the IA inner option.
> -  @param[out] IaInnerLen      The length of the IA inner option.
> -
> -  @retval EFI_SUCCESS         Seek the inner option successfully.
> -  @retval EFI_DEVICE_ERROR    The OptionLen is invalid. On Error,
> -                              the pointers are not modified
> -**/
> -EFI_STATUS
> -Dhcp6SeekInnerOptionSafe (
> -  IN  UINT16  IaType,
> -  IN  UINT8   *Option,
> -  IN  UINT32  OptionLen,
> -  OUT UINT8   **IaInnerOpt,
> -  OUT UINT16  *IaInnerLen
> -  )
> -{
> -  UINT16  IaInnerLenTmp;
> -  UINT8   *IaInnerOptTmp;
> -
> -  if (Option == NULL) {
> -    ASSERT (Option != NULL);
> -    return EFI_DEVICE_ERROR;
> -  }
> -
> -  if (IaInnerOpt == NULL) {
> -    ASSERT (IaInnerOpt != NULL);
> -    return EFI_DEVICE_ERROR;
> -  }
> -
> -  if (IaInnerLen == NULL) {
> -    ASSERT (IaInnerLen != NULL);
> -    return EFI_DEVICE_ERROR;
> -  }
> -
> -  if (IaType == Dhcp6OptIana) {
> -    // Verify we have a fully formed IA_NA
> -    if (OptionLen < DHCP6_MIN_SIZE_OF_IA_NA) {
> -      return EFI_DEVICE_ERROR;
> -    }
> -
> -    //
> -    IaInnerOptTmp = DHCP6_OFFSET_OF_IA_NA_INNER_OPT (Option);
> -
> -    // Verify the IaInnerLen is valid.
> -    IaInnerLenTmp = (UINT16)NTOHS (ReadUnaligned16 ((UINT16 *)DHCP6_OFFSET_OF_OPT_LEN (Option)));
> -    if (IaInnerLenTmp < DHCP6_SIZE_OF_COMBINED_IAID_T1_T2) {
> -      return EFI_DEVICE_ERROR;
> -    }
> -
> -    IaInnerLenTmp -= DHCP6_SIZE_OF_COMBINED_IAID_T1_T2;
> -  } else if (IaType == Dhcp6OptIata) {
> -    // Verify the OptionLen is valid.
> -    if (OptionLen < DHCP6_MIN_SIZE_OF_IA_TA) {
> -      return EFI_DEVICE_ERROR;
> -    }
> -
> -    IaInnerOptTmp = DHCP6_OFFSET_OF_IA_TA_INNER_OPT (Option);
> -
> -    // Verify the IaInnerLen is valid.
> -    IaInnerLenTmp = (UINT16)NTOHS (ReadUnaligned16 ((UINT16 *)(DHCP6_OFFSET_OF_OPT_LEN (Option))));
> -    if (IaInnerLenTmp < DHCP6_SIZE_OF_IAID) {
> -      return EFI_DEVICE_ERROR;
> -    }
> -
> -    IaInnerLenTmp -= DHCP6_SIZE_OF_IAID;
> -  } else {
> -    return EFI_DEVICE_ERROR;
> -  }
> -
> -  *IaInnerOpt = IaInnerOptTmp;
> -  *IaInnerLen = IaInnerLenTmp;
> -
> -  return EFI_SUCCESS;
> -}
> -
>  /**
>    Seek StatusCode Option in package. A Status Code option may appear in the
>    options field of a DHCP message and/or in the options field of another option.
> -- 
> 2.43.0
> 


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

Re: [edk2-devel] [PATCH 2/3] [edk2-stable202402] NetworkPkg: Dhcp6Dxe: Additional Code Cleanup

[Leif Lindholm]

On Fri, Feb 09, 2024 at 19:04:57 -0800, Douglas Flick [MSFT] wrote:
> From: Doug Flick <dougflick@microsoft.com>
> 
> 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;
> >  }
> >
> 
> Additionally Calculates the Packet->Length before appending

If this was one of the packages I maintain, I would ask you to break
any "Additionally" out as a separate commit.

> Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
> Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>
> 
> Cc: Andrew Fish <afish@apple.com>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> 
> Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>
> ---
>  NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
> index 705c665c519d..348602c4e1a8 100644
> --- a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
> +++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
> @@ -657,15 +657,6 @@ Dhcp6AppendOption (
>      return EFI_BUFFER_TOO_SMALL;
>    }
>  
> -  //
> -  // 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;
> -  }
> -

I see this exact pattern repeated identically 4 times in this
file. After this deletion.
If we're cleaning this up, someone should look into making a macro or
static helper function.

>    WriteUnaligned16 ((UINT16 *)*PacketCursor, OptType);
>    *PacketCursor += DHCP6_SIZE_OF_OPT_CODE;
>    WriteUnaligned16 ((UINT16 *)*PacketCursor, OptLen);
> @@ -929,6 +920,11 @@ Dhcp6AppendIaOption (
>      *PacketCursor += sizeof (T2);
>    }
>  
> +  //
> +  // Update the packet length
> +  //
> +  Packet->Length += BytesNeeded;
> +

That's not just cleanup though, this is a bugfix in its own right?
As in, even if the code is currently laid out in a way that doesn't
trigger an issue, Dhcp6AppendIaAddrOption () depends on this option
being up to date when called.
Or am I overreacting?

/
    Leif

>    //
>    // Fill all the addresses belong to the Ia
>    //
> @@ -945,11 +941,6 @@ Dhcp6AppendIaOption (
>    //
>    *Len = HTONS ((UINT16)(*PacketCursor - (UINT8 *)Len - 2));
>  
> -  //
> -  // Update the packet length
> -  //
> -  Packet->Length += BytesNeeded;
> -
>    return EFI_SUCCESS;
>  }
>  
> @@ -957,6 +948,7 @@ Dhcp6AppendIaOption (
>    Append the appointed Elapsed time option to Buf, and move Buf to the end.
>  
>    @param[in, out] Packet        A pointer to the packet, on success Packet->Length
> +                                will be updated.
>    @param[in, out] PacketCursor  The pointer in the packet, on success PacketCursor
>                                  will be moved to the end of the option.
>    @param[in]      Instance      The pointer to the Dhcp6 instance.
> -- 
> 2.43.0
> 


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

Re: [edk2-devel] [PATCH 2/3] [edk2-stable202402] NetworkPkg: Dhcp6Dxe: Additional Code Cleanup

[Doug Flick via groups.io]

[-- Attachment #1: Type: text/plain, Size: 5297 bytes --]

> If this was one of the packages I maintain, I would ask
> you to break any "Additionally" out as a separate commit.

Thats fair enough. I can go ahead and do that.

> I see this exact pattern repeated identically 4 times in
> this file. After this deletion. If we're cleaning this up,
> someone should look into making a macro or static
> helper function.

That's a good suggestion

> That's not just cleanup though, this is a bugfix in its own > right? As in, even if the code is currently laid out in a way > that doesn't trigger an issue,
> Dhcp6AppendIaAddrOption () depends on this option
> being up to date when called. Or am I overreacting?

Ultimately, I think you're right. This issue was originally pointed on the original mail thread but it was merged in before it could be corrected. Should I create a bugzilla for this and then attach that to the change?

Douglas Flick
________________________________
From: Leif Lindholm <quic_llindhol@quicinc.com>
Sent: Monday, February 12, 2024 11:16:44 AM
To: Douglas Flick [MSFT] <doug.edk2@gmail.com>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>; Doug Flick <dougflick@microsoft.com>; Saloni Kasbekar <saloni.kasbekar@intel.com>; Zachary Clark-williams <zachary.clark-williams@intel.com>; Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: [EXTERNAL] Re: [PATCH 2/3] [edk2-stable202402] NetworkPkg: Dhcp6Dxe: Additional Code Cleanup

[You don't often get email from quic_llindhol@quicinc.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

On Fri, Feb 09, 2024 at 19:04:57 -0800, Douglas Flick [MSFT] wrote:
> From: Doug Flick <dougflick@microsoft.com>
>
> 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;
> >  }
> >
>
> Additionally Calculates the Packet->Length before appending

If this was one of the packages I maintain, I would ask you to break
any "Additionally" out as a separate commit.

> Cc: Saloni Kasbekar <saloni.kasbekar@intel.com>
> Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>
>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>
> Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>
> ---
>  NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
> index 705c665c519d..348602c4e1a8 100644
> --- a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
> +++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
> @@ -657,15 +657,6 @@ Dhcp6AppendOption (
>      return EFI_BUFFER_TOO_SMALL;
>    }
>
> -  //
> -  // 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;
> -  }
> -

I see this exact pattern repeated identically 4 times in this
file. After this deletion.
If we're cleaning this up, someone should look into making a macro or
static helper function.

>    WriteUnaligned16 ((UINT16 *)*PacketCursor, OptType);
>    *PacketCursor += DHCP6_SIZE_OF_OPT_CODE;
>    WriteUnaligned16 ((UINT16 *)*PacketCursor, OptLen);
> @@ -929,6 +920,11 @@ Dhcp6AppendIaOption (
>      *PacketCursor += sizeof (T2);
>    }
>
> +  //
> +  // Update the packet length
> +  //
> +  Packet->Length += BytesNeeded;
> +

That's not just cleanup though, this is a bugfix in its own right?
As in, even if the code is currently laid out in a way that doesn't
trigger an issue, Dhcp6AppendIaAddrOption () depends on this option
being up to date when called.
Or am I overreacting?

/
    Leif

>    //
>    // Fill all the addresses belong to the Ia
>    //
> @@ -945,11 +941,6 @@ Dhcp6AppendIaOption (
>    //
>    *Len = HTONS ((UINT16)(*PacketCursor - (UINT8 *)Len - 2));
>
> -  //
> -  // Update the packet length
> -  //
> -  Packet->Length += BytesNeeded;
> -
>    return EFI_SUCCESS;
>  }
>
> @@ -957,6 +948,7 @@ Dhcp6AppendIaOption (
>    Append the appointed Elapsed time option to Buf, and move Buf to the end.
>
>    @param[in, out] Packet        A pointer to the packet, on success Packet->Length
> +                                will be updated.
>    @param[in, out] PacketCursor  The pointer in the packet, on success PacketCursor
>                                  will be moved to the end of the option.
>    @param[in]      Instance      The pointer to the Dhcp6 instance.
> --
> 2.43.0
>


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



[-- Attachment #2: Type: text/html, Size: 12137 bytes --]