public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [edk2-devel] [PATCH v2 0/4] Corrects additional concern in NetworkPkg
@ 2024-02-13 18:45 Doug Flick via groups.io
  2024-02-13 18:46 ` [edk2-devel] [PATCH v2 1/4] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch Doug Flick via groups.io
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Doug Flick via groups.io @ 2024-02-13 18:45 UTC (permalink / raw)
  To: devel
  Cc: Doug Flick, Saloni Kasbekar, Zachary Clark-williams, Andrew Fish,
	Leif Lindholm, Michael D Kinney

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>

Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>

Doug Flick (4):
  NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch
  NetworkPkg: Dhcp6Dxe: Removes duplicate check and replaces with macro
  NetworkPkg: Dhcp6Dxe: Packet-Length is not updated before appending
  NetworkPkg: : Updating SecurityFixes.yaml

 NetworkPkg/Dhcp6Dxe/Dhcp6Io.h      | 22 ++++++
 NetworkPkg/Dhcp6Dxe/Dhcp6Io.c      | 70 +++++++++++++++-----
 NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c | 46 ++++++-------
 NetworkPkg/SecurityFixes.yaml      |  1 +
 4 files changed, 96 insertions(+), 43 deletions(-)

-- 
2.34.1


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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* [edk2-devel] [PATCH v2 1/4] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch
  2024-02-13 18:45 [edk2-devel] [PATCH v2 0/4] Corrects additional concern in NetworkPkg Doug Flick via groups.io
@ 2024-02-13 18:46 ` Doug Flick via groups.io
  2024-02-13 21:51   ` Saloni Kasbekar
  2024-02-13 18:46 ` [edk2-devel] [PATCH v2 2/4] NetworkPkg: Dhcp6Dxe: Removes duplicate check and replaces with macro Doug Flick via groups.io
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Doug Flick via groups.io @ 2024-02-13 18:46 UTC (permalink / raw)
  To: devel
  Cc: Doug Flick, Saloni Kasbekar, Zachary Clark-williams,
	Doug Flick [MSFT]

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.h | 22 ++++++
 NetworkPkg/Dhcp6Dxe/Dhcp6Io.c | 70 +++++++++++++++-----
 2 files changed, 75 insertions(+), 17 deletions(-)

diff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Io.h b/NetworkPkg/Dhcp6Dxe/Dhcp6Io.h
index 051a652f2b1f..ab0e1ac27f10 100644
--- a/NetworkPkg/Dhcp6Dxe/Dhcp6Io.h
+++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Io.h
@@ -217,4 +217,26 @@ Dhcp6OnTimerTick (
   IN VOID       *Context
   );
 
+/**
+  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
+  );
+
 #endif
diff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c b/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
index 3b8feb4a2032..a9bffae353d7 100644
--- a/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
+++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
@@ -528,13 +528,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 +559,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 +617,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 +668,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;
     }
@@ -703,15 +729,21 @@ Dhcp6SeekInnerOptionSafe (
   }
 
   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;
@@ -719,14 +751,18 @@ Dhcp6SeekInnerOptionSafe (
 
     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;
-- 
2.34.1



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



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [edk2-devel] [PATCH v2 2/4] NetworkPkg: Dhcp6Dxe: Removes duplicate check and replaces with macro
  2024-02-13 18:45 [edk2-devel] [PATCH v2 0/4] Corrects additional concern in NetworkPkg Doug Flick via groups.io
  2024-02-13 18:46 ` [edk2-devel] [PATCH v2 1/4] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch Doug Flick via groups.io
@ 2024-02-13 18:46 ` Doug Flick via groups.io
  2024-02-13 18:46 ` [edk2-devel] [PATCH v2 3/4] NetworkPkg: Dhcp6Dxe: Packet-Length is not updated before appending Doug Flick via groups.io
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Doug Flick via groups.io @ 2024-02-13 18:46 UTC (permalink / raw)
  To: devel
  Cc: Doug Flick, Saloni Kasbekar, Zachary Clark-williams,
	Doug Flick [MSFT]

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

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

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 | 46 +++++++++-----------
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
index 705c665c519d..e4e072562296 100644
--- a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
+++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
@@ -10,6 +10,16 @@
 
 #include "Dhcp6Impl.h"
 
+//
+// Verifies the packet cursor is within the packet
+// otherwise it is invalid
+//
+#define IS_INVALID_PACKET_CURSOR(PacketCursor, Packet) \
+  (((*PacketCursor) < (Packet)->Dhcp6.Option) || \
+   ((*PacketCursor) >= (Packet)->Dhcp6.Option + ((Packet)->Size - sizeof(EFI_DHCP6_HEADER))) \
+  )                                                                            \
+
+
 /**
   Generate client Duid in the format of Duid-llt.
 
@@ -638,9 +648,7 @@ Dhcp6AppendOption (
   //
   // Verify the PacketCursor is within the packet
   //
-  if (  (*PacketCursor < Packet->Dhcp6.Option)
-     || (*PacketCursor >= Packet->Dhcp6.Option + (Packet->Size - sizeof (EFI_DHCP6_HEADER))))
-  {
+  if (IS_INVALID_PACKET_CURSOR (PacketCursor, Packet)) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -657,15 +665,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);
@@ -744,9 +743,7 @@ Dhcp6AppendIaAddrOption (
   //
   // Verify the PacketCursor is within the packet
   //
-  if (  (*PacketCursor < Packet->Dhcp6.Option)
-     || (*PacketCursor >= Packet->Dhcp6.Option + (Packet->Size - sizeof (EFI_DHCP6_HEADER))))
-  {
+  if (IS_INVALID_PACKET_CURSOR (PacketCursor, Packet)) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -877,9 +874,7 @@ Dhcp6AppendIaOption (
   //
   // Verify the PacketCursor is within the packet
   //
-  if (  (*PacketCursor < Packet->Dhcp6.Option)
-     || (*PacketCursor >= Packet->Dhcp6.Option + (Packet->Size - sizeof (EFI_DHCP6_HEADER))))
-  {
+  if (IS_INVALID_PACKET_CURSOR (PacketCursor, Packet)) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -940,16 +935,16 @@ Dhcp6AppendIaOption (
     }
   }
 
+  //
+  // Update the packet length
+  //
+  Packet->Length += BytesNeeded;
+
   //
   // Fill the value of Ia option length
   //
   *Len = HTONS ((UINT16)(*PacketCursor - (UINT8 *)Len - 2));
 
-  //
-  // Update the packet length
-  //
-  Packet->Length += BytesNeeded;
-
   return EFI_SUCCESS;
 }
 
@@ -957,6 +952,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.
@@ -1012,9 +1008,7 @@ Dhcp6AppendETOption (
   //
   // Verify the PacketCursor is within the packet
   //
-  if (  (*PacketCursor < Packet->Dhcp6.Option)
-     || (*PacketCursor >= Packet->Dhcp6.Option + (Packet->Size - sizeof (EFI_DHCP6_HEADER))))
-  {
+  if (IS_INVALID_PACKET_CURSOR (PacketCursor, Packet)) {
     return EFI_INVALID_PARAMETER;
   }
 
-- 
2.34.1



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



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [edk2-devel] [PATCH v2 3/4] NetworkPkg: Dhcp6Dxe: Packet-Length is not updated before appending
  2024-02-13 18:45 [edk2-devel] [PATCH v2 0/4] Corrects additional concern in NetworkPkg Doug Flick via groups.io
  2024-02-13 18:46 ` [edk2-devel] [PATCH v2 1/4] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch Doug Flick via groups.io
  2024-02-13 18:46 ` [edk2-devel] [PATCH v2 2/4] NetworkPkg: Dhcp6Dxe: Removes duplicate check and replaces with macro Doug Flick via groups.io
@ 2024-02-13 18:46 ` Doug Flick via groups.io
  2024-02-13 20:16   ` Leif Lindholm
  2024-02-13 18:46 ` [edk2-devel] [PATCH v2 4/4] NetworkPkg: : Updating SecurityFixes.yaml Doug Flick via groups.io
  2024-02-13 20:18 ` [edk2-devel] [PATCH v2 0/4] Corrects additional concern in NetworkPkg Leif Lindholm
  4 siblings, 1 reply; 13+ messages in thread
From: Doug Flick via groups.io @ 2024-02-13 18:46 UTC (permalink / raw)
  To: devel
  Cc: Doug Flick, Saloni Kasbekar, Zachary Clark-williams,
	Doug Flick [MSFT]

From: Doug Flick <dougflick@microsoft.com>

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 <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 | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
index e4e072562296..f38e3ee3fe1a 100644
--- a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
+++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
@@ -924,6 +924,11 @@ Dhcp6AppendIaOption (
     *PacketCursor += sizeof (T2);
   }
 
+  //
+  // Update the packet length
+  //
+  Packet->Length += BytesNeeded;
+
   //
   // Fill all the addresses belong to the Ia
   //
@@ -935,11 +940,6 @@ Dhcp6AppendIaOption (
     }
   }
 
-  //
-  // Update the packet length
-  //
-  Packet->Length += BytesNeeded;
-
   //
   // Fill the value of Ia option length
   //
-- 
2.34.1



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



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [edk2-devel] [PATCH v2 4/4] NetworkPkg: : Updating SecurityFixes.yaml
  2024-02-13 18:45 [edk2-devel] [PATCH v2 0/4] Corrects additional concern in NetworkPkg Doug Flick via groups.io
                   ` (2 preceding siblings ...)
  2024-02-13 18:46 ` [edk2-devel] [PATCH v2 3/4] NetworkPkg: Dhcp6Dxe: Packet-Length is not updated before appending Doug Flick via groups.io
@ 2024-02-13 18:46 ` Doug Flick via groups.io
  2024-02-15 13:54   ` Rebecca Cran
  2024-02-13 20:18 ` [edk2-devel] [PATCH v2 0/4] Corrects additional concern in NetworkPkg Leif Lindholm
  4 siblings, 1 reply; 13+ messages in thread
From: Doug Flick via groups.io @ 2024-02-13 18:46 UTC (permalink / raw)
  To: devel
  Cc: Doug Flick, Saloni Kasbekar, Zachary Clark-williams,
	Doug Flick [MSFT]

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.34.1



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



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v2 3/4] NetworkPkg: Dhcp6Dxe: Packet-Length is not updated before appending
  2024-02-13 18:46 ` [edk2-devel] [PATCH v2 3/4] NetworkPkg: Dhcp6Dxe: Packet-Length is not updated before appending Doug Flick via groups.io
@ 2024-02-13 20:16   ` Leif Lindholm
  0 siblings, 0 replies; 13+ messages in thread
From: Leif Lindholm @ 2024-02-13 20:16 UTC (permalink / raw)
  To: devel, dougflick
  Cc: Saloni Kasbekar, Zachary Clark-williams, Doug Flick [MSFT],
	Kinney, Michael D

On 2024-02-13 18:46, Doug Flick via groups.io wrote:
> From: Doug Flick <dougflick@microsoft.com>
> 
> 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 <saloni.kasbekar@intel.com>
> Cc: Zachary Clark-williams <zachary.clark-williams@intel.com>
> 
> Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>

Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

Thanks for this!

> ---
>   NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
> index e4e072562296..f38e3ee3fe1a 100644
> --- a/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
> +++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c
> @@ -924,6 +924,11 @@ Dhcp6AppendIaOption (
>       *PacketCursor += sizeof (T2);
> 
>     }
> 
>   
> 
> +  //
> 
> +  // Update the packet length
> 
> +  //
> 
> +  Packet->Length += BytesNeeded;
> 
> +
> 
>     //
> 
>     // Fill all the addresses belong to the Ia
> 
>     //
> 
> @@ -935,11 +940,6 @@ Dhcp6AppendIaOption (
>       }
> 
>     }
> 
>   
> 
> -  //
> 
> -  // Update the packet length
> 
> -  //
> 
> -  Packet->Length += BytesNeeded;
> 
> -
> 
>     //
> 
>     // Fill the value of Ia option length
> 
>     //
> 



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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v2 0/4] Corrects additional concern in NetworkPkg
  2024-02-13 18:45 [edk2-devel] [PATCH v2 0/4] Corrects additional concern in NetworkPkg Doug Flick via groups.io
                   ` (3 preceding siblings ...)
  2024-02-13 18:46 ` [edk2-devel] [PATCH v2 4/4] NetworkPkg: : Updating SecurityFixes.yaml Doug Flick via groups.io
@ 2024-02-13 20:18 ` Leif Lindholm
  4 siblings, 0 replies; 13+ messages in thread
From: Leif Lindholm @ 2024-02-13 20:18 UTC (permalink / raw)
  To: Doug Flick, devel
  Cc: Saloni Kasbekar, Zachary Clark-williams, Andrew Fish,
	Michael D Kinney

I'm happy for this bugfix to go into the stable tag.

/
     Leif

On 2024-02-13 18:45, Doug Flick 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>
> 
> Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>
> 
> Doug Flick (4):
>    NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch
>    NetworkPkg: Dhcp6Dxe: Removes duplicate check and replaces with macro
>    NetworkPkg: Dhcp6Dxe: Packet-Length is not updated before appending
>    NetworkPkg: : Updating SecurityFixes.yaml
> 
>   NetworkPkg/Dhcp6Dxe/Dhcp6Io.h      | 22 ++++++
>   NetworkPkg/Dhcp6Dxe/Dhcp6Io.c      | 70 +++++++++++++++-----
>   NetworkPkg/Dhcp6Dxe/Dhcp6Utility.c | 46 ++++++-------
>   NetworkPkg/SecurityFixes.yaml      |  1 +
>   4 files changed, 96 insertions(+), 43 deletions(-)
> 



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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v2 1/4] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch
  2024-02-13 18:46 ` [edk2-devel] [PATCH v2 1/4] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch Doug Flick via groups.io
@ 2024-02-13 21:51   ` Saloni Kasbekar
  2024-02-13 23:31     ` Doug Flick via groups.io
  0 siblings, 1 reply; 13+ messages in thread
From: Saloni Kasbekar @ 2024-02-13 21:51 UTC (permalink / raw)
  To: Doug Flick, devel@edk2.groups.io; +Cc: Doug Flick, Clark-williams, Zachary

Doug,

Thanks! This makes it much easier to read. Could you share the list of tests that were done to verify these changes, and if the actual bug was reproducible with a failing test case that now passes?

Thanks,
Saloni

-----Original Message-----
From: Doug Flick <doug.edk2@gmail.com> 
Sent: Tuesday, February 13, 2024 10:46 AM
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>; Doug Flick [MSFT] <doug.edk2@gmail.com>
Subject: [PATCH v2 1/4] 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>

Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>
---
 NetworkPkg/Dhcp6Dxe/Dhcp6Io.h | 22 ++++++  NetworkPkg/Dhcp6Dxe/Dhcp6Io.c | 70 +++++++++++++++-----
 2 files changed, 75 insertions(+), 17 deletions(-)

diff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Io.h b/NetworkPkg/Dhcp6Dxe/Dhcp6Io.h index 051a652f2b1f..ab0e1ac27f10 100644
--- a/NetworkPkg/Dhcp6Dxe/Dhcp6Io.h
+++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Io.h
@@ -217,4 +217,26 @@ Dhcp6OnTimerTick (
   IN VOID       *Context   ); +/**+  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+  );+ #endifdiff --git a/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c b/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
index 3b8feb4a2032..a9bffae353d7 100644
--- a/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
+++ b/NetworkPkg/Dhcp6Dxe/Dhcp6Io.c
@@ -528,13 +528,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 +559,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 +617,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 +668,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;     }@@ -703,15 +729,21 @@ Dhcp6SeekInnerOptionSafe (
   }    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;@@ -719,14 +751,18 @@ Dhcp6SeekInnerOptionSafe (
      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;-- 
2.34.1



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



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v2 1/4] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch
  2024-02-13 21:51   ` Saloni Kasbekar
@ 2024-02-13 23:31     ` Doug Flick via groups.io
  2024-02-14  2:40       ` Michael D Kinney
  0 siblings, 1 reply; 13+ messages in thread
From: Doug Flick via groups.io @ 2024-02-13 23:31 UTC (permalink / raw)
  To: Saloni Kasbekar, devel

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

Saloni,

Yeah there was never any tests that showed this bug exists mostly it was brought up through static analysis since it's related to a known CVE. I have written some unit tests (that I'm not particularly satisfied with) that show that I'm hitting the desired code paths that can trigger the issue. However this code path is not particularly nice to unit tests because the first option I have for a status code that isn't EFI_DEVICE_ERROR occurs in Dhcp6GenerateIaCb and I had to do some gross things to satisfy Dhcp6ParseAddrOption. Regardless through that testing I can confirm that I can hit the code paths that I need to be testing for this change. The Dhcp6SeekInnerOptionSafe function is well unit tested, and the code pattern is used elsewhere and is unit tested. So, I feel confident with the unit testing I have done that this change is successful, and I would like to follow up with unit tests / more code cleanup once we're out of code cleanup.

Further, I've performed a PxeBoot to ensure the device still boots - but that test generally doesn't feel like it's good enough for any confidence since I have no control over the code path. 

If you would like I can upload the Unit tests, but they're likely to undergo more changes and I wouldn't recommend getting them in right now.

- Doug


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v2 1/4] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch
  2024-02-13 23:31     ` Doug Flick via groups.io
@ 2024-02-14  2:40       ` Michael D Kinney
  2024-02-14  3:32         ` Michael D Kinney
  0 siblings, 1 reply; 13+ messages in thread
From: Michael D Kinney @ 2024-02-14  2:40 UTC (permalink / raw)
  To: devel@edk2.groups.io, dougflick@microsoft.com, Kasbekar, Saloni,
	Leif Lindholm, Andrew Fish (afish@apple.com), Gao, Liming
  Cc: Kinney, Michael D

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

Thanks for the details Doug.

I have applied the Rb tags and opened a PR: https://github.com/tianocore/edk2/pull/5372

Mike

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Doug Flick via groups.io
Sent: Tuesday, February 13, 2024 3:31 PM
To: Kasbekar, Saloni <saloni.kasbekar@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH v2 1/4] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch


Saloni,

Yeah there was never any tests that showed this bug exists mostly it was brought up through static analysis since it's related to a known CVE. I have written some unit tests (that I'm not particularly satisfied with) that show that I'm hitting the desired code paths that can trigger the issue. However this code path is not particularly nice to unit tests because the first option I have for a status code that isn't EFI_DEVICE_ERROR occurs in Dhcp6GenerateIaCb and I had to do some gross things to satisfy Dhcp6ParseAddrOption. Regardless through that testing I can confirm that I can hit the code paths that I need to be testing for this change. The Dhcp6SeekInnerOptionSafe function is well unit tested, and the code pattern is used elsewhere and is unit tested. So, I feel confident with the unit testing I have done that this change is successful, and I would like to follow up with unit tests / more code cleanup once we're out of code cleanup.

Further, I've performed a PxeBoot to ensure the device still boots - but that test generally doesn't feel like it's good enough for any confidence since I have no control over the code path.

If you would like I can upload the Unit tests, but they're likely to undergo more changes and I wouldn't recommend getting them in right now.

  *   Doug



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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v2 1/4] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch
  2024-02-14  2:40       ` Michael D Kinney
@ 2024-02-14  3:32         ` Michael D Kinney
  0 siblings, 0 replies; 13+ messages in thread
From: Michael D Kinney @ 2024-02-14  3:32 UTC (permalink / raw)
  To: devel@edk2.groups.io, dougflick@microsoft.com, Kasbekar, Saloni,
	Leif Lindholm, Andrew Fish (afish@apple.com), Gao, Liming
  Cc: Kinney, Michael D

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

Merged: https://github.com/tianocore/edk2/pull/5372

From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Tuesday, February 13, 2024 6:41 PM
To: devel@edk2.groups.io; dougflick@microsoft.com; Kasbekar, Saloni <saloni.kasbekar@intel.com>; Leif Lindholm <llindhol@qti.qualcomm.com>; Andrew Fish (afish@apple.com) <afish@apple.com>; Gao, Liming <gaoliming@byosoft.com.cn>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH v2 1/4] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch

Thanks for the details Doug.

I have applied the Rb tags and opened a PR: https://github.com/tianocore/edk2/pull/5372

Mike

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Doug Flick via groups.io
Sent: Tuesday, February 13, 2024 3:31 PM
To: Kasbekar, Saloni <saloni.kasbekar@intel.com<mailto:saloni.kasbekar@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [PATCH v2 1/4] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch


Saloni,

Yeah there was never any tests that showed this bug exists mostly it was brought up through static analysis since it's related to a known CVE. I have written some unit tests (that I'm not particularly satisfied with) that show that I'm hitting the desired code paths that can trigger the issue. However this code path is not particularly nice to unit tests because the first option I have for a status code that isn't EFI_DEVICE_ERROR occurs in Dhcp6GenerateIaCb and I had to do some gross things to satisfy Dhcp6ParseAddrOption. Regardless through that testing I can confirm that I can hit the code paths that I need to be testing for this change. The Dhcp6SeekInnerOptionSafe function is well unit tested, and the code pattern is used elsewhere and is unit tested. So, I feel confident with the unit testing I have done that this change is successful, and I would like to follow up with unit tests / more code cleanup once we're out of code cleanup.

Further, I've performed a PxeBoot to ensure the device still boots - but that test generally doesn't feel like it's good enough for any confidence since I have no control over the code path.

If you would like I can upload the Unit tests, but they're likely to undergo more changes and I wouldn't recommend getting them in right now.

  *   Doug



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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v2 4/4] NetworkPkg: : Updating SecurityFixes.yaml
  2024-02-13 18:46 ` [edk2-devel] [PATCH v2 4/4] NetworkPkg: : Updating SecurityFixes.yaml Doug Flick via groups.io
@ 2024-02-15 13:54   ` Rebecca Cran
  2024-02-15 18:14     ` Doug Flick via groups.io
  0 siblings, 1 reply; 13+ messages in thread
From: Rebecca Cran @ 2024-02-15 13:54 UTC (permalink / raw)
  To: devel, dougflick
  Cc: Saloni Kasbekar, Zachary Clark-williams, Doug Flick [MSFT]

I noticed the advisory at 
https://github.com/advisories/GHSA-h9v6-q439-p7j2 is labeled "Unreviewed".
Should it be updated, and should the 'package', 'affected' and 'patched' 
fields be updated?

-- 
Rebecca Cran


On 2/13/2024 11:46 AM, Doug Flick via groups.io wrote:
> 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"
>



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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [edk2-devel] [PATCH v2 4/4] NetworkPkg: : Updating SecurityFixes.yaml
  2024-02-15 13:54   ` Rebecca Cran
@ 2024-02-15 18:14     ` Doug Flick via groups.io
  0 siblings, 0 replies; 13+ messages in thread
From: Doug Flick via groups.io @ 2024-02-15 18:14 UTC (permalink / raw)
  To: Rebecca Cran, devel

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

At this point - yes - but I don't have the ability to edit it. The advisory should reflect the current status. SecurtiyFixes.yaml is a way to express which commits are needed to be cherrypicked by a downstream consumer and what the current release is protected against. 


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2024-02-15 18:14 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-02-13 18:45 [edk2-devel] [PATCH v2 0/4] Corrects additional concern in NetworkPkg Doug Flick via groups.io
2024-02-13 18:46 ` [edk2-devel] [PATCH v2 1/4] NetworkPkg: Dhcp6Dxe: SECURITY PATCH CVE-2023-45229 Related Patch Doug Flick via groups.io
2024-02-13 21:51   ` Saloni Kasbekar
2024-02-13 23:31     ` Doug Flick via groups.io
2024-02-14  2:40       ` Michael D Kinney
2024-02-14  3:32         ` Michael D Kinney
2024-02-13 18:46 ` [edk2-devel] [PATCH v2 2/4] NetworkPkg: Dhcp6Dxe: Removes duplicate check and replaces with macro Doug Flick via groups.io
2024-02-13 18:46 ` [edk2-devel] [PATCH v2 3/4] NetworkPkg: Dhcp6Dxe: Packet-Length is not updated before appending Doug Flick via groups.io
2024-02-13 20:16   ` Leif Lindholm
2024-02-13 18:46 ` [edk2-devel] [PATCH v2 4/4] NetworkPkg: : Updating SecurityFixes.yaml Doug Flick via groups.io
2024-02-15 13:54   ` Rebecca Cran
2024-02-15 18:14     ` Doug Flick via groups.io
2024-02-13 20:18 ` [edk2-devel] [PATCH v2 0/4] Corrects additional concern in NetworkPkg Leif Lindholm

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox