* [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy
@ 2020-01-16 19:06 Laszlo Ersek
2020-01-16 19:06 ` [PATCH 01/11] SecurityPkg/DxeImageVerificationHandler: simplify "VerifyStatus" Laszlo Ersek
` (11 more replies)
0 siblings, 12 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-16 19:06 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Chao Zhang, Jian J Wang, Jiewen Yao
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
Repo: https://github.com/lersek/edk2.git
Branch: deny_execute_2129
The DxeImageVerificationHandler() function does not handle the
DENY_EXECUTE_ON_SECURITY_VIOLATION policy correctly. When an image is
rejected, and the platform sets this policy for the corresponding image
source, the function should return EFI_ACCESS_DENIED. Instead, the
function currently returns EFI_SECURITY_VIOLATION. The consequence is
that gBS->LoadImage() will keep the image loaded (in untrusted state),
rather than unloading it immediately. If the platform sets the
DENY_EXECUTE_ON_SECURITY_VIOLATION policy for all image sources, then
the platform may not expect EFI_SECURITY_VIOLATION at all. Then,
rejected images may linger in RAM, in untrusted state, and may be leaked
forever.
This series refactors the DxeImageVerificationHandler() function,
simplifying the control flow. The series also improves the conformance
of the return values to the SECURITY2_FILE_AUTHENTICATION_HANDLER
prototype. The last two patches are actual bugfixes, with the last one
fixing the problem laid out above.
The patches in this posting have been formatted with
"--function-context", for easier review.
Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Thanks,
Laszlo
Laszlo Ersek (11):
SecurityPkg/DxeImageVerificationHandler: simplify "VerifyStatus"
SecurityPkg/DxeImageVerificationHandler: remove "else" after
return/break
SecurityPkg/DxeImageVerificationHandler: keep PE/COFF info status
internal
SecurityPkg/DxeImageVerificationHandler: narrow down PE/COFF hash
status
SecurityPkg/DxeImageVerificationHandler: fix retval on memalloc
failure
SecurityPkg/DxeImageVerificationHandler: remove superfluous Status
setting
SecurityPkg/DxeImageVerificationHandler: unnest AddImageExeInfo() call
SecurityPkg/DxeImageVerificationHandler: eliminate "Status" variable
SecurityPkg/DxeImageVerificationHandler: fix retval for
(FileBuffer==NULL)
SecurityPkg/DxeImageVerificationHandler: fix imgexec info on memalloc
fail
SecurityPkg/DxeImageVerificationHandler: fix "defer" vs. "deny"
policies
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 118 ++++++++++----------
1 file changed, 59 insertions(+), 59 deletions(-)
--
2.19.1.3.g30247aa5d201
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 01/11] SecurityPkg/DxeImageVerificationHandler: simplify "VerifyStatus"
2020-01-16 19:06 [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Laszlo Ersek
@ 2020-01-16 19:06 ` Laszlo Ersek
2020-01-16 19:06 ` [PATCH 02/11] SecurityPkg/DxeImageVerificationHandler: remove "else" after return/break Laszlo Ersek
` (10 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-16 19:06 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Chao Zhang, Jian J Wang, Jiewen Yao
In the DxeImageVerificationHandler() function, the "VerifyStatus" variable
can only contain one of two values: EFI_SUCCESS and EFI_ACCESS_DENIED.
Furthermore, the variable is only consumed with EFI_ERROR().
Therefore, using the EFI_STATUS type for the variable is unnecessary.
Worse, given the complex meanings of the function's return values, using
EFI_STATUS for "VerifyStatus" is actively confusing.
Rename the variable to "IsVerified", and make it a simple BOOLEAN.
This patch is a no-op, regarding behavior.
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
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index a0a12b50ddd1..5afd723adae8 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1556,349 +1556,349 @@ 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;
- EFI_STATUS VerifyStatus;
+ 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;
SignatureList = NULL;
SignatureListSize = 0;
WinCertificate = NULL;
SecDataDir = NULL;
PkcsCertData = NULL;
Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
Status = EFI_ACCESS_DENIED;
- VerifyStatus = EFI_ACCESS_DENIED;
+ 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;
} else 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;
}
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
//
Status = 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;
}
Status = EFI_ACCESS_DENIED;
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 Done;
}
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 Done;
}
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 Done;
}
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 Done;
}
//
// 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;
}
Status = HashPeImageByType (AuthData, AuthDataSize);
if (EFI_ERROR (Status)) {
continue;
}
//
// Check the digital signature against the revoked certificate in forbidden database (dbx).
//
if (IsForbiddenByDbx (AuthData, AuthDataSize)) {
Action = EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED;
- VerifyStatus = EFI_ACCESS_DENIED;
+ IsVerified = FALSE;
break;
}
//
// Check the digital signature against the valid certificate in allowed database (db).
//
- if (EFI_ERROR (VerifyStatus)) {
+ if (!IsVerified) {
if (IsAllowedByDb (AuthData, AuthDataSize)) {
- VerifyStatus = EFI_SUCCESS;
+ 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));
- VerifyStatus = EFI_ACCESS_DENIED;
+ IsVerified = FALSE;
break;
- } else if (EFI_ERROR (VerifyStatus)) {
+ } else if (!IsVerified) {
if (IsSignatureFoundInDatabase (EFI_IMAGE_SECURITY_DATABASE, mImageDigest, &mCertType, mImageDigestSize)) {
- VerifyStatus = EFI_SUCCESS;
+ 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.
//
- VerifyStatus = EFI_ACCESS_DENIED;
+ IsVerified = FALSE;
}
- if (!EFI_ERROR (VerifyStatus)) {
+ if (IsVerified) {
return EFI_SUCCESS;
} else {
Status = EFI_ACCESS_DENIED;
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) {
Status = EFI_OUT_OF_RESOURCES;
goto Done;
}
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);
}
}
Done:
if (Status != EFI_SUCCESS) {
//
// 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);
}
Status = EFI_SECURITY_VIOLATION;
}
if (SignatureList != NULL) {
FreePool (SignatureList);
}
return Status;
}
/**
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 02/11] SecurityPkg/DxeImageVerificationHandler: remove "else" after return/break
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 ` Laszlo Ersek
2020-01-16 19:06 ` [PATCH 03/11] SecurityPkg/DxeImageVerificationHandler: keep PE/COFF info status internal Laszlo Ersek
` (9 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-16 19:06 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Chao Zhang, Jian J Wang, Jiewen Yao
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 <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
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 41 ++++++++++----------
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.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;
SignatureList = NULL;
SignatureListSize = 0;
WinCertificate = NULL;
SecDataDir = NULL;
PkcsCertData = NULL;
Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
Status = EFI_ACCESS_DENIED;
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;
- } else if (Policy == NEVER_EXECUTE) {
+ }
+ 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;
}
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
//
Status = 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;
}
Status = EFI_ACCESS_DENIED;
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 Done;
}
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 Done;
}
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 Done;
}
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 Done;
}
//
// 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;
}
Status = HashPeImageByType (AuthData, AuthDataSize);
if (EFI_ERROR (Status)) {
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;
- } else if (!IsVerified) {
+ }
+ 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;
- } else {
- Status = EFI_ACCESS_DENIED;
- 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) {
- Status = EFI_OUT_OF_RESOURCES;
- goto Done;
- }
- 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);
+ }
+ Status = EFI_ACCESS_DENIED;
+ 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) {
+ Status = EFI_OUT_OF_RESOURCES;
+ goto Done;
}
+ 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);
}
Done:
if (Status != EFI_SUCCESS) {
//
// 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);
}
Status = EFI_SECURITY_VIOLATION;
}
if (SignatureList != NULL) {
FreePool (SignatureList);
}
return Status;
}
/**
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 03/11] SecurityPkg/DxeImageVerificationHandler: keep PE/COFF info status internal
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 ` Laszlo Ersek
2020-01-16 19:06 ` [PATCH 04/11] SecurityPkg/DxeImageVerificationHandler: narrow down PE/COFF hash status Laszlo Ersek
` (8 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-16 19:06 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Chao Zhang, Jian J Wang, Jiewen Yao
The PeCoffLoaderGetImageInfo() function may return various error codes,
such as RETURN_INVALID_PARAMETER and RETURN_UNSUPPORTED.
Such error values should not be assigned to our "Status" variable in the
DxeImageVerificationHandler() function, because "Status" generally stands
for the main exit value of the function. And
SECURITY2_FILE_AUTHENTICATION_HANDLER functions are expected to return one
of EFI_SUCCESS, EFI_SECURITY_VIOLATION, and EFI_ACCESS_DENIED only.
Introduce the "PeCoffStatus" helper variable for keeping the return value
of PeCoffLoaderGetImageInfo() internal to the function. If
PeCoffLoaderGetImageInfo() fails, we'll jump to the "Done" label with
"Status" being EFI_ACCESS_DENIED, inherited from the top of the function.
Note that this is consistent with the subsequent PE/COFF Signature check,
where we jump to the "Done" label with "Status" having been re-set to
EFI_ACCESS_DENIED.
As a consequence, we can at once remove the
Status = EFI_ACCESS_DENIED;
assignment right after the "PeCoffStatus" check.
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
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 8204c9c0f105..e6c8a5408752 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1556,350 +1556,349 @@ 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;
+ RETURN_STATUS PeCoffStatus;
SignatureList = NULL;
SignatureListSize = 0;
WinCertificate = NULL;
SecDataDir = NULL;
PkcsCertData = NULL;
Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
Status = EFI_ACCESS_DENIED;
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;
}
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
//
- Status = PeCoffLoaderGetImageInfo (&ImageContext);
- if (EFI_ERROR (Status)) {
+ 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 Done;
}
- Status = EFI_ACCESS_DENIED;
-
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 Done;
}
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 Done;
}
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 Done;
}
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 Done;
}
//
// 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;
}
Status = HashPeImageByType (AuthData, AuthDataSize);
if (EFI_ERROR (Status)) {
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;
}
Status = EFI_ACCESS_DENIED;
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) {
Status = EFI_OUT_OF_RESOURCES;
goto Done;
}
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);
}
Done:
if (Status != EFI_SUCCESS) {
//
// 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);
}
Status = EFI_SECURITY_VIOLATION;
}
if (SignatureList != NULL) {
FreePool (SignatureList);
}
return Status;
}
/**
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 04/11] SecurityPkg/DxeImageVerificationHandler: narrow down PE/COFF hash status
2020-01-16 19:06 [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Laszlo Ersek
` (2 preceding siblings ...)
2020-01-16 19:06 ` [PATCH 03/11] SecurityPkg/DxeImageVerificationHandler: keep PE/COFF info status internal Laszlo Ersek
@ 2020-01-16 19:06 ` Laszlo Ersek
2020-01-16 19:06 ` [PATCH 05/11] SecurityPkg/DxeImageVerificationHandler: fix retval on memalloc failure Laszlo Ersek
` (7 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-16 19:06 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Chao Zhang, Jian J Wang, Jiewen Yao
Inside the "for" loop that scans the signatures of the image, we call
HashPeImageByType(), and assign its return value to "Status".
Beyond the immediate retval check, this assignment is useless (never
consumed). That's because a subsequent access to "Status" may only be one
of the following:
- the "Status" assignment when we call HashPeImageByType() in the next
iteration of the loop,
- the "Status = EFI_ACCESS_DENIED" assignment right after the final
"IsVerified" check.
To make it clear that the assignment is only useful for the immediate
HashPeImageByType() retval check, introduce a specific helper variable,
called "HashStatus".
This patch is a no-op, functionally.
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
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index e6c8a5408752..5cc82c1b3b22 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;
RETURN_STATUS PeCoffStatus;
+ EFI_STATUS HashStatus;
SignatureList = NULL;
SignatureListSize = 0;
WinCertificate = NULL;
SecDataDir = NULL;
PkcsCertData = NULL;
Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
Status = EFI_ACCESS_DENIED;
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;
}
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 Done;
}
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 Done;
}
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 Done;
}
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 Done;
}
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 Done;
}
//
// 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;
}
- Status = HashPeImageByType (AuthData, AuthDataSize);
- if (EFI_ERROR (Status)) {
+ 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;
}
Status = EFI_ACCESS_DENIED;
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) {
Status = EFI_OUT_OF_RESOURCES;
goto Done;
}
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);
}
Done:
if (Status != EFI_SUCCESS) {
//
// 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);
}
Status = EFI_SECURITY_VIOLATION;
}
if (SignatureList != NULL) {
FreePool (SignatureList);
}
return Status;
}
/**
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 05/11] SecurityPkg/DxeImageVerificationHandler: fix retval on memalloc failure
2020-01-16 19:06 [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Laszlo Ersek
` (3 preceding siblings ...)
2020-01-16 19:06 ` [PATCH 04/11] SecurityPkg/DxeImageVerificationHandler: narrow down PE/COFF hash status Laszlo Ersek
@ 2020-01-16 19:06 ` Laszlo Ersek
2020-01-16 19:07 ` [PATCH 06/11] SecurityPkg/DxeImageVerificationHandler: remove superfluous Status setting Laszlo Ersek
` (6 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-16 19:06 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Chao Zhang, Jian J Wang, Jiewen Yao
A SECURITY2_FILE_AUTHENTICATION_HANDLER function is not expected to return
EFI_OUT_OF_RESOURCES. We should only return EFI_SUCCESS,
EFI_SECURITY_VIOLATION, or EFI_ACCESS_DENIED.
In case we run out of memory while preparing "SignatureList" for
AddImageExeInfo(), we should simply stick with the EFI_ACCESS_DENIED value
that is already in "Status" -- from just before the "Action" condition --,
and not suppress it with EFI_OUT_OF_RESOURCES.
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, 2 deletions(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 5cc82c1b3b22..5f09a66bc9ce 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1384,170 +1384,169 @@ BOOLEAN
IsAllowedByDb (
IN UINT8 *AuthData,
IN UINTN AuthDataSize
)
{
EFI_STATUS Status;
BOOLEAN VerifyStatus;
EFI_SIGNATURE_LIST *CertList;
EFI_SIGNATURE_DATA *CertData;
UINTN DataSize;
UINT8 *Data;
UINT8 *RootCert;
UINTN RootCertSize;
UINTN Index;
UINTN CertCount;
UINTN DbxDataSize;
UINT8 *DbxData;
EFI_TIME RevocationTime;
Data = NULL;
CertList = NULL;
CertData = NULL;
RootCert = NULL;
DbxData = NULL;
RootCertSize = 0;
VerifyStatus = FALSE;
DataSize = 0;
Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE, &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, NULL);
if (Status == EFI_BUFFER_TOO_SMALL) {
Data = (UINT8 *) AllocateZeroPool (DataSize);
if (Data == NULL) {
return VerifyStatus;
}
Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE, &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, (VOID *) Data);
if (EFI_ERROR (Status)) {
goto Done;
}
//
// Find X509 certificate in Signature List to verify the signature in pkcs7 signed data.
//
CertList = (EFI_SIGNATURE_LIST *) Data;
while ((DataSize > 0) && (DataSize >= CertList->SignatureListSize)) {
if (CompareGuid (&CertList->SignatureType, &gEfiCertX509Guid)) {
CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
CertCount = (CertList->SignatureListSize - sizeof (EFI_SIGNATURE_LIST) - CertList->SignatureHeaderSize) / CertList->SignatureSize;
for (Index = 0; Index < CertCount; Index++) {
//
// Iterate each Signature Data Node within this CertList for verify.
//
RootCert = CertData->SignatureData;
RootCertSize = CertList->SignatureSize - sizeof (EFI_GUID);
//
// Call AuthenticodeVerify library to Verify Authenticode struct.
//
VerifyStatus = AuthenticodeVerify (
AuthData,
AuthDataSize,
RootCert,
RootCertSize,
mImageDigest,
mImageDigestSize
);
if (VerifyStatus) {
//
// Here We still need to check if this RootCert's Hash is revoked
//
Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, NULL);
if (Status == EFI_BUFFER_TOO_SMALL) {
goto Done;
}
DbxData = (UINT8 *) AllocateZeroPool (DbxDataSize);
if (DbxData == NULL) {
goto Done;
}
Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, (VOID *) DbxData);
if (EFI_ERROR (Status)) {
goto Done;
}
if (IsCertHashFoundInDatabase (RootCert, RootCertSize, (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {
//
// Check the timestamp signature and signing time to determine if the RootCert can be trusted.
//
VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime);
if (!VerifyStatus) {
DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed and signature is accepted by DB, but its root cert failed the timestamp check.\n"));
}
}
goto Done;
}
CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertData + CertList->SignatureSize);
}
}
DataSize -= CertList->SignatureListSize;
CertList = (EFI_SIGNATURE_LIST *) ((UINT8 *) CertList + CertList->SignatureListSize);
}
}
Done:
if (VerifyStatus) {
SecureBootHook (EFI_IMAGE_SECURITY_DATABASE, &gEfiImageSecurityDatabaseGuid, CertList->SignatureSize, CertData);
}
if (Data != NULL) {
FreePool (Data);
}
if (DbxData != NULL) {
FreePool (DbxData);
}
return VerifyStatus;
}
/**
Provide verification service for signed images, which include both signature validation
and platform policy control. For signature types, both UEFI WIN_CERTIFICATE_UEFI_GUID and
MSFT Authenticode type signatures are supported.
In this implementation, only verify external executables when in USER MODE.
Executables from FV is bypass, so pass in AuthenticationStatus is ignored.
The image verification policy is:
If the image is signed,
At least one valid signature or at least one hash value of the image must match a record
in the security database "db", and no valid signature nor any hash value of the image may
be reflected in the security database "dbx".
Otherwise, the 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".
Caution: This function may receive untrusted input.
PE/COFF image is external input, so this function will validate its data structure
within this image buffer before use.
@param[in] AuthenticationStatus
This is the authentication status returned from the security
measurement services for the input file.
@param[in] File This is a pointer to the device path of the file that is
being dispatched. This will optionally be used for logging.
@param[in] FileBuffer File buffer matches the input file device path.
@param[in] FileSize Size of File buffer matches the input file device path.
@param[in] BootPolicy A boot policy that was used to call LoadImage() UEFI service.
@retval EFI_SUCCESS The file specified by DevicePath and non-NULL
FileBuffer did authenticate, and the platform policy dictates
that the DXE Foundation may use the file.
@retval EFI_SUCCESS The device path specified by NULL device path DevicePath
and non-NULL FileBuffer did authenticate, and the platform
policy dictates that the DXE Foundation may execute the image in
FileBuffer.
- @retval EFI_OUT_RESOURCE Fail to allocate memory.
@retval EFI_SECURITY_VIOLATION The file specified by File did not authenticate, and
the platform policy dictates that File should be placed
in the untrusted state. The image has been added to the file
execution table.
@retval EFI_ACCESS_DENIED The file specified by File and FileBuffer did not
authenticate, and the platform policy dictates that the DXE
Foundation many not use File.
**/
@@ -1556,350 +1555,349 @@ 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;
RETURN_STATUS PeCoffStatus;
EFI_STATUS HashStatus;
SignatureList = NULL;
SignatureListSize = 0;
WinCertificate = NULL;
SecDataDir = NULL;
PkcsCertData = NULL;
Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
Status = EFI_ACCESS_DENIED;
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;
}
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 Done;
}
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 Done;
}
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 Done;
}
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 Done;
}
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 Done;
}
//
// 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;
}
Status = EFI_ACCESS_DENIED;
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) {
- Status = EFI_OUT_OF_RESOURCES;
goto Done;
}
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);
}
Done:
if (Status != EFI_SUCCESS) {
//
// 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);
}
Status = EFI_SECURITY_VIOLATION;
}
if (SignatureList != NULL) {
FreePool (SignatureList);
}
return Status;
}
/**
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 06/11] SecurityPkg/DxeImageVerificationHandler: remove superfluous Status setting
2020-01-16 19:06 [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Laszlo Ersek
` (4 preceding siblings ...)
2020-01-16 19:06 ` [PATCH 05/11] SecurityPkg/DxeImageVerificationHandler: fix retval on memalloc failure Laszlo Ersek
@ 2020-01-16 19:07 ` Laszlo Ersek
2020-01-16 19:07 ` [PATCH 07/11] SecurityPkg/DxeImageVerificationHandler: unnest AddImageExeInfo() call Laszlo Ersek
` (5 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-16 19:07 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Chao Zhang, Jian J Wang, Jiewen Yao
After the final "IsVerified" check, we set "Status" to EFI_ACCESS_DENIED.
This is superfluous, as "Status" already carries EFI_ACCESS_DENIED value
there, from the top of the function. Remove the assignment.
Functionally, this change is a no-op.
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
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 5f09a66bc9ce..6ccce1f35843 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1555,349 +1555,348 @@ 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;
RETURN_STATUS PeCoffStatus;
EFI_STATUS HashStatus;
SignatureList = NULL;
SignatureListSize = 0;
WinCertificate = NULL;
SecDataDir = NULL;
PkcsCertData = NULL;
Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
Status = EFI_ACCESS_DENIED;
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;
}
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 Done;
}
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 Done;
}
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 Done;
}
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 Done;
}
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 Done;
}
//
// 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;
}
- Status = EFI_ACCESS_DENIED;
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 Done;
}
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);
}
Done:
if (Status != EFI_SUCCESS) {
//
// 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);
}
Status = EFI_SECURITY_VIOLATION;
}
if (SignatureList != NULL) {
FreePool (SignatureList);
}
return Status;
}
/**
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 07/11] SecurityPkg/DxeImageVerificationHandler: unnest AddImageExeInfo() call
2020-01-16 19:06 [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Laszlo Ersek
` (5 preceding siblings ...)
2020-01-16 19:07 ` [PATCH 06/11] SecurityPkg/DxeImageVerificationHandler: remove superfluous Status setting Laszlo Ersek
@ 2020-01-16 19:07 ` Laszlo Ersek
2020-01-16 19:07 ` [PATCH 08/11] SecurityPkg/DxeImageVerificationHandler: eliminate "Status" variable Laszlo Ersek
` (4 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-16 19:07 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Chao Zhang, Jian J Wang, Jiewen Yao
Before the "Done" label at the end of DxeImageVerificationHandler(), we
now have a single access to "Status": we set "Status" to EFI_ACCESS_DENIED
at the top of the function. Therefore, the (Status != EFI_SUCCESS)
condition is always true under the "Done" label.
Accordingly, unnest the AddImageExeInfo() call dependent on that
condition, remove the condition, and also rename the "Done" label to
"Failed".
Functionally, this patch is a no-op. It's easier to review with:
git show -b -W
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
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 34 +++++++++-----------
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 6ccce1f35843..b98404ab465b 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1555,348 +1555,346 @@ 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;
RETURN_STATUS PeCoffStatus;
EFI_STATUS HashStatus;
SignatureList = NULL;
SignatureListSize = 0;
WinCertificate = NULL;
SecDataDir = NULL;
PkcsCertData = NULL;
Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
Status = EFI_ACCESS_DENIED;
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;
}
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 Done;
+ 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 Done;
+ 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 Done;
+ 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 Done;
+ 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 Done;
+ 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 Done;
+ 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);
}
-Done:
- if (Status != EFI_SUCCESS) {
- //
- // 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);
- }
- Status = EFI_SECURITY_VIOLATION;
+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);
}
+ Status = EFI_SECURITY_VIOLATION;
if (SignatureList != NULL) {
FreePool (SignatureList);
}
return Status;
}
/**
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 08/11] SecurityPkg/DxeImageVerificationHandler: eliminate "Status" variable
2020-01-16 19:06 [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Laszlo Ersek
` (6 preceding siblings ...)
2020-01-16 19:07 ` [PATCH 07/11] SecurityPkg/DxeImageVerificationHandler: unnest AddImageExeInfo() call Laszlo Ersek
@ 2020-01-16 19:07 ` Laszlo Ersek
2020-01-16 19:07 ` [PATCH 09/11] SecurityPkg/DxeImageVerificationHandler: fix retval for (FileBuffer==NULL) Laszlo Ersek
` (3 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-16 19:07 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Chao Zhang, Jian J Wang, Jiewen Yao
The "Status" variable is set to EFI_ACCESS_DENIED at the top of the
function. Then it is overwritten with EFI_SECURITY_VIOLATION under the
"Failed" (earlier: "Done") label. We finally return "Status".
The above covers the complete usage of "Status" in
DxeImageVerificationHandler(). Remove the variable, and simply return
EFI_SECURITY_VIOLATION in the end.
This patch is a no-op, regarding behavior.
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
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index b98404ab465b..3041c8718beb 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1555,346 +1555,343 @@ 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;
RETURN_STATUS PeCoffStatus;
EFI_STATUS HashStatus;
SignatureList = NULL;
SignatureListSize = 0;
WinCertificate = NULL;
SecDataDir = NULL;
PkcsCertData = NULL;
Action = EFI_IMAGE_EXECUTION_AUTH_UNTESTED;
- Status = EFI_ACCESS_DENIED;
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;
}
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);
}
- Status = EFI_SECURITY_VIOLATION;
if (SignatureList != NULL) {
FreePool (SignatureList);
}
- return Status;
+ 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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 09/11] SecurityPkg/DxeImageVerificationHandler: fix retval for (FileBuffer==NULL)
2020-01-16 19:06 [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Laszlo Ersek
` (7 preceding siblings ...)
2020-01-16 19:07 ` [PATCH 08/11] SecurityPkg/DxeImageVerificationHandler: eliminate "Status" variable Laszlo Ersek
@ 2020-01-16 19:07 ` Laszlo Ersek
2020-01-16 19:07 ` [PATCH 10/11] SecurityPkg/DxeImageVerificationHandler: fix imgexec info on memalloc fail Laszlo Ersek
` (2 subsequent siblings)
11 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-16 19:07 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Chao Zhang, Jian J Wang, Jiewen Yao
"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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 10/11] SecurityPkg/DxeImageVerificationHandler: fix imgexec info on memalloc fail
2020-01-16 19:06 [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Laszlo Ersek
` (8 preceding siblings ...)
2020-01-16 19:07 ` [PATCH 09/11] SecurityPkg/DxeImageVerificationHandler: fix retval for (FileBuffer==NULL) Laszlo Ersek
@ 2020-01-16 19:07 ` 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
11 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-16 19:07 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Chao Zhang, Jian J Wang, Jiewen Yao
It makes no sense to call AddImageExeInfo() with (Signature == NULL) and
(SignatureSize > 0). AddImageExeInfo() does not crash in such a case -- it
avoids the CopyMem() call --, but it creates an invalid
EFI_IMAGE_EXECUTION_INFO record. Namely, the
"EFI_IMAGE_EXECUTION_INFO.InfoSize" field includes "SignatureSize", but
the actual signature bytes are not filled in.
Document and ASSERT() this condition in AddImageExeInfo().
In DxeImageVerificationHandler(), zero out "SignatureListSize" if we set
"SignatureList" to NULL due to AllocateZeroPool() failure.
(Another approach could be to avoid calling AddImageExeInfo() completely,
in case AllocateZeroPool() fails. Unfortunately, the UEFI v2.8 spec does
not seem to state clearly whether a signature is mandatory in
EFI_IMAGE_EXECUTION_INFO, if the "Action" field is
EFI_IMAGE_EXECUTION_AUTH_SIG_FAILED or EFI_IMAGE_EXECUTION_AUTH_SIG_FOUND.
For now, the EFI_IMAGE_EXECUTION_INFO addition logic is not changed; we
only make sure that the record we add is not malformed.)
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
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index f6ad5931be07..53ef340c08ad 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -678,151 +678,152 @@ UINTN
GetImageExeInfoTableSize (
EFI_IMAGE_EXECUTION_INFO_TABLE *ImageExeInfoTable
)
{
UINTN Index;
EFI_IMAGE_EXECUTION_INFO *ImageExeInfoItem;
UINTN TotalSize;
if (ImageExeInfoTable == NULL) {
return 0;
}
ImageExeInfoItem = (EFI_IMAGE_EXECUTION_INFO *) ((UINT8 *) ImageExeInfoTable + sizeof (EFI_IMAGE_EXECUTION_INFO_TABLE));
TotalSize = sizeof (EFI_IMAGE_EXECUTION_INFO_TABLE);
for (Index = 0; Index < ImageExeInfoTable->NumberOfImages; Index++) {
TotalSize += ReadUnaligned32 ((UINT32 *) &ImageExeInfoItem->InfoSize);
ImageExeInfoItem = (EFI_IMAGE_EXECUTION_INFO *) ((UINT8 *) ImageExeInfoItem + ReadUnaligned32 ((UINT32 *) &ImageExeInfoItem->InfoSize));
}
return TotalSize;
}
/**
Create an Image Execution Information Table entry and add it to system configuration table.
@param[in] Action Describes the action taken by the firmware regarding this image.
@param[in] Name Input a null-terminated, user-friendly name.
@param[in] DevicePath Input device path pointer.
@param[in] Signature Input signature info in EFI_SIGNATURE_LIST data structure.
- @param[in] SignatureSize Size of signature.
+ @param[in] SignatureSize Size of signature. Must be zero if Signature is NULL.
**/
VOID
AddImageExeInfo (
IN EFI_IMAGE_EXECUTION_ACTION Action,
IN CHAR16 *Name OPTIONAL,
IN CONST EFI_DEVICE_PATH_PROTOCOL *DevicePath,
IN EFI_SIGNATURE_LIST *Signature OPTIONAL,
IN UINTN SignatureSize
)
{
EFI_IMAGE_EXECUTION_INFO_TABLE *ImageExeInfoTable;
EFI_IMAGE_EXECUTION_INFO_TABLE *NewImageExeInfoTable;
EFI_IMAGE_EXECUTION_INFO *ImageExeInfoEntry;
UINTN ImageExeInfoTableSize;
UINTN NewImageExeInfoEntrySize;
UINTN NameStringLen;
UINTN DevicePathSize;
CHAR16 *NameStr;
ImageExeInfoTable = NULL;
NewImageExeInfoTable = NULL;
ImageExeInfoEntry = NULL;
NameStringLen = 0;
NameStr = NULL;
if (DevicePath == NULL) {
return ;
}
if (Name != NULL) {
NameStringLen = StrSize (Name);
} else {
NameStringLen = sizeof (CHAR16);
}
EfiGetSystemConfigurationTable (&gEfiImageSecurityDatabaseGuid, (VOID **) &ImageExeInfoTable);
if (ImageExeInfoTable != NULL) {
//
// The table has been found!
// We must enlarge the table to accommodate the new exe info entry.
//
ImageExeInfoTableSize = GetImageExeInfoTableSize (ImageExeInfoTable);
} else {
//
// Not Found!
// We should create a new table to append to the configuration table.
//
ImageExeInfoTableSize = sizeof (EFI_IMAGE_EXECUTION_INFO_TABLE);
}
DevicePathSize = GetDevicePathSize (DevicePath);
//
// Signature size can be odd. Pad after signature to ensure next EXECUTION_INFO entry align
//
+ ASSERT (Signature != NULL || SignatureSize == 0);
NewImageExeInfoEntrySize = sizeof (EFI_IMAGE_EXECUTION_INFO) + NameStringLen + DevicePathSize + SignatureSize;
NewImageExeInfoTable = (EFI_IMAGE_EXECUTION_INFO_TABLE *) AllocateRuntimePool (ImageExeInfoTableSize + NewImageExeInfoEntrySize);
if (NewImageExeInfoTable == NULL) {
return ;
}
if (ImageExeInfoTable != NULL) {
CopyMem (NewImageExeInfoTable, ImageExeInfoTable, ImageExeInfoTableSize);
} else {
NewImageExeInfoTable->NumberOfImages = 0;
}
NewImageExeInfoTable->NumberOfImages++;
ImageExeInfoEntry = (EFI_IMAGE_EXECUTION_INFO *) ((UINT8 *) NewImageExeInfoTable + ImageExeInfoTableSize);
//
// Update new item's information.
//
WriteUnaligned32 ((UINT32 *) ImageExeInfoEntry, Action);
WriteUnaligned32 ((UINT32 *) ((UINT8 *) ImageExeInfoEntry + sizeof (EFI_IMAGE_EXECUTION_ACTION)), (UINT32) NewImageExeInfoEntrySize);
NameStr = (CHAR16 *)(ImageExeInfoEntry + 1);
if (Name != NULL) {
CopyMem ((UINT8 *) NameStr, Name, NameStringLen);
} else {
ZeroMem ((UINT8 *) NameStr, sizeof (CHAR16));
}
CopyMem (
(UINT8 *) NameStr + NameStringLen,
DevicePath,
DevicePathSize
);
if (Signature != NULL) {
CopyMem (
(UINT8 *) NameStr + NameStringLen + DevicePathSize,
Signature,
SignatureSize
);
}
//
// Update/replace the image execution table.
//
gBS->InstallConfigurationTable (&gEfiImageSecurityDatabaseGuid, (VOID *) NewImageExeInfoTable);
//
// Free Old table data!
//
if (ImageExeInfoTable != NULL) {
FreePool (ImageExeInfoTable);
}
}
/**
Check whether the hash of an given X.509 certificate is in forbidden database (DBX).
@param[in] Certificate Pointer to X.509 Certificate that is searched for.
@param[in] CertSize Size of X.509 Certificate.
@param[in] SignatureList Pointer to the Signature List in forbidden database.
@param[in] SignatureListSize Size of Signature List.
@param[out] RevocationTime Return the time that the certificate was revoked.
@return TRUE The certificate hash is found in the forbidden database.
@return FALSE The certificate hash is not found in the forbidden database.
**/
@@ -1555,343 +1556,344 @@ 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_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) {
+ SignatureListSize = 0;
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 11/11] SecurityPkg/DxeImageVerificationHandler: fix "defer" vs. "deny" policies
2020-01-16 19:06 [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Laszlo Ersek
` (9 preceding siblings ...)
2020-01-16 19:07 ` [PATCH 10/11] SecurityPkg/DxeImageVerificationHandler: fix imgexec info on memalloc fail Laszlo Ersek
@ 2020-01-16 19:07 ` Laszlo Ersek
2020-01-31 2:59 ` [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Michael D Kinney
11 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-16 19:07 UTC (permalink / raw)
To: edk2-devel-groups-io; +Cc: Chao Zhang, Jian J Wang, Jiewen Yao
In DxeImageVerificationHandler(), we should return EFI_SECURITY_VIOLATION
for a rejected image only if the platform sets
DEFER_EXECUTE_ON_SECURITY_VIOLATION as the policy for the image's source.
Otherwise, EFI_ACCESS_DENIED must be returned.
Right now, EFI_SECURITY_VIOLATION is returned for all rejected images,
which is wrong -- it causes LoadImage() to hold on to rejected images (in
untrusted state), for further platform actions. However, if a platform
already set DENY_EXECUTE_ON_SECURITY_VIOLATION, the platform will not
expect the rejected image to stick around in memory (regardless of its
untrusted state).
Therefore, adhere to the platform policy in the return value of the
DxeImageVerificationHandler() function.
Furthermore, according to "32.4.2 Image Execution Information Table" in
the UEFI v2.8 spec, and considering that edk2 only supports (AuditMode==0)
at the moment:
> When AuditMode==0, if the image's signature is not found in the
> authorized database, or is found in the forbidden database, the image
> will not be started and instead, information about it will be placed in
> this table.
we have to store an EFI_IMAGE_EXECUTION_INFO record in both the "defer"
case and the "deny" case. Thus, the AddImageExeInfo() call is not being
made conditional on (Policy == DEFER_EXECUTE_ON_SECURITY_VIOLATION); the
documentation is updated instead.
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: 5db28a6753d307cdfb1cfdeb2f63739a9f959837
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
index 53ef340c08ad..ff79e30ef83e 100644
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
+++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -1385,170 +1385,171 @@ BOOLEAN
IsAllowedByDb (
IN UINT8 *AuthData,
IN UINTN AuthDataSize
)
{
EFI_STATUS Status;
BOOLEAN VerifyStatus;
EFI_SIGNATURE_LIST *CertList;
EFI_SIGNATURE_DATA *CertData;
UINTN DataSize;
UINT8 *Data;
UINT8 *RootCert;
UINTN RootCertSize;
UINTN Index;
UINTN CertCount;
UINTN DbxDataSize;
UINT8 *DbxData;
EFI_TIME RevocationTime;
Data = NULL;
CertList = NULL;
CertData = NULL;
RootCert = NULL;
DbxData = NULL;
RootCertSize = 0;
VerifyStatus = FALSE;
DataSize = 0;
Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE, &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, NULL);
if (Status == EFI_BUFFER_TOO_SMALL) {
Data = (UINT8 *) AllocateZeroPool (DataSize);
if (Data == NULL) {
return VerifyStatus;
}
Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE, &gEfiImageSecurityDatabaseGuid, NULL, &DataSize, (VOID *) Data);
if (EFI_ERROR (Status)) {
goto Done;
}
//
// Find X509 certificate in Signature List to verify the signature in pkcs7 signed data.
//
CertList = (EFI_SIGNATURE_LIST *) Data;
while ((DataSize > 0) && (DataSize >= CertList->SignatureListSize)) {
if (CompareGuid (&CertList->SignatureType, &gEfiCertX509Guid)) {
CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertList + sizeof (EFI_SIGNATURE_LIST) + CertList->SignatureHeaderSize);
CertCount = (CertList->SignatureListSize - sizeof (EFI_SIGNATURE_LIST) - CertList->SignatureHeaderSize) / CertList->SignatureSize;
for (Index = 0; Index < CertCount; Index++) {
//
// Iterate each Signature Data Node within this CertList for verify.
//
RootCert = CertData->SignatureData;
RootCertSize = CertList->SignatureSize - sizeof (EFI_GUID);
//
// Call AuthenticodeVerify library to Verify Authenticode struct.
//
VerifyStatus = AuthenticodeVerify (
AuthData,
AuthDataSize,
RootCert,
RootCertSize,
mImageDigest,
mImageDigestSize
);
if (VerifyStatus) {
//
// Here We still need to check if this RootCert's Hash is revoked
//
Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, NULL);
if (Status == EFI_BUFFER_TOO_SMALL) {
goto Done;
}
DbxData = (UINT8 *) AllocateZeroPool (DbxDataSize);
if (DbxData == NULL) {
goto Done;
}
Status = gRT->GetVariable (EFI_IMAGE_SECURITY_DATABASE1, &gEfiImageSecurityDatabaseGuid, NULL, &DbxDataSize, (VOID *) DbxData);
if (EFI_ERROR (Status)) {
goto Done;
}
if (IsCertHashFoundInDatabase (RootCert, RootCertSize, (EFI_SIGNATURE_LIST *)DbxData, DbxDataSize, &RevocationTime)) {
//
// Check the timestamp signature and signing time to determine if the RootCert can be trusted.
//
VerifyStatus = PassTimestampCheck (AuthData, AuthDataSize, &RevocationTime);
if (!VerifyStatus) {
DEBUG ((DEBUG_INFO, "DxeImageVerificationLib: Image is signed and signature is accepted by DB, but its root cert failed the timestamp check.\n"));
}
}
goto Done;
}
CertData = (EFI_SIGNATURE_DATA *) ((UINT8 *) CertData + CertList->SignatureSize);
}
}
DataSize -= CertList->SignatureListSize;
CertList = (EFI_SIGNATURE_LIST *) ((UINT8 *) CertList + CertList->SignatureListSize);
}
}
Done:
if (VerifyStatus) {
SecureBootHook (EFI_IMAGE_SECURITY_DATABASE, &gEfiImageSecurityDatabaseGuid, CertList->SignatureSize, CertData);
}
if (Data != NULL) {
FreePool (Data);
}
if (DbxData != NULL) {
FreePool (DbxData);
}
return VerifyStatus;
}
/**
Provide verification service for signed images, which include both signature validation
and platform policy control. For signature types, both UEFI WIN_CERTIFICATE_UEFI_GUID and
MSFT Authenticode type signatures are supported.
In this implementation, only verify external executables when in USER MODE.
Executables from FV is bypass, so pass in AuthenticationStatus is ignored.
The image verification policy is:
If the image is signed,
At least one valid signature or at least one hash value of the image must match a record
in the security database "db", and no valid signature nor any hash value of the image may
be reflected in the security database "dbx".
Otherwise, the 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".
Caution: This function may receive untrusted input.
PE/COFF image is external input, so this function will validate its data structure
within this image buffer before use.
@param[in] AuthenticationStatus
This is the authentication status returned from the security
measurement services for the input file.
@param[in] File This is a pointer to the device path of the file that is
being dispatched. This will optionally be used for logging.
@param[in] FileBuffer File buffer matches the input file device path.
@param[in] FileSize Size of File buffer matches the input file device path.
@param[in] BootPolicy A boot policy that was used to call LoadImage() UEFI service.
@retval EFI_SUCCESS The file specified by DevicePath and non-NULL
FileBuffer did authenticate, and the platform policy dictates
that the DXE Foundation may use the file.
@retval EFI_SUCCESS The device path specified by NULL device path DevicePath
and non-NULL FileBuffer did authenticate, and the platform
policy dictates that the DXE Foundation may execute the image in
FileBuffer.
@retval EFI_SECURITY_VIOLATION The file specified by File did not authenticate, and
the platform policy dictates that File should be placed
in the untrusted state. The image has been added to the file
execution table.
@retval EFI_ACCESS_DENIED The file specified by File and FileBuffer did not
authenticate, and the platform policy dictates that the DXE
- Foundation many not use File.
+ Foundation may not use File. The image has
+ been added to the file execution table.
**/
EFI_STATUS
@@ -1556,344 +1557,348 @@ 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_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) {
SignatureListSize = 0;
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.
+ // Policy decides to defer or reject the image; add its information in image
+ // executable information table in either case.
//
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;
+ if (Policy == DEFER_EXECUTE_ON_SECURITY_VIOLATION) {
+ return EFI_SECURITY_VIOLATION;
+ }
+ return EFI_ACCESS_DENIED;
}
/**
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy
2020-01-16 19:06 [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Laszlo Ersek
` (10 preceding siblings ...)
2020-01-16 19:07 ` [PATCH 11/11] SecurityPkg/DxeImageVerificationHandler: fix "defer" vs. "deny" policies Laszlo Ersek
@ 2020-01-31 2:59 ` Michael D Kinney
2020-01-31 8:12 ` Laszlo Ersek
11 siblings, 1 reply; 27+ messages in thread
From: Michael D Kinney @ 2020-01-31 2:59 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com, Kinney, Michael D
Cc: Zhang, Chao B, Wang, Jian J, Yao, Jiewen
Hi Laszlo,
I have reviewed this patch series. All the patches look
good. The detailed description of each change made it
easy to review.
Series Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
I have one question about that is not directly related to
this logic change.
I see this logic that checks for invalid policy settings and
ASSERT()s and halts in a deadloop if either of 2 specific values
are used that have been removed from the UEFI Spec.
//
// 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 ();
}
There are 3 conditions where the Policy comes from a PCD.
//
// 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;
}
However, there are no checks in this function to verify that
Policy is set to one of the allowed values. This means
an invalid PCD value will fall through to the EFI_ACCESS_DENIED
case. Is this the expected behavior for an invalid PCD setting?
If so, then why is there a check for the retired values from
the UEFI Spec and a deadloop performed. That seems inconsistent
and not a good idea to deadloop if we do not have to. Would
it be better to return EFI_ACCESS_DENIED for these 2 retired
Policy values as well?
I am ok if you consider this to be a different issue.
Thanks,
Mike
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Laszlo Ersek
> Sent: Thursday, January 16, 2020 11:07 AM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Wang, Jian
> J <jian.j.wang@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Subject: [edk2-devel] [PATCH 00/11]
> SecurityPkg/DxeImageVerificationHandler: fix retval for
> "deny" policy
>
> Ref:
> https://bugzilla.tianocore.org/show_bug.cgi?id=2129
> Repo: https://github.com/lersek/edk2.git
> Branch: deny_execute_2129
>
> The DxeImageVerificationHandler() function does not
> handle the
> DENY_EXECUTE_ON_SECURITY_VIOLATION policy correctly.
> When an image is
> rejected, and the platform sets this policy for the
> corresponding image
> source, the function should return EFI_ACCESS_DENIED.
> Instead, the
> function currently returns EFI_SECURITY_VIOLATION. The
> consequence is
> that gBS->LoadImage() will keep the image loaded (in
> untrusted state),
> rather than unloading it immediately. If the platform
> sets the
> DENY_EXECUTE_ON_SECURITY_VIOLATION policy for all image
> sources, then
> the platform may not expect EFI_SECURITY_VIOLATION at
> all. Then,
> rejected images may linger in RAM, in untrusted state,
> and may be leaked
> forever.
>
> This series refactors the DxeImageVerificationHandler()
> function,
> simplifying the control flow. The series also improves
> the conformance
> of the return values to the
> SECURITY2_FILE_AUTHENTICATION_HANDLER
> prototype. The last two patches are actual bugfixes,
> with the last one
> fixing the problem laid out above.
>
> The patches in this posting have been formatted with
> "--function-context", for easier review.
>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
>
> Thanks,
> Laszlo
>
> Laszlo Ersek (11):
> SecurityPkg/DxeImageVerificationHandler: simplify
> "VerifyStatus"
> SecurityPkg/DxeImageVerificationHandler: remove
> "else" after
> return/break
> SecurityPkg/DxeImageVerificationHandler: keep PE/COFF
> info status
> internal
> SecurityPkg/DxeImageVerificationHandler: narrow down
> PE/COFF hash
> status
> SecurityPkg/DxeImageVerificationHandler: fix retval
> on memalloc
> failure
> SecurityPkg/DxeImageVerificationHandler: remove
> superfluous Status
> setting
> SecurityPkg/DxeImageVerificationHandler: unnest
> AddImageExeInfo() call
> SecurityPkg/DxeImageVerificationHandler: eliminate
> "Status" variable
> SecurityPkg/DxeImageVerificationHandler: fix retval
> for
> (FileBuffer==NULL)
> SecurityPkg/DxeImageVerificationHandler: fix imgexec
> info on memalloc
> fail
> SecurityPkg/DxeImageVerificationHandler: fix "defer"
> vs. "deny"
> policies
>
>
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVer
> ificationLib.c | 118 ++++++++++----------
> 1 file changed, 59 insertions(+), 59 deletions(-)
>
> --
> 2.19.1.3.g30247aa5d201
>
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy
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 16:31 ` [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Michael D Kinney
0 siblings, 2 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-31 8:12 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Zhang, Chao B, Wang, Jian J, Yao, Jiewen
On 01/31/20 03:59, Kinney, Michael D wrote:
> Hi Laszlo,
>
> I have reviewed this patch series. All the patches look good. The
> detailed description of each change made it easy to review.
>
> Series Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
Thank you very much!
(more below)
>
> I have one question about that is not directly related to this logic
> change.
>
> I see this logic that checks for invalid policy settings and ASSERT()s
> and halts in a deadloop if either of 2 specific values are used that
> have been removed from the UEFI Spec.
>
> //
> // 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 ();
> }
>
> There are 3 conditions where the Policy comes from a PCD.
>
> //
> // 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;
> }
>
> However, there are no checks in this function to verify that Policy is
> set to one of the allowed values.
That's right. I seem to recall that I noticed it too, and I wrote it off
with "you can do damage with a bunch of other PCDs as well, if you
misconfigure them".
> This means an invalid PCD value will fall through to the
> EFI_ACCESS_DENIED case.
Yes. I find that the safest (silent) fallback for an undefined (per DEC)
PCD value.
> Is this the expected behavior for an invalid PCD setting? If so, then
> why is there a check for the retired values from the UEFI Spec and a
> deadloop performed. That seems inconsistent and not a good idea to
> deadloop if we do not have to. Would it be better to return
> EFI_ACCESS_DENIED for these 2 retired Policy values as well?
Hmm, good point.
I think these scenarios are different, historically. In one case we have
a plain misconfigured platform. In the other case we have a platform
that used to have correct configuration (considering edk2 in itself),
but then for spec conformance reasons, it got invalidated
*retroactively*. And I guess we wanted to be as loud as possible about
the second case. There's a difference between "you didn't do your
homework" and "you did your homework but we've changed the curriculum
meanwhile".
So the main question is if we want to remain "silent" about "plain
misconfig", vs. "loud" about "obsolete config".
We could unify the handling of both cases ("loudly") like this, for
example:
> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index ff79e30ef83e..1d41161abedc 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1617,23 +1617,32 @@ DxeImageVerificationHandler (
> Policy = DENY_EXECUTE_ON_SECURITY_VIOLATION;
> break;
> }
> +
> + switch (Policy) {
> //
> // If policy is always/never execute, return directly.
> //
> - if (Policy == ALWAYS_EXECUTE) {
> + case ALWAYS_EXECUTE:
> return EFI_SUCCESS;
> - }
> - if (Policy == NEVER_EXECUTE) {
> + case 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.
> + // "Defer" and "deny" are valid policies that require actual verification.
> //
> - 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) {
> + case DEFER_EXECUTE_ON_SECURITY_VIOLATION:
> + case DENY_EXECUTE_ON_SECURITY_VIOLATION:
> + break;
> +
> + //
> + // QUERY_USER_ON_SECURITY_VIOLATION and ALLOW_EXECUTE_ON_SECURITY_VIOLATION
> + // violate the UEFI spec; they now indicate platform misconfig. Hang here if
> + // we find those policies. Handle undefined policy values the same way.
> + //
> + default:
> + ASSERT (FALSE);
> CpuDeadLoop ();
> + return EFI_ACCESS_DENIED;
> }
>
> GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, (VOID**)&SecureBoot, NULL);
Continuing:
On 01/31/20 03:59, Kinney, Michael D wrote:
> I am ok if you consider this to be a different issue.
Yes, TianoCore#2129 is about mistaking DENY for DEFER, while this other
topic is "complain loudly, rather than silently, about invalid PCD
settings".
So let me push this series as-is for TianoCore#2129, with your R-b
applied. Then I will submit the above patch for separate review -- I'll
put something like "hang on undefined image verification policy PCDs" in
the subject.
Would you like me to file a separate BZ too, for that patch?
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy
2020-01-31 8:12 ` Laszlo Ersek
@ 2020-01-31 9:28 ` Laszlo Ersek
2020-01-31 10:01 ` Laszlo Ersek
` (2 more replies)
2020-01-31 16:31 ` [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Michael D Kinney
1 sibling, 3 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-31 9:28 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Zhang, Chao B, Wang, Jian J, Yao, Jiewen
Hi Mike,
On 01/31/20 09:12, Laszlo Ersek wrote:
> So let me push this series as-is for TianoCore#2129, with your R-b
> applied.
My pull request (with the "push" label set) seems to have stalled. The
checks have passed (twice -- I closed and reopened the PR once, to
re-trigger mergify), but the branch is not being merged.
https://github.com/tianocore/edk2/pull/324
Thanks
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy
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
2 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-31 10:01 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Zhang, Chao B, Wang, Jian J, Yao, Jiewen
On 01/31/20 10:28, Laszlo Ersek wrote:
> Hi Mike,
>
> On 01/31/20 09:12, Laszlo Ersek wrote:
>
>> So let me push this series as-is for TianoCore#2129, with your R-b
>> applied.
>
> My pull request (with the "push" label set) seems to have stalled. The
> checks have passed (twice -- I closed and reopened the PR once, to
> re-trigger mergify), but the branch is not being merged.
>
> https://github.com/tianocore/edk2/pull/324
BTW, here are the changes between the posted & reviewed series, and the
pull request:
- I had to replace an EFI_D_INFO macro with DEBUG_INFO, due to
checkpatch complaints. (The macro is not introduced anew, it is
touched only by un-indenting.)
- Normal administrativa (picked up R-b tags and Message-Id's, and noted
Mike substituting for the SecurityPkg reviewers during the CNY
holidays)
See the git-range-diff output after my sig.
Thanks,
Laszlo
1: 71155b00b2b7 ! 1: 4c8cd26ce423 SecurityPkg/DxeImageVerificationHandler: simplify "VerifyStatus"
@@ -19,6 +19,11 @@
Cc: Jiewen Yao <jiewen.yao@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
+ Message-Id: <20200116190705.18816-2-lersek@redhat.com>
+ Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
+ [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
+ Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
+ <d3fbb76dabed4e1987c512c328c82810@intel.com>]
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
2: 9ad18d2e3adb ! 2: f04114b6d6b2 SecurityPkg/DxeImageVerificationHandler: remove "else" after return/break
@@ -45,6 +45,11 @@
Cc: Jiewen Yao <jiewen.yao@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
+ Message-Id: <20200116190705.18816-3-lersek@redhat.com>
+ Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
+ [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
+ Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
+ <d3fbb76dabed4e1987c512c328c82810@intel.com>]
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
3: e211153f9a32 ! 3: da0e0dfc67c4 SecurityPkg/DxeImageVerificationHandler: keep PE/COFF info status internal
@@ -35,6 +35,11 @@
Cc: Jiewen Yao <jiewen.yao@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
+ Message-Id: <20200116190705.18816-4-lersek@redhat.com>
+ Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
+ [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
+ Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
+ <d3fbb76dabed4e1987c512c328c82810@intel.com>]
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
4: 3ad36b80defa ! 4: d930abc95422 SecurityPkg/DxeImageVerificationHandler: narrow down PE/COFF hash status
@@ -26,6 +26,11 @@
Cc: Jiewen Yao <jiewen.yao@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
+ Message-Id: <20200116190705.18816-5-lersek@redhat.com>
+ Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
+ [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
+ Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
+ <d3fbb76dabed4e1987c512c328c82810@intel.com>]
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
5: 379ac43e909b ! 5: 91b24a413440 SecurityPkg/DxeImageVerificationHandler: fix retval on memalloc failure
@@ -21,6 +21,11 @@
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
Fixes: 570b3d1a7278df29878da87990e8366bd42d0ec5
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
+ Message-Id: <20200116190705.18816-6-lersek@redhat.com>
+ Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
+ [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
+ Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
+ <d3fbb76dabed4e1987c512c328c82810@intel.com>]
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
6: c53a99ceb9f2 ! 6: 937d1c73965e SecurityPkg/DxeImageVerificationHandler: remove superfluous Status setting
@@ -13,6 +13,11 @@
Cc: Jiewen Yao <jiewen.yao@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
+ Message-Id: <20200116190705.18816-7-lersek@redhat.com>
+ Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
+ [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
+ Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
+ <d3fbb76dabed4e1987c512c328c82810@intel.com>]
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
7: c259648bbb30 ! 7: be0040ffa6cf SecurityPkg/DxeImageVerificationHandler: unnest AddImageExeInfo() call
@@ -20,6 +20,12 @@
Cc: Jiewen Yao <jiewen.yao@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
+ Message-Id: <20200116190705.18816-8-lersek@redhat.com>
+ Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
+ [lersek@redhat.com: replace EFI_D_INFO w/ DEBUG_INFO for PatchCheck.py]
+ [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
+ Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
+ <d3fbb76dabed4e1987c512c328c82810@intel.com>]
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -101,7 +107,7 @@
+ 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));
++ DEBUG ((DEBUG_INFO, "The image doesn't pass verification: %s\n", NameStr));
+ FreePool(NameStr);
}
+ Status = EFI_SECURITY_VIOLATION;
8: ca43b52bbd96 ! 8: feffd6bfd886 SecurityPkg/DxeImageVerificationHandler: eliminate "Status" variable
@@ -17,6 +17,11 @@
Cc: Jiewen Yao <jiewen.yao@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
+ Message-Id: <20200116190705.18816-9-lersek@redhat.com>
+ Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
+ [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
+ Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
+ <d3fbb76dabed4e1987c512c328c82810@intel.com>]
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
@@ -38,7 +43,7 @@
@@
- DEBUG((EFI_D_INFO, "The image doesn't pass verification: %s\n", NameStr));
+ DEBUG ((DEBUG_INFO, "The image doesn't pass verification: %s\n", NameStr));
FreePool(NameStr);
}
- Status = EFI_SECURITY_VIOLATION;
9: 22edc076c210 ! 9: 116742d3de8f SecurityPkg/DxeImageVerificationHandler: fix retval for (FileBuffer==NULL)
@@ -21,6 +21,11 @@
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
Fixes: 570b3d1a7278df29878da87990e8366bd42d0ec5
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
+ Message-Id: <20200116190705.18816-10-lersek@redhat.com>
+ Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
+ [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
+ Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
+ <d3fbb76dabed4e1987c512c328c82810@intel.com>]
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
10: e0b5e3b25eff ! 10: b73c1a576b78 SecurityPkg/DxeImageVerificationHandler: fix imgexec info on memalloc fail
@@ -28,6 +28,11 @@
Cc: Jiewen Yao <jiewen.yao@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
+ Message-Id: <20200116190705.18816-11-lersek@redhat.com>
+ Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
+ [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
+ Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
+ <d3fbb76dabed4e1987c512c328c82810@intel.com>]
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
11: 60363427926f ! 11: 1493b3ebadca SecurityPkg/DxeImageVerificationHandler: fix "defer" vs. "deny" policies
@@ -37,6 +37,11 @@
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
Fixes: 5db28a6753d307cdfb1cfdeb2f63739a9f959837
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
+ Message-Id: <20200116190705.18816-12-lersek@redhat.com>
+ Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
+ [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
+ Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
+ <d3fbb76dabed4e1987c512c328c82810@intel.com>]
diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
--- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy
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
2 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-31 10:07 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Zhang, Chao B, Wang, Jian J, Yao, Jiewen
On 01/31/20 10:28, Laszlo Ersek wrote:
> Hi Mike,
>
> On 01/31/20 09:12, Laszlo Ersek wrote:
>
>> So let me push this series as-is for TianoCore#2129, with your R-b
>> applied.
>
> My pull request (with the "push" label set) seems to have stalled. The
> checks have passed (twice -- I closed and reopened the PR once, to
> re-trigger mergify), but the branch is not being merged.
>
> https://github.com/tianocore/edk2/pull/324
Merged now: commit range 83357313dd67..8b0932c19f31.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy
2020-01-31 8:12 ` Laszlo Ersek
2020-01-31 9:28 ` Laszlo Ersek
@ 2020-01-31 16:31 ` Michael D Kinney
2020-01-31 17:00 ` Laszlo Ersek
1 sibling, 1 reply; 27+ messages in thread
From: Michael D Kinney @ 2020-01-31 16:31 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io, Kinney, Michael D
Cc: Zhang, Chao B, Wang, Jian J, Yao, Jiewen
Laszlo,
I think a new BZ is a good idea. I am sure there is more
history here and more discussion required on this invalid
policy PCD setting case.
I would also like to see a DEBUG() message or even better
a REPORT_STATUS_CODE() for an invalid policy PCD setting
and I would like platform policy to decide if the platform
should deadloop or continue with EFI_ACCESS_DENIED. By
putting the deadloop in this function, it takes away the
option for the platform to make that decision.
I also find ASSERT(FALSE) harder to triage. I prefer the
debug log to provide some indication of the cause of the
assert. Then I can go look up the file/line number for
more context.
Mike
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, January 31, 2020 12:13 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Wang, Jian
> J <jian.j.wang@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Subject: Re: [edk2-devel] [PATCH 00/11]
> SecurityPkg/DxeImageVerificationHandler: fix retval for
> "deny" policy
>
> On 01/31/20 03:59, Kinney, Michael D wrote:
> > Hi Laszlo,
> >
> > I have reviewed this patch series. All the patches
> look good. The
> > detailed description of each change made it easy to
> review.
> >
> > Series Reviewed-by: Michael D Kinney
> <michael.d.kinney@intel.com>
>
> Thank you very much!
>
> (more below)
>
> >
> > I have one question about that is not directly
> related to this logic
> > change.
> >
> > I see this logic that checks for invalid policy
> settings and ASSERT()s
> > and halts in a deadloop if either of 2 specific
> values are used that
> > have been removed from the UEFI Spec.
> >
> > //
> > // 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 ();
> > }
> >
> > There are 3 conditions where the Policy comes from a
> PCD.
> >
> > //
> > // 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;
> > }
> >
> > However, there are no checks in this function to
> verify that Policy is
> > set to one of the allowed values.
>
> That's right. I seem to recall that I noticed it too,
> and I wrote it off
> with "you can do damage with a bunch of other PCDs as
> well, if you
> misconfigure them".
>
> > This means an invalid PCD value will fall through to
> the
> > EFI_ACCESS_DENIED case.
>
> Yes. I find that the safest (silent) fallback for an
> undefined (per DEC)
> PCD value.
>
> > Is this the expected behavior for an invalid PCD
> setting? If so, then
> > why is there a check for the retired values from the
> UEFI Spec and a
> > deadloop performed. That seems inconsistent and not
> a good idea to
> > deadloop if we do not have to. Would it be better to
> return
> > EFI_ACCESS_DENIED for these 2 retired Policy values
> as well?
>
> Hmm, good point.
>
> I think these scenarios are different, historically. In
> one case we have
> a plain misconfigured platform. In the other case we
> have a platform
> that used to have correct configuration (considering
> edk2 in itself),
> but then for spec conformance reasons, it got
> invalidated
> *retroactively*. And I guess we wanted to be as loud as
> possible about
> the second case. There's a difference between "you
> didn't do your
> homework" and "you did your homework but we've changed
> the curriculum
> meanwhile".
>
> So the main question is if we want to remain "silent"
> about "plain
> misconfig", vs. "loud" about "obsolete config".
>
> We could unify the handling of both cases ("loudly")
> like this, for
> example:
>
> > diff --git
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageV
> erificationLib.c
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageV
> erificationLib.c
> > index ff79e30ef83e..1d41161abedc 100644
> > ---
> a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageV
> erificationLib.c
> > +++
> b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageV
> erificationLib.c
> > @@ -1617,23 +1617,32 @@ DxeImageVerificationHandler (
> > Policy = DENY_EXECUTE_ON_SECURITY_VIOLATION;
> > break;
> > }
> > +
> > + switch (Policy) {
> > //
> > // If policy is always/never execute, return
> directly.
> > //
> > - if (Policy == ALWAYS_EXECUTE) {
> > + case ALWAYS_EXECUTE:
> > return EFI_SUCCESS;
> > - }
> > - if (Policy == NEVER_EXECUTE) {
> > + case 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.
> > + // "Defer" and "deny" are valid policies that
> require actual verification.
> > //
> > - 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) {
> > + case DEFER_EXECUTE_ON_SECURITY_VIOLATION:
> > + case DENY_EXECUTE_ON_SECURITY_VIOLATION:
> > + break;
> > +
> > + //
> > + // QUERY_USER_ON_SECURITY_VIOLATION and
> ALLOW_EXECUTE_ON_SECURITY_VIOLATION
> > + // violate the UEFI spec; they now indicate
> platform misconfig. Hang here if
> > + // we find those policies. Handle undefined policy
> values the same way.
> > + //
> > + default:
> > + ASSERT (FALSE);
> > CpuDeadLoop ();
> > + return EFI_ACCESS_DENIED;
> > }
> >
> > GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME,
> (VOID**)&SecureBoot, NULL);
>
> Continuing:
>
> On 01/31/20 03:59, Kinney, Michael D wrote:
> > I am ok if you consider this to be a different issue.
>
> Yes, TianoCore#2129 is about mistaking DENY for DEFER,
> while this other
> topic is "complain loudly, rather than silently, about
> invalid PCD
> settings".
>
> So let me push this series as-is for TianoCore#2129,
> with your R-b
> applied. Then I will submit the above patch for
> separate review -- I'll
> put something like "hang on undefined image
> verification policy PCDs" in
> the subject.
>
> Would you like me to file a separate BZ too, for that
> patch?
>
> Thanks!
> Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy
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
2 siblings, 1 reply; 27+ messages in thread
From: Michael D Kinney @ 2020-01-31 16:52 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com, Kinney, Michael D
Cc: Zhang, Chao B, Wang, Jian J, Yao, Jiewen
Hi Laszlo,
I suspect the behavior you observed was due to the
PR being opened, which triggers the CI actions
immediately. Then you set the 'push' label, which
was not seen when the PR was opened. This required
a second pass which you did by re-opening.
There are 2 approaches a maintainer can take:
1) Open PR without 'push' label. Wait for CI checks
to run and review status. If all pass, then set
the 'push' label and re-open. This option does
cause 2 passes through the CI checks for a good
patch series.
2) Open PR with the 'push' label set. If all checks
pass, then it is merged. If any checks fail, then
the maintainer can address and do a forced push to
their branch to retry.
Mike
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Laszlo Ersek
> Sent: Friday, January 31, 2020 1:28 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Wang, Jian
> J <jian.j.wang@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Subject: Re: [edk2-devel] [PATCH 00/11]
> SecurityPkg/DxeImageVerificationHandler: fix retval for
> "deny" policy
>
> Hi Mike,
>
> On 01/31/20 09:12, Laszlo Ersek wrote:
>
> > So let me push this series as-is for TianoCore#2129,
> with your R-b
> > applied.
>
> My pull request (with the "push" label set) seems to
> have stalled. The
> checks have passed (twice -- I closed and reopened the
> PR once, to
> re-trigger mergify), but the branch is not being
> merged.
>
> https://github.com/tianocore/edk2/pull/324
>
> Thanks
> Laszlo
>
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy
2020-01-31 16:52 ` Michael D Kinney
@ 2020-01-31 16:59 ` Laszlo Ersek
2020-01-31 17:28 ` Michael D Kinney
0 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-31 16:59 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Zhang, Chao B, Wang, Jian J, Yao, Jiewen
On 01/31/20 17:52, Kinney, Michael D wrote:
> 2) Open PR with the 'push' label set. If all checks
> pass, then it is merged. If any checks fail, then
> the maintainer can address and do a forced push to
> their branch to retry.
Yes, this would be ideal, but I don't know how I can open a PR with the
push label atomically set. Is this available on the WebUI, or just from
the command line?
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy
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
0 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-31 17:00 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Zhang, Chao B, Wang, Jian J, Yao, Jiewen
On 01/31/20 17:31, Kinney, Michael D wrote:
> Laszlo,
>
> I think a new BZ is a good idea. I am sure there is more
> history here and more discussion required on this invalid
> policy PCD setting case.
>
> I would also like to see a DEBUG() message or even better
> a REPORT_STATUS_CODE() for an invalid policy PCD setting
> and I would like platform policy to decide if the platform
> should deadloop or continue with EFI_ACCESS_DENIED. By
> putting the deadloop in this function, it takes away the
> option for the platform to make that decision.
>
> I also find ASSERT(FALSE) harder to triage. I prefer the
> debug log to provide some indication of the cause of the
> assert. Then I can go look up the file/line number for
> more context.
OK. I'll abandon the patch, and only open a BZ with this information.
It's best if the SecurityPkg reviewers evaluate it carefully.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy
2020-01-31 17:00 ` Laszlo Ersek
@ 2020-01-31 17:12 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-31 17:12 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Zhang, Chao B, Wang, Jian J, Yao, Jiewen
On 01/31/20 18:00, Laszlo Ersek wrote:
> On 01/31/20 17:31, Kinney, Michael D wrote:
>> Laszlo,
>>
>> I think a new BZ is a good idea. I am sure there is more
>> history here and more discussion required on this invalid
>> policy PCD setting case.
>>
>> I would also like to see a DEBUG() message or even better
>> a REPORT_STATUS_CODE() for an invalid policy PCD setting
>> and I would like platform policy to decide if the platform
>> should deadloop or continue with EFI_ACCESS_DENIED. By
>> putting the deadloop in this function, it takes away the
>> option for the platform to make that decision.
>>
>> I also find ASSERT(FALSE) harder to triage. I prefer the
>> debug log to provide some indication of the cause of the
>> assert. Then I can go look up the file/line number for
>> more context.
>
> OK. I'll abandon the patch, and only open a BZ with this information.
> It's best if the SecurityPkg reviewers evaluate it carefully.
Here's the ticket:
https://bugzilla.tianocore.org/show_bug.cgi?id=2497
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy
2020-01-31 16:59 ` Laszlo Ersek
@ 2020-01-31 17:28 ` Michael D Kinney
2020-01-31 20:19 ` Laszlo Ersek
0 siblings, 1 reply; 27+ messages in thread
From: Michael D Kinney @ 2020-01-31 17:28 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com, Kinney, Michael D
Cc: Zhang, Chao B, Wang, Jian J, Yao, Jiewen
Hi Laszlo,
It can be done with the WebUI. There is a picture on
this page that shows setting the 'push' label before
selecting 'Create Pull Request'
https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
I can also be done with the 'hub' command. The following
Command shows all the flags for opening a PR using the
hub command. The '-l' flag allows a label to be set.
hub help pull-request
Mike
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Laszlo Ersek
> Sent: Friday, January 31, 2020 8:59 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Wang, Jian
> J <jian.j.wang@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Subject: Re: [edk2-devel] [PATCH 00/11]
> SecurityPkg/DxeImageVerificationHandler: fix retval for
> "deny" policy
>
> On 01/31/20 17:52, Kinney, Michael D wrote:
>
> > 2) Open PR with the 'push' label set. If all checks
> > pass, then it is merged. If any checks fail, then
> > the maintainer can address and do a forced push to
> > their branch to retry.
>
> Yes, this would be ideal, but I don't know how I can
> open a PR with the
> push label atomically set. Is this available on the
> WebUI, or just from
> the command line?
>
> Thanks!
> Laszlo
>
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy
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
0 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2020-01-31 20:19 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Zhang, Chao B, Wang, Jian J, Yao, Jiewen
On 01/31/20 18:28, Kinney, Michael D wrote:
> Hi Laszlo,
>
> It can be done with the WebUI. There is a picture on
> this page that shows setting the 'push' label before
> selecting 'Create Pull Request'
>
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
Aha, found it:
https://raw.githubusercontent.com/wiki/tianocore/tianocore.github.io/images/CreateGitHubPullRequest2.png
Many thanks, I'll use it in the future!
Laszlo
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On
>> Behalf Of Laszlo Ersek
>> Sent: Friday, January 31, 2020 8:59 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
>> devel@edk2.groups.io
>> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Wang, Jian
>> J <jian.j.wang@intel.com>; Yao, Jiewen
>> <jiewen.yao@intel.com>
>> Subject: Re: [edk2-devel] [PATCH 00/11]
>> SecurityPkg/DxeImageVerificationHandler: fix retval for
>> "deny" policy
>>
>> On 01/31/20 17:52, Kinney, Michael D wrote:
>>
>>> 2) Open PR with the 'push' label set. If all checks
>>> pass, then it is merged. If any checks fail, then
>>> the maintainer can address and do a forced push to
>>> their branch to retry.
>>
>> Yes, this would be ideal, but I don't know how I can
>> open a PR with the
>> push label atomically set. Is this available on the
>> WebUI, or just from
>> the command line?
>>
>> Thanks!
>> Laszlo
>>
>>
>>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* setting the push label at once, when opening a PR [was: SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy]
2020-01-31 20:19 ` Laszlo Ersek
@ 2020-02-05 13:02 ` Laszlo Ersek
2020-02-05 16:16 ` Michael D Kinney
0 siblings, 1 reply; 27+ messages in thread
From: Laszlo Ersek @ 2020-02-05 13:02 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Zhang, Chao B, Wang, Jian J, Yao, Jiewen
[-- Attachment #1: Type: text/plain, Size: 1734 bytes --]
Hi Mike,
On 01/31/20 21:19, Laszlo Ersek wrote:
> On 01/31/20 18:28, Kinney, Michael D wrote:
>> Hi Laszlo,
>>
>> It can be done with the WebUI. There is a picture on
>> this page that shows setting the 'push' label before
>> selecting 'Create Pull Request'
>>
>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
>
> Aha, found it:
>
> https://raw.githubusercontent.com/wiki/tianocore/tianocore.github.io/images/CreateGitHubPullRequest2.png
>
> Many thanks, I'll use it in the future!
the label in question is not there for me.
I've just pushed a new topic branch, for the purposes of PR / merging. After running the git-push command locally, I opened the URL in my browser that github.com's remote git daemon printed to my terminal, in response to my git-push command:
> Enumerating objects: 103, done.
> Counting objects: 100% (103/103), done.
> Delta compression using up to 4 threads
> Compressing objects: 100% (76/76), done.
> Writing objects: 100% (76/76), 15.62 KiB | 3.91 MiB/s, done.
> Total 76 (delta 64), reused 0 (delta 0)
> remote: Resolving deltas: 100% (64/64), completed with 24 local objects.
> remote:
> remote: Create a pull request for 'smram_at_default_smbase_bz_1512_wave_1_v2_pull' on GitHub by visiting:
> remote: https://github.com/lersek/edk2/pull/new/smram_at_default_smbase_bz_1512_wave_1_v2_pull
> remote:
> To github.com:lersek/edk2.git
> * [new branch] smram_at_default_smbase_bz_1512_wave_1_v2_pull -> smram_at_default_smbase_bz_1512_wave_1_v2_pull
Please see the attachment (screenshot) of what I found under that link. (I even used the browser's "in-page search" function, to locate the word "label"; it was not there.)
Thanks!
Laszlo
[-- Attachment #2: Screenshot from 2020-02-05 13-56-10.png --]
[-- Type: image/png, Size: 138382 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: setting the push label at once, when opening a PR [was: SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy]
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
0 siblings, 1 reply; 27+ messages in thread
From: Michael D Kinney @ 2020-02-05 16:16 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com, Kinney, Michael D
Cc: Zhang, Chao B, Wang, Jian J, Yao, Jiewen
Hi Laszlo,
If I follow this link, I see the expected screen with the ability to set a label:
https://github.com/lersek/edk2/pull/new/smram_at_default_smbase_bz_1512_wave_1_v2_pull
If I type in the following URL from your screen shot, I also get the same screen with the ability to set a label:
https://github.com/tianocore/edk2/compare/master...lersek:smram_at_default_smbase_bz_1512_wave_1_v2_pull?expand=1
It also looks like you are logged into GitHub, so that does not appear to be the issue.
I also verified you are a member of EDK II Maintainers, so that does not appear to be the issue.
Can you try both of the links above again?
Mike
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, February 5, 2020 5:02 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Wang, Jian
> J <jian.j.wang@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Subject: setting the push label at once, when opening a
> PR [was: SecurityPkg/DxeImageVerificationHandler: fix
> retval for "deny" policy]
>
> Hi Mike,
>
> On 01/31/20 21:19, Laszlo Ersek wrote:
> > On 01/31/20 18:28, Kinney, Michael D wrote:
> >> Hi Laszlo,
> >>
> >> It can be done with the WebUI. There is a picture on
> >> this page that shows setting the 'push' label before
> >> selecting 'Create Pull Request'
> >>
> >>
> https://github.com/tianocore/tianocore.github.io/wiki/E
> DK-II-Development-Process
> >
> > Aha, found it:
> >
> >
> https://raw.githubusercontent.com/wiki/tianocore/tianoc
> ore.github.io/images/CreateGitHubPullRequest2.png
> >
> > Many thanks, I'll use it in the future!
>
> the label in question is not there for me.
>
> I've just pushed a new topic branch, for the purposes
> of PR / merging. After running the git-push command
> locally, I opened the URL in my browser that
> github.com's remote git daemon printed to my terminal,
> in response to my git-push command:
>
> > Enumerating objects: 103, done.
> > Counting objects: 100% (103/103), done.
> > Delta compression using up to 4 threads
> > Compressing objects: 100% (76/76), done.
> > Writing objects: 100% (76/76), 15.62 KiB | 3.91
> MiB/s, done.
> > Total 76 (delta 64), reused 0 (delta 0)
> > remote: Resolving deltas: 100% (64/64), completed
> with 24 local objects.
> > remote:
> > remote: Create a pull request for
> 'smram_at_default_smbase_bz_1512_wave_1_v2_pull' on
> GitHub by visiting:
> > remote:
> https://github.com/lersek/edk2/pull/new/smram_at_defaul
> t_smbase_bz_1512_wave_1_v2_pull
> > remote:
> > To github.com:lersek/edk2.git
> > * [new branch]
> smram_at_default_smbase_bz_1512_wave_1_v2_pull ->
> smram_at_default_smbase_bz_1512_wave_1_v2_pull
>
> Please see the attachment (screenshot) of what I found
> under that link. (I even used the browser's "in-page
> search" function, to locate the word "label"; it was
> not there.)
>
> Thanks!
> Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: setting the push label at once, when opening a PR [was: SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy]
2020-02-05 16:16 ` Michael D Kinney
@ 2020-02-05 20:01 ` Laszlo Ersek
0 siblings, 0 replies; 27+ messages in thread
From: Laszlo Ersek @ 2020-02-05 20:01 UTC (permalink / raw)
To: Kinney, Michael D, devel@edk2.groups.io
Cc: Zhang, Chao B, Wang, Jian J, Yao, Jiewen
On 02/05/20 17:16, Kinney, Michael D wrote:
> Hi Laszlo,
>
> If I follow this link, I see the expected screen with the ability to set a label:
>
> https://github.com/lersek/edk2/pull/new/smram_at_default_smbase_bz_1512_wave_1_v2_pull
>
> If I type in the following URL from your screen shot, I also get the same screen with the ability to set a label:
>
> https://github.com/tianocore/edk2/compare/master...lersek:smram_at_default_smbase_bz_1512_wave_1_v2_pull?expand=1
>
> It also looks like you are logged into GitHub, so that does not appear to be the issue.
>
> I also verified you are a member of EDK II Maintainers, so that does not appear to be the issue.
>
> Can you try both of the links above again?
I've just done that, I even disabled uBlock Origin temporarily. No
change -- I still don't get the labels selection, under either link. And
I'm still logged in.
Perhaps the problem is that I don't have write access to the repo.
https://help.github.com/en/github/managing-your-work-on-github/labeling-issues-and-pull-requests
https://help.github.com/en/github/managing-your-work-on-github/applying-labels-to-issues-and-pull-requests
"In repositories where you have write access, you can assign labels to
issues and pull requests to help organize your projects."
(But then I don't understand why I'm permitted to set the "push" label
on an existent PR...)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2020-02-05 20:01 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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 ` [PATCH 09/11] SecurityPkg/DxeImageVerificationHandler: fix retval for (FileBuffer==NULL) Laszlo Ersek
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox