[PATCH v1 0/2] New commands: load_env_untrusted, save_env_untrusted

[PATCH v1 0/2] New commands: load_env_untrusted, save_env_untrusted

[Jon McCune]

These patches implement the commands load_env_untrusted and save_env_untrusted
as per the design in my email to grub-devel entitled
"Proposal to enable savedefault, one-shot reboot, etc with check_signatures=enforce"

Jon McCune (2):
  make check_blocklists() not depend on an envblk data type
  introduce new commands load_env_untrusted and save_env_untrusted:

 grub-core/commands/loadenv.c | 292 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 287 insertions(+), 5 deletions(-)

-- 
1.8.3

[PATCH v1 1/2] make check_blocklists() not depend on an envblk data type

[Jon McCune]

Signed-off-by: Jon McCune <jonmccune@google.com>

This is a small patch that makes check_blocklists() more generally useful,
and facilitates the next patch.

---
 grub-core/commands/loadenv.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.c
index c0a42c5..691d163 100644
--- a/grub-core/commands/loadenv.c
+++ b/grub-core/commands/loadenv.c
@@ -202,7 +202,7 @@ free_blocklists (struct blocklist *p)
 }
 
 static grub_err_t
-check_blocklists (grub_envblk_t envblk, struct blocklist *blocklists,
+check_blocklists (const char* expected_file_data, struct blocklist *blocklists,
                   grub_file_t file)
 {
   grub_size_t total_length;
@@ -210,7 +210,6 @@ check_blocklists (grub_envblk_t envblk, struct blocklist *blocklists,
   grub_disk_t disk;
   grub_disk_addr_t part_start;
   struct blocklist *p;
-  char *buf;
 
   /* Sanity checks.  */
   total_length = 0;
@@ -243,7 +242,6 @@ check_blocklists (grub_envblk_t envblk, struct blocklist *blocklists,
 
   part_start = grub_partition_get_start (disk->partition);
 
-  buf = grub_envblk_buffer (envblk);
   for (p = blocklists, index = 0; p; index += p->length, p = p->next)
     {
       char blockbuf[GRUB_DISK_SECTOR_SIZE];
@@ -252,7 +250,7 @@ check_blocklists (grub_envblk_t envblk, struct blocklist *blocklists,
                           p->offset, p->length, blockbuf))
         return grub_errno;
 
-      if (grub_memcmp (buf + index, blockbuf, p->length) != 0)
+      if (grub_memcmp (expected_file_data + index, blockbuf, p->length) != 0)
 	return grub_error (GRUB_ERR_FILE_READ_ERROR, "invalid blocklist");
     }
 
@@ -350,7 +348,7 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
   if (! envblk)
     goto fail;
 
-  if (check_blocklists (envblk, ctx.head, file))
+  if (check_blocklists (grub_envblk_buffer (envblk), ctx.head, file))
     goto fail;
 
   while (argc)
-- 
1.8.3

[PATCH v1 2/2] introduce new commands load_env_untrusted and save_env_untrusted:

[Jon McCune]

These commands will load / store a single named environment variable
from / to a 1024-byte file, regardless of the value of check_signatures.

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

diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.c
index 691d163..f6a363c 100644
--- a/grub-core/commands/loadenv.c
+++ b/grub-core/commands/loadenv.c
@@ -26,6 +26,7 @@
 #include <grub/partition.h>
 #include <grub/lib/envblk.h>
 #include <grub/extcmd.h>
+#include <grub/command.h>
 #include <grub/i18n.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
@@ -379,7 +380,280 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
   return grub_errno;
 }
 
+/* It is an error for a file being read or written with load_env_untrusted or
+   save_env_untrusted to be any other size. */
+#define UNTRUSTED_VAR_FILE_SIZE 1024
+
+/* Returns 0 if c is not in the set [A-Za-z0-9_] */
+static int
+is_AZaz09_(int c)
+{
+  return (grub_isalpha(c) || grub_isdigit(c) || '_' == c);
+}
+
+/* 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_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 = grub_file_open(filename);
+  /* Restore filters */
+  grub_memcpy (grub_file_filters_enabled, curfilt, sizeof (curfilt));
+
+  return file;
+}
+
+/* Reads an already-opened file into a freshly allocated buffer of size
+   UNTRUSTED_VAR_FILE_SIZE.  Returns pointer to buffer, or NULL on error.  The
+   returned data is *not* guaranteed to be NULL-terminated.
+   TODO: Reduce duplication of logic with read_envblk_file(). (Requires
+   refactoring read_envblk_file() and first-pass goal is to keep changes as
+   self-contained as possible.) */
+static char*
+read_file_untrusted (grub_file_t file)
+{
+  char *buf = NULL;
+  grub_size_t size;
+  grub_off_t offset;
+
+  if (! file)
+    return NULL;
+
+  /* Read file's size, allocate space, and read the file's contents */
+  size = grub_file_size(file);
+  if (size != UNTRUSTED_VAR_FILE_SIZE)
+    {
+      grub_dprintf ("crypt", "ERROR: size %d != %d\n", size,
+                    UNTRUSTED_VAR_FILE_SIZE);
+      return NULL;
+    }
+
+  buf = grub_malloc (size);
+  if (! buf)
+    return NULL;
+
+  grub_dprintf ("crypt", "malloc succeeded\n");
+
+  offset = 0;
+  while (size > 0)
+    {
+      grub_ssize_t ret = grub_file_read (file, buf + offset, size);
+      if (ret == 0)
+        break;
+      else if (ret < 0)
+        {
+          grub_free (buf);
+          return NULL;
+        }
+
+      size -= ret;
+      offset += ret;
+    }
+  grub_dprintf ("crypt", "successfully read %llu bytes from file!\n", offset);
+
+  if (offset != UNTRUSTED_VAR_FILE_SIZE)
+    {
+      grub_dprintf ("crypt", "ERROR read too few bytes (%llu instead of %d) from "
+                    "file!\n", offset, UNTRUSTED_VAR_FILE_SIZE);
+      grub_free (buf);
+      return NULL;
+    }
+
+  return buf;
+}
+
+/* NULL-terminates buf by changing all characters after the first run of 0 or
+   more characters in the set [A-Za-z0-9_].  Assumes buf points to at least
+   UNTRUSTED_VAR_FILE_SIZE bytes. */
+static void
+sanitize_untrusted_string (char buf[UNTRUSTED_VAR_FILE_SIZE])
+{
+  grub_size_t i;
+
+  /* First, NULL-terminate the input for safety */
+  buf[UNTRUSTED_VAR_FILE_SIZE - 1] = '\0';
+  /* Now nullify all characters not in [A-Za-z0-9_] */
+  for (i = 0; i < UNTRUSTED_VAR_FILE_SIZE; i++)
+    if (!is_AZaz09_(buf[i]))
+        buf[i] = '\0';
+}
+
+/* Now that we have read the untrusted data, assign it to the requested
+   environment variable.  It is *not* an error for the data to be a string of
+   length 0 (i.e., for buf[0] == '\0'), it just results in an empty
+   environment variable. */
+static grub_err_t
+grub_cmd_load_env_untrusted (grub_command_t cmd  __attribute__ ((unused)),
+                             int argc, char **argv)
+{
+  grub_file_t file;
+  grub_err_t err;
+  char *buf;
+  unsigned int i;
+
+  grub_dprintf ("crypt", "load_env_untrusted\n");
+
+  if (argc != 2)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("two arguments expected"));
+
+  for (i = 0; argv[0][i] != '\0'; i++)
+    if (! is_AZaz09_ (argv[0][i]))
+      return grub_error (GRUB_ERR_BAD_ARGUMENT,
+                         N_("VARIABLE_NAME must be in set [A-Za-z0-9_]"));
+
+  grub_dprintf ("crypt", "variable_name: %s\n", argv[0]);
+  grub_dprintf ("crypt", "file: %s\n", argv[1]);
+
+  file = open_file_untrusted(argv[1]);
+  if (! file)
+    {
+      grub_dprintf ("crypt", "ERROR: couldn't open file: %s\n", argv[1]);
+      return grub_errno;
+    }
+
+  buf = read_file_untrusted(file);
+  grub_file_close (file);
+
+  if (NULL == buf)
+    return GRUB_ERR_FILE_READ_ERROR;
+
+  sanitize_untrusted_string(buf);
+
+  err = grub_env_set(argv[0], buf);
+
+  if (buf)
+    grub_free (buf);
+  return err;
+}
+
+/* Overwrites the contents of an existing 1024-byte file with the first run of
+   bytes from the specified environment variable that comply with [A-Za-z0-9_].
+   Two arguments: the name of the environment variable, and the name of the file
+   into which that variable's contents should be written.  Extra space in the
+   file is padded out with a single '\n' followed by '#' characters. */
+static grub_err_t
+grub_cmd_save_env_untrusted (grub_command_t cmd  __attribute__ ((unused)),
+                             int argc, char **argv)
+{
+  grub_file_t file;
+  const char* var_contents;
+  grub_size_t var_contents_len;
+  char* old_file_contents;
+  grub_err_t err;
+  char *new_file_contents;
+  struct grub_cmd_save_env_ctx ctx = {
+    .head = 0,
+    .tail = 0
+  };
+  grub_disk_t disk;
+  grub_disk_addr_t part_start;
+  struct blocklist *p;
+  grub_size_t index;
+
+  grub_dprintf ("crypt", "save_env_untrusted\n");
+
+  if (argc != 2)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("two arguments expected"));
+
+  for (index = 0; argv[0][index] != '\0'; index++)
+    if (! is_AZaz09_ (argv[0][index]))
+      return grub_error (GRUB_ERR_BAD_ARGUMENT,
+                         N_("VARIABLE_NAME must be in set [A-Za-z0-9_]"));
+
+  var_contents = grub_env_get(argv[0]);
+  if (NULL == var_contents)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("no such variable"));
+  /* This ensures var_contents is NULL-terminated for processing below */
+  var_contents_len = grub_strlen (var_contents);  /* TODO: get strnlen */
+  if (var_contents_len >= UNTRUSTED_VAR_FILE_SIZE)
+    return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("variable value too long"));
+
+  /* Make sure the variable only contains characters in [A-Za-z0-9_] */
+  for (index = 0; index < var_contents_len; index++)
+    if (!is_AZaz09_ (var_contents[index]))
+      return grub_error (GRUB_ERR_OUT_OF_RANGE,
+                         N_("illegal characters in variable"));
+
+  file = open_file_untrusted(argv[1]);
+  if (! file)
+    return grub_errno;
+
+  grub_dprintf ("crypt", "save_env_untrusted opened file %s for reading\n",
+                argv[1]);
+
+  if (! file->device->disk)
+    {
+      grub_file_close (file);
+      return grub_error (GRUB_ERR_BAD_DEVICE, "disk device required");
+    }
+
+  file->read_hook = save_env_read_hook;
+  file->read_hook_data = &ctx;
+  old_file_contents = read_file_untrusted(file);
+  file->read_hook = 0;
+  if (NULL == old_file_contents)
+    {
+      err = grub_error (GRUB_ERR_FILE_READ_ERROR,
+                        N_("Unable to read existing file blocks"));
+      goto fail;
+    }
+
+  grub_dprintf ("crypt", "read file, blocklist is built.\n");
+
+  err = check_blocklists (old_file_contents, ctx.head, file);
+  if (GRUB_ERR_NONE != err)
+    goto fail;
+
+  grub_dprintf ("crypt", "blocklist checked successfully.\n");
+
+  new_file_contents = grub_malloc (UNTRUSTED_VAR_FILE_SIZE);
+  if (NULL == new_file_contents)
+    {
+      err = grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("malloc failed"));
+      goto fail;
+    }
+
+  grub_memcpy(new_file_contents, old_file_contents, UNTRUSTED_VAR_FILE_SIZE);
+  grub_strncpy(new_file_contents, var_contents, UNTRUSTED_VAR_FILE_SIZE);
+  new_file_contents[var_contents_len] = '\n';
+  for (index = var_contents_len + 1; index < UNTRUSTED_VAR_FILE_SIZE; index++)
+    new_file_contents[index] = '#';  /* backfill file with '#' character */
+
+  grub_dprintf ("crypt", "ready to write: ---\n%s\n---\n", new_file_contents);
+
+  /* TODO: Refactor write_blocklists() so we can just call it here instead. */
+  disk = file->device->disk;
+  part_start = grub_partition_get_start (disk->partition);
+
+  index = 0;
+  for (p = ctx.head; p; index += p->length, p = p->next)
+    {
+      err = grub_disk_write (disk, p->sector - part_start,
+                             p->offset, p->length, new_file_contents + index);
+      if (GRUB_ERR_NONE != err)
+        goto fail;
+    }
+
+fail:
+  if (old_file_contents)
+    grub_free (old_file_contents);
+  if (new_file_contents)
+    grub_free (new_file_contents);
+  free_blocklists (ctx.head);
+  grub_file_close (file);
+  return err;
+}
+
 static grub_extcmd_t cmd_load, cmd_list, cmd_save;
+static grub_command_t cmd_load_env_untrusted, cmd_save_env_untrusted;
 
 GRUB_MOD_INIT(loadenv)
 {
@@ -396,6 +670,14 @@ GRUB_MOD_INIT(loadenv)
 			  N_("[-f FILE] variable_name [...]"),
 			  N_("Save variables to environment block file."),
 			  options);
+  cmd_load_env_untrusted =
+    grub_register_command ("load_env_untrusted", grub_cmd_load_env_untrusted,
+              N_("VARIABLE_NAME FILE"),
+              N_("Assign the contents of FILE to VARIABLE_NAME"));
+  cmd_save_env_untrusted =
+    grub_register_command ("save_env_untrusted", grub_cmd_save_env_untrusted,
+              N_("VARIABLE_NAME FILE"),
+              N_("Write the contents of VARIABLE_NAME into pre-existing FILE"));
 }
 
 GRUB_MOD_FINI(loadenv)
@@ -403,4 +685,6 @@ GRUB_MOD_FINI(loadenv)
   grub_unregister_extcmd (cmd_load);
   grub_unregister_extcmd (cmd_list);
   grub_unregister_extcmd (cmd_save);
+  grub_unregister_command (cmd_load_env_untrusted);
+  grub_unregister_command (cmd_save_env_untrusted);
 }
-- 
1.8.3