public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v1] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation (CVE-2019-14559)
@ 2020-03-02 12:38 Maciej Rabeda
  2020-03-25 11:35 ` [edk2-devel] " Laszlo Ersek
  0 siblings, 1 reply; 3+ messages in thread
From: Maciej Rabeda @ 2020-03-02 12:38 UTC (permalink / raw)
  To: devel; +Cc: Jiaxin Wu, Siyuan Fu

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>
---
 NetworkPkg/Ip6Dxe/Ip6Nd.c     | 44 +++++++++------
 NetworkPkg/Ip6Dxe/Ip6Nd.h     | 13 +++++
 NetworkPkg/Ip6Dxe/Ip6Option.c | 57 +++++++++++++++-----
 3 files changed, 83 insertions(+), 31 deletions(-)

diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c
index 4288ef02dd46..fd7f60b2f92c 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 560dfa343782..5f1bd6fb922a 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 4d92a852dc86..6b4b029d1479 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;
-- 
2.24.0.windows.2


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [edk2-devel] [PATCH v1] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation (CVE-2019-14559)
  2020-03-02 12:38 [PATCH v1] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation (CVE-2019-14559) Maciej Rabeda
@ 2020-03-25 11:35 ` Laszlo Ersek
  2020-03-26  0:01   ` Siyuan, Fu
  0 siblings, 1 reply; 3+ messages in thread
From: Laszlo Ersek @ 2020-03-25 11:35 UTC (permalink / raw)
  To: Jiaxin Wu, Siyuan Fu; +Cc: devel, maciej.rabeda

Jiaxin, Siyuan,

On 03/02/20 13:38, Maciej Rabeda wrote:
> 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>
> ---
>  NetworkPkg/Ip6Dxe/Ip6Nd.c     | 44 +++++++++------
>  NetworkPkg/Ip6Dxe/Ip6Nd.h     | 13 +++++
>  NetworkPkg/Ip6Dxe/Ip6Option.c | 57 +++++++++++++++-----
>  3 files changed, 83 insertions(+), 31 deletions(-)

can you please review this patch? It has spent 3+ weeks on the list.

Thanks,
Laszlo

> diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c
> index 4288ef02dd46..fd7f60b2f92c 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 560dfa343782..5f1bd6fb922a 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 4d92a852dc86..6b4b029d1479 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;
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [edk2-devel] [PATCH v1] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation (CVE-2019-14559)
  2020-03-25 11:35 ` [edk2-devel] " Laszlo Ersek
@ 2020-03-26  0:01   ` Siyuan, Fu
  0 siblings, 0 replies; 3+ messages in thread
From: Siyuan, Fu @ 2020-03-26  0:01 UTC (permalink / raw)
  To: devel@edk2.groups.io, lersek@redhat.com, Wu, Jiaxin
  Cc: maciej.rabeda@linux.intel.com

Maciej/Laszlo,

Sorry I missed this patch. It looks good with me.

Reviewed-by: Siyuan Fu <siyuan.fu@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: 2020年3月25日 19:36
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Cc: devel@edk2.groups.io; maciej.rabeda@linux.intel.com
> Subject: Re: [edk2-devel] [PATCH v1] NetworkPkg/Ip6Dxe: Improve Neightbor
> Discovery message validation (CVE-2019-14559)
> 
> Jiaxin, Siyuan,
> 
> On 03/02/20 13:38, Maciej Rabeda wrote:
> > 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>
> > ---
> >  NetworkPkg/Ip6Dxe/Ip6Nd.c     | 44 +++++++++------
> >  NetworkPkg/Ip6Dxe/Ip6Nd.h     | 13 +++++
> >  NetworkPkg/Ip6Dxe/Ip6Option.c | 57 +++++++++++++++-----
> >  3 files changed, 83 insertions(+), 31 deletions(-)
> 
> can you please review this patch? It has spent 3+ weeks on the list.
> 
> Thanks,
> Laszlo
> 
> > diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c
> > index 4288ef02dd46..fd7f60b2f92c 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 560dfa343782..5f1bd6fb922a 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 4d92a852dc86..6b4b029d1479 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;
> >
> 
> 
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-03-26  0:01 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-03-02 12:38 [PATCH v1] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation (CVE-2019-14559) Maciej Rabeda
2020-03-25 11:35 ` [edk2-devel] " Laszlo Ersek
2020-03-26  0:01   ` Siyuan, Fu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox