[PATCH v2 0/5] Enable savedefault, etc with check_signatures=enforce

[PATCH v2 0/5] Enable savedefault, etc with check_signatures=enforce

[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".  Some additional work is done to make
this support usable: A {-k, --pubkey} option is added to
grub-install, and significant documentation is included.  See the
individual patch descriptions for more specifics.

Jon McCune (5):
  style: indent --no-tabs --gnu-style grub-core/commands/loadenv.c
  load_env support for whitelisting which variables are read
  save_env should work, even if check_signatures=enforce
  Add -k, --pubkey=FILE support to grub-install command
  Additional security-relevant documentation

 docs/grub.texi               | 180 ++++++++++++++++++++++++++++++++++++++++++-
 grub-core/commands/loadenv.c | 171 ++++++++++++++++++++++++++++------------
 util/grub-install.in         |  13 +++-
 util/grub-install_header     |   6 ++
 4 files changed, 316 insertions(+), 54 deletions(-)

-- 
1.8.4

[PATCH v2 1/5] 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

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

[PATCH v2 3/5] 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 49e8004..efecf6f 100644
--- a/grub-core/commands/loadenv.c
+++ b/grub-core/commands/loadenv.c
@@ -406,7 +406,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 v2 4/5] Add -k, --pubkey=FILE support to grub-install command

[Jon McCune]

This simply passes along the public key to the grub-mkimage invocation

Signed-off-by: Jon McCune <jonmccune@google.com>
---
 util/grub-install.in     | 13 +++++++++----
 util/grub-install_header |  6 ++++++
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/util/grub-install.in b/util/grub-install.in
index 1816bb1..3df0087 100644
--- a/util/grub-install.in
+++ b/util/grub-install.in
@@ -650,10 +650,15 @@ case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
     *) imgext=img ;;
 esac
 
+pubkey_file_arg=""
+if [ -e "$pubkey_file" ]; then
+    pubkey_file_arg="--pubkey=$pubkey_file"
+fi
+
 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
+    "$grub_mkimage" "$pubkey_file_arg" -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
 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
+    "$grub_mkimage" -c "${config_opt_file}" "$pubkey_file_arg" -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
 fi
 
 # Backward-compatibility kludges
@@ -664,9 +669,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
+	"$grub_mkimage" "$pubkey_file_arg" -d "${source_directory}" -O "${mkimage_target}" --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/grub.efi" --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
+	"$grub_mkimage" -c "${config_opt_file}" "$pubkey_file_arg" -d "${source_directory}" -O "${mkimage_target}" --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/grub.efi" --prefix="" $grub_decompression_module $modules || exit 1
     fi
 fi
 
diff --git a/util/grub-install_header b/util/grub-install_header
index cf7fa9d..20a0321 100644
--- a/util/grub-install_header
+++ b/util/grub-install_header
@@ -156,6 +156,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 +169,7 @@ grub_decompression_module=""
 compressor=""
 compressor_opts=""
 source_directory=""
+pubkey_file=""
 
 argument () {
   opt=$1
@@ -245,6 +247,10 @@ 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)
+	    pubkey_file=`argument $option "$@"`; grub_process_install_options_consumed=2 ;;
+	--pubkey=*)
+	    pubkey_file=`echo "$option" | sed 's/--pubkey=//'` grub_process_install_options_consumed=1;;
         --modules)
 	    modules=`argument $option "$@"`; grub_process_install_options_consumed=2;;
         --modules=*)
-- 
1.8.4

[PATCH v2 5/5] Additional security-relevant documentation

[Jon McCune]

Documentation for commands associated with check_signatures:

* High-level discussion in new section "Security and signatures"
* Environment variable check_signatures
* New documentation for commands distrust, trust, list_trusted, verify_detached
  (pre-existing commands, but not covered by existing documentation)
* Modifications to documentation for load_env, save_env, hashsum

