From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1VOqau-0001bU-IP for mharc-grub-devel@gnu.org; Wed, 25 Sep 2013 11:01:36 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57776) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VOqar-0001an-U9 for grub-devel@gnu.org; Wed, 25 Sep 2013 11:01:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VOqak-0006wy-TU for grub-devel@gnu.org; Wed, 25 Sep 2013 11:01:33 -0400 Received: from mail-ie0-x22f.google.com ([2607:f8b0:4001:c03::22f]:43655) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VOqak-0006wt-MM for grub-devel@gnu.org; Wed, 25 Sep 2013 11:01:26 -0400 Received: by mail-ie0-f175.google.com with SMTP id e14so10769584iej.20 for ; Wed, 25 Sep 2013 08:01:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=VnB2jEALsK1eukjbytYMmgNK7LknHPQQKwDERkSmL6A=; b=TlMbSebSXe4BoRR6MxGI7SX9dacYy0CGTrZqZdwjB6Rpz2ay+syuN4vGfO/RtW3v+1 qMC248ZKQ3aIQtkR3h+UKu3sBXZ/jwjbdM9dvxZ/YEYVJEMvMjClTIPLq99QKv/M/7J9 7F3YpTJ62e4vj6gI9ohr3WlDPNLPMJfsUDdDDaxgeuqsAZcmDz96uuQUqls0v3f42jGF 7IuFS+ZQ0ZziOXmc6p8L6HL3WlB5fm2U/OmIG1BiHVpgT97THHMnHK4cZjTJjhqYrbhg vmZ5hqOajXGy44REn4wuyY04epj9sgb4dH/Aaik0yLYATkslX1UCegm0OIq2FWJ8xUqk lUDA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=VnB2jEALsK1eukjbytYMmgNK7LknHPQQKwDERkSmL6A=; b=VVe48JAdupjniDSngz9jXK4CXhlFvQy7AaJb0KZjHzwaqLLn1aFx5URhC74A+zVyes wwGKkPJeEwaFjlMqY2VrO40NINUQ6fgqL9sYVMNVM3JiHKuf1WXkNh3lKSCdcagatBmK ERqxfuJ9kK0Pd+2zhyeyi+tlFt4+e7Md2/IvVQpxLStHJ9kSgBzjjNSGPaMnBbsXeYoz 4HsPU3WEWweSMa3imlMiBiTJHzTYN7OKWJ6DurbLpHMKzBqoH9CdAdx5SvL7pY9ausvH yiEd5oCQw2uTRSRl0aL7k5NPamYnL7j9EN1djB6NVXvEacM+24/YaamrEvjMLZ9fg/l/ GEjA== X-Gm-Message-State: ALoCoQnd/EYlf7/f01YVF62ueRiH+w3TYC7BIZddVRh4uzY9IBAeN+KqyCez05kPuEJ1dfhNuaqxv2NMwulD0yV/z/NHvfjklj7GCvHZDooXM8/cjlibqpI8pk9z6LcE0w2qCa+Ed9SEzCCQ2aRmUjim7Ft6GTZkZu//viLXsvFWtPl983K/vNpF81lh0+8kGsHGgHOZZHDB MIME-Version: 1.0 X-Received: by 10.42.76.196 with SMTP id f4mr1361746ick.99.1380121285953; Wed, 25 Sep 2013 08:01:25 -0700 (PDT) Received: by 10.64.232.9 with HTTP; Wed, 25 Sep 2013 08:01:25 -0700 (PDT) In-Reply-To: <20130925100918.2087bec5@opensuse.site> References: <1380074432-27299-1-git-send-email-jonmccune@google.com> <1380074432-27299-3-git-send-email-jonmccune@google.com> <20130925100918.2087bec5@opensuse.site> Date: Wed, 25 Sep 2013 08:01:25 -0700 Message-ID: Subject: Re: [PATCH v4 2/4] load_env support for whitelisting which variables are read from an env file, even if check_signatures=enforce From: Jonathan McCune To: Andrey Borzenkov Content-Type: multipart/alternative; boundary=90e6ba6144f6ff53a704e736865c X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2607:f8b0:4001:c03::22f Cc: The development of GNU GRUB 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 15:01:35 -0000 --90e6ba6144f6ff53a704e736865c Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Tue, Sep 24, 2013 at 11:09 PM, Andrey Borzenkov wro= te: > =D0=92 Tue, 24 Sep 2013 19:00:30 -0700 > Jon McCune =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > > > This version of the patch implements whitelisting in the envblk > subsystem, > > instead of in loadenv.c. It seems to be cleaner than the previous > patches. > > > > This works by adding an open_envblk_file_untrusted() method that bypass= es > > 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. > > I do not really like such silent assumptions. Being able to load only > selected environment variables is useful by itself, but I still may > want to ensure file was not tampered with. > > I suggest you simply add flag "--skip-signature-check" to load_env and > add support for explicit variable list. Then it is up to user how and > when to use it. > This is okay with me. Other opinions? > And please update also documentation for command changes. > Do you mean grub.texi as well, or just within the source code? (e.g., for 'help load_env') > > > +static grub_file_t > > +open_envblk_file_untrusted (char *filename) > > Why do you need extra function? Is not flag to open_envblk_file enough? > I wanted to keep the security-relevant disable / re-enable of the PUBKEY filter as close together as possible. open_envblk_file() does not currently restore any filters that it disabled (the disable functions operate globally, and not per-file), so it would complicate that function somewhat to guarantee that PUBKEY was restored (as there are multiple valid exit points from that function). I will change this to use a flag instead (and possibly restructure the function somewhat) if others agree. > > > +{ > > + 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 (); > > Why do you need to disable *all* filters? Assuming disabling > compression was good enough, you just need to disable signature > checking, right? > That is all the filters at the present time... PUBKEY and then various compression algorithms. I'm not sure what filters might be introduced in the future, so I'm not sure whether it's safer to disable / enable all, or disable / enable the full list of filters that exist today. I thought the more conservative option was to do them all. I will change this to the list of currently existing filters instead if others agree. > > > void > > grub_envblk_iterate (grub_envblk_t envblk, > > + const grub_env_whitelist_t* whitelist, > > int hook (const char *name, const char *value)) > > Again, that's really too restrictive. Like with any other iterators, > I'd make hook accept extra pointer for hook-specific data and pass this > data to grub_envblk_iterate. This will let caller decide the policy. > This sounds good to me. -Jon --90e6ba6144f6ff53a704e736865c Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
On Tue, Sep 24, 2013 at 11:09 PM, Andrey Borzenkov <arv= idjaar@gmail.com> wrote:
=D0=92 Tue, 24 Sep 2013 19:00:30 -0700
Jon McCune <jonmccune@google.com= > =D0=BF=D0=B8=D1=88=D0=B5=D1=82:

