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

["Laszlo Ersek" <lersek@redhat.com>]

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
>