Signed-off-by: Jon McCune <jonmccune@google.com>
---
 docs/grub.texi | 180 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 178 insertions(+), 2 deletions(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index d5ed014..80baa03 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -96,6 +96,7 @@ This edition documents version @value{VERSION}.
 * Commands::                    The list of available builtin commands
 * Internationalisation::        Topics relating to language support
 * Security::                    Authentication and authorisation
+* Security and signatures::     Verifying digital signatures in GRUB
 * Platform limitations::        The list of platform-specific limitations
 * Platform-specific operations:: Platform-specific operations
 * Supported kernels::           The list of supported kernels
@@ -2741,6 +2742,7 @@ These variables have special meaning to GRUB.
 
 @menu
 * biosnum::
+* check_signatures::
 * chosen::
 * color_highlight::
 * color_normal::
@@ -2794,6 +2796,25 @@ For an alternative approach which also changes BIOS drive mappings for the
 chain-loaded system, @pxref{drivemap}.
 
 
+@node check_signatures
+@subsection check_signatures
+
+This variable controls whether GRUB enforces digital signature
+validation (@pxref{Security and signatures}) on all loaded files.  If
+@var{check_signatures=enforce}, then every attempt by the GRUB
+@file{core.img} to load another file @file{foo} (e.g., a loadable
+module, a configuration file, or a Linux kernel) implicitly invokes
+@code{verify_detached foo foo.sig} (@pxref{verify_detached}).
+@code{foo.sig} must contain a valid digital signature over the
+contents of @code{foo}, which can be verified with a public key
+currently trusted by grub (@pxref{list_trusted}, @pxref{trust},
+and @pxref{distrust}).  If validation fails, then file @file{foo}
+cannot be opened.  This failure may halt or otherwise impact the boot
+process.  An initial trusted public key can be embedded within the
+GRUB @file{core.img} using the @code{--pubkey} option to
+@command{grub-install} (@pxref{Invoking grub-install}).
+
+
 @node chosen
 @subsection chosen
 
@@ -3442,6 +3463,7 @@ you forget a command, you can run the command @command{help}
 * cryptomount::                 Mount a crypto device
 * date::                        Display or set current date and time
 * devicetree::                  Load a device tree blob
+* distrust::                    Remove a pubkey from trusted keys
 * drivemap::                    Map a drive to another
 * echo::                        Display a line of text
 * eval::                        Evaluate agruments as GRUB commands
@@ -3459,6 +3481,7 @@ you forget a command, you can run the command @command{help}
 * linux::                       Load a Linux kernel
 * linux16::                     Load a Linux kernel (16-bit mode)
 * list_env::                    List variables in environment block
+* list_trusted::                List trusted public keys
 * loadfont::                    Load font files
 * load_env::                    Load variables from environment block
 * loopback::                    Make a device from a filesystem image
@@ -3490,9 +3513,11 @@ you forget a command, you can run the command @command{help}
 * source::                      Read a configuration file in same context
 * test::                        Check file types and compare values
 * true::                        Do nothing, successfully
+* trust::                       Add public key to list of trusted keys
 * unset::                       Unset an environment variable
 * uppermem::                    Set the upper memory size
 @comment * vbeinfo::                     List available video modes
+* verify_detached::             Verify detached digital signature
 * videoinfo::                   List available video modes
 @end menu
 
@@ -3763,6 +3788,16 @@ but rather replaces it completely.
 @ref{GNU/Linux}.
 @end deffn
 
+@node distrust
+@subsection distrust
+
+@deffn Command distrust pubkey_id
+Remove public key @var{pubkey_id} from GRUB's keyring of trusted keys.
+These keys are used to validate signatures when
+@var{check_signatures=enforce} (@pxref{check_signatures}), and by some
+invocations of @command{verify_detached} (@pxref{verify_detached}).
+@xref{Security and signatures}, for more information.
+@end deffn
 
 @node drivemap
 @subsection drivemap
@@ -3921,7 +3956,8 @@ list of @var{hash name} pairs in the same format as used by UNIX
 @command{md5sum} command. Option @option{--prefix}
 may be used to give directory where files are located. Hash verification
 stops after the first mismatch was found unless option @option{--keep-going}
-was given.
+was given.  The exit code @code{$?} is set to 0 if hash verification
+is successful.  If it fails, @code{$?} is set to a nonzero value.
 @end deffn
 
 
@@ -4030,16 +4066,41 @@ The @option{-f} option overrides the default location of the environment
 block.
 @end deffn
 
+@node list_trusted
+@subsection list_trusted
+
+@deffn Command list_trusted
+List all public keys trusted by GRUB for validating signatures. These
+public keys are used implicitly when environment variable
+@var{check_signatures=enforce} (@pxref{check_signatures}), and by some
+invocations of @command{verify_detached}.  @xref{Security and
+signatures}, for more information.
+@end deffn
 
 @node load_env
 @subsection load_env
 
-@deffn Command load_env [@option{-f} file]
+@deffn Command load_env [@option{-f} file] [whitelisted_variable_name] @dots{}
 Load all variables from the environment block file into the environment.
 @xref{Environment block}.
 
 The @option{-f} option overrides the default location of the environment
 block.
+
+If one or more variable names are provided as arguments, they are
+interpreted as a whitelist of variables to load from the environment
+block file.  Variables set in the file but not on the whitelist are
+ignored.
+
+If one or more whitelisted variable names are provided, and
+@var{check_signatures=enforce} (@pxref{check_signatures}), then
+those variables can be read from an untrusted (either unsigned or
+incorrectly signed) file.  When used with care, this enables an
+administrator to configure a system to boot only signed
+configurations, but to allow the user to select from among multiple
+configurations, and to enable ``one-shot'' boot attempts and
+``savedefault'' behavior.  @xref{Security and signatures}, for
+more information.
 @end deffn
 
 
@@ -4289,6 +4350,13 @@ Save the named variables from the environment to the environment block file.
 
 The @option{-f} option overrides the default location of the environment
 block.
+
+This command will operate successfully even when
+@var{check_signatures=enforce} (@pxref{check_signatures}), meaning
+that it is possible to modify a digitally signed environment block
+file from within grub such that its signature will no longer be valid
+on subsequent boots.  @xref{Security and signatures}, for more
+information.
 @end deffn
 
 
@@ -4598,6 +4666,19 @@ Do nothing, successfully.  This is mainly useful in control constructs such
 as @code{if} and @code{while} (@pxref{Shell-like scripting}).
 @end deffn
 
+@node trust
+@subsection trust
+
+@deffn Command trust pubkey_file
+Read public key from @var{pubkey_file} and add it to GRUB's internal
+list of trusted public keys.  These keys are used to validate digital
+signatures when @var{check_signatures=enforce}.  Note that if
+@var{check_signatures=enforce} when this command is run, then
+@var{pubkey_file} must itself be signed such that an already-loaded
+public key (@pxref{list_trusted}) can validate that signature.
+@xref{Security and signatures}, for more information.
+@end deffn
+
 
 @node unset
 @subsection unset
@@ -4624,6 +4705,21 @@ only on PC BIOS platforms.
 @end ignore
 
 
+@node verify_detached
+@subsection verify_detached
+
+@deffn Command verify_detached file signature_file [pubkey_file]
+Verifies a GPG-style detached signature, where the signed file is
+@var{file}, and the signature itself is in file @var{signature_file}.
+Optionally, a specific public key to use can be specified using
+@var{pubkey_file}.  Otherwise, public keys from GRUB's trusted keys
+(@pxref{list_trusted}, @pxref{trust}, and @pxref{distrust}) are
+tried.
+
+Exit code @code{$?} is set to 0 if the signature validates
+successfully.  If validation fails, it is set to a non-zero value.
+@end deffn
+
 @node videoinfo
 @subsection videoinfo
 
@@ -4813,6 +4909,86 @@ generating configuration files with authentication.  You can use
 adding @kbd{set superusers=} and @kbd{password} or @kbd{password_pbkdf2}
 commands.
 
+@node Security and signatures
+@chapter Security considerations when using digital signatures
+
+GRUB's @file{core.img} can optionally provide enforcement that all
+files subsequently read from disk are covered by a valid digital
+signature.  This includes GRUB configuration files, the GRUB
+environment block, GRUB loadable modules and their dependency files,
+and loaded operating system files such as a Linux kernel.  This
+document does @strong{not} cover how to ensure that your platform's
+firmware (e.g., GNU Coreboot or UEFI Secure Boot) validates
+@file{core.img}.
+
+GRUB uses GPG-style detached signatures (meaning that a file
+@file{foo.sig} will be produced when file @file{foo} is signed), and
+currently supports the DSA signing algorithm.  Both 2048-bit and
+3072-bit keys are supported. A signing key can be generated as
+follows:
+
+@example
+gpg --gen-key
+@end example
+
+The corresponding public key must be embedded in @file{core.img} when
+executing the @command{grub-install} (@pxref{Invoking grub-install})
+utility.  This can be done using the @code{--pubkey} option and
+manually specifying that the modules required for signature
+verification be embedded in @file{core.img}.  For example:
+
+@example
+grub-install \
+  --pubkey=/boot/pubkey.gpg \
+  --modules="verify gcry_rsa gcry_dsa gcry_sha256 hashsum"\
+"gcry_sha1 mpi echo loadenv" \
+  /dev/sda
+@end example
+
+An individual file can be signed as follows:
+
+@example
+gpg --detach-sign /path/to/file
+@end example
+
+For successful validation of all of GRUB's subcomponents and the
+loaded OS kernel, they must all be signed.  One way to accomplish this
+is the following (after having already produced the desired
+@file{grub.cfg} file, e.g., by running @command{grub-mkconfig}
+(@pxref{Invoking grub-mkconfig}):
+
+@example
+@group
+# Edit /dev/shm/passphrase.txt to contain your signing key's passphrase
+for i in `find /boot -name "*.cfg" -or -name "*.lst" -or \
+  -name "*.mod" -or -name "vmlinuz*" -or -name "initrd*" -or \
+  -name "grubenv"`;
+do
+  gpg --batch --detach-sign --passphrase-fd 0 $i < \
+    /dev/shm/passphrase.txt
+done
+shred /dev/shm/passphrase.txt
+@end group
+@end example
+
+See also: @ref{check_signatures}, @ref{verify_detached}, @ref{trust},
+@ref{list_trusted}, @ref{distrust}, @ref{load_env}, @ref{save_env}.
+
+Note that internally signature enforcement is controlled by setting
+the environment variable @var{check_signatures=enforce}.  Passing a
+@code{--pubkey} option to @command{grub-install} implicitly enables
+enforcement in @file{core.img}.
+
+Note that signature checking does @strong{not} prevent an attacker
+with (serial, physical, ...) console access from dropping manually to
+the grub console and executing:
+
+@example
+set check_signatures=no
+@end example
+
+To prevent this, password-protection as described in @pxref{Security}
+is essential.
 
 @node Platform limitations
 @chapter Platform limitations
-- 
1.8.4

Re: [PATCH v2 4/5] Add -k, --pubkey=FILE support to grub-install command

[Andrey Borzenkov]

В Fri,  6 Sep 2013 09:18:52 -0700
Jon McCune <jonmccune@google.com> пишет:

> This simply passes along the public key to the grub-mkimage invocation
> 
> Signed-off-by: Jon McCune <jonmccune@google.com>
> ---
>  util/grub-install.in     | 13 +++++++++----
>  util/grub-install_header |  6 ++++++
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/util/grub-install.in b/util/grub-install.in
> index 1816bb1..3df0087 100644
> --- a/util/grub-install.in
> +++ b/util/grub-install.in
> @@ -650,10 +650,15 @@ case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
>      *) imgext=img ;;
>  esac
>  
> +pubkey_file_arg=""
> +if [ -e "$pubkey_file" ]; then
> +    pubkey_file_arg="--pubkey=$pubkey_file"
> +fi
> +

You should return an error if file does not exist, not silently ignore
it.

> +	--pubkey | -k)
> +	    pubkey_file=`argument $option "$@"`; grub_process_install_options_consumed=2 ;;
> +	--pubkey=*)
> +	    pubkey_file=`echo "$option" | sed 's/--pubkey=//'` grub_process_install_options_consumed=1;;

