Re: [PATCH v6] load_env support for whitelisting, save_env support for check_signatures

[Vladimir 'φ-coder/phcoder' Serbinenko]

Committed, thanks.
On 27.09.2013 01:54, Jon McCune wrote:
> Whitelisting which variables are read from an env file prevents a
> malicious environment block file from overwriting the value of
> security-critical environment variables such as check_signatures,
> while still allowing a properly constructed configuration file to
> offer "savedefault" and "one-shot" functionality.
> 
> Tested with 'make check' to the best of my ability. Failures (both
> before and after the changes proposed in this patch set, i.e.,
> unchanged by this patch set):
> gettext_strings_test, fddboot_test, grub_func_test
> 
> Changes in v6 of the patch:
> 
>   filter disable_...() semantics are such that save/restore is
>   not necessary. grub_file_open() will re-enable all disabled filters
>   right after opening the target file.
> 
> Changes in v5 of the patch:
> 
>   Drop whitespace changes.
> 
>   Drop grub-install script changes.
> 
>   Generalized hook support in grub_envblk_iterate().
>   Whitelist-specific hook data structures are self-contained in
>   loadenv.c.
> 
>   Add flag {-s, --skip-sig} to load_env command that explicitly
>   controls whether signature-checking is required.  Whitelisting
>   and signature checking are thus controlled independently, and
>   the user may now choose to load only whitelisted variables from
>   either of a signed or unsigned grubenv-style file.
> 
>   Add untrusted flag to open_envblk_file().  Replaces the v4
>   patch's function open_envblk_file_untrusted() with a flag
>   passed to the existing open_envblk_file().  Also restructured
>   open_envblk_file() to make it more readable with the additional
>   logic.
> 
>   open_envblk_file(), when untrusted == 1, disables filters using
>   the compression- and pubkey-specific methods in file.h. The
>   pubkey filter is saved and restored across untrusted file opens.
> 
>   save_env always calls grub_envblk_file() with untrusted set to 1.
>   The contents that are read from the file are discarded, as the
>   only purpose of reading the file is to construct the blocklist to
>   which the new environment block will be written.  Thus, the
>   actual contents of the file are not parsed and do not pose a
>   security risk.
> 
> Signed-off-by: Jon McCune <address@hidden>
> ---
>  grub-core/commands/loadenv.c | 108 
> ++++++++++++++++++++++++++++++-------------
>  grub-core/lib/envblk.c       |   5 +-
>  include/grub/lib/envblk.h    |   3 +-
>  util/grub-editenv.c          |   5 +-
>  4 files changed, 84 insertions(+), 37 deletions(-)
> 
> diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.c
> index c0a42c5..0062c77 100644
> --- a/grub-core/commands/loadenv.c
> +++ b/grub-core/commands/loadenv.c
> @@ -35,45 +35,54 @@ static const struct grub_arg_option options[] =
>      /* TRANSLATORS: This option is used to override default filename
>         for loading and storing environment.  */
>      {"file", 'f', 0, N_("Specify filename."), 0, ARG_TYPE_PATHNAME},
> +    {"skip-sig", 's', 0,
> +     N_("Skip signature-checking of the environment file."), 0, 
> ARG_TYPE_NONE},
>      {0, 0, 0, 0, 0, 0}
>    };
>  
> +/* Opens 'filename' with compression filters disabled. Optionally disables 
> the
> +   PUBKEY filter (that insists upon properly signed files) as well.  PUBKEY
> +   filter is restored before the function returns. */
>  static grub_file_t
> -open_envblk_file (char *filename)
> +open_envblk_file (char *filename, int untrusted)
>  {
>    grub_file_t file;
> +  char *buf = 0;
>  
>    if (! filename)
>      {
>        const char *prefix;
> +      int len;
>  
>        prefix = grub_env_get ("prefix");
> -      if (prefix)
> -        {
> -          int len;
> -
> -          len = grub_strlen (prefix);
> -          filename = grub_malloc (len + 1 + sizeof (GRUB_ENVBLK_DEFCFG));
> -          if (! filename)
> -            return 0;
> -
> -          grub_strcpy (filename, prefix);
> -          filename[len] = '/';
> -          grub_strcpy (filename + len + 1, GRUB_ENVBLK_DEFCFG);
> -       grub_file_filter_disable_compression ();
> -          file = grub_file_open (filename);
> -          grub_free (filename);
> -          return file;
> -        }
> -      else
> +      if (! prefix)
>          {
>            grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("variable `%s' isn't 
> set"), "prefix");
>            return 0;
>          }
> +
> +      len = grub_strlen (prefix);
> +      buf = grub_malloc (len + 1 + sizeof (GRUB_ENVBLK_DEFCFG));
> +      if (! buf)
> +        return 0;
> +      filename = buf;
> +
> +      grub_strcpy (filename, prefix);
> +      filename[len] = '/';
> +      grub_strcpy (filename + len + 1, GRUB_ENVBLK_DEFCFG);
>      }
>  
> +  /* The filters that are disabled will be re-enabled by the call to
> +     grub_file_open() after this particular file is opened. */
>    grub_file_filter_disable_compression ();
> -  return grub_file_open (filename);
> +  if (untrusted)
> +    grub_file_filter_disable_pubkey ();
> +
> +  file = grub_file_open (filename);
> +
> +  if (buf)
> +    grub_free (buf);
> +  return file;
>  }
>  
>  static grub_envblk_t
> @@ -114,24 +123,55 @@ read_envblk_file (grub_file_t file)
>    return envblk;
>  }
>  
> +struct grub_env_whitelist
> +{
> +  grub_size_t len;
> +  char **list;
> +};
> +typedef struct grub_env_whitelist grub_env_whitelist_t;
> +
> +static int
> +test_whitelist_membership (const char* name,
> +                           const grub_env_whitelist_t* whitelist)
> +{
> +  grub_size_t i;
> +
> +  for (i = 0; i < whitelist->len; i++)
> +    if (grub_strcmp (name, whitelist->list[i]) == 0)
> +      return 1;  /* found it */
> +
> +  return 0;  /* not found */
> +}
> +
>  /* Helper for grub_cmd_load_env.  */
>  static int
> -set_var (const char *name, const char *value)
> +set_var (const char *name, const char *value, void *whitelist)
>  {
> -  grub_env_set (name, value);
> +  if (! whitelist)
> +    {
> +      grub_env_set (name, value);
> +      return 0;
> +    }
> +
> +  if (test_whitelist_membership (name, (const 
> grub_env_whitelist_t*)whitelist))
> +    grub_env_set (name, value);
> +
>    return 0;
>  }
>  
>  static grub_err_t
> -grub_cmd_load_env (grub_extcmd_context_t ctxt,
> -                int argc __attribute__ ((unused)),
> -                char **args __attribute__ ((unused)))
> +grub_cmd_load_env (grub_extcmd_context_t ctxt, int argc, char **args)
>  {
>    struct grub_arg_list *state = ctxt->state;
>    grub_file_t file;
>    grub_envblk_t envblk;
> +  grub_env_whitelist_t whitelist;
> +
> +  whitelist.len = argc;
> +  whitelist.list = args;
>  
> -  file = open_envblk_file ((state[0].set) ? state[0].arg : 0);
> +  /* state[0] is the -f flag; state[1] is the --skip-sig flag */
> +  file = open_envblk_file ((state[0].set) ? state[0].arg : 0, state[1].set);
>    if (! file)
>      return grub_errno;
>  
> @@ -139,7 +179,8 @@ grub_cmd_load_env (grub_extcmd_context_t ctxt,
>    if (! envblk)
>      goto fail;
>  
> -  grub_envblk_iterate (envblk, set_var);
> +  /* argc > 0 indicates caller provided a whitelist of variables to read. */
> +  grub_envblk_iterate (envblk, argc > 0 ? &whitelist : 0, set_var);
>    grub_envblk_close (envblk);
>  
>   fail:
> @@ -149,7 +190,8 @@ grub_cmd_load_env (grub_extcmd_context_t ctxt,
>  
>  /* Print all variables in current context.  */
>  static int
> -print_var (const char *name, const char *value)
> +print_var (const char *name, const char *value,
> +           void *hook_data __attribute__ ((unused)))
>  {
>    grub_printf ("%s=%s\n", name, value);
>    return 0;
> @@ -164,7 +206,7 @@ grub_cmd_list_env (grub_extcmd_context_t ctxt,
>    grub_file_t file;
>    grub_envblk_t envblk;
>  
> -  file = open_envblk_file ((state[0].set) ? state[0].arg : 0);
> +  file = open_envblk_file ((state[0].set) ? state[0].arg : 0, 0);
>    if (! file)
>      return grub_errno;
>  
> @@ -172,7 +214,7 @@ grub_cmd_list_env (grub_extcmd_context_t ctxt,
>    if (! envblk)
>      goto fail;
>  
> -  grub_envblk_iterate (envblk, print_var);
> +  grub_envblk_iterate (envblk, NULL, print_var);
>    grub_envblk_close (envblk);
>  
>   fail:
> @@ -333,7 +375,8 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, 
> char **args)
>    if (! argc)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "no variable is specified");
>  
> -  file = open_envblk_file ((state[0].set) ? state[0].arg : 0);
> +  file = open_envblk_file ((state[0].set) ? state[0].arg : 0,
> +                           1 /* allow untrusted */);
>    if (! file)
>      return grub_errno;
>  
> @@ -386,7 +429,8 @@ static grub_extcmd_t cmd_load, cmd_list, cmd_save;
>  GRUB_MOD_INIT(loadenv)
>  {
>    cmd_load =
> -    grub_register_extcmd ("load_env", grub_cmd_load_env, 0, N_("[-f FILE]"),
> +    grub_register_extcmd ("load_env", grub_cmd_load_env, 0,
> +                       N_("[-f FILE] [-s|--skip-sig] 
> [whitelisted_variable_name] [...]"),
>                         N_("Load variables from environment block file."),
>                         options);
>    cmd_list =
> diff --git a/grub-core/lib/envblk.c b/grub-core/lib/envblk.c
> index 311927b..230e0e9 100644
> --- a/grub-core/lib/envblk.c
> +++ b/grub-core/lib/envblk.c
> @@ -225,7 +225,8 @@ grub_envblk_delete (grub_envblk_t envblk, const char 
> *name)
>  
>  void
>  grub_envblk_iterate (grub_envblk_t envblk,
> -                     int hook (const char *name, const char *value))
> +                     void *hook_data,
> +                     int hook (const char *name, const char *value, void 
> *hook_data))
>  {
>    char *p, *pend;
>  
> @@ -285,7 +286,7 @@ grub_envblk_iterate (grub_envblk_t envblk,
>              }
>            *q = '\0';
>  
> -          ret = hook (name, value);
> +          ret = hook (name, value, hook_data);
>            grub_free (name);
>            if (ret)
>              return;
> diff --git a/include/grub/lib/envblk.h b/include/grub/lib/envblk.h
> index 368ba53..c3e6559 100644
> --- a/include/grub/lib/envblk.h
> +++ b/include/grub/lib/envblk.h
> @@ -35,7 +35,8 @@ grub_envblk_t grub_envblk_open (char *buf, grub_size_t 
> size);
>  int grub_envblk_set (grub_envblk_t envblk, const char *name, const char 
> *value);
>  void grub_envblk_delete (grub_envblk_t envblk, const char *name);
>  void grub_envblk_iterate (grub_envblk_t envblk,
> -                          int hook (const char *name, const char *value));
> +                          void *hook_data,
> +                          int hook (const char *name, const char *value, 
> void *hook_data));
>  void grub_envblk_close (grub_envblk_t envblk);
>  
>  static inline char *
> diff --git a/util/grub-editenv.c b/util/grub-editenv.c
> index 9b51acf..591604b 100644
> --- a/util/grub-editenv.c
> +++ b/util/grub-editenv.c
> @@ -185,7 +185,8 @@ open_envblk_file (const char *name)
>  }
>  
>  static int
> -print_var (const char *varname, const char *value)
> +print_var (const char *varname, const char *value,
> +           void *hook_data __attribute__ ((unused)))
>  {
>    printf ("%s=%s\n", varname, value);
>    return 0;
> @@ -197,7 +198,7 @@ list_variables (const char *name)
>    grub_envblk_t envblk;
>  
>    envblk = open_envblk_file (name);
> -  grub_envblk_iterate (envblk, print_var);
> +  grub_envblk_iterate (envblk, NULL, print_var);
>    grub_envblk_close (envblk);
>  }
>  
>

signature.asc
Description: OpenPGP digital signature