Re: [edk2-devel] [PATCH v1] MdeModulePkg/Ip4Dxe: Check the received package length (CVE-2019-14559).

["Liming Gao" <liming.gao@intel.com>]

Another minor comment. Ip4Dxe is moved into NetworkPkg. So, the patch title should be NetworkPkg/Ip4Dxe.

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Monday, February 17, 2020 6:40 PM
> To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>
> Cc: Fu, Siyuan <siyuan.fu@intel.com>; Maciej Rabeda <maciej.rabeda@linux.intel.com>; Armour, Nicholas <nicholas.armour@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1] MdeModulePkg/Ip4Dxe: Check the received package length (CVE-2019-14559).
> 
> On 02/17/20 08:43, Wu, Jiaxin wrote:
> > This patch is to check the received package length to make sure the package
> > has a valid length field.
> >
> > Cc: Fu Siyuan <siyuan.fu@intel.com>
> > Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> > Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> > Reviewed-by: Siyuan Fu <siyuan.fu@intel.com>
> > ---
> >  NetworkPkg/Ip4Dxe/Ip4Input.c | 46 +++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 37 insertions(+), 9 deletions(-)
> 
> There are two patches on the list for CVE-2019-14559:
> 
> - [edk2-devel] [PATCH v1] MdeModulePkg/Ip4Dxe: Check the received package length (CVE-2019-14559).
> - [edk2-devel] [PATCH 1/1] NetworkPkg/ArpDxe: Recycle invalid ARP packets(CVE-2019-14559).
> 
> sent by different submitters.
> 
> How do they relate to each other?
> 
> Also, while Nick's patch mentions TianoCore#2031, the current patch doesn't include a BZ link. Is the current patch for TianoCore#2032?
> (Per <https://bugzilla.tianocore.org/show_bug.cgi?id=2032#c8>, both BZs share the same CVE ID.)
> 
> Also, I remain confused (with comment 11 being the latest one, as of this time, in TianoCore#2032), whether the issue affects IPv4 only,
> IPv6 only, or both. This patch is only for IPv4, apparently.
> 
> If the present patch is related to TianoCore#2032, then please add a mailing list archive link to the BZ, and move the BZ to IN_PROGRESS
> status.
> 
> Laszlo
> 
> >
> > diff --git a/NetworkPkg/Ip4Dxe/Ip4Input.c b/NetworkPkg/Ip4Dxe/Ip4Input.c
> > index fec242c71f..95fbd01d05 100644
> > --- a/NetworkPkg/Ip4Dxe/Ip4Input.c
> > +++ b/NetworkPkg/Ip4Dxe/Ip4Input.c
> > @@ -1,9 +1,9 @@
> >  /** @file
> >    IP4 input process.
> >
> > -Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.<BR>
> > +Copyright (c) 2005 - 2020, Intel Corporation. All rights reserved.<BR>
> >  (C) Copyright 2015 Hewlett-Packard Development Company, L.P.<BR>
> >
> >  SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >  **/
> > @@ -709,14 +709,10 @@ Ip4PreProcessPacket (
> >    UINT16                    Checksum;
> >
> >    //
> >    // Check if the IP4 header is correctly formatted.
> >    //
> > -  if ((*Packet)->TotalSize < IP4_MIN_HEADLEN) {
> > -    return EFI_INVALID_PARAMETER;
> > -  }
> > -
> >    HeadLen  = (Head->HeadLen << 2);
> >    TotalLen = NTOHS (Head->TotalLen);
> >
> >    //
> >    // Mnp may deliver frame trailer sequence up, trim it off.
> > @@ -806,10 +802,34 @@ Ip4PreProcessPacket (
> >    }
> >
> >    return EFI_SUCCESS;
> >  }
> >
> > +/**
> > +  This function checks the IPv4 packet length.
> > +
> > +  @param[in]       Packet          Pointer to the IPv4 Packet to be checked.
> > +
> > +  @retval TRUE                   The input IPv4 packet length is valid.
> > +  @retval FALSE                  The input IPv4 packet length is invalid.
> > +
> > +**/
> > +BOOLEAN
> > +Ip4IsValidPacketLength (
> > +  IN NET_BUF        *Packet
> > +  )
> > +{
> > +  //
> > +  // Check the IP4 packet length.
> > +  //
> > +  if (Packet->TotalSize < IP4_MIN_HEADLEN) {
> > +    return FALSE;
> > +  }
> > +
> > +  return TRUE;
> > +}
> > +
> >  /**
> >    The IP4 input routine. It is called by the IP4_INTERFACE when a
> >    IP4 fragment is received from MNP.
> >
> >    @param[in]  Ip4Instance        The IP4 child that request the receive, most like
> > @@ -842,10 +862,14 @@ Ip4AccpetFrame (
> >
> >    if (EFI_ERROR (IoStatus) || (IpSb->State == IP4_SERVICE_DESTROY)) {
> >      goto DROP;
> >    }
> >
> > +  if (!Ip4IsValidPacketLength (Packet)) {
> > +    goto RESTART;
> > +  }
> > +
> >    Head      = (IP4_HEAD *) NetbufGetByte (Packet, 0, NULL);
> >    ASSERT (Head != NULL);
> >    OptionLen = (Head->HeadLen << 2) - IP4_MIN_HEADLEN;
> >    if (OptionLen > 0) {
> >      Option = (UINT8 *) (Head + 1);
> > @@ -888,14 +912,18 @@ Ip4AccpetFrame (
> >    //
> >    // If the packet is protected by tunnel mode, parse the inner Ip Packet.
> >    //
> >    ZeroMem (&ZeroHead, sizeof (IP4_HEAD));
> >    if (0 == CompareMem (Head, &ZeroHead, sizeof (IP4_HEAD))) {
> > -  // Packet may have been changed. Head, HeadLen, TotalLen, and
> > -  // info must be reloaded before use. The ownership of the packet
> > -  // is transferred to the packet process logic.
> > -  //
> > +    // Packet may have been changed. Head, HeadLen, TotalLen, and
> > +    // info must be reloaded before use. The ownership of the packet
> > +    // is transferred to the packet process logic.
> > +    //
> > +    if (!Ip4IsValidPacketLength (Packet)) {
> > +      goto RESTART;
> > +    }
> > +
> >      Head = (IP4_HEAD *) NetbufGetByte (Packet, 0, NULL);
> >      ASSERT (Head != NULL);
> >      Status = Ip4PreProcessPacket (
> >                 IpSb,
> >                 &Packet,
> >
> 
> 
>