> This version of the patch implements whitelisting in the envblk subsys= tem,
> instead of in loadenv.c. =C2=A0It seems to be cleaner than the previou= s patches.
>
> This works by adding an open_envblk_file_untrusted() method that bypas= ses
> signature checking, but only if the invocation of load_env includes a<= br> > whitelist of one or more environment variables that are to be read fro= m the
> file.

I do not really like such silent assumptions. Being able to load only=
selected environment variables is useful by itself, but I still may
want to ensure file was not tampered with.

I suggest you simply add flag "--skip-signature-check" to load_en= v and
add support for explicit variable list. Then it is up to user how and
when to use it.

This is okay with me. = =C2=A0Other opinions?
=C2=A0
And please update also documentation for command changes.
<= div>
Do you mean grub.texi as well, or just within the source= code? =C2=A0(e.g., for 'help load_env')
=C2=A0

> +static grub_file_t
> +open_envblk_file_untrusted (char *filename)

Why do you need extra function? Is not flag to open_envblk_file enoug= h?

I wanted to keep the security-releva= nt disable / re-enable of the PUBKEY filter as close together as possible. = =C2=A0open_envblk_file() does not currently restore any filters that it dis= abled (the disable functions operate globally, and not per-file), so it wou= ld complicate that function somewhat to guarantee that PUBKEY was restored = (as there are multiple valid exit points from that function). =C2=A0I will = change this to use a flag instead (and possibly restructure the function so= mewhat) if others agree.=C2=A0
=C2=A0

> +{
> + =C2=A0grub_file_filter_t curfilt[GRUB_FILE_FILTER_MAX];
> + =C2=A0grub_file_t file;
> +
> + =C2=A0/* Temporarily disable all filters so as to read the untrusted= file */
> + =C2=A0grub_memcpy (curfilt, grub_file_filters_enabled, sizeof (curfi= lt));
> + =C2=A0grub_file_filter_disable_all ();

Why do you need to disable *all* filters? Assuming disabling
compression was good enough, you just need to disable signature
checking, right?

That is all the filter= s at the present time... PUBKEY and then various compression algorithms. = =C2=A0I'm not sure what filters might be introduced in the future, so I= 'm not sure whether it's safer to disable / enable all, or disable = / enable the full list of filters that exist today. =C2=A0I thought the mor= e conservative option was to do them all. =C2=A0I will change this to the l= ist of currently existing filters instead if others agree.
=C2=A0

> =C2=A0void
> =C2=A0grub_envblk_iterate (grub_envblk_t envblk,
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 const grub_env_whitelist_t* whitelist,
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 int hook (const char *name, const char *value))

Again, that's really too restrictive. Like with any other iterato= rs,
I'd make hook accept extra pointer for hook-specific data and pass this=
data to grub_envblk_iterate. This will let caller decide the policy.

This sounds good to me. =C2=A0

=
-Jon
=C2=A0

--90e6ba6144f6ff53a704e736865c--