diff mbox

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

Message ID 1525182503-13849-4-git-send-email-zohar@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mimi Zohar May 1, 2018, 1:48 p.m. UTC
Allow LSMs and IMA to differentiate between signed regulatory.db and
other firmware.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Luis R. Rodriguez <mcgrof@suse.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Seth Forshee <seth.forshee@canonical.com>
Cc: Johannes Berg <johannes.berg@intel.com>
---
 drivers/base/firmware_loader/main.c | 5 +++++
 include/linux/fs.h                  | 1 +
 2 files changed, 6 insertions(+)

Comments

Luis Chamberlain May 4, 2018, 12:07 a.m. UTC | #1
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 <zohar@linux.vnet.ibm.com>
> Cc: Luis R. Rodriguez <mcgrof@suse.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Seth Forshee <seth.forshee@canonical.com>
> Cc: Johannes Berg <johannes.berg@intel.com>
> ---
>  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, &fw_priv->data, &size,
>  						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
> 
>
Mimi Zohar May 4, 2018, 12:24 a.m. UTC | #2
On Fri, 2018-05-04 at 00:07 +0000, 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 <zohar@linux.vnet.ibm.com>
> > Cc: Luis R. Rodriguez <mcgrof@suse.com>
> > Cc: David Howells <dhowells@redhat.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Seth Forshee <seth.forshee@canonical.com>
> > Cc: Johannes Berg <johannes.berg@intel.com>
> > ---
> >  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, &fw_priv->data, &size,
> >  						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
> > 
> > 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis Chamberlain May 8, 2018, 5:34 p.m. UTC | #3
On Thu, May 03, 2018 at 08:24:26PM -0400, Mimi Zohar wrote:
> On Fri, 2018-05-04 at 00:07 +0000, 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 <zohar@linux.vnet.ibm.com>
> > > Cc: Luis R. Rodriguez <mcgrof@suse.com>
> > > Cc: David Howells <dhowells@redhat.com>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Seth Forshee <seth.forshee@canonical.com>
> > > Cc: Johannes Berg <johannes.berg@intel.com>
> > > ---
> > >  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, &fw_priv->data, &size,
> > >  						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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mimi Zohar May 9, 2018, 11:30 a.m. UTC | #4
On Tue, 2018-05-08 at 17:34 +0000, 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 +0000, 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 <zohar@linux.vnet.ibm.com>
> > > > Cc: Luis R. Rodriguez <mcgrof@suse.com>
> > > > Cc: David Howells <dhowells@redhat.com>
> > > > Cc: Kees Cook <keescook@chromium.org>
> > > > Cc: Seth Forshee <seth.forshee@canonical.com>
> > > > Cc: Johannes Berg <johannes.berg@intel.com>
> > > > ---
> > > >  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, &fw_priv->data, &size,
> > > >  						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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis Chamberlain May 9, 2018, 7:15 p.m. UTC | #5
On Wed, May 09, 2018 at 07:30:28AM -0400, Mimi Zohar wrote:
> On Tue, 2018-05-08 at 17:34 +0000, 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 +0000, 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 <zohar@linux.vnet.ibm.com>
> > > > > Cc: Luis R. Rodriguez <mcgrof@suse.com>
> > > > > Cc: David Howells <dhowells@redhat.com>
> > > > > Cc: Kees Cook <keescook@chromium.org>
> > > > > Cc: Seth Forshee <seth.forshee@canonical.com>
> > > > > Cc: Johannes Berg <johannes.berg@intel.com>
> > > > > ---
> > > > >  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 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.

Its not clear to me why we need to add a new READING id to any open coded
firmware signature checks if we don't have this futuristic option
CONFIG_IMA_APPRAISE_FIRMWARE.

