grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[PATCH v4 2/4] load_env support for whitelisting which variables are rea


From: Jon McCune
Subject: [PATCH v4 2/4] load_env support for whitelisting which variables are read from an env file, even if check_signatures=enforce
Date: Tue, 24 Sep 2013 19:00:30 -0700

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. 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.

If no whitelist is provided, the behavior is the same as today: the entire
input file must be signed when check_signatures=enforce.

Signed-off-by: Jon McCune <address@hidden>
---
 grub-core/commands/loadenv.c | 54 ++++++++++++++++++++++++++++++++++++++------
 grub-core/lib/envblk.c       | 20 ++++++++++++++--
 include/grub/lib/envblk.h    |  8 +++++++
 util/grub-editenv.c          |  2 +-
 4 files changed, 74 insertions(+), 10 deletions(-)

diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.c
index a431499..16aafbf 100644
--- a/grub-core/commands/loadenv.c
+++ b/grub-core/commands/loadenv.c
@@ -76,6 +76,28 @@ 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 ();
+  /* TODO: Is there any possible code path due to open_envblk_file that could
+   * cause untrusted modules to be loaded? */
+  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 +144,33 @@ set_var (const char *name, const char *value)
   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;
+  grub_env_whitelist_t whitelist;
+
+  whitelist.len = argc;
+  whitelist.list = args;
+
+  /* argc > 0 indicates caller provided a whitelist of variables to read.
+     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. */
+  if (argc > 0)
+      file = open_envblk_file_untrusted ((state[0].set) ? state[0].arg : 0);
+  else
+      file = open_envblk_file ((state[0].set) ? state[0].arg : 0);
 
-  file = open_envblk_file ((state[0].set) ? state[0].arg : 0);
   if (!file)
     return grub_errno;
 
@@ -139,7 +178,7 @@ 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 ? &whitelist : NULL, set_var);
   grub_envblk_close (envblk);
 
 fail:
@@ -172,7 +211,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:
@@ -387,7 +426,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 =
diff --git a/grub-core/lib/envblk.c b/grub-core/lib/envblk.c
index 311927b..50d87c8 100644
--- a/grub-core/lib/envblk.c
+++ b/grub-core/lib/envblk.c
@@ -223,8 +223,22 @@ grub_envblk_delete (grub_envblk_t envblk, const char *name)
     }
 }
 
+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 */
+}
+
 void
 grub_envblk_iterate (grub_envblk_t envblk,
+                     const grub_env_whitelist_t* whitelist,
                      int hook (const char *name, const char *value))
 {
   char *p, *pend;
@@ -240,7 +254,7 @@ grub_envblk_iterate (grub_envblk_t envblk,
           char *value;
           char *name_start, *name_end, *value_start;
           char *q;
-          int ret;
+          int ret = 0;
 
           name_start = p;
           while (p < pend && *p != '=')
@@ -285,7 +299,9 @@ grub_envblk_iterate (grub_envblk_t envblk,
             }
           *q = '\0';
 
-          ret = hook (name, value);
+          if (!whitelist ||
+              (whitelist && test_whitelist_membership (name, whitelist)))
+            ret = hook (name, value);
           grub_free (name);
           if (ret)
             return;
diff --git a/include/grub/lib/envblk.h b/include/grub/lib/envblk.h
index 368ba53..8857058 100644
--- a/include/grub/lib/envblk.h
+++ b/include/grub/lib/envblk.h
@@ -31,10 +31,18 @@ struct grub_envblk
 };
 typedef struct grub_envblk *grub_envblk_t;
 
+struct grub_env_whitelist
+{
+  grub_size_t len;
+  char **list;
+};
+typedef struct grub_env_whitelist grub_env_whitelist_t;
+
 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,
+                          const grub_env_whitelist_t *whitelist,
                           int hook (const char *name, const char *value));
 void grub_envblk_close (grub_envblk_t envblk);
 
diff --git a/util/grub-editenv.c b/util/grub-editenv.c
index 9b51acf..f9b6959 100644
--- a/util/grub-editenv.c
+++ b/util/grub-editenv.c
@@ -197,7 +197,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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]