Re: [edk2-devel] [PATCH v2 08/15] NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45232 Patch

["Saloni Kasbekar" <saloni.kasbekar@intel.com>]

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

-----Original Message-----
From: Douglas Flick [MSFT] <doug.edk2@gmail.com> 
Sent: Thursday, January 25, 2024 1:55 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>; Doug Flick [MSFT] <doug.edk2@gmail.com>
Subject: [PATCH v2 08/15] NetworkPkg: Ip6Dxe: SECURITY PATCH CVE-2023-45232 Patch

From: Doug Flick <dougflick@microsoft.com>

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4537
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4538

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

Infinite loop when parsing unknown options in the Destination Options header

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

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

Change Overview:

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

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

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

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

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

Additionally this check adds additional logic to santize the incoming data

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

Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com>
---
 NetworkPkg/Ip6Dxe/Ip6Nd.h     | 35 ++++++++++++++++
 NetworkPkg/Ip6Dxe/Ip6Option.h | 71 ++++++++++++++++++++++++++++++++  NetworkPkg/Ip6Dxe/Ip6Option.c | 76 ++++++++++++++++++++++++++++++-----
 3 files changed, 171 insertions(+), 11 deletions(-)

diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.h b/NetworkPkg/Ip6Dxe/Ip6Nd.h index 860934a167eb..bf64e9114e13 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Nd.h
+++ b/NetworkPkg/Ip6Dxe/Ip6Nd.h
@@ -56,13 +56,48 @@ VOID
   VOID  *Context   ); +//+// Per RFC8200 Section 4.2+//+//   Two of the currently-defined extension headers -- the Hop-by-Hop+//   Options header and the Destination Options header -- carry a variable+//   number of type-length-value (TLV) encoded "options", of the following+//   format:+//+//      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- - - - - - - - -+//      |  Option Type  |  Opt Data Len |  Option Data+//      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+- - - - - - - - -+//+//      Option Type          8-bit identifier of the type of option.+//+//      Opt Data Len         8-bit unsigned integer.  Length of the Option+//                           Data field of this option, in octets.+//+//      Option Data          Variable-length field.  Option-Type-specific+//                           data.+// typedef struct _IP6_OPTION_HEADER {+  ///+  /// identifier of the type of option.+  ///   UINT8    Type;+  ///+  /// Length of the Option Data field of this option, in octets.+  ///   UINT8    Length;+  ///+  /// Option-Type-specific data.+  /// } IP6_OPTION_HEADER;  STATIC_ASSERT (sizeof (IP6_OPTION_HEADER) == 2, "IP6_OPTION_HEADER is expected to be exactly 2 bytes long."); +#define IP6_NEXT_OPTION_OFFSET(offset, length)  (offset + sizeof(IP6_OPTION_HEADER) + length)+STATIC_ASSERT (+  IP6_NEXT_OPTION_OFFSET (0, 0) == 2,+  "The next option is minimally the combined size of the option tag and length"+  );+ typedef struct _IP6_ETHE_ADDR_OPTION {   UINT8    Type;   UINT8    Length;diff --git a/NetworkPkg/Ip6Dxe/Ip6Option.h b/NetworkPkg/Ip6Dxe/Ip6Option.h
