Re: [Patch 0/3] Add more checker for Tianocompress and Ueficompress

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

Laszlo:

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, October 19, 2018 7:25 PM
> To: Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; Holtsclaw, Brent <brent.holtsclaw@intel.com>
> Subject: Re: [edk2] [Patch 0/3] Add more checker for Tianocompress and Ueficompress
> 
> On 10/19/18 08:40, Gao, Liming wrote:
> > Laszlo:
> >   I try to answer your question. I also include the BZ submitter
> >   brent.holtsclaw@intel.com. Holtsclaw, please add your comments if my
> >   info is not enough.
> >
> > Thanks
> > Liming
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Friday, October 19, 2018 12:01 AM
> >> To: Gao, Liming <liming.gao@intel.com>; Zeng, Star
> >> <star.zeng@intel.com>; edk2-devel@lists.01.org; Ard Biesheuvel
> >> <ard.biesheuvel@linaro.org>
> >> Subject: Re: [edk2] [Patch 0/3] Add more checker for Tianocompress
> >> and Ueficompress
> >>
> >> On 10/18/18 15:36, Gao, Liming wrote:
> >>> Laszlo and Star:
> >>>   Thank your notes. I will add CVE number in patch subject although
> >>>   it will make subject long than 80 characters.
> >>
> >> I agree the subject will be overlong, but I also think that including
> >> the CVE numbers is important enough for that.
> >>
> >>> Here is my proposed patch subject: CVE-2017-5731..5735 MdePkg: Add
> >>> more checker in UefiDecompressLib to access the valid buffer only.
> >>
> >> I suggest (based on tradition) that we keep the normal subject at the
> >> front, and then we append the CVE numbers at the end. Also, we should
> >> spell out all those CVE identifiers individually, if the same patch
> >> solves them all. It should be possible to search the subject line for
> >> any one of these CVE numbers in separation, using the official CVE
> >> number format.
> >>
> >
> > So, your proposal is like:  MdePkg: Add more checker in
> > UefiDecompressLib to access the valid buffer only CVE-2017-5731
> > CVE-2017-5732 CVE-2017-5733 CVE-2017-5734 CVE-2017-5735
> 
> Yes:
> 
>   MdePkg: Add more checker in UefiDecompressLib to access the valid buffer only (CVE-2017-5731 CVE-2017-5732 CVE-2017-5733
> CVE-2017-5734 CVE-2017-5735)
> 
> It looks terrible, but the real subject is still readable to the left,
> and subjects with searchable CVE numbers take priority (in my opinion
> anyway).
> 
> Actually: I wonder why we needed five different CVEs, if they can all be
> fixed with a small, single patch.
> 
> More precisely: looking at the patch in more detail, I see that the
> patch fixes multiple functions / separate buffer overflows. Is it
> possible to associate each CVE with a specific, small code change in the
> patch? Because if it is possible, then I think we should split the patch
> *per CVE*. The subjects would go:
> 
> - MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE-2017-5731)
> - MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE-2017-5732)
> - MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE-2017-5733)
> - MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE-2017-5734)
> - MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE-2017-5735)
> 
> (71 characters, in each subject)
> 
> If such separation is technically possible, then I think it would be an
> improvement; minimally for documentation purposes.
> 

I don't find the detail information for each CVE. BZ 686 attaches one doc to list all issues. So, I fix them together. I think one patch is allowed to include more than one CVEs. Even if with single CVE, patch subject may be longer than 80 characters. If we need strictly follow subject length rule, I suggest to mention CVE FIX in subject, and list CVE number info in the commit message. User can use git command to get full commit log and know which commit is CVE fix. For example:
MdePkg/UefiDecompressLib: fix potential buffer overflow (CVE FIX)

> >>>   In PEI phase, the recovery image is from the external device. If
> >>>   the recovery image has the corrupt EFI compression section, they
> >>>   will be handled by EFI Decompression PPI.
> >>
> >> In the PEI phase, if the recovery image is crafted, it could cause a
> >> buffer overflow during decompression. However, if the recovery image
> >> is crafted, it might as well decompress cleanly, and once it is
> >> dispatched, do "bad things". Do the decompression and the dispatch
> >> occur at different privilege levels?
> >>
> >
> > This patch focuses on the wrong decompression data that cause the
> > decompression failure or hang.  The data content can be signed and
> > verified.
> >
> >>> In DXE phase, UEFI option ROM is the third party code. If it is EFI
> >>> compression option ROM, EFI decompression protocol will be used to
> >>> decode its data. I don't think SMM uses EFI decompression protocol.
> >>> UefiDecompressionLib is used as EFI compression PPI/Protocol. It
> >>> matches PI EFI compression section instead of GUID section. So, it
> >>> has no GUID extraction PPI/Protocol.
> >>
> >> In the DXE phase, if the option ROM is crafted, it could cause a
> >> buffer overflow when it is decompressed. But, again, how is that
> >> different from when a crafted oprom decompresses cleanly, and then
> >> does "bad things" when it is dispatched?
> >>
> >> Here (in the DXE phase), I can imagine two answers myself:
> >>
> >> (1) Decompression occurs before Secure Boot validation, but dispatch
> >> occurs only after. Therefore a crafted UEFI image could cause
> >> problems via decompression even if it would fail SB verification
> >> later.
> >>
> >> (2) Decompression of UEFI option ROMs occurs before PlatformBDS locks
> >> down SMRAM and lockboxes. However, the execution of UEFI option ROMs
> >> is deferred until after the lockdown.
> >>
> >> Do these scenarios apply? Because, if they do, I agree the issue
> >> qualifies as privilege escalation.
> >>
> >
> > Yes. Decompression happen early. After decompression, PE image will be
> > verified.
> 
> Got it now. Thanks!
> Laszlo