Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Tue, 2018-05-15 at 08:32 -0400, Josh Boyer wrote:

> One aspect that was always a concern to some is whether the firmware files
> were modified directly to have the signature attached to them.  That may
> run afoul of the "no modification" license that most blobs are shipped
> under.  Does IMA have the signatures for the files stored in xattrs or in
> some other detached manner?

They're stored as xattrs.  RPM has support for including file
signatures in the RPM header.

Mimi

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Tue, 2018-05-15 at 08:32 -0400, Josh Boyer wrote:

> One aspect that was always a concern to some is whether the firmware files
> were modified directly to have the signature attached to them.  That may
> run afoul of the "no modification" license that most blobs are shipped
> under.  Does IMA have the signatures for the files stored in xattrs or in
> some other detached manner?

They're stored as xattrs.  RPM has support for including file
signatures in the RPM header.

Mimi

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Josh Boyer]

On Mon, May 14, 2018 at 11:27 PM Luis R. Rodriguez 
wrote:

> On Mon, May 14, 2018 at 10:02:31PM -0400, Mimi Zohar wrote:
> > On Mon, 2018-05-14 at 19:28 +, Luis R. Rodriguez wrote:
> > > > - 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.
> > >
> > > Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will
be
> > > doing firmware verification in userspace or in the kernel.
> >
> > The kernel is verifying signatures.
> >
> > > > 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).
> > >
> > > Oh I agree, Linux enterprise distributions also have a strong reason
to
> > > have this, so that for instance we only trust and run vendor-approved
> > > signed firmware. Otherwise the driver should reject the firmware.
Every
> > > now and then enterprise distros may run into cases were certain
customers
> > > may run oddball firmwares, and its unclear if we expect proper
functionality
> > > with that firmware. Having some form of firmware signing would help
with
> > > this pipeline, but this is currently dealt with at the packaging, and
> > > noting other than logs ensures the driver is using an intended
firmware.
> > > But these needs *IMHO* have not been enough to push to generalize a
kernel
> > > firmware signing facility.
> >
> > In order for IMA-appraisal to verify firmware signatures, the
> > signatures need to be distributed with the firmware.  Perhaps this
> > will be enough of an incentive for distros to start including firmware
> > signatures in the packages.

> Best to poke the maintainers about that... We have been sending mixed
messages
> about firmware signing over years now. Josh, heads up the new one is we
can
> do firmware signing through IMA future CONFIG_IMA_APPRAISE_FIRMWARE. I'll
> bounce you a few emails related to this.

> > > If CONFIG_IMA_APPRAISE_FIRMWARE is going to provide this
functionality somehow
> > > I'm happy to hear it.
> >
> > The functionality has been there since commit 5a9196d ("ima: add
> > support for measuring and appraising firmware").  The
> > security_kernel_fw_from_file() hook was later replaced with the
> > generic security_kernel_read_file() hook.

> Groovy, its unclear from the code on that commit how this is done, so I
> suppose I need to study this a bit more. Josh, do you grok it?

I haven't looked to be honest.  I don't do much in the way of kernel
maintenance on the distro side any longer.  You already have David copied
and I've added Justin Forbes and Laura Abbott to cover Fedora.

One aspect that was always a concern to some is whether the firmware files
were modified directly to have the signature attached to them.  That may
run afoul of the "no modification" license that most blobs are shipped
under.  Does IMA have the signatures for the files stored in xattrs or in
some other detached manner?

josh

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Josh Boyer]

On Mon, May 14, 2018 at 11:27 PM Luis R. Rodriguez 
wrote:

> On Mon, May 14, 2018 at 10:02:31PM -0400, Mimi Zohar wrote:
> > On Mon, 2018-05-14 at 19:28 +, Luis R. Rodriguez wrote:
> > > > - 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.
> > >
> > > Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will
be
> > > doing firmware verification in userspace or in the kernel.
> >
> > The kernel is verifying signatures.
> >
> > > > 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).
> > >
> > > Oh I agree, Linux enterprise distributions also have a strong reason
to
> > > have this, so that for instance we only trust and run vendor-approved
> > > signed firmware. Otherwise the driver should reject the firmware.
Every
> > > now and then enterprise distros may run into cases were certain
customers
> > > may run oddball firmwares, and its unclear if we expect proper
functionality
> > > with that firmware. Having some form of firmware signing would help
with
> > > this pipeline, but this is currently dealt with at the packaging, and
> > > noting other than logs ensures the driver is using an intended
firmware.
> > > But these needs *IMHO* have not been enough to push to generalize a
kernel
> > > firmware signing facility.
> >
> > In order for IMA-appraisal to verify firmware signatures, the
> > signatures need to be distributed with the firmware.  Perhaps this
> > will be enough of an incentive for distros to start including firmware
> > signatures in the packages.

> Best to poke the maintainers about that... We have been sending mixed
messages
> about firmware signing over years now. Josh, heads up the new one is we
can
> do firmware signing through IMA future CONFIG_IMA_APPRAISE_FIRMWARE. I'll
> bounce you a few emails related to this.

> > > If CONFIG_IMA_APPRAISE_FIRMWARE is going to provide this
functionality somehow
> > > I'm happy to hear it.
> >
> > The functionality has been there since commit 5a9196d ("ima: add
> > support for measuring and appraising firmware").  The
> > security_kernel_fw_from_file() hook was later replaced with the
> > generic security_kernel_read_file() hook.

> Groovy, its unclear from the code on that commit how this is done, so I
> suppose I need to study this a bit more. Josh, do you grok it?

I haven't looked to be honest.  I don't do much in the way of kernel
maintenance on the distro side any longer.  You already have David copied
and I've added Justin Forbes and Laura Abbott to cover Fedora.

One aspect that was always a concern to some is whether the firmware files
were modified directly to have the signature attached to them.  That may
run afoul of the "no modification" license that most blobs are shipped
under.  Does IMA have the signatures for the files stored in xattrs or in
some other detached manner?

josh

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Mon, May 14, 2018 at 10:02:31PM -0400, Mimi Zohar wrote:
> On Mon, 2018-05-14 at 19:28 +, Luis R. Rodriguez wrote:
> > > - 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.
> > 
> > Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will be
> > doing firmware verification in userspace or in the kernel.
> 
> The kernel is verifying signatures.
> 
> > > 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).
> > 
> > Oh I agree, Linux enterprise distributions also have a strong reason to
> > have this, so that for instance we only trust and run vendor-approved
> > signed firmware. Otherwise the driver should reject the firmware. Every
> > now and then enterprise distros may run into cases were certain customers
> > may run oddball firmwares, and its unclear if we expect proper functionality
> > with that firmware. Having some form of firmware signing would help with
> > this pipeline, but this is currently dealt with at the packaging, and
> > noting other than logs ensures the driver is using an intended firmware.
> > But these needs *IMHO* have not been enough to push to generalize a kernel
> > firmware signing facility.
> 
> In order for IMA-appraisal to verify firmware signatures, the
> signatures need to be distributed with the firmware.  Perhaps this
> will be enough of an incentive for distros to start including firmware
> signatures in the packages.

Best to poke the maintainers about that... We have been sending mixed messages
about firmware signing over years now. Josh, heads up the new one is we can
do firmware signing through IMA future CONFIG_IMA_APPRAISE_FIRMWARE. I'll
bounce you a few emails related to this.

> > If CONFIG_IMA_APPRAISE_FIRMWARE is going to provide this functionality 
> > somehow
> > I'm happy to hear it.
> 
> The functionality has been there since commit 5a9196d ("ima: add
> support for measuring and appraising firmware").  The
> security_kernel_fw_from_file() hook was later replaced with the
> generic security_kernel_read_file() hook.

Groovy, its unclear from the code on that commit how this is done, so I
suppose I need to study this a bit more. Josh, do you grok it?

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Mon, May 14, 2018 at 10:02:31PM -0400, Mimi Zohar wrote:
> On Mon, 2018-05-14 at 19:28 +, Luis R. Rodriguez wrote:
> > > - 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.
> > 
> > Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will be
> > doing firmware verification in userspace or in the kernel.
> 
> The kernel is verifying signatures.
> 
> > > 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).
> > 
> > Oh I agree, Linux enterprise distributions also have a strong reason to
> > have this, so that for instance we only trust and run vendor-approved
> > signed firmware. Otherwise the driver should reject the firmware. Every
> > now and then enterprise distros may run into cases were certain customers
> > may run oddball firmwares, and its unclear if we expect proper functionality
> > with that firmware. Having some form of firmware signing would help with
> > this pipeline, but this is currently dealt with at the packaging, and
> > noting other than logs ensures the driver is using an intended firmware.
> > But these needs *IMHO* have not been enough to push to generalize a kernel
> > firmware signing facility.
> 
> In order for IMA-appraisal to verify firmware signatures, the
> signatures need to be distributed with the firmware.  Perhaps this
> will be enough of an incentive for distros to start including firmware
> signatures in the packages.

Best to poke the maintainers about that... We have been sending mixed messages
about firmware signing over years now. Josh, heads up the new one is we can
do firmware signing through IMA future CONFIG_IMA_APPRAISE_FIRMWARE. I'll
bounce you a few emails related to this.

> > If CONFIG_IMA_APPRAISE_FIRMWARE is going to provide this functionality 
> > somehow
> > I'm happy to hear it.
> 
> The functionality has been there since commit 5a9196d ("ima: add
> support for measuring and appraising firmware").  The
> security_kernel_fw_from_file() hook was later replaced with the
> generic security_kernel_read_file() hook.

Groovy, its unclear from the code on that commit how this is done, so I
suppose I need to study this a bit more. Josh, do you grok it?

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Mon, 2018-05-14 at 19:28 +, Luis R. Rodriguez wrote:

[...] 

> > 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.
> 
> OK so you're saying that if CONFIG_IMA_APPRAISE_FIRMWARE is disabled you can
> still end up with CONFIG_CFG80211_REQUIRE_SIGNED_REGDB as enabled *and* a
> custom policy which requires IMA-appraisal for the certain firmware signature
> verifications?

Right



> > There are two problems:
> > - there's no way of configuring a builtin policy to verify firmware
> > signatures.
> 
> I'm not too familiar with IMA however it sounds like you can extend the IMA
> built-in policy on the boot command line.

No, there are a couple of policies predefined in the kernel that can
be loaded by specifying them on the boot command line.  A custom
policy can be loaded later.  Only after specifying a policy on the
boot command line or loading a custom policy, does IMA do anything.


> > - 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.
> 
> Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will be
> doing firmware verification in userspace or in the kernel.

The kernel is verifying signatures.



> > 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).
> 
> Oh I agree, Linux enterprise distributions also have a strong reason to
> have this, so that for instance we only trust and run vendor-approved
> signed firmware. Otherwise the driver should reject the firmware. Every
> now and then enterprise distros may run into cases were certain customers
> may run oddball firmwares, and its unclear if we expect proper functionality
> with that firmware. Having some form of firmware signing would help with
> this pipeline, but this is currently dealt with at the packaging, and
> noting other than logs ensures the driver is using an intended firmware.
> But these needs *IMHO* have not been enough to push to generalize a kernel
> firmware signing facility.

