From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1VOyxb-00026I-KK for mharc-grub-devel@gnu.org; Wed, 25 Sep 2013 19:57:35 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42508) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VOyxX-00025d-CC for grub-devel@gnu.org; Wed, 25 Sep 2013 19:57:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VOyxV-0005lL-Us for grub-devel@gnu.org; Wed, 25 Sep 2013 19:57:31 -0400 Received: from mail-ve0-x249.google.com ([2607:f8b0:400c:c01::249]:40574) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VOyxV-0005lH-Oe for grub-devel@gnu.org; Wed, 25 Sep 2013 19:57:29 -0400 Received: by mail-ve0-f201.google.com with SMTP id c14so54984vea.4 for ; Wed, 25 Sep 2013 16:57:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=from:to:cc:subject:date:message-id; bh=ExjpkhODa+J262MuQzxKzkw0GcX/X3v93yVNRVMfRZM=; b=C2bWbw5hihQq2yYtnAaBAA/B2u7guOpaqFdLwDhBnFAGRaEL4/TVFqTPBU3rBCRBHA Qtxhh1SGtN1cJzSS/bNtSEBYrVw8peUjPF1jUd1eysgXGaTa15f4Tn8vHQZRI7dyGdJl KSw8GmIwbvcJppALpt1Di69+XxZpLVxoz4SSQ58ESGCEoRjhFvcFIGxwOjGey2PFjQH2 IkamnFpr/jSU1nh/Wm/pWvv+w84vdiqixUVoa5uPZAUnqWyPk4ZNq92+dAIJnf6Nsh61 bAKx1gBrX3vXQ+vnYQPc8esXGTBRdAMO0pEIzxjZFakzvOKfcMzHjyXxHQwLdx/6ECOX Gpkw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=ExjpkhODa+J262MuQzxKzkw0GcX/X3v93yVNRVMfRZM=; b=RsMFzjrWya/PDGXDEscnYZ125jC1LQPJ1SzdFpcSjH02VsT/t5P28iQJ6SXA8TbtYg yPDWEqquQVUCLLq8G1DtlehbDouA1PHkvmlBPvNQvzip5VCdqayWhv0jKsM5Npqce92E 7FhsnCNKAkSYgfW3mi1/mDlJ1g1yqQG0rOJgi+znYYTbTi70OP1gayBYGNda6Tf+T18G /DrWeK6qg7JS8HoS8c9fGb5DMZ04UzI5V0ncZD8BLMPfVp5/HnfCHdowKz3tnN0qTvNw jMN4FsSLC2nqNBUL8VbNuQpIQ+Vu80Lspr03QLzOuyUP6bqA7ajF6pM0bzLQLHYw/8x1 yQMA== X-Gm-Message-State: ALoCoQmk40IvzhnRqYrbxCQK7CH36JrxmPM3ONf7AzGDO69T/Vgc9lEUdy8UyYT91vgfUcceIqrk+S8xbaR3toeej+opbM9R9KJmo/qnvOmSBY/dI2a8HAEUK5bMJbemE+GtWYnzysyelIpM50wqReG7+IQxWZO/0kxHQAk9KDIzZtcaFLkGR9M0NQbFTxoEhZdAf5dGbj3wPDSx47kxFbPPsLSahsF5AcKdMA4jahW1NTRptV/fMXQ= X-Received: by 10.236.174.232 with SMTP id x68mr10105778yhl.42.1380153449048; Wed, 25 Sep 2013 16:57:29 -0700 (PDT) Received: from corp2gmr1-1.hot.corp.google.com (corp2gmr1-1.hot.corp.google.com [172.24.189.92]) by gmr-mx.google.com with ESMTPS id t42si6248949yhm.3.1969.12.31.16.00.00 (version=TLSv1.1 cipher=AES128-SHA bits=128/128); Wed, 25 Sep 2013 16:57:29 -0700 (PDT) Received: from yinz.mtv.corp.google.com (yinz.mtv.corp.google.com [172.17.81.122]) by corp2gmr1-1.hot.corp.google.com (Postfix) with ESMTP id D0D3C31C266; Wed, 25 Sep 2013 16:57:28 -0700 (PDT) Received: by yinz.mtv.corp.google.com (Postfix, from userid 184367) id 783A0C09E5; Wed, 25 Sep 2013 16:57:28 -0700 (PDT) From: Jon McCune To: grub-devel@gnu.org Subject: [PATCH v5] load_env support for whitelisting, save_env support for check_signatures Date: Wed, 25 Sep 2013 16:57:16 -0700 Message-Id: <1380153436-22432-1-git-send-email-jonmccune@google.com> X-Mailer: git-send-email 1.8.4 X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2607:f8b0:400c:c01::249 Cc: Jon McCune X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Sep 2013 23:57:33 -0000 Whitelisting which variables are read from an env file 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. 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 Changes in this version of the patch: Drop whitespace changes. Drop grub-install script changes. Generalized hook support in grub_envblk_iterate(). Whitelist-specific hook data structures are self-contained in loadenv.c. Add flag {-s, --skip-sig} to load_env command that explicitly controls whether signature-checking is required. Whitelisting and signature checking are thus controlled independently, and the user may now choose to load only whitelisted variables from either of a signed or unsigned grubenv-style file. Add untrusted flag to open_envblk_file(). Replaces the v4 patch's function open_envblk_file_untrusted() with a flag passed to the existing open_envblk_file(). Also restructured open_envblk_file() to make it more readable with the additional logic. open_envblk_file(), when untrusted == 1, disables filters using the compression- and pubkey-specific methods in file.h. The pubkey filter is saved and restored across untrusted file opens. save_env always calls grub_envblk_file() with untrusted set to 1. 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 are not parsed and do not pose a security risk. Signed-off-by: Jon McCune --- grub-core/commands/loadenv.c | 111 ++++++++++++++++++++++++++++++------------- grub-core/lib/envblk.c | 5 +- include/grub/lib/envblk.h | 3 +- util/grub-editenv.c | 5 +- 4 files changed, 87 insertions(+), 37 deletions(-) diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.c index c0a42c5..295a4da 100644 --- a/grub-core/commands/loadenv.c +++ b/grub-core/commands/loadenv.c @@ -35,45 +35,57 @@ 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}, + {"skip-sig", 's', 0, + N_("Skip signature-checking the environment file."), 0, ARG_TYPE_NONE}, {0, 0, 0, 0, 0, 0} }; +/* Opens 'filename' with compression filters disabled. Optionally disables the + PUBKEY filter (that insists upon properly signed files) as well. PUBKEY + filter is restored before the function returns. */ static grub_file_t -open_envblk_file (char *filename) +open_envblk_file (char *filename, int untrusted) { grub_file_t file; + grub_file_filter_t saved_pubkey_filter; + char *buf = 0; if (! filename) { const char *prefix; + int len; prefix = grub_env_get ("prefix"); - if (prefix) - { - int len; - - len = grub_strlen (prefix); - filename = grub_malloc (len + 1 + sizeof (GRUB_ENVBLK_DEFCFG)); - if (! filename) - return 0; - - grub_strcpy (filename, prefix); - filename[len] = '/'; - grub_strcpy (filename + len + 1, GRUB_ENVBLK_DEFCFG); - grub_file_filter_disable_compression (); - file = grub_file_open (filename); - grub_free (filename); - return file; - } - else + if (! prefix) { grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("variable `%s' isn't set"), "prefix"); return 0; } + + len = grub_strlen (prefix); + buf = grub_malloc (len + 1 + sizeof (GRUB_ENVBLK_DEFCFG)); + if (! buf) + return 0; + filename = buf; + + grub_strcpy (filename, prefix); + filename[len] = '/'; + grub_strcpy (filename + len + 1, GRUB_ENVBLK_DEFCFG); } grub_file_filter_disable_compression (); - return grub_file_open (filename); + if (untrusted) + { + saved_pubkey_filter = grub_file_filters_enabled[GRUB_FILE_FILTER_PUBKEY]; + grub_file_filter_disable_pubkey (); + } + file = grub_file_open (filename); + if (untrusted) + grub_file_filter_register (GRUB_FILE_FILTER_PUBKEY, saved_pubkey_filter); + + if (buf) + grub_free (buf); + return file; } static grub_envblk_t @@ -114,24 +126,55 @@ read_envblk_file (grub_file_t file) return envblk; } +struct grub_env_whitelist +{ + grub_size_t len; + char **list; +}; +typedef struct grub_env_whitelist grub_env_whitelist_t; + +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 */ +} + /* Helper for grub_cmd_load_env. */ static int -set_var (const char *name, const char *value) +set_var (const char *name, const char *value, void *whitelist) { - grub_env_set (name, value); + if (! whitelist) + { + grub_env_set (name, value); + return 0; + } + + if (test_whitelist_membership (name, (const grub_env_whitelist_t*)whitelist)) + grub_env_set (name, value); + return 0; } 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; - file = open_envblk_file ((state[0].set) ? state[0].arg : 0); + /* state[0] is the -f flag; state[1] is the --skip-sig flag */ + file = open_envblk_file ((state[0].set) ? state[0].arg : 0, state[1].set); if (! file) return grub_errno; @@ -139,7 +182,8 @@ grub_cmd_load_env (grub_extcmd_context_t ctxt, if (! envblk) goto fail; - grub_envblk_iterate (envblk, set_var); + /* argc > 0 indicates caller provided a whitelist of variables to read. */ + grub_envblk_iterate (envblk, argc > 0 ? &whitelist : 0, set_var); grub_envblk_close (envblk); fail: @@ -149,7 +193,8 @@ grub_cmd_load_env (grub_extcmd_context_t ctxt, /* Print all variables in current context. */ static int -print_var (const char *name, const char *value) +print_var (const char *name, const char *value, + void *hook_data __attribute__ ((unused))) { grub_printf ("%s=%s\n", name, value); return 0; @@ -164,7 +209,7 @@ grub_cmd_list_env (grub_extcmd_context_t ctxt, grub_file_t file; grub_envblk_t envblk; - file = open_envblk_file ((state[0].set) ? state[0].arg : 0); + file = open_envblk_file ((state[0].set) ? state[0].arg : 0, 0); if (! file) return grub_errno; @@ -172,7 +217,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: @@ -333,7 +378,8 @@ 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 ((state[0].set) ? state[0].arg : 0, + 1 /* allow untrusted */); if (! file) return grub_errno; @@ -386,7 +432,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] [-s|--skip-sig] [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..230e0e9 100644 --- a/grub-core/lib/envblk.c +++ b/grub-core/lib/envblk.c @@ -225,7 +225,8 @@ grub_envblk_delete (grub_envblk_t envblk, const char *name) void grub_envblk_iterate (grub_envblk_t envblk, - int hook (const char *name, const char *value)) + void *hook_data, + int hook (const char *name, const char *value, void *hook_data)) { char *p, *pend; @@ -285,7 +286,7 @@ grub_envblk_iterate (grub_envblk_t envblk, } *q = '\0'; - ret = hook (name, value); + ret = hook (name, value, hook_data); grub_free (name); if (ret) return; diff --git a/include/grub/lib/envblk.h b/include/grub/lib/envblk.h index 368ba53..c3e6559 100644 --- a/include/grub/lib/envblk.h +++ b/include/grub/lib/envblk.h @@ -35,7 +35,8 @@ 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, - int hook (const char *name, const char *value)); + void *hook_data, + int hook (const char *name, const char *value, void *hook_data)); void grub_envblk_close (grub_envblk_t envblk); static inline char * diff --git a/util/grub-editenv.c b/util/grub-editenv.c index 9b51acf..591604b 100644 --- a/util/grub-editenv.c +++ b/util/grub-editenv.c @@ -185,7 +185,8 @@ open_envblk_file (const char *name) } static int -print_var (const char *varname, const char *value) +print_var (const char *varname, const char *value, + void *hook_data __attribute__ ((unused))) { printf ("%s=%s\n", varname, value); return 0; @@ -197,7 +198,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