| Message ID | 20220211073750.733348-2-ilias.apalodimas@linaro.org |
|---|---|
| State | Accepted |
| Commit | 72b509b7019878e2a5f69bcf7198a0927a77ad60 |
| Headers | show |
| Series | [1/2] efi_loader: fix dual signed image certification | expand |
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 only > > Please 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 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. > > > > > + 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 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. > > > > > > > > + 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 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, > > > > > > > > > > > > + # 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 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
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
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 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 > -- > 2.32.0 >
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
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 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 > > -- > > 2.32.0 > >
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
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
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(-)