| Message ID | 1525182503-13849-4-git-send-email-zohar@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > >
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 > > > > >
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
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 >
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
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
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
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
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
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 >
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
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
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) \
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(+)