From nobody Mon Feb 9 23:39:28 2026 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of groups.io designates 66.175.222.12 as permitted sender) client-ip=66.175.222.12; envelope-from=bounce+27952+53315+1787277+3901457@groups.io; helo=web01.groups.io; Authentication-Results: mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of groups.io designates 66.175.222.12 as permitted sender) smtp.mailfrom=bounce+27952+53315+1787277+3901457@groups.io; dmarc=fail(p=none dis=none) header.from=redhat.com ARC-Seal: i=1; a=rsa-sha256; t=1579201639; cv=none; d=zohomail.com; s=zohoarc; b=Pj3yzuFUO0wiW05KDskVte4iIRGfwsftLCp1mK6fkw0TF/T3sRptQ/pCIi3WRUXTJlfjkXx8Coftv7ah0oQPNA1nS18z4bQs6yrLgJ8mGrXBAF0gEdplmrVJeiROzQNI0M5kOC5/xWC06ptHe49ZIjz3Q9DetgeE4J4aE/GfNYU= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1579201639; h=Content-Type:Content-Transfer-Encoding:Cc:Date:From:In-Reply-To:List-Id:List-Unsubscribe:MIME-Version:Message-ID:Reply-To:References:Sender:Subject:To; bh=Yn1LtQArQYNqJp0a8AeBEMEdBd7wuRehVnTAas4+2r0=; b=TRkakTz4Zpp9CsZOAdXWiUNjDRrGkKBILRUDCHPeqzQYrLho8Zm/lwQWCVHf6fUG33B/44svhBFioWu5eF+UaiBu9b6UpjPFPTwgu3lTv73Xfmn9c5EfJ1HOelYJkbc+SIz0UobXjO2IPW+FHqgXZ+V+L9WIqd26o+0vYyJo8fY= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass; spf=pass (zohomail.com: domain of groups.io designates 66.175.222.12 as permitted sender) smtp.mailfrom=bounce+27952+53315+1787277+3901457@groups.io; dmarc=fail header.from= (p=none dis=none) header.from= Received: from web01.groups.io (web01.groups.io [66.175.222.12]) by mx.zohomail.com with SMTPS id 1579201639712536.7854849229888; Thu, 16 Jan 2020 11:07:19 -0800 (PST) Return-Path: X-Received: by 127.0.0.2 with SMTP id CAUrYY1788612xvfaeNh9UMq; Thu, 16 Jan 2020 11:07:19 -0800 X-Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web11.1415.1579201638616189528 for ; Thu, 16 Jan 2020 11:07:18 -0800 X-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-145-oxviG3u1O6CKk_g6rWDlBw-1; Thu, 16 Jan 2020 14:07:13 -0500 X-Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D692A800D5E; Thu, 16 Jan 2020 19:07:11 +0000 (UTC) X-Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-120.ams2.redhat.com [10.36.116.120]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8C6DB80890; Thu, 16 Jan 2020 19:07:10 +0000 (UTC) From: "Laszlo Ersek" To: edk2-devel-groups-io Cc: Chao Zhang , Jian J Wang , Jiewen Yao Subject: [edk2-devel] [PATCH 02/11] SecurityPkg/DxeImageVerificationHandler: remove "else" after return/break Date: Thu, 16 Jan 2020 20:06:56 +0100 Message-Id: <20200116190705.18816-3-lersek@redhat.com> In-Reply-To: <20200116190705.18816-1-lersek@redhat.com> References: <20200116190705.18816-1-lersek@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-MC-Unique: oxviG3u1O6CKk_g6rWDlBw-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Unsubscribe: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com X-Gm-Message-State: mJPuKaLQvcxLuXDhHLReBZTHx1787277AA= Content-Transfer-Encoding: quoted-printable DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=groups.io; q=dns/txt; s=20140610; t=1579201639; bh=Yn1LtQArQYNqJp0a8AeBEMEdBd7wuRehVnTAas4+2r0=; h=Cc:Content-Type:Date:From:Reply-To:Subject:To; b=RDzednt/sMy1ezV6Tw12vqK9r5R/Wk6fdb3pBf3c6xpObllCw/bc2PLhuMRHc2LloJ9 J6Qe11EOko9w17bT2TfSNAcR9ExJWXUpg//5wJYhKhISt6tj6InhnOJRce4trqKbja/SR gcMl6516DanOLYndDiC46c+ygAObjgDUSI0= X-ZohoMail-DKIM: pass (identity @groups.io) Content-Type: text/plain; charset="utf-8" In the code structure if (condition) { // // block1 // return; } else { // // block2 // } nesting "block2" in an "else" branch is superfluous, and harms readability. It can be transformed to: if (condition) { // // block1 // return; } // // block2 // with identical behavior, and improved readability (less nesting). The same applies to "break" (instead of "return") in a loop body. Perform these transformations on DxeImageVerificationHandler(). This patch is a no-op for behavior. Use git show -b -W for reviewing it more easily. Cc: Chao Zhang Cc: Jian J Wang Cc: Jiewen Yao Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2129 Signed-off-by: Laszlo Ersek --- SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 41= ++++++++++---------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificati= onLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationL= ib.c index 5afd723adae8..8204c9c0f105 100644 --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c @@ -1556,349 +1556,350 @@ EFIAPI DxeImageVerificationHandler ( IN UINT32 AuthenticationStatus, IN CONST EFI_DEVICE_PATH_PROTOCOL *File, IN VOID *FileBuffer, IN UINTN FileSize, IN BOOLEAN BootPolicy ) { EFI_STATUS Status; EFI_IMAGE_DOS_HEADER *DosHdr; BOOLEAN IsVerified; EFI_SIGNATURE_LIST *SignatureList; UINTN SignatureListSize; EFI_SIGNATURE_DATA *Signature; EFI_IMAGE_EXECUTION_ACTION Action; WIN_CERTIFICATE *WinCertificate; UINT32 Policy; UINT8 *SecureBoot; PE_COFF_LOADER_IMAGE_CONTEXT ImageContext; UINT32 NumberOfRvaAndSizes; WIN_CERTIFICATE_EFI_PKCS *PkcsCertData; WIN_CERTIFICATE_UEFI_GUID *WinCertUefiGuid; UINT8 *AuthData; UINTN AuthDataSize; EFI_IMAGE_DATA_DIRECTORY *SecDataDir; UINT32 OffSet; CHAR16 *NameStr; =20 SignatureList =3D NULL; SignatureListSize =3D 0; WinCertificate =3D NULL; SecDataDir =3D NULL; PkcsCertData =3D NULL; Action =3D EFI_IMAGE_EXECUTION_AUTH_UNTESTED; Status =3D EFI_ACCESS_DENIED; IsVerified =3D FALSE; =20 =20 // // Check the image type and get policy setting. // switch (GetImageType (File)) { =20 case IMAGE_FROM_FV: Policy =3D ALWAYS_EXECUTE; break; =20 case IMAGE_FROM_OPTION_ROM: Policy =3D PcdGet32 (PcdOptionRomImageVerificationPolicy); break; =20 case IMAGE_FROM_REMOVABLE_MEDIA: Policy =3D PcdGet32 (PcdRemovableMediaImageVerificationPolicy); break; =20 case IMAGE_FROM_FIXED_MEDIA: Policy =3D PcdGet32 (PcdFixedMediaImageVerificationPolicy); break; =20 default: Policy =3D DENY_EXECUTE_ON_SECURITY_VIOLATION; break; } // // If policy is always/never execute, return directly. // if (Policy =3D=3D ALWAYS_EXECUTE) { return EFI_SUCCESS; - } else if (Policy =3D=3D NEVER_EXECUTE) { + } + if (Policy =3D=3D NEVER_EXECUTE) { return EFI_ACCESS_DENIED; } =20 // // The policy QUERY_USER_ON_SECURITY_VIOLATION and ALLOW_EXECUTE_ON_SECU= RITY_VIOLATION // violates the UEFI spec and has been removed. // ASSERT (Policy !=3D QUERY_USER_ON_SECURITY_VIOLATION && Policy !=3D ALLO= W_EXECUTE_ON_SECURITY_VIOLATION); if (Policy =3D=3D QUERY_USER_ON_SECURITY_VIOLATION || Policy =3D=3D ALLO= W_EXECUTE_ON_SECURITY_VIOLATION) { CpuDeadLoop (); } =20 GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, (VOID**)&SecureBoot, N= ULL); // // Skip verification if SecureBoot variable doesn't exist. // if (SecureBoot =3D=3D NULL) { return EFI_SUCCESS; } =20 // // Skip verification if SecureBoot is disabled but not AuditMode // if (*SecureBoot =3D=3D SECURE_BOOT_MODE_DISABLE) { FreePool (SecureBoot); return EFI_SUCCESS; } FreePool (SecureBoot); =20 // // Read the Dos header. // if (FileBuffer =3D=3D NULL) { return EFI_INVALID_PARAMETER; } =20 mImageBase =3D (UINT8 *) FileBuffer; mImageSize =3D FileSize; =20 ZeroMem (&ImageContext, sizeof (ImageContext)); ImageContext.Handle =3D (VOID *) FileBuffer; ImageContext.ImageRead =3D (PE_COFF_LOADER_READ_FILE) DxeImageVerificati= onLibImageRead; =20 // // Get information about the image being loaded // Status =3D PeCoffLoaderGetImageInfo (&ImageContext); if (EFI_ERROR (Status)) { // // The information can't be got from the invalid PeImage // DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: PeImage invalid. Cannot = retrieve image information.\n")); goto Done; } =20 Status =3D EFI_ACCESS_DENIED; =20 DosHdr =3D (EFI_IMAGE_DOS_HEADER *) mImageBase; if (DosHdr->e_magic =3D=3D EFI_IMAGE_DOS_SIGNATURE) { // // DOS image header is present, // so read the PE header after the DOS image header. // mPeCoffHeaderOffset =3D DosHdr->e_lfanew; } else { mPeCoffHeaderOffset =3D 0; } // // Check PE/COFF image. // mNtHeader.Pe32 =3D (EFI_IMAGE_NT_HEADERS32 *) (mImageBase + mPeCoffHeade= rOffset); if (mNtHeader.Pe32->Signature !=3D EFI_IMAGE_NT_SIGNATURE) { // // It is not a valid Pe/Coff file. // DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Not a valid PE/COFF imag= e.\n")); goto Done; } =20 if (mNtHeader.Pe32->OptionalHeader.Magic =3D=3D EFI_IMAGE_NT_OPTIONAL_HD= R32_MAGIC) { // // Use PE32 offset. // NumberOfRvaAndSizes =3D mNtHeader.Pe32->OptionalHeader.NumberOfRvaAndS= izes; if (NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_SECURITY) { SecDataDir =3D (EFI_IMAGE_DATA_DIRECTORY *) &mNtHeader.Pe32->Optiona= lHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY]; } } else { // // Use PE32+ offset. // NumberOfRvaAndSizes =3D mNtHeader.Pe32Plus->OptionalHeader.NumberOfRva= AndSizes; if (NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_SECURITY) { SecDataDir =3D (EFI_IMAGE_DATA_DIRECTORY *) &mNtHeader.Pe32Plus->Opt= ionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY]; } } =20 // // Start Image Validation. // if (SecDataDir =3D=3D NULL || SecDataDir->Size =3D=3D 0) { // // This image is not signed. The SHA256 hash value of the image must m= atch a record in the security database "db", // and not be reflected in the security data base "dbx". // if (!HashPeImage (HASHALG_SHA256)) { DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Failed to hash this im= age using %s.\n", mHashTypeStr)); goto Done; } =20 if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE1, mImageDi= gest, &mCertType, mImageDigestSize)) { // // Image Hash is in forbidden database (DBX). // DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is not signed an= d %s hash of image is forbidden by DBX.\n", mHashTypeStr)); goto Done; } =20 if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE, mImageDig= est, &mCertType, mImageDigestSize)) { // // Image Hash is in allowed database (DB). // return EFI_SUCCESS; } =20 // // Image Hash is not found in both forbidden and allowed database. // DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is not signed and = %s hash of image is not found in DB/DBX.\n", mHashTypeStr)); goto Done; } =20 // // Verify the signature of the image, multiple signatures are allowed as= per PE/COFF Section 4.7 // "Attribute Certificate Table". // The first certificate starts at offset (SecDataDir->VirtualAddress) f= rom the start of the file. // for (OffSet =3D SecDataDir->VirtualAddress; OffSet < (SecDataDir->VirtualAddress + SecDataDir->Size); OffSet +=3D (WinCertificate->dwLength + ALIGN_SIZE (WinCertificate-= >dwLength))) { WinCertificate =3D (WIN_CERTIFICATE *) (mImageBase + OffSet); if ((SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) <=3D size= of (WIN_CERTIFICATE) || (SecDataDir->VirtualAddress + SecDataDir->Size - OffSet) < WinCert= ificate->dwLength) { break; } =20 // // Verify the image's Authenticode signature, only DER-encoded PKCS#7 = signed data is supported. // if (WinCertificate->wCertificateType =3D=3D WIN_CERT_TYPE_PKCS_SIGNED_= DATA) { // // The certificate is formatted as WIN_CERTIFICATE_EFI_PKCS which is= described in the // Authenticode specification. // PkcsCertData =3D (WIN_CERTIFICATE_EFI_PKCS *) WinCertificate; if (PkcsCertData->Hdr.dwLength <=3D sizeof (PkcsCertData->Hdr)) { break; } AuthData =3D PkcsCertData->CertData; AuthDataSize =3D PkcsCertData->Hdr.dwLength - sizeof(PkcsCertData->H= dr); } else if (WinCertificate->wCertificateType =3D=3D WIN_CERT_TYPE_EFI_G= UID) { // // The certificate is formatted as WIN_CERTIFICATE_UEFI_GUID which i= s described in UEFI Spec. // WinCertUefiGuid =3D (WIN_CERTIFICATE_UEFI_GUID *) WinCertificate; if (WinCertUefiGuid->Hdr.dwLength <=3D OFFSET_OF(WIN_CERTIFICATE_UEF= I_GUID, CertData)) { break; } if (!CompareGuid (&WinCertUefiGuid->CertType, &gEfiCertPkcs7Guid)) { continue; } AuthData =3D WinCertUefiGuid->CertData; AuthDataSize =3D WinCertUefiGuid->Hdr.dwLength - OFFSET_OF(WIN_CERTI= FICATE_UEFI_GUID, CertData); } else { if (WinCertificate->dwLength < sizeof (WIN_CERTIFICATE)) { break; } continue; } =20 Status =3D HashPeImageByType (AuthData, AuthDataSize); if (EFI_ERROR (Status)) { continue; } =20 // // Check the digital signature against the revoked certificate in forb= idden database (dbx). // if (IsForbiddenByDbx (AuthData, AuthDataSize)) { Action =3D EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED; IsVerified =3D FALSE; break; } =20 // // Check the digital signature against the valid certificate in allowe= d database (db). // if (!IsVerified) { if (IsAllowedByDb (AuthData, AuthDataSize)) { IsVerified =3D TRUE; } } =20 // // Check the image's hash value. // if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE1, mImageDi= gest, &mCertType, mImageDigestSize)) { Action =3D 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 =3D FALSE; break; - } else if (!IsVerified) { + } + if (!IsVerified) { if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE, mImageD= igest, &mCertType, mImageDigestSize)) { IsVerified =3D 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)); } } } =20 if (OffSet !=3D (SecDataDir->VirtualAddress + SecDataDir->Size)) { // // The Size in Certificate Table or the attribute certificate table is= corrupted. // IsVerified =3D FALSE; } =20 if (IsVerified) { return EFI_SUCCESS; - } else { - Status =3D EFI_ACCESS_DENIED; - if (Action =3D=3D EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED || Action =3D=3D= EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND) { - // - // Get image hash value as signature of executable. - // - SignatureListSize =3D sizeof (EFI_SIGNATURE_LIST) + sizeof (EFI_SIGN= ATURE_DATA) - 1 + mImageDigestSize; - SignatureList =3D (EFI_SIGNATURE_LIST *) AllocateZeroPool (Signa= tureListSize); - if (SignatureList =3D=3D NULL) { - Status =3D EFI_OUT_OF_RESOURCES; - goto Done; - } - SignatureList->SignatureHeaderSize =3D 0; - SignatureList->SignatureListSize =3D (UINT32) SignatureListSize; - SignatureList->SignatureSize =3D (UINT32) (sizeof (EFI_SIGNAT= URE_DATA) - 1 + mImageDigestSize); - CopyMem (&SignatureList->SignatureType, &mCertType, sizeof (EFI_GUID= )); - Signature =3D (EFI_SIGNATURE_DATA *) ((UINT8 *) SignatureList + size= of (EFI_SIGNATURE_LIST)); - CopyMem (Signature->SignatureData, mImageDigest, mImageDigestSize); + } + Status =3D EFI_ACCESS_DENIED; + if (Action =3D=3D EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED || Action =3D=3D E= FI_IMAGE_EXECUTION_AUTH_SIG_FOUND) { + // + // Get image hash value as signature of executable. + // + SignatureListSize =3D sizeof (EFI_SIGNATURE_LIST) + sizeof (EFI_SIGNAT= URE_DATA) - 1 + mImageDigestSize; + SignatureList =3D (EFI_SIGNATURE_LIST *) AllocateZeroPool (Signatu= reListSize); + if (SignatureList =3D=3D NULL) { + Status =3D EFI_OUT_OF_RESOURCES; + goto Done; } + SignatureList->SignatureHeaderSize =3D 0; + SignatureList->SignatureListSize =3D (UINT32) SignatureListSize; + SignatureList->SignatureSize =3D (UINT32) (sizeof (EFI_SIGNATUR= E_DATA) - 1 + mImageDigestSize); + CopyMem (&SignatureList->SignatureType, &mCertType, sizeof (EFI_GUID)); + Signature =3D (EFI_SIGNATURE_DATA *) ((UINT8 *) SignatureList + sizeof= (EFI_SIGNATURE_LIST)); + CopyMem (Signature->SignatureData, mImageDigest, mImageDigestSize); } =20 Done: if (Status !=3D EFI_SUCCESS) { // // Policy decides to defer or reject the image; add its information in= image executable information table. // NameStr =3D ConvertDevicePathToText (File, FALSE, TRUE); AddImageExeInfo (Action, NameStr, File, SignatureList, SignatureListSi= ze); if (NameStr !=3D NULL) { DEBUG((EFI_D_INFO, "The image doesn't pass verification: %s\n", Name= Str)); FreePool(NameStr); } Status =3D EFI_SECURITY_VIOLATION; } =20 if (SignatureList !=3D NULL) { FreePool (SignatureList); } =20 return Status; } =20 /** On Ready To Boot Services Event notification handler. =20 Add the image execution information table if it is not in system configu= ration table. =20 @param[in] Event Event whose notification function is being invoked @param[in] Context Pointer to the notification function's context =20 **/ --=20 2.19.1.3.g30247aa5d201 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#53315): https://edk2.groups.io/g/devel/message/53315 Mute This Topic: https://groups.io/mt/69752219/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-