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 <jonmcc...@google.com>
> ---
> 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
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
