[PATCH v4 0/4] Add whitelisting support for load_env

[PATCH v4 0/4] Add whitelisting support for load_env

[Jon McCune]

These patches add support to load_env and save_env to work nicely
in concert with check_signatures=enforce.  This represents an
evolution from the design in my email to grub-devel entitled
"Proposal to enable savedefault, one-shot reboot, etc with
check_signatures=enforce".  In particular, no new commands are added.
Instead, the behavior of load_env is modified based on the presence
of a whitelist of environment variables to be updated from an
untrusted grubenv-style file.

Some additional work is done to make this support usable:
A {-k, --pubkey} option is added to:
grub-install, grub-mkrescue, grub-mkstandalone, grub-mknetdir.
See the individual patch descriptions for more specifics.

Jon McCune (4):
  style: indent --no-tabs --gnu-style grub-core/commands/loadenv.c
  load_env support for whitelisting which variables are read from an env
    file, even if check_signatures=enforce
  save_env should work, even if check_signatures=enforce
  Add (multiple) -k, --pubkey=FILE support to installation commands.

 grub-core/commands/loadenv.c | 139 ++++++++++++++++++++++++++++---------------
 grub-core/lib/envblk.c       |  20 ++++++-
 include/grub/lib/envblk.h    |   8 +++
 util/grub-editenv.c          |   2 +-
 util/grub-install.in         |   8 +--
 util/grub-install_header     |  17 ++++++
 util/grub-mknetdir.in        |   2 +-
 util/grub-mkrescue.in        |  12 ++--
 util/grub-mkstandalone.in    |   2 +-
 9 files changed, 146 insertions(+), 64 deletions(-)

-- 
1.8.4

[PATCH v4 1/4] style: indent --no-tabs --gnu-style grub-core/commands/loadenv.c

[Jon McCune]

Added the option --no-tabs after observing that 'indent' did not
result in consistent tab usage.

An example of the inconsistent tab usage: the very first function
in loadenv.c is open_envblk_file(), and it contains a line
indented exclusively with tabs, and lines indented exclusively
with spaces.  The space-idented lines use two spaces for each
level of indentation.  In order for the tab-indented lines to
format properly, the tabs must consume the equivalent of 8
spaces (i.e., four levels of indentation). To my knowledge, this
is not consistent with the default GNU style.

Signed-off-by: Jon McCune <jonmccune@google.com>
---
 grub-core/commands/loadenv.c | 87 ++++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 43 deletions(-)

diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.c
index c0a42c5..a431499 100644
--- a/grub-core/commands/loadenv.c
+++ b/grub-core/commands/loadenv.c
@@ -30,20 +30,19 @@
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
-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},
-    {0, 0, 0, 0, 0, 0}
-  };
+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},
+  {0, 0, 0, 0, 0, 0}
+};
 
 static grub_file_t
 open_envblk_file (char *filename)
 {
   grub_file_t file;
 
-  if (! filename)
+  if (!filename)
     {
       const char *prefix;
 
@@ -54,20 +53,21 @@ open_envblk_file (char *filename)
 
           len = grub_strlen (prefix);
           filename = grub_malloc (len + 1 + sizeof (GRUB_ENVBLK_DEFCFG));
-          if (! filename)
+          if (!filename)
             return 0;
 
           grub_strcpy (filename, prefix);
           filename[len] = '/';
           grub_strcpy (filename + len + 1, GRUB_ENVBLK_DEFCFG);
-	  grub_file_filter_disable_compression ();
+          grub_file_filter_disable_compression ();
           file = grub_file_open (filename);
           grub_free (filename);
           return file;
         }
       else
         {
-          grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("variable `%s' isn't set"), "prefix");
+          grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("variable `%s' isn't set"),
+                      "prefix");
           return 0;
         }
     }
@@ -85,7 +85,7 @@ read_envblk_file (grub_file_t file)
   grub_envblk_t envblk;
 
   buf = grub_malloc (size);
