[PATCH 06/13] efi_loader: image_loader: verification for all signatures should pass

AKASHI Takahiro takahiro.akashi at linaro.org
Tue Jun 2 07:22:51 CEST 2020


Heinrich,

On Sat, May 30, 2020 at 09:01:53AM +0200, Heinrich Schuchardt wrote:
> On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> > A signed image may have multiple signatures in
> >   - each WIN_CERTIFICATE in authenticode, and/or
> >   - each SignerInfo in pkcs7 SignedData (of WIN_CERTIFICATE)
> >
> > In the initial implementation of efi_image_authenticate(), the criteria
> > of verification check for multiple signatures case is a bit ambiguous
> > and it may cause inconsistent result.
> >
> > With this patch, we will make sure that verification check in
> > efi_image_authenticate() should pass against all the signatures.
> > The only exception would be
> >   - the case where a digest algorithm used in signature is not supported by
> >     U-Boot, or
> >   - the case where parsing some portion of authenticode has failed
> > In those cases, we don't know how the signature be handled and should
> > just ignore them.
> >
> > Please note that, due to this change, efi_signature_verify_with_sigdb()'s
> > function prototype will be modified, taking "dbx" as well as "db"
> > instead of outputing a "certificate." If "dbx" is null, the behavior would
> > be the exact same as before.
> > The function's name will be changed to efi_signature_verify() once
> > current efi_signature_verify() has gone due to further improvement
> > in intermedaite certificates support.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > ---
> >  include/efi_loader.h              |  10 +-
> >  lib/efi_loader/efi_image_loader.c |  37 +++--
> >  lib/efi_loader/efi_signature.c    | 266 ++++++++++++++----------------
> >  3 files changed, 146 insertions(+), 167 deletions(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 9533df26dc9e..2cbd52e273d4 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -761,14 +761,12 @@ struct efi_signature_store {
> >  struct x509_certificate;
> >  struct pkcs7_message;
> >
> > -bool efi_signature_verify_cert(struct x509_certificate *cert,
> > -			       struct efi_signature_store *dbx);
> > -bool efi_signature_verify_signers(struct pkcs7_message *msg,
> > -				  struct efi_signature_store *dbx);
> >  bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
> >  				     struct pkcs7_message *msg,
> > -				  struct efi_signature_store *db,
> > -				  struct x509_certificate **cert);
> > +				     struct efi_signature_store *db,
> > +				     struct efi_signature_store *dbx);
> > +bool efi_signature_check_signers(struct pkcs7_message *msg,
> > +				 struct efi_signature_store *dbx);
> >
> >  efi_status_t efi_image_region_add(struct efi_image_regions *regs,
> >  				  const void *start, const void *end,
> > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> > index 5cdaa93519e7..33ffb43f3886 100644
> > --- a/lib/efi_loader/efi_image_loader.c
> > +++ b/lib/efi_loader/efi_image_loader.c
> > @@ -492,11 +492,12 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  	size_t wincerts_len;
> >  	struct pkcs7_message *msg = NULL;
> >  	struct efi_signature_store *db = NULL, *dbx = NULL;
> > -	struct x509_certificate *cert = NULL;
> >  	void *new_efi = NULL, *auth, *wincerts_end;
> >  	size_t new_efi_size, auth_size;
> >  	bool ret = false;
> >
> > +	debug("%s: Enter, %d\n", __func__, ret);
> > +
> >  	if (!efi_secure_boot_enabled())
> >  		return true;
> >
> > @@ -542,7 +543,17 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  		goto err;
> >  	}
> >
> > -	/* go through WIN_CERTIFICATE list */
> > +	/*
> > +	 * go through WIN_CERTIFICATE list
> > +	 * NOTE:
> > +	 * We may have multiple signatures either as WIN_CERTIFICATE's
> > +	 * 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
> 
> If one signature is good and the others are rejected (e.g. due to the
> revocation list), why won't you be happy with the one good signature? Is
> this a requirement in the UEFI spec?

Actually, I don't know the right answer.
UEFI specification doesn't say anything here and as far as I correctly
understand EDK2 code, it behaves in the same way.
Since there is no conformance test available (in UEFI SCT), it is
difficult to confirm that.
# FYI, I have been requesting such test cases in SCT since last October:
  https://bugzilla.tianocore.org/show_bug.cgi?id=2230

I also think that applying more strict rules and then easing them
later would be much easier and safer than the reverse(?).

Thanks,
-Takahiro Akashi



> Best regards
> 
> Heinrich
> 
> > +	 *       none of digest algorithms are supported
> > +	 */
> >  	for (wincert = wincerts, wincerts_end = (void *)wincerts + wincerts_len;
> >  	     (void *)wincert < wincerts_end;
> >  	     wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
> > @@ -596,37 +607,27 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >  			goto err;
> >  		}
> >
> > -		if (!efi_signature_verify_signers(msg, dbx)) {
> > -			debug("Signer was rejected by \"dbx\"\n");
> > +		if (!efi_signature_check_signers(msg, dbx)) {
> > +			debug("Signer(s) in \"dbx\"\n");
> >  			goto err;
> > -		} else {
> > -			ret = true;
> >  		}
> >
> >  		/* try white-list */
> > -		if (!efi_signature_verify_with_sigdb(regs, msg, db, &cert)) {
> > -			debug("Verifying signature with \"db\" failed\n");
> > +		if (!efi_signature_verify_with_sigdb(regs, msg, db, dbx)) {
> > +			debug("Signature was not verified by \"db\"\n");
> >  			goto err;
> > -		} else {
> > -			ret = true;
> > -		}
> > -
> > -		if (!efi_signature_verify_cert(cert, dbx)) {
> > -			debug("Certificate was rejected by \"dbx\"\n");
> > -			goto err;
> > -		} else {
> > -			ret = true;
> >  		}
> >  	}
> > +	ret = true;
> >
> >  err:
> > -	x509_free_certificate(cert);
> >  	efi_sigstore_free(db);
> >  	efi_sigstore_free(dbx);
> >  	pkcs7_free_message(msg);
> >  	free(regs);
> >  	free(new_efi);
> >
> > +	debug("%s: Exit, %d\n", __func__, ret);
> >  	return ret;
> >  }
> >  #else
> > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> > index 00e442783059..ab5687040a38 100644
> > --- a/lib/efi_loader/efi_signature.c
> > +++ b/lib/efi_loader/efi_signature.c
> > @@ -300,27 +300,111 @@ out:
> >  }
> >
> >  /**
> > - * efi_signature_verify_with_sigdb - verify a signature with db
> > + * efi_signature_check_revocation - check revocation with dbx
> > + * @sinfo:	Signer's info
> > + * @cert:	x509 certificate
> > + * @dbx:	Revocation signature database
> > + *
> > + * Search revocation signature database pointed to by @dbx and find
> > + * an entry matching to certificate pointed to by @cert.
> > + *
> > + * While this entry contains revocation time, we don't support timestamp
> > + * protocol at this time and any image will be unconditionally revoked
> > + * when this match occurs.
> > + *
> > + * Return:	true if check passed, false otherwise.
> > + */
> > +static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
> > +					   struct x509_certificate *cert,
> > +					   struct efi_signature_store *dbx)
> > +{
> > +	struct efi_signature_store *siglist;
> > +	struct efi_sig_data *sig_data;
> > +	struct image_region reg[1];
> > +	void *hash = NULL;
> > +	size_t size = 0;
> > +	time64_t revoc_time;
> > +	bool revoked = false;
> > +
> > +	debug("%s: Enter, %p, %p, %p\n", __func__, sinfo, cert, dbx);
> > +
> > +	if (!sinfo || !cert || !dbx || !dbx->sig_data_list)
> > +		goto out;
> > +
> > +	debug("Checking revocation against %s\n", cert->subject);
> > +	for (siglist = dbx; siglist; siglist = siglist->next) {
> > +		if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509_sha256))
> > +			continue;
> > +
> > +		/* calculate hash of TBSCertificate */
> > +		reg[0].data = cert->tbs;
> > +		reg[0].size = cert->tbs_size;
> > +		if (!efi_hash_regions(reg, 1, &hash, &size))
> > +			goto out;
> > +
> > +		for (sig_data = siglist->sig_data_list; sig_data;
> > +		     sig_data = sig_data->next) {
> > +			/*
> > +			 * struct efi_cert_x509_sha256 {
> > +			 *	u8 tbs_hash[256/8];
> > +			 *	time64_t revocation_time;
> > +			 * };
> > +			 */
> > +#ifdef DEBUG
> > +			if (sig_data->size >= size) {
> > +				debug("hash in db:\n");
> > +				print_hex_dump("    ", DUMP_PREFIX_OFFSET,
> > +					       16, 1,
> > +					       sig_data->data, size, false);
> > +			}
> > +#endif
> > +			if ((sig_data->size < size + sizeof(time64_t)) ||
> > +			    memcmp(sig_data->data, hash, size))
> > +				continue;
> > +
> > +			memcpy(&revoc_time, sig_data->data + size,
> > +			       sizeof(revoc_time));
> > +			debug("revocation time: 0x%llx\n", revoc_time);
> > +			/*
> > +			 * TODO: compare signing timestamp in sinfo
> > +			 * with revocation time
> > +			 */
> > +
> > +			revoked = true;
> > +			free(hash);
> > +			goto out;
> > +		}
> > +		free(hash);
> > +		hash = NULL;
> > +	}
> > +out:
> > +	debug("%s: Exit, revoked: %d\n", __func__, revoked);
> > +	return !revoked;
> > +}
> > +
> > +/**
> > + * efi_signature_verify_with_sigdb - verify signatures with db and dbx
> >   * @regs:	List of regions to be authenticated
> >   * @msg:	Signature
> >   * @db:		Signature database for trusted certificates
> > - * @cert:	x509 certificate that verifies this signature
> > + * @dbx:	Revocation signature database
> >   *
> > - * Signature pointed to by @msg against image pointed to by @regs
> > - * is verified by signature database pointed to by @db.
> > + * All the signature pointed to by @msg against image pointed to by @regs
> > + * will be verified by signature database pointed to by @db and @dbx.
> >   *
> > - * Return:	true if signature is verified, false if not
> > + * Return:	true if verification for all signatures passed, false otherwise
> >   */
> >  bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
> >  				     struct pkcs7_message *msg,
> >  				     struct efi_signature_store *db,
> > -				     struct x509_certificate **cert)
> > +				     struct efi_signature_store *dbx)
> >  {
> >  	struct pkcs7_signed_info *info;
> >  	struct efi_signature_store *siglist;
> > +	struct x509_certificate *cert;
> >  	bool verified = false;
> >
> > -	debug("%s: Enter, %p, %p, %p, %p\n", __func__, regs, msg, db, cert);
> > +	debug("%s: Enter, %p, %p, %p, %p\n", __func__, regs, msg, db, dbx);
> >
> >  	if (!db)
> >  		goto out;
> > @@ -333,7 +417,7 @@ bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
> >  		debug("%s: Verify unsigned image with db\n", __func__);
> >  		for (siglist = db; siglist; siglist = siglist->next)
> >  			if (efi_signature_verify_with_list(regs, NULL, NULL,
> > -							   siglist, cert)) {
> > +							   siglist, &cert)) {
> >  				verified = true;
> >  				goto out;
> >  			}
> > @@ -349,12 +433,21 @@ bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,
> >
> >  		for (siglist = db; siglist; siglist = siglist->next) {
> >  			if (efi_signature_verify_with_list(regs, msg, info,
> > -							   siglist, cert)) {
> > -				verified = true;
> > -				goto out;
> > -			}
> > +							   siglist, &cert))
> > +				break;
> >  		}
> > +		if (!siglist) {
> > +			debug("No valid certificate in \"db\"\n");
> > +			goto out;
> > +		}
> > +
> > +		if (!dbx || efi_signature_check_revocation(info, cert, dbx))
> > +			continue;
> > +
> > +		debug("Certificate in \"dbx\"\n");
> > +		goto out;
> >  	}
> > +	verified = true;
> >
> >  out:
> >  	debug("%s: Exit, verified: %d\n", __func__, verified);
> > @@ -362,150 +455,37 @@ out:
> >  }
> >
> >  /**
> > - * efi_search_siglist - search signature list for a certificate
> > - * @cert:	x509 certificate
> > - * @siglist:	Signature list
> > - * @revoc_time:	Pointer to buffer for revocation time
> > - *
> > - * Search signature list pointed to by @siglist and find a certificate
> > - * pointed to by @cert.
> > - * If found, revocation time that is specified in signature database is
> > - * returned in @revoc_time.
> > - *
> > - * Return:	true if certificate is found, false if not
> > - */
> > -static bool efi_search_siglist(struct x509_certificate *cert,
> > -			       struct efi_signature_store *siglist,
> > -			       time64_t *revoc_time)
> > -{
> > -	struct image_region reg[1];
> > -	void *hash = NULL, *msg = NULL;
> > -	struct efi_sig_data *sig_data;
> > -	bool found = false;
> > -
> > -	/* can be null */
> > -	if (!siglist->sig_data_list)
> > -		return false;
> > -
> > -	if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509_sha256)) {
> > -		/* TODO: other hash algos */
> > -		debug("Certificate's digest type is not supported: %pUl\n",
> > -		      &siglist->sig_type);
> > -		goto out;
> > -	}
> > -
> > -	/* calculate hash of TBSCertificate */
> > -	msg = calloc(1, SHA256_SUM_LEN);
> > -	if (!msg) {
> > -		debug("Out of memory\n");
> > -		goto out;
> > -	}
> > -
> > -	hash = calloc(1, SHA256_SUM_LEN);
> > -	if (!hash) {
> > -		debug("Out of memory\n");
> > -		goto out;
> > -	}
> > -
> > -	reg[0].data = cert->tbs;
> > -	reg[0].size = cert->tbs_size;
> > -	hash_calculate("sha256", reg, 1, msg);
> > -
> > -	/* go through signature list */
> > -	for (sig_data = siglist->sig_data_list; sig_data;
> > -	     sig_data = sig_data->next) {
> > -		/*
> > -		 * struct efi_cert_x509_sha256 {
> > -		 *	u8 tbs_hash[256/8];
> > -		 *	time64_t revocation_time;
> > -		 * };
> > -		 */
> > -		if ((sig_data->size >= SHA256_SUM_LEN + sizeof(time64_t)) &&
> > -		    !memcmp(sig_data->data, msg, SHA256_SUM_LEN)) {
> > -			memcpy(revoc_time, sig_data->data + SHA256_SUM_LEN,
> > -			       sizeof(*revoc_time));
> > -			debug("revocation time: 0x%llx\n", *revoc_time);
> > -			found = true;
> > -			goto out;
> > -		}
> > -	}
> > -
> > -out:
> > -	free(hash);
> > -	free(msg);
> > -
> > -	return found;
> > -}
> > -
> > -/**
> > - * efi_signature_verify_cert - verify a certificate with dbx
> > - * @cert:	x509 certificate
> > - * @dbx:	Signature database
> > - *
> > - * Search signature database pointed to by @dbx and find a certificate
> > - * pointed to by @cert.
> > - * This function is expected to be used against "dbx".
> > - *
> > - * Return:	true if a certificate is not rejected, false otherwise.
> > - */
> > -bool efi_signature_verify_cert(struct x509_certificate *cert,
> > -			       struct efi_signature_store *dbx)
> > -{
> > -	struct efi_signature_store *siglist;
> > -	time64_t revoc_time;
> > -	bool found = false;
> > -
> > -	debug("%s: Enter, %p, %p\n", __func__, dbx, cert);
> > -
> > -	if (!cert)
> > -		return false;
> > -
> > -	for (siglist = dbx; siglist; siglist = siglist->next) {
> > -		if (efi_search_siglist(cert, siglist, &revoc_time)) {
> > -			/* TODO */
> > -			/* compare signing time with revocation time */
> > -
> > -			found = true;
> > -			break;
> > -		}
> > -	}
> > -
> > -	debug("%s: Exit, verified: %d\n", __func__, !found);
> > -	return !found;
> > -}
> > -
> > -/**
> > - * efi_signature_verify_signers - verify signers' certificates with dbx
> > + * efi_signature_check_signers - check revocation against all signers with dbx
> >   * @msg:	Signature
> > - * @dbx:	Signature database
> > + * @dbx:	Revocation signature database
> >   *
> > - * Determine if any of signers' certificates in @msg may be verified
> > - * by any of certificates in signature database pointed to by @dbx.
> > - * This function is expected to be used against "dbx".
> > + * Determine if none of signers' certificates in @msg are revoked
> > + * by signature database pointed to by @dbx.
> >   *
> > - * Return:	true if none of certificates is rejected, false otherwise.
> > + * Return:	true if all signers passed, false otherwise.
> >   */
> > -bool efi_signature_verify_signers(struct pkcs7_message *msg,
> > -				  struct efi_signature_store *dbx)
> > +bool efi_signature_check_signers(struct pkcs7_message *msg,
> > +				 struct efi_signature_store *dbx)
> >  {
> > -	struct pkcs7_signed_info *info;
> > -	bool found = false;
> > +	struct pkcs7_signed_info *sinfo;
> > +	bool revoked = false;
> >
> >  	debug("%s: Enter, %p, %p\n", __func__, msg, dbx);
> >
> > -	if (!msg)
> > +	if (!msg || !dbx)
> >  		goto out;
> >
> > -	for (info = msg->signed_infos; info; info = info->next) {
> > -		if (info->signer &&
> > -		    !efi_signature_verify_cert(info->signer, dbx)) {
> > -			found = true;
> > -			goto out;
> > +	for (sinfo = msg->signed_infos; sinfo; sinfo = sinfo->next) {
> > +		if (sinfo->signer &&
> > +		    !efi_signature_check_revocation(sinfo, sinfo->signer,
> > +						    dbx)) {
> > +			revoked = true;
> > +			break;
> >  		}
> >  	}
> >  out:
> > -	debug("%s: Exit, verified: %d\n", __func__, !found);
> > -	return !found;
> > +	debug("%s: Exit, revoked: %d\n", __func__, revoked);
> > +	return !revoked;
> >  }
> >
> >  /**
> >
> 


More information about the U-Boot mailing list