Re: [PATCH v4 2/4] load_env support for whitelisting which variables are read from an env file, even if check_signatures=enforce

[Jonathan McCune <jonmccune@google.com>]

[-- Attachment #1: Type: text/plain, Size: 3199 bytes --]

On Tue, Sep 24, 2013 at 11:09 PM, Andrey Borzenkov <arvidjaar@gmail.com>wrote:

> В Tue, 24 Sep 2013 19:00:30 -0700
> Jon McCune <jonmccune@google.com> пишет:
>
> > This version of the patch implements whitelisting in the envblk
> subsystem,
> > instead of in loadenv.c.  It seems to be cleaner than the previous
> patches.
> >
> > This works by adding an open_envblk_file_untrusted() method that bypasses
> > signature checking, but only if the invocation of load_env includes a
> > whitelist of one or more environment variables that are to be read from
> the
> > file.
>
> I do not really like such silent assumptions. Being able to load only
> selected environment variables is useful by itself, but I still may
> want to ensure file was not tampered with.
>
> I suggest you simply add flag "--skip-signature-check" to load_env and
> add support for explicit variable list. Then it is up to user how and
> when to use it.
>

This is okay with me.  Other opinions?


> And please update also documentation for command changes.
>

Do you mean grub.texi as well, or just within the source code?  (e.g., for
'help load_env')


>
> > +static grub_file_t
> > +open_envblk_file_untrusted (char *filename)
>
> Why do you need extra function? Is not flag to open_envblk_file enough?
>

I wanted to keep the security-relevant disable / re-enable of the PUBKEY
filter as close together as possible.  open_envblk_file() does not
currently restore any filters that it disabled (the disable functions
operate globally, and not per-file), so it would complicate that function
somewhat to guarantee that PUBKEY was restored (as there are multiple valid
exit points from that function).  I will change this to use a flag instead
(and possibly restructure the function somewhat) if others agree.


>
> > +{
> > +  grub_file_filter_t curfilt[GRUB_FILE_FILTER_MAX];
> > +  grub_file_t file;
> > +
> > +  /* Temporarily disable all filters so as to read the untrusted file */
> > +  grub_memcpy (curfilt, grub_file_filters_enabled, sizeof (curfilt));
> > +  grub_file_filter_disable_all ();
>
> Why do you need to disable *all* filters? Assuming disabling
> compression was good enough, you just need to disable signature
> checking, right?
>

That is all the filters at the present time... PUBKEY and then various
compression algorithms.  I'm not sure what filters might be introduced in
the future, so I'm not sure whether it's safer to disable / enable all, or
disable / enable the full list of filters that exist today.  I thought the
more conservative option was to do them all.  I will change this to the
list of currently existing filters instead if others agree.


>
> >  void
> >  grub_envblk_iterate (grub_envblk_t envblk,
> > +                     const grub_env_whitelist_t* whitelist,
> >                       int hook (const char *name, const char *value))
>
> Again, that's really too restrictive. Like with any other iterators,
> I'd make hook accept extra pointer for hook-specific data and pass this
> data to grub_envblk_iterate. This will let caller decide the policy.
>

This sounds good to me.

-Jon

[-- Attachment #2: Type: text/html, Size: 4475 bytes --]