NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation.
authorMaciej Rabeda <maciej.rabeda@linux.intel.com>
Mon, 2 Mar 2020 12:25:20 +0000 (13:25 +0100)
committermergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Mon, 30 Mar 2020 13:13:29 +0000 (13:13 +0000)
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>
NetworkPkg/Ip6Dxe/Ip6Nd.c
NetworkPkg/Ip6Dxe/Ip6Nd.h
NetworkPkg/Ip6Dxe/Ip6Option.c

index 4288ef0..fd7f60b 100644 (file)
@@ -1927,7 +1927,7 @@ Ip6ProcessRouterAdvertise (
   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
@@ -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
-  // 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
-  while (Offset < Head->PayloadLength) {\r
+  while (Offset < (UINT32) Head->PayloadLength) {\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
-      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
@@ -2151,13 +2155,17 @@ Ip6ProcessRouterAdvertise (
         }\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
-      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
@@ -2321,9 +2329,12 @@ Ip6ProcessRouterAdvertise (
       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
@@ -2338,11 +2349,10 @@ Ip6ProcessRouterAdvertise (
       // Silently ignore unrecognized options\r
       //\r
       NetbufCopy (Packet, Offset + sizeof (UINT8), sizeof (UINT8), &Length);\r
-      if (Length <= 0) {\r
-        goto Exit;\r
-      }\r
 \r
-      Offset = (UINT16) (Offset + (UINT16) Length * 8);\r
+      ASSERT (Length != 0);\r
+\r
+      Offset += (UINT32) Length * 8;\r
       break;\r
     }\r
   }\r
index 560dfa3..5f1bd6f 100644 (file)
@@ -56,12 +56,21 @@ VOID
   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
+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
@@ -69,6 +78,8 @@ typedef struct _IP6_MTU_OPTION {
   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
@@ -80,6 +91,8 @@ typedef struct _IP6_PREFIX_INFO_OPTION {
   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
index 4d92a85..6b4b029 100644 (file)
@@ -129,45 +129,74 @@ Ip6IsNDOptionValid (
   IN UINT16                 OptionLen\r
   )\r
 {\r
-  UINT16                    Offset;\r
-  UINT8                     OptionType;\r
+  UINT32                    Offset;\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
-  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
-    switch (OptionType) {\r
+    switch (OptionHeader->Type) {\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
-\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
+      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
+    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
-    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