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

[Jon McCune]

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);
 }
 
-- 
1.8.4