From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1VHylW-000622-LG for mharc-grub-devel@gnu.org; Fri, 06 Sep 2013 12:20:10 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51562) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VHylS-0005wn-7X for grub-devel@gnu.org; Fri, 06 Sep 2013 12:20:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VHylM-00035v-UL for grub-devel@gnu.org; Fri, 06 Sep 2013 12:20:06 -0400 Received: from mail-ve0-x249.google.com ([2607:f8b0:400c:c01::249]:55565) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VHylM-00035q-OT for grub-devel@gnu.org; Fri, 06 Sep 2013 12:20:00 -0400 Received: by mail-ve0-f201.google.com with SMTP id c14so248023vea.2 for ; Fri, 06 Sep 2013 09:20:00 -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:in-reply-to:references; bh=32xMVCEjUJXH0txmmE4bymjpXQDhXbHPolyrw2nYVsM=; b=W5m9Y2kVhJ6RRev02eSnVbQSD4Cb/0qKR7g7fMcDO6sKYwpQgOPrOsYSuv0kfm9dnO q0vknwGLi5NKgb4sQblQsqHoN1SjLvvBKT/uxjEx6b7/ItPUNjakzgfUoo/EnhcAeLTq uKbpIAoN0+AuM4mOabOjL2RLDB+C3Fwb/GklqOtJ/FVyhGifEb5G8WeFLLw++8YDa4Kp 7LOalsW7mu79+PiTnQyqYyrmzavvjhEZ5ygeWmSrBeVuiJ7HuBDs51SPfa7BiMKUbW/O 9dSH9Edu+9aLDFrCAregRkvrd8H8s/9IUnipI5pM0y8zWlMyiCYnaAm9pCEHoMX12rAS MGug== 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:in-reply-to :references; bh=32xMVCEjUJXH0txmmE4bymjpXQDhXbHPolyrw2nYVsM=; b=eV7ciaMA/iltxjmvDgZlrQN8NouESQc5hQvcJmPTLfPMxIh3GPYn+mQrLMUEKUt5L8 Ckeoq3CssWS2q6LJk3Gk3IShATzMRDQLKtfH80sHhP6/SubW9mmbUnVcPh6Q/zMGZNgd LOE/4TIM1T3vXBIfzrKbxLufh5WYOVvNgp2pbilF1qjniD4YdPmajg4f7qK6+YVZeEdU cvDM111rjqDwpsnD3JbDkHLr/z4DpQGGfvYHWGMf3HyOdyrPjK8k7qwLwXkqmzuwB902 IqutsDbo5+JHKV23VjOePMo1ziER2yJh2jyL6iG7I7xPe44ROCodyuhXfEUCXmye2bqh xKkQ== X-Gm-Message-State: ALoCoQlWhlBhI2aWGEXvrTjjHWQyhuiFloz2IEL6i7C707gvU580B6iMIlCKktxeQh0zf4d2mSzj52/WHcLOhubOrh/RhLiSfpJy4b/xFMIXS4lMMXLiSDblvMwAM9s3HMV25zPgmadoBj8WBFR3J1x6u2yq3BChw4XnM68GmYNHkVU8JcJmFjwBxJggqKNyAwlSEiTRmlrBkK+ESzc5+PrfcFBjDlbtXJgqPes6Ak1r6qJioeg3Yr0= X-Received: by 10.236.172.34 with SMTP id s22mr1072281yhl.25.1378484400428; Fri, 06 Sep 2013 09:20:00 -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 s21si163909yhc.1.1969.12.31.16.00.00 (version=TLSv1.1 cipher=AES128-SHA bits=128/128); Fri, 06 Sep 2013 09:20:00 -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 3A0DD31C1DC; Fri, 6 Sep 2013 09:20:00 -0700 (PDT) Received: by yinz.mtv.corp.google.com (Postfix, from userid 184367) id EE907C076E; Fri, 6 Sep 2013 09:19:59 -0700 (PDT) From: Jon McCune To: grub-devel@gnu.org Subject: [PATCH v2 2/5] load_env support for whitelisting which variables are read from an env file, even if check_signatures=enforce Date: Fri, 6 Sep 2013 09:18:50 -0700 Message-Id: <1378484333-13577-3-git-send-email-jonmccune@google.com> X-Mailer: git-send-email 1.8.4 In-Reply-To: <1378484333-13577-1-git-send-email-jonmccune@google.com> References: <1378484333-13577-1-git-send-email-jonmccune@google.com> 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: Fri, 06 Sep 2013 16:20:08 -0000 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 --- 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