[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