[PATCH 1/2] efi_loader: fix dual signed image certification
The EFI spec allows for images to carry multiple signatures. Currently we don't adhere to the verification process for such images.
The spec 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."
With our current implementation signing the image with two certificates and inserting both of them in db and one of them dbx doesn't always reject the image. The rejection depends on the order that the image was signed and the order the certificates are read (and checked) in db.
While at it move the sha256 hash verification outside the signature checking loop, since it only needs to run once per image and get simplify the logic for authenticating an unsigned imahe using sha256 hashes.
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- changes since RFC: - none lib/efi_loader/efi_image_loader.c | 88 +++++++------------------------ 1 file changed, 18 insertions(+), 70 deletions(-)
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index f41cfa4fccd5..5df35939f702 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -516,53 +516,6 @@ err: }
#ifdef CONFIG_EFI_SECURE_BOOT -/** - * efi_image_unsigned_authenticate() - authenticate unsigned image with - * SHA256 hash - * @regs: List of regions to be verified - * - * If an image is not signed, it doesn't have a signature. In this case, - * its message digest is calculated and it will be compared with one of - * hash values stored in signature databases. - * - * Return: true if authenticated, false if not - */ -static bool efi_image_unsigned_authenticate(struct efi_image_regions *regs) -{ - struct efi_signature_store *db = NULL, *dbx = NULL; - bool ret = false; - - dbx = efi_sigstore_parse_sigdb(u"dbx"); - if (!dbx) { - EFI_PRINT("Getting signature database(dbx) failed\n"); - goto out; - } - - db = efi_sigstore_parse_sigdb(u"db"); - if (!db) { - EFI_PRINT("Getting signature database(db) failed\n"); - goto out; - } - - /* try black-list first */ - if (efi_signature_lookup_digest(regs, dbx, true)) { - EFI_PRINT("Image is not signed and its digest found in "dbx"\n"); - goto out; - } - - /* try white-list */ - if (efi_signature_lookup_digest(regs, db, false)) - ret = true; - else - EFI_PRINT("Image is not signed and its digest not found in "db" or "dbx"\n"); - -out: - efi_sigstore_free(db); - efi_sigstore_free(dbx); - - return ret; -} - /** * efi_image_authenticate() - verify a signature of signed image * @efi: Pointer to image @@ -608,14 +561,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) if (!efi_image_parse(new_efi, efi_size, ®s, &wincerts, &wincerts_len)) { EFI_PRINT("Parsing PE executable image failed\n"); - goto err; - } - - if (!wincerts) { - /* The image is not signed */ - ret = efi_image_unsigned_authenticate(regs); - - goto err; + goto out; }
/* @@ -624,18 +570,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) db = efi_sigstore_parse_sigdb(u"db"); if (!db) { EFI_PRINT("Getting signature database(db) failed\n"); - goto err; + goto out; }
dbx = efi_sigstore_parse_sigdb(u"dbx"); if (!dbx) { EFI_PRINT("Getting signature database(dbx) failed\n"); - goto err; + goto out; }
if (efi_signature_lookup_digest(regs, dbx, true)) { EFI_PRINT("Image's digest was found in "dbx"\n"); - goto err; + goto out; }
/* @@ -678,7 +624,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) { EFI_PRINT("Certificate type not supported: %pUs\n", auth); - continue; + ret = false; + goto out; }
auth += sizeof(efi_guid_t); @@ -686,7 +633,8 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) } else if (wincert->wCertificateType != WIN_CERT_TYPE_PKCS_SIGNED_DATA) { EFI_PRINT("Certificate type not supported\n"); - continue; + ret = false; + goto out; }
msg = pkcs7_parse_message(auth, auth_size); @@ -717,32 +665,32 @@ static bool efi_image_authenticate(void *efi, size_t efi_size) */ /* try black-list first */ if (efi_signature_verify_one(regs, msg, dbx)) { + ret = false; EFI_PRINT("Signature was rejected by "dbx"\n"); - continue; + goto out; }
if (!efi_signature_check_signers(msg, dbx)) { + ret = false; EFI_PRINT("Signer(s) in "dbx"\n"); - continue; + goto out; }
/* try white-list */ if (efi_signature_verify(regs, msg, db, dbx)) { ret = true; - break; + continue; }
EFI_PRINT("Signature was not verified by "db"\n"); + }
- if (efi_signature_lookup_digest(regs, db, false)) { - ret = true; - break; - }
- EFI_PRINT("Image's digest was not found in "db" or "dbx"\n"); - } + /* last resort try the image sha256 hash in db */ + if (!ret && efi_signature_lookup_digest(regs, db, false)) + ret = true;
-err: +out: efi_sigstore_free(db); efi_sigstore_free(dbx); pkcs7_free_message(msg);
The previous patch is changing U-Boot's behavior wrt certificate based binary authentication. Specifically an image who's digest of a certificate is found in dbx is now rejected. Fix the test accordingly and add another one testing signatures in reverse order
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org --- changes since RFC: - Added another test cases checking signature hashes in reverse order test/py/tests/test_efi_secboot/test_signed.py | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py index 0aee34479f55..cc9396a11d48 100644 --- a/test/py/tests/test_efi_secboot/test_signed.py +++ b/test/py/tests/test_efi_secboot/test_signed.py @@ -186,7 +186,7 @@ class TestEfiSignedImage(object): assert 'Hello, world!' in ''.join(output)
with u_boot_console.log.section('Test Case 5c'): - # Test Case 5c, not rejected if one of signatures (digest of + # Test Case 5c, rejected if one of signatures (digest of # certificate) is revoked output = u_boot_console.run_command_list([ 'fatload host 0:1 4000000 dbx_hash.auth', @@ -195,7 +195,8 @@ class TestEfiSignedImage(object): output = u_boot_console.run_command_list([ 'efidebug boot next 1', 'efidebug test bootmgr']) - assert 'Hello, world!' in ''.join(output) + assert ''HELLO' failed' in ''.join(output) + assert 'efi_start_image() returned: 26' in ''.join(output)
with u_boot_console.log.section('Test Case 5d'): # Test Case 5d, rejected if both of signatures are revoked @@ -209,6 +210,31 @@ class TestEfiSignedImage(object): assert ''HELLO' failed' in ''.join(output) assert 'efi_start_image() returned: 26' in ''.join(output)
+ # Try rejection in reverse order. + u_boot_console.restart_uboot() + with u_boot_console.log.section('Test Case 5e'): + # Test Case 5e, authenticated even if only one of signatures + # is verified. Same as before but reject dbx_hash1.auth only + output = u_boot_console.run_command_list([ + 'host bind 0 %s' % disk_img, + 'fatload host 0:1 4000000 db.auth', + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize db', + 'fatload host 0:1 4000000 KEK.auth', + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', + 'fatload host 0:1 4000000 PK.auth', + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', + 'fatload host 0:1 4000000 db1.auth', + 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', + 'fatload host 0:1 4000000 dbx_hash1.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 -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""', + 'efidebug boot next 1', + 'efidebug test bootmgr']) + assert ''HELLO' failed' in ''.join(output) + assert 'efi_start_image() returned: 26' in ''.join(output) + def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env): """ Test Case 6 - using digest of signed image in database
Ilias,
On Fri, Feb 11, 2022 at 09:37:50AM +0200, Ilias Apalodimas wrote:
The previous patch is changing U-Boot's behavior wrt certificate based binary authentication. Specifically an image who's digest of a certificate is found in dbx is now rejected. Fix the test accordingly and add another one testing signatures in reverse order
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
changes since RFC:
- Added another test cases checking signature hashes in reverse order
test/py/tests/test_efi_secboot/test_signed.py | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py index 0aee34479f55..cc9396a11d48 100644 --- a/test/py/tests/test_efi_secboot/test_signed.py +++ b/test/py/tests/test_efi_secboot/test_signed.py @@ -186,7 +186,7 @@ class TestEfiSignedImage(object): assert 'Hello, world!' in ''.join(output)
with u_boot_console.log.section('Test Case 5c'):
# Test Case 5c, not rejected if one of signatures (digest of
# Test Case 5c, rejected if one of signatures (digest of # certificate) is revoked output = u_boot_console.run_command_list([ 'fatload host 0:1 4000000 dbx_hash.auth',@@ -195,7 +195,8 @@ class TestEfiSignedImage(object): output = u_boot_console.run_command_list([ 'efidebug boot next 1', 'efidebug test bootmgr'])
assert 'Hello, world!' in ''.join(output)
assert '\'HELLO\' failed' in ''.join(output)assert 'efi_start_image() returned: 26' in ''.join(output) with u_boot_console.log.section('Test Case 5d'): # Test Case 5d, rejected if both of signatures are revoked@@ -209,6 +210,31 @@ class TestEfiSignedImage(object): assert ''HELLO' failed' in ''.join(output) assert 'efi_start_image() returned: 26' in ''.join(output)
# Try rejection in reverse order.
"Reverse order" of what?
u_boot_console.restart_uboot()
I don't think we need 'restart' here. I added it in each test function (not test case), IIRC, because we didn't have file-based non-volatile variables at that time.
with u_boot_console.log.section('Test Case 5e'):# Test Case 5e, authenticated even if only one of signatures# is verified. Same as before but reject dbx_hash1.auth only
Please specify what test case "before" means.
output = u_boot_console.run_command_list(['host bind 0 %s' % disk_img,'fatload host 0:1 4000000 db.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize db','fatload host 0:1 4000000 KEK.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK','fatload host 0:1 4000000 PK.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize PK','fatload host 0:1 4000000 db1.auth','setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db','fatload host 0:1 4000000 dbx_hash1.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])
Now "db" has db.auth and db1.auth in this order and 'dbx" has dbx_hash1.auth. Is this what you intend to test?
-Takahiro Akashi
assert 'Failed to set EFI variable' not in ''.join(output)output = u_boot_console.run_command_list(['efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""','efidebug boot next 1','efidebug test bootmgr'])assert '\'HELLO\' failed' in ''.join(output)assert 'efi_start_image() returned: 26' in ''.join(output)- def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env): """ Test Case 6 - using digest of signed image in database
-- 2.32.0
On Mon, Feb 14, 2022 at 10:50:08AM +0900, AKASHI Takahiro wrote:
Ilias,
On Fri, Feb 11, 2022 at 09:37:50AM +0200, Ilias Apalodimas wrote:
The previous patch is changing U-Boot's behavior wrt certificate based binary authentication. Specifically an image who's digest of a certificate is found in dbx is now rejected. Fix the test accordingly and add another one testing signatures in reverse order
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
changes since RFC:
- Added another test cases checking signature hashes in reverse order
test/py/tests/test_efi_secboot/test_signed.py | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py index 0aee34479f55..cc9396a11d48 100644 --- a/test/py/tests/test_efi_secboot/test_signed.py +++ b/test/py/tests/test_efi_secboot/test_signed.py @@ -186,7 +186,7 @@ class TestEfiSignedImage(object): assert 'Hello, world!' in ''.join(output)
with u_boot_console.log.section('Test Case 5c'):
# Test Case 5c, not rejected if one of signatures (digest of
# Test Case 5c, rejected if one of signatures (digest of # certificate) is revoked output = u_boot_console.run_command_list([ 'fatload host 0:1 4000000 dbx_hash.auth',@@ -195,7 +195,8 @@ class TestEfiSignedImage(object): output = u_boot_console.run_command_list([ 'efidebug boot next 1', 'efidebug test bootmgr'])
assert 'Hello, world!' in ''.join(output)
assert '\'HELLO\' failed' in ''.join(output)assert 'efi_start_image() returned: 26' in ''.join(output) with u_boot_console.log.section('Test Case 5d'): # Test Case 5d, rejected if both of signatures are revoked@@ -209,6 +210,31 @@ class TestEfiSignedImage(object): assert ''HELLO' failed' in ''.join(output) assert 'efi_start_image() returned: 26' in ''.join(output)
# Try rejection in reverse order."Reverse order" of what?
Of the test right above
u_boot_console.restart_uboot()I don't think we need 'restart' here. I added it in each test function (not test case), IIRC, because we didn't have file-based non-volatile variables at that time.
You do. dbx already holds dbx_hash.auth and dbx1_hash.auth (in that order) at that point. The point is cleaning up dbx and testing against dbx1_hash.
with u_boot_console.log.section('Test Case 5e'):# Test Case 5e, authenticated even if only one of signatures# is verified. Same as before but reject dbx_hash1.auth onlyPlease specify what test case "before" means.
The test that run right before that
output = u_boot_console.run_command_list(['host bind 0 %s' % disk_img,'fatload host 0:1 4000000 db.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize db','fatload host 0:1 4000000 KEK.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK','fatload host 0:1 4000000 PK.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize PK','fatload host 0:1 4000000 db1.auth','setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db','fatload host 0:1 4000000 dbx_hash1.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])Now "db" has db.auth and db1.auth in this order and 'dbx" has dbx_hash1.auth. Is this what you intend to test?
Yes. The patchset solved 2 bugs. One was not rejecting the image when a single dbx entry was found. The second was that depending on the order the image was signed and the keys inserted into dbx, the code could reject or accept the image.
-Takahiro Akashi
assert 'Failed to set EFI variable' not in ''.join(output)output = u_boot_console.run_command_list(['efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""','efidebug boot next 1','efidebug test bootmgr'])assert '\'HELLO\' failed' in ''.join(output)assert 'efi_start_image() returned: 26' in ''.join(output)- def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env): """ Test Case 6 - using digest of signed image in database
-- 2.32.0
Regards /Ilias
On Mon, Feb 14, 2022 at 08:18:03AM +0200, Ilias Apalodimas wrote:
On Mon, Feb 14, 2022 at 10:50:08AM +0900, AKASHI Takahiro wrote:
Ilias,
On Fri, Feb 11, 2022 at 09:37:50AM +0200, Ilias Apalodimas wrote:
The previous patch is changing U-Boot's behavior wrt certificate based binary authentication. Specifically an image who's digest of a certificate is found in dbx is now rejected. Fix the test accordingly and add another one testing signatures in reverse order
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
changes since RFC:
- Added another test cases checking signature hashes in reverse order
test/py/tests/test_efi_secboot/test_signed.py | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py index 0aee34479f55..cc9396a11d48 100644 --- a/test/py/tests/test_efi_secboot/test_signed.py +++ b/test/py/tests/test_efi_secboot/test_signed.py @@ -186,7 +186,7 @@ class TestEfiSignedImage(object): assert 'Hello, world!' in ''.join(output)
with u_boot_console.log.section('Test Case 5c'):
# Test Case 5c, not rejected if one of signatures (digest of
# Test Case 5c, rejected if one of signatures (digest of # certificate) is revoked output = u_boot_console.run_command_list([ 'fatload host 0:1 4000000 dbx_hash.auth',@@ -195,7 +195,8 @@ class TestEfiSignedImage(object): output = u_boot_console.run_command_list([ 'efidebug boot next 1', 'efidebug test bootmgr'])
assert 'Hello, world!' in ''.join(output)
assert '\'HELLO\' failed' in ''.join(output)assert 'efi_start_image() returned: 26' in ''.join(output) with u_boot_console.log.section('Test Case 5d'): # Test Case 5d, rejected if both of signatures are revoked@@ -209,6 +210,31 @@ class TestEfiSignedImage(object): assert ''HELLO' failed' in ''.join(output) assert 'efi_start_image() returned: 26' in ''.join(output)
# Try rejection in reverse order."Reverse order" of what?
Of the test right above
Please specify the signature database, I guess "dbx"?
u_boot_console.restart_uboot()I don't think we need 'restart' here. I added it in each test function (not test case), IIRC, because we didn't have file-based non-volatile variables at that time.
You do. dbx already holds dbx_hash.auth and dbx1_hash.auth (in that order) at that point. The point is cleaning up dbx and testing against dbx1_hash.
Why not simply overwrite "dbx" variable? Without "-a", "env set -e" does it if it is properly signed with KEK.
with u_boot_console.log.section('Test Case 5e'):# Test Case 5e, authenticated even if only one of signatures# is verified. Same as before but reject dbx_hash1.auth onlyPlease specify what test case "before" means.
The test that run right before that
Please add a particular test case number to avoid any ambiguity. I believe that a test case description should be easy enough to understand and convey no ambiguity especially if there is some subtle difference between cases.
output = u_boot_console.run_command_list(['host bind 0 %s' % disk_img,'fatload host 0:1 4000000 db.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize db','fatload host 0:1 4000000 KEK.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK','fatload host 0:1 4000000 PK.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize PK','fatload host 0:1 4000000 db1.auth','setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db','fatload host 0:1 4000000 dbx_hash1.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])Now "db" has db.auth and db1.auth in this order and 'dbx" has dbx_hash1.auth. Is this what you intend to test?
Yes. The patchset solved 2 bugs. One was not rejecting the image when a single dbx entry was found. The second was that depending on the order the image was signed and the keys inserted into dbx, the code could reject or accept the image.
Which part of "dbx" (or "db"?) is in a reverse order?
-Takahiro Akashi
-Takahiro Akashi
assert 'Failed to set EFI variable' not in ''.join(output)output = u_boot_console.run_command_list(['efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""','efidebug boot next 1','efidebug test bootmgr'])assert '\'HELLO\' failed' in ''.join(output)assert 'efi_start_image() returned: 26' in ''.join(output)- def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env): """ Test Case 6 - using digest of signed image in database
-- 2.32.0
Regards /Ilias
On Mon, Feb 14, 2022 at 03:36:06PM +0900, AKASHI Takahiro wrote:
On Mon, Feb 14, 2022 at 08:18:03AM +0200, Ilias Apalodimas wrote:
On Mon, Feb 14, 2022 at 10:50:08AM +0900, AKASHI Takahiro wrote:
Ilias,
On Fri, Feb 11, 2022 at 09:37:50AM +0200, Ilias Apalodimas wrote:
The previous patch is changing U-Boot's behavior wrt certificate based binary authentication. Specifically an image who's digest of a certificate is found in dbx is now rejected. Fix the test accordingly and add another one testing signatures in reverse order
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
changes since RFC:
- Added another test cases checking signature hashes in reverse order
test/py/tests/test_efi_secboot/test_signed.py | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py index 0aee34479f55..cc9396a11d48 100644 --- a/test/py/tests/test_efi_secboot/test_signed.py +++ b/test/py/tests/test_efi_secboot/test_signed.py @@ -186,7 +186,7 @@ class TestEfiSignedImage(object): assert 'Hello, world!' in ''.join(output)
with u_boot_console.log.section('Test Case 5c'):
# Test Case 5c, not rejected if one of signatures (digest of
# Test Case 5c, rejected if one of signatures (digest of # certificate) is revoked output = u_boot_console.run_command_list([ 'fatload host 0:1 4000000 dbx_hash.auth',@@ -195,7 +195,8 @@ class TestEfiSignedImage(object): output = u_boot_console.run_command_list([ 'efidebug boot next 1', 'efidebug test bootmgr'])
assert 'Hello, world!' in ''.join(output)
assert '\'HELLO\' failed' in ''.join(output)assert 'efi_start_image() returned: 26' in ''.join(output) with u_boot_console.log.section('Test Case 5d'): # Test Case 5d, rejected if both of signatures are revoked@@ -209,6 +210,31 @@ class TestEfiSignedImage(object): assert ''HELLO' failed' in ''.join(output) assert 'efi_start_image() returned: 26' in ''.join(output)
# Try rejection in reverse order."Reverse order" of what?
Of the test right above
Please specify the signature database, I guess "dbx"?
u_boot_console.restart_uboot()I don't think we need 'restart' here. I added it in each test function (not test case), IIRC, because we didn't have file-based non-volatile variables at that time.
You do. dbx already holds dbx_hash.auth and dbx1_hash.auth (in that order) at that point. The point is cleaning up dbx and testing against dbx1_hash.
Why not simply overwrite "dbx" variable? Without "-a", "env set -e" does it if it is properly signed with KEK.
I am not sure you've understood the bug yet. If I did that, db's 1sts entry would still be there. The whole point is insert dbx1_hash first. The easiest way to do this is on an empty database, instead of starting overwriting and cleaning variables. Why is rebooting even a problem?
with u_boot_console.log.section('Test Case 5e'):# Test Case 5e, authenticated even if only one of signatures# is verified. Same as before but reject dbx_hash1.auth onlyPlease specify what test case "before" means.
The test that run right before that
Please add a particular test case number to avoid any ambiguity. I believe that a test case description should be easy enough to understand and convey no ambiguity especially if there is some subtle difference between cases.
This is exactly the test case right above with dbx1_auth inserted first. I think it's fine under the current test.
output = u_boot_console.run_command_list(['host bind 0 %s' % disk_img,'fatload host 0:1 4000000 db.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize db','fatload host 0:1 4000000 KEK.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK','fatload host 0:1 4000000 PK.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize PK','fatload host 0:1 4000000 db1.auth','setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db','fatload host 0:1 4000000 dbx_hash1.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])Now "db" has db.auth and db1.auth in this order and 'dbx" has dbx_hash1.auth. Is this what you intend to test?
Yes. The patchset solved 2 bugs. One was not rejecting the image when a single dbx entry was found. The second was that depending on the order the image was signed and the keys inserted into dbx, the code could reject or accept the image.
Which part of "dbx" (or "db"?) is in a reverse order?
the first tests add dbx_hash -> dbx1_hash, while the second purges the dbx database and adds dbx1_hash to test against.
Regards /Ilias
-Takahiro Akashi
-Takahiro Akashi
assert 'Failed to set EFI variable' not in ''.join(output)output = u_boot_console.run_command_list(['efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""','efidebug boot next 1','efidebug test bootmgr'])assert '\'HELLO\' failed' in ''.join(output)assert 'efi_start_image() returned: 26' in ''.join(output)- def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env): """ Test Case 6 - using digest of signed image in database
-- 2.32.0
Regards /Ilias
On Mon, Feb 14, 2022 at 08:56:07AM +0200, Ilias Apalodimas wrote:
On Mon, Feb 14, 2022 at 03:36:06PM +0900, AKASHI Takahiro wrote:
On Mon, Feb 14, 2022 at 08:18:03AM +0200, Ilias Apalodimas wrote:
On Mon, Feb 14, 2022 at 10:50:08AM +0900, AKASHI Takahiro wrote:
Ilias,
On Fri, Feb 11, 2022 at 09:37:50AM +0200, Ilias Apalodimas wrote:
The previous patch is changing U-Boot's behavior wrt certificate based binary authentication. Specifically an image who's digest of a certificate is found in dbx is now rejected. Fix the test accordingly and add another one testing signatures in reverse order
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
changes since RFC:
- Added another test cases checking signature hashes in reverse order
test/py/tests/test_efi_secboot/test_signed.py | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py index 0aee34479f55..cc9396a11d48 100644 --- a/test/py/tests/test_efi_secboot/test_signed.py +++ b/test/py/tests/test_efi_secboot/test_signed.py @@ -186,7 +186,7 @@ class TestEfiSignedImage(object): assert 'Hello, world!' in ''.join(output)
with u_boot_console.log.section('Test Case 5c'):
# Test Case 5c, not rejected if one of signatures (digest of
# Test Case 5c, rejected if one of signatures (digest of # certificate) is revoked output = u_boot_console.run_command_list([ 'fatload host 0:1 4000000 dbx_hash.auth',@@ -195,7 +195,8 @@ class TestEfiSignedImage(object): output = u_boot_console.run_command_list([ 'efidebug boot next 1', 'efidebug test bootmgr'])
assert 'Hello, world!' in ''.join(output)
assert '\'HELLO\' failed' in ''.join(output)assert 'efi_start_image() returned: 26' in ''.join(output) with u_boot_console.log.section('Test Case 5d'): # Test Case 5d, rejected if both of signatures are revoked@@ -209,6 +210,31 @@ class TestEfiSignedImage(object): assert ''HELLO' failed' in ''.join(output) assert 'efi_start_image() returned: 26' in ''.join(output)
# Try rejection in reverse order."Reverse order" of what?
Of the test right above
Please specify the signature database, I guess "dbx"?
u_boot_console.restart_uboot()I don't think we need 'restart' here. I added it in each test function (not test case), IIRC, because we didn't have file-based non-volatile variables at that time.
You do. dbx already holds dbx_hash.auth and dbx1_hash.auth (in that order) at that point. The point is cleaning up dbx and testing against dbx1_hash.
Why not simply overwrite "dbx" variable? Without "-a", "env set -e" does it if it is properly signed with KEK.
I am not sure you've understood the bug yet. If I did that, db's 1sts entry would still be there. The whole point is insert dbx1_hash first.
I think that I understand your intension.
You meant "db's 1st entry" -> "dbx's 1st entry" in above sentence. Right?
# That is why, in my previous comment, I asked you to specify the test case number and the signature database's name explicitly in a comment to avoid any ambiguity.
When you said "in a reversed order" in your commit, I expected that either 1.the image(helloworld.efi) has two signatures in a reversed order, or (You hinted this possibility in our chat yesterday.) 2."db" has "db1.auth" and "db.auth" in this order, or 3."dbx" has "dbx_hash1.auth" and "dbx_hash.auth" in this order in this context, but your change didn't do neither.
You intended (3). Right?
The easiest way to do this is on an empty database, instead of starting overwriting and cleaning variables. Why is rebooting even a problem?
If "dbx" is a matter, the easiest way is to simply overwrite that variable. (Apparently we don't need any cleanup.)
with u_boot_console.log.section('Test Case 5e'):# Test Case 5e, authenticated even if only one of signatures# is verified. Same as before but reject dbx_hash1.auth onlyPlease specify what test case "before" means.
The test that run right before that
Please add a particular test case number to avoid any ambiguity. I believe that a test case description should be easy enough to understand and convey no ambiguity especially if there is some subtle difference between cases.
This is exactly the test case right above with dbx1_auth inserted first. I think it's fine under the current test.
See my comment above.
output = u_boot_console.run_command_list(['host bind 0 %s' % disk_img,'fatload host 0:1 4000000 db.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize db','fatload host 0:1 4000000 KEK.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK','fatload host 0:1 4000000 PK.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize PK','fatload host 0:1 4000000 db1.auth','setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db','fatload host 0:1 4000000 dbx_hash1.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])Now "db" has db.auth and db1.auth in this order and 'dbx" has dbx_hash1.auth. Is this what you intend to test?
Yes. The patchset solved 2 bugs. One was not rejecting the image when a single dbx entry was found. The second was that depending on the order the image was signed and the keys inserted into dbx, the code could reject or accept the image.
Which part of "dbx" (or "db"?) is in a reverse order?
the first tests add dbx_hash -> dbx1_hash, while the second purges the dbx database and adds dbx1_hash to test against.
See my comment above.
-Takahiro Akashi
Regards /Ilias
-Takahiro Akashi
-Takahiro Akashi
assert 'Failed to set EFI variable' not in ''.join(output)output = u_boot_console.run_command_list(['efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""','efidebug boot next 1','efidebug test bootmgr'])assert '\'HELLO\' failed' in ''.join(output)assert 'efi_start_image() returned: 26' in ''.join(output)- def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env): """ Test Case 6 - using digest of signed image in database
-- 2.32.0
Regards /Ilias
Akashi-san,
# Try rejection in reverse order."Reverse order" of what?
Of the test right above
Please specify the signature database, I guess "dbx"?
u_boot_console.restart_uboot()I don't think we need 'restart' here. I added it in each test function (not test case), IIRC, because we didn't have file-based non-volatile variables at that time.
You do. dbx already holds dbx_hash.auth and dbx1_hash.auth (in that order) at that point. The point is cleaning up dbx and testing against dbx1_hash.
Why not simply overwrite "dbx" variable? Without "-a", "env set -e" does it if it is properly signed with KEK.
I am not sure you've understood the bug yet. If I did that, db's 1sts entry would still be there. The whole point is insert dbx1_hash first.
I think that I understand your intension.
You meant "db's 1st entry" -> "dbx's 1st entry" in above sentence. Right?
Yes
# That is why, in my previous comment, I asked you to specify the test case number and the signature database's name explicitly in a comment to avoid any ambiguity.
Ok. I was planning on updating some more tests, so I'll try to spit that up there as well.
When you said "in a reversed order" in your commit, I expected that either 1.the image(helloworld.efi) has two signatures in a reversed order, or (You hinted this possibility in our chat yesterday.) 2."db" has "db1.auth" and "db.auth" in this order, or 3."dbx" has "dbx_hash1.auth" and "dbx_hash.auth" in this order in this context, but your change didn't do neither.
You intended (3). Right?
Yes, however inserting dbx_hash.auth right after dbx_hash1.auth didnt work for me. There's something date related which prevents us from adding both of the sha256 hashes of the certs in reverse order. However I think that inserting dbx_hash1.auth is enough for the test purpose. The whole point was to verify the change of the first patch, were a binary gets rejected on ony dbx match.
The easiest way to do this is on an empty database, instead of starting overwriting and cleaning variables. Why is rebooting even a problem?
If "dbx" is a matter, the easiest way is to simply overwrite that variable. (Apparently we don't need any cleanup.)
Ah sure, I can test that and send a patch along with some more test cases I got in mind.
with u_boot_console.log.section('Test Case 5e'):# Test Case 5e, authenticated even if only one of signatures# is verified. Same as before but reject dbx_hash1.auth onlyPlease specify what test case "before" means.
The test that run right before that
Please add a particular test case number to avoid any ambiguity. I believe that a test case description should be easy enough to understand and convey no ambiguity especially if there is some subtle difference between cases.
This is exactly the test case right above with dbx1_auth inserted first. I think it's fine under the current test.
See my comment above.
output = u_boot_console.run_command_list(['host bind 0 %s' % disk_img,'fatload host 0:1 4000000 db.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize db','fatload host 0:1 4000000 KEK.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK','fatload host 0:1 4000000 PK.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize PK','fatload host 0:1 4000000 db1.auth','setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db','fatload host 0:1 4000000 dbx_hash1.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])Now "db" has db.auth and db1.auth in this order and 'dbx" has dbx_hash1.auth. Is this what you intend to test?
Yes. The patchset solved 2 bugs. One was not rejecting the image when a single dbx entry was found. The second was that depending on the order the image was signed and the keys inserted into dbx, the code could reject or accept the image.
Which part of "dbx" (or "db"?) is in a reverse order?
the first tests add dbx_hash -> dbx1_hash, while the second purges the dbx database and adds dbx1_hash to test against.
See my comment above.
-Takahiro Akashi
Regards /Ilias
-Takahiro Akashi
-Takahiro Akashi
assert 'Failed to set EFI variable' not in ''.join(output)output = u_boot_console.run_command_list(['efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""','efidebug boot next 1','efidebug test bootmgr'])assert '\'HELLO\' failed' in ''.join(output)assert 'efi_start_image() returned: 26' in ''.join(output)- def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env): """ Test Case 6 - using digest of signed image in database
-- 2.32.0
Regards /Ilias
Ilias,
On Tue, Feb 15, 2022 at 08:50:08AM +0200, Ilias Apalodimas wrote:
Akashi-san,
> > + # Try rejection in reverse order.
"Reverse order" of what?
Of the test right above
Please specify the signature database, I guess "dbx"?
> + u_boot_console.restart_uboot()
I don't think we need 'restart' here. I added it in each test function (not test case), IIRC, because we didn't have file-based non-volatile variables at that time.
You do. dbx already holds dbx_hash.auth and dbx1_hash.auth (in that order) at that point. The point is cleaning up dbx and testing against dbx1_hash.
Why not simply overwrite "dbx" variable? Without "-a", "env set -e" does it if it is properly signed with KEK.
I am not sure you've understood the bug yet. If I did that, db's 1sts entry would still be there. The whole point is insert dbx1_hash first.
I think that I understand your intension.
You meant "db's 1st entry" -> "dbx's 1st entry" in above sentence. Right?
Yes
# That is why, in my previous comment, I asked you to specify the test case number and the signature database's name explicitly in a comment to avoid any ambiguity.
Ok. I was planning on updating some more tests, so I'll try to spit that up there as well.
When you said "in a reversed order" in your commit, I expected that either 1.the image(helloworld.efi) has two signatures in a reversed order, or (You hinted this possibility in our chat yesterday.) 2."db" has "db1.auth" and "db.auth" in this order, or 3."dbx" has "dbx_hash1.auth" and "dbx_hash.auth" in this order in this context, but your change didn't do neither.
You intended (3). Right?
Yes, however inserting dbx_hash.auth right after dbx_hash1.auth didnt work for me. There's something date related which prevents us from adding both of the sha256 hashes of the certs in reverse order.
I don't know why we can't do that.
However I think that inserting dbx_hash1.auth is enough for the test purpose. The whole point was to verify the change of the first patch, were a binary gets rejected on ony dbx match.
In your commit message for the first one, you said, "The rejection depends on the order that the image was signed and the order the certificates are read (and checked) in db."
In your new test case (5e), you mentioned "reverse order."
That kind of things confused me (and probably others as well) regarding what this test case is meant for. # Again, appropriate description about test cases is very much crucial # for reviewing test scenario.
The easiest way to do this is on an empty database, instead of starting overwriting and cleaning variables. Why is rebooting even a problem?
If "dbx" is a matter, the easiest way is to simply overwrite that variable. (Apparently we don't need any cleanup.)
Ah sure, I can test that and send a patch along with some more test cases I got in mind.
Anyhow, I'm looking forward for more test cases here :)
-Takahiro Akashi
> + with u_boot_console.log.section('Test Case 5e'): > + # Test Case 5e, authenticated even if only one of signatures > + # is verified. Same as before but reject dbx_hash1.auth only
Please specify what test case "before" means.
The test that run right before that
Please add a particular test case number to avoid any ambiguity. I believe that a test case description should be easy enough to understand and convey no ambiguity especially if there is some subtle difference between cases.
This is exactly the test case right above with dbx1_auth inserted first. I think it's fine under the current test.
See my comment above.
> + output = u_boot_console.run_command_list([ > + 'host bind 0 %s' % disk_img, > + 'fatload host 0:1 4000000 db.auth', > + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize db', > + 'fatload host 0:1 4000000 KEK.auth', > + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', > + 'fatload host 0:1 4000000 PK.auth', > + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', > + 'fatload host 0:1 4000000 db1.auth', > + 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > + 'fatload host 0:1 4000000 dbx_hash1.auth', > + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])
Now "db" has db.auth and db1.auth in this order and 'dbx" has dbx_hash1.auth. Is this what you intend to test?
Yes. The patchset solved 2 bugs. One was not rejecting the image when a single dbx entry was found. The second was that depending on the order the image was signed and the keys inserted into dbx, the code could reject or accept the image.
Which part of "dbx" (or "db"?) is in a reverse order?
the first tests add dbx_hash -> dbx1_hash, while the second purges the dbx database and adds dbx1_hash to test against.
See my comment above.
-Takahiro Akashi
Regards /Ilias
-Takahiro Akashi
-Takahiro Akashi
> + assert 'Failed to set EFI variable' not in ''.join(output) > + output = u_boot_console.run_command_list([ > + 'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""', > + 'efidebug boot next 1', > + 'efidebug test bootmgr']) > + assert ''HELLO' failed' in ''.join(output) > + assert 'efi_start_image() returned: 26' in ''.join(output) > + > def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env): > """ > Test Case 6 - using digest of signed image in database > -- > 2.32.0 >
Regards /Ilias
On Wed, 16 Feb 2022 at 04:18, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Ilias,
On Tue, Feb 15, 2022 at 08:50:08AM +0200, Ilias Apalodimas wrote:
Akashi-san,
> > > > + # Try rejection in reverse order. > > "Reverse order" of what?
Of the test right above
Please specify the signature database, I guess "dbx"?
> > > + u_boot_console.restart_uboot() > > I don't think we need 'restart' here. > I added it in each test function (not test case), IIRC, because we didn't > have file-based non-volatile variables at that time.
You do. dbx already holds dbx_hash.auth and dbx1_hash.auth (in that order) at that point. The point is cleaning up dbx and testing against dbx1_hash.
Why not simply overwrite "dbx" variable? Without "-a", "env set -e" does it if it is properly signed with KEK.
I am not sure you've understood the bug yet. If I did that, db's 1sts entry would still be there. The whole point is insert dbx1_hash first.
I think that I understand your intension.
You meant "db's 1st entry" -> "dbx's 1st entry" in above sentence. Right?
Yes
# That is why, in my previous comment, I asked you to specify the test case number and the signature database's name explicitly in a comment to avoid any ambiguity.
Ok. I was planning on updating some more tests, so I'll try to spit that up there as well.
When you said "in a reversed order" in your commit, I expected that either 1.the image(helloworld.efi) has two signatures in a reversed order, or (You hinted this possibility in our chat yesterday.) 2."db" has "db1.auth" and "db.auth" in this order, or 3."dbx" has "dbx_hash1.auth" and "dbx_hash.auth" in this order in this context, but your change didn't do neither.
You intended (3). Right?
Yes, however inserting dbx_hash.auth right after dbx_hash1.auth didnt work for me. There's something date related which prevents us from adding both of the sha256 hashes of the certs in reverse order.
I don't know why we can't do that.
There's a security vioilation reported if you try to insert dbx_hash after dbx_hash1, I assumed it's date related but didn't have time to check it. Adding dbx_hash1 alone is enough to test the order though.
However I think that inserting dbx_hash1.auth is enough for the test purpose. The whole point was to verify the change of the first patch, were a binary gets rejected on ony dbx match.
In your commit message for the first one, you said, "The rejection depends on the order that the image was signed and the order the certificates are read (and checked) in db."
In your new test case (5e), you mentioned "reverse order."
That kind of things confused me (and probably others as well) regarding what this test case is meant for. # Again, appropriate description about test cases is very much crucial # for reviewing test scenario.
You either have to revert the signing order of the binary or the sha256 hashes of certs that are inserted in dbx to test the order.
The easiest way to do this is on an empty database, instead of starting overwriting and cleaning variables. Why is rebooting even a problem?
If "dbx" is a matter, the easiest way is to simply overwrite that variable. (Apparently we don't need any cleanup.)
Ah sure, I can test that and send a patch along with some more test cases I got in mind.
Anyhow, I'm looking forward for more test cases here :)
-Takahiro Akashi
> > > + with u_boot_console.log.section('Test Case 5e'): > > + # Test Case 5e, authenticated even if only one of signatures > > + # is verified. Same as before but reject dbx_hash1.auth only > > Please specify what test case "before" means.
The test that run right before that
Please add a particular test case number to avoid any ambiguity. I believe that a test case description should be easy enough to understand and convey no ambiguity especially if there is some subtle difference between cases.
This is exactly the test case right above with dbx1_auth inserted first. I think it's fine under the current test.
See my comment above.
> > > + output = u_boot_console.run_command_list([ > > + 'host bind 0 %s' % disk_img, > > + 'fatload host 0:1 4000000 db.auth', > > + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize db', > > + 'fatload host 0:1 4000000 KEK.auth', > > + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', > > + 'fatload host 0:1 4000000 PK.auth', > > + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', > > + 'fatload host 0:1 4000000 db1.auth', > > + 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > > + 'fatload host 0:1 4000000 dbx_hash1.auth', > > + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) > > Now "db" has db.auth and db1.auth in this order and > 'dbx" has dbx_hash1.auth. > Is this what you intend to test?
Yes. The patchset solved 2 bugs. One was not rejecting the image when a single dbx entry was found. The second was that depending on the order the image was signed and the keys inserted into dbx, the code could reject or accept the image.
Which part of "dbx" (or "db"?) is in a reverse order?
the first tests add dbx_hash -> dbx1_hash, while the second purges the dbx database and adds dbx1_hash to test against.
See my comment above.
-Takahiro Akashi
Regards /Ilias
-Takahiro Akashi
> > -Takahiro Akashi > > > + assert 'Failed to set EFI variable' not in ''.join(output) > > + output = u_boot_console.run_command_list([ > > + 'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""', > > + 'efidebug boot next 1', > > + 'efidebug test bootmgr']) > > + assert ''HELLO' failed' in ''.join(output) > > + assert 'efi_start_image() returned: 26' in ''.join(output) > > + > > def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env): > > """ > > Test Case 6 - using digest of signed image in database > > -- > > 2.32.0 > >
Regards /Ilias
Akashi-san,
Here's an example of what was not working in case it helps you understand
[...]
When you said "in a reversed order" in your commit, I expected that either 1.the image(helloworld.efi) has two signatures in a reversed order, or (You hinted this possibility in our chat yesterday.) 2."db" has "db1.auth" and "db.auth" in this order, or 3."dbx" has "dbx_hash1.auth" and "dbx_hash.auth" in this order in this context, but your change didn't do neither.
You intended (3). Right?
Yes, however inserting dbx_hash.auth right after dbx_hash1.auth didnt work for me. There's something date related which prevents us from adding both of the sha256 hashes of the certs in reverse order.
I don't know why we can't do that.
There's a security vioilation reported if you try to insert dbx_hash after dbx_hash1, I assumed it's date related but didn't have time to check it. Adding dbx_hash1 alone is enough to test the order though.
However I think that inserting dbx_hash1.auth is enough for the test purpose. The whole point was to verify the change of the first patch, were a binary gets rejected on ony dbx match.
In your commit message for the first one, you said, "The rejection depends on the order that the image was signed and the order the certificates are read (and checked) in db."
In your new test case (5e), you mentioned "reverse order."
That kind of things confused me (and probably others as well) regarding what this test case is meant for. # Again, appropriate description about test cases is very much crucial # for reviewing test scenario.
You either have to revert the signing order of the binary or the sha256 hashes of certs that are inserted in dbx to test the order.
If you have an image signed with 2 certs
sbverify --list tmp/Image.signed.signed signature 1 image signature issuers: - /CN=apalos DB2 image signature certificates: - subject: /CN=apalos DB2 issuer: /CN=apalos DB2 signature 2 image signature issuers: - /CN=apalos DB image signature certificates: - subject: /CN=apalos DB issuer: /CN=apalos DB
and add both of the certificates in DB and and 'apalos DB2' in dbx, without this patch the image is allowed to run. However if you add 'apalos DB' the image gets rejected. What this patch does is reject the image properly if any of the certificates match.
Regards /Ilias
The easiest way to do this is on an empty database, instead of starting overwriting and cleaning variables. Why is rebooting even a problem?
If "dbx" is a matter, the easiest way is to simply overwrite that variable. (Apparently we don't need any cleanup.)
Ah sure, I can test that and send a patch along with some more test cases I got in mind.
Anyhow, I'm looking forward for more test cases here :)
-Takahiro Akashi
> > > > > + with u_boot_console.log.section('Test Case 5e'): > > > + # Test Case 5e, authenticated even if only one of signatures > > > + # is verified. Same as before but reject dbx_hash1.auth only > > > > Please specify what test case "before" means. > > The test that run right before that
Please add a particular test case number to avoid any ambiguity. I believe that a test case description should be easy enough to understand and convey no ambiguity especially if there is some subtle difference between cases.
This is exactly the test case right above with dbx1_auth inserted first. I think it's fine under the current test.
See my comment above.
> > > > > + output = u_boot_console.run_command_list([ > > > + 'host bind 0 %s' % disk_img, > > > + 'fatload host 0:1 4000000 db.auth', > > > + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize db', > > > + 'fatload host 0:1 4000000 KEK.auth', > > > + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK', > > > + 'fatload host 0:1 4000000 PK.auth', > > > + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK', > > > + 'fatload host 0:1 4000000 db1.auth', > > > + 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db', > > > + 'fatload host 0:1 4000000 dbx_hash1.auth', > > > + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx']) > > > > Now "db" has db.auth and db1.auth in this order and > > 'dbx" has dbx_hash1.auth. > > Is this what you intend to test? > > Yes. The patchset solved 2 bugs. One was not rejecting the image when a > single dbx entry was found. The second was that depending on the order the > image was signed and the keys inserted into dbx, the code could reject or > accept the image.
Which part of "dbx" (or "db"?) is in a reverse order?
the first tests add dbx_hash -> dbx1_hash, while the second purges the dbx database and adds dbx1_hash to test against.
See my comment above.
-Takahiro Akashi
Regards /Ilias
-Takahiro Akashi
> > > > -Takahiro Akashi > > > > > + assert 'Failed to set EFI variable' not in ''.join(output) > > > + output = u_boot_console.run_command_list([ > > > + 'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""', > > > + 'efidebug boot next 1', > > > + 'efidebug test bootmgr']) > > > + assert ''HELLO' failed' in ''.join(output) > > > + assert 'efi_start_image() returned: 26' in ''.join(output) > > > + > > > def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env): > > > """ > > > Test Case 6 - using digest of signed image in database > > > -- > > > 2.32.0 > > > > > Regards > /Ilias
Heinrich,
We (I and Ilias) are still discussing on this patch. I don't think it appropriate to merge it (in -rc2) at this point.
-Takahiro Akashi
On Fri, Feb 11, 2022 at 09:37:50AM +0200, Ilias Apalodimas wrote:
The previous patch is changing U-Boot's behavior wrt certificate based binary authentication. Specifically an image who's digest of a certificate is found in dbx is now rejected. Fix the test accordingly and add another one testing signatures in reverse order
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
changes since RFC:
- Added another test cases checking signature hashes in reverse order
test/py/tests/test_efi_secboot/test_signed.py | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py index 0aee34479f55..cc9396a11d48 100644 --- a/test/py/tests/test_efi_secboot/test_signed.py +++ b/test/py/tests/test_efi_secboot/test_signed.py @@ -186,7 +186,7 @@ class TestEfiSignedImage(object): assert 'Hello, world!' in ''.join(output)
with u_boot_console.log.section('Test Case 5c'):
# Test Case 5c, not rejected if one of signatures (digest of
# Test Case 5c, rejected if one of signatures (digest of # certificate) is revoked output = u_boot_console.run_command_list([ 'fatload host 0:1 4000000 dbx_hash.auth',@@ -195,7 +195,8 @@ class TestEfiSignedImage(object): output = u_boot_console.run_command_list([ 'efidebug boot next 1', 'efidebug test bootmgr'])
assert 'Hello, world!' in ''.join(output)
assert '\'HELLO\' failed' in ''.join(output)assert 'efi_start_image() returned: 26' in ''.join(output) with u_boot_console.log.section('Test Case 5d'): # Test Case 5d, rejected if both of signatures are revoked@@ -209,6 +210,31 @@ class TestEfiSignedImage(object): assert ''HELLO' failed' in ''.join(output) assert 'efi_start_image() returned: 26' in ''.join(output)
# Try rejection in reverse order.u_boot_console.restart_uboot()with u_boot_console.log.section('Test Case 5e'):# Test Case 5e, authenticated even if only one of signatures# is verified. Same as before but reject dbx_hash1.auth onlyoutput = u_boot_console.run_command_list(['host bind 0 %s' % disk_img,'fatload host 0:1 4000000 db.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize db','fatload host 0:1 4000000 KEK.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK','fatload host 0:1 4000000 PK.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize PK','fatload host 0:1 4000000 db1.auth','setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db','fatload host 0:1 4000000 dbx_hash1.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 -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""','efidebug boot next 1','efidebug test bootmgr'])assert '\'HELLO\' failed' in ''.join(output)assert 'efi_start_image() returned: 26' in ''.join(output)- def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env): """ Test Case 6 - using digest of signed image in database
-- 2.32.0
On Wed, 16 Feb 2022 at 04:20, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
Heinrich,
We (I and Ilias) are still discussing on this patch. I don't think it appropriate to merge it (in -rc2) at this point.
I think it's fine to keep this as is since it's actually testing for a case that the patch fixed. I'll send a patch updating the description though.
Cheers /Ilias
-Takahiro Akashi
On Fri, Feb 11, 2022 at 09:37:50AM +0200, Ilias Apalodimas wrote:
The previous patch is changing U-Boot's behavior wrt certificate based binary authentication. Specifically an image who's digest of a certificate is found in dbx is now rejected. Fix the test accordingly and add another one testing signatures in reverse order
Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org
changes since RFC:
- Added another test cases checking signature hashes in reverse order
test/py/tests/test_efi_secboot/test_signed.py | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py index 0aee34479f55..cc9396a11d48 100644 --- a/test/py/tests/test_efi_secboot/test_signed.py +++ b/test/py/tests/test_efi_secboot/test_signed.py @@ -186,7 +186,7 @@ class TestEfiSignedImage(object): assert 'Hello, world!' in ''.join(output)
with u_boot_console.log.section('Test Case 5c'):
# Test Case 5c, not rejected if one of signatures (digest of
# Test Case 5c, rejected if one of signatures (digest of # certificate) is revoked output = u_boot_console.run_command_list([ 'fatload host 0:1 4000000 dbx_hash.auth',@@ -195,7 +195,8 @@ class TestEfiSignedImage(object): output = u_boot_console.run_command_list([ 'efidebug boot next 1', 'efidebug test bootmgr'])
assert 'Hello, world!' in ''.join(output)
assert '\'HELLO\' failed' in ''.join(output)assert 'efi_start_image() returned: 26' in ''.join(output) with u_boot_console.log.section('Test Case 5d'): # Test Case 5d, rejected if both of signatures are revoked@@ -209,6 +210,31 @@ class TestEfiSignedImage(object): assert ''HELLO' failed' in ''.join(output) assert 'efi_start_image() returned: 26' in ''.join(output)
# Try rejection in reverse order.u_boot_console.restart_uboot()with u_boot_console.log.section('Test Case 5e'):# Test Case 5e, authenticated even if only one of signatures# is verified. Same as before but reject dbx_hash1.auth onlyoutput = u_boot_console.run_command_list(['host bind 0 %s' % disk_img,'fatload host 0:1 4000000 db.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize db','fatload host 0:1 4000000 KEK.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK','fatload host 0:1 4000000 PK.auth','setenv -e -nv -bs -rt -at -i 4000000:$filesize PK','fatload host 0:1 4000000 db1.auth','setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db','fatload host 0:1 4000000 dbx_hash1.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 -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""','efidebug boot next 1','efidebug test bootmgr'])assert '\'HELLO\' failed' in ''.join(output)assert 'efi_start_image() returned: 26' in ''.join(output)- def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env): """ Test Case 6 - using digest of signed image in database
-- 2.32.0
participants (2)
-
AKASHI Takahiro -
Ilias Apalodimas