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

["Doug Flick via groups.io" <dougflick=microsoft.com@groups.io>]

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 (#114470): https://edk2.groups.io/g/devel/message/114470
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]
-=-=-=-=-=-=-=-=-=-=-=-