Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow.

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

Also include NetworkPkg reviewer to collect the feedback for this change. 

Thanks
Liming
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, February 28, 2020 1:40 AM
> To: devel@edk2.groups.io; maciej.rabeda@linux.intel.com; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Andrew Fish <afish@apple.com>; Leif Lindholm (Linaro address) <leif.lindholm@linaro.org>
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Armour, Nicholas <nicholas.armour@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow.
> 
> On 02/27/20 14:14, Laszlo Ersek wrote:
> > (+Liming and stewards; CC Nick)
> >
> > On 02/27/20 12:02, Maciej Rabeda wrote:
> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2032
> >>
> >> 'ping' command's receive flow utilizes a single Rx token which it
> >> attempts to reuse before recycling the previously received packet.
> >> This causes a situation where under ICMP traffic,
> >> Ping6OnEchoReplyReceived() function will receive an already
> >> recycled packet with EFI_SUCCESS token status and finally
> >> dereference invalid pointers from RxData structure.
> >>
> >> Cc: Ray Ni <ray.ni@intel.com>
> >> Cc: Zhichao Gao <zhichao.gao@intel.com>
> >> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> >> ---
> >>  ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c | 9 +++++----
> >>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> >> index 23567fa2c1bb..a3fa32515192 100644
> >> --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> >> +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> >> @@ -614,6 +614,11 @@ Ping6OnEchoReplyReceived (
> >>
> >>  ON_EXIT:
> >>
> >> +  //
> >> +  // Recycle the packet before reusing RxToken
> >> +  //
> >> +  gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)-
> >RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal);
> >> +
> >>    if (Private->RxCount < Private->SendNum) {
> >>      //
> >>      // Continue to receive icmp echo reply packets.
> >> @@ -632,10 +637,6 @@ ON_EXIT:
> >>      //
> >>      Private->Status = EFI_SUCCESS;
> >>    }
> >> -  //
> >> -  // Singal to recycle the each rxdata here, not at the end of process.
> >> -  //
> >> -  gBS->SignalEvent (Private->IpChoice == PING_IP_CHOICE_IP6?((EFI_IP6_RECEIVE_DATA*)Private->RxToken.Packet.RxData)-
> >RecycleSignal:((EFI_IP4_RECEIVE_DATA*)Private->RxToken.Packet.RxData)->RecycleSignal);
> >>  }
> >>
> >>  /**
> >>
> >
> > (1) This patch proposes to fix one of the BZs (2032) that fall under
> > CVE-2019-14559 (joint tracker: 2550).
> >
> > Consequently:
> >
> > (1a) Do we want to include this in the upcoming stable tag?
> >
> > If so, we might want to extend the hard feature freeze by a few days.
> >
> > (1b) Please append the string " (CVE-2019-14559)" -- note the separating
> > space! -- to the subject line.
> >
> > (2) However: I remember from an earlier Bugzilla entry (can't tell
> > off-hand, which one, sorry) that ShellPkg issues are *never* considered
> > CVE-worthy, because the shell is not considered a "production element"
> > of the UEFI boot path.
> 
> I misremembered -- there is indeed a comment like that, in the TianoCore
> bugzilla, but it does not refer to ShellPkg. It refers to StdLib (which
> has since been split off to the edk2-libc project):
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1510#c1
> 
>     StdLib is supposed to be used only by applications in shell, all of
>     which are meant for debug, diagnosis and/or test purpose, not for
>     product UEFI BIOS. Any issue in it will not be taken as security
>     issue but just normal bug.
> 
> Sorry about causing confusion. So, the ShellPkg maintainers should
> decide what to do about this bug (keep it under the CVE scope vs.
> exclude it from the CVE scope; and then, propose it for the stable tag
> or merge it afterwards).
> 
> One data point: the bug appears to go back to the inception of the Ping
> command, in historical commit 68fb05272b45 ("Add Network1 profile.",
> 2011-03-25). It's not a new bug, it seems.
> 
> Thanks
> Laszlo
> 
> >
> > TianoCore#2032 was originally filed for NetworkPkg, and indeed that
> > seemed to justify the CVE assignment. However, now that Nick's and
> > Maciej's analysis shows that NetworkPkg is unaffected (and we know, per
> > above, that ShellPkg is not CVE-worthy), should we rather *remove* this
> > BZ from the CVE-2019-14559 umbrella?
> >
> > Because, in that case, modifying the subject line on the patch is not
> > necessary; and more importantly, we might not even want to put this into
> > edk2-stable202002. (It's still a bugfix, but may not be important enough.)
> >
> > Thanks!
> > Laszlo
> >