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

[AKASHI Takahiro]

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