* [PATCH v1 1/2] make check_blocklists() not depend on an envblk data type
2013-08-26 19:29 [PATCH v1 0/2] New commands: load_env_untrusted, save_env_untrusted Jon McCune
@ 2013-08-26 19:29 ` Jon McCune
2013-08-26 19:29 ` [PATCH v1 2/2] introduce new commands load_env_untrusted and save_env_untrusted: Jon McCune
1 sibling, 0 replies; 3+ messages in thread
From: Jon McCune @ 2013-08-26 19:29 UTC (permalink / raw)
To: grub-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH v1 2/2] introduce new commands load_env_untrusted and save_env_untrusted:
2013-08-26 19:29 [PATCH v1 0/2] New commands: load_env_untrusted, save_env_untrusted Jon McCune
2013-08-26 19:29 ` [PATCH v1 1/2] make check_blocklists() not depend on an envblk data type Jon McCune
@ 2013-08-26 19:29 ` Jon McCune
1 sibling, 0 replies; 3+ messages in thread
From: Jon McCune @ 2013-08-26 19:29 UTC (permalink / raw)
To: grub-devel; +Cc: 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
^ permalink raw reply related [flat|nested] 3+ messages in thread