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

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

Maciej:
  Thanks for your analysis. I agree with you. This change is not necessary to catch edk2 stable tag 202002.

Thanks
Liming
> -----Original Message-----
> From: Rabeda, Maciej <maciej.rabeda@linux.intel.com>
> Sent: Friday, February 28, 2020 8:35 PM
> To: Gao, Liming <liming.gao@intel.com>; devel@edk2.groups.io; Laszlo Ersek <lersek@redhat.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>; Fu, Siyuan
> <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow.
> 
> Liming,
> 
> I assume that CVE-2019-14559 relates to problems with network drivers
> within NetworkPkg.
> BZ 2032 and this patch fix address incorrect usage of IP4 protocol by
> ShellPkg 'ping' command (signalling packet recycling after triggering
> Ip4->Receive() on the same Rx token).
> 
> This issue was reported in July 2019 and occurs by executing a specific
> fuzzing scenario (as described in BZ).
> As Laszlo has mentioned, it is not a new bug (introduced in 2011).
> 
> Based on the above, I can advise moving this issue out of CVE scope (and
> from stable-202002).
> However, if the CVE should be treated as "overall problems with UEFI FW
> networking", then 'ping' command seems to be in CVE scope, despite the
> code residing in ShellPkg.
> 
> Thanks,
> Maciej
> 
> On 28-Feb-20 12:50, Gao, Liming wrote:
> > Maciej:
> >    I see you submit the patch. So, you are in the loop. I don't invite you again. 😊
> >
> >    Yes. I also want to get your opinion for this change. Do you think whether this fix is in CVE scope? If no, this change will be merged
> after this stable tag 202002. Is it OK to you?
> >
> > Thanks
> > Liming
> >> -----Original Message-----
> >> From: Rabeda, Maciej <maciej.rabeda@linux.intel.com>
> >> Sent: Friday, February 28, 2020 7:42 PM
> >> To: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Laszlo Ersek <lersek@redhat.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>; Fu, Siyuan
> >> <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> >> Subject: Re: [edk2-devel] [PATCH v1] ShellPkg: Fix 'ping' command Ip4 receive flow.
> >>
> >> Laszlo,
> >>
> >> Thanks for the detailed response on the patch. Always happy to learn
> >> about stuff from the past.
> >>
> >> Liming,
> >>
> >> I am currently the maintainer of NetworkPkg :) If you require additional
> >> feedback from Siyuan or/and Jiaxin, that's ok.
> >> Please let me know if any corrections to the patch (like CVE note) are
> >> required from your point of view.
> >>
> >> Thanks,
> >> Maciej
> >>
> >> On 28-Feb-20 03:59, Liming Gao wrote:
> >>> 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
> >>>>>
> >>> 
> >>>