grub-mkimage supports multiple keys. This will work only for exactly
one.

>          --modules)
>  	    modules=`argument $option "$@"`; grub_process_install_options_consumed=2;;
>          --modules=*)

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

[Andrey Borzenkov]

В Fri,  6 Sep 2013 09:18:50 -0700
Jon McCune <jonmccune@google.com> пишет:

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

What is the use case? load_env is called exactly once at the beginning
of configfile processing. At this point file still has valid signature
assuming grub-editenv (or some other tool) computed one. When do you
need to load environment more than once? 

Re: [PATCH v2 2/5] 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: 4781 bytes --]

Thanks for the feedback, inline:

On Fri, Sep 6, 2013 at 12:48 PM, Andrey Borzenkov <arvidjaar@gmail.com>wrote:

> В Fri,  6 Sep 2013 09:18:50 -0700
> Jon McCune <jonmccune@google.com> пишет:
>
> > 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.
>
> What is the use case? load_env is called exactly once at the beginning
> of configfile processing. At this point file still has valid signature
> assuming grub-editenv (or some other tool) computed one. When do you
> need to load environment more than once?
>

I agree that the default grub.cfg behaves such as you describe, but
consider a configuration where the signing key is not available during
every boot cycle.  E.g., it is password-protected by a password that I
know, but that other users of the machine do not know.  Let's assume it's a
server in a physically secure location so that, e.g., booting from a CD or
USB drive is not a viable attack.  Let us also assume that the attacker may
gain root privileges in the OS at some point after the bootloader
configuration is completed and the signing key is secured.

Now suppose I want to enable "savedefault" functionality, so that users can
control which of several installed OSes, kernels, kernel configurations,
... to boot.  I don't care which configuration boots, or which one is the
default, but I want to make sure the machine only boots known
configurations.

If I attempt such a configuration today, with check_signatures=enforce, it
becomes impossible to save_env any kind of default (or next_entry, or
boot_once) variable.  I might try to cope with this by writing my (signed)
grub.cfg to temporarily disable check_signatures:

set check_signatures=no
save_env next_entry boot_once prev_next_entry
set check_signatures=yes

Unfortunately, the grubenv file has now changed.  If it was previously
signed, that signature is no longer valid.  Thus, in order to load in any
environment variables (on the next reboot), I again have to disable
check_signatures.

set check_signatures=no
load_env
set check_signatures=yes

In the default commodity grub.cfg, where load_env is called exactly once at
the top, I agree that this is not an especially serious limitation
(although changing, e.g., root or prefix could lead to availability
problems, but an attacker who can modify grubenv (i.e., they're root) can
already cause availability problems).

However, this opens up an attack for any more complex configurations:  I
have no idea what variables an attacker may have written into the grubenv
file during the previous boot.  Thus, after an untrusted load_env, I must
write the remainder of my grub.cfg not to depend on any pre-existing
variables that may now be attacker-controlled.  An untrusted load_env is
essentially a barrier across which any already-set environment variables
have to be discarded.  There's no way to securely save and restore them
across the untrusted load_env (as an attacker might guess the "saved_foo"
name or simply read it out of the .cfg file).

This "barrier" unnecessarily constrains the author of grub.cfg, and
negatively impacts more complex configurations.  Consider, e.g., using
multiple different signing keys to cover different configuration files that
are sourced by a primary configuration file, each of which may have their
own associated grubenv file.  Such a configuration cannot be created
securely with today's load_env and save_env.  My patch is intended to
remedy situations such as this.

Perhaps it is desirable to eliminate the changes to load_env and save_env
that control whether the PUBKEY filter is active, and simply add the
ability to specify that only certain variables will be set based on the
arguments to a load_env command?  This corresponds to the existing
functionality of save_env.  This would require an idiom in grub.cfg that
calls to save_env and load_env (if a previous save_env could have possibly
modified the relevant grubenv file) are wrapped in explicit disabling of
signature checking:

check_signatures=no
save_env foo bar baz
check_signatures=enforce

...

set check_signatures=no
load_env foo bar baz
set check_signatures=enforce

However, without my modifications (or something else offering equivalent
functionality) to load_env, I don't see a way for it to be possible to
securely load only selected configuration settings from more than one
untrusted environment file.

I hope this is clear, and I'm certainly open to alternatives if there's a
simpler approach available.

Thanks,
-Jon

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

Re: [PATCH v2 4/5] Add -k, --pubkey=FILE support to grub-install command

[Jonathan McCune]

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

Good points, I'll fix these and send a new version of the patch after I
hear what folks think about the more substantial changes.

On Fri, Sep 6, 2013 at 12:40 PM, Andrey Borzenkov <arvidjaar@gmail.com>wrote:

> В Fri,  6 Sep 2013 09:18:52 -0700
> Jon McCune <jonmccune@google.com> пишет:
>
>
> > +pubkey_file_arg=""
> > +if [ -e "$pubkey_file" ]; then
> > +    pubkey_file_arg="--pubkey=$pubkey_file"
> > +fi
> > +
>
> You should return an error if file does not exist, not silently ignore
> it.


Will fix.


> > +     --pubkey | -k)
> > +         pubkey_file=`argument $option "$@"`;
> grub_process_install_options_consumed=2 ;;
> > +     --pubkey=*)
> > +         pubkey_file=`echo "$option" | sed 's/--pubkey=//'`
> grub_process_install_options_consumed=1;;
>
> grub-mkimage supports multiple keys. This will work only for exactly
> one.
>
>
Will fix.

Thanks,
-Jon

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

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

[Andrey Borzenkov]

В Fri, 6 Sep 2013 14:10:01 -0700
Jonathan McCune <jonmccune@google.com> пишет:

> Thanks for the feedback, inline:
> 
> On Fri, Sep 6, 2013 at 12:48 PM, Andrey Borzenkov <arvidjaar@gmail.com>wrote:
> 
> > В Fri,  6 Sep 2013 09:18:50 -0700
> > Jon McCune <jonmccune@google.com> пишет:
> >
> > > 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.
> >
> > What is the use case? load_env is called exactly once at the beginning
> > of configfile processing. At this point file still has valid signature
> > assuming grub-editenv (or some other tool) computed one. When do you
> > need to load environment more than once?
> >
> 
> I agree that the default grub.cfg behaves such as you describe, but
> consider a configuration where the signing key is not available during
> every boot cycle.  E.g., it is password-protected by a password that I
> know, but that other users of the machine do not know.  Let's assume it's a
> server in a physically secure location so that, e.g., booting from a CD or
> USB drive is not a viable attack.  Let us also assume that the attacker may
> gain root privileges in the OS at some point after the bootloader
> configuration is completed and the signing key is secured.
> 
> Now suppose I want to enable "savedefault" functionality, so that users can
> control which of several installed OSes, kernels, kernel configurations,
> ... to boot.  I don't care which configuration boots, or which one is the
> default, but I want to make sure the machine only boots known
> configurations.
> 

