В Tue, 24 Sep 2013 19:00:30 -0700
Jon McCune <jonmcc...@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.
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.
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
