public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: edk2-devel-groups-io <devel@edk2.groups.io>
Cc: Chao Zhang <chao.b.zhang@intel.com>,
	Jian J Wang <jian.j.wang@intel.com>,
	Jiewen Yao <jiewen.yao@intel.com>
Subject: [PATCH 09/11] SecurityPkg/DxeImageVerificationHandler: fix retval for (FileBuffer==NULL)
Date: Thu, 16 Jan 2020 20:07:03 +0100	[thread overview]
Message-ID: <20200116190705.18816-10-lersek@redhat.com> (raw)
In-Reply-To: <20200116190705.18816-1-lersek@redhat.com>

"FileBuffer" is a non-optional input (pointer) parameter to
DxeImageVerificationHandler(). Normally, when an edk2 function receives a
NULL argument for such a parameter, we return EFI_INVALID_PARAMETER or
RETURN_INVALID_PARAMETER. However, those don't conform to the
SECURITY2_FILE_AUTHENTICATION_HANDLER prototype.

Return EFI_ACCESS_DENIED when "FileBuffer" is NULL; it means that no image
has been loaded.

This patch does not change the control flow in the function, it only
changes the "Status" outcome from API-incompatible error codes to
EFI_ACCESS_DENIED, under some circumstances.

Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
Fixes: 570b3d1a7278df29878da87990e8366bd42d0ec5
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 3041c8718beb..f6ad5931be07 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1555,343 +1555,343 @@ EFIAPI
 DxeImageVerificationHandler (
   IN  UINT32                           AuthenticationStatus,
   IN  CONST EFI_DEVICE_PATH_PROTOCOL   *File,
   IN  VOID                             *FileBuffer,
   IN  UINTN                            FileSize,
   IN  BOOLEAN                          BootPolicy
   )
 {
   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;
   RETURN_STATUS                        PeCoffStatus;
   EFI_STATUS                           HashStatus;
 
   SignatureList     = NULL;
   SignatureListSize = 0;
   WinCertificate    = NULL;
   SecDataDir        = NULL;
   PkcsCertData      = NULL;
   Action            = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
   IsVerified        = FALSE;
 
 
   //
   // Check the image type and get policy setting.
   //
   switch (GetImageType (File)) {
 
   case IMAGE_FROM_FV:
     Policy = ALWAYS_EXECUTE;
     break;
 
   case IMAGE_FROM_OPTION_ROM:
     Policy = PcdGet32 (PcdOptionRomImageVerificationPolicy);
     break;
 
   case IMAGE_FROM_REMOVABLE_MEDIA:
     Policy = PcdGet32 (PcdRemovableMediaImageVerificationPolicy);
     break;
 
   case IMAGE_FROM_FIXED_MEDIA:
     Policy = PcdGet32 (PcdFixedMediaImageVerificationPolicy);
     break;
 
   default:
     Policy = DENY_EXECUTE_ON_SECURITY_VIOLATION;
     break;
   }
   //
   // If policy is always/never execute, return directly.
   //
   if (Policy == ALWAYS_EXECUTE) {
     return EFI_SUCCESS;
   }
   if (Policy == NEVER_EXECUTE) {
     return EFI_ACCESS_DENIED;
   }
 
   //
   // The policy QUERY_USER_ON_SECURITY_VIOLATION and ALLOW_EXECUTE_ON_SECURITY_VIOLATION
   // violates the UEFI spec and has been removed.
   //
   ASSERT (Policy != QUERY_USER_ON_SECURITY_VIOLATION && Policy != ALLOW_EXECUTE_ON_SECURITY_VIOLATION);
   if (Policy == QUERY_USER_ON_SECURITY_VIOLATION || Policy == ALLOW_EXECUTE_ON_SECURITY_VIOLATION) {
     CpuDeadLoop ();
   }
 
   GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, (VOID**)&SecureBoot, NULL);
   //
   // Skip verification if SecureBoot variable doesn't exist.
   //
   if (SecureBoot == NULL) {
     return EFI_SUCCESS;
   }
 
   //
   // Skip verification if SecureBoot is disabled but not AuditMode
   //
   if (*SecureBoot == SECURE_BOOT_MODE_DISABLE) {
     FreePool (SecureBoot);
     return EFI_SUCCESS;
   }
   FreePool (SecureBoot);
 
   //
   // Read the Dos header.
   //
   if (FileBuffer == NULL) {
-    return EFI_INVALID_PARAMETER;
+    return EFI_ACCESS_DENIED;
   }
 
   mImageBase  = (UINT8 *) FileBuffer;
   mImageSize  = FileSize;
 
   ZeroMem (&ImageContext, sizeof (ImageContext));
   ImageContext.Handle    = (VOID *) FileBuffer;
   ImageContext.ImageRead = (PE_COFF_LOADER_READ_FILE) DxeImageVerificationLibImageRead;
 
   //
   // Get information about the image being loaded
   //
   PeCoffStatus = PeCoffLoaderGetImageInfo (&ImageContext);
   if (RETURN_ERROR (PeCoffStatus)) {
     //
     // The information can't be got from the invalid PeImage
     //
     DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: PeImage invalid. Cannot retrieve image information.\n"));
     goto Failed;
   }
 
   DosHdr = (EFI_IMAGE_DOS_HEADER *) mImageBase;
   if (DosHdr->e_magic == EFI_IMAGE_DOS_SIGNATURE) {
     //
     // DOS image header is present,
     // so read the PE header after the DOS image header.
     //
     mPeCoffHeaderOffset = DosHdr->e_lfanew;
   } else {
     mPeCoffHeaderOffset = 0;
   }
   //
   // Check PE/COFF image.
   //
   mNtHeader.Pe32 = (EFI_IMAGE_NT_HEADERS32 *) (mImageBase + mPeCoffHeaderOffset);
   if (mNtHeader.Pe32->Signature != EFI_IMAGE_NT_SIGNATURE) {
     //
     // It is not a valid Pe/Coff file.
     //
     DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Not a valid PE/COFF image.\n"));
     goto Failed;
   }
 
   if (mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
     //
     // Use PE32 offset.
     //
     NumberOfRvaAndSizes = mNtHeader.Pe32->OptionalHeader.NumberOfRvaAndSizes;
     if (NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_SECURITY) {
       SecDataDir = (EFI_IMAGE_DATA_DIRECTORY *) &mNtHeader.Pe32->OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY];
     }
   } else {
     //
     // Use PE32+ offset.
     //
     NumberOfRvaAndSizes = mNtHeader.Pe32Plus->OptionalHeader.NumberOfRvaAndSizes;
     if (NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_SECURITY) {
       SecDataDir = (EFI_IMAGE_DATA_DIRECTORY *) &mNtHeader.Pe32Plus->OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY];
     }
   }
 
   //
   // Start Image Validation.
   //
   if (SecDataDir == NULL || SecDataDir->Size == 0) {
     //
     // This image is not signed. The SHA256 hash value of the image must match 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 image using %s.\n", mHashTypeStr));
       goto Failed;
     }
 
     if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE1, mImageDigest, &mCertType, mImageDigestSize)) {
       //
       // Image Hash is in forbidden database (DBX).
       //
       DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is not signed and %s hash of image is forbidden by DBX.\n", mHashTypeStr));
       goto Failed;
     }
 
     if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE, mImageDigest, &mCertType, mImageDigestSize)) {
       //
       // Image Hash is in allowed database (DB).
       //
       return EFI_SUCCESS;
     }
 
     //
     // 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 Failed;
   }
 
   //
   // 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) from the start of the file.
   //
   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;
     }
 
     //
     // Verify the image's Authenticode signature, only DER-encoded PKCS#7 signed data is supported.
     //
     if (WinCertificate->wCertificateType == WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
       //
       // The certificate is formatted as WIN_CERTIFICATE_EFI_PKCS which is described in the
       // Authenticode specification.
       //
       PkcsCertData = (WIN_CERTIFICATE_EFI_PKCS *) WinCertificate;
       if (PkcsCertData->Hdr.dwLength <= sizeof (PkcsCertData->Hdr)) {
         break;
       }
       AuthData   = PkcsCertData->CertData;
       AuthDataSize = PkcsCertData->Hdr.dwLength - sizeof(PkcsCertData->Hdr);
     } else if (WinCertificate->wCertificateType == WIN_CERT_TYPE_EFI_GUID) {
       //
       // The certificate is formatted as WIN_CERTIFICATE_UEFI_GUID which is described in UEFI Spec.
       //
       WinCertUefiGuid = (WIN_CERTIFICATE_UEFI_GUID *) WinCertificate;
       if (WinCertUefiGuid->Hdr.dwLength <= OFFSET_OF(WIN_CERTIFICATE_UEFI_GUID, CertData)) {
         break;
       }
       if (!CompareGuid (&WinCertUefiGuid->CertType, &gEfiCertPkcs7Guid)) {
         continue;
       }
       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;
       }
     }
 
     //
     // Check the image's hash value.
     //
     if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE1, mImageDigest, &mCertType, mImageDigestSize)) {
       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) {
       if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE, mImageDigest, &mCertType, mImageDigestSize)) {
         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 (OffSet != (SecDataDir->VirtualAddress + SecDataDir->Size)) {
     //
     // The Size in Certificate Table or the attribute certificate table is corrupted.
     //
     IsVerified = FALSE;
   }
 
   if (IsVerified) {
     return EFI_SUCCESS;
   }
   if (Action == EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED || Action == EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND) {
     //
     // Get image hash value as signature of executable.
     //
     SignatureListSize = sizeof (EFI_SIGNATURE_LIST) + sizeof (EFI_SIGNATURE_DATA) - 1 + mImageDigestSize;
     SignatureList     = (EFI_SIGNATURE_LIST *) AllocateZeroPool (SignatureListSize);
     if (SignatureList == NULL) {
       goto Failed;
     }
     SignatureList->SignatureHeaderSize  = 0;
     SignatureList->SignatureListSize    = (UINT32) SignatureListSize;
     SignatureList->SignatureSize        = (UINT32) (sizeof (EFI_SIGNATURE_DATA) - 1 + mImageDigestSize);
     CopyMem (&SignatureList->SignatureType, &mCertType, sizeof (EFI_GUID));
     Signature = (EFI_SIGNATURE_DATA *) ((UINT8 *) SignatureList + sizeof (EFI_SIGNATURE_LIST));
     CopyMem (Signature->SignatureData, mImageDigest, mImageDigestSize);
   }
 
 Failed:
   //
   // Policy decides to defer or reject the image; add its information in image executable information table.
   //
   NameStr = ConvertDevicePathToText (File, FALSE, TRUE);
   AddImageExeInfo (Action, NameStr, File, SignatureList, SignatureListSize);
   if (NameStr != NULL) {
     DEBUG((EFI_D_INFO, "The image doesn't pass verification: %s\n", NameStr));
     FreePool(NameStr);
   }
 
   if (SignatureList != NULL) {
     FreePool (SignatureList);
   }
 
   return EFI_SECURITY_VIOLATION;
 }
 
 /**
   On Ready To Boot Services Event notification handler.
 
   Add the image execution information table if it is not in system configuration table.
 
   @param[in]  Event     Event whose notification function is being invoked
   @param[in]  Context   Pointer to the notification function's context
 
 **/