index bd8e223c8a67..fb07c28f5ad7 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Option.h
+++ b/NetworkPkg/Ip6Dxe/Ip6Option.h
@@ -12,6 +12,77 @@
  #define IP6_FRAGMENT_OFFSET_MASK  (~0x3) +//+// For more information see RFC 8200, Section 4.3, 4.4, and 4.6+//+//  This example format is from section 4.6+//  This does not apply to fragment headers+//+//     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-++//    |  Next Header  |  Hdr Ext Len  |                               |+//    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+                               ++//    |                                                               |+//    .                                                               .+//    .                  Header-Specific Data                         .+//    .                                                               .+//    |                                                               |+//    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-++//+//      Next Header           8-bit selector.  Identifies the type of+//                            header immediately following the extension+//                            header.  Uses the same values as the IPv4+//                            Protocol field [IANA-PN].+//+//      Hdr Ext Len           8-bit unsigned integer.  Length of the+//                            Destination Options header in 8-octet units,+//                            not including the first 8 octets.++//+// These defines apply to the following:+//   1. Hop by Hop+//   2. Routing+//   3. Destination+//+typedef struct _IP6_EXT_HDR {+  ///+  /// The Next Header field identifies the type of header immediately+  ///+  UINT8    NextHeader;+  ///+  /// The Hdr Ext Len field specifies the length of the Hop-by-Hop Options+  ///+  UINT8    HdrExtLen;+  ///+  /// Header-Specific Data+  ///+} IP6_EXT_HDR;++STATIC_ASSERT (+  sizeof (IP6_EXT_HDR) == 2,+  "The combined size of Next Header and Len is two 8 bit fields"+  );++//+// IPv6 extension headers contain an 8-bit length field which describes the size of+// the header. However, the length field only includes the size of the extension+// header options, not the size of the first 8 bytes of the header. Therefore, in+// order to calculate the full size of the extension header, we add 1 (to account+// for the first 8 bytes omitted by the length field reporting) and then multiply+// by 8 (since the size is represented in 8-byte units).+//+// a is the length field of the extension header (UINT8)+// The result may be up to 2046 octets (UINT16)+//+#define IP6_HDR_EXT_LEN(a)  (((UINT16)((UINT8)(a)) + 1) * 8)++// This is the maxmimum length permissible by a extension header+// Length is UINT8 of 8 octets not including the first 8 octets+#define IP6_MAX_EXT_DATA_LENGTH  (IP6_HDR_EXT_LEN (MAX_UINT8) - sizeof(IP6_EXT_HDR))+STATIC_ASSERT (+  IP6_MAX_EXT_DATA_LENGTH == 2046,+  "Maximum data length is ((MAX_UINT8 + 1) * 8) - 2"+  );+ typedef struct _IP6_FRAGMENT_HEADER {   UINT8     NextHeader;   UINT8     Reserved;diff --git a/NetworkPkg/Ip6Dxe/Ip6Option.c b/NetworkPkg/Ip6Dxe/Ip6Option.c
