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

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

[Doug Flick via groups.io]

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

Apologizes, 

I'm going to resend this following a different guide so that it's not base64 encoded


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115279): https://edk2.groups.io/g/devel/message/115279
Mute This Topic: https://groups.io/mt/104249911/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: 893 bytes --]

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

[Doug Flick via groups.io]

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>

Doug Flick (3):
  NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch
  NetworkPkg: Dhcp6Dxe: Additional Code Cleanup
  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

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

[Doug Flick via groups.io]

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>

Doug Flick (3):
  NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch
  NetworkPkg: Dhcp6Dxe: Additional Code Cleanup
  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 (#115280): https://edk2.groups.io/g/devel/message/115280
Mute This Topic: https://groups.io/mt/104249911/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/3] 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>

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 (#115281): https://edk2.groups.io/g/devel/message/115281
Mute This Topic: https://groups.io/mt/104250123/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 2/3] 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>

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 (#115282): https://edk2.groups.io/g/devel/message/115282
Mute This Topic: https://groups.io/mt/104250124/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 3/3] 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>

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 (#115283): https://edk2.groups.io/g/devel/message/115283
Mute This Topic: https://groups.io/mt/104250125/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-