[PATCH 2/2] test/py: efi_secboot: adjust secure boot tests to code changes

[Ilias Apalodimas]

On Wed, 16 Feb 2022 at 04:18, AKASHI Takahiro
<takahiro.akashi at 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