Re: test command and signature checks

[Jonathan McCune]

On Thu, Jan 29, 2015 at 7:19 AM, Andrei Borzenkov <address@hidden> wrote:

В Thu, 29 Jan 2015 06:17:42 -0800
Jonathan McCune <address@hidden> пишет:

> On Jan 29, 2015 1:19 AM, "Andrei Borzenkov" <address@hidden> wrote:
> >
> > What sematic of file tests should be? I think they should just test
> > file existence; this already happens for compressed files that checks
> > that on-disk file size, not uncompressed. I think same should apply to
> > signature checks.
> >
>
> Where the alternative is that an existence check will only succeed if a
> file has a corresponding (and verifiable) .sig?
>
> I think existence-only is the right semantics because verify_detached can
> be used to achieve the signature-check in a standalone fashion.
>
> (I.e., the existing behavior of test and verify_detached seems correct to
> me.)
>

Existing behavior is to simply open file so any filter in effect will
be applied.

I think it's more subtle than that. I think it depends on which arguments are provided to test. As I understand it filters are applied in grub_file_open() (http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/kern/file.c), and I think I can use -f to test the existence of an unsigned file while check_signatures=enforce, without causing signature verification to fail.

Let's consider the case of a grub.cfg using 'test -f', checking for file existence. From http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/commands/test.c:

if (grub_strcmp (args[*argn], "-f") == 0)

   {

     get_fileinfo (args[*argn + 1], &ctx);

     /* FIXME: check for other types. */

     update_val (ctx.file_exists && ! ctx.file_info.dir, &ctx);

     (*argn) += 2;

     return ctx.or || ctx.and;

   }

In get_fileinfo() (also test.c) I don't think grub_file_open() is called. The final else branch calls through a filesystem-specific function pointer that invokes a callback with entries in some directory ("/* Call HOOK with each file under DIR.  */" from http://git.savannah.gnu.org/cgit/grub.git/tree/include/grub/fs.h):

(fs->dir) (dev, path, find_file, ctx);

, which leads to a call to the find_file() function (also in test.c) for each entry in the directory. The files do *not* seem to be opened.

It looks like the handler for the "-s" ("file exists and has a size greater than zero") *does* cause the file to get opened (i.e., calls grub_file_open(), causing filters to get applied), but I think the basic existence check ("-f") *not* leading to a call to grub_file_open() (and hence applying all the filters / enforcing mandatory signature checks if check_signatures=enforce) is the right behavior.

I think this behavior is reasonable, because the attack surface for testing file existence is the filesystem parsing code, as opposed to the [potentially unsigned, and populated with some kind of evil] file whose existence is being checked.

Please do let me know if I've misunderstood the code somehow.

-Jon

 

> > May be file checks should simply disable all filters unconditionally
> > to become more lightweight.
> >
> > _______________________________________________
> > Grub-devel mailing list
> > address@hidden
> > https://lists.gnu.org/mailman/listinfo/grub-devel