* [PATCH v3 0/3] Add whitelisting support for load_env
@ 2013-09-06 23:59 Jon McCune
2013-09-06 23:59 ` [PATCH v3 1/3] style: indent --no-tabs --gnu-style grub-core/commands/loadenv.c Jon McCune
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Jon McCune @ 2013-09-06 23:59 UTC (permalink / raw)
To: grub-devel; +Cc: Jon McCune
This is an iteration of the code that was discussed in the v2 patch.
I have not yet updated the documentation to match these changes, since
I would prefer to conclude discussion of the suitability of these
changes first.
Jon McCune (3):
style: indent --no-tabs --gnu-style grub-core/commands/loadenv.c
load_env support for whitelisting which variables are read
Add (multiple) -k, --pubkey=FILE support to grub-install command
grub-core/commands/loadenv.c | 140 +++++++++++++++++++++++++++++--------------
util/grub-install.in | 19 ++++--
util/grub-install_header | 6 ++
3 files changed, 115 insertions(+), 50 deletions(-)
--
1.8.4
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3 1/3] style: indent --no-tabs --gnu-style grub-core/commands/loadenv.c
2013-09-06 23:59 [PATCH v3 0/3] Add whitelisting support for load_env Jon McCune
@ 2013-09-06 23:59 ` Jon McCune
2013-09-06 23:59 ` [PATCH v3 2/3] load_env support for whitelisting which variables are read from an env file Jon McCune
2013-09-06 23:59 ` [PATCH v3 3/3] Add (multiple) -k, --pubkey=FILE support to grub-install command Jon McCune
2 siblings, 0 replies; 5+ messages in thread
From: Jon McCune @ 2013-09-06 23:59 UTC (permalink / raw)
To: grub-devel; +Cc: Jon McCune
Added the option --no-tabs after observing that 'indent' did not
result in consistent tab usage.
An example of the inconsistent tab usage: the very first function
in loadenv.c is open_envblk_file(), and it contains a line
indented exclusively with tabs, and lines indented exclusively
with spaces. The space-idented lines use two spaces for each
level of indentation. In order for the tab-indented lines to
format properly, the tabs must consume the equivalent of 8
spaces (i.e., four levels of indentation). To my knowledge, this
is not consistent with the default GNU style.
Signed-off-by: Jon McCune <jonmccune@google.com>
---
grub-core/commands/loadenv.c | 87 ++++++++++++++++++++++----------------------
1 file changed, 44 insertions(+), 43 deletions(-)
diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.c
index c0a42c5..a431499 100644
--- a/grub-core/commands/loadenv.c
+++ b/grub-core/commands/loadenv.c
@@ -30,20 +30,19 @@
GRUB_MOD_LICENSE ("GPLv3+");
-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},
- {0, 0, 0, 0, 0, 0}
- };
+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},
+ {0, 0, 0, 0, 0, 0}
+};
static grub_file_t
open_envblk_file (char *filename)
{
grub_file_t file;
- if (! filename)
+ if (!filename)
{
const char *prefix;
@@ -54,20 +53,21 @@ open_envblk_file (char *filename)
len = grub_strlen (prefix);
filename = grub_malloc (len + 1 + sizeof (GRUB_ENVBLK_DEFCFG));
- if (! filename)
+ if (!filename)
return 0;
grub_strcpy (filename, prefix);
filename[len] = '/';
grub_strcpy (filename + len + 1, GRUB_ENVBLK_DEFCFG);
- grub_file_filter_disable_compression ();
+ grub_file_filter_disable_compression ();
file = grub_file_open (filename);
grub_free (filename);
return file;
}
else
{
- grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("variable `%s' isn't set"), "prefix");
+ grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("variable `%s' isn't set"),
+ "prefix");
return 0;
}
}
@@ -85,7 +85,7 @@ read_envblk_file (grub_file_t file)
grub_envblk_t envblk;
buf = grub_malloc (size);
- if (! buf)
+ if (!buf)
return 0;
while (size > 0)
@@ -104,7 +104,7 @@ read_envblk_file (grub_file_t file)
}
envblk = grub_envblk_open (buf, offset);
- if (! envblk)
+ if (!envblk)
{
grub_free (buf);
grub_error (GRUB_ERR_BAD_FILE_TYPE, "invalid environment block");
@@ -124,25 +124,25 @@ set_var (const char *name, const char *value)
static grub_err_t
grub_cmd_load_env (grub_extcmd_context_t ctxt,
- int argc __attribute__ ((unused)),
- char **args __attribute__ ((unused)))
+ int argc __attribute__ ((unused)),
+ char **args __attribute__ ((unused)))
{
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);
- if (! file)
+ if (!file)
return grub_errno;
envblk = read_envblk_file (file);
- if (! envblk)
+ if (!envblk)
goto fail;
grub_envblk_iterate (envblk, set_var);
grub_envblk_close (envblk);
- fail:
+fail:
grub_file_close (file);
return grub_errno;
}
@@ -157,25 +157,25 @@ print_var (const char *name, const char *value)
static grub_err_t
grub_cmd_list_env (grub_extcmd_context_t ctxt,
- int argc __attribute__ ((unused)),
- char **args __attribute__ ((unused)))
+ int argc __attribute__ ((unused)),
+ char **args __attribute__ ((unused)))
{
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);
- if (! file)
+ if (!file)
return grub_errno;
envblk = read_envblk_file (file);
- if (! envblk)
+ if (!envblk)
goto fail;
grub_envblk_iterate (envblk, print_var);
grub_envblk_close (envblk);
- fail:
+fail:
grub_file_close (file);
return grub_errno;
}
@@ -253,7 +253,7 @@ check_blocklists (grub_envblk_t envblk, struct blocklist *blocklists,
return grub_errno;
if (grub_memcmp (buf + index, blockbuf, p->length) != 0)
- return grub_error (GRUB_ERR_FILE_READ_ERROR, "invalid blocklist");
+ return grub_error (GRUB_ERR_FILE_READ_ERROR, "invalid blocklist");
}
return GRUB_ERR_NONE;
@@ -293,7 +293,7 @@ struct grub_cmd_save_env_ctx
/* Store blocklists in a linked list. */
static void
save_env_read_hook (grub_disk_addr_t sector, unsigned offset, unsigned length,
- void *data)
+ void *data)
{
struct grub_cmd_save_env_ctx *ctx = data;
struct blocklist *block;
@@ -303,7 +303,7 @@ save_env_read_hook (grub_disk_addr_t sector, unsigned offset, unsigned length,
return;
block = grub_malloc (sizeof (*block));
- if (! block)
+ if (!block)
return;
block->sector = sector;
@@ -315,7 +315,7 @@ save_env_read_hook (grub_disk_addr_t sector, unsigned offset, unsigned length,
if (ctx->tail)
ctx->tail->next = block;
ctx->tail = block;
- if (! ctx->head)
+ if (!ctx->head)
ctx->head = block;
}
@@ -330,14 +330,14 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
.tail = 0
};
- if (! argc)
+ if (!argc)
return grub_error (GRUB_ERR_BAD_ARGUMENT, "no variable is specified");
file = open_envblk_file ((state[0].set) ? state[0].arg : 0);
- if (! file)
+ if (!file)
return grub_errno;
- if (! file->device->disk)
+ if (!file->device->disk)
{
grub_file_close (file);
return grub_error (GRUB_ERR_BAD_DEVICE, "disk device required");
@@ -347,7 +347,7 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
file->read_hook_data = &ctx;
envblk = read_envblk_file (file);
file->read_hook = 0;
- if (! envblk)
+ if (!envblk)
goto fail;
if (check_blocklists (envblk, ctx.head, file))
@@ -360,9 +360,10 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
value = grub_env_get (args[0]);
if (value)
{
- if (! grub_envblk_set (envblk, args[0], value))
+ if (!grub_envblk_set (envblk, args[0], value))
{
- grub_error (GRUB_ERR_BAD_ARGUMENT, "environment block too small");
+ grub_error (GRUB_ERR_BAD_ARGUMENT,
+ "environment block too small");
goto fail;
}
}
@@ -373,7 +374,7 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
write_blocklists (envblk, ctx.head, file);
- fail:
+fail:
if (envblk)
grub_envblk_close (envblk);
free_blocklists (ctx.head);
@@ -383,24 +384,24 @@ grub_cmd_save_env (grub_extcmd_context_t ctxt, int argc, char **args)
static grub_extcmd_t cmd_load, cmd_list, cmd_save;
-GRUB_MOD_INIT(loadenv)
+GRUB_MOD_INIT (loadenv)
{
cmd_load =
grub_register_extcmd ("load_env", grub_cmd_load_env, 0, N_("[-f FILE]"),
- N_("Load variables from environment block file."),
- options);
+ N_("Load variables from environment block file."),
+ options);
cmd_list =
grub_register_extcmd ("list_env", grub_cmd_list_env, 0, N_("[-f FILE]"),
- N_("List variables from environment block file."),
- options);
+ N_("List variables from environment block file."),
+ options);
cmd_save =
grub_register_extcmd ("save_env", grub_cmd_save_env, 0,
- N_("[-f FILE] variable_name [...]"),
- N_("Save variables to environment block file."),
- options);
+ N_("[-f FILE] variable_name [...]"),
+ N_("Save variables to environment block file."),
+ options);
}
-GRUB_MOD_FINI(loadenv)
+GRUB_MOD_FINI (loadenv)
{
grub_unregister_extcmd (cmd_load);
grub_unregister_extcmd (cmd_list);
--
1.8.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v3 2/3] load_env support for whitelisting which variables are read from an env file
2013-09-06 23:59 [PATCH v3 0/3] Add whitelisting support for load_env Jon McCune
2013-09-06 23:59 ` [PATCH v3 1/3] style: indent --no-tabs --gnu-style grub-core/commands/loadenv.c Jon McCune
@ 2013-09-06 23:59 ` Jon McCune
2013-09-06 23:59 ` [PATCH v3 3/3] Add (multiple) -k, --pubkey=FILE support to grub-install command Jon McCune
2 siblings, 0 replies; 5+ messages in thread
From: Jon McCune @ 2013-09-06 23:59 UTC (permalink / raw)
To: grub-devel; +Cc: Jon McCune
This gives load_env support for naming variables that is the same as save_env.
This is useful to prevent potentially unwanted variables within a grubenv file
from impacting the behavior of a configuration, especially if using
check_signatures=enforce but wishing to retain support for savedefault,
boot_once, and similar.
This version of this patch drops all direct interaction with the logic in
verify.c. The author of grub.cfg must take care to disable and re-enable
check_signatures as appropriate.
Signed-off-by: Jon McCune <jonmccune@google.com>
---
grub-core/commands/loadenv.c | 57 ++++++++++++++++++++++++++++++++++++++++----
1 file changed, 52 insertions(+), 5 deletions(-)
diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.c
index a431499..5828f5f 100644
--- a/grub-core/commands/loadenv.c
+++ b/grub-core/commands/loadenv.c
@@ -122,15 +122,59 @@ 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;
+ /* argc > 0 indicates caller provided a whitelist of variables to read */
+ if (argc > 0)
+ {
+ whitelist_len = (argc >= 0) ? argc : 0;
+ whitelist_vars = args;
+ }
file = open_envblk_file ((state[0].set) ? state[0].arg : 0);
if (!file)
return grub_errno;
@@ -139,10 +183,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 +433,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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v3 3/3] Add (multiple) -k, --pubkey=FILE support to grub-install command
2013-09-06 23:59 [PATCH v3 0/3] Add whitelisting support for load_env Jon McCune
2013-09-06 23:59 ` [PATCH v3 1/3] style: indent --no-tabs --gnu-style grub-core/commands/loadenv.c Jon McCune
2013-09-06 23:59 ` [PATCH v3 2/3] load_env support for whitelisting which variables are read from an env file Jon McCune
@ 2013-09-06 23:59 ` Jon McCune
2013-09-19 7:23 ` Vladimir 'φ-coder/phcoder' Serbinenko
2 siblings, 1 reply; 5+ messages in thread
From: Jon McCune @ 2013-09-06 23:59 UTC (permalink / raw)
To: grub-devel; +Cc: Jon McCune
Passes along one or more public keys to the grub-mkimage invocation
Signed-off-by: Jon McCune <jonmccune@google.com>
---
util/grub-install.in | 19 +++++++++++++++----
| 6 ++++++
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/util/grub-install.in b/util/grub-install.in
index 1816bb1..034cf10 100644
--- a/util/grub-install.in
+++ b/util/grub-install.in
@@ -650,10 +650,21 @@ case "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" in
*) imgext=img ;;
esac
+pubkey_file_arg=""
+if [ x"$pubkey_file_list" != x ]; then
+ for file in $pubkey_file_list; do
+ if [ ! -e "$file" ]; then
+ gettext_printf "Public key file %s not found.\n" "${file}" 1>&2
+ exit 1
+ fi
+ pubkey_file_arg="$pubkey_file_arg --pubkey=$file"
+ done
+fi
+
if [ x"$config_opt_file" = x ]; then
- "$grub_mkimage" -d "${source_directory}" -O "${mkimage_target}" --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/core.${imgext}" --prefix="${prefix_drive}${relative_grubdir}" $grub_decompression_module $modules || exit 1
+ "$grub_mkimage" $pubkey_file_arg -d "${source_directory}" -O "${mkimage_target}" --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/core.${imgext}" --prefix="${prefix_drive}${relative_grubdir}" $grub_decompression_module $modules || exit 1
else
- "$grub_mkimage" -c "${config_opt_file}" -d "${source_directory}" -O "${mkimage_target}" --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/core.${imgext}" --prefix="${prefix_drive}${relative_grubdir}" $grub_decompression_module $modules || exit 1
+ "$grub_mkimage" -c "${config_opt_file}" $pubkey_file_arg -d "${source_directory}" -O "${mkimage_target}" --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/core.${imgext}" --prefix="${prefix_drive}${relative_grubdir}" $grub_decompression_module $modules || exit 1
fi
# Backward-compatibility kludges
@@ -664,9 +675,9 @@ elif [ "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" = "i386-ieee1275" ]
elif [ "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" = "i386-efi" ] || [ "${grub_modinfo_target_cpu}-${grub_modinfo_platform}" = "x86_64-efi" ]; then
if [ x"$config_opt_file" = x ]; then
- "$grub_mkimage" -d "${source_directory}" -O "${mkimage_target}" --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/grub.efi" --prefix="" $grub_decompression_module $modules || exit 1
+ "$grub_mkimage" $pubkey_file_arg -d "${source_directory}" -O "${mkimage_target}" --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/grub.efi" --prefix="" $grub_decompression_module $modules || exit 1
else
- "$grub_mkimage" -c "${config_opt_file}" -d "${source_directory}" -O "${mkimage_target}" --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/grub.efi" --prefix="" $grub_decompression_module $modules || exit 1
+ "$grub_mkimage" -c "${config_opt_file}" $pubkey_file_arg -d "${source_directory}" -O "${mkimage_target}" --output="${grubdir}/${grub_modinfo_target_cpu}-$grub_modinfo_platform/grub.efi" --prefix="" $grub_decompression_module $modules || exit 1
fi
fi
--git a/util/grub-install_header b/util/grub-install_header
index cf7fa9d..5ea27d6 100644
--- a/util/grub-install_header
+++ b/util/grub-install_header
@@ -156,6 +156,7 @@ grub_print_install_files_help () {
dir_msg="$(gettext_printf "use images and modules under DIR [default=%s/<platform>]" "${libdir}/@PACKAGE@")"
print_option_help "-d, --directory=$(gettext "DIR")" "$dir_msg"
print_option_help "--grub-mkimage=$(gettext "FILE")" "$(gettext "use FILE as grub-mkimage")"
+ print_option_help "-k, --pubkey=$(gettext "FILE")" "$(gettext "embed FILE as public key for signature checking")"
print_option_help "-v, --version" "$(gettext "print the version information and exit")"
}
@@ -168,6 +169,7 @@ grub_decompression_module=""
compressor=""
compressor_opts=""
source_directory=""
+pubkey_file_list=""
argument () {
opt=$1
@@ -245,6 +247,10 @@ grub_process_install_options () {
grub_mkimage=`argument $option "$@"`; grub_process_install_options_consumed=2 ;;
--grub-mkimage=*)
grub_mkimage=`echo "$option" | sed 's/--grub-mkimage=//'`;grub_process_install_options_consumed=1 ;;
+ --pubkey | -k)
+ pubkey_file_list=`echo -n "$pubkey_file_list "; argument $option "$@"`; grub_process_install_options_consumed=2;;
+ --pubkey=*)
+ pubkey_file_list=`echo -n "$pubkey_file_list "; echo "$option" | sed 's/--pubkey=//'` grub_process_install_options_consumed=1;;
--modules)
modules=`argument $option "$@"`; grub_process_install_options_consumed=2;;
--modules=*)
--
1.8.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3 3/3] Add (multiple) -k, --pubkey=FILE support to grub-install command
2013-09-06 23:59 ` [PATCH v3 3/3] Add (multiple) -k, --pubkey=FILE support to grub-install command Jon McCune
@ 2013-09-19 7:23 ` Vladimir 'φ-coder/phcoder' Serbinenko
0 siblings, 0 replies; 5+ messages in thread
From: Vladimir 'φ-coder/phcoder' Serbinenko @ 2013-09-19 7:23 UTC (permalink / raw)
To: The development of GNU GRUB
[-- Attachment #1: Type: text/plain, Size: 511 bytes --]
Please update other install methods as well
On 07.09.2013 01:59, Jon McCune wrote:
> +pubkey_file_arg=""
> +if [ x"$pubkey_file_list" != x ]; then
> + for file in $pubkey_file_list; do
> + if [ ! -e "$file" ]; then
> + gettext_printf "Public key file %s not found.\n" "${file}" 1>&2
> + exit 1
> + fi
> + pubkey_file_arg="$pubkey_file_arg --pubkey=$file"
> + done
> +fi
> +
This would go to grub-install_header. Also this code doesn't handle
spaces properly.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 291 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-09-19 7:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-06 23:59 [PATCH v3 0/3] Add whitelisting support for load_env Jon McCune
2013-09-06 23:59 ` [PATCH v3 1/3] style: indent --no-tabs --gnu-style grub-core/commands/loadenv.c Jon McCune
2013-09-06 23:59 ` [PATCH v3 2/3] load_env support for whitelisting which variables are read from an env file Jon McCune
2013-09-06 23:59 ` [PATCH v3 3/3] Add (multiple) -k, --pubkey=FILE support to grub-install command Jon McCune
2013-09-19 7:23 ` Vladimir 'φ-coder/phcoder' Serbinenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).