LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Mimi Zohar <zohar@linux.vnet.ibm.com>
To: "Luis R. Rodriguez" <mcgrof@kernel.org>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Casey Schaufler <casey@schaufler-ca.com>,
	Alexei Starovoitov <ast@kernel.org>,
	David Miller <davem@davemloft.net>, Jessica Yu <jeyu@kernel.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>,
	Peter Jones <pjones@redhat.com>,
	"AKASHI, Takahiro" <takahiro.akashi@linaro.org>,
	David Howells <dhowells@redhat.com>,
	linux-wireless <linux-wireless@vger.kernel.org>,
	Kalle Valo <kvalo@codeaurora.org>,
	Seth Forshee <seth.forshee@canonical.com>,
	Johannes Berg <johannes.berg@intel.com>,
	linux-integrity@vger.kernel.org,
	Hans de Goede <hdegoede@redhat.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	linux-security-module@vger.kernel.org,
	linux-kernel@vger.kernel.org, Kees Cook <keescook@chromium.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Andres Rodriguez <andresx7@gmail.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Andy Lutomirski <luto@kernel.org>
Subject: Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
Date: Mon, 14 May 2018 08:58:12 -0400	[thread overview]
Message-ID: <1526302692.3898.145.camel@linux.vnet.ibm.com> (raw)
In-Reply-To: <20180511215250.GJ27853@wotan.suse.de>

On Fri, 2018-05-11 at 21:52 +0000, Luis R. Rodriguez wrote:
> On Fri, May 11, 2018 at 01:00:26AM -0400, Mimi Zohar wrote:
> > On Thu, 2018-05-10 at 23:26 +0000, Luis R. Rodriguez wrote:
> > > On Wed, May 09, 2018 at 10:00:58PM -0400, Mimi Zohar wrote:
> > > > On Wed, 2018-05-09 at 23:48 +0000, Luis R. Rodriguez wrote:
> > > > > On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> > > > 
> > > > > > > > Yes, writing regdb as a micro/mini LSM sounds reasonable.  The LSM
> > > > > > > > would differentiate between other firmware and the regulatory.db based
> > > > > > > > on the firmware's pathname.
> > > > > > > 
> > > > > > > If that is the only way then it would be silly to do the mini LSM as all
> > > > > > > calls would have to have the check. A special LSM hook for just the
> > > > > > > regulatory db also doesn't make much sense.
> > > > > > 
> > > > > > All calls to request_firmware() are already going through this LSM
> > > > > > hook.  I should have said, it would be based on both READING_FIRMWARE
> > > > > > and the firmware's pathname.
> > > > > 
> > > > > Yes, but it would still be a strcmp() computation added for all
> > > > > READING_FIRMWARE. In that sense, the current arrangement is only open coding the
> > > > > signature verification for the regulatory.db file.  One way to avoid this would
> > > > > be to add an LSM specific to the regulatory db
> > > > 
> > > > Casey already commented on this suggestion.
> > > 
> > > Sorry but I must have missed this, can you send me the email or URL where he did that?
> > > I never got a copy of that email I think.
> > 
> > My mistake.  I've posted similar patches for kexec_load and for the
> > firmware sysfs fallback, both call security_kernel_read_file().
> > Casey's comment was in regards to kexec_load[1], not for the sysfs
> > fallback mode.  Here's the link to the most recent version of the
> > kexec_load patches.[2]
> > 
> > [1] http://kernsec.org/pipermail/linux-security-module-archive/2018-May/006690.html
> > [2] http://kernsec.org/pipermail/linux-security-module-archive/2018-May/006854.html
> 
> It seems I share Eric's concern on these threads are over general architecture,
> below some notes which I think may help for the long term on that regards.
> 
> In the firmware_loader case we have *one* subsystem which as open coded firmware
> signing -- the wireless subsystem open codes firmware verification by doing two
> request_firmware() calls, one for the regulatory.bin and one for regulatory.bin.p7s,
> and then it does its own check. In this patch set you suggested adding
> a new READING_FIRMWARE_REGULATORY_DB. But your first patch in the series also
> adds READING_FIRMWARE_FALLBACK for the fallback case where we enable use of
> the old syfs loading facility.
> 
> My concerns are two fold for this case:
> 
> a) This would mean adding a new READING_* ID tag per any kernel mechanism which open
> codes its own signature verification scheme.

Yes, that's true.  In order to differentiate between different
methods, there needs to be some way of differentiating between them.
> 
> b) The way it was implemented was to do (just showing
> READING_FIRMWARE_REGULATORY_DB here):