-  if (! buf)
+  if (!buf)
     return 0;
 
   while (size > 0)
@@ -104,7 +104,7 @@ read_envblk_file (grub_file_t file)
     }
 
   envblk = grub_envblk_open (buf, offset);
-  if (! envblk)
+  if (!envblk)
     {
       grub_free (buf);
       grub_error (GRUB_ERR_BAD_FILE_TYPE, "invalid environment block");
@@ -124,25 +124,25 @@ set_var (const char *name, const char *value)
 
 static grub_err_t
 grub_cmd_load_env (grub_extcmd_context_t ctxt,
-		   int argc __attribute__ ((unused)),
-		   char **args __attribute__ ((unused)))
+                   int argc __attribute__ ((unused)),
+                   char **args __attribute__ ((unused)))
 {
   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);
-  if (! file)
+  if (!file)
     return grub_errno;
 
   envblk = read_envblk_file (file);
-  if (! envblk)
+  if (!envblk)
     goto fail;
 
   grub_envblk_iterate (envblk, set_var);
   grub_envblk_close (envblk);
 
- fail:
+fail:
   grub_file_close (file);
   return grub_errno;
 }
@@ -157,25 +157,25 @@ print_var (const char *name, const char *value)
 
 static grub_err_t
 grub_cmd_list_env (grub_extcmd_context_t ctxt,
-		   int argc __attribute__ ((unused)),
-		   char **args __attribute__ ((unused)))
+                   int argc __attribute__ ((unused)),
+                   char **args __attribute__ ((unused)))
 {
   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);
-  if (! file)
+  if (!file)
     return grub_errno;
 
   envblk = read_envblk_file (file);
-  if (! envblk)
+  if (!envblk)
     goto fail;
 
   grub_envblk_iterate (envblk, print_var);
   grub_envblk_close (envblk);
 
- fail:
+fail:
   grub_file_close (file);
   return grub_errno;
 }
@@ -253,7 +253,7 @@ check_blocklists (grub_envblk_t envblk, struct blocklist *blocklists,
         return grub_errno;
 
       if (grub_memcmp (buf + index, blockbuf, p->length) != 0)
-	return grub_error (GRUB_ERR_FILE_READ_ERROR, "invalid blocklist");
+        return grub_error (GRUB_ERR_FILE_READ_ERROR, "invalid blocklist");
     }
 
   return GRUB_ERR_NONE;
@@ -293,7 +293,7 @@ struct grub_cmd_save_env_ctx
 /* Store blocklists in a linked list.  */
 static void
 save_env_read_hook (grub_disk_addr_t sector, unsigned offset, unsigned length,
-		    void *data)
+                    void *data)
 {
   struct grub_cmd_save_env_ctx *ctx = data;
   struct blocklist *block;
@@ -303,7 +303,7 @@ save_env_read_hook (grub_disk_addr_t sector, unsigned offset, unsigned length,
     return;
 
   block = grub_malloc (sizeof (*block));
-  if (! block)
+  if (!block)
     return;
 
   block->sector = sector;
@@ -315,7 +315,7 @@ save_env_read_hook (grub_disk_addr_t sector, unsigned offset, unsigned length,
   if (ctx->tail)
     ctx->tail->next = block;
   ctx->tail = block;
-  if (! ctx->head)
+  if (!ctx->head)
     ctx->head = block;
 }
 
@@ -330,14 +330,14 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
     .tail = 0
   };
 
-  if (! argc)
+  if (!argc)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "no variable is specified");
 
   file = open_envblk_file ((state[0].set) ? state[0].arg : 0);
-  if (! file)
+  if (!file)
     return grub_errno;
 