So just use another environment block for untrusted variables, that's
all. I do not see why any change in sources is required.

Now if you could come up with solution that maintains compatibility
with existing grub.cfg, that would be valid reason. But right now
grub.cfg must be changed anyway at which point just save untrusted
variables separately from trusted. 

> If I attempt such a configuration today, with check_signatures=enforce, it
> becomes impossible to save_env any kind of default (or next_entry, or
> boot_once) variable.  I might try to cope with this by writing my (signed)
> grub.cfg to temporarily disable check_signatures:
> 
> set check_signatures=no
> save_env next_entry boot_once prev_next_entry
> set check_signatures=yes
> 
> Unfortunately, the grubenv file has now changed.  If it was previously
> signed, that signature is no longer valid.  Thus, in order to load in any
> environment variables (on the next reboot), I again have to disable
> check_signatures.
> 
> set check_signatures=no
> load_env
> set check_signatures=yes
> 
> In the default commodity grub.cfg, where load_env is called exactly once at
> the top, I agree that this is not an especially serious limitation
> (although changing, e.g., root or prefix could lead to availability
> problems, but an attacker who can modify grubenv (i.e., they're root) can
> already cause availability problems).
> 
> However, this opens up an attack for any more complex configurations:  I
> have no idea what variables an attacker may have written into the grubenv
> file during the previous boot.  Thus, after an untrusted load_env, I must
> write the remainder of my grub.cfg not to depend on any pre-existing
> variables that may now be attacker-controlled.  An untrusted load_env is
> essentially a barrier across which any already-set environment variables
> have to be discarded.  There's no way to securely save and restore them
> across the untrusted load_env (as an attacker might guess the "saved_foo"
> name or simply read it out of the .cfg file).
> 
> This "barrier" unnecessarily constrains the author of grub.cfg, and
> negatively impacts more complex configurations.  Consider, e.g., using
> multiple different signing keys to cover different configuration files that
> are sourced by a primary configuration file, each of which may have their
> own associated grubenv file.  Such a configuration cannot be created
> securely with today's load_env and save_env.  My patch is intended to
> remedy situations such as this.
> 
> Perhaps it is desirable to eliminate the changes to load_env and save_env
> that control whether the PUBKEY filter is active, and simply add the
> ability to specify that only certain variables will be set based on the
> arguments to a load_env command?  This corresponds to the existing
> functionality of save_env.  This would require an idiom in grub.cfg that
> calls to save_env and load_env (if a previous save_env could have possibly
> modified the relevant grubenv file) are wrapped in explicit disabling of
> signature checking:
> 
> check_signatures=no
> save_env foo bar baz
> check_signatures=enforce
> 
> ...
> 
> set check_signatures=no
> load_env foo bar baz
> set check_signatures=enforce
> 
> However, without my modifications (or something else offering equivalent
> functionality) to load_env, I don't see a way for it to be possible to
> securely load only selected configuration settings from more than one
> untrusted environment file.
> 
> I hope this is clear, and I'm certainly open to alternatives if there's a
> simpler approach available.
> 
> Thanks,
> -Jon

Re: [PATCH v2 2/5] 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: 2637 bytes --]

