HI Wenyi
Thank you very much to take time to reproduce.
I am particular interested in below:
"As PE file is modified, function PeCoffLoaderGetImageInfo will return
error, so I have to remove it so that for loop can be tested in
DxeImageVerificationHandler."
By design, the PeCoffLib should catch illegal PE/COFF image and return error.
(even it cannot catch all, it should catch most ones).
Other PE/COFF parser may rely on the checker in PeCoffLib and *no need* to
duplicate all checkers.
As such, DxeImageVerificationLib (and other PeCoff consumer) just need checks
what has passed the check in PeCoffLib.
I don’t think you should remove the checker. If people can remove the checker,
I am sure the rest code will be vulnerable, according to the current design.
Could you please to create a case that bypass all checks in PeCoffLib, then run
into DxeImageVerificationLib and cause the problem? That would be more valuable
for us.
Thank you
Yao Jiewen
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of wenyi,xie
> via groups.io
> Sent: Friday, August 28, 2020 11:18 AM
> To: Laszlo Ersek <ler...@redhat.com>; Wang, Jian J <jian.j.w...@intel.com>;
> devel@edk2.groups.io; Yao, Jiewen <jiewen....@intel.com>
> Cc: songdongku...@huawei.com; Mathews, John <john.math...@intel.com>
> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1]
> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
>
> Hi，Laszlo and everyone，
>
> These days I tried to reproduce the issue，and made some progress. I think
> there are two way to cause overflow from a mathematical point of view,
> 1.As Laszlo analysed before, if WinCertificate->dwLength is large enough,
> close
> to MAX_UINT32, then (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate-
> >dwLength)) will cause overflow.
> 2.(WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength)) is good,
> OffSet is good, but OffSet += (WinCertificate->dwLength + ALIGN_SIZE
> (WinCertificate->dwLength)) cause overflow.
>
> Here I choose the second way to reproduce the issue, I choose a PE file and
> sign
> it with my own db certificate. Then I use binary edit tool to modify the PE
> file like
> below,
>
> 1.change SecDataDir->Size from 0x5F8 to 0xFFFF1FFF
> 2.change WinCertificate->dwLength from 0x5F1 to 0xFFFF1FFE
> SecDataDir->VirtualAddress in my PE is 0xe000 and no need to change.
>
> As PE file is modified, function PeCoffLoaderGetImageInfo will return error,
> so I
> have to remove it so that for loop can be tested in
> DxeImageVerificationHandler.
> The log is as below,
>
> SecDataDir->VirtualAddress=0xE000, SecDataDir->Size=0xFFFF1FFF.
> (First Loop)
> OffSet=0xE000.
> WinCertificate->dwLength=0xFFFF1FFE, ALIGN_SIZE (WinCertificate-
> >dwLength)=0x2.
> (Second Loop)
> OffSet=0x0.
> WinCertificate->dwLength=0x5A4D, ALIGN_SIZE (WinCertificate-
> >dwLength)=0x3.
> (Third Loop)
> OffSet=0x5A50.
> WinCertificate->dwLength=0x9024, ALIGN_SIZE (WinCertificate-
> >dwLength)=0x4.
> (Forth Loop)
> OffSet=0xEA78.
> WinCertificate->dwLength=0xAFAFAFAF, ALIGN_SIZE (WinCertificate-
> >dwLength)=0x1.
> (Fifth Loop)
> OffSet=0xAFB09A28.
>
> As I modify SecDataDir->Size and WinCertificate->dwLength, so in first loop
> the
> condition check whether the Security Data Directory has enough room left for
> "WinCertificate->dwLength" is ok.
>
> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof
> (WIN_CERTIFICATE) ||
> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < WinCertificate-
> >dwLength) {
> break;
> }
>
> In the beginning of second loop, WinCertificate->dwLength + ALIGN_SIZE
> (WinCertificate->dwLength) is 0xFFFF2000, offset is 0xE000
>
> OffSet += (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength))
>
> Offset now is 0 and overflow happens. So even if my PE only have one
> signature,
> the for loop is still going untill exception happens.
>
>
> I can't reproduce the issue using the first way, because if WinCertificate-
> >dwLength is close to MAX_UINT32, it means SecDataDir->Size should also close
> to MAX_UINT32, or the condition check
> "(SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < WinCertificate-
> >dwLength" will break. But if SecDataDir->Size is very large, SecDataDir-
> >VirtualAddress have to be smaller than 8 bytes,
> because SecDataDir->VirtualAddress + SecDataDir->Size < MAX_UINT32.
> SecDataDir->VirtualAddress is the beginning address of the signature, and
> before
> SecDataDir->VirtualAddress is the content of origin PE file, I think it's
> impossible
> that the size of PE file is only 8 bytes.
>
> As I changed the one line code in DxeImageVerificationHandler to reproduce the
> issue, I'm not sure whether it's ok.
>
> Thanks
> Wenyi
>
> On 2020/8/19 17:26, Laszlo Ersek wrote:
> > On 08/18/20 17:18, Mathews, John wrote:
> >> I dug up the original report details. This was noted as a concern during a
> source code inspection. There was no demonstration of how it might be
> triggered.
> >>
> >> " There is an integer overflow vulnerability in the
> DxeImageVerificationHandler function when
> >> parsing the PE files attribute certificate table. In cases where
> >> WinCertificate-
> >dwLength is
> >> sufficiently large, it's possible to overflow Offset back to 0 causing an
> >> endless
> loop."
> >>
> >> The recommendation was to add stricter checking of "Offset" and the
> embedded length fields of certificate data
> >> before using them.
> >
> > Thanks for checking!
> >
> > Laszlo
> >
> >>
> >>
> >>
> >> -----Original Message-----
> >> From: Laszlo Ersek <ler...@redhat.com>
> >> Sent: Tuesday, August 18, 2020 1:59 AM
> >> To: Wang, Jian J <jian.j.w...@intel.com>; devel@edk2.groups.io; Yao,
> Jiewen <jiewen....@intel.com>; xiewen...@huawei.com
> >> Cc: huangmin...@huawei.com; songdongku...@huawei.com; Mathews,
> John <john.math...@intel.com>
> >> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1]
> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
> >>
> >> On 08/18/20 04:10, Wang, Jian J wrote:
> >>> Laszlo,
> >>>
> >>> My apologies for the slow response. I'm not the original reporter but
> >>> just the BZ submitter. And I didn't do deep analysis to this issue.
> >>> The issues was reported from one internal team. Add John in loop to see if
> he knows more about it or not.
> >>>
> >>> My superficial understanding on such issue is that, if there's
> >>> "potential" issue in theory and hard to reproduce, it's still worthy
> >>> of using an alternative way to replace the original implementation
> >>> with no "potential" issue at all. Maybe we don't have to prove old way is
> something wrong but must prove that the new way is really safe.
> >>
> >> I agree, thanks.
> >>
> >> It would be nice to hear more from the internal team about the originally
> reported (even if hard-to-trigger) issue.
> >>
> >> Thanks!
> >> Laszlo
> >>
> >>>
> >>> Regards,
> >>> Jian
> >>>
> >>>> -----Original Message-----
> >>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> >>>> Ersek
> >>>> Sent: Tuesday, August 18, 2020 12:53 AM
> >>>> To: Yao, Jiewen <jiewen....@intel.com>; devel@edk2.groups.io;
> >>>> xiewen...@huawei.com; Wang, Jian J <jian.j.w...@intel.com>
> >>>> Cc: huangmin...@huawei.com; songdongku...@huawei.com
> >>>> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1]
> >>>> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
> >>>>
> >>>> Hi Jiewen,
> >>>>
> >>>> On 08/14/20 10:53, Yao, Jiewen wrote:
> >>>>>> To Jiewen,
> >>>>>> Sorry, I don't have environment to reproduce the issue.
> >>>>>
> >>>>> Please help me understand, if you don’t have environment to
> >>>>> reproduce the
> >>>> issue, how do you guarantee that your patch does fix the problem and
> >>>> we don’t have any other vulnerabilities?
> >>>>
> >>>> The original bug report in
> >>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=2215#c0> is seriously
> >>>> lacking. It does not go into detail about the alleged integer overflow.
> >>>> It does not quote the code, does not explain the control flow, does
> >>>> not identify the exact edk2 commit at which the vulnerability exists.
> >>>>
> >>>> The bug report also does not offer a reproducer.
> >>>>
> >>>> Additionally, the exact statement that the bug report does make,
> >>>> namely
> >>>>
> >>>> it's possible to overflow Offset back to 0 causing an endless loop
> >>>>
> >>>> is wrong (as far as I can tell anyway). It is not "OffSet" that can
> >>>> be overflowed to zero, but the *addend* that is added to OffSet can
> >>>> be overflowed to zero. Therefore the infinite loop will arise because
> >>>> OffSet remains stuck at its present value, and not because OffSet
> >>>> will be re-set to zero.
> >>>>
> >>>> For the reasons, we can only speculate as to what the actual problem
> >>>> is, unless Jian decides to join the discussion and clarifies what he
> >>>> had in mind originally.
> >>>>
> >>>> My understanding (or even "reconstruction") of the vulnerability is
> >>>> described above, and in the patches that I proposed.
> >>>>
> >>>> We can write a patch based on code analysis. It's possible to
> >>>> identify integer overflows based on code analysis, and it's possible
> >>>> to verify the correctness of fixes by code review. Obviously testing
> >>>> is always good, but many times, constructing reproducers for such
> >>>> issues that were found by code review, is difficult and time
> >>>> consuming. We can say that we don't fix vulnerabilities without
> >>>> reproducers, or we can say that we make an effort to fix them even if
> >>>> all we have is code analysis (and not a reproducer).
> >>>>
> >>>> So the above paragraph concerns "correctness". Regarding
> >>>> "completeness", I guarantee you that this patch does not fix *all*
> >>>> problems related to PE parsing. (See the other BZ tickets.) It does
> >>>> fix *one* issue with PE parsing. We can say that we try to fix such
> >>>> issues gradually (give different CVE numbers to different issues, and
> >>>> address them one at a time), or we can say that we rewrite PE parsing
> from the ground up.
> >>>> (BTW: I have seriously attempted that in the past, and I gave up,
> >>>> because the PE format is FUBAR.)
> >>>>
> >>>> In summary:
> >>>>
> >>>> - the problem statement is unclear,
> >>>>
> >>>> - it seems like there is indeed an integer overflow problem in the
> >>>> SecDataDir parsing loop, but it's uncertain whether the bug reporter
> >>>> had exactly that in mind
> >>>>
> >>>> - PE parsing is guaranteed to have other vulnerabilities elsewhere in
> >>>> edk2, but I'm currently unaware of other such issues in
> >>>> DxeImageVerificationLib specifically
> >>>>
> >>>> - even if there are other such problems (in DxeImageVerificationLib
> >>>> or elswehere), fixing this bug that we know about is likely
> >>>> worthwhile
> >>>>
> >>>> - for many such bugs, constructing a reproducer is difficult and time
> >>>> consuming; code analysis, and *regression-testing* are frequently the
> >>>> only tools we have. That doesn't mean we should ignore this class of
> >>>> bugs.
> >>>>
> >>>> (Fixing integer overflows retro-actively is more difficult than
> >>>> writing overflow-free code in the first place, but that ship has
> >>>> sailed; so we can only fight these bugs incrementally now, unless we
> >>>> can rewrite PE parsing with a new data structure from the ground up.
> >>>> Again I tried that and gave up, because the spec is not public, and
> >>>> what I did manage to learn about PE, showed that it was insanely
> >>>> over-engineered. I'm not saying that other binary / executable
> >>>> formats are better, of course.)
> >>>>
> >>>> Please check out my patches (inlined elsewhere in this thread), and
> >>>> comment whether you'd like me to post them to the list as a
> >>>> standalone series.
> >>>>
> >>>> Jian: it wouldn't hurt if you commented as well.
> >>>>
> >>>> Thanks
> >>>> Laszlo
> >>>>
> >>>>>> -----Original Message-----
> >>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> >>>> wenyi,xie
> >>>>>> via groups.io
> >>>>>> Sent: Friday, August 14, 2020 3:54 PM
> >>>>>> To: Laszlo Ersek <ler...@redhat.com>; devel@edk2.groups.io; Yao,
> >>>>>> Jiewen <jiewen....@intel.com>; Wang, Jian J <jian.j.w...@intel.com>
> >>>>>> Cc: huangmin...@huawei.com; songdongku...@huawei.com
> >>>>>> Subject: Re: [edk2-devel] [PATCH EDK2 v2 1/1]
> >>>>>> SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset
> >>>>>>
> >>>>>> To Laszlo,
> >>>>>> Thank you for your detailed description, I agree with what you
> >>>>>> analyzed and
> >>>> I'm
> >>>>>> OK with your patches, it's
> >>>>>> correct and much simpler.
> >>>>>>
> >>>>>> To Jiewen,
> >>>>>> Sorry, I don't have environment to reproduce the issue.
> >>>>>>
> >>>>>> Thanks
> >>>>>> Wenyi
> >>>>>>
> >>>>>> On 2020/8/14 2:50, Laszlo Ersek wrote:
> >>>>>>> On 08/13/20 13:55, Wenyi Xie wrote:
> >>>>>>>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2215
> >>>>>>>>
> >>>>>>>> There is an integer overflow vulnerability in
> >>>>>>>> DxeImageVerificationHandler function when parsing the PE files
> >>>>>>>> attribute certificate table. In cases where
> >>>>>>>> WinCertificate->dwLength is sufficiently large, it's possible to
> overflow Offset back to 0 causing an endless loop.
> >>>>>>>>
> >>>>>>>> Check offset inbetween VirtualAddress and VirtualAddress + Size.
> >>>>>>>> Using SafeintLib to do offset addition with result check.
> >>>>>>>>
> >>>>>>>> Cc: Jiewen Yao <jiewen....@intel.com>
> >>>>>>>> Cc: Jian J Wang <jian.j.w...@intel.com>
> >>>>>>>> Cc: Laszlo Ersek <ler...@redhat.com>
> >>>>>>>> Signed-off-by: Wenyi Xie <xiewen...@huawei.com>
> >>>>>>>> ---
> >>>>>>>>
> >>>>>>>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.inf
> >>>> |
> >>>>>> 1 +
> >>>>>>>>
> >>>>>>>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.h
> >>>> |
> >>>>>> 1 +
> >>>>>>>>
> >>>>>>>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.c
> >>>> |
> >>>>>> 111 +++++++++++---------
> >>>>>>>> 3 files changed, 63 insertions(+), 50 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git
> >>>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.inf
> >>>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.inf
> >>>>>>>> index 1e1a639857e0..a7ac4830b3d4 100644
> >>>>>>>> ---
> >>>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.inf
> >>>>>>>> +++
> >>>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.inf
> >>>>>>>> @@ -53,6 +53,7 @@ [LibraryClasses]
> >>>>>>>> SecurityManagementLib
> >>>>>>>> PeCoffLib
> >>>>>>>> TpmMeasurementLib
> >>>>>>>> + SafeIntLib
> >>>>>>>>
> >>>>>>>> [Protocols]
> >>>>>>>> gEfiFirmwareVolume2ProtocolGuid ## SOMETIMES_CONSUMES
> >>>>>>>> diff --git
> >>>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.h
> >>>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.h
> >>>>>>>> index 17955ff9774c..060273917d5d 100644
> >>>>>>>> ---
> >>>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.h
> >>>>>>>> +++
> >>>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.h
> >>>>>>>> @@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >>>>>>>> #include <Library/DevicePathLib.h> #include
> >>>>>>>> <Library/SecurityManagementLib.h> #include <Library/PeCoffLib.h>
> >>>>>>>> +#include <Library/SafeIntLib.h>
> >>>>>>>> #include <Protocol/FirmwareVolume2.h> #include
> >>>>>>>> <Protocol/DevicePath.h> #include <Protocol/BlockIo.h> diff --git
> >>>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.c
> >>>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.c
> >>>>>>>> index 36b87e16d53d..dbc03e28c05b 100644
> >>>>>>>> ---
> >>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib
> >>>> .c
> >>>>>>>> +++
> >>>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.c
> >>>>>>>> @@ -1658,6 +1658,10 @@ DxeImageVerificationHandler (
> >>>>>>>> EFI_STATUS HashStatus;
> >>>>>>>> EFI_STATUS DbStatus;
> >>>>>>>> BOOLEAN IsFound;
> >>>>>>>> + UINT32 AlignedLength;
> >>>>>>>> + UINT32 Result;
> >>>>>>>> + EFI_STATUS AddStatus;
> >>>>>>>> + BOOLEAN IsAuthDataAssigned;
> >>>>>>>>
> >>>>>>>> SignatureList = NULL;
> >>>>>>>> SignatureListSize = 0;
> >>>>>>>> @@ -1667,6 +1671,7 @@ DxeImageVerificationHandler (
> >>>>>>>> Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
> >>>>>>>> IsVerified = FALSE;
> >>>>>>>> IsFound = FALSE;
> >>>>>>>> + Result = 0;
> >>>>>>>>
> >>>>>>>> //
> >>>>>>>> // Check the image type and get policy setting.
> >>>>>>>> @@ -1850,9 +1855,10 @@ DxeImageVerificationHandler (
> >>>>>>>> // The first certificate starts at offset
> >>>>>>>> (SecDataDir->VirtualAddress) from
> >>>> the
> >>>>>> start of the file.
> >>>>>>>> //
> >>>>>>>> for (OffSet = SecDataDir->VirtualAddress;
> >>>>>>>> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
> >>>>>>>> - OffSet += (WinCertificate->dwLength + ALIGN_SIZE
> (WinCertificate-
> >>>>>>> dwLength))) {
> >>>>>>>> + (OffSet >= SecDataDir->VirtualAddress) && (OffSet <
> >>>>>>>> + (SecDataDir-
> >>>>>>> VirtualAddress + SecDataDir->Size));) {
> >>>>>>>> + IsAuthDataAssigned = FALSE;
> >>>>>>>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> >>>>>>>> + AlignedLength = WinCertificate->dwLength + ALIGN_SIZE
> >>>> (WinCertificate-
> >>>>>>> dwLength);
> >>>>>>>
> >>>>>>> I disagree with this patch.
> >>>>>>>
> >>>>>>> The primary reason for my disagreement is that the bug report
> >>>>>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=2215#c0> is
> >>>>>>> inexact, and so this patch tries to fix the wrong thing.
> >>>>>>>
> >>>>>>> With edk2 master at commit 65904cdbb33c, it is *not* possible to
> >>>>>>> overflow the OffSet variable to zero with "WinCertificate->dwLength"
> >>>>>>> *purely*, and cause an endless loop. Note that we have (at commit
> >>>>>>> 65904cdbb33c):
> >>>>>>>
> >>>>>>> for (OffSet = SecDataDir->VirtualAddress;
> >>>>>>> OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
> >>>>>>> OffSet += (WinCertificate->dwLength + ALIGN_SIZE
> >>>>>>> (WinCertificate-
> >>>>>>> dwLength))) {
> >>>>>>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> >>>>>>> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet)
> >>>>>>> <= sizeof
> >>>>>> (WIN_CERTIFICATE) ||
> >>>>>>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <
> >>>> WinCertificate-
> >>>>>>> dwLength) {
> >>>>>>> break;
> >>>>>>> }
> >>>>>>>
> >>>>>>> The last sub-condition checks whether the Security Data Directory
> >>>>>>> has enough room left for "WinCertificate->dwLength". If not, then
> >>>>>>> we break out of the loop.
> >>>>>>>
> >>>>>>> If we *do* have enough room, that is:
> >>>>>>>
> >>>>>>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) >=
> >>>> WinCertificate-
> >>>>>>> dwLength
> >>>>>>>
> >>>>>>> then we have (by adding OffSet to both sides):
> >>>>>>>
> >>>>>>> SecDataDir->VirtualAddress + SecDataDir->Size >= OffSet +
> >>>>>>> WinCertificate- dwLength
> >>>>>>>
> >>>>>>> The left hand side is a known-good UINT32, and so incrementing
> >>>>>>> OffSet (a
> >>>>>>> UINT32) *solely* by "WinCertificate->dwLength" (also a UINT32)
> >>>>>>> does not cause an overflow.
> >>>>>>>
> >>>>>>> Instead, the problem is with the alignment. The "if" statement
> >>>>>>> checks whether we have enough room for "dwLength", but then
> >>>>>>> "OffSet" is advanced by "dwLength" *aligned up* to the next
> >>>>>>> multiple of 8. And that may indeed cause various overflows.
> >>>>>>>
> >>>>>>> Now, the main problem with the present patch is that it does not
> >>>>>>> fix one of those overflows. Namely, consider that "dwLength" is
> >>>>>>> very close to
> >>>>>>> MAX_UINT32 (or even think it's exactly MAX_UINT32). Then aligning
> >>>>>>> it up to the next multiple of 8 will yield 0. In other words,
> "AlignedLength"
> >>>>>>> will be zero.
> >>>>>>>
> >>>>>>> And when that happens, there's going to be an infinite loop just
> >>>>>>> the
> >>>>>>> same: "OffSet" will not be zero, but it will be *stuck*. The
> >>>>>>> SafeUint32Add() call at the bottom will succeed, but it will not
> >>>>>>> change the value of "OffSet".
> >>>>>>>
> >>>>>>> More at the bottom.
> >>>>>>>
> >>>>>>>
> >>>>>>>> if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet)
> >>>>>>>> <= sizeof
> >>>>>> (WIN_CERTIFICATE) ||
> >>>>>>>> (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet)
> >>>>>>>> <
> >>>>>> WinCertificate->dwLength) {
> >>>>>>>> break;
> >>>>>>>> @@ -1872,6 +1878,8 @@ DxeImageVerificationHandler (
> >>>>>>>> }
> >>>>>>>> AuthData = PkcsCertData->CertData;
> >>>>>>>> AuthDataSize = PkcsCertData->Hdr.dwLength -
> >>>>>>>> sizeof(PkcsCertData-
> >>>>> Hdr);
> >>>>>>>> + IsAuthDataAssigned = TRUE;
> >>>>>>>> + HashStatus = HashPeImageByType (AuthData, AuthDataSize);
> >>>>>>>> } else if (WinCertificate->wCertificateType ==
> >>>> WIN_CERT_TYPE_EFI_GUID)
> >>>>>> {
> >>>>>>>> //
> >>>>>>>> // The certificate is formatted as
> >>>>>>>> WIN_CERTIFICATE_UEFI_GUID which
> >>>> is
> >>>>>> described in UEFI Spec.
> >>>>>>>> @@ -1880,72 +1888,75 @@ DxeImageVerificationHandler (
> >>>>>>>> if (WinCertUefiGuid->Hdr.dwLength <=
> >>>>>> OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData)) {
> >>>>>>>> break;
> >>>>>>>> }
> >>>>>>>> - if (!CompareGuid (&WinCertUefiGuid->CertType,
> &gEfiCertPkcs7Guid))
> >>>> {
> >>>>>>>> - continue;
> >>>>>>>> + if (CompareGuid (&WinCertUefiGuid->CertType,
> >>>>>>>> + &gEfiCertPkcs7Guid))
> >>>> {
> >>>>>>>> + AuthData = WinCertUefiGuid->CertData;
> >>>>>>>> + AuthDataSize = WinCertUefiGuid->Hdr.dwLength -
> >>>>>> OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData);
> >>>>>>>> + IsAuthDataAssigned = TRUE;
> >>>>>>>> + HashStatus = HashPeImageByType (AuthData, AuthDataSize);
> >>>>>>>> }
> >>>>>>>> - AuthData = WinCertUefiGuid->CertData;
> >>>>>>>> - AuthDataSize = WinCertUefiGuid->Hdr.dwLength -
> >>>>>> OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData);
> >>>>>>>> } else {
> >>>>>>>> if (WinCertificate->dwLength < sizeof (WIN_CERTIFICATE)) {
> >>>>>>>> break;
> >>>>>>>> }
> >>>>>>>> - continue;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> - HashStatus = HashPeImageByType (AuthData, AuthDataSize);
> >>>>>>>> - if (EFI_ERROR (HashStatus)) {
> >>>>>>>> - continue;
> >>>>>>>> - }
> >>>>>>>> -
> >>>>>>>> - //
> >>>>>>>> - // Check the digital signature against the revoked certificate
> >>>>>>>> in
> >>>> forbidden
> >>>>>> database (dbx).
> >>>>>>>> - //
> >>>>>>>> - if (IsForbiddenByDbx (AuthData, AuthDataSize)) {
> >>>>>>>> - Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED;
> >>>>>>>> - IsVerified = FALSE;
> >>>>>>>> - break;
> >>>>>>>> - }
> >>>>>>>> -
> >>>>>>>> - //
> >>>>>>>> - // Check the digital signature against the valid certificate in
> allowed
> >>>>>> database (db).
> >>>>>>>> - //
> >>>>>>>> - if (!IsVerified) {
> >>>>>>>> - if (IsAllowedByDb (AuthData, AuthDataSize)) {
> >>>>>>>> - IsVerified = TRUE;
> >>>>>>>> + if (IsAuthDataAssigned && !EFI_ERROR (HashStatus)) {
> >>>>>>>> + //
> >>>>>>>> + // Check the digital signature against the revoked
> >>>>>>>> + certificate in
> >>>> forbidden
> >>>>>> database (dbx).
> >>>>>>>> + //
> >>>>>>>> + if (IsForbiddenByDbx (AuthData, AuthDataSize)) {
> >>>>>>>> + Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED;
> >>>>>>>> + IsVerified = FALSE;
> >>>>>>>> + break;
> >>>>>>>> }
> >>>>>>>> - }
> >>>>>>>>
> >>>>>>>> - //
> >>>>>>>> - // Check the image's hash value.
> >>>>>>>> - //
> >>>>>>>> - DbStatus = IsSignatureFoundInDatabase (
> >>>>>>>> - EFI_IMAGE_SECURITY_DATABASE1,
> >>>>>>>> - mImageDigest,
> >>>>>>>> - &mCertType,
> >>>>>>>> - mImageDigestSize,
> >>>>>>>> - &IsFound
> >>>>>>>> - );
> >>>>>>>> - if (EFI_ERROR (DbStatus) || IsFound) {
> >>>>>>>> - Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND;
> >>>>>>>> - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
> signed
> >>>> but %s
> >>>>>> hash of image is found in DBX.\n", mHashTypeStr));
> >>>>>>>> - IsVerified = FALSE;
> >>>>>>>> - break;
> >>>>>>>> - }
> >>>>>>>> + //
> >>>>>>>> + // Check the digital signature against the valid
> >>>>>>>> + certificate in allowed
> >>>>>> database (db).
> >>>>>>>> + //
> >>>>>>>> + if (!IsVerified) {
> >>>>>>>> + if (IsAllowedByDb (AuthData, AuthDataSize)) {
> >>>>>>>> + IsVerified = TRUE;
> >>>>>>>> + }
> >>>>>>>> + }
> >>>>>>>>
> >>>>>>>> - if (!IsVerified) {
> >>>>>>>> + //
> >>>>>>>> + // Check the image's hash value.
> >>>>>>>> + //
> >>>>>>>> DbStatus = IsSignatureFoundInDatabase (
> >>>>>>>> - EFI_IMAGE_SECURITY_DATABASE,
> >>>>>>>> + EFI_IMAGE_SECURITY_DATABASE1,
> >>>>>>>> mImageDigest,
> >>>>>>>> &mCertType,
> >>>>>>>> mImageDigestSize,
> >>>>>>>> &IsFound
> >>>>>>>> );
> >>>>>>>> - if (!EFI_ERROR (DbStatus) && IsFound) {
> >>>>>>>> - IsVerified = TRUE;
> >>>>>>>> - } else {
> >>>>>>>> - DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
> signed
> >>>> but
> >>>>>> signature is not allowed by DB and %s hash of image is not found in
> >>>> DB/DBX.\n",
> >>>>>> mHashTypeStr));
> >>>>>>>> + if (EFI_ERROR (DbStatus) || IsFound) {
> >>>>>>>> + Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND;
> >>>>>>>> + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
> >>>>>>>> + signed
> >>>>>> but %s hash of image is found in DBX.\n", mHashTypeStr));
> >>>>>>>> + IsVerified = FALSE;
> >>>>>>>> + break;
> >>>>>>>> }
> >>>>>>>> +
> >>>>>>>> + if (!IsVerified) {
> >>>>>>>> + DbStatus = IsSignatureFoundInDatabase (
> >>>>>>>> + EFI_IMAGE_SECURITY_DATABASE,
> >>>>>>>> + mImageDigest,
> >>>>>>>> + &mCertType,
> >>>>>>>> + mImageDigestSize,
> >>>>>>>> + &IsFound
> >>>>>>>> + );
> >>>>>>>> + if (!EFI_ERROR (DbStatus) && IsFound) {
> >>>>>>>> + IsVerified = TRUE;
> >>>>>>>> + } else {
> >>>>>>>> + DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is
> >>>>>>>> + signed
> >>>> but
> >>>>>> signature is not allowed by DB and %s hash of image is not found in
> >>>> DB/DBX.\n",
> >>>>>> mHashTypeStr));
> >>>>>>>> + }
> >>>>>>>> + }
> >>>>>>>> + }
> >>>>>>>> +
> >>>>>>>> + AddStatus = SafeUint32Add (OffSet, AlignedLength, &Result);
> >>>>>>>> + if (EFI_ERROR (AddStatus)) {
> >>>>>>>> + break;
> >>>>>>>> }
> >>>>>>>> + OffSet = Result;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size))
> >>>>>>>> {
> >>>>>>>>
> >>>>>>>
> >>>>>>> There are other (smaller) reasons why I dislike this patch:
> >>>>>>>
> >>>>>>> - The "IsAuthDataAssigned" variable is superfluous; we could use
> >>>>>>> the existent "AuthData" variable (with a NULL-check and a
> >>>>>>> NULL-assignment) similarly.
> >>>>>>>
> >>>>>>> - The patch complicates / reorganizes the control flow needlessly.
> >>>>>>> This complication originates from placing the checked "OffSet"
> >>>>>>> increment at the bottom of the loop, which then requires the
> >>>>>>> removal of all the "continue" statements. But we don't need to
> >>>>>>> check-and-increment at the bottom. We can keep the increment
> >>>>>>> inside the "for" statement, only extend the *existent* room check
> >>>>>>> (which I've quoted) to take the alignment into account as well. If
> >>>>>>> there is enough room for the alignment in the security data
> >>>>>>> directory, then that guarantees there won't be a UINT32 overflow
> either.
> >>>>>>>
> >>>>>>> All in all, I'm proposing the following three patches instead. The
> >>>>>>> first two patches are preparation, the last patch is the fix.
> >>>>>>>
> >>>>>>> Patch#1:
> >>>>>>>
> >>>>>>>> From 11af0a104d34d39bf1b1aab256428ae4edbddd77 Mon Sep 17
> >>>> 00:00:00
> >>>>>> 2001
> >>>>>>>> From: Laszlo Ersek <ler...@redhat.com>
> >>>>>>>> Date: Thu, 13 Aug 2020 19:11:39 +0200
> >>>>>>>> Subject: [PATCH 1/3] SecurityPkg/DxeImageVerificationLib: extract
> >>>>>>>> SecDataDirEnd, SecDataDirLeft
> >>>>>>>>
> >>>>>>>> The following two quantities:
> >>>>>>>>
> >>>>>>>> SecDataDir->VirtualAddress + SecDataDir->Size
> >>>>>>>> SecDataDir->VirtualAddress + SecDataDir->Size - OffSet
> >>>>>>>>
> >>>>>>>> are used multiple times in DxeImageVerificationHandler().
> >>>>>>>> Introduce helper variables for them: "SecDataDirEnd" and
> "SecDataDirLeft", respectively.
> >>>>>>>> This saves us multiple calculations and significantly simplifies the
> >>>>>>>> code.
> >>>>>>>>
> >>>>>>>> Note that all three summands above have type UINT32, therefore
> >>>>>>>> the new variables are also of type UINT32.
> >>>>>>>>
> >>>>>>>> This patch does not change behavior.
> >>>>>>>>
> >>>>>>>> (Note that the code already handles the case when the
> >>>>>>>>
> >>>>>>>> SecDataDir->VirtualAddress + SecDataDir->Size
> >>>>>>>>
> >>>>>>>> UINT32 addition overflows -- namely, in that case, the
> >>>>>>>> certificate loop is never entered, and the corruption check right
> >>>>>>>> after the loop fires.)
> >>>>>>>>
> >>>>>>>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> >>>>>>>> ---
> >>>>>>>>
> >>>>>>>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.c |
> >>>> 12
> >>>>>> ++++++++----
> >>>>>>>> 1 file changed, 8 insertions(+), 4 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git
> >>>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.c
> >>>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.c
> >>>>>>>> index 36b87e16d53d..8761980c88aa 100644
> >>>>>>>> ---
> >>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib
> >>>> .c
> >>>>>>>> +++
> >>>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.c
> >>>>>>>> @@ -1652,6 +1652,8 @@ DxeImageVerificationHandler (
> >>>>>>>> UINT8 *AuthData;
> >>>>>>>> UINTN AuthDataSize;
> >>>>>>>> EFI_IMAGE_DATA_DIRECTORY *SecDataDir;
> >>>>>>>> + UINT32 SecDataDirEnd;
> >>>>>>>> + UINT32 SecDataDirLeft;
> >>>>>>>> UINT32 OffSet;
> >>>>>>>> CHAR16 *NameStr;
> >>>>>>>> RETURN_STATUS PeCoffStatus;
> >>>>>>>> @@ -1849,12 +1851,14 @@ DxeImageVerificationHandler (
> >>>>>>>> // "Attribute Certificate Table".
> >>>>>>>> // The first certificate starts at offset
> >>>>>>>> (SecDataDir->VirtualAddress) from
> >>>> the
> >>>>>> start of the file.
> >>>>>>>> //
> >>>>>>>> + SecDataDirEnd = SecDataDir->VirtualAddress + SecDataDir->Size;
> >>>>>>>> for (OffSet = SecDataDir->VirtualAddress;
> >>>>>>>> - OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size);
> >>>>>>>> + OffSet < SecDataDirEnd;
> >>>>>>>> OffSet += (WinCertificate->dwLength + ALIGN_SIZE
> >>>>>>>> (WinCertificate-
> >>>>>>> dwLength))) {
> >>>>>>>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> >>>>>>>> - if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <=
> sizeof
> >>>>>> (WIN_CERTIFICATE) ||
> >>>>>>>> - (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <
> >>>>>> WinCertificate->dwLength) {
> >>>>>>>> + SecDataDirLeft = SecDataDirEnd - OffSet;
> >>>>>>>> + if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) ||
> >>>>>>>> + SecDataDirLeft < WinCertificate->dwLength) {
> >>>>>>>> break;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> @@ -1948,7 +1952,7 @@ DxeImageVerificationHandler (
> >>>>>>>> }
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> - if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size))
> >>>>>>>> {
> >>>>>>>> + if (OffSet != SecDataDirEnd) {
> >>>>>>>> //
> >>>>>>>> // The Size in Certificate Table or the attribute
> >>>>>>>> certificate table is
> >>>> corrupted.
> >>>>>>>> //
> >>>>>>>> --
> >>>>>>>> 2.19.1.3.g30247aa5d201
> >>>>>>>>
> >>>>>>>
> >>>>>>> Patch#2:
> >>>>>>>
> >>>>>>>> From 72012c065a53582f7df695e7b9730c45f49226c6 Mon Sep 17
> 00:00:00
> >>>>>> 2001
> >>>>>>>> From: Laszlo Ersek <ler...@redhat.com>
> >>>>>>>> Date: Thu, 13 Aug 2020 19:19:06 +0200
> >>>>>>>> Subject: [PATCH 2/3] SecurityPkg/DxeImageVerificationLib: assign
> >>>>>>>> WinCertificate after size check
> >>>>>>>>
> >>>>>>>> Currently the (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) check
> >>>>>>>> only guards the de-referencing of the "WinCertificate" pointer.
> >>>>>>>> It does not guard the calculation of hte pointer itself:
> >>>>>>>>
> >>>>>>>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> >>>>>>>>
> >>>>>>>> This is wrong; if we don't know for sure that we have enough room
> >>>>>>>> for a WIN_CERTIFICATE, then even creating such a pointer, not
> >>>>>>>> just de-referencing it, may invoke undefined behavior.
> >>>>>>>>
> >>>>>>>> Move the pointer calculation after the size check.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> >>>>>>>> ---
> >>>>>>>>
> >>>>>>>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.c |
> >>>> 8
> >>>>>> +++++---
> >>>>>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git
> >>>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.c
> >>>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.c
> >>>>>>>> index 8761980c88aa..461ed7cfb5ac 100644
> >>>>>>>> ---
> >>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib
> >>>> .c
> >>>>>>>> +++
> >>>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.c
> >>>>>>>> @@ -1855,10 +1855,12 @@ DxeImageVerificationHandler (
> >>>>>>>> for (OffSet = SecDataDir->VirtualAddress;
> >>>>>>>> OffSet < SecDataDirEnd;
> >>>>>>>> OffSet += (WinCertificate->dwLength + ALIGN_SIZE
> >>>>>>>> (WinCertificate-
> >>>>>>> dwLength))) {
> >>>>>>>> - WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> >>>>>>>> SecDataDirLeft = SecDataDirEnd - OffSet;
> >>>>>>>> - if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE) ||
> >>>>>>>> - SecDataDirLeft < WinCertificate->dwLength) {
> >>>>>>>> + if (SecDataDirLeft <= sizeof (WIN_CERTIFICATE)) {
> >>>>>>>> + break;
> >>>>>>>> + }
> >>>>>>>> + WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> >>>>>>>> + if (SecDataDirLeft < WinCertificate->dwLength) {
> >>>>>>>> break;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> --
> >>>>>>>> 2.19.1.3.g30247aa5d201
> >>>>>>>>
> >>>>>>>
> >>>>>>> Patch#3:
> >>>>>>>
> >>>>>>>> From 0bbba15b84f8f9f2cdc770a89f418aaec6cfb31e Mon Sep 17
> 00:00:00
> >>>>>> 2001
> >>>>>>>> From: Laszlo Ersek <ler...@redhat.com>
> >>>>>>>> Date: Thu, 13 Aug 2020 19:34:33 +0200
> >>>>>>>> Subject: [PATCH 3/3] SecurityPkg/DxeImageVerificationLib: catch
> >>>> alignment
> >>>>>>>> overflow (CVE-2019-14562)
> >>>>>>>>
> >>>>>>>> The DxeImageVerificationHandler() function currently checks
> >>>>>>>> whether "SecDataDir" has enough room for
> >>>>>>>> "WinCertificate->dwLength". However,
> >>>>>> for
> >>>>>>>> advancing "OffSet", "WinCertificate->dwLength" is aligned to the
> >>>>>>>> next multiple of 8. If "WinCertificate->dwLength" is large
> >>>>>>>> enough, the alignment will return 0, and "OffSet" will be stuck at
> >>>>>>>> the
> same value.
> >>>>>>>>
> >>>>>>>> Check whether "SecDataDir" has room left for both
> >>>>>>>> "WinCertificate->dwLength" and the alignment.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> >>>>>>>> ---
> >>>>>>>>
> >>>>>>>> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>>>> ib.c |
> >>>> 4
> >>>>>> +++-
> >>>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>>>>
> >>>>>>>> diff --git
> >>>>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.c
> >>>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.c
> >>>>>>>> index 461ed7cfb5ac..e38eb981b7a0 100644
> >>>>>>>> ---
> >>>> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib
> >>>> .c
> >>>>>>>> +++
> >>>>>> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL
> >>>>>> ib.c
> >>>>>>>> @@ -1860,7 +1860,9 @@ DxeImageVerificationHandler (
> >>>>>>>> break;
> >>>>>>>> }
> >>>>>>>> WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet);
> >>>>>>>> - if (SecDataDirLeft < WinCertificate->dwLength) {
> >>>>>>>> + if (SecDataDirLeft < WinCertificate->dwLength ||
> >>>>>>>> + (SecDataDirLeft - WinCertificate->dwLength <
> >>>>>>>> + ALIGN_SIZE (WinCertificate->dwLength))) {
> >>>>>>>> break;
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> --
> >>>>>>>> 2.19.1.3.g30247aa5d201
> >>>>>>>>
> >>>>>>>
> >>>>>>> If Wenyi and the reviewers are OK with these patches, I can submit
> >>>>>>> them as a standalone patch series.
> >>>>>>>
> >>>>>>> Note that I do not have any reproducer for the issue; the best
> >>>>>>> testing that I could offer would be some light-weight Secure Boot
> >>>>>>> regression tests.
> >>>>>>>
> >>>>>>> Thanks
> >>>>>>> Laszlo
> >>>>>>>
> >>>>>>>
> >>>>>>> .
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>>
> >>>>
> >>>
> >>
> >
> >
> > .
> >
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#64711): https://edk2.groups.io/g/devel/message/64711
Mute This Topic: https://groups.io/mt/76165658/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