-  if (! file->device->disk)
+  if (!file->device->disk)
     {
       grub_file_close (file);
       return grub_error (GRUB_ERR_BAD_DEVICE, "disk device required");
@@ -347,7 +347,7 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
   file->read_hook_data = &ctx;
   envblk = read_envblk_file (file);
   file->read_hook = 0;
-  if (! envblk)
+  if (!envblk)
     goto fail;
 
   if (check_blocklists (envblk, ctx.head, file))
@@ -360,9 +360,10 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
       value = grub_env_get (args[0]);
       if (value)
         {
-          if (! grub_envblk_set (envblk, args[0], value))
+          if (!grub_envblk_set (envblk, args[0], value))
             {
-              grub_error (GRUB_ERR_BAD_ARGUMENT, "environment block too small");
+              grub_error (GRUB_ERR_BAD_ARGUMENT,
+                          "environment block too small");
               goto fail;
             }
         }
@@ -373,7 +374,7 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
 
   write_blocklists (envblk, ctx.head, file);
 
- fail:
+fail:
   if (envblk)
     grub_envblk_close (envblk);
   free_blocklists (ctx.head);
@@ -383,24 +384,24 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
 
 static grub_extcmd_t cmd_load, cmd_list, cmd_save;
 
-GRUB_MOD_INIT(loadenv)
+GRUB_MOD_INIT (loadenv)
 {
   cmd_load =
     grub_register_extcmd ("load_env", grub_cmd_load_env, 0, N_("[-f FILE]"),
-			  N_("Load variables from environment block file."),
-			  options);
+                          N_("Load variables from environment block file."),
+                          options);
   cmd_list =
     grub_register_extcmd ("list_env", grub_cmd_list_env, 0, N_("[-f FILE]"),
-			  N_("List variables from environment block file."),
-			  options);
+                          N_("List variables from environment block file."),
+                          options);
   cmd_save =
     grub_register_extcmd ("save_env", grub_cmd_save_env, 0,
-			  N_("[-f FILE] variable_name [...]"),
-			  N_("Save variables to environment block file."),
-			  options);
+                          N_("[-f FILE] variable_name [...]"),
+                          N_("Save variables to environment block file."),
+                          options);
 }
 
-GRUB_MOD_FINI(loadenv)
+GRUB_MOD_FINI (loadenv)
 {
   grub_unregister_extcmd (cmd_load);
   grub_unregister_extcmd (cmd_list);
-- 
1.8.4

Re: [PATCH v4 1/4] style: indent --no-tabs --gnu-style grub-core/commands/loadenv.c

[Andrey Borzenkov]

В Tue, 24 Sep 2013 19:00:29 -0700
Jon McCune <jonmccune@google.com> пишет:

> Added the option --no-tabs after observing that 'indent' did not
> result in consistent tab usage.
> 

How does it belong to this patch series? If you want to send whitespace
fixes, send them as separate patch. It sure does not affect
whitelisting implementation, right? :)

Re: [PATCH v4 1/4] style: indent --no-tabs --gnu-style grub-core/commands/loadenv.c

[Jonathan McCune]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

Okay, I'll drop the whitespace fixes for now.

-Jon



On Tue, Sep 24, 2013 at 11:11 PM, Andrey Borzenkov <arvidjaar@gmail.com>wrote:

> В Tue, 24 Sep 2013 19:00:29 -0700
> Jon McCune <jonmccune@google.com> пишет:
>
> > Added the option --no-tabs after observing that 'indent' did not
> > result in consistent tab usage.
> >
>
> How does it belong to this patch series? If you want to send whitespace
> fixes, send them as separate patch. It sure does not affect
> whitelisting implementation, right? :)
>

[-- Attachment #2: Type: text/html, Size: 971 bytes --]

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

[Jon McCune]

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 <jonmccune@google.com>
---
 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

Re: [PATCH v4 2/4] load_env support for whitelisting which variables are read from an env file, even if check_signatures=enforce

[Andrey Borzenkov]

В Tue, 24 Sep 2013 19:00:30 -0700
Jon McCune <jonmccune@google.com> пишет:

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

I do not really like such silent assumptions. Being able to load only
selected environment variables is useful by itself, but I still may
want to ensure file was not tampered with.

I suggest you simply add flag "--skip-signature-check" to load_env and
add support for explicit variable list. Then it is up to user how and
when to use it. 

And please update also documentation for command changes.

> +static grub_file_t
> +open_envblk_file_untrusted (char *filename)

Why do you need extra function? Is not flag to open_envblk_file enough?

> +{
> +  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 ();

Why do you need to disable *all* filters? Assuming disabling
compression was good enough, you just need to disable signature
checking, right?

>  void
>  grub_envblk_iterate (grub_envblk_t envblk,
> +                     const grub_env_whitelist_t* whitelist,
>                       int hook (const char *name, const char *value))

Again, that's really too restrictive. Like with any other iterators,
I'd make hook accept extra pointer for hook-specific data and pass this
data to grub_envblk_iterate. This will let caller decide the policy.

Re: [PATCH v4 2/4] load_env support for whitelisting which variables are read from an env file, even if check_signatures=enforce

[Jonathan McCune]

[-- Attachment #1: Type: text/plain, Size: 3199 bytes --]

On Tue, Sep 24, 2013 at 11:09 PM, Andrey Borzenkov <arvidjaar@gmail.com>wrote:

> В Tue, 24 Sep 2013 19:00:30 -0700
> Jon McCune <jonmccune@google.com> пишет:
>
> > 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.
>
> I do not really like such silent assumptions. Being able to load only
> selected environment variables is useful by itself, but I still may
> want to ensure file was not tampered with.
>
> I suggest you simply add flag "--skip-signature-check" to load_env and
> add support for explicit variable list. Then it is up to user how and
> when to use it.
>

This is okay with me.  Other opinions?


> And please update also documentation for command changes.
>

Do you mean grub.texi as well, or just within the source code?  (e.g., for
'help load_env')


>
> > +static grub_file_t
> > +open_envblk_file_untrusted (char *filename)
>
> Why do you need extra function? Is not flag to open_envblk_file enough?
>

I wanted to keep the security-relevant disable / re-enable of the PUBKEY
filter as close together as possible.  open_envblk_file() does not
currently restore any filters that it disabled (the disable functions
operate globally, and not per-file), so it would complicate that function
somewhat to guarantee that PUBKEY was restored (as there are multiple valid
exit points from that function).  I will change this to use a flag instead
(and possibly restructure the function somewhat) if others agree.


>
> > +{
> > +  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 ();
>
> Why do you need to disable *all* filters? Assuming disabling
> compression was good enough, you just need to disable signature
> checking, right?
>

That is all the filters at the present time... PUBKEY and then various
compression algorithms.  I'm not sure what filters might be introduced in
the future, so I'm not sure whether it's safer to disable / enable all, or
disable / enable the full list of filters that exist today.  I thought the
more conservative option was to do them all.  I will change this to the
list of currently existing filters instead if others agree.


>
> >  void
> >  grub_envblk_iterate (grub_envblk_t envblk,
> > +                     const grub_env_whitelist_t* whitelist,
> >                       int hook (const char *name, const char *value))
>
> Again, that's really too restrictive. Like with any other iterators,
> I'd make hook accept extra pointer for hook-specific data and pass this
> data to grub_envblk_iterate. This will let caller decide the policy.
>

This sounds good to me.

-Jon

[-- Attachment #2: Type: text/html, Size: 4475 bytes --]

Re: [PATCH v4 2/4] load_env support for whitelisting which variables are read from an env file, even if check_signatures=enforce

[Andrey Borzenkov]

В Wed, 25 Sep 2013 08:01:25 -0700
Jonathan McCune <jonmccune@google.com> пишет:

> 
> 
> > And please update also documentation for command changes.
> >
> 
> Do you mean grub.texi as well, or just within the source code?  (e.g., for
> 'help load_env')
> 

I mean grub.texi

[PATCH v4 3/4] save_env should work, even if check_signatures=enforce

[Jon McCune]

This is accomplished by changing the call used to read the environment
block file to avoid being subject to signature checks.  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 do not pose a
security risk.

Signed-off-by: Jon McCune <jonmccune@google.com>
---
 grub-core/commands/loadenv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.c
index 16aafbf..05157df 100644
--- a/grub-core/commands/loadenv.c
+++ b/grub-core/commands/loadenv.c
@@ -372,7 +372,7 @@ 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_untrusted ((state[0].set) ? state[0].arg : 0);
   if (!file)
     return grub_errno;
 
-- 
1.8.4

[PATCH v4 4/4] Add (multiple) -k, --pubkey=FILE support to installation commands.

[Jon McCune]

Passes along one or more public keys to the grub-mkimage invocation.
Impacts grub-install, grub-mkrescue, grub-mkstandalone, grub-mknetdir.

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

This also introduces a quote () utility function to quote arbitrary
argument strings in a way that is POSIX compatible. Such strings
require 'eval' to be parsed properly. Quote function and help with
appropriate use of eval thanks to dalias (the author of:
http://www.etalabs.net/sh_tricks.html )

Signed-off-by: Jon McCune <jonmccune@google.com>
---
 util/grub-install.in      |  8 ++++----
 util/grub-install_header  | 17 +++++++++++++++++
 util/grub-mknetdir.in     |  2 +-
 util/grub-mkrescue.in     | 12 ++++++------
 util/grub-mkstandalone.in |  2 +-
 5 files changed, 29 insertions(+), 12 deletions(-)

diff --git a/util/grub-install.in b/util/grub-install.in
index 1816bb1..25a903f 100644
--- a/util/grub-install.in
+++ b/util/grub-install.in
@@ -651,9 +651,9 @@ case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
 esac
 
 if [ x"$config_opt_file" = x ]; then
-    "$grub_mkimage" -d "${source_directory}" -O "${mkimage_target}" --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/core.${imgext}" --prefix="${prefix_drive}${relative_grubdir}" $grub_decompression_module $modules || exit 1
+    eval "$(quote "$grub_mkimage") $pubkey_file_args -d $(quote "${source_directory}") -O $(quote "${mkimage_target}") $(quote --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/core.${imgext}") $(quote --prefix="${prefix_drive}${relative_grubdir}") $grub_decompression_module $modules" || exit 1
 else
-    "$grub_mkimage" -c "${config_opt_file}" -d "${source_directory}" -O "${mkimage_target}" --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/core.${imgext}" --prefix="${prefix_drive}${relative_grubdir}" $grub_decompression_module $modules || exit 1
+    eval "$(quote "$grub_mkimage") -c $(quote "${config_opt_file}") $pubkey_file_args -d $(quote "${source_directory}") -O $(quote "${mkimage_target}") $(quote --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/core.${imgext}") $(quote --prefix="${prefix_drive}${relative_grubdir}") $grub_decompression_module $modules" || exit 1
 fi
 
 # Backward-compatibility kludges
@@ -664,9 +664,9 @@ elif [ "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" = "i386-ieee1275" ]
 elif [ "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" = "i386-efi" ] || [ "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" = "x86_64-efi" ]; then
 
     if [ x"$config_opt_file" = x ]; then
-	"$grub_mkimage" -d "${source_directory}" -O "${mkimage_target}" --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/grub.efi" --prefix="" $grub_decompression_module $modules || exit 1
+        eval "$(quote "$grub_mkimage") $pubkey_file_args -d $(quote "${source_directory}") -O $(quote "${mkimage_target}") $(quote --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/grub.efi") $(quote --prefix="") $grub_decompression_module $modules" || exit 1
     else
-	"$grub_mkimage" -c "${config_opt_file}" -d "${source_directory}" -O "${mkimage_target}" --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/grub.efi" --prefix="" $grub_decompression_module $modules || exit 1
+        eval "$(quote "$grub_mkimage") -c $(quote "${config_opt_file}") $pubkey_file_args -d $(quote "${source_directory}") -O $(quote "${mkimage_target}") $(quote --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/grub.efi") $(quote --prefix="") $grub_decompression_module $modules" || exit 1
     fi
 fi
 
diff --git a/util/grub-install_header b/util/grub-install_header
index cf7fa9d..87550f6 100644
--- a/util/grub-install_header
+++ b/util/grub-install_header
@@ -44,6 +44,11 @@ handler.lst video.lst crypto.lst terminal.lst"
 
 grub_mkimage="${bindir}/@grub_mkimage@"
 
+# Thanks to http://www.etalabs.net/sh_tricks.html
+quote () {
+    printf %s\\n "$1" | sed "s/'/'\\\\''/g;1s/^/'/;\$s/\$/'/"
+}
+
 grub_compress_file () {
     if [ "$compressor" != "" ] ; then
         "$compressor" $compressor_opts "$1" > "$2"
@@ -156,6 +161,7 @@ grub_print_install_files_help () {
     dir_msg="$(gettext_printf "use images and modules under DIR [default=%s/<platform>]" "${libdir}/@PACKAGE@")"
     print_option_help "-d, --directory=$(gettext "DIR")" "$dir_msg"
     print_option_help  "--grub-mkimage=$(gettext "FILE")" "$(gettext "use FILE as grub-mkimage")"
+    print_option_help  "-k, --pubkey=$(gettext "FILE")" "$(gettext "embed FILE as public key for signature checking")"
     print_option_help "-v, --version" "$(gettext "print the version information and exit")"
 }
 
@@ -168,6 +174,7 @@ grub_decompression_module=""
 compressor=""
 compressor_opts=""
 source_directory=""
+pubkey_file_args=""
 
 argument () {
   opt=$1
@@ -245,6 +252,16 @@ grub_process_install_options () {
 	    grub_mkimage=`argument $option "$@"`; grub_process_install_options_consumed=2 ;;
         --grub-mkimage=*)
 	    grub_mkimage=`echo "$option" | sed 's/--grub-mkimage=//'`;grub_process_install_options_consumed=1 ;;
+	--pubkey | -k)
+        new_pubkey_file=`argument $option "$@"`
+        new_pubkey_file_quoted=`quote "$new_pubkey_file"`
+        pubkey_file_args="$pubkey_file_args --pubkey $new_pubkey_file_quoted"
+	    grub_process_install_options_consumed=2;;
+	--pubkey=*)
+        new_pubkey_file=`printf "%s" "$option" | sed 's/--pubkey=//'`
+        new_pubkey_file_quoted=`quote "$new_pubkey_file"`
+        pubkey_file_args="$pubkey_file_args --pubkey $new_pubkey_file_quoted"
+        grub_process_install_options_consumed=1;;
         --modules)
 	    modules=`argument $option "$@"`; grub_process_install_options_consumed=2;;
         --modules=*)
diff --git a/util/grub-mknetdir.in b/util/grub-mknetdir.in
index b7b59da..a573449 100644
--- a/util/grub-mknetdir.in
+++ b/util/grub-mknetdir.in
@@ -146,7 +146,7 @@ process_input_dir ()
 source ${subdir}/grub.cfg
 EOF
 
-    "$grub_mkimage" ${config_opt} -d "${input_dir}" -O ${mkimage_target} "--output=${grubdir}/core.$ext" "--prefix=$prefix" $modules $grub_decompression_module $netmodules tftp || exit 1
+    eval "$(quote "$grub_mkimage") ${config_opt} -d $(quote "${input_dir}") -O ${mkimage_target} $(quote "--output=${grubdir}/core.$ext") $(quote "--prefix=$prefix") $modules $grub_decompression_module $netmodules tftp" || exit 1
     # TRANSLATORS: First %s is replaced by platform name. Second one by filename.
     gettext_printf "Netboot directory for %s created. Configure your DHCP server to point to %s\n" "${platform}" "${subdir}/${platform}/core.$ext"
 }
diff --git a/util/grub-mkrescue.in b/util/grub-mkrescue.in
index 0adde3b..18be94c 100644
--- a/util/grub-mkrescue.in
+++ b/util/grub-mkrescue.in
@@ -209,8 +209,8 @@ EOF
         echo "insmod $i"
     done ; ) > "${load_cfg}"
 
