From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web12.13337.1597235017287406302 for ; Wed, 12 Aug 2020 05:23:37 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=eDv7FZ4U; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1597235016; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=+BnQk9O4ZmY+RuvT9IH/fLD1MOHWbWJfMXSn/v7gp4g=; b=eDv7FZ4U3pIyPwlMNW2PpI4tbcJ9NeeS1x7Uvu0z67U9Y+vcQuts2/LkTS/aQBwTdVPsfV cPhLisMqCVWEc7ByJzlgvc/sF4JiOJdyR9NR8zN9ceHBTlmeZ/fWdlmAvtUQWHiqsYBcR7 O0/sG/KvMvJtMiigfEgZhI9Jtv91GJ4= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-130-Tpx7I379NNKfxv2bF1KnXA-1; Wed, 12 Aug 2020 08:23:28 -0400 X-MC-Unique: Tpx7I379NNKfxv2bF1KnXA-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 16765800D55; Wed, 12 Aug 2020 12:23:26 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-34.ams2.redhat.com [10.36.114.34]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2672C19C4F; Wed, 12 Aug 2020 12:23:23 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH EDK2 v1 1/1] SecurityPkg/DxeImageVerificationLib:Enhanced verification of Offset(CVE-2019-14562) To: devel@edk2.groups.io, xiewenyi2@huawei.com, jiewen.yao@intel.com, jian.j.wang@intel.com, chao.b.zhang@intel.com Cc: huangming23@huawei.com, songdongkuang@huawei.com References: <1597215886-48713-1-git-send-email-xiewenyi2@huawei.com> <1597215886-48713-2-git-send-email-xiewenyi2@huawei.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 12 Aug 2020 14:23:23 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1597215886-48713-2-git-send-email-xiewenyi2@huawei.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0.002 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 08/12/20 09:04, wenyi,xie via groups.io 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 > Cc: Jian J Wang > Cc: Chao Zhang > Signed-off-by: Wenyi Xie > --- > SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf | 1 + > SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h | 1 + > SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 21 +++++++++++++++----- > 3 files changed, 18 insertions(+), 5 deletions(-) > > diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf > index 1e1a639857e0..a7ac4830b3d4 100644 > --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf > +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf > @@ -53,6 +53,7 @@ [LibraryClasses] > SecurityManagementLib > PeCoffLib > TpmMeasurementLib > + SafeIntLib > > [Protocols] > gEfiFirmwareVolume2ProtocolGuid ## SOMETIMES_CONSUMES > diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h > index 17955ff9774c..060273917d5d 100644 > --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h > +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.h > @@ -23,6 +23,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include > #include > #include > +#include > #include > #include > #include > diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > index 36b87e16d53d..2b42d4595f2c 100644 > --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c > @@ -1658,6 +1658,9 @@ DxeImageVerificationHandler ( > EFI_STATUS HashStatus; > EFI_STATUS DbStatus; > BOOLEAN IsFound; > + UINT32 AlignedLength; > + UINT32 Result; > + EFI_STATUS AddStatus; > > SignatureList = NULL; > SignatureListSize = 0; > @@ -1667,6 +1670,7 @@ DxeImageVerificationHandler ( > Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED; > IsVerified = FALSE; > IsFound = FALSE; > + Result = 0; > > // > // Check the image type and get policy setting. > @@ -1850,9 +1854,9 @@ 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));) { > WinCertificate = (WIN_CERTIFICATE *) (mImageBase + OffSet); > + AlignedLength = WinCertificate->dwLength + ALIGN_SIZE (WinCertificate->dwLength); > if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <= sizeof (WIN_CERTIFICATE) || > (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < WinCertificate->dwLength) { > break; > @@ -1881,7 +1885,7 @@ DxeImageVerificationHandler ( > break; > } > if (!CompareGuid (&WinCertUefiGuid->CertType, &gEfiCertPkcs7Guid)) { > - continue; > + goto NEXT_LOOP; > } > AuthData = WinCertUefiGuid->CertData; > AuthDataSize = WinCertUefiGuid->Hdr.dwLength - OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData); > @@ -1889,12 +1893,12 @@ DxeImageVerificationHandler ( > if (WinCertificate->dwLength < sizeof (WIN_CERTIFICATE)) { > break; > } > - continue; > + goto NEXT_LOOP; > } > > HashStatus = HashPeImageByType (AuthData, AuthDataSize); > if (EFI_ERROR (HashStatus)) { > - continue; > + goto NEXT_LOOP; > } > > // > @@ -1946,6 +1950,13 @@ DxeImageVerificationHandler ( > 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)); > } > } > + > +NEXT_LOOP: > + AddStatus = SafeUint32Add (OffSet, AlignedLength, &Result); > + if (EFI_ERROR (AddStatus)) { > + break; > + } > + OffSet = Result; > } > > if (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size)) { > (1) Please mark the patch URL on the bugzilla ticket, in a comment. (2) We should not use "goto" statements for such control transfers, only for error handling. If necessary, please split the function in two, or reorganize the loop otherwise. Thanks Laszlo