From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1VPLd9-0006HP-OF for mharc-grub-devel@gnu.org; Thu, 26 Sep 2013 20:09:59 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33297) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VPLcz-0006Gw-Dx for grub-devel@gnu.org; Thu, 26 Sep 2013 20:09:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VPLcq-0002UK-Ug for grub-devel@gnu.org; Thu, 26 Sep 2013 20:09:49 -0400 Received: from mail-ea0-x231.google.com ([2a00:1450:4013:c01::231]:63060) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VPLcq-0002UC-IH for grub-devel@gnu.org; Thu, 26 Sep 2013 20:09:40 -0400 Received: by mail-ea0-f177.google.com with SMTP id f15so874902eak.36 for ; Thu, 26 Sep 2013 17:09:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:subject:references :in-reply-to:content-type; bh=TEOgsre7urBiKrRC03Qt5aLkP6ksL0CGf+xs1/w9VI4=; b=W2szfr2gOj3TJoSnJyH/oMP5zB2dgPnPKruYPPZVlJcNP11lKMqiJnoEZta9oF+nIQ PtAgTg81KM4uMUs5qPVi+X4/e7KslO/UyjRENeFNNgI+QD+CZQ2V28uuXVM33jGJp6fi d/sFofONlpt0Nj842pks92eatAVRObniUZF/MNxC2FHgcjfwxXu7QDkSwrjFP5KGXswy t6gYJ0uY1oqHDkXEeJTK6efO9fDGVkLBrYOQliVDNqBIZNHO0HrQQRYa06o7ugN9oFCz o0l39s5Asu2uROJ7DsyPBYnd+4G1QKC3R9wrKHt7QmNigySWZTwInVJSeI+jzClvsjI0 eEaw== X-Received: by 10.15.43.13 with SMTP id w13mr5474425eev.37.1380240579738; Thu, 26 Sep 2013 17:09:39 -0700 (PDT) Received: from [192.168.1.113] (31-249.1-85.cust.bluewin.ch. [85.1.249.31]) by mx.google.com with ESMTPSA id a43sm9598417eep.9.1969.12.31.16.00.00 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 26 Sep 2013 17:09:39 -0700 (PDT) Message-ID: <5244CCC1.5050805@gmail.com> Date: Fri, 27 Sep 2013 02:09:37 +0200 From: =?UTF-8?B?VmxhZGltaXIgJ8+GLWNvZGVyL3BoY29kZXInIFNlcmJpbmVua28=?= User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130821 Icedove/17.0.8 MIME-Version: 1.0 To: grub-devel@gnu.org Subject: Re: [PATCH v6] load_env support for whitelisting, save_env support for check_signatures References: <1380239685-7168-1-git-send-email-jonmccune@google.com> In-Reply-To: <1380239685-7168-1-git-send-email-jonmccune@google.com> X-Enigmail-Version: 1.5.1 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="----enig2AOKMVDLVKTWTUXRUKJGR" X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4013:c01::231 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, 27 Sep 2013 00:09:58 -0000 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2AOKMVDLVKTWTUXRUKJGR Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Committed, thanks. On 27.09.2013 01:54, Jon McCune wrote: > 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. >=20 > 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 >=20 > Changes in v6 of the patch: >=20 > filter disable_...() semantics are such that save/restore is > not necessary. grub_file_open() will re-enable all disabled filters > right after opening the target file. >=20 > Changes in v5 of the patch: >=20 > Drop whitespace changes. >=20 > Drop grub-install script changes. >=20 > Generalized hook support in grub_envblk_iterate(). > Whitelist-specific hook data structures are self-contained in > loadenv.c. >=20 > 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. >=20 > 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. >=20 > open_envblk_file(), when untrusted =3D=3D 1, disables filters using > the compression- and pubkey-specific methods in file.h. The > pubkey filter is saved and restored across untrusted file opens. >=20 > 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. >=20 > Signed-off-by: Jon McCune > --- > grub-core/commands/loadenv.c | 108 ++++++++++++++++++++++++++++++-----= -------- > grub-core/lib/envblk.c | 5 +- > include/grub/lib/envblk.h | 3 +- > util/grub-editenv.c | 5 +- > 4 files changed, 84 insertions(+), 37 deletions(-) >=20 > diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.= c > index c0a42c5..0062c77 100644 > --- a/grub-core/commands/loadenv.c > +++ b/grub-core/commands/loadenv.c > @@ -35,45 +35,54 @@ static const struct grub_arg_option options[] =3D > /* 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 of the environment file."), 0, ARG_TY= PE_NONE}, > {0, 0, 0, 0, 0, 0} > }; > =20 > +/* Opens 'filename' with compression filters disabled. Optionally disa= bles the > + PUBKEY filter (that insists upon properly signed files) as well. P= UBKEY > + 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; > + char *buf =3D 0; > =20 > if (! filename) > { > const char *prefix; > + int len; > =20 > prefix =3D grub_env_get ("prefix"); > - if (prefix) > - { > - int len; > - > - len =3D grub_strlen (prefix); > - filename =3D grub_malloc (len + 1 + sizeof (GRUB_ENVBLK_DEFC= FG)); > - if (! filename) > - return 0; > - > - grub_strcpy (filename, prefix); > - filename[len] =3D '/'; > - grub_strcpy (filename + len + 1, GRUB_ENVBLK_DEFCFG); > - grub_file_filter_disable_compression (); > - file =3D 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 =3D grub_strlen (prefix); > + buf =3D grub_malloc (len + 1 + sizeof (GRUB_ENVBLK_DEFCFG)); > + if (! buf) > + return 0; > + filename =3D buf; > + > + grub_strcpy (filename, prefix); > + filename[len] =3D '/'; > + grub_strcpy (filename + len + 1, GRUB_ENVBLK_DEFCFG); > } > =20 > + /* The filters that are disabled will be re-enabled by the call to > + grub_file_open() after this particular file is opened. */ > grub_file_filter_disable_compression (); > - return grub_file_open (filename); > + if (untrusted) > + grub_file_filter_disable_pubkey (); > + > + file =3D grub_file_open (filename); > + > + if (buf) > + grub_free (buf); > + return file; > } > =20 > static grub_envblk_t > @@ -114,24 +123,55 @@ read_envblk_file (grub_file_t file) > return envblk; > } > =20 > +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 =3D 0; i < whitelist->len; i++) > + if (grub_strcmp (name, whitelist->list[i]) =3D=3D 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*)wh= itelist)) > + grub_env_set (name, value); > + > return 0; > } > =20 > 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 =3D ctxt->state; > grub_file_t file; > grub_envblk_t envblk; > + grub_env_whitelist_t whitelist; > + > + whitelist.len =3D argc; > + whitelist.list =3D args; > =20 > - file =3D open_envblk_file ((state[0].set) ? state[0].arg : 0); > + /* state[0] is the -f flag; state[1] is the --skip-sig flag */ > + file =3D open_envblk_file ((state[0].set) ? state[0].arg : 0, state[= 1].set); > if (! file) > return grub_errno; > =20 > @@ -139,7 +179,8 @@ grub_cmd_load_env (grub_extcmd_context_t ctxt, > if (! envblk) > goto fail; > =20 > - grub_envblk_iterate (envblk, set_var); > + /* argc > 0 indicates caller provided a whitelist of variables to re= ad. */ > + grub_envblk_iterate (envblk, argc > 0 ? &whitelist : 0, set_var); > grub_envblk_close (envblk); > =20 > fail: > @@ -149,7 +190,8 @@ grub_cmd_load_env (grub_extcmd_context_t ctxt, > =20 > /* 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=3D%s\n", name, value); > return 0; > @@ -164,7 +206,7 @@ grub_cmd_list_env (grub_extcmd_context_t ctxt, > grub_file_t file; > grub_envblk_t envblk; > =20 > - file =3D open_envblk_file ((state[0].set) ? state[0].arg : 0); > + file =3D open_envblk_file ((state[0].set) ? state[0].arg : 0, 0); > if (! file) > return grub_errno; > =20 > @@ -172,7 +214,7 @@ grub_cmd_list_env (grub_extcmd_context_t ctxt, > if (! envblk) > goto fail; > =20 > - grub_envblk_iterate (envblk, print_var); > + grub_envblk_iterate (envblk, NULL, print_var); > grub_envblk_close (envblk); > =20 > fail: > @@ -333,7 +375,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 specifie= d"); > =20 > - file =3D open_envblk_file ((state[0].set) ? state[0].arg : 0); > + file =3D open_envblk_file ((state[0].set) ? state[0].arg : 0, > + 1 /* allow untrusted */); > if (! file) > return grub_errno; > =20 > @@ -386,7 +429,8 @@ static grub_extcmd_t cmd_load, cmd_list, cmd_save; > GRUB_MOD_INIT(loadenv) > { > cmd_load =3D > - grub_register_extcmd ("load_env", grub_cmd_load_env, 0, N_("[-f FI= LE]"), > + 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 =3D > 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 cha= r *name) > =20 > 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, vo= id *hook_data)) > { > char *p, *pend; > =20 > @@ -285,7 +286,7 @@ grub_envblk_iterate (grub_envblk_t envblk, > } > *q =3D '\0'; > =20 > - ret =3D hook (name, value); > + ret =3D 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 cha= r *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 *valu= e)); > + void *hook_data, > + int hook (const char *name, const char *valu= e, void *hook_data)); > void grub_envblk_close (grub_envblk_t envblk); > =20 > 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) > } > =20 > 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=3D%s\n", varname, value); > return 0; > @@ -197,7 +198,7 @@ list_variables (const char *name) > grub_envblk_t envblk; > =20 > envblk =3D open_envblk_file (name); > - grub_envblk_iterate (envblk, print_var); > + grub_envblk_iterate (envblk, NULL, print_var); > grub_envblk_close (envblk); > } > =20 >=20 ------enig2AOKMVDLVKTWTUXRUKJGR Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Icedove - http://www.enigmail.net/ iF4EAREKAAYFAlJEzMEACgkQNak7dOguQgkZPAD+MebHToV8ceE9/mpvOrtYEC5v guLiieZWRf3a11t+ivwA/ilwBuwnAlGT6T+KTL44yNFX2+Er0UoA/TxcnpbPKTmM =lNLF -----END PGP SIGNATURE----- ------enig2AOKMVDLVKTWTUXRUKJGR--