shim: immediately stop if signature is matched.

If an image signed by multiple keys matches any one of vendor certificates, the image will be considered valid and should stop checking the next signature. This PR immediately stops do-while() next signature checking. in verify_buffer_authenticode() when key matches. Signed-off-by: Dennis Tseng <[email protected]>
rhboot · Oct 28, 2024 · bd376c1 · bd376c1
1 parent 0287c6b
commit bd376c1
Showing 1 changed file with 26 additions and 28 deletions.
diff --git a/shim.c b/shim.c
@@ -563,7 +563,7 @@ verify_buffer_authenticode (char *data, int datasize,
 			    PE_COFF_LOADER_IMAGE_CONTEXT *context,
 			    UINT8 *sha256hash, UINT8 *sha1hash)
 {
-	EFI_STATUS ret_efi_status;
+	EFI_STATUS efi_status;
 	size_t size = datasize;
 	size_t offset = 0;
 	unsigned int i = 0;
@@ -578,48 +578,48 @@ verify_buffer_authenticode (char *data, int datasize,
 	 */
 	drain_openssl_errors();
 
-	ret_efi_status = generate_hash(data, datasize, context, sha256hash, sha1hash);
-	if (EFI_ERROR(ret_efi_status)) {
-		dprint(L"generate_hash: %r\n", ret_efi_status);
+	efi_status = generate_hash(data, datasize, context, sha256hash, sha1hash);
+	if (EFI_ERROR(efi_status)) {
+		dprint(L"generate_hash: %r\n", efi_status);
 		PrintErrors();
 		ClearErrors();
-		crypterr(ret_efi_status);
-		return ret_efi_status;
+		crypterr(efi_status);
+		return efi_status;
 	}
 
 	/*
 	 * Ensure that the binary isn't forbidden by hash
 	 */
 	drain_openssl_errors();
-	ret_efi_status = check_denylist(NULL, sha256hash, sha1hash);
-	if (EFI_ERROR(ret_efi_status)) {
+	efi_status = check_denylist(NULL, sha256hash, sha1hash);
+	if (EFI_ERROR(efi_status)) {
 //		perror(L"Binary is forbidden\n");
-//		dprint(L"Binary is forbidden: %r\n", ret_efi_status);
+//		dprint(L"Binary is forbidden: %r\n", efi_status);
 		PrintErrors();
 		ClearErrors();
-		crypterr(ret_efi_status);
-		return ret_efi_status;
+		crypterr(efi_status);
+		return efi_status;
 	}
 
 	/*
 	 * Check whether the binary is authorized by hash in any of the
 	 * firmware databases
 	 */
 	drain_openssl_errors();
-	ret_efi_status = check_allowlist(NULL, sha256hash, sha1hash);
-	if (EFI_ERROR(ret_efi_status)) {
-		LogError(L"check_allowlist(): %r\n", ret_efi_status);
-		dprint(L"check_allowlist: %r\n", ret_efi_status);
-		if (ret_efi_status != EFI_NOT_FOUND) {
-			dprint(L"check_allowlist(): %r\n", ret_efi_status);
+	efi_status = check_allowlist(NULL, sha256hash, sha1hash);
+	if (EFI_ERROR(efi_status)) {
+		LogError(L"check_allowlist(): %r\n", efi_status);
+		dprint(L"check_allowlist: %r\n", efi_status);
+		if (efi_status != EFI_NOT_FOUND) {
+			dprint(L"check_allowlist(): %r\n", efi_status);
 			PrintErrors();
 			ClearErrors();
-			crypterr(ret_efi_status);
-			return ret_efi_status;
+			crypterr(efi_status);
+			return efi_status;
 		}
 	} else {
 		drain_openssl_errors();
-		return ret_efi_status;
+		return efi_status;
 	}
 
 	if (context->SecDir->Size == 0) {
@@ -634,7 +634,7 @@ verify_buffer_authenticode (char *data, int datasize,
 	}
 
 	offset = 0;
-	ret_efi_status = EFI_NOT_FOUND;
+	efi_status = EFI_NOT_FOUND;
 	do {
 		WIN_CERTIFICATE_EFI_PKCS *sig = NULL;
 		size_t sz;
@@ -669,8 +669,6 @@ verify_buffer_authenticode (char *data, int datasize,
 		}
 
 		if (sig->Hdr.wCertificateType == WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
-			EFI_STATUS efi_status;
-
 			dprint(L"Attempting to verify signature %d:\n", i++);
 
 			efi_status = verify_one_signature(sig, sha256hash, sha1hash);
@@ -683,24 +681,24 @@ verify_buffer_authenticode (char *data, int datasize,
 			 * So don't clobber successes with security violation
 			 * here; that just means it isn't a success.
 			 */
-			if (ret_efi_status != EFI_SUCCESS)
-				ret_efi_status = efi_status;
+			if (efi_status == EFI_SUCCESS)
+				break;
 		} else {
 			perror(L"Unsupported certificate type %x\n",
 				sig->Hdr.wCertificateType);
 		}
 		offset = ALIGN_VALUE(offset + sz, 8);
 	} while (offset < context->SecDir->Size);
 
-	if (ret_efi_status != EFI_SUCCESS) {
+	if (efi_status != EFI_SUCCESS) {
 		dprint(L"Binary is not authorized\n");
 		PrintErrors();
 		ClearErrors();
 		crypterr(EFI_SECURITY_VIOLATION);
-		ret_efi_status = EFI_SECURITY_VIOLATION;
+		efi_status = EFI_SECURITY_VIOLATION;
 	}
 	drain_openssl_errors();
-	return ret_efi_status;
+	return efi_status;
 }
 
 /*