index 8718d5d8756a..fd97ce116f98 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Option.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Option.c
@@ -17,7 +17,8 @@
   @param[in]  IpSb              The IP6 service data.   @param[in]  Packet            The to be validated packet.   @param[in]  Option            The first byte of the option.-  @param[in]  OptionLen         The length of the whole option.+  @param[in]  OptionLen         The length of all options, expressed in byte length of octets.+                                Maximum length is 2046 bytes or ((n + 1) * 8) - 2 where n is 255.   @param[in]  Pointer           Identifies the octet offset within                                 the invoking packet where the error was detected. @@ -31,12 +32,33 @@ Ip6IsOptionValid (
   IN IP6_SERVICE  *IpSb,   IN NET_BUF      *Packet,   IN UINT8        *Option,-  IN UINT8        OptionLen,+  IN UINT16       OptionLen,   IN UINT32       Pointer   ) {-  UINT8  Offset;-  UINT8  OptionType;+  UINT16  Offset;+  UINT8   OptionType;+  UINT8   OptDataLen;++  if (Option == NULL) {+    ASSERT (Option != NULL);+    return FALSE;+  }++  if ((OptionLen <= 0) || (OptionLen > IP6_MAX_EXT_DATA_LENGTH)) {+    ASSERT (OptionLen > 0 && OptionLen <= IP6_MAX_EXT_DATA_LENGTH);+    return FALSE;+  }++  if (Packet == NULL) {+    ASSERT (Packet != NULL);+    return FALSE;+  }++  if (IpSb == NULL) {+    ASSERT (IpSb != NULL);+    return FALSE;+  }    Offset = 0; @@ -54,7 +76,8 @@ Ip6IsOptionValid (
         //         // It is a PadN option         //-        Offset = (UINT8)(Offset + *(Option + Offset + 1) + 2);+        OptDataLen = ((IP6_OPTION_HEADER *)(Option + Offset))->Length;+        Offset     = IP6_NEXT_OPTION_OFFSET (Offset, OptDataLen);         break;       case Ip6OptionRouterAlert:         //@@ -69,7 +92,8 @@ Ip6IsOptionValid (
         //         switch (OptionType & Ip6OptionMask) {           case Ip6OptionSkip:-            Offset = (UINT8)(Offset + *(Option + Offset + 1));+            OptDataLen = ((IP6_OPTION_HEADER *)(Option + Offset))->Length;+            Offset     = IP6_NEXT_OPTION_OFFSET (Offset, OptDataLen);             break;           case Ip6OptionDiscard:             return FALSE;@@ -308,7 +332,7 @@ Ip6IsExtsValid (
   UINT32               Pointer;   UINT32               Offset;   UINT8                *Option;-  UINT8                OptionLen;+  UINT16               OptionLen;   BOOLEAN              Flag;   UINT8                CountD;   UINT8                CountA;@@ -385,6 +409,36 @@ Ip6IsExtsValid (
       // Fall through       //       case IP6_DESTINATION:+        //+        // See https://www.rfc-editor.org/rfc/rfc2460#section-4.2 page 23+        //+        // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-++        // |  Next Header  |  Hdr Ext Len  |                               |+        // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+                               ++        // |                                                               |+        // .                                                               .+        // .                            Options                            .+        // .                                                               .+        // |                                                               |+        // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-++        //+        //+        //   Next Header    8-bit selector.  Identifies the type of header+        //                  immediately following the Destination Options+        //                  header.  Uses the same values as the IPv4+        //                  Protocol field [RFC-1700 et seq.].+        //+        //   Hdr Ext Len    8-bit unsigned integer.  Length of the+        //                  Destination Options header in 8-octet units, not+        //                  including the first 8 octets.+        //+        //   Options        Variable-length field, of length such that the+        //                  complete Destination Options header is an+        //                  integer multiple of 8 octets long.  Contains one+        //                  or  more TLV-encoded options, as described in+        //                  section 4.2.+        //+         if (*NextHeader == IP6_DESTINATION) {           CountD++;         }@@ -398,7 +452,7 @@ Ip6IsExtsValid (
          Offset++;         Option    = ExtHdrs + Offset;-        OptionLen = (UINT8)((*Option + 1) * 8 - 2);+        OptionLen = IP6_HDR_EXT_LEN (*Option) - sizeof (IP6_EXT_HDR);         Option++;         Offset++; @@ -430,7 +484,7 @@ Ip6IsExtsValid (
           //           // Ignore the routing header and proceed to process the next header.           //-          Offset = Offset + (RoutingHead->HeaderLen + 1) * 8;+          Offset = Offset + IP6_HDR_EXT_LEN (RoutingHead->HeaderLen);            if (UnFragmentLen != NULL) {             *UnFragmentLen = Offset;@@ -441,7 +495,7 @@ Ip6IsExtsValid (
           // to the packet's source address, pointing to the unrecognized routing           // type.           //-          Pointer = Offset + 2 + sizeof (EFI_IP6_HEADER);+          Pointer = Offset + sizeof (IP6_EXT_HDR) + sizeof (EFI_IP6_HEADER);           if ((IpSb != NULL) && (Packet != NULL) &&               !IP6_IS_MULTICAST (&Packet->Ip.Ip6->DestinationAddress))           {@@ -527,7 +581,7 @@ Ip6IsExtsValid (
         //         // RFC2402, Payload length is specified in 32-bit words, minus "2".         //-        OptionLen = (UINT8)((*Option + 2) * 4);+        OptionLen = ((UINT16)(*Option + 2) * 4);         Offset    = Offset + OptionLen;         break; -- 
2.43.0



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