In order for IMA-appraisal to verify firmware signatures, the
signatures need to be distributed with the firmware.  Perhaps this
will be enough of an incentive for distros to start including firmware
signatures in the packages.

> If CONFIG_IMA_APPRAISE_FIRMWARE is going to provide this functionality somehow
> I'm happy to hear it.

The functionality has been there since commit 5a9196d ("ima: add
support for measuring and appraising firmware").  The
security_kernel_fw_from_file() hook was later replaced with the
generic security_kernel_read_file() hook.

Mimi

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Mon, 2018-05-14 at 19:28 +, Luis R. Rodriguez wrote:

[...] 

> > 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.
> 
> OK so you're saying that if CONFIG_IMA_APPRAISE_FIRMWARE is disabled you can
> still end up with CONFIG_CFG80211_REQUIRE_SIGNED_REGDB as enabled *and* a
> custom policy which requires IMA-appraisal for the certain firmware signature
> verifications?

Right



> > There are two problems:
> > - there's no way of configuring a builtin policy to verify firmware
> > signatures.
> 
> I'm not too familiar with IMA however it sounds like you can extend the IMA
> built-in policy on the boot command line.

No, there are a couple of policies predefined in the kernel that can
be loaded by specifying them on the boot command line.  A custom
policy can be loaded later.  Only after specifying a policy on the
boot command line or loading a custom policy, does IMA do anything.


> > - 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.
> 
> Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will be
> doing firmware verification in userspace or in the kernel.

The kernel is verifying signatures.



> > 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).
> 
> Oh I agree, Linux enterprise distributions also have a strong reason to
> have this, so that for instance we only trust and run vendor-approved
> signed firmware. Otherwise the driver should reject the firmware. Every
> now and then enterprise distros may run into cases were certain customers
> may run oddball firmwares, and its unclear if we expect proper functionality
> with that firmware. Having some form of firmware signing would help with
> this pipeline, but this is currently dealt with at the packaging, and
> noting other than logs ensures the driver is using an intended firmware.
> But these needs *IMHO* have not been enough to push to generalize a kernel
> firmware signing facility.

In order for IMA-appraisal to verify firmware signatures, the
signatures need to be distributed with the firmware.  Perhaps this
will be enough of an incentive for distros to start including firmware
signatures in the packages.

> If CONFIG_IMA_APPRAISE_FIRMWARE is going to provide this functionality somehow
> I'm happy to hear it.

The functionality has been there since commit 5a9196d ("ima: add
support for measuring and appraising firmware").  The
security_kernel_fw_from_file() hook was later replaced with the
generic security_kernel_read_file() hook.

Mimi

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Mon, May 14, 2018 at 08:58:12AM -0400, Mimi Zohar wrote:
> On Fri, 2018-05-11 at 21:52 +, Luis R. Rodriguez wrote:
> > 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, _priv->data, ,
> > 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. 

But not both, right?

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

OK so you're saying that if CONFIG_IMA_APPRAISE_FIRMWARE is disabled you can
still end up with CONFIG_CFG80211_REQUIRE_SIGNED_REGDB as enabled *and* a
custom policy which requires IMA-appraisal for the certain firmware signature
verifications?

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

I'm not too familiar with IMA however it sounds like you can extend the IMA
built-in policy on the boot command line. If so I'll note MODULE_FIRMWARE()
macro is typically used to annotate firmware description -- not all drivers use
this though for a few reasons, for instance once is that some names are
constructed dynamically at run time. Consider how some drivers rev firmware
with versions, and as they do this they want to keep certain features only for
certain firmware versions. Despite this lack of a direct 1-1 mapping for all
firmwares needed, I *believe* one current use case for this macro is to extract
required firmwares needed on early boot so they can be stashed into the
initramfs if you have these modules enabled on the initramfs. Dracut folks
can confirm but -- dracut *seems* broken then given the semantic gap I
mentioned above.

dracut-init.sh:for _fw in $(modinfo -k $kernel -F firmware $1 2>/dev/null); 
do

If I read this correctly this just complains if the firmware file is
missing if the module is installed on initramfs and the firmware file
is not present. If so we have a current semantic gap and modules with
dynamic names are not handled. And its unclear which modules would be
affected. This is a not a big issue for dracut though since not all drivers
strive to be included on initramfs, unless their storage I suppose -- not
sure how common these storage drivers are for initramfs with dynamic firmware
names which do *not* use MODULE_FIRMWARE().

While the idea of MODULE_FIRMWARE() may need to be given some love or adjusted
to incorporate globs / regexps to fix this existing gap, or we come up with
something more reliable, if fixed, it in theory could end up being re-used to
enable you to extract and build policies for firmware signing at build time...

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

Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will be
doing firmware verification in userspace or in the kernel.

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

nit: To be clear I was not stating options, I was stating premises to
justify my recommendations.

> > b) CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was added to replace needing
> > CRDA, with an in-kernel interface. CRDA just did a 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Mon, May 14, 2018 at 08:58:12AM -0400, Mimi Zohar wrote:
> On Fri, 2018-05-11 at 21:52 +, Luis R. Rodriguez wrote:
> > 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, _priv->data, ,
> > 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. 

But not both, right?

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

OK so you're saying that if CONFIG_IMA_APPRAISE_FIRMWARE is disabled you can
still end up with CONFIG_CFG80211_REQUIRE_SIGNED_REGDB as enabled *and* a
custom policy which requires IMA-appraisal for the certain firmware signature
verifications?

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

I'm not too familiar with IMA however it sounds like you can extend the IMA
built-in policy on the boot command line. If so I'll note MODULE_FIRMWARE()
macro is typically used to annotate firmware description -- not all drivers use
this though for a few reasons, for instance once is that some names are
constructed dynamically at run time. Consider how some drivers rev firmware
with versions, and as they do this they want to keep certain features only for
certain firmware versions. Despite this lack of a direct 1-1 mapping for all
firmwares needed, I *believe* one current use case for this macro is to extract
required firmwares needed on early boot so they can be stashed into the
initramfs if you have these modules enabled on the initramfs. Dracut folks
can confirm but -- dracut *seems* broken then given the semantic gap I
mentioned above.

dracut-init.sh:for _fw in $(modinfo -k $kernel -F firmware $1 2>/dev/null); 
do

If I read this correctly this just complains if the firmware file is
missing if the module is installed on initramfs and the firmware file
is not present. If so we have a current semantic gap and modules with
dynamic names are not handled. And its unclear which modules would be
affected. This is a not a big issue for dracut though since not all drivers
strive to be included on initramfs, unless their storage I suppose -- not
sure how common these storage drivers are for initramfs with dynamic firmware
names which do *not* use MODULE_FIRMWARE().

While the idea of MODULE_FIRMWARE() may need to be given some love or adjusted
to incorporate globs / regexps to fix this existing gap, or we come up with
something more reliable, if fixed, it in theory could end up being re-used to
enable you to extract and build policies for firmware signing at build time...

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

Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will be
doing firmware verification in userspace or in the kernel.

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

nit: To be clear I was not stating options, I was stating premises to
justify my recommendations.

> > b) CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was added to replace needing
> > CRDA, with an in-kernel interface. CRDA just did a 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Fri, 2018-05-11 at 21:52 +, 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 +, 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 +, 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, _priv->data, ,
> 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 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Fri, 2018-05-11 at 21:52 +, 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 +, 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 +, 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, _priv->data, ,
> 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 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Fri, May 11, 2018 at 01:00:26AM -0400, Mimi Zohar wrote:
> On Thu, 2018-05-10 at 23:26 +, 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 +, 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.

b) The way it was implemented was to do (just showing
READING_FIRMWARE_REGULATORY_DB here):

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, _priv->data, ,
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...

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.

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 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Fri, May 11, 2018 at 01:00:26AM -0400, Mimi Zohar wrote:
> On Thu, 2018-05-10 at 23:26 +, 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 +, 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.

b) The way it was implemented was to do (just showing
READING_FIRMWARE_REGULATORY_DB here):

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, _priv->data, ,
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...

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.

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 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Thu, 2018-05-10 at 23:26 +, 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 +, 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

Mimi

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Thu, 2018-05-10 at 23:26 +, 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 +, 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

Mimi

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 10:00:58PM -0400, Mimi Zohar wrote:
> On Wed, 2018-05-09 at 23:48 +, 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.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 10:00:58PM -0400, Mimi Zohar wrote:
> On Wed, 2018-05-09 at 23:48 +, 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.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Wed, 2018-05-09 at 23:48 +, Luis R. Rodriguez wrote:
> On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> > On Wed, 2018-05-09 at 21:22 +, Luis R. Rodriguez wrote:
> > > 
> > > OK, its still not clear to what it will do. If it does not touch the 
> > > firmware
> > > loader code, and it just sets and configures IMA to do file signature 
> > > checking
> > > on its own, then yes I think both mechanisms would be doing the similar 
> > > work.
> > > 
> > > Wouldn't IMA do file signature checks then for all files? Or it would just
> > > enable this for whatever files userspace wishes to cover?
> > 
> > Enabling CONFIG_IMA_APPRAISE_FIRMWARE would enforce firmware
> > signatures on all directly loaded firmware and fail any method of
> > loading firmware that the signature couldn't be verified.
> 
> Ah, so a generic firmware signing mechanism via IMA. Sounds very sensible to 
> me.
> Specially in light of the fact that its what we recommend folks to consider
> if they need to address firmware signing for devices which do not have the
> ability to do hardware firmware signing verification on their own.
> 
> > > One of the things with READING_FIRMWARE_REGULATORY_DB is to also use and 
> > > trust
> > > the wireless-regdgb maintainer's key for this file, could IMA be 
> > > configured to
> > > do that?
> > 
> > IMA has its own trusted keyring.  So either the maintainer's key would
> > need to be added to the IMA keyring,
> 
> I see so we'd need this documented somehow.
> 
> > or IMA-appraisal would need to use the regdb keyring.
> 
> Can you describe this a bit more, for those not too familiar with IMA, in 
> terms
> of what would be involved in the kernel? Or is this all userspace 
> configuration
> stuff?

I think it's a bit premature to be discussing how IMA could add the
builtin regulatory key to its keyring or use the regdb keyring, as
IMA-appraisal doesn't (yet) support detached signatures.

The other option would be to include the regulatory.db signature in
the package.  For rpm, the file signature is included in the RPM
header.  Multiple attempts have been made to have Debian packages
include file signatures.  This is the most recent attempt - https://li
sts.debian.org/debian-dpkg/2018/05/msg5.html

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

Mimi

> and have the
> security_check_regulatory_db() do what it needs per LSM, but that would mean
> setting a precedent for open possibly open coded future firmware verification
> call. Its not too crazy to consider if an end goal may be avoid further open
> coded firmware signature verification hacks.
> 
> > > > Making regdb an LSM would have the same issues as currently - deciding
> > > > if regdb, IMA-appraisal, or both verify the regdb's signature.
> > > 
> > > Its unclear to me why they can't co-exist yet and not have to touch
> > > the firmware_loader code at all.
> > 
> > With the changes discussed above, they will co-exist.  Other than the
> > Kconfig changes, I don't think it will touch the firmware_loader code.
> 
> Great.
> 
>   Luis
> 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Wed, 2018-05-09 at 23:48 +, Luis R. Rodriguez wrote:
> On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> > On Wed, 2018-05-09 at 21:22 +, Luis R. Rodriguez wrote:
> > > 
> > > OK, its still not clear to what it will do. If it does not touch the 
> > > firmware
> > > loader code, and it just sets and configures IMA to do file signature 
> > > checking
> > > on its own, then yes I think both mechanisms would be doing the similar 
> > > work.
> > > 
> > > Wouldn't IMA do file signature checks then for all files? Or it would just
> > > enable this for whatever files userspace wishes to cover?
> > 
> > Enabling CONFIG_IMA_APPRAISE_FIRMWARE would enforce firmware
> > signatures on all directly loaded firmware and fail any method of
> > loading firmware that the signature couldn't be verified.
> 
> Ah, so a generic firmware signing mechanism via IMA. Sounds very sensible to 
> me.
> Specially in light of the fact that its what we recommend folks to consider
> if they need to address firmware signing for devices which do not have the
> ability to do hardware firmware signing verification on their own.
> 
> > > One of the things with READING_FIRMWARE_REGULATORY_DB is to also use and 
> > > trust
> > > the wireless-regdgb maintainer's key for this file, could IMA be 
> > > configured to
> > > do that?
> > 
> > IMA has its own trusted keyring.  So either the maintainer's key would
> > need to be added to the IMA keyring,
> 
> I see so we'd need this documented somehow.
> 
> > or IMA-appraisal would need to use the regdb keyring.
> 
> Can you describe this a bit more, for those not too familiar with IMA, in 
> terms
> of what would be involved in the kernel? Or is this all userspace 
> configuration
> stuff?

I think it's a bit premature to be discussing how IMA could add the
builtin regulatory key to its keyring or use the regdb keyring, as
IMA-appraisal doesn't (yet) support detached signatures.

The other option would be to include the regulatory.db signature in
the package.  For rpm, the file signature is included in the RPM
header.  Multiple attempts have been made to have Debian packages
include file signatures.  This is the most recent attempt - https://li
sts.debian.org/debian-dpkg/2018/05/msg5.html

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

Mimi

> and have the
> security_check_regulatory_db() do what it needs per LSM, but that would mean
> setting a precedent for open possibly open coded future firmware verification
> call. Its not too crazy to consider if an end goal may be avoid further open
> coded firmware signature verification hacks.
> 
> > > > Making regdb an LSM would have the same issues as currently - deciding
> > > > if regdb, IMA-appraisal, or both verify the regdb's signature.
> > > 
> > > Its unclear to me why they can't co-exist yet and not have to touch
> > > the firmware_loader code at all.
> > 
> > With the changes discussed above, they will co-exist.  Other than the
> > Kconfig changes, I don't think it will touch the firmware_loader code.
> 
> Great.
> 
>   Luis
> 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> On Wed, 2018-05-09 at 21:22 +, Luis R. Rodriguez wrote:
> > 
> > OK, its still not clear to what it will do. If it does not touch the 
> > firmware
> > loader code, and it just sets and configures IMA to do file signature 
> > checking
> > on its own, then yes I think both mechanisms would be doing the similar 
> > work.
> > 
> > Wouldn't IMA do file signature checks then for all files? Or it would just
> > enable this for whatever files userspace wishes to cover?
> 
> Enabling CONFIG_IMA_APPRAISE_FIRMWARE would enforce firmware
> signatures on all directly loaded firmware and fail any method of
> loading firmware that the signature couldn't be verified.

Ah, so a generic firmware signing mechanism via IMA. Sounds very sensible to me.
Specially in light of the fact that its what we recommend folks to consider
if they need to address firmware signing for devices which do not have the
ability to do hardware firmware signing verification on their own.

> > One of the things with READING_FIRMWARE_REGULATORY_DB is to also use and 
> > trust
> > the wireless-regdgb maintainer's key for this file, could IMA be configured 
> > to
> > do that?
> 
> IMA has its own trusted keyring.  So either the maintainer's key would
> need to be added to the IMA keyring,

I see so we'd need this documented somehow.

> or IMA-appraisal would need to use the regdb keyring.

Can you describe this a bit more, for those not too familiar with IMA, in terms
of what would be involved in the kernel? Or is this all userspace configuration
stuff?

> > Because that would be one difference here. The whole point of adding
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was to replace CRDA which is a 
> > userspace
> > component which checks the signature of regulatory.db before reading it and
> > passing data to the kernel from it.
> > 
> > Now, however silly it may be to have CONFIG_IMA_APPRAISE_FIRMWARE *and*
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, is your intent in this new patch set
> > you are mentioning, to still enable both to co-exist?
> 
> At build, either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
> CONFIG_IMA_APPRAISE_FIRMWARE, where IMA is appraising all firwmare,
> would be enabled, not both.

OK.

> > > 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 and have the
security_check_regulatory_db() do what it needs per LSM, but that would mean
setting a precedent for open possibly open coded future firmware verification
call. Its not too crazy to consider if an end goal may be avoid further open
coded firmware signature verification hacks.

> > > Making regdb an LSM would have the same issues as currently - deciding
> > > if regdb, IMA-appraisal, or both verify the regdb's signature.
> > 
> > Its unclear to me why they can't co-exist yet and not have to touch
> > the firmware_loader code at all.
> 
> With the changes discussed above, they will co-exist.  Other than the
> Kconfig changes, I don't think it will touch the firmware_loader code.

Great.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> On Wed, 2018-05-09 at 21:22 +, Luis R. Rodriguez wrote:
> > 
> > OK, its still not clear to what it will do. If it does not touch the 
> > firmware
> > loader code, and it just sets and configures IMA to do file signature 
> > checking
> > on its own, then yes I think both mechanisms would be doing the similar 
> > work.
> > 
> > Wouldn't IMA do file signature checks then for all files? Or it would just
> > enable this for whatever files userspace wishes to cover?
> 
> Enabling CONFIG_IMA_APPRAISE_FIRMWARE would enforce firmware
> signatures on all directly loaded firmware and fail any method of
> loading firmware that the signature couldn't be verified.

Ah, so a generic firmware signing mechanism via IMA. Sounds very sensible to me.
Specially in light of the fact that its what we recommend folks to consider
if they need to address firmware signing for devices which do not have the
ability to do hardware firmware signing verification on their own.

> > One of the things with READING_FIRMWARE_REGULATORY_DB is to also use and 
> > trust
> > the wireless-regdgb maintainer's key for this file, could IMA be configured 
> > to
> > do that?
> 
> IMA has its own trusted keyring.  So either the maintainer's key would
> need to be added to the IMA keyring,

I see so we'd need this documented somehow.

> or IMA-appraisal would need to use the regdb keyring.

Can you describe this a bit more, for those not too familiar with IMA, in terms
of what would be involved in the kernel? Or is this all userspace configuration
stuff?

> > Because that would be one difference here. The whole point of adding
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was to replace CRDA which is a 
> > userspace
> > component which checks the signature of regulatory.db before reading it and
> > passing data to the kernel from it.
> > 
> > Now, however silly it may be to have CONFIG_IMA_APPRAISE_FIRMWARE *and*
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, is your intent in this new patch set
> > you are mentioning, to still enable both to co-exist?
> 
> At build, either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
> CONFIG_IMA_APPRAISE_FIRMWARE, where IMA is appraising all firwmare,
> would be enabled, not both.

OK.

> > > 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 and have the
security_check_regulatory_db() do what it needs per LSM, but that would mean
setting a precedent for open possibly open coded future firmware verification
call. Its not too crazy to consider if an end goal may be avoid further open
coded firmware signature verification hacks.

> > > Making regdb an LSM would have the same issues as currently - deciding
> > > if regdb, IMA-appraisal, or both verify the regdb's signature.
> > 
> > Its unclear to me why they can't co-exist yet and not have to touch
> > the firmware_loader code at all.
> 
> With the changes discussed above, they will co-exist.  Other than the
> Kconfig changes, I don't think it will touch the firmware_loader code.

