efi_loader: tighten PE verification code?

[Patrick Wildt]

Hi,

even though this mail has a diff, it's not supposed to be a patch.  I
have started adjusting my fuzzer to the upstreamed EFI Secure Boot code
and I hit a few issues right away.  Two of those I have already sent and
were reviewed.  I have seen more, but since I needed to reschedule some
things I couldn't continue and unfortunately have to postpone fuzzing
the code.  I can assure you if someone starts fuzzing that code, we will
find a few more bugs.

Especially since this is "Secure Boot", this part should definitely be
air tight.

One thing I saw is that the virt_size is smaller then the memcpy below
at the "Copy PE headers" line then actually copies.

Another thing I saw is SizeOfRawData can be bigger then the VirtualSize,
and PointerToRawData + SizeOfRawData bigger then the allocated size.

I'm not sure if this is the right place to do the checks.  Maybe they
need to be at the place where we are adding the image regions.  I guess
the fields should be properly verified in view of "does it make sense?".

Also I'm questioning whether it's a good idea to continue parsing the
file if it's already clear that the signature can't be verified against
the "db".  I can understand that you'd like to finish loading the file
into an object, and some other instance decides whether or not it's fine
to start that image, but you also open yourself to issues when you're
parsing a file that obviously is against the current security policy.

If you keep on parsing a file that obviously (because tested against the
"db") cannot be allowed to boot anyway, the checks for the headers need
to be even tighter.

I'd be unhappy to see U-Boot CVEs come up regarding PE parsing issues.

Best regards,
Patrick

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index 43a53d3dd1..d134b748be 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -681,10 +681,11 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 	}
 
 	/* Authenticate an image */
-	if (efi_image_authenticate(efi, efi_size))
-		handle->auth_status = EFI_IMAGE_AUTH_PASSED;
-	else
+	if (!efi_image_authenticate(efi, efi_size)) {
 		handle->auth_status = EFI_IMAGE_AUTH_FAILED;
+		ret = EFI_SECURITY_VIOLATION;
+		goto err;
+	}
 
 	/* Calculate upper virtual address boundary */
 	for (i = num_sections - 1; i >= 0; i--) {
@@ -736,6 +737,15 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 		goto err;
 	}
 
+	if (virt_size < sizeof(*dos) + sizeof(*nt) +
+	    nt->FileHeader.SizeOfOptionalHeader +
+	    num_sections * sizeof(IMAGE_SECTION_HEADER)) {
+		efi_free_pages((uintptr_t) efi_reloc,
+		       (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT);
+		ret = EFI_LOAD_ERROR;
+		goto err;
+	}
+
 	/* Copy PE headers */
 	memcpy(efi_reloc, efi,
 	       sizeof(*dos)
@@ -746,6 +756,13 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 	/* Load sections into RAM */
 	for (i = num_sections - 1; i >= 0; i--) {
 		IMAGE_SECTION_HEADER *sec = &sections[i];
+		if (sec->SizeOfRawData > sec->Misc.VirtualSize ||
+		    sec->PointerToRawData + sec->SizeOfRawData > efi_size) {
+			efi_free_pages((uintptr_t) efi_reloc,
+			       (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT);
+			ret = EFI_LOAD_ERROR;
+			goto err;
+		}
 		memset(efi_reloc + sec->VirtualAddress, 0,
 		       sec->Misc.VirtualSize);
 		memcpy(efi_reloc + sec->VirtualAddress,
@@ -771,10 +788,8 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
 	loaded_image_info->image_base = efi_reloc;
 	loaded_image_info->image_size = virt_size;
 
-	if (handle->auth_status == EFI_IMAGE_AUTH_PASSED)
-		return EFI_SUCCESS;
-	else
-		return EFI_SECURITY_VIOLATION;
+	handle->auth_status = EFI_IMAGE_AUTH_PASSED;
+	return EFI_SUCCESS;
 
 err:
 	return ret;