-    "$grub_mkimage" -O ${platform} -d "${source_directory}" -c "${load_cfg}" -o "$3" \
-        $grub_decompression_module search iso9660 $4
+    eval "$(quote "$grub_mkimage") $pubkey_file_args -O ${platform} -d $(quote "${source_directory}") -c $(quote "${load_cfg}") -o $(quote "$3") \
+        $grub_decompression_module search iso9660 $4"
     rm -rf "${load_cfg}"
 }
 
@@ -224,8 +224,8 @@ make_image_fwdisk ()
 
     gettext_printf "Enabling %s support ...\n" "$2"
 
-    "$grub_mkimage" -O ${platform} -d "${source_directory}" -p '()/boot/grub' -o "$3" \
-        $grub_decompression_module iso9660 $4
+    eval "$(quote "$grub_mkimage") $pubkey_file_args -O ${platform} -d $(quote "${source_directory}") -p '()/boot/grub' -o $(quote "$3") \
+        $grub_decompression_module iso9660 $4"
 }
 
 if [ "${source_directory}" = "" ] ; then
@@ -335,8 +335,8 @@ if test -e "${pc_dir}" ; then
         echo "insmod $i"
     done ;) > "${load_cfg}"
 
-    "$grub_mkimage" -O i386-pc -d "${pc_dir}/" -o "${core_img}" -c "$load_cfg" --prefix=/boot/grub \
-        $grub_decompression_module iso9660 biosdisk
+    eval "$(quote "$grub_mkimage") $pubkey_file_args -O i386-pc -d $(quote "${pc_dir}/") -o $(quote "${core_img}") -c $(quote "$load_cfg") --prefix=/boot/grub \
+        $grub_decompression_module iso9660 biosdisk"
     cat "${pc_dir}/cdboot.img" "${core_img}" > "${iso9660_dir}/boot/grub/i386-pc/eltorito.img"
 
     grub_mkisofs_arguments="${grub_mkisofs_arguments} -b boot/grub/i386-pc/eltorito.img -no-emul-boot -boot-load-size 4 -boot-info-table"
diff --git a/util/grub-mkstandalone.in b/util/grub-mkstandalone.in
index b692c48..6e62a3f 100644
--- a/util/grub-mkstandalone.in
+++ b/util/grub-mkstandalone.in
@@ -124,7 +124,7 @@ memdisk_img=`mktemp "${TMPDIR:-/tmp}/tmp.XXXXXXXXXX"` || exit 1
 
 (cd "${memdisk_dir}"; tar -cf - * $source) > "${memdisk_img}"
 rm -rf "${memdisk_dir}"
-"$grub_mkimage" -O "${format}" -C "$compression" -d "${source_directory}" -m "${memdisk_img}" -o "$output_image" --prefix='(memdisk)/boot/grub' memdisk tar $grub_decompression_module $modules
+eval "$(quote "$grub_mkimage") -O $(quote "${format}") -C $(quote "$compression") -d $(quote "${source_directory}") -m $(quote "${memdisk_img}") -o $(quote "$output_image") --prefix='(memdisk)/boot/grub' memdisk tar $grub_decompression_module $modules"
 rm -rf "${memdisk_img}"
 
 exit 0
-- 
1.8.4