Great.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Wed, 2018-05-09 at 21:22 +, Luis R. Rodriguez wrote:
> On Wed, May 09, 2018 at 03:57:18PM -0400, Mimi Zohar wrote:
> > On Wed, 2018-05-09 at 19:15 +, Luis R. Rodriguez wrote:
> > 
> > > > > > If both are enabled, do we require both signatures or is one enough.
> > > > > 
> > > > > Good question. Considering it as a stacked LSM (although not 
> > > > > implemented
> > > > > as one), I'd say its up to who enabled the Kconfig entries. If IMA and
> > > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If 
> > > > > someone enabled
> > > > > IMA though, then surely I agree that enabling
> > > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its 
> > > > > up to the
> > > > > system integrator to decide.
> > > > 
> > > > Just because IMA-appraisal is enabled in the kernel doesn't mean that
> > > > firmware signatures will be verified.  That is a run time policy
> > > > decision.
> > > 
> > > Sure, I accept this if IMA does not do signature verification. However
> > > signature verification seems like a stackable LSM decision, no?
> > 
> > IMA-appraisal can be configured to enforce file signatures.  Refer to
> > discussion below as to how.
> > 
> > > > > If we however want to make it clear that such things as
> > > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is 
> > > > > enabled we
> > > > > could just make the kconfig depend on !IMA or something?  Or perhaps 
> > > > > a new
> > > > > kconfig for IMA which if selected it means that drivers can opt to 
> > > > > open code
> > > > > *further* kernel signature verification, even though IMA already is 
> > > > > sufficient.
> > > > > Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it?
> > > > 
> > > > The existing CONFIG_IMA_APPRAISE is not enough.  If there was a build
> > > > time IMA config that translated into an IMA policy requiring firmware
> > > > signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could
> > > > be sorted out at build time.
> > > 
> > > I see makes sense.
> > 
> > Ok, so instead of introducing READING_FIRMWARE_REGULATORY_DB, I'll
> > post patches introducing CONFIG_IMA_APPRAISE_FIRMWARE, as described
> > above.
> 
> OK, its still not clear to what it will do. If it does not touch the firmware
> loader code, and it just sets and configures IMA to do file signature checking
> on its own, then yes I think both mechanisms would be doing the similar work.
> 
> Wouldn't IMA do file signature checks then for all files? Or it would just
> enable this for whatever files userspace wishes to cover?

Enabling CONFIG_IMA_APPRAISE_FIRMWARE would enforce firmware
signatures on all directly loaded firmware and fail any method of
loading firmware that the signature couldn't be verified.

> One of the things with READING_FIRMWARE_REGULATORY_DB is to also use and trust
> the wireless-regdgb maintainer's key for this file, could IMA be configured to
> do that?

IMA has its own trusted keyring.  So either the maintainer's key would
need to be added to the IMA keyring, or IMA-appraisal would need to
use the regdb keyring.
  
> Because that would be one difference here. The whole point of adding
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was to replace CRDA which is a userspace
> component which checks the signature of regulatory.db before reading it and
> passing data to the kernel from it.
> 
> Now, however silly it may be to have CONFIG_IMA_APPRAISE_FIRMWARE *and*
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, is your intent in this new patch set
> you are mentioning, to still enable both to co-exist?

At build, either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
CONFIG_IMA_APPRAISE_FIRMWARE, where IMA is appraising all firwmare,
would be enabled, not both.

The builtin IMA-policies could be replaced with a custom policy,
requiring firmware signature verification.  In that case, the regdb
signature would be verified twice.

> 
> > > > > > Assigning a different id for regdb signed firmware allows LSMs and 
> > > > > > IMA
> > > > > > to handle regdb files differently.
> > > > > 
> > > > > That's not the main concern here, its the precedent we are setting 
> > > > > here for
> > > > > any new kernel interface which open codes firmware signing on its 
> > > > > own. What
> > > > > you are doing means other kernel users who open codes their own 
> > > > > firmware
> > > > > signing may need to add yet-another reading ID. That doesn't either 
> > > > > look
> > > > > well on code, and seems kind of silly from a coding perspective given
> > > > > the above, in which I clarify IMA still is doing its own appraisal on 
> > > > > it.
> > > > 
> > > > Suppose,
> > > > 
> > > > 1. Either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
> > > > "CONFIG_IMA_APPRAISE_FIRMWARE" would be configured at build.
> > > > 
> > > > 2. If CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is configured, not
> > > > "CONFIG_IMA_APPRAISE_FIRMWARE", a custom IMA-policy rule that
> > > > appraises the firmware signature could be 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Wed, 2018-05-09 at 21:22 +, Luis R. Rodriguez wrote:
> On Wed, May 09, 2018 at 03:57:18PM -0400, Mimi Zohar wrote:
> > On Wed, 2018-05-09 at 19:15 +, Luis R. Rodriguez wrote:
> > 
> > > > > > If both are enabled, do we require both signatures or is one enough.
> > > > > 
> > > > > Good question. Considering it as a stacked LSM (although not 
> > > > > implemented
> > > > > as one), I'd say its up to who enabled the Kconfig entries. If IMA and
> > > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If 
> > > > > someone enabled
> > > > > IMA though, then surely I agree that enabling
> > > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its 
> > > > > up to the
> > > > > system integrator to decide.
> > > > 
> > > > Just because IMA-appraisal is enabled in the kernel doesn't mean that
> > > > firmware signatures will be verified.  That is a run time policy
> > > > decision.
> > > 
> > > Sure, I accept this if IMA does not do signature verification. However
> > > signature verification seems like a stackable LSM decision, no?
> > 
> > IMA-appraisal can be configured to enforce file signatures.  Refer to
> > discussion below as to how.
> > 
> > > > > If we however want to make it clear that such things as
> > > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is 
> > > > > enabled we
> > > > > could just make the kconfig depend on !IMA or something?  Or perhaps 
> > > > > a new
> > > > > kconfig for IMA which if selected it means that drivers can opt to 
> > > > > open code
> > > > > *further* kernel signature verification, even though IMA already is 
> > > > > sufficient.
> > > > > Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it?
> > > > 
> > > > The existing CONFIG_IMA_APPRAISE is not enough.  If there was a build
> > > > time IMA config that translated into an IMA policy requiring firmware
> > > > signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could
> > > > be sorted out at build time.
> > > 
> > > I see makes sense.
> > 
> > Ok, so instead of introducing READING_FIRMWARE_REGULATORY_DB, I'll
> > post patches introducing CONFIG_IMA_APPRAISE_FIRMWARE, as described
> > above.
> 
> OK, its still not clear to what it will do. If it does not touch the firmware
> loader code, and it just sets and configures IMA to do file signature checking
> on its own, then yes I think both mechanisms would be doing the similar work.
> 
> Wouldn't IMA do file signature checks then for all files? Or it would just
> enable this for whatever files userspace wishes to cover?

Enabling CONFIG_IMA_APPRAISE_FIRMWARE would enforce firmware
signatures on all directly loaded firmware and fail any method of
loading firmware that the signature couldn't be verified.

> One of the things with READING_FIRMWARE_REGULATORY_DB is to also use and trust
> the wireless-regdgb maintainer's key for this file, could IMA be configured to
> do that?

IMA has its own trusted keyring.  So either the maintainer's key would
need to be added to the IMA keyring, or IMA-appraisal would need to
use the regdb keyring.
  
> Because that would be one difference here. The whole point of adding
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was to replace CRDA which is a userspace
> component which checks the signature of regulatory.db before reading it and
> passing data to the kernel from it.
> 
> Now, however silly it may be to have CONFIG_IMA_APPRAISE_FIRMWARE *and*
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, is your intent in this new patch set
> you are mentioning, to still enable both to co-exist?

At build, either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
CONFIG_IMA_APPRAISE_FIRMWARE, where IMA is appraising all firwmare,
would be enabled, not both.

The builtin IMA-policies could be replaced with a custom policy,
requiring firmware signature verification.  In that case, the regdb
signature would be verified twice.

> 
> > > > > > Assigning a different id for regdb signed firmware allows LSMs and 
> > > > > > IMA
> > > > > > to handle regdb files differently.
> > > > > 
> > > > > That's not the main concern here, its the precedent we are setting 
> > > > > here for
> > > > > any new kernel interface which open codes firmware signing on its 
> > > > > own. What
> > > > > you are doing means other kernel users who open codes their own 
> > > > > firmware
> > > > > signing may need to add yet-another reading ID. That doesn't either 
> > > > > look
> > > > > well on code, and seems kind of silly from a coding perspective given
> > > > > the above, in which I clarify IMA still is doing its own appraisal on 
> > > > > it.
> > > > 
> > > > Suppose,
> > > > 
> > > > 1. Either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
> > > > "CONFIG_IMA_APPRAISE_FIRMWARE" would be configured at build.
> > > > 
> > > > 2. If CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is configured, not
> > > > "CONFIG_IMA_APPRAISE_FIRMWARE", a custom IMA-policy rule that
> > > > appraises the firmware signature could be 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 03:57:18PM -0400, Mimi Zohar wrote:
> On Wed, 2018-05-09 at 19:15 +, Luis R. Rodriguez wrote:
> 
> > > > > If both are enabled, do we require both signatures or is one enough.
> > > > 
> > > > Good question. Considering it as a stacked LSM (although not implemented
> > > > as one), I'd say its up to who enabled the Kconfig entries. If IMA and
> > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone 
> > > > enabled
> > > > IMA though, then surely I agree that enabling
> > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its 
> > > > up to the
> > > > system integrator to decide.
> > > 
> > > Just because IMA-appraisal is enabled in the kernel doesn't mean that
> > > firmware signatures will be verified.  That is a run time policy
> > > decision.
> > 
> > Sure, I accept this if IMA does not do signature verification. However
> > signature verification seems like a stackable LSM decision, no?
> 
> IMA-appraisal can be configured to enforce file signatures.  Refer to
> discussion below as to how.
> 
> > > > If we however want to make it clear that such things as
> > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is 
> > > > enabled we
> > > > could just make the kconfig depend on !IMA or something?  Or perhaps a 
> > > > new
> > > > kconfig for IMA which if selected it means that drivers can opt to open 
> > > > code
> > > > *further* kernel signature verification, even though IMA already is 
> > > > sufficient.
> > > > Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it?
> > > 
> > > The existing CONFIG_IMA_APPRAISE is not enough.  If there was a build
> > > time IMA config that translated into an IMA policy requiring firmware
> > > signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could
> > > be sorted out at build time.
> > 
> > I see makes sense.
> 
> Ok, so instead of introducing READING_FIRMWARE_REGULATORY_DB, I'll
> post patches introducing CONFIG_IMA_APPRAISE_FIRMWARE, as described
> above.

