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

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@linaro.org Reported-by: Heinrich Schuchardt xypron.glpk@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;