The purpose for READING_FIRMWARE_REGULATORY_DB is different than for
adding enumerations for other firmware verification methods (eg.
fallback sysfs).  In this case, both IMA-appraisal and REGDB are being
called to verify the firmware signature.  Adding
READING_FIRMWARE_REGULATORY_DB was in order to coordinate between
them.

continued below ...

> 
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index eb34089e4299..d7cdf04a8681 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -318,6 +318,11 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
>                         break;
>                 }
> 
> +#ifdef CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> +               if ((strcmp(fw_priv->fw_name, "regulatory.db") == 0) ||
> +                   (strcmp(fw_priv->fw_name, "regulatory.db.p7s") == 0))
> +                       id = READING_FIRMWARE_REGULATORY_DB;
> +#endif
>                 fw_priv->size = 0;
>                 rc = kernel_read_file_from_path(path, &fw_priv->data, &size,
>                                                 msize, id);
> 
> This is eye-soring, and in turn would mean adding yet more #ifdefs for any
> code on the kernel which open codes other firmware signing efforts with
> its own kconfig...

Agreed, adding ifdef's is ugly.  As previously discussed, this code
will be removed.

To coordinate the signature verification, at build time either regdb
or IMA-appraisal firmware will be enabled.  At runtime, in the case
that regdb is enabled and a custom policy requires IMA-appraisal
firmware signature verification, then both signature verification
methods will verify the signatures.  If either fails, then the
signature verification will fail.

> I gather from reading the threads above that Eric's concerns are the re-use of
> an API for security to read files for something which is really not a file, but
> a binary blob of some sort and Casey's rebuttal is adding more hooks for small
> things is a bad idea.
> 
> In light of all this I'll say that the concerns Eric has are unfortunately
> too late, that ship has sailed eons ago. The old non-fd API for module loading
> init_module() calls       security_kernel_read_file(NULL, READING_MODULE). Your
> patch in this series adds security_kernel_read_file(NULL, READING_FIRMWARE_FALLBACK)
> for the old syfs loading facility.

It goes back even farther than that.  Commit 2e72d51 ("security:
introduce kernel_module_from_file hook") introduced calling
security_kernel_module_from_file() in copy_module_from_user().
Commit a1db74209483 ("module: replace copy_module_from_fd with kernel
version") replaced it with the call to security_kernel_read_file().

> So in this regard, I think we have no other option but what you suggested, to
> add a wrapper, say a security_kernel_read_blob() wrapper that calls
> security_kernel_read_file(NULL, id); and make the following legacy calls use
> it:
> 
>   o kernel/module.c for init_module()
>   o kexec_load()
>   o firmware loader sysfs facility
> 
> I think its fair then to add a new READING entry per functionality here
> *but* with the compromise that we *document* that such interfaces are
> discouraged, in preference for interfaces where at least the fd can be
> captured some how. This should hopefully put a stop gap to such interfaces.

Thanks!  Eric, are you on board with this?

> Then as for my concern on extending and adding new READING_* ID tags
> for other future open-coded firmware calls, since:
> 
> a) You seem to be working on  a CONFIG_IMA_APPRAISE_FIRMWARE which would
> enable similar functionality as firmware signing but in userspace.

There are a number of different builtin policies.  The "secure_boot"
builtin policy enables file signature verification for firmware,
kernel modules, kexec'ed image, and the IMA policy, but builtin
policies are enabled on the boot command line.

There are two problems:
- there's no way of configuring a builtin policy to verify firmware
signatures.
- CONFIG_IMA_APPRAISE is not fine enough grained.

The CONFIG_IMA_APPRAISE_FIRMWARE will be a Kconfig option.  Similar
Kconfig options will require kernel modules, kexec'ed image, and the
IMA policy to be signed.

With this, option "d", below, will be possible.

> b) CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was added to replace needing
> CRDA, with an in-kernel interface. CRDA just did a signature check on
> the regulatory.bin prior to tossing regulatory data over the kernel.
> 
> c) We've taken a position to *not* implement generic firmware singing
> upstream in light of the fact that UEFI has pushed hw manufacturers
> to implement FW singing in hardware, and for devices outside of these
> we're happy to suggest IMA use.

There are a number of reasons that the kernel should be verifying
firmware signatures (eg. requiring a specific version of the firmware,
that was locally signed).

> d) It may be possible to have CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> depend on !CONFIG_IMA_APPRAISE_FIRMWARE this way we'd take a policy
> that CONFIG_IMA_APPRAISE_FIRMWARE replaces in-kernel open coded
> firmware singing facilities
> 
> Then I think it makes sense to adapt a policy of being *very careful* allowing
> future open coded firmware signing efforts such as
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, and recommend folks to do work for things
> like this through IMA with CONFIG_IMA_APPRAISE_FIRMWARE. This would limit our
> needs to extend READING_* ID tags for new open coded firmwares.
> 
> Then as for the eye-sore you added for CONFIG_CFG80211_REQUIRE_SIGNED_REGDB,
> in light of all this, I accept we have no other way to deal with it but with
> #ifdefs.. however it could be dealt with, as helpers where if
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is not set we just set the id to
> READING_FIRMWARE, ie, just keep the #ifdefs out of the actual code we read.