OK, its still not clear to what it will do. If it does not touch the firmware
loader code, and it just sets and configures IMA to do file signature checking
on its own, then yes I think both mechanisms would be doing the similar work.

Wouldn't IMA do file signature checks then for all files? Or it would just
enable this for whatever files userspace wishes to cover?

One of the things with READING_FIRMWARE_REGULATORY_DB is to also use and trust
the wireless-regdgb maintainer's key for this file, could IMA be configured to
do that?

Because that would be one difference here. The whole point of adding
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was to replace CRDA which is a userspace
component which checks the signature of regulatory.db before reading it and
passing data to the kernel from it.

Now, however silly it may be to have CONFIG_IMA_APPRAISE_FIRMWARE *and*
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, is your intent in this new patch set
you are mentioning, to still enable both to co-exist?

> > > > > Assigning a different id for regdb signed firmware allows LSMs and IMA
> > > > > to handle regdb files differently.
> > > > 
> > > > That's not the main concern here, its the precedent we are setting here 
> > > > for
> > > > any new kernel interface which open codes firmware signing on its own. 
> > > > What
> > > > you are doing means other kernel users who open codes their own firmware
> > > > signing may need to add yet-another reading ID. That doesn't either look
> > > > well on code, and seems kind of silly from a coding perspective given
> > > > the above, in which I clarify IMA still is doing its own appraisal on 
> > > > it.
> > > 
> > > Suppose,
> > > 
> > > 1. Either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
> > > "CONFIG_IMA_APPRAISE_FIRMWARE" would be configured at build.
> > > 
> > > 2. If CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is configured, not
> > > "CONFIG_IMA_APPRAISE_FIRMWARE", a custom IMA-policy rule that
> > > appraises the firmware signature could be defined.  In this case, both
> > > signature verification methods would be enforced.
> > > 
> > > then READING_FIRMWARE_REGULATORY_DB would not be needed.
> > 
> > True, however I'm suggesting that CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> > could just be a mini subsystem stackable LSM.
> 
> 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.

> Making regdb an LSM would have the same issues as currently - deciding
> if regdb, IMA-appraisal, or both verify the regdb's signature.

Its unclear to me why they can't co-exist yet and not have to touch
the firmware_loader code at all.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 03:57:18PM -0400, Mimi Zohar wrote:
> On Wed, 2018-05-09 at 19:15 +, Luis R. Rodriguez wrote:
> 
> > > > > If both are enabled, do we require both signatures or is one enough.
> > > > 
> > > > Good question. Considering it as a stacked LSM (although not implemented
> > > > as one), I'd say its up to who enabled the Kconfig entries. If IMA and
> > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone 
> > > > enabled
> > > > IMA though, then surely I agree that enabling
> > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its 
> > > > up to the
> > > > system integrator to decide.
> > > 
> > > Just because IMA-appraisal is enabled in the kernel doesn't mean that
> > > firmware signatures will be verified.  That is a run time policy
> > > decision.
> > 
> > Sure, I accept this if IMA does not do signature verification. However
> > signature verification seems like a stackable LSM decision, no?
> 
> IMA-appraisal can be configured to enforce file signatures.  Refer to
> discussion below as to how.
> 
> > > > If we however want to make it clear that such things as
> > > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is 
> > > > enabled we
> > > > could just make the kconfig depend on !IMA or something?  Or perhaps a 
> > > > new
> > > > kconfig for IMA which if selected it means that drivers can opt to open 
> > > > code
> > > > *further* kernel signature verification, even though IMA already is 
> > > > sufficient.
> > > > Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it?
> > > 
> > > The existing CONFIG_IMA_APPRAISE is not enough.  If there was a build
> > > time IMA config that translated into an IMA policy requiring firmware
> > > signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could
> > > be sorted out at build time.
> > 
> > I see makes sense.
> 
> Ok, so instead of introducing READING_FIRMWARE_REGULATORY_DB, I'll
> post patches introducing CONFIG_IMA_APPRAISE_FIRMWARE, as described
> above.

OK, its still not clear to what it will do. If it does not touch the firmware
loader code, and it just sets and configures IMA to do file signature checking
on its own, then yes I think both mechanisms would be doing the similar work.

Wouldn't IMA do file signature checks then for all files? Or it would just
enable this for whatever files userspace wishes to cover?

One of the things with READING_FIRMWARE_REGULATORY_DB is to also use and trust
the wireless-regdgb maintainer's key for this file, could IMA be configured to
do that?

Because that would be one difference here. The whole point of adding
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was to replace CRDA which is a userspace
component which checks the signature of regulatory.db before reading it and
passing data to the kernel from it.

Now, however silly it may be to have CONFIG_IMA_APPRAISE_FIRMWARE *and*
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, is your intent in this new patch set
you are mentioning, to still enable both to co-exist?

> > > > > Assigning a different id for regdb signed firmware allows LSMs and IMA
> > > > > to handle regdb files differently.
> > > > 
> > > > That's not the main concern here, its the precedent we are setting here 
> > > > for
> > > > any new kernel interface which open codes firmware signing on its own. 
> > > > What
> > > > you are doing means other kernel users who open codes their own firmware
> > > > signing may need to add yet-another reading ID. That doesn't either look
> > > > well on code, and seems kind of silly from a coding perspective given
> > > > the above, in which I clarify IMA still is doing its own appraisal on 
> > > > it.
> > > 
> > > Suppose,
> > > 
> > > 1. Either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
> > > "CONFIG_IMA_APPRAISE_FIRMWARE" would be configured at build.
> > > 
> > > 2. If CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is configured, not
> > > "CONFIG_IMA_APPRAISE_FIRMWARE", a custom IMA-policy rule that
> > > appraises the firmware signature could be defined.  In this case, both
> > > signature verification methods would be enforced.
> > > 
> > > then READING_FIRMWARE_REGULATORY_DB would not be needed.
> > 
> > True, however I'm suggesting that CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> > could just be a mini subsystem stackable LSM.
> 
> 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.

> Making regdb an LSM would have the same issues as currently - deciding
> if regdb, IMA-appraisal, or both verify the regdb's signature.

Its unclear to me why they can't co-exist yet and not have to touch
the firmware_loader code at all.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Wed, 2018-05-09 at 19:15 +, Luis R. Rodriguez wrote:

> > > > If both are enabled, do we require both signatures or is one enough.
> > > 
> > > Good question. Considering it as a stacked LSM (although not implemented
> > > as one), I'd say its up to who enabled the Kconfig entries. If IMA and
> > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone 
> > > enabled
> > > IMA though, then surely I agree that enabling
> > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its up 
> > > to the
> > > system integrator to decide.
> > 
> > Just because IMA-appraisal is enabled in the kernel doesn't mean that
> > firmware signatures will be verified.  That is a run time policy
> > decision.
> 
> Sure, I accept this if IMA does not do signature verification. However
> signature verification seems like a stackable LSM decision, no?

IMA-appraisal can be configured to enforce file signatures.  Refer to
discussion below as to how.

> > > If we however want to make it clear that such things as
> > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is enabled 
> > > we
> > > could just make the kconfig depend on !IMA or something?  Or perhaps a new
> > > kconfig for IMA which if selected it means that drivers can opt to open 
> > > code
> > > *further* kernel signature verification, even though IMA already is 
> > > sufficient.
> > > Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it?
> > 
> > The existing CONFIG_IMA_APPRAISE is not enough.  If there was a build
> > time IMA config that translated into an IMA policy requiring firmware
> > signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could
> > be sorted out at build time.
> 
> I see makes sense.

Ok, so instead of introducing READING_FIRMWARE_REGULATORY_DB, I'll
post patches introducing CONFIG_IMA_APPRAISE_FIRMWARE, as described
above.

> 
> > > > Assigning a different id for regdb signed firmware allows LSMs and IMA
> > > > to handle regdb files differently.
> > > 
> > > That's not the main concern here, its the precedent we are setting here 
> > > for
> > > any new kernel interface which open codes firmware signing on its own. 
> > > What
> > > you are doing means other kernel users who open codes their own firmware
> > > signing may need to add yet-another reading ID. That doesn't either look
> > > well on code, and seems kind of silly from a coding perspective given
> > > the above, in which I clarify IMA still is doing its own appraisal on it.
> > 
> > Suppose,
> > 
> > 1. Either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
> > "CONFIG_IMA_APPRAISE_FIRMWARE" would be configured at build.
> > 
> > 2. If CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is configured, not
> > "CONFIG_IMA_APPRAISE_FIRMWARE", a custom IMA-policy rule that
> > appraises the firmware signature could be defined.  In this case, both
> > signature verification methods would be enforced.
> > 
> > then READING_FIRMWARE_REGULATORY_DB would not be needed.
> 
> True, however I'm suggesting that CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> could just be a mini subsystem stackable LSM.

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.

Making regdb an LSM would have the same issues as currently - deciding
if regdb, IMA-appraisal, or both verify the regdb's signature.

Mimi

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Wed, 2018-05-09 at 19:15 +, Luis R. Rodriguez wrote:

> > > > If both are enabled, do we require both signatures or is one enough.
> > > 
> > > Good question. Considering it as a stacked LSM (although not implemented
> > > as one), I'd say its up to who enabled the Kconfig entries. If IMA and
> > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone 
> > > enabled
> > > IMA though, then surely I agree that enabling
> > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its up 
> > > to the
> > > system integrator to decide.
> > 
> > Just because IMA-appraisal is enabled in the kernel doesn't mean that
> > firmware signatures will be verified.  That is a run time policy
> > decision.
> 
> Sure, I accept this if IMA does not do signature verification. However
> signature verification seems like a stackable LSM decision, no?

IMA-appraisal can be configured to enforce file signatures.  Refer to
discussion below as to how.

> > > If we however want to make it clear that such things as
> > > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is enabled 
> > > we
> > > could just make the kconfig depend on !IMA or something?  Or perhaps a new
> > > kconfig for IMA which if selected it means that drivers can opt to open 
> > > code
> > > *further* kernel signature verification, even though IMA already is 
> > > sufficient.
> > > Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it?
> > 
> > The existing CONFIG_IMA_APPRAISE is not enough.  If there was a build
> > time IMA config that translated into an IMA policy requiring firmware
> > signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could
> > be sorted out at build time.
> 
> I see makes sense.

