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>

diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c
index 4288ef02dd46df6d1be7f79fd1197b87f91d97b9..fd7f60b2f92c01a036328ed56060ea801604ab2a 100644 (file)
--- a/NetworkPkg/Ip6Dxe/Ip6Nd.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Nd.c
@@ -1927,7 +1927,7 @@ Ip6ProcessRouterAdvertise (
   UINT32                    ReachableTime;\r
   UINT32                    RetransTimer;\r
   UINT16                    RouterLifetime;\r
   UINT32                    ReachableTime;\r
   UINT32                    RetransTimer;\r
   UINT16                    RouterLifetime;\r

-  UINT16                    Offset;\r
+  UINT32                    Offset;\r

   UINT8                     Type;\r
   UINT8                     Length;\r
   IP6_ETHER_ADDR_OPTION     LinkLayerOption;\r
   UINT8                     Type;\r
   UINT8                     Length;\r
   IP6_ETHER_ADDR_OPTION     LinkLayerOption;\r


@@ -2094,10 +2094,11 @@ Ip6ProcessRouterAdvertise (
   //\r
   // The only defined options that may appear are the Source\r
   // Link-Layer Address, Prefix information and MTU options.\r
   //\r
   // The only defined options that may appear are the Source\r
   // Link-Layer Address, Prefix information and MTU options.\r

-  // All included options have a length that is greater than zero.\r
+  // All included options have a length that is greater than zero and\r
+  // fit within the input packet.\r

   //\r
   Offset = 16;\r
   //\r
   Offset = 16;\r

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

     NetbufCopy (Packet, Offset, sizeof (UINT8), &Type);\r
     switch (Type) {\r
     case Ip6OptionEtherSource:\r
     NetbufCopy (Packet, Offset, sizeof (UINT8), &Type);\r
     switch (Type) {\r
     case Ip6OptionEtherSource:\r


@@ -2105,9 +2106,12 @@ Ip6ProcessRouterAdvertise (
       // Update the neighbor cache\r
       //\r
       NetbufCopy (Packet, Offset, sizeof (IP6_ETHER_ADDR_OPTION), (UINT8 *) &LinkLayerOption);\r
       // Update the neighbor cache\r
       //\r
       NetbufCopy (Packet, Offset, sizeof (IP6_ETHER_ADDR_OPTION), (UINT8 *) &LinkLayerOption);\r

-      if (LinkLayerOption.Length <= 0) {\r
-        goto Exit;\r
-      }\r
+\r
+      //\r
+      // Option size validity ensured by Ip6IsNDOptionValid().\r
+      //\r
+      ASSERT (LinkLayerOption.Length != 0);\r
+      ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 >= (UINT32) Head->PayloadLength);\r

 \r
       ZeroMem (&LinkLayerAddress, sizeof (EFI_MAC_ADDRESS));\r
       CopyMem (&LinkLayerAddress, LinkLayerOption.EtherAddr, 6);\r
 \r
       ZeroMem (&LinkLayerAddress, sizeof (EFI_MAC_ADDRESS));\r
       CopyMem (&LinkLayerAddress, LinkLayerOption.EtherAddr, 6);\r


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

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

       break;\r
     case Ip6OptionPrefixInfo:\r
       NetbufCopy (Packet, Offset, sizeof (IP6_PREFIX_INFO_OPTION), (UINT8 *) &PrefixOption);\r
       break;\r
     case Ip6OptionPrefixInfo:\r
       NetbufCopy (Packet, Offset, sizeof (IP6_PREFIX_INFO_OPTION), (UINT8 *) &PrefixOption);\r

-      if (PrefixOption.Length != 4) {\r
-        goto Exit;\r
-      }\r
+\r
+      //\r
+      // Option size validity ensured by Ip6IsNDOptionValid().\r
+      //\r
+      ASSERT (PrefixOption.Length == 4);\r
+      ASSERT (Offset + (UINT32) PrefixOption.Length * 8 >= (UINT32) Head->PayloadLength);\r
+\r

       PrefixOption.ValidLifetime     = NTOHL (PrefixOption.ValidLifetime);\r
       PrefixOption.PreferredLifetime = NTOHL (PrefixOption.PreferredLifetime);\r
 \r
       PrefixOption.ValidLifetime     = NTOHL (PrefixOption.ValidLifetime);\r
       PrefixOption.PreferredLifetime = NTOHL (PrefixOption.PreferredLifetime);\r
 \r


@@ -2321,9 +2329,12 @@ Ip6ProcessRouterAdvertise (
       break;\r
     case Ip6OptionMtu:\r
       NetbufCopy (Packet, Offset, sizeof (IP6_MTU_OPTION), (UINT8 *) &MTUOption);\r
       break;\r
     case Ip6OptionMtu:\r
       NetbufCopy (Packet, Offset, sizeof (IP6_MTU_OPTION), (UINT8 *) &MTUOption);\r

-      if (MTUOption.Length != 1) {\r
-        goto Exit;\r
-      }\r
+\r
+      //\r
+      // Option size validity ensured by Ip6IsNDOptionValid().\r
+      //\r
+      ASSERT (MTUOption.Length == 1);\r
+      ASSERT (Offset + (UINT32) MTUOption.Length * 8 >= (UINT32) Head->PayloadLength);\r

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


@@ -2338,11 +2349,10 @@ Ip6ProcessRouterAdvertise (
       // Silently ignore unrecognized options\r
       //\r
       NetbufCopy (Packet, Offset + sizeof (UINT8), sizeof (UINT8), &Length);\r
       // Silently ignore unrecognized options\r
       //\r
       NetbufCopy (Packet, Offset + sizeof (UINT8), sizeof (UINT8), &Length);\r

-      if (Length <= 0) {\r
-        goto Exit;\r
-      }\r

 \r
 \r

-      Offset = (UINT16) (Offset + (UINT16) Length * 8);\r
+      ASSERT (Length != 0);\r
+\r
+      Offset += (UINT32) Length * 8;\r

       break;\r
     }\r
   }\r
       break;\r
     }\r
   }\r

diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.h b/NetworkPkg/Ip6Dxe/Ip6Nd.h
index 560dfa343782e38a2c2d83e13b2e99f10d017ffe..5f1bd6fb922a7faeb4d687e543d37f1a9c1ea5a9 100644 (file)
--- a/NetworkPkg/Ip6Dxe/Ip6Nd.h
+++ b/NetworkPkg/Ip6Dxe/Ip6Nd.h
@@ -56,12 +56,21 @@ VOID
   VOID                      *Context\r
   );\r
 \r
   VOID                      *Context\r
   );\r
 \r