-- 
2.19.1.3.g30247aa5d201



  parent reply	other threads:[~2020-01-16 19:07 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-16 19:06 [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Laszlo Ersek
2020-01-16 19:06 ` [PATCH 01/11] SecurityPkg/DxeImageVerificationHandler: simplify "VerifyStatus" Laszlo Ersek
2020-01-16 19:06 ` [PATCH 02/11] SecurityPkg/DxeImageVerificationHandler: remove "else" after return/break Laszlo Ersek
2020-01-16 19:06 ` [PATCH 03/11] SecurityPkg/DxeImageVerificationHandler: keep PE/COFF info status internal Laszlo Ersek
2020-01-16 19:06 ` [PATCH 04/11] SecurityPkg/DxeImageVerificationHandler: narrow down PE/COFF hash status Laszlo Ersek
2020-01-16 19:06 ` [PATCH 05/11] SecurityPkg/DxeImageVerificationHandler: fix retval on memalloc failure Laszlo Ersek
2020-01-16 19:07 ` [PATCH 06/11] SecurityPkg/DxeImageVerificationHandler: remove superfluous Status setting Laszlo Ersek
2020-01-16 19:07 ` [PATCH 07/11] SecurityPkg/DxeImageVerificationHandler: unnest AddImageExeInfo() call Laszlo Ersek
2020-01-16 19:07 ` [PATCH 08/11] SecurityPkg/DxeImageVerificationHandler: eliminate "Status" variable Laszlo Ersek
2020-01-16 19:07 ` Laszlo Ersek [this message]
2020-01-16 19:07 ` [PATCH 10/11] SecurityPkg/DxeImageVerificationHandler: fix imgexec info on memalloc fail Laszlo Ersek
2020-01-16 19:07 ` [PATCH 11/11] SecurityPkg/DxeImageVerificationHandler: fix "defer" vs. "deny" policies Laszlo Ersek
2020-01-31  2:59 ` [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Michael D Kinney
2020-01-31  8:12   ` Laszlo Ersek
2020-01-31  9:28     ` Laszlo Ersek
2020-01-31 10:01       ` Laszlo Ersek
2020-01-31 10:07       ` Laszlo Ersek
2020-01-31 16:52       ` Michael D Kinney
2020-01-31 16:59         ` Laszlo Ersek
2020-01-31 17:28           ` Michael D Kinney
2020-01-31 20:19             ` Laszlo Ersek
2020-02-05 13:02               ` setting the push label at once, when opening a PR [was: SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy] Laszlo Ersek
2020-02-05 16:16                 ` Michael D Kinney
2020-02-05 20:01                   ` Laszlo Ersek
2020-01-31 16:31     ` [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Michael D Kinney
2020-01-31 17:00       ` Laszlo Ersek
2020-01-31 17:12         ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200116190705.18816-10-lersek@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox