[PATCH 1/2] efi_loader: signature: correct a behavior against multiple signatures
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");
- 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;
The test case 5 in test_signed (multiple signatures) must be modified and aligned with the change introduced in the previous commit ("efi_loader: signature: correct a behavior against multiple signatures").
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- test/py/tests/test_efi_secboot/conftest.py | 5 +--- test/py/tests/test_efi_secboot/test_signed.py | 26 +++++++++++++------ 2 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py index bf27d995b1c2..69a498ca003c 100644 --- a/test/py/tests/test_efi_secboot/conftest.py +++ b/test/py/tests/test_efi_secboot/conftest.py @@ -70,9 +70,6 @@ def efi_boot_env(request, u_boot_config): check_call('cd %s; %scert-to-efi-sig-list -g %s db1.crt db1.esl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key db db1.esl db1.auth' % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), shell=True) - # db1-update - check_call('cd %s; %ssign-efi-sig-list -t "2020-04-06" -a -c KEK.crt -k KEK.key db db1.esl db1-update.auth' - % (mnt_point, EFITOOLS_PATH), shell=True) # dbx (TEST_dbx certificate) check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_dbx/ -keyout dbx.key -out dbx.crt -nodes -days 365' % mnt_point, shell=True) @@ -84,7 +81,7 @@ def efi_boot_env(request, u_boot_config): % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), shell=True) # dbx_hash1 (digest of TEST_db1 certificate) - check_call('cd %s; %scert-to-efi-hash-list -g %s -t 0 -s 256 db1.crt dbx_hash1.crl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx dbx_hash1.crl dbx_hash1.auth' + check_call('cd %s; %scert-to-efi-hash-list -g %s -t 0 -s 256 db1.crt dbx_hash1.crl; %ssign-efi-sig-list -t "2020-04-06" -c KEK.crt -k KEK.key dbx dbx_hash1.crl dbx_hash1.auth' % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH), shell=True) # dbx_db (with TEST_db certificate) diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py index 7531bbac6a5f..1443ba7b62be 100644 --- a/test/py/tests/test_efi_secboot/test_signed.py +++ b/test/py/tests/test_efi_secboot/test_signed.py @@ -157,7 +157,8 @@ class TestEfiSignedImage(object): u_boot_console.restart_uboot() disk_img = efi_boot_env with u_boot_console.log.section('Test Case 5a'): - # Test Case 5a, rejected if any of signatures is not verified + # Test Case 5a, authenticated even if only one of signatures + # is verified output = u_boot_console.run_command_list([ 'host bind 0 %s' % disk_img, 'fatload host 0:1 4000000 db.auth', @@ -171,8 +172,7 @@ class TestEfiSignedImage(object): 'efidebug boot add 1 HELLO host 0:1 /helloworld.efi.signed_2sigs ""', 'efidebug boot next 1', 'efidebug test bootmgr']) - assert ''HELLO' failed' in ''.join(output) - assert 'efi_start_image() returned: 26' in ''.join(output) + assert 'Hello, world!' in ''.join(output)
with u_boot_console.log.section('Test Case 5b'): # Test Case 5b, authenticated if both signatures are verified @@ -181,19 +181,29 @@ class TestEfiSignedImage(object): 'setenv -e -nv -bs -rt -at -a -i 4000000,$filesize db']) assert 'Failed to set EFI variable' not in ''.join(output) output = u_boot_console.run_command_list([ - 'efidebug boot add 1 HELLO host 0:1 /helloworld.efi.signed_2sigs ""', 'efidebug boot next 1', - 'bootefi bootmgr']) + 'efidebug test bootmgr']) assert 'Hello, world!' in ''.join(output)
with u_boot_console.log.section('Test Case 5c'): - # Test Case 5c, rejected if any of signatures is revoked + # Test Case 5c, not rejected if one of signatures (digest of + # certificate) is revoked output = u_boot_console.run_command_list([ - 'fatload host 0:1 4000000 dbx_hash1.auth', + 'fatload host 0:1 4000000 dbx_hash.auth', 'setenv -e -nv -bs -rt -at -i 4000000,$filesize dbx']) assert 'Failed to set EFI variable' not in ''.join(output) output = u_boot_console.run_command_list([ - 'efidebug boot add 1 HELLO host 0:1 /helloworld.efi.signed_2sigs ""', + 'efidebug boot next 1', + 'efidebug test bootmgr']) + assert 'Hello, world!' in ''.join(output) + + with u_boot_console.log.section('Test Case 5d'): + # Test Case 5d, rejected if both of signatures are revoked + output = u_boot_console.run_command_list([ + 'fatload host 0:1 4000000 dbx_hash1.auth', + 'setenv -e -nv -bs -rt -at -a -i 4000000,$filesize dbx']) + assert 'Failed to set EFI variable' not in ''.join(output) + output = u_boot_console.run_command_list([ 'efidebug boot next 1', 'efidebug test bootmgr']) assert ''HELLO' failed' in ''.join(output)
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;@@ -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;
participants (2)
-
AKASHI Takahiro -
Heinrich Schuchardt