Ok, so instead of introducing READING_FIRMWARE_REGULATORY_DB, I'll
post patches introducing CONFIG_IMA_APPRAISE_FIRMWARE, as described
above.

> 
> > > > Assigning a different id for regdb signed firmware allows LSMs and IMA
> > > > to handle regdb files differently.
> > > 
> > > That's not the main concern here, its the precedent we are setting here 
> > > for
> > > any new kernel interface which open codes firmware signing on its own. 
> > > What
> > > you are doing means other kernel users who open codes their own firmware
> > > signing may need to add yet-another reading ID. That doesn't either look
> > > well on code, and seems kind of silly from a coding perspective given
> > > the above, in which I clarify IMA still is doing its own appraisal on it.
> > 
> > Suppose,
> > 
> > 1. Either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
> > "CONFIG_IMA_APPRAISE_FIRMWARE" would be configured at build.
> > 
> > 2. If CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is configured, not
> > "CONFIG_IMA_APPRAISE_FIRMWARE", a custom IMA-policy rule that
> > appraises the firmware signature could be defined.  In this case, both
> > signature verification methods would be enforced.
> > 
> > then READING_FIRMWARE_REGULATORY_DB would not be needed.
> 
> True, however I'm suggesting that CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> could just be a mini subsystem stackable LSM.

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.

Making regdb an LSM would have the same issues as currently - deciding
if regdb, IMA-appraisal, or both verify the regdb's signature.

Mimi

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 07:30:28AM -0400, Mimi Zohar wrote:
> On Tue, 2018-05-08 at 17:34 +, Luis R. Rodriguez wrote:
> > On Thu, May 03, 2018 at 08:24:26PM -0400, Mimi Zohar wrote:
> > > On Fri, 2018-05-04 at 00:07 +, Luis R. Rodriguez wrote:
> > > > On Tue, May 01, 2018 at 09:48:20AM -0400, Mimi Zohar wrote:
> > > > > Allow LSMs and IMA to differentiate between signed regulatory.db and
> > > > > other firmware.
> > > > > 
> > > > > Signed-off-by: Mimi Zohar 
> > > > > Cc: Luis R. Rodriguez 
> > > > > Cc: David Howells 
> > > > > Cc: Kees Cook 
> > > > > Cc: Seth Forshee 
> > > > > Cc: Johannes Berg 
> > > > > ---
> > > > >  drivers/base/firmware_loader/main.c | 5 +
> > > > >  include/linux/fs.h  | 1 +
> > > > >  2 files changed, 6 insertions(+)
> > > > > 
> > > > > 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
> > > > 
> > > > Whoa, no way.
> > > 
> > > There are two methods for the kernel to verify firmware signatures.
> > 
> > Yes, but although CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is its own kernel
> > mechanism to verify firmware it uses the request_firmware*() API for
> > regulatory.db and regulatory.db.p7s, and IMA already can appraise these two
> > files since the firmware API is used.
> 
> IMA-appraisal can verify a signature stored as an xattr, but not a
> detached signature.  That support could be added, but isn't there
> today.  Today, a regulatory.db signature would have to be stored as an
> xattr. 

Right, my point was that if someone has IMA installed:

a) they would add those xattr to files in /lib/firmware/ already
b) upon request_firmware*() calls a security hook would trigger
   which would enable IMA to appraise those files. So not only
   would the kernel in turn do double checks on regulatory.db,
   but also a check on regulatory.db.p7s as well.

The difference I suppose is IMA would use a hash function instead of signature
check, correct?

> > As such I see no reason to add a new ID for them at all.
> > K
> > Its not providing an *alternative*, its providing an *extra* kernel measure.
> > If anything CONFIG_CFG80211_REQUIRE_SIGNED_REGDB perhaps should be its own
> > stacked LSM. I'd be open to see patches which set that out. May be a
> > cleaner interface.
> > 
> > > If both are enabled, do we require both signatures or is one enough.
> > 
> > Good question. Considering it as a stacked LSM (although not implemented
> > as one), I'd say its up to who enabled the Kconfig entries. If IMA and
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone 
> > enabled
> > IMA though, then surely I agree that enabling
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its up to 
> > the
> > system integrator to decide.
> 
> Just because IMA-appraisal is enabled in the kernel doesn't mean that
> firmware signatures will be verified.  That is a run time policy
> decision.

Sure, I accept this if IMA does not do signature verification. However
signature verification seems like a stackable LSM decision, no?

> > If we however want to make it clear that such things as
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is enabled we
> > could just make the kconfig depend on !IMA or something?  Or perhaps a new
> > kconfig for IMA which if selected it means that drivers can opt to open code
> > *further* kernel signature verification, even though IMA already is 
> > sufficient.
> > Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it?
> 
> The existing CONFIG_IMA_APPRAISE is not enough.  If there was a build
> time IMA config that translated into an IMA policy requiring firmware
> signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could
> be sorted out at build time.

I see makes sense.

> > > Assigning a different id for regdb signed firmware allows LSMs and IMA
> > > to handle regdb files differently.
> > 
> > That's not the main concern here, its the precedent we are setting here for
> > any new kernel interface which open codes firmware signing on its own. What
> > you are doing means other kernel users who open 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 07:30:28AM -0400, Mimi Zohar wrote:
> On Tue, 2018-05-08 at 17:34 +, Luis R. Rodriguez wrote:
> > On Thu, May 03, 2018 at 08:24:26PM -0400, Mimi Zohar wrote:
> > > On Fri, 2018-05-04 at 00:07 +, Luis R. Rodriguez wrote:
> > > > On Tue, May 01, 2018 at 09:48:20AM -0400, Mimi Zohar wrote:
> > > > > Allow LSMs and IMA to differentiate between signed regulatory.db and
> > > > > other firmware.
> > > > > 
> > > > > Signed-off-by: Mimi Zohar 
> > > > > Cc: Luis R. Rodriguez 
> > > > > Cc: David Howells 
> > > > > Cc: Kees Cook 
> > > > > Cc: Seth Forshee 
> > > > > Cc: Johannes Berg 
> > > > > ---
> > > > >  drivers/base/firmware_loader/main.c | 5 +
> > > > >  include/linux/fs.h  | 1 +
> > > > >  2 files changed, 6 insertions(+)
> > > > > 
> > > > > 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
> > > > 
> > > > Whoa, no way.
> > > 
> > > There are two methods for the kernel to verify firmware signatures.
> > 
> > Yes, but although CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is its own kernel
> > mechanism to verify firmware it uses the request_firmware*() API for
> > regulatory.db and regulatory.db.p7s, and IMA already can appraise these two
> > files since the firmware API is used.
> 
> IMA-appraisal can verify a signature stored as an xattr, but not a
> detached signature.  That support could be added, but isn't there
> today.  Today, a regulatory.db signature would have to be stored as an
> xattr. 

Right, my point was that if someone has IMA installed:

a) they would add those xattr to files in /lib/firmware/ already
b) upon request_firmware*() calls a security hook would trigger
   which would enable IMA to appraise those files. So not only
   would the kernel in turn do double checks on regulatory.db,
   but also a check on regulatory.db.p7s as well.

The difference I suppose is IMA would use a hash function instead of signature
check, correct?

> > As such I see no reason to add a new ID for them at all.
> > K
> > Its not providing an *alternative*, its providing an *extra* kernel measure.
> > If anything CONFIG_CFG80211_REQUIRE_SIGNED_REGDB perhaps should be its own
> > stacked LSM. I'd be open to see patches which set that out. May be a
> > cleaner interface.
> > 
> > > If both are enabled, do we require both signatures or is one enough.
> > 
> > Good question. Considering it as a stacked LSM (although not implemented
> > as one), I'd say its up to who enabled the Kconfig entries. If IMA and
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone 
> > enabled
> > IMA though, then surely I agree that enabling
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its up to 
> > the
> > system integrator to decide.
> 
> Just because IMA-appraisal is enabled in the kernel doesn't mean that
> firmware signatures will be verified.  That is a run time policy
> decision.

Sure, I accept this if IMA does not do signature verification. However
signature verification seems like a stackable LSM decision, no?

> > If we however want to make it clear that such things as
> > CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is enabled we
> > could just make the kconfig depend on !IMA or something?  Or perhaps a new
> > kconfig for IMA which if selected it means that drivers can opt to open code
> > *further* kernel signature verification, even though IMA already is 
> > sufficient.
> > Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it?
> 
> The existing CONFIG_IMA_APPRAISE is not enough.  If there was a build
> time IMA config that translated into an IMA policy requiring firmware
> signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could
> be sorted out at build time.

I see makes sense.

> > > Assigning a different id for regdb signed firmware allows LSMs and IMA
> > > to handle regdb files differently.
> > 
> > That's not the main concern here, its the precedent we are setting here for
> > any new kernel interface which open codes firmware signing on its own. What
> > you are doing means other kernel users who open codes their own firmware
> > signing may need to add yet-another reading ID. That doesn't either look
> > well on code, and seems kind of 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Tue, 2018-05-08 at 17:34 +, Luis R. Rodriguez wrote:
> On Thu, May 03, 2018 at 08:24:26PM -0400, Mimi Zohar wrote:
> > On Fri, 2018-05-04 at 00:07 +, Luis R. Rodriguez wrote:
> > > On Tue, May 01, 2018 at 09:48:20AM -0400, Mimi Zohar wrote:
> > > > Allow LSMs and IMA to differentiate between signed regulatory.db and
> > > > other firmware.
> > > > 
> > > > Signed-off-by: Mimi Zohar 
> > > > Cc: Luis R. Rodriguez 
> > > > Cc: David Howells 
> > > > Cc: Kees Cook 
> > > > Cc: Seth Forshee 
> > > > Cc: Johannes Berg 
> > > > ---
> > > >  drivers/base/firmware_loader/main.c | 5 +
> > > >  include/linux/fs.h  | 1 +
> > > >  2 files changed, 6 insertions(+)
> > > > 
> > > > 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
> > > 
> > > Whoa, no way.
> > 
> > There are two methods for the kernel to verify firmware signatures.
> 
> Yes, but although CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is its own kernel
> mechanism to verify firmware it uses the request_firmware*() API for
> regulatory.db and regulatory.db.p7s, and IMA already can appraise these two
> files since the firmware API is used.

IMA-appraisal can verify a signature stored as an xattr, but not a
detached signature.  That support could be added, but isn't there
today.  Today, a regulatory.db signature would have to be stored as an
xattr. 

> 
> As such I see no reason to add a new ID for them at all.
> K
> Its not providing an *alternative*, its providing an *extra* kernel measure.
> If anything CONFIG_CFG80211_REQUIRE_SIGNED_REGDB perhaps should be its own
> stacked LSM. I'd be open to see patches which set that out. May be a
> cleaner interface.
> 
> > If both are enabled, do we require both signatures or is one enough.
> 
> Good question. Considering it as a stacked LSM (although not implemented
> as one), I'd say its up to who enabled the Kconfig entries. If IMA and
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone enabled
> IMA though, then surely I agree that enabling
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its up to 
> the
> system integrator to decide.

Just because IMA-appraisal is enabled in the kernel doesn't mean that
firmware signatures will be verified.  That is a run time policy
decision.

> 
> If we however want to make it clear that such things as
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is enabled we
> could just make the kconfig depend on !IMA or something?  Or perhaps a new
> kconfig for IMA which if selected it means that drivers can opt to open code
> *further* kernel signature verification, even though IMA already is 
> sufficient.
> Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it?

The existing CONFIG_IMA_APPRAISE is not enough.  If there was a build
time IMA config that translated into an IMA policy requiring firmware
signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could
be sorted out at build time.

> 
> > Assigning a different id for regdb signed firmware allows LSMs and IMA
> > to handle regdb files differently.
> 
> That's not the main concern here, its the precedent we are setting here for
> any new kernel interface which open codes firmware signing on its own. What
> you are doing means other kernel users who open codes their own firmware
> signing may need to add yet-another reading ID. That doesn't either look
> well on code, and seems kind of silly from a coding perspective given
> the above, in which I clarify IMA still is doing its own appraisal on it.

Suppose,

1. Either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
"CONFIG_IMA_APPRAISE_FIRMWARE" would be configured at build.

2. If CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is configured, not
"CONFIG_IMA_APPRAISE_FIRMWARE", a custom IMA-policy rule that
appraises the firmware signature could be defined.  In this case, both
signature verification methods would be enforced.

then READING_FIRMWARE_REGULATORY_DB would not be needed.

Mimi

> 
> > > > fw_priv->size = 0;
> > > > rc = kernel_read_file_from_path(path, _priv->data, 
> > > > ,
> > > > 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Tue, 2018-05-08 at 17:34 +, Luis R. Rodriguez wrote:
> On Thu, May 03, 2018 at 08:24:26PM -0400, Mimi Zohar wrote:
> > On Fri, 2018-05-04 at 00:07 +, Luis R. Rodriguez wrote:
> > > On Tue, May 01, 2018 at 09:48:20AM -0400, Mimi Zohar wrote:
> > > > Allow LSMs and IMA to differentiate between signed regulatory.db and
> > > > other firmware.
> > > > 
> > > > Signed-off-by: Mimi Zohar 
> > > > Cc: Luis R. Rodriguez 
> > > > Cc: David Howells 
> > > > Cc: Kees Cook 
> > > > Cc: Seth Forshee 
> > > > Cc: Johannes Berg 
> > > > ---
> > > >  drivers/base/firmware_loader/main.c | 5 +
> > > >  include/linux/fs.h  | 1 +
> > > >  2 files changed, 6 insertions(+)
> > > > 
> > > > 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
> > > 
> > > Whoa, no way.
> > 
> > There are two methods for the kernel to verify firmware signatures.
> 
> Yes, but although CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is its own kernel
> mechanism to verify firmware it uses the request_firmware*() API for
> regulatory.db and regulatory.db.p7s, and IMA already can appraise these two
> files since the firmware API is used.

IMA-appraisal can verify a signature stored as an xattr, but not a
detached signature.  That support could be added, but isn't there
today.  Today, a regulatory.db signature would have to be stored as an
xattr. 

> 
> As such I see no reason to add a new ID for them at all.
> K
> Its not providing an *alternative*, its providing an *extra* kernel measure.
> If anything CONFIG_CFG80211_REQUIRE_SIGNED_REGDB perhaps should be its own
> stacked LSM. I'd be open to see patches which set that out. May be a
> cleaner interface.
> 
> > If both are enabled, do we require both signatures or is one enough.
> 
> Good question. Considering it as a stacked LSM (although not implemented
> as one), I'd say its up to who enabled the Kconfig entries. If IMA and
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone enabled
> IMA though, then surely I agree that enabling
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its up to 
> the
> system integrator to decide.

Just because IMA-appraisal is enabled in the kernel doesn't mean that
firmware signatures will be verified.  That is a run time policy
decision.

> 
> If we however want to make it clear that such things as
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is enabled we
> could just make the kconfig depend on !IMA or something?  Or perhaps a new
> kconfig for IMA which if selected it means that drivers can opt to open code
> *further* kernel signature verification, even though IMA already is 
> sufficient.
> Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it?

The existing CONFIG_IMA_APPRAISE is not enough.  If there was a build
time IMA config that translated into an IMA policy requiring firmware
signature verification (eg. CONFIG_IMA_APPRAISE_FIRMWARE), this could
be sorted out at build time.

> 
> > Assigning a different id for regdb signed firmware allows LSMs and IMA
> > to handle regdb files differently.
> 
> That's not the main concern here, its the precedent we are setting here for
> any new kernel interface which open codes firmware signing on its own. What
> you are doing means other kernel users who open codes their own firmware
> signing may need to add yet-another reading ID. That doesn't either look
> well on code, and seems kind of silly from a coding perspective given
> the above, in which I clarify IMA still is doing its own appraisal on it.

Suppose,

1. Either CONFIG_CFG80211_REQUIRE_SIGNED_REGDB or
"CONFIG_IMA_APPRAISE_FIRMWARE" would be configured at build.

2. If CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is configured, not
"CONFIG_IMA_APPRAISE_FIRMWARE", a custom IMA-policy rule that
appraises the firmware signature could be defined.  In this case, both
signature verification methods would be enforced.

then READING_FIRMWARE_REGULATORY_DB would not be needed.

Mimi

> 
> > > > fw_priv->size = 0;
> > > > rc = kernel_read_file_from_path(path, _priv->data, 
> > > > ,
> > > > msize, id);
> > > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > > index dc16a73c3d38..d1153c2884b9 100644
> > > > --- 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Thu, May 03, 2018 at 08:24:26PM -0400, Mimi Zohar wrote:
> On Fri, 2018-05-04 at 00:07 +, Luis R. Rodriguez wrote:
> > On Tue, May 01, 2018 at 09:48:20AM -0400, Mimi Zohar wrote:
> > > Allow LSMs and IMA to differentiate between signed regulatory.db and
> > > other firmware.
> > > 
> > > Signed-off-by: Mimi Zohar 
> > > Cc: Luis R. Rodriguez 
> > > Cc: David Howells 
> > > Cc: Kees Cook 
> > > Cc: Seth Forshee 
> > > Cc: Johannes Berg 
> > > ---
> > >  drivers/base/firmware_loader/main.c | 5 +
> > >  include/linux/fs.h  | 1 +
> > >  2 files changed, 6 insertions(+)
> > > 
> > > 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
> > 
> > Whoa, no way.
> 
> There are two methods for the kernel to verify firmware signatures.

Yes, but although CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is its own kernel
mechanism to verify firmware it uses the request_firmware*() API for
regulatory.db and regulatory.db.p7s, and IMA already can appraise these two
files since the firmware API is used.

As such I see no reason to add a new ID for them at all.

Its not providing an *alternative*, its providing an *extra* kernel measure.
If anything CONFIG_CFG80211_REQUIRE_SIGNED_REGDB perhaps should be its own
stacked LSM. I'd be open to see patches which set that out. May be a
cleaner interface.

> If both are enabled, do we require both signatures or is one enough.

Good question. Considering it as a stacked LSM (although not implemented
as one), I'd say its up to who enabled the Kconfig entries. If IMA and
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone enabled
IMA though, then surely I agree that enabling
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its up to the
system integrator to decide.

If we however want to make it clear that such things as
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is enabled we
could just make the kconfig depend on !IMA or something? Or perhaps a new
kconfig for IMA which if selected it means that drivers can opt to open code
*further* kernel signature verification, even though IMA already is sufficient.
Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it?

> Assigning a different id for regdb signed firmware allows LSMs and IMA
> to handle regdb files differently.

That's not the main concern here, its the precedent we are setting here for
any new kernel interface which open codes firmware signing on its own. What
you are doing means other kernel users who open codes their own firmware
signing may need to add yet-another reading ID. That doesn't either look
well on code, and seems kind of silly from a coding perspective given
the above, in which I clarify IMA still is doing its own appraisal on it.

> > >   fw_priv->size = 0;
> > >   rc = kernel_read_file_from_path(path, _priv->data, ,
> > >   msize, id);
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index dc16a73c3d38..d1153c2884b9 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -2811,6 +2811,7 @@ extern int do_pipe_flags(int *, int);
> > >   id(FIRMWARE, firmware)  \
> > >   id(FIRMWARE_PREALLOC_BUFFER, firmware)  \
> > >   id(FIRMWARE_FALLBACK, firmware) \
> > > + id(FIRMWARE_REGULATORY_DB, firmware)\
> > 
> > Why could IMA not appriase these files? They are part of the standard path.
> 
> The subsequent patch attempts to verify the IMA-appraisal signature, but on
> failure it falls back to allowing regdb signatures. 
> For systems that only want to load firmware based on IMA-appraisal, then
>regdb wouldn't be enabled.

I think we can codify this a bit better, without a new ID.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Thu, May 03, 2018 at 08:24:26PM -0400, Mimi Zohar wrote:
> On Fri, 2018-05-04 at 00:07 +, Luis R. Rodriguez wrote:
> > On Tue, May 01, 2018 at 09:48:20AM -0400, Mimi Zohar wrote:
> > > Allow LSMs and IMA to differentiate between signed regulatory.db and
> > > other firmware.
> > > 
> > > Signed-off-by: Mimi Zohar 
> > > Cc: Luis R. Rodriguez 
> > > Cc: David Howells 
> > > Cc: Kees Cook 
> > > Cc: Seth Forshee 
> > > Cc: Johannes Berg 
> > > ---
> > >  drivers/base/firmware_loader/main.c | 5 +
> > >  include/linux/fs.h  | 1 +
> > >  2 files changed, 6 insertions(+)
> > > 
> > > 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
> > 
> > Whoa, no way.
> 
> There are two methods for the kernel to verify firmware signatures.

Yes, but although CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is its own kernel
mechanism to verify firmware it uses the request_firmware*() API for
regulatory.db and regulatory.db.p7s, and IMA already can appraise these two
files since the firmware API is used.

As such I see no reason to add a new ID for them at all.

Its not providing an *alternative*, its providing an *extra* kernel measure.
If anything CONFIG_CFG80211_REQUIRE_SIGNED_REGDB perhaps should be its own
stacked LSM. I'd be open to see patches which set that out. May be a
cleaner interface.

> If both are enabled, do we require both signatures or is one enough.

Good question. Considering it as a stacked LSM (although not implemented
as one), I'd say its up to who enabled the Kconfig entries. If IMA and
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are enabled then both. If someone enabled
IMA though, then surely I agree that enabling
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is stupid and redundant, but its up to the
system integrator to decide.

If we however want to make it clear that such things as
CONFIG_CFG80211_REQUIRE_SIGNED_REGDB are not required when IMA is enabled we
could just make the kconfig depend on !IMA or something? Or perhaps a new
kconfig for IMA which if selected it means that drivers can opt to open code
*further* kernel signature verification, even though IMA already is sufficient.
Perhaps CONFIG_ENABLE_IMA_OVERLAPPING, and the driver depends on it?

> Assigning a different id for regdb signed firmware allows LSMs and IMA
> to handle regdb files differently.

That's not the main concern here, its the precedent we are setting here for
any new kernel interface which open codes firmware signing on its own. What
you are doing means other kernel users who open codes their own firmware
signing may need to add yet-another reading ID. That doesn't either look
well on code, and seems kind of silly from a coding perspective given
the above, in which I clarify IMA still is doing its own appraisal on it.

> > >   fw_priv->size = 0;
> > >   rc = kernel_read_file_from_path(path, _priv->data, ,
> > >   msize, id);
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index dc16a73c3d38..d1153c2884b9 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -2811,6 +2811,7 @@ extern int do_pipe_flags(int *, int);
> > >   id(FIRMWARE, firmware)  \
> > >   id(FIRMWARE_PREALLOC_BUFFER, firmware)  \
> > >   id(FIRMWARE_FALLBACK, firmware) \
> > > + id(FIRMWARE_REGULATORY_DB, firmware)\
> > 
> > Why could IMA not appriase these files? They are part of the standard path.
> 
> The subsequent patch attempts to verify the IMA-appraisal signature, but on
> failure it falls back to allowing regdb signatures. 
> For systems that only want to load firmware based on IMA-appraisal, then
>regdb wouldn't be enabled.

I think we can codify this a bit better, without a new ID.

  Luis

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Fri, 2018-05-04 at 00:07 +, Luis R. Rodriguez wrote:
> On Tue, May 01, 2018 at 09:48:20AM -0400, Mimi Zohar wrote:
> > Allow LSMs and IMA to differentiate between signed regulatory.db and
> > other firmware.
> > 
> > Signed-off-by: Mimi Zohar 
> > Cc: Luis R. Rodriguez 
> > Cc: David Howells 
> > Cc: Kees Cook 
> > Cc: Seth Forshee 
> > Cc: Johannes Berg 
> > ---
> >  drivers/base/firmware_loader/main.c | 5 +
> >  include/linux/fs.h  | 1 +
> >  2 files changed, 6 insertions(+)
> > 
> > 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
> 
> Whoa, no way.

There are two methods for the kernel to verify firmware signatures.
 If both are enabled, do we require both signatures or is one enough.
Assigning a different id for regdb signed firmware allows LSMs and IMA
to handle regdb files differently.

> 
> > fw_priv->size = 0;
> > rc = kernel_read_file_from_path(path, _priv->data, ,
> > msize, id);
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index dc16a73c3d38..d1153c2884b9 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2811,6 +2811,7 @@ extern int do_pipe_flags(int *, int);
> > id(FIRMWARE, firmware)  \
> > id(FIRMWARE_PREALLOC_BUFFER, firmware)  \
> > id(FIRMWARE_FALLBACK, firmware) \
> > +   id(FIRMWARE_REGULATORY_DB, firmware)\
> 
> Why could IMA not appriase these files? They are part of the standard path.

The subsequent patch attempts to verify the IMA-appraisal signature,
but on failure it falls back to allowing regdb signatures.  For
systems that only want to load firmware based on IMA-appraisal, then
regdb wouldn't be enabled.

Mimi

> 
> > id(MODULE, kernel-module)   \
> > id(KEXEC_IMAGE, kexec-image)\
> > id(KEXEC_INITRAMFS, kexec-initramfs)\
> > -- 
> > 2.7.5
> > 
> > 
> 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Mimi Zohar]

