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]

On Tue, Sep 24, 2013 at 11:09 PM, Andrey Borzenkov <address@hidden> wrote:

В Tue, 24 Sep 2013 19:00:30 -0700
Jon McCune <address@hidden> пишет:

> 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