Assuming you agree with either REGDB or IMA-appraisal firmware being
configured at build, but allowing a custom policy to require firmware
signature verification based on IMA-appraisal at runtime, so that both
REGDB and IMA-appraisal firmware signature verification methods are
required, then the REGDB ifdef's can be removed.

> Perhaps it makes sense to throw this check into the helper:
> 
>         /* Already populated data member means we're loading into a buffer */
>         if (fw_priv->data) {
>                 id = READING_FIRMWARE_PREALLOC_BUFFER;
>                 msize = fw_priv->allocated_size;
>         }

Thanks, this looks good.  What IMA-appraisal should do with
READING_FIRMWARE_PREALLOC_BUFFER still needs to be determined.

> PS: the work Alexei is doing with fork_usermode_blob() may sound similar
> to the above legacy cases, but as I have noted before -- it already uses
> an LSM hook to vet the data loaded as the data gets loaded as module.

Thank you for the clarification.

Mimi

  reply	other threads:[~2018-05-14 12:58 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-01 13:48 [PATCH 0/6] firmware: kernel signature verification Mimi Zohar
2018-05-01 13:48 ` [PATCH 1/6] firmware: permit LSMs and IMA to fail firmware sysfs fallback loading Mimi Zohar
2018-05-04  0:02   ` Luis R. Rodriguez
2018-05-04  0:36     ` Mimi Zohar
2018-05-01 13:48 ` [PATCH 2/6] ima: prevent sysfs fallback firmware loading Mimi Zohar
2018-05-04  0:06   ` Luis R. Rodriguez
2018-05-01 13:48 ` [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware Mimi Zohar
2018-05-04  0:07   ` Luis R. Rodriguez
2018-05-04  0:24     ` Mimi Zohar
2018-05-08 17:34       ` Luis R. Rodriguez
2018-05-09 11:30         ` Mimi Zohar
2018-05-09 19:15           ` Luis R. Rodriguez
2018-05-09 19:57             ` Mimi Zohar
2018-05-09 21:22               ` Luis R. Rodriguez
2018-05-09 22:06                 ` Mimi Zohar
2018-05-09 23:48                   ` Luis R. Rodriguez
2018-05-10  2:00                     ` Mimi Zohar
2018-05-10 23:26                       ` Luis R. Rodriguez
2018-05-11  5:00                         ` Mimi Zohar
2018-05-11 21:52                           ` Luis R. Rodriguez
2018-05-14 12:58                             ` Mimi Zohar [this message]
2018-05-14 19:28                               ` Luis R. Rodriguez
2018-05-15  2:02                                 ` Mimi Zohar
2018-05-15  3:26                                   ` Luis R. Rodriguez
2018-05-15 12:32                                     ` Josh Boyer
2018-05-15 12:43                                       ` Mimi Zohar
2018-05-01 13:48 ` [PATCH 4/6] ima: coordinate with signed regulatory.db Mimi Zohar
2018-05-01 13:48 ` [PATCH 5/6] ima: verify kernel firmware signatures when using a preallocated buffer Mimi Zohar
2018-05-01 13:48 ` [RFC PATCH 6/6] ima: prevent loading firmware into a pre-allocated buffer Mimi Zohar
2018-05-04  0:10   ` Luis R. Rodriguez

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1526302692.3898.145.camel@linux.vnet.ibm.com \
    --to=zohar@linux.vnet.ibm.com \
    --cc=andresx7@gmail.com \
    --cc=ard.biesheuvel@linaro.org \
    --cc=ast@kernel.org \
    --cc=casey@schaufler-ca.com \
    --cc=davem@davemloft.net \
    --cc=dhowells@redhat.com \
    --cc=ebiederm@xmission.com \
    --cc=gnomes@lxorguk.ukuu.org.uk \
    --cc=gregkh@linuxfoundation.org \
    --cc=hdegoede@redhat.com \
    --cc=jeyu@kernel.org \
    --cc=johannes.berg@intel.com \
    --cc=keescook@chromium.org \
    --cc=kvalo@codeaurora.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mcgrof@kernel.org \
    --cc=mjg59@srcf.ucam.org \
    --cc=pjones@redhat.com \
    --cc=seth.forshee@canonical.com \
    --cc=takahiro.akashi@linaro.org \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    --subject='Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).