On Fri, 2018-05-04 at 00:07 +, Luis R. Rodriguez wrote:
> On Tue, May 01, 2018 at 09:48:20AM -0400, Mimi Zohar wrote:
> > Allow LSMs and IMA to differentiate between signed regulatory.db and
> > other firmware.
> > 
> > Signed-off-by: Mimi Zohar 
> > Cc: Luis R. Rodriguez 
> > Cc: David Howells 
> > Cc: Kees Cook 
> > Cc: Seth Forshee 
> > Cc: Johannes Berg 
> > ---
> >  drivers/base/firmware_loader/main.c | 5 +
> >  include/linux/fs.h  | 1 +
> >  2 files changed, 6 insertions(+)
> > 
> > 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
> 
> Whoa, no way.

There are two methods for the kernel to verify firmware signatures.
 If both are enabled, do we require both signatures or is one enough.
Assigning a different id for regdb signed firmware allows LSMs and IMA
to handle regdb files differently.

> 
> > fw_priv->size = 0;
> > rc = kernel_read_file_from_path(path, _priv->data, ,
> > msize, id);
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index dc16a73c3d38..d1153c2884b9 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2811,6 +2811,7 @@ extern int do_pipe_flags(int *, int);
> > id(FIRMWARE, firmware)  \
> > id(FIRMWARE_PREALLOC_BUFFER, firmware)  \
> > id(FIRMWARE_FALLBACK, firmware) \
> > +   id(FIRMWARE_REGULATORY_DB, firmware)\
> 
> Why could IMA not appriase these files? They are part of the standard path.

The subsequent patch attempts to verify the IMA-appraisal signature,
but on failure it falls back to allowing regdb signatures.  For
systems that only want to load firmware based on IMA-appraisal, then
regdb wouldn't be enabled.

Mimi

> 
> > id(MODULE, kernel-module)   \
> > id(KEXEC_IMAGE, kexec-image)\
> > id(KEXEC_INITRAMFS, kexec-initramfs)\
> > -- 
> > 2.7.5
> > 
> > 
> 

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Tue, May 01, 2018 at 09:48:20AM -0400, Mimi Zohar wrote:
> Allow LSMs and IMA to differentiate between signed regulatory.db and
> other firmware.
> 
> Signed-off-by: Mimi Zohar 
> Cc: Luis R. Rodriguez 
> Cc: David Howells 
> Cc: Kees Cook 
> Cc: Seth Forshee 
> Cc: Johannes Berg 
> ---
>  drivers/base/firmware_loader/main.c | 5 +
>  include/linux/fs.h  | 1 +
>  2 files changed, 6 insertions(+)
> 
> 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

Whoa, no way.

>   fw_priv->size = 0;
>   rc = kernel_read_file_from_path(path, _priv->data, ,
>   msize, id);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index dc16a73c3d38..d1153c2884b9 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2811,6 +2811,7 @@ extern int do_pipe_flags(int *, int);
>   id(FIRMWARE, firmware)  \
>   id(FIRMWARE_PREALLOC_BUFFER, firmware)  \
>   id(FIRMWARE_FALLBACK, firmware) \
> + id(FIRMWARE_REGULATORY_DB, firmware)\

Why could IMA not appriase these files? They are part of the standard path.

  Luis

>   id(MODULE, kernel-module)   \
>   id(KEXEC_IMAGE, kexec-image)\
>   id(KEXEC_INITRAMFS, kexec-initramfs)\
> -- 
> 2.7.5
> 
> 

-- 
Do not panic

Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware

[Luis R. Rodriguez]

On Tue, May 01, 2018 at 09:48:20AM -0400, Mimi Zohar wrote:
> Allow LSMs and IMA to differentiate between signed regulatory.db and
> other firmware.
> 
> Signed-off-by: Mimi Zohar 
> Cc: Luis R. Rodriguez 
> Cc: David Howells 
> Cc: Kees Cook 
> Cc: Seth Forshee 
> Cc: Johannes Berg 
> ---
>  drivers/base/firmware_loader/main.c | 5 +
>  include/linux/fs.h  | 1 +
>  2 files changed, 6 insertions(+)
> 
> 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

Whoa, no way.

>   fw_priv->size = 0;
>   rc = kernel_read_file_from_path(path, _priv->data, ,
>   msize, id);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index dc16a73c3d38..d1153c2884b9 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2811,6 +2811,7 @@ extern int do_pipe_flags(int *, int);
>   id(FIRMWARE, firmware)  \
>   id(FIRMWARE_PREALLOC_BUFFER, firmware)  \
>   id(FIRMWARE_FALLBACK, firmware) \
> + id(FIRMWARE_REGULATORY_DB, firmware)\

Why could IMA not appriase these files? They are part of the standard path.

  Luis

>   id(MODULE, kernel-module)   \
>   id(KEXEC_IMAGE, kexec-image)\
>   id(KEXEC_INITRAMFS, kexec-initramfs)\
> -- 
> 2.7.5
> 
> 

-- 
Do not panic