From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Wed, 29 Jul 2020 11:49:51 +0900 Subject: [PATCH v5 6/8] efi_loader: signature: rework for intermediate certificates support In-Reply-To: <19bd9bda-aab8-a202-e78d-9e8d397749f3@gmx.de> References: <20200721103524.5956-1-takahiro.akashi@linaro.org> <20200721103524.5956-7-takahiro.akashi@linaro.org> <19bd9bda-aab8-a202-e78d-9e8d397749f3@gmx.de> Message-ID: <20200729024951.GA11887@laputa> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Heinrich, On Wed, Jul 22, 2020 at 01:00:30PM +0200, Heinrich Schuchardt wrote: > On 21.07.20 12:35, AKASHI Takahiro wrote: > > In this commit, efi_signature_verify(with_sigdb) will be re-implemented > > using pcks7_verify_one() in order to support certificates chain, where > > the signer's certificate will be signed by an intermediate CA (certificate > > authority) and the latter's certificate will also be signed by another CA > > and so on. > > > > What we need to do here is to search for certificates in a signature, > > build up a chain of certificates and verify one by one. pkcs7_verify_one() > > handles most of these steps except the last one. > > > > pkcs7_verify_one() returns, if succeeded, the last certificate to verify, > > which can be either a self-signed one or one that should be signed by one > > of certificates in "db". Re-worked efi_signature_verify() will take care > > of this step. > > > > Signed-off-by: AKASHI Takahiro > > --- > > include/efi_loader.h | 8 +- > > lib/efi_loader/Kconfig | 1 + > > lib/efi_loader/efi_image_loader.c | 2 +- > > lib/efi_loader/efi_signature.c | 385 ++++++++++++++---------------- > > lib/efi_loader/efi_variable.c | 5 +- > > 5 files changed, 188 insertions(+), 213 deletions(-) > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index 98944640bee7..df8dc377257c 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -773,10 +773,10 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs, > > bool efi_signature_verify_one(struct efi_image_regions *regs, > > struct pkcs7_message *msg, > > struct efi_signature_store *db); > > -bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs, > > - struct pkcs7_message *msg, > > - struct efi_signature_store *db, > > - struct efi_signature_store *dbx); > > +bool efi_signature_verify(struct efi_image_regions *regs, > > + struct pkcs7_message *msg, > > + struct efi_signature_store *db, > > + struct efi_signature_store *dbx); > > bool efi_signature_check_signers(struct pkcs7_message *msg, > > struct efi_signature_store *dbx); > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > index 6017ffe9a600..bad1a29ba804 100644 > > --- a/lib/efi_loader/Kconfig > > +++ b/lib/efi_loader/Kconfig > > @@ -205,6 +205,7 @@ config EFI_SECURE_BOOT > > select ASYMMETRIC_PUBLIC_KEY_SUBTYPE > > select X509_CERTIFICATE_PARSER > > select PKCS7_MESSAGE_PARSER > > + select PKCS7_VERIFY > > default n > > help > > Select this option to enable EFI secure boot support. > > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c > > index d81ae8c93a52..d930811141af 100644 > > --- a/lib/efi_loader/efi_image_loader.c > > +++ b/lib/efi_loader/efi_image_loader.c > > @@ -642,7 +642,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) > > } > > > > /* try white-list */ > > - if (efi_signature_verify_with_sigdb(regs, msg, db, dbx)) > > + if (efi_signature_verify(regs, msg, db, dbx)) > > continue; > > > > debug("Signature was not verified by \"db\"\n"); > > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c > > index 8413d83e343b..7b33a4010fe8 100644 > > --- a/lib/efi_loader/efi_signature.c > > +++ b/lib/efi_loader/efi_signature.c > > @@ -10,7 +10,9 @@ > > #include > > #include > > #include > > +#include > > #include > > +#include > > #include > > This line does not exist in origin master. Which patch is missing? The line is what you deleted when you merged this file while I said that the header should be directly included because this file uses an x509 parser function explicitly. I still believe that it should. > > Using sbsigntool 0.9.4 and this whole series applied 'make test' fails > > test/py/tests/test_efi_secboot/test_authvar.py FFFFF > test/py/tests/test_efi_secboot/test_signed.py .FF.FF > test/py/tests/test_efi_secboot/test_signed_intca.py .FF > test/py/tests/test_efi_secboot/test_unsigned.py ..F I rebased v5 to your *current* efi-2020-10 and tried to reproduce the problem but failed. All the test cases passed. -Takahiro Akashi > Up to patch 5 make test succeeds. I will take patches 1-5 into my next > pull-request. > > Anyway we have to wait for sbsigntool 0.9.4 in the Docker image and > Travis CI. > > Best regards > > Heinrich > > > > #include > > #include > > @@ -61,143 +63,6 @@ static bool efi_hash_regions(struct image_region *regs, int count, > > return true; > > } > > > > -/** > > - * efi_hash_msg_content - calculate a hash value of contentInfo > > - * @msg: Signature > > - * @hash: Pointer to a pointer to buffer holding a hash value > > - * @size: Size of buffer to be returned > > - * > > - * Calculate a sha256 value of contentInfo in @msg and return a value in @hash. > > - * > > - * Return: true on success, false on error > > - */ > > -static bool efi_hash_msg_content(struct pkcs7_message *msg, void **hash, > > - size_t *size) > > -{ > > - struct image_region regtmp; > > - > > - regtmp.data = msg->data; > > - regtmp.size = msg->data_len; > > - > > - return efi_hash_regions(®tmp, 1, hash, size); > > -} > > - > > -/** > > - * efi_signature_verify - verify a signature with a certificate > > - * @regs: List of regions to be authenticated > > - * @signed_info: Pointer to PKCS7's signed_info > > - * @cert: x509 certificate > > - * > > - * Signature pointed to by @signed_info against image pointed to by @regs > > - * is verified by a certificate pointed to by @cert. > > - * @signed_info holds a signature, including a message digest which is to be > > - * compared with a hash value calculated from @regs. > > - * > > - * Return: true if signature is verified, false if not > > - */ > > -static bool efi_signature_verify(struct efi_image_regions *regs, > > - struct pkcs7_message *msg, > > - struct pkcs7_signed_info *ps_info, > > - struct x509_certificate *cert) > > -{ > > - struct image_sign_info info; > > - struct image_region regtmp[2]; > > - void *hash; > > - size_t size; > > - char c; > > - bool verified; > > - > > - EFI_PRINT("%s: Enter, %p, %p, %p(issuer: %s, subject: %s)\n", __func__, > > - regs, ps_info, cert, cert->issuer, cert->subject); > > - > > - verified = false; > > - > > - memset(&info, '\0', sizeof(info)); > > - info.padding = image_get_padding_algo("pkcs-1.5"); > > - /* > > - * Note: image_get_[checksum|crypto]_algo takes an string > > - * argument like "," > > - * TODO: support other hash algorithms > > - */ > > - if (!strcmp(ps_info->sig->hash_algo, "sha1")) { > > - info.checksum = image_get_checksum_algo("sha1,rsa2048"); > > - info.name = "sha1,rsa2048"; > > - } else if (!strcmp(ps_info->sig->hash_algo, "sha256")) { > > - info.checksum = image_get_checksum_algo("sha256,rsa2048"); > > - info.name = "sha256,rsa2048"; > > - } else { > > - EFI_PRINT("unknown msg digest algo: %s\n", > > - ps_info->sig->hash_algo); > > - goto out; > > - } > > - info.crypto = image_get_crypto_algo(info.name); > > - > > - info.key = cert->pub->key; > > - info.keylen = cert->pub->keylen; > > - > > - /* verify signature */ > > - EFI_PRINT("%s: crypto: %s, signature len:%x\n", __func__, > > - info.name, ps_info->sig->s_size); > > - if (ps_info->aa_set & (1UL << sinfo_has_message_digest)) { > > - EFI_PRINT("%s: RSA verify authentication attribute\n", > > - __func__); > > - /* > > - * NOTE: This path will be executed only for > > - * PE image authentication > > - */ > > - > > - /* check if hash matches digest first */ > > - EFI_PRINT("checking msg digest first, len:0x%x\n", > > - ps_info->msgdigest_len); > > - > > -#ifdef DEBUG > > - EFI_PRINT("hash in database:\n"); > > - print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, > > - ps_info->msgdigest, ps_info->msgdigest_len, > > - false); > > -#endif > > - /* against contentInfo first */ > > - hash = NULL; > > - if ((msg->data && efi_hash_msg_content(msg, &hash, &size)) || > > - /* for signed image */ > > - efi_hash_regions(regs->reg, regs->num, &hash, &size)) { > > - /* for authenticated variable */ > > - if (ps_info->msgdigest_len != size || > > - memcmp(hash, ps_info->msgdigest, size)) { > > - EFI_PRINT("Digest doesn't match\n"); > > - free(hash); > > - goto out; > > - } > > - > > - free(hash); > > - } else { > > - EFI_PRINT("Digesting image failed\n"); > > - goto out; > > - } > > - > > - /* against digest */ > > - c = 0x31; > > - regtmp[0].data = &c; > > - regtmp[0].size = 1; > > - regtmp[1].data = ps_info->authattrs; > > - regtmp[1].size = ps_info->authattrs_len; > > - > > - if (!rsa_verify(&info, regtmp, 2, > > - ps_info->sig->s, ps_info->sig->s_size)) > > - verified = true; > > - } else { > > - EFI_PRINT("%s: RSA verify content data\n", __func__); > > - /* against all data */ > > - if (!rsa_verify(&info, regs->reg, regs->num, > > - ps_info->sig->s, ps_info->sig->s_size)) > > - verified = true; > > - } > > - > > -out: > > - EFI_PRINT("%s: Exit, verified: %d\n", __func__, verified); > > - return verified; > > -} > > - > > /** > > * efi_signature_lookup_digest - search for an image's digest in sigdb > > * @regs: List of regions to be authenticated > > @@ -261,61 +126,127 @@ out: > > } > > > > /** > > - * efi_signature_verify_with_list - verify a signature with signature list > > - * @regs: List of regions to be authenticated > > - * @msg: Signature > > - * @signed_info: Pointer to PKCS7's signed_info > > - * @siglist: Signature list for certificates > > - * @valid_cert: x509 certificate that verifies this signature > > + * efi_lookup_certificate - find a certificate within db > > + * @msg: Signature > > + * @db: Signature database > > * > > - * Signature pointed to by @signed_info against image pointed to by @regs > > - * is verified by signature list pointed to by @siglist. > > - * Signature database is a simple concatenation of one or more > > - * signature list(s). > > + * Search signature database pointed to by @db and find a certificate > > + * pointed to by @cert. > > * > > - * Return: true if signature is verified, false if not > > + * Return: true if found, false otherwise. > > */ > > -static > > -bool efi_signature_verify_with_list(struct efi_image_regions *regs, > > - struct pkcs7_message *msg, > > - struct pkcs7_signed_info *signed_info, > > - struct efi_signature_store *siglist, > > - struct x509_certificate **valid_cert) > > +static bool efi_lookup_certificate(struct x509_certificate *cert, > > + struct efi_signature_store *db) > > { > > - struct x509_certificate *cert; > > + struct efi_signature_store *siglist; > > struct efi_sig_data *sig_data; > > - bool verified = false; > > + struct image_region reg[1]; > > + void *hash = NULL, *hash_tmp = NULL; > > + size_t size = 0; > > + bool found = false; > > > > - EFI_PRINT("%s: Enter, %p, %p, %p, %p\n", __func__, > > - regs, signed_info, siglist, valid_cert); > > + EFI_PRINT("%s: Enter, %p, %p\n", __func__, cert, db); > > > > - if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509)) { > > - EFI_PRINT("Signature type is not supported: %pUl\n", > > - &siglist->sig_type); > > + if (!cert || !db || !db->sig_data_list) > > goto out; > > - } > > > > - /* go through the list */ > > - for (sig_data = siglist->sig_data_list; sig_data; > > - sig_data = sig_data->next) { > > - /* TODO: support owner check based on policy */ > > + /* > > + * TODO: identify a certificate using sha256 digest > > + * Is there any better way? > > + */ > > + /* 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; > > > > - cert = x509_cert_parse(sig_data->data, sig_data->size); > > - if (IS_ERR(cert)) { > > - EFI_PRINT("Parsing x509 certificate failed\n"); > > - goto out; > > + EFI_PRINT("%s: searching for %s\n", __func__, cert->subject); > > + for (siglist = db; siglist; siglist = siglist->next) { > > + /* only with x509 certificate */ > > + if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509)) > > + continue; > > + > > + for (sig_data = siglist->sig_data_list; sig_data; > > + sig_data = sig_data->next) { > > + struct x509_certificate *cert_tmp; > > + > > + cert_tmp = x509_cert_parse(sig_data->data, > > + sig_data->size); > > + if (IS_ERR_OR_NULL(cert_tmp)) > > + continue; > > + > > + reg[0].data = cert_tmp->tbs; > > + reg[0].size = cert_tmp->tbs_size; > > + if (!efi_hash_regions(reg, 1, &hash_tmp, NULL)) > > + goto out; > > + > > + x509_free_certificate(cert_tmp); > > + > > + if (!memcmp(hash, hash_tmp, size)) { > > + found = true; > > + goto out; > > + } > > } > > + } > > +out: > > + free(hash); > > + free(hash_tmp); > > > > - verified = efi_signature_verify(regs, msg, signed_info, cert); > > + EFI_PRINT("%s: Exit, found: %d\n", __func__, found); > > + return found; > > +} > > > > - if (verified) { > > - if (valid_cert) > > - *valid_cert = cert; > > - else > > - x509_free_certificate(cert); > > - break; > > +/** > > + * efi_verify_certificate - verify certificate's signature with database > > + * @signer: Certificate > > + * @db: Signature database > > + * @root: Certificate to verify @signer > > + * > > + * Determine if certificate pointed to by @signer may be verified > > + * by one of certificates in signature database pointed to by @db. > > + * > > + * Return: true if certificate is verified, false otherwise. > > + */ > > +static bool efi_verify_certificate(struct x509_certificate *signer, > > + struct efi_signature_store *db, > > + struct x509_certificate **root) > > +{ > > + struct efi_signature_store *siglist; > > + struct efi_sig_data *sig_data; > > + struct x509_certificate *cert; > > + bool verified = false; > > + int ret; > > + > > + EFI_PRINT("%s: Enter, %p, %p\n", __func__, signer, db); > > + > > + if (!signer || !db || !db->sig_data_list) > > + goto out; > > + > > + for (siglist = db; siglist; siglist = siglist->next) { > > + /* only with x509 certificate */ > > + if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509)) > > + continue; > > + > > + for (sig_data = siglist->sig_data_list; sig_data; > > + sig_data = sig_data->next) { > > + cert = x509_cert_parse(sig_data->data, sig_data->size); > > + if (IS_ERR_OR_NULL(cert)) { > > + EFI_PRINT("Cannot parse x509 certificate\n"); > > + continue; > > + } > > + > > + ret = public_key_verify_signature(cert->pub, > > + signer->sig); > > + if (!ret) { > > + verified = true; > > + if (root) > > + *root = cert; > > + else > > + x509_free_certificate(cert); > > + goto out; > > + } > > + x509_free_certificate(cert); > > } > > - x509_free_certificate(cert); > > } > > > > out: > > @@ -423,9 +354,9 @@ bool efi_signature_verify_one(struct efi_image_regions *regs, > > struct efi_signature_store *db) > > { > > struct pkcs7_signed_info *sinfo; > > - struct efi_signature_store *siglist; > > - struct x509_certificate *cert; > > + struct x509_certificate *signer; > > bool verified = false; > > + int ret; > > > > EFI_PRINT("%s: Enter, %p, %p, %p\n", __func__, regs, msg, db); > > > > @@ -440,13 +371,29 @@ bool efi_signature_verify_one(struct efi_image_regions *regs, > > EFI_PRINT("Signed Info: digest algo: %s, pkey algo: %s\n", > > sinfo->sig->hash_algo, sinfo->sig->pkey_algo); > > > > - for (siglist = db; siglist; siglist = siglist->next) > > - if (efi_signature_verify_with_list(regs, msg, sinfo, > > - siglist, &cert)) { > > + 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; > > } > > - EFI_PRINT("Valid certificate not in \"db\"\n"); > > + } else if (efi_verify_certificate(signer, db, NULL)) { > > + verified = true; > > + goto out; > > + } > > + EFI_PRINT("Valid certificate not in db\n"); > > } > > > > out: > > @@ -454,8 +401,8 @@ out: > > return verified; > > } > > > > -/** > > - * efi_signature_verify_with_sigdb - verify signatures with db and dbx > > +/* > > + * efi_signature_verify - verify signatures with db and dbx > > * @regs: List of regions to be authenticated > > * @msg: Signature > > * @db: Signature database for trusted certificates > > @@ -466,43 +413,71 @@ out: > > * > > * 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 efi_signature_store *dbx) > > +bool efi_signature_verify(struct efi_image_regions *regs, > > + struct pkcs7_message *msg, > > + struct efi_signature_store *db, > > + struct efi_signature_store *dbx) > > { > > - struct pkcs7_signed_info *info; > > - struct efi_signature_store *siglist; > > - struct x509_certificate *cert; > > + struct pkcs7_signed_info *sinfo; > > + struct x509_certificate *signer, *root; > > bool verified = false; > > + int ret; > > > > EFI_PRINT("%s: Enter, %p, %p, %p, %p\n", __func__, regs, msg, db, dbx); > > > > if (!regs || !msg || !db || !db->sig_data_list) > > goto out; > > > > - for (info = msg->signed_infos; info; info = info->next) { > > + for (sinfo = msg->signed_infos; sinfo; sinfo = sinfo->next) { > > EFI_PRINT("Signed Info: digest algo: %s, pkey algo: %s\n", > > - info->sig->hash_algo, info->sig->pkey_algo); > > + sinfo->sig->hash_algo, sinfo->sig->pkey_algo); > > > > - for (siglist = db; siglist; siglist = siglist->next) { > > - if (efi_signature_verify_with_list(regs, msg, info, > > - siglist, &cert)) > > - break; > > - } > > - if (!siglist) { > > - EFI_PRINT("Valid certificate not in \"db\"\n"); > > + /* > > + * only for authenticated variable. > > + * > > + * If this function is called for image, > > + * hash calculation will be done in > > + * pkcs7_verify_one(). > > + */ > > + if (!msg->data && > > + !efi_hash_regions(regs->reg, regs->num, > > + (void **)&sinfo->sig->digest, NULL)) { > > + EFI_PRINT("Digesting an image failed\n"); > > goto out; > > } > > > > - if (!dbx || efi_signature_check_revocation(info, cert, dbx)) > > + EFI_PRINT("Verifying certificate chain\n"); > > + signer = NULL; > > + ret = pkcs7_verify_one(msg, sinfo, &signer); > > + if (ret == -ENOPKG) > > continue; > > > > - EFI_PRINT("Certificate in \"dbx\"\n"); > > + if (ret < 0 || !signer) > > + goto out; > > + > > + if (sinfo->blacklisted) > > + goto out; > > + > > + EFI_PRINT("Verifying last certificate in chain\n"); > > + if (signer->self_signed) { > > + if (efi_lookup_certificate(signer, db)) > > + if (efi_signature_check_revocation(sinfo, > > + signer, dbx)) > > + continue; > > + } else if (efi_verify_certificate(signer, db, &root)) { > > + bool check; > > + > > + check = efi_signature_check_revocation(sinfo, root, > > + dbx); > > + x509_free_certificate(root); > > + if (check) > > + continue; > > + } > > + > > + EFI_PRINT("Certificate chain didn't reach trusted CA\n"); > > goto out; > > } > > verified = true; > > - > > out: > > EFI_PRINT("%s: Exit, verified: %d\n", __func__, verified); > > return verified; > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > > index 39a848290380..6b5c5c45dc1d 100644 > > --- a/lib/efi_loader/efi_variable.c > > +++ b/lib/efi_loader/efi_variable.c > > @@ -241,12 +241,11 @@ static efi_status_t efi_variable_authenticate(u16 *variable, > > } > > > > /* verify signature */ > > - if (efi_signature_verify_with_sigdb(regs, var_sig, truststore, NULL)) { > > + if (efi_signature_verify(regs, var_sig, truststore, NULL)) { > > EFI_PRINT("Verified\n"); > > } else { > > if (truststore2 && > > - efi_signature_verify_with_sigdb(regs, var_sig, > > - truststore2, NULL)) { > > + efi_signature_verify(regs, var_sig, truststore2, NULL)) { > > EFI_PRINT("Verified\n"); > > } else { > > EFI_PRINT("Verifying variable's signature failed\n"); > > >