Yes it provides *more*, but IMA is still seeing the exact file descriptor
and do its own thing.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mimi Zohar May 9, 2018, 7:57 p.m. UTC | #6
On Wed, 2018-05-09 at 19:15 +0000, 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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis Chamberlain May 9, 2018, 9:22 p.m. UTC | #7
On Wed, May 09, 2018 at 03:57:18PM -0400, Mimi Zohar wrote:
> On Wed, 2018-05-09 at 19:15 +0000, 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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mimi Zohar May 9, 2018, 10:06 p.m. UTC | #8
On Wed, 2018-05-09 at 21:22 +0000, 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 +0000, 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 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.

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.

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

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis Chamberlain May 9, 2018, 11:48 p.m. UTC | #9
On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> On Wed, 2018-05-09 at 21:22 +0000, 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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mimi Zohar May 10, 2018, 2 a.m. UTC | #10
On Wed, 2018-05-09 at 23:48 +0000, Luis R. Rodriguez wrote:
> On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> > On Wed, 2018-05-09 at 21:22 +0000, 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/msg00005.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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis Chamberlain May 10, 2018, 11:26 p.m. UTC | #11
On Wed, May 09, 2018 at 10:00:58PM -0400, Mimi Zohar wrote:
> On Wed, 2018-05-09 at 23:48 +0000, Luis R. Rodriguez wrote:
> > On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> 
> > > > > Yes, writing regdb as a micro/mini LSM sounds reasonable.  The LSM
> > > > > would differentiate between other firmware and the regulatory.db based
> > > > > on the firmware's pathname.
> > > > 
> > > > If that is the only way then it would be silly to do the mini LSM as all
> > > > calls would have to have the check. A special LSM hook for just the
> > > > regulatory db also doesn't make much sense.
> > > 
> > > All calls to request_firmware() are already going through this LSM
> > > hook.  I should have said, it would be based on both READING_FIRMWARE
> > > and the firmware's pathname.
> > 
> > Yes, but it would still be a strcmp() computation added for all
> > READING_FIRMWARE. In that sense, the current arrangement is only open coding the
> > signature verification for the regulatory.db file.  One way to avoid this would
> > be to add an LSM specific to the regulatory db
> 
> Casey already commented on this suggestion.

Sorry but I must have missed this, can you send me the email or URL where he did that?
I never got a copy of that email I think.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mimi Zohar May 11, 2018, 5 a.m. UTC | #12
On Thu, 2018-05-10 at 23:26 +0000, Luis R. Rodriguez wrote:
> On Wed, May 09, 2018 at 10:00:58PM -0400, Mimi Zohar wrote:
> > On Wed, 2018-05-09 at 23:48 +0000, Luis R. Rodriguez wrote:
> > > On Wed, May 09, 2018 at 06:06:57PM -0400, Mimi Zohar wrote:
> > 
> > > > > > Yes, writing regdb as a micro/mini LSM sounds reasonable.  The LSM
> > > > > > would differentiate between other firmware and the regulatory.db based
> > > > > > on the firmware's pathname.
> > > > > 
> > > > > If that is the only way then it would be silly to do the mini LSM as all
> > > > > calls would have to have the check. A special LSM hook for just the
> > > > > regulatory db also doesn't make much sense.
> > > > 
> > > > All calls to request_firmware() are already going through this LSM
> > > > hook.  I should have said, it would be based on both READING_FIRMWARE
> > > > and the firmware's pathname.
> > > 
> > > Yes, but it would still be a strcmp() computation added for all
> > > READING_FIRMWARE. In that sense, the current arrangement is only open coding the
> > > signature verification for the regulatory.db file.  One way to avoid this would
> > > be to add an LSM specific to the regulatory db
> > 
> > Casey already commented on this suggestion.
> 
> Sorry but I must have missed this, can you send me the email or URL where he did that?
> I never got a copy of that email I think.

My mistake.  I've posted similar patches for kexec_load and for the
firmware sysfs fallback, both call security_kernel_read_file().
Casey's comment was in regards to kexec_load[1], not for the sysfs
fallback mode.  Here's the link to the most recent version of the
kexec_load patches.[2]

[1] http://kernsec.org/pipermail/linux-security-module-archive/2018-May/006690.html
[2] http://kernsec.org/pipermail/linux-security-module-archive/2018-May/006854.html

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index eb34089e4299..d7cdf04a8681 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -318,6 +318,11 @@  fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
 			break;
 		}
 
+#ifdef CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
+		if ((strcmp(fw_priv->fw_name, "regulatory.db") == 0) ||
+		    (strcmp(fw_priv->fw_name, "regulatory.db.p7s") == 0))
+			id = READING_FIRMWARE_REGULATORY_DB;
+#endif
 		fw_priv->size = 0;
 		rc = kernel_read_file_from_path(path, &fw_priv->data, &size,
 						msize, id);
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)	\
 	id(MODULE, kernel-module)		\
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\