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

["Maciej Rabeda" <maciej.rabeda@linux.intel.com>]

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
>>>
>
> 
>