From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 26E5921168206 for ; Fri, 19 Oct 2018 04:25:02 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3539D31649BC; Fri, 19 Oct 2018 11:25:02 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-18.rdu2.redhat.com [10.10.121.18]) by smtp.corp.redhat.com (Postfix) with ESMTP id B190B6530E; Fri, 19 Oct 2018 11:25:00 +0000 (UTC) To: "Gao, Liming" , "Zeng, Star" , "edk2-devel@lists.01.org" , Ard Biesheuvel , "Holtsclaw, Brent" References: <1539655573-5456-1-git-send-email-liming.gao@intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E33D7CF@SHSMSX104.ccr.corp.intel.com> <155ff850-d652-30c9-5c02-7f1412a8f079@redhat.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E33DD17@SHSMSX104.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <860c2c8f-c026-5873-0405-131a40a133aa@redhat.com> Date: Fri, 19 Oct 2018 13:24:59 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E33DD17@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.41]); Fri, 19 Oct 2018 11:25:02 +0000 (UTC) Subject: Re: [Patch 0/3] Add more checker for Tianocompress and Ueficompress X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Oct 2018 11:25:03 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 ; Zeng, Star >> ; edk2-devel@lists.01.org; Ard Biesheuvel >> >> 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. >>> 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