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

[Andrey Borzenkov]

В 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. 

And please update also documentation for command changes.

> +static grub_file_t
> +open_envblk_file_untrusted (char *filename)

Why do you need extra function? Is not flag to open_envblk_file enough?

> +{
> +  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?

>  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.