[PATCH v3 07/11] efi_loader: check certificate lengths in efi_image_authenticate()

[Ana Bobillo <anabobillo3@gmail.com>]

From: Ana Bobillo <anabobillo3@gmail.com>

efi_image_authenticate() walks the certificate table of an image by
dwLength, rounded up to a multiple of 8, without checking it against
the table:

- a certificate no longer than its header is skipped, but a dwLength
  of 0, or one that wraps around to 0 when rounded up, does not
  advance the walk, so U-Boot hangs on such an image;
- a dwLength larger than the rest of the table makes
  pkcs7_parse_message() read past the end of the table and of the
  image.

Turn the check that skipped a certificate no longer than its header
into one that rejects the image when a certificate is no longer than
its header or longer than the rest of the table. EDK2's
DxeImageVerificationLib also rejects an image whose certificate table
holds a certificate shorter than its header or longer than the rest of
the table.

Fixes: 4540dabdcaca ("efi_loader: image_loader: support image authentication")
Fixes: 1a44b7059c18 ("efi_loader: image_loader: add a check against certificate type of authenticode")
Signed-off-by: Ana Bobillo <anabobillo3@gmail.com>
---
Changes in v3:
- fold the new check and the old "dwLength too small" one into a
  single check (Ilias)

 lib/efi_loader/efi_image_loader.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index 417282698fa..8f48d8fb5a5 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -808,14 +808,26 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
 	for (wincert = wincerts, wincerts_end = (u8 *)wincerts + wincerts_len;
 	     (u8 *)wincert < wincerts_end;
 	     wincert = (WIN_CERTIFICATE *)
-			((u8 *)wincert + ALIGN(wincert->dwLength, 8))) {
-		if ((u8 *)wincert + sizeof(*wincert) >= wincerts_end)
+			((u8 *)wincert + ALIGN((size_t)wincert->dwLength, 8))) {
+		size_t remaining = wincerts_end - (u8 *)wincert;
+
+		if (sizeof(*wincert) >= remaining)
 			break;
 
-		if (wincert->dwLength <= sizeof(*wincert)) {
-			log_debug("dwLength too small: %u < %zu\n",
-				  wincert->dwLength, sizeof(*wincert));
-			continue;
+		/*
+		 * dwLength includes the WIN_CERTIFICATE header. A certificate
+		 * must hold a signature after its header and end within the
+		 * certificate table. Reject an image with a corrupted
+		 * certificate table: a dwLength of 0 would not advance the
+		 * walk, and one past the end of the table would make the
+		 * signature run past it.
+		 */
+		if (wincert->dwLength <= sizeof(*wincert) ||
+		    wincert->dwLength > remaining) {
+			log_err("Invalid certificate length: %u\n",
+				wincert->dwLength);
+			ret = false;
+			goto out;
 		}
 
 		log_debug("WIN_CERTIFICATE_TYPE: 0x%x\n",