Re: Firmware signing -- Re: [PATCH 00/27] security, efi: Add kernel lockdown

["Luis R. Rodriguez" <mcgrof@kernel.org>]

On Wed, Nov 15, 2017 at 02:56:57PM -0500, Mimi Zohar wrote:
> On Wed, 2017-11-15 at 18:52 +0100, Luis R. Rodriguez wrote:
> > On Wed, Nov 15, 2017 at 06:49:57AM -0500, Mimi Zohar wrote:
> > > On Tue, 2017-11-14 at 21:50 +0100, Luis R. Rodriguez wrote:
> > > 
> > > > Johannes made cfg80211 recently just use request_firmware() now via commit on
> > > > linux-next 90a53e4432 ("cfg80211: implement regdb signature checking") [0] as
> > > > he got tired of waiting firmware signing, but note he implemented a signature
> > > > checking on its own so he open codes verify_pkcs7_signature() after the
> > > > request_firmware() call. If we are happy to live with this, then so be it.
> > > > 
> > > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=90a53e4432b12288316efaa5f308adafb8d304b0
> > > 
> > > Johannes was tired of waiting?  Commit 5a9196d "ima: add support for
> > > measuring and appraising firmware" has been in the kernel since linux-
> > > 3.17.
> > > 
> > > The original firmware hook for verifying firmware signatures were
> > > replaced with the common LSM pre and post kernel_read_file() hooks
> > > in linux-4.6.y.
> > > 
> > > Even if you wanted to support firmware signature verification without
> > > IMA-appraisal, it should be using the LSM hooks.
> > 
> > request_firmware() uses kernel_read_file_from_path() underneath the hood,
> > and so its used for both:
> > 
> > 	/lib/firmware/regulatory.db
> > 	/lib/firmware/regulatory.db.p7s
> 
> The firmware signature validation should occur as part of
> kernel_read_file_from_path(), not as a stand alone verification.
> 
> Why not extend kernel_read_file_from_path() to pass the detached signature?
> Since the signature would only be used for the verification, there's no need
> to return the open file descriptor.

This goes along with the question if there were an other users who wanted it,
or more importantly -- if firmware signing was desirable for any reason, a
modified kernel_read_file_from_path_signed() could in turn be used, *or* an LSM
added to handle READING_FIRMWARE and READING_FIRMWARE_PREALLOC_BUFFER.  The
above use case was one example outside of the typical firmware use.  I've long
pointed out that we no longer use the firmware API for just firmware, and the
above is now a very good example of it. I've been suggesting uses of the
firmware API for non-firmware had already happened and that more uses were on
its way. Trusted boot has nothing to do with these uses as such the gains of
systems pegged with "trusted boot" have nothing to do validation of these files
through hardware.

In this particular case its simply easier to re-use the existing redistribution
mechanisms in place already so that users can get the regulatory.db and
regulatory.db.p7s.

So here is now an example  use of request_firmware() API  with its own key
validation / keyring solution completely open coded.

So, given the wireless regulatory use case *does* benefit from re-using
request_firmware() due to the file redistribution aspects, using a new
kernel_read_file_from_path_signed() for non-firmware with the same
/lib/firmware/ path specified would be rather sloppy and awkward.  The right
way to not open code this is to consider adding firmware signing support
properly.

> > The later only if CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, which defaults
> > to y anyway.
> > 
> > What I meant was that net/wireless/reg.c now open codes firmware signature
> > validation on its own rather than using a helper. IMA appraisal will still
> > be used if enabled given kernel_read_file_from_path() is used.
> > 
> > The open coding of the firmware signature check is what I wanted to highlight.
> 
> How are the keys in the CFG80211_EXTRA_REGDB_KEYDIR verified?  The
> call to key_create_or_update() with the KEY_ALLOC_BYPASS_RESTRICTION
> option by-passes any requirement that the keys in this directory are
> signed.  This by-passes the concept of extending the secure boot
> signature chain of trust.  To safely validate the keys use the
> restrict_link_by_builtin_trusted option.

I'll let Johannes chime in, if he so wishes.

  Luis