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.apalodi...@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