On Sat, Sep 7, 2013 at 2:33 AM, Andrey Borzenkov <arvidjaar@gmail.com>wrote:

> В Fri, 6 Sep 2013 14:10:01 -0700
> Jonathan McCune <jonmccune@google.com> пишет:
>
> > Thanks for the feedback, inline:
> >
> > On Fri, Sep 6, 2013 at 12:48 PM, Andrey Borzenkov <arvidjaar@gmail.com
> >wrote:
> >
> > > В Fri,  6 Sep 2013 09:18:50 -0700
> > > Jon McCune <jonmccune@google.com> пишет:
> > >
> > > > 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.
> > >
> > > What is the use case? load_env is called exactly once at the beginning
> > > of configfile processing. At this point file still has valid signature
> > > assuming grub-editenv (or some other tool) computed one. When do you
> > > need to load environment more than once?
> > >
> >
> > I agree that the default grub.cfg behaves such as you describe, but
> > consider a configuration where the signing key is not available during
> > every boot cycle.  E.g., it is password-protected by a password that I
> > know, but that other users of the machine do not know.  Let's assume
> it's a
> > server in a physically secure location so that, e.g., booting from a CD
> or
> > USB drive is not a viable attack.  Let us also assume that the attacker
> may
> > gain root privileges in the OS at some point after the bootloader
> > configuration is completed and the signing key is secured.
> >
> > Now suppose I want to enable "savedefault" functionality, so that users
> can
> > control which of several installed OSes, kernels, kernel configurations,
> > ... to boot.  I don't care which configuration boots, or which one is the
> > default, but I want to make sure the machine only boots known
> > configurations.
> >
>
> So just use another environment block for untrusted variables, that's
> all. I do not see why any change in sources is required.
>
>
What about variables that are set in a load.cfg (grub-mkimage -c FILE)?
 Those can no longer be trusted after doing an untrusted load-env, even if
