| Message ID | 20200814053924.342094-1-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] efi_loader: signature: correct a behavior against multiple signatures | expand |
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; >
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"); - 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;
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(-) -- 2.28.0