+typedef struct _IP6_OPTION_HEADER {\r
+  UINT8                     Type;\r
+  UINT8                     Length;\r
+} IP6_OPTION_HEADER;\r
+\r
+STATIC_ASSERT (sizeof (IP6_OPTION_HEADER) == 2, "IP6_OPTION_HEADER is expected to be exactly 2 bytes long.");\r
+\r

 typedef struct _IP6_ETHE_ADDR_OPTION {\r
   UINT8                     Type;\r
   UINT8                     Length;\r
   UINT8                     EtherAddr[6];\r
 } IP6_ETHER_ADDR_OPTION;\r
 \r
 typedef struct _IP6_ETHE_ADDR_OPTION {\r
   UINT8                     Type;\r
   UINT8                     Length;\r
   UINT8                     EtherAddr[6];\r
 } IP6_ETHER_ADDR_OPTION;\r
 \r

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

 typedef struct _IP6_MTU_OPTION {\r
   UINT8                     Type;\r
   UINT8                     Length;\r
 typedef struct _IP6_MTU_OPTION {\r
   UINT8                     Type;\r
   UINT8                     Length;\r


@@ -69,6 +78,8 @@ typedef struct _IP6_MTU_OPTION {
   UINT32                    Mtu;\r
 } IP6_MTU_OPTION;\r
 \r
   UINT32                    Mtu;\r
 } IP6_MTU_OPTION;\r
 \r

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

 typedef struct _IP6_PREFIX_INFO_OPTION {\r
   UINT8                     Type;\r
   UINT8                     Length;\r
 typedef struct _IP6_PREFIX_INFO_OPTION {\r
   UINT8                     Type;\r
   UINT8                     Length;\r


@@ -80,6 +91,8 @@ typedef struct _IP6_PREFIX_INFO_OPTION {
   EFI_IPv6_ADDRESS          Prefix;\r
 } IP6_PREFIX_INFO_OPTION;\r
 \r
   EFI_IPv6_ADDRESS          Prefix;\r
 } IP6_PREFIX_INFO_OPTION;\r
 \r

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

 typedef\r
 VOID\r
 (*IP6_DAD_CALLBACK) (\r
 typedef\r
 VOID\r
 (*IP6_DAD_CALLBACK) (\r

diff --git a/NetworkPkg/Ip6Dxe/Ip6Option.c b/NetworkPkg/Ip6Dxe/Ip6Option.c
index 4d92a852dc864757aa8d3f4b05acaa58740d0c3e..6b4b029d14791c1643ed607fb7775d4c0745da2c 100644 (file)
--- a/NetworkPkg/Ip6Dxe/Ip6Option.c
+++ b/NetworkPkg/Ip6Dxe/Ip6Option.c
@@ -129,45 +129,74 @@ Ip6IsNDOptionValid (
   IN UINT16                 OptionLen\r
   )\r
 {\r
   IN UINT16                 OptionLen\r
   )\r
 {\r

-  UINT16                    Offset;\r
-  UINT8                     OptionType;\r
+  UINT32                    Offset;\r

   UINT16                    Length;\r
   UINT16                    Length;\r

+  IP6_OPTION_HEADER         *OptionHeader;\r
+\r
+  if (Option == NULL) {\r
+    ASSERT (Option != NULL);\r
+    return FALSE;\r
+  }\r

 \r
   Offset = 0;\r
 \r
 \r
   Offset = 0;\r
 \r

-  while (Offset < OptionLen) {\r
-    OptionType = *(Option + Offset);\r
-     Length    = (UINT16) (*(Option + Offset + 1) * 8);\r
+  //\r
+  // RFC 4861 states that Neighbor Discovery packet can contain zero or more\r
+  // options. Start processing the options if at least Type + Length fields\r
+  // fit within the input buffer.\r
+  //\r
+  while (Offset + sizeof (IP6_OPTION_HEADER) - 1 < OptionLen) {\r
+    OptionHeader  = (IP6_OPTION_HEADER*) (Option + Offset);\r
+    Length        = (UINT16) OptionHeader->Length * 8;\r

 \r
 \r

-    switch (OptionType) {\r
+    switch (OptionHeader->Type) {\r

     case Ip6OptionPrefixInfo:\r
       if (Length != 32) {\r
         return FALSE;\r
       }\r
     case Ip6OptionPrefixInfo:\r
       if (Length != 32) {\r
         return FALSE;\r
       }\r

-\r

       break;\r
 \r
     case Ip6OptionMtu:\r
       if (Length != 8) {\r
         return FALSE;\r
       }\r
       break;\r
 \r
     case Ip6OptionMtu:\r
       if (Length != 8) {\r
         return FALSE;\r
       }\r

-\r

       break;\r
 \r
     default:\r
       break;\r
 \r
     default:\r

-      //\r
-      // Check the length of Ip6OptionEtherSource, Ip6OptionEtherTarget, and\r
-      // Ip6OptionRedirected here. For unrecognized options, silently ignore\r
-      // and continue processing the message.\r
-      //\r
+      // RFC 4861 states that Length field cannot be 0.\r

       if (Length == 0) {\r
         return FALSE;\r
       }\r
       if (Length == 0) {\r
         return FALSE;\r
       }\r

+      break;\r
+    }\r
+\r
+    //\r
+    // Check whether recognized options are within the input buffer's scope.\r
+    //\r
+    switch (OptionHeader->Type) {\r
+    case Ip6OptionEtherSource:\r
+    case Ip6OptionEtherTarget:\r
+    case Ip6OptionPrefixInfo:\r
+    case Ip6OptionRedirected:\r
+    case Ip6OptionMtu:\r
+      if (Offset + Length > (UINT32) OptionLen) {\r
+        return FALSE;\r
+      }\r
+      break;\r

 \r
 \r

+    default:\r
+      //\r
+      // Unrecognized options can be either valid (but unused) or invalid\r
+      // (garbage in between or right after valid options). Silently ignore.\r
+      //\r

       break;\r
     }\r
 \r
       break;\r
     }\r
 \r

-    Offset = (UINT16) (Offset + Length);\r
+    //\r
+    // Advance to the next option.\r
+    // Length already considers option header's Type + Length.\r
+    //\r
+    Offset += Length;\r

   }\r
 \r
   return TRUE;\r
   }\r
 \r
   return TRUE;\r