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:

quoted

On Fri, 2018-05-04 at 00:07 +0000, Luis R. Rodriguez wrote:

quoted

On Tue, May 01, 2018 at 09:48:20AM -0400, Mimi Zohar wrote:

quoted

Allow LSMs and IMA to differentiate between signed regulatory.db and
other firmware.

Signed-off-by: Mimi Zohar <redacted>
Cc: Luis R. Rodriguez <redacted>
Cc: David Howells <dhowells@redhat.com>
Cc: Kees Cook <redacted>
Cc: Seth Forshee <redacted>
Cc: Johannes Berg <redacted>
---
 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.

quoted

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

quoted

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

quoted

quoted

quoted

 		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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html