NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation.

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

Problem has been identified with Ip6ProcessRouterAdvertise() when
Router Advertise packet contains options with malicious/invalid
'Length' field. This can lead to platform entering infinite loop
when processing options from that packet.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Reviewed-by: Siyuan Fu <siyuan.fu@intel.com>

3 files changed, 83 insertions, 31 deletions

diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c
index 4288ef02dd..fd7f60b2f9 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Nd.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Nd.c
@@ -1927,7 +1927,7 @@ Ip6ProcessRouterAdvertise (
   UINT32                    ReachableTime;

   UINT32                    RetransTimer;

   UINT16                    RouterLifetime;

-  UINT16                    Offset;

+  UINT32                    Offset;

   UINT8                     Type;

   UINT8                     Length;

   IP6_ETHER_ADDR_OPTION     LinkLayerOption;

@@ -2094,10 +2094,11 @@ Ip6ProcessRouterAdvertise (
   //

   // The only defined options that may appear are the Source

   // Link-Layer Address, Prefix information and MTU options.

-  // All included options have a length that is greater than zero.

+  // All included options have a length that is greater than zero and

+  // fit within the input packet.

   //

   Offset = 16;

-  while (Offset < Head->PayloadLength) {

+  while (Offset < (UINT32) Head->PayloadLength) {

     NetbufCopy (Packet, Offset, sizeof (UINT8), &Type);

     switch (Type) {

     case Ip6OptionEtherSource:

@@ -2105,9 +2106,12 @@ Ip6ProcessRouterAdvertise (
       // Update the neighbor cache

       //

       NetbufCopy (Packet, Offset, sizeof (IP6_ETHER_ADDR_OPTION), (UINT8 *) &LinkLayerOption);

-      if (LinkLayerOption.Length <= 0) {

-        goto Exit;

-      }

+

+      //

+      // Option size validity ensured by Ip6IsNDOptionValid().

+      //

+      ASSERT (LinkLayerOption.Length != 0);

+      ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 >= (UINT32) Head->PayloadLength);

 

       ZeroMem (&LinkLayerAddress, sizeof (EFI_MAC_ADDRESS));

       CopyMem (&LinkLayerAddress, LinkLayerOption.EtherAddr, 6);

@@ -2151,13 +2155,17 @@ Ip6ProcessRouterAdvertise (
         }

       }

 

-      Offset = (UINT16) (Offset + (UINT16) LinkLayerOption.Length * 8);

+      Offset += (UINT32) LinkLayerOption.Length * 8;

       break;

     case Ip6OptionPrefixInfo:

       NetbufCopy (Packet, Offset, sizeof (IP6_PREFIX_INFO_OPTION), (UINT8 *) &PrefixOption);

-      if (PrefixOption.Length != 4) {

-        goto Exit;

-      }

+

+      //

+      // Option size validity ensured by Ip6IsNDOptionValid().

+      //

+      ASSERT (PrefixOption.Length == 4);

+      ASSERT (Offset + (UINT32) PrefixOption.Length * 8 >= (UINT32) Head->PayloadLength);

+

       PrefixOption.ValidLifetime     = NTOHL (PrefixOption.ValidLifetime);

       PrefixOption.PreferredLifetime = NTOHL (PrefixOption.PreferredLifetime);

 

@@ -2321,9 +2329,12 @@ Ip6ProcessRouterAdvertise (
       break;

     case Ip6OptionMtu:

       NetbufCopy (Packet, Offset, sizeof (IP6_MTU_OPTION), (UINT8 *) &MTUOption);

-      if (MTUOption.Length != 1) {

-        goto Exit;

-      }

+

+      //

+      // Option size validity ensured by Ip6IsNDOptionValid().

+      //

+      ASSERT (MTUOption.Length == 1);

+      ASSERT (Offset + (UINT32) MTUOption.Length * 8 >= (UINT32) Head->PayloadLength);

 

       //

       // Use IPv6 minimum link MTU 1280 bytes as the maximum packet size in order

@@ -2338,11 +2349,10 @@ Ip6ProcessRouterAdvertise (
       // Silently ignore unrecognized options

       //

       NetbufCopy (Packet, Offset + sizeof (UINT8), sizeof (UINT8), &Length);

-      if (Length <= 0) {

-        goto Exit;

-      }

 

-      Offset = (UINT16) (Offset + (UINT16) Length * 8);

+      ASSERT (Length != 0);

+

+      Offset += (UINT32) Length * 8;

       break;

     }

   }

diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.h b/NetworkPkg/Ip6Dxe/Ip6Nd.h
index 560dfa3437..5f1bd6fb92 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Nd.h
+++ b/NetworkPkg/Ip6Dxe/Ip6Nd.h
@@ -56,12 +56,21 @@ VOID
   VOID                      *Context

   );

 

+typedef struct _IP6_OPTION_HEADER {

+  UINT8                     Type;

+  UINT8                     Length;

+} IP6_OPTION_HEADER;

+

+STATIC_ASSERT (sizeof (IP6_OPTION_HEADER) == 2, "IP6_OPTION_HEADER is expected to be exactly 2 bytes long.");

+

 typedef struct _IP6_ETHE_ADDR_OPTION {

   UINT8                     Type;

   UINT8                     Length;

   UINT8                     EtherAddr[6];

 } IP6_ETHER_ADDR_OPTION;

 

+STATIC_ASSERT (sizeof (IP6_ETHER_ADDR_OPTION) == 8, "IP6_ETHER_ADDR_OPTION is expected to be exactly 8 bytes long.");

+

 typedef struct _IP6_MTU_OPTION {

   UINT8                     Type;

   UINT8                     Length;

@@ -69,6 +78,8 @@ typedef struct _IP6_MTU_OPTION {
   UINT32                    Mtu;

 } IP6_MTU_OPTION;

 

+STATIC_ASSERT (sizeof (IP6_MTU_OPTION) == 8, "IP6_MTU_OPTION is expected to be exactly 8 bytes long.");

+

 typedef struct _IP6_PREFIX_INFO_OPTION {

   UINT8                     Type;

   UINT8                     Length;

@@ -80,6 +91,8 @@ typedef struct _IP6_PREFIX_INFO_OPTION {
   EFI_IPv6_ADDRESS          Prefix;

 } IP6_PREFIX_INFO_OPTION;

 

+STATIC_ASSERT (sizeof (IP6_PREFIX_INFO_OPTION) == 32, "IP6_PREFIX_INFO_OPTION is expected to be exactly 32 bytes long.");

+

 typedef

 VOID

 (*IP6_DAD_CALLBACK) (

diff --git a/NetworkPkg/Ip6Dxe/Ip6Option.c b/NetworkPkg/Ip6Dxe/Ip6Option.c
index 4d92a852dc..6b4b029d14 100644
--- a/NetworkPkg/Ip6Dxe/Ip6Option.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Option.c
@@ -129,45 +129,74 @@ Ip6IsNDOptionValid (
   IN UINT16                 OptionLen

   )

 {

-  UINT16                    Offset;

-  UINT8                     OptionType;

+  UINT32                    Offset;

   UINT16                    Length;

+  IP6_OPTION_HEADER         *OptionHeader;

+

+  if (Option == NULL) {

+    ASSERT (Option != NULL);

+    return FALSE;

+  }

 

   Offset = 0;

 

-  while (Offset < OptionLen) {

-    OptionType = *(Option + Offset);

-     Length    = (UINT16) (*(Option + Offset + 1) * 8);

+  //

+  // RFC 4861 states that Neighbor Discovery packet can contain zero or more

+  // options. Start processing the options if at least Type + Length fields

+  // fit within the input buffer.

+  //

+  while (Offset + sizeof (IP6_OPTION_HEADER) - 1 < OptionLen) {

+    OptionHeader  = (IP6_OPTION_HEADER*) (Option + Offset);

+    Length        = (UINT16) OptionHeader->Length * 8;

 

-    switch (OptionType) {

+    switch (OptionHeader->Type) {

     case Ip6OptionPrefixInfo:

       if (Length != 32) {

         return FALSE;

       }

-

       break;

 

     case Ip6OptionMtu:

       if (Length != 8) {

         return FALSE;

       }

-

       break;

 

     default:

-      //

-      // Check the length of Ip6OptionEtherSource, Ip6OptionEtherTarget, and

-      // Ip6OptionRedirected here. For unrecognized options, silently ignore

-      // and continue processing the message.

-      //

+      // RFC 4861 states that Length field cannot be 0.

       if (Length == 0) {

         return FALSE;

       }

+      break;

+    }

+

+    //

+    // Check whether recognized options are within the input buffer's scope.

+    //

+    switch (OptionHeader->Type) {

+    case Ip6OptionEtherSource:

+    case Ip6OptionEtherTarget:

+    case Ip6OptionPrefixInfo:

+    case Ip6OptionRedirected:

+    case Ip6OptionMtu:

+      if (Offset + Length > (UINT32) OptionLen) {

+        return FALSE;

+      }

+      break;

 

+    default:

+      //

+      // Unrecognized options can be either valid (but unused) or invalid

+      // (garbage in between or right after valid options). Silently ignore.

+      //

       break;

     }

 

-    Offset = (UINT16) (Offset + Length);

+    //

+    // Advance to the next option.

+    // Length already considers option header's Type + Length.

+    //

+    Offset += Length;

   }

 

   return TRUE;