[PATCH v2 2/5] load_env support for whitelisting which variables are read from an env file, even if check_signatures=enforce

[Jon McCune]

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. Only variables in this whitelist will actually be modified based on
the contents of the file.  This 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.

Signed-off-by: Jon McCune <address@hidden>
---
 grub-core/commands/loadenv.c | 86 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 80 insertions(+), 6 deletions(-)

diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.c
index a431499..49e8004 100644
--- a/grub-core/commands/loadenv.c
+++ b/grub-core/commands/loadenv.c
@@ -76,6 +76,26 @@ open_envblk_file (char *filename)
   return grub_file_open (filename);
 }
 
+/* Opens 'filename' with all filters disabled (in particular, the PUBKEY filter
+   that insists upon properly signed files, and any kind of compression, since
+   we don't want untrusted input being processed by complex compression
+   algorithms). */
+static grub_file_t
+open_envblk_file_untrusted (char *filename)
+{
+  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 ();
+  file = open_envblk_file (filename);
+  /* Restore filters */
+  grub_memcpy (grub_file_filters_enabled, curfilt, sizeof (curfilt));
+
+  return file;
+}
+
 static grub_envblk_t
 read_envblk_file (grub_file_t file)
 {
@@ -122,16 +142,67 @@ set_var (const char *name, const char *value)
   return 0;
 }
 
+#define MAX_VAR_NAME 1024
+/* TODO: eliminate the need for these to be global */
+static grub_size_t whitelist_len = 0;
+static char **whitelist_vars = NULL;
+static int
+/* Assumptions: name, value, and each element of whitelist_vars[] are non-NULL
+   and NULL-terminated. */
+set_var_whitelist (const char *name, const char *value)
+{
+  grub_size_t i;
+  if (whitelist_len < 1 || whitelist_vars == NULL)
+    return GRUB_ERR_OUT_OF_RANGE;
+
+  /* search for 'name' in the whitelist */
+  for (i = 0; i < whitelist_len; i++)
+    {
+      if (0 == grub_strncmp (name, whitelist_vars[i], MAX_VAR_NAME))
+        {
+          /* TODO: also validate the characters in the variables */
+          grub_dprintf ("crypt", "set_var_whitelist APPOVED: %s=%s\n",
+                        name, value);
+          grub_env_set (name, value);
+          return 0;
+        }
+    }
+
+  grub_dprintf ("crypt",
+                "set_var_whitelist REFUSING TO SET name='%s' value='%s'\n",
+                name, value);
+  /* Still considered success */
+  return 0;
+}
+
+/* Load environment variables from a file.  Accepts a flag which can override
+   the file from which variables are read.  If additional parameters are passed
+   (argc > 0), then those are interpreted as a whitelist of environment
+   variables whose values can be changed based on the contents of the file, and
+   the file is never signature-checked.
+   Otherwise, all variables from the file are processed, and the file must be
+   properly signed when check_signatures=enforce. */
 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;
 
-  file = open_envblk_file ((state[0].set) ? state[0].arg : 0);
+  /* argc > 0 indicates caller provided a whitelist of variables to read */
+  if (argc > 0)
+    {
+      whitelist_len = (argc >= 0) ? argc : 0;
+      whitelist_vars = args;
+      /* Open the envblk file even if check_signatures=enforce and the file is
+         not properly signed, since we will only update environment variables
+         whose names are present in the whitelist. */
+      file = open_envblk_file_untrusted ((state[0].set) ? state[0].arg : 0);
+    }
+  else
+    {
+      file = open_envblk_file ((state[0].set) ? state[0].arg : 0);
+    }
   if (!file)
     return grub_errno;
 
@@ -139,10 +210,12 @@ grub_cmd_load_env (grub_extcmd_context_t ctxt,
   if (!envblk)
     goto fail;
 
-  grub_envblk_iterate (envblk, set_var);
+  grub_envblk_iterate (envblk, argc > 0 ? set_var_whitelist : set_var);
   grub_envblk_close (envblk);
 
 fail:
+  whitelist_len = 0;
+  whitelist_vars = NULL;
   grub_file_close (file);
   return grub_errno;
 }
@@ -387,7 +460,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] [whitelisted_variable_name] [...]"),
                           N_("Load variables from environment block file."),
                           options);
   cmd_list =
-- 
1.8.4