New commands: load_env_untrusted, save_env_untrusted
cover New commands: load_env_untrusted, save_env_untrusted
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(-)
Signature · 1 line
1.8.3
PATCH 1/2 make check_blocklists() not depend on an envblk data type
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(-)
grub-core/commands/loadenv.c +3 -5
@@ -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)
Signature · 1 line
1.8.3
PATCH 2/2 introduce new commands load_env_untrusted and save_env_untrusted:
In reply to Jon McCuneThese commands will load / store a single named environment variable from / to a 1024-byte file, regardless of the value of check_signatures.
--- grub-core/commands/loadenv.c | 284 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 284 insertions(+)
grub-core/commands/loadenv.c +284 -0
@@ -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); }
Signature · 1 line
1.8.3