it's the very first thing in grub.cfg.


> Now if you could come up with solution that maintains compatibility
> with existing grub.cfg, that would be valid reason. But right now
> grub.cfg must be changed anyway at which point just save untrusted
> variables separately from trusted.
>
>
I don't think my changes break compatibility with anybody's existing
grub.cfg.  Can you be more specific?

Thanks,
-Jon

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

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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 07.09.2013 11:33, Andrey Borzenkov wrote:
> So just use another environment block for untrusted variables, that's
> all. I do not see why any change in sources is required.
Trouble is that right now we unconditionally load all variables from
block, whether trusted or not. So by modifying untrusted but loaded
block you can override core variables i.a. check_signatures. That's why
some ability to filter is required.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]

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

[Andrey Borzenkov]

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

В Thu, 19 Sep 2013 09:18:55 +0200
Vladimir 'φ-coder/phcoder' Serbinenko <phcoder@gmail.com> пишет:

> On 07.09.2013 11:33, Andrey Borzenkov wrote:
> > So just use another environment block for untrusted variables, that's
> > all. I do not see why any change in sources is required.
> Trouble is that right now we unconditionally load all variables from
> block, whether trusted or not. So by modifying untrusted but loaded
> block you can override core variables i.a. check_signatures. That's why
> some ability to filter is required.
> 

Yep, I realized this after replying. So extending load_env to take
environment variable names is needed (somehow I was sure it already
supported it).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Andrey Borzenkov]

В Mon, 9 Sep 2013 08:34:10 -0700
Jonathan McCune <jonmccune@google.com> пишет:

> 
> > Now if you could come up with solution that maintains compatibility
> > with existing grub.cfg, that would be valid reason. But right now
> > grub.cfg must be changed anyway at which point just save untrusted
> > variables separately from trusted.
> >
> >
> I don't think my changes break compatibility with anybody's existing
> grub.cfg.  Can you be more specific?
> 

Currently grub.cfg loads all variables from environment block. Your
change would require changing it to load only whitelisted variables.

Re: [PATCH v2 2/5] 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: 1652 bytes --]

I was thinking that an empty whitelist should implicitly *allow* all.  The
presence of one or more variables in the whitelist is a signal that the
user cares and explicitly disallows anything not in the whitelist.  I think
this is totally compatible with any existing grub.cfg, unless somebody has
some junk similar to load_env [-f FILE] junk1 junk2...  The existing code
in loadenv.c:grub_cmd_load_env() doesn't even look at argc, so I think it
would ignore such junk.

I have some other feedback from irc that I will incorporate, and do a v4 of
these patches.  The v3 changes to loadenv.c don't completely make sense, as
I was trying to react to Andrey's feedback before he realized the whitelist
wasn't already implemented.

Thanks,
-Jon






On Thu, Sep 19, 2013 at 3:12 AM, Andrey Borzenkov <arvidjaar@gmail.com>wrote:

> В Mon, 9 Sep 2013 08:34:10 -0700
> Jonathan McCune <jonmccune@google.com> пишет:
>
> >
> > > Now if you could come up with solution that maintains compatibility
> > > with existing grub.cfg, that would be valid reason. But right now
> > > grub.cfg must be changed anyway at which point just save untrusted
> > > variables separately from trusted.
> > >
> > >
> > I don't think my changes break compatibility with anybody's existing
> > grub.cfg.  Can you be more specific?
> >
>
> Currently grub.cfg loads all variables from environment block. Your
> change would require changing it to load only whitelisted variables.
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

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