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.aka...@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;
> > }
> >
> > /**
> >
>
