[PATCH v2 06/17] efi_loader: image_loader: add a check against certificate type of authenticode

[Heinrich Schuchardt]

On 09.06.20 07:09, AKASHI Takahiro wrote:
> UEFI specification requires that we shall support three type of
> certificates of authenticode in PE image:
>   WIN_CERT_TYPE_EFI_GUID with the guid, EFI_CERT_TYPE_PCKS7_GUID
>   WIN_CERT_TYPE_PKCS_SIGNED_DATA
>   WIN_CERT_TYPE_EFI_PKCS1_15
>
> As EDK2 does, we will support the first two that are pkcs7 SignedData.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> ---
>  lib/efi_loader/efi_image_loader.c | 56 ++++++++++++++++++++++++-------
>  1 file changed, 44 insertions(+), 12 deletions(-)
>
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index 5b00fea2f113..38b2c24ab1d6 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -484,7 +484,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  	struct efi_signature_store *db = NULL, *dbx = NULL;
>  	struct x509_certificate *cert = NULL;
>  	void *new_efi = NULL;
> -	size_t new_efi_size;
> +	u8 *auth, *wincerts_end;
> +	size_t new_efi_size, auth_size;
>  	bool ret = false;
>
>  	if (!efi_secure_boot_enabled())
> @@ -533,21 +534,52 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  	}
>
>  	/* go through WIN_CERTIFICATE list */
> -	for (wincert = wincerts;
> -	     (void *)wincert < (void *)wincerts + wincerts_len;
> -	     wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
> -		if (wincert->dwLength < sizeof(*wincert)) {
> -			EFI_PRINT("%s: dwLength too small: %u < %zu\n",
> -				  __func__, wincert->dwLength,
> -				  sizeof(*wincert));
> -			goto err;
> +	for (wincert = wincerts, wincerts_end = (u8 *)wincerts + wincerts_len;
> +	     (u8 *)wincert < wincerts_end;

Shouldn't you compare the end of the current certificate to wincerts_end?

    ((u8 *)wincert + ALIGN(wincert->dwLength, 8))) <= wincerts_end

But you doing such a comparison anyway two lines below. So there is no
need to duplicate the test here.

> +	     wincert = (WIN_CERTIFICATE *)
> +			((u8 *)wincert + ALIGN(wincert->dwLength, 8))) {
> +		if ((u8 *)wincert + sizeof(*wincert) >= wincerts_end)
> +			break;

Why is this >= and not >?

Best regards

Heinrich

> +
> +		if (wincert->dwLength <= sizeof(*wincert)) {
> +			EFI_PRINT("dwLength too small: %u < %zu\n",
> +				  wincert->dwLength, sizeof(*wincert));
> +			continue;
> +		}
> +
> +		EFI_PRINT("WIN_CERTIFICATE_TYPE: 0x%x\n",
> +			  wincert->wCertificateType);
> +
> +		auth = (u8 *)wincert + sizeof(*wincert);
> +		auth_size = wincert->dwLength - sizeof(*wincert);
> +		if (wincert->wCertificateType == WIN_CERT_TYPE_EFI_GUID) {
> +			if (auth + sizeof(efi_guid_t) >= wincerts_end)
> +				break;
> +
> +			if (auth_size <= sizeof(efi_guid_t)) {
> +				EFI_PRINT("dwLength too small: %u < %zu\n",
> +					  wincert->dwLength, sizeof(*wincert));
> +				continue;
> +			}
> +			if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
> +				EFI_PRINT("Certificate type not supported: %pUl\n",
> +					  auth);
> +				continue;
> +			}
> +
> +			auth += sizeof(efi_guid_t);
> +			auth_size -= sizeof(efi_guid_t);
> +		} else if (wincert->wCertificateType
> +				!= WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
> +			EFI_PRINT("Certificate type not supported\n");
> +			continue;
>  		}
> -		msg = pkcs7_parse_message((void *)wincert + sizeof(*wincert),
> -					  wincert->dwLength - sizeof(*wincert));
> +
> +		msg = pkcs7_parse_message(auth, auth_size);
>  		if (IS_ERR(msg)) {
>  			EFI_PRINT("Parsing image's signature failed\n");
>  			msg = NULL;
> -			goto err;
> +			continue;
>  		}
>
>  		/* try black-list first */
>