[PATCH 1/2] efi_loader: signature: correct a behavior against multiple signatures

Heinrich Schuchardt xypron.glpk at gmx.de
Fri Aug 14 12:33:39 CEST 2020


On 14.08.20 07:39, AKASHI Takahiro wrote:
> Under the current implementation, all the signatures, if any, in
> a signed image must be verified before loading it.
>
> Meanwhile, UEFI specification v2.8b section 32.5.3.3 says,
>     Multiple signatures are allowed to exist in the binary’s certificate
>     table (as per PE/COFF Section “Attribute Certificate Table”). Only
>     one hash or signature is required to be present in db in order to pass
>     validation, so long as neither the SHA-256 hash of the binary nor any
>     present signature is reflected in dbx.
>
> This patch makes the semantics of signature verification compliant with
> the specification mentioned above.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> Reported-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> ---
>  include/efi_loader.h              |  9 ++--
>  lib/efi_loader/efi_image_loader.c | 33 +++++++-------
>  lib/efi_loader/efi_signature.c    | 76 +++----------------------------
>  3 files changed, 30 insertions(+), 88 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index df8dc377257c..ae01e909b56c 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -770,13 +770,16 @@ struct pkcs7_message;
>
>  bool efi_signature_lookup_digest(struct efi_image_regions *regs,
>  				 struct efi_signature_store *db);
> -bool efi_signature_verify_one(struct efi_image_regions *regs,
> -			      struct pkcs7_message *msg,
> -			      struct efi_signature_store *db);
>  bool efi_signature_verify(struct efi_image_regions *regs,
>  			  struct pkcs7_message *msg,
>  			  struct efi_signature_store *db,
>  			  struct efi_signature_store *dbx);
> +static inline bool efi_signature_verify_one(struct efi_image_regions *regs,
> +					    struct pkcs7_message *msg,
> +					    struct efi_signature_store *db)
> +{
> +	return efi_signature_verify(regs, msg, db, NULL);
> +}
>  bool efi_signature_check_signers(struct pkcs7_message *msg,
>  				 struct efi_signature_store *dbx);
>
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index 832bce939403..eea42cc20436 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -546,6 +546,11 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  		goto err;
>  	}
>
> +	if (efi_signature_lookup_digest(regs, dbx)) {
> +		EFI_PRINT("Image's digest was found in \"dbx\"\n");
> +		goto err;
> +	}
> +
>  	/*
>  	 * go through WIN_CERTIFICATE list
>  	 * NOTE:
> @@ -553,10 +558,9 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  	 * in PE header, or as pkcs7 SignerInfo's in SignedData.
>  	 * So the verification policy here is:
>  	 *   - Success if, at least, one of signatures is verified
> -	 *   - unless
> -	 *       any of signatures is rejected explicitly, or
> -	 *       none of digest algorithms are supported
> +	 *   - unless signature is rejected explicitly with its digest.
>  	 */
> +
>  	for (wincert = wincerts, wincerts_end = (u8 *)wincerts + wincerts_len;
>  	     (u8 *)wincert < wincerts_end;
>  	     wincert = (WIN_CERTIFICATE *)
> @@ -627,32 +631,29 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
>  		/* try black-list first */
>  		if (efi_signature_verify_one(regs, msg, dbx)) {
>  			EFI_PRINT("Signature was rejected by \"dbx\"\n");
> -			goto err;
> +			continue;
>  		}
>
>  		if (!efi_signature_check_signers(msg, dbx)) {
>  			EFI_PRINT("Signer(s) in \"dbx\"\n");
> -			goto err;
> -		}
> -
> -		if (efi_signature_lookup_digest(regs, dbx)) {
> -			EFI_PRINT("Image's digest was found in \"dbx\"\n");
> -			goto err;
> +			continue;
>  		}
>
>  		/* try white-list */
> -		if (efi_signature_verify(regs, msg, db, dbx))
> -			continue;
> +		if (efi_signature_verify(regs, msg, db, dbx)) {
> +			ret = true;
> +			break;
> +		}
>
>  		debug("Signature was not verified by \"db\"\n");

Hello Takahiro,

thanks for for fixing the logic for multiple sigantures.

Here we have a mishmash of EFI_PRINT() and debug(). I think it is
worthwhile to clean this up. But that will be a separate patch.

Best regards

Heinrich

>
> -		if (efi_signature_lookup_digest(regs, db))
> -			continue;
> +		if (efi_signature_lookup_digest(regs, db)) {
> +			ret = true;
> +			break;
> +		}
>
>  		debug("Image's digest was not found in \"db\" or \"dbx\"\n");
> -		goto err;
>  	}
> -	ret = true;
>
>  err:
>  	efi_sigstore_free(db);
> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> index 7b33a4010fe8..a8da2496360d 100644
> --- a/lib/efi_loader/efi_signature.c
> +++ b/lib/efi_loader/efi_signature.c
> @@ -175,6 +175,8 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
>  			if (IS_ERR_OR_NULL(cert_tmp))
>  				continue;
>
> +			EFI_PRINT("%s: against %s\n", __func__,
> +				  cert_tmp->subject);
>  			reg[0].data = cert_tmp->tbs;
>  			reg[0].size = cert_tmp->tbs_size;
>  			if (!efi_hash_regions(reg, 1, &hash_tmp, NULL))
> @@ -267,7 +269,7 @@ out:
>   * protocol at this time and any image will be unconditionally revoked
>   * when this match occurs.
>   *
> - * Return:	true if check passed, false otherwise.
> + * Return:	true if check passed (not found), false otherwise.
>   */
>  static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
>  					   struct x509_certificate *cert,
> @@ -337,70 +339,6 @@ out:
>  	return !revoked;
>  }
>
> -/**
> - * efi_signature_verify_one - verify signatures with database
> - * @regs:	List of regions to be authenticated
> - * @msg:	Signature
> - * @db:		Signature database
> - *
> - * All the signature pointed to by @msg against image pointed to by @regs
> - * will be verified by signature database pointed to by @db.
> - *
> - * Return:	true if verification for one of signatures passed, false
> - *		otherwise
> - */
> -bool efi_signature_verify_one(struct efi_image_regions *regs,
> -			      struct pkcs7_message *msg,
> -			      struct efi_signature_store *db)
> -{
> -	struct pkcs7_signed_info *sinfo;
> -	struct x509_certificate *signer;
> -	bool verified = false;
> -	int ret;
> -
> -	EFI_PRINT("%s: Enter, %p, %p, %p\n", __func__, regs, msg, db);
> -
> -	if (!db)
> -		goto out;
> -
> -	if (!db->sig_data_list)
> -		goto out;
> -
> -	EFI_PRINT("%s: Verify signed image with db\n", __func__);
> -	for (sinfo = msg->signed_infos; sinfo; sinfo = sinfo->next) {
> -		EFI_PRINT("Signed Info: digest algo: %s, pkey algo: %s\n",
> -			  sinfo->sig->hash_algo, sinfo->sig->pkey_algo);
> -
> -		EFI_PRINT("Verifying certificate chain\n");
> -		signer = NULL;
> -		ret = pkcs7_verify_one(msg, sinfo, &signer);
> -		if (ret == -ENOPKG)
> -			continue;
> -
> -		if (ret < 0 || !signer)
> -			goto out;
> -
> -		if (sinfo->blacklisted)
> -			continue;
> -
> -		EFI_PRINT("Verifying last certificate in chain\n");
> -		if (signer->self_signed) {
> -			if (efi_lookup_certificate(signer, db)) {
> -				verified = true;
> -				goto out;
> -			}
> -		} else if (efi_verify_certificate(signer, db, NULL)) {
> -			verified = true;
> -			goto out;
> -		}
> -		EFI_PRINT("Valid certificate not in db\n");
> -	}
> -
> -out:
> -	EFI_PRINT("%s: Exit, verified: %d\n", __func__, verified);
> -	return verified;
> -}
> -
>  /*
>   * efi_signature_verify - verify signatures with db and dbx
>   * @regs:	List of regions to be authenticated
> @@ -463,7 +401,7 @@ bool efi_signature_verify(struct efi_image_regions *regs,
>  			if (efi_lookup_certificate(signer, db))
>  				if (efi_signature_check_revocation(sinfo,
>  								   signer, dbx))
> -					continue;
> +					break;
>  		} else if (efi_verify_certificate(signer, db, &root)) {
>  			bool check;
>
> @@ -471,13 +409,13 @@ bool efi_signature_verify(struct efi_image_regions *regs,
>  							       dbx);
>  			x509_free_certificate(root);
>  			if (check)
> -				continue;
> +				break;
>  		}
>
>  		EFI_PRINT("Certificate chain didn't reach trusted CA\n");
> -		goto out;
>  	}
> -	verified = true;
> +	if (sinfo)
> +		verified = true;
>  out:
>  	EFI_PRINT("%s: Exit, verified: %d\n", __func__, verified);
>  	return verified;
>



More information about the U-Boot mailing list