[PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
Mimi Zohar
zohar at linux.vnet.ibm.com
Mon May 14 12:58:12 UTC 2018
On Fri, 2018-05-11 at 21:52 +0000, Luis R. Rodriguez wrote:
> On Fri, May 11, 2018 at 01:00:26AM -0400, Mimi Zohar wrote:
> > On Thu, 2018-05-10 at 23:26 +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
>
> It seems I share Eric's concern on these threads are over general architecture,
> below some notes which I think may help for the long term on that regards.
>
> In the firmware_loader case we have *one* subsystem which as open coded firmware
> signing -- the wireless subsystem open codes firmware verification by doing two
> request_firmware() calls, one for the regulatory.bin and one for regulatory.bin.p7s,
> and then it does its own check. In this patch set you suggested adding
> a new READING_FIRMWARE_REGULATORY_DB. But your first patch in the series also
> adds READING_FIRMWARE_FALLBACK for the fallback case where we enable use of
> the old syfs loading facility.
>
> My concerns are two fold for this case:
>
> a) This would mean adding a new READING_* ID tag per any kernel mechanism which open
> codes its own signature verification scheme.
Yes, that's true. In order to differentiate between different
methods, there needs to be some way of differentiating between them.
>
> b) The way it was implemented was to do (just showing
> READING_FIRMWARE_REGULATORY_DB here):
The purpose for READING_FIRMWARE_REGULATORY_DB is different than for
adding enumerations for other firmware verification methods (eg.
fallback sysfs). In this case, both IMA-appraisal and REGDB are being
called to verify the firmware signature. Adding
READING_FIRMWARE_REGULATORY_DB was in order to coordinate between
them.
continued below ...
>
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index eb34089e4299..d7cdf04a8681 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -318,6 +318,11 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv)
> break;
> }
>
> +#ifdef CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> + if ((strcmp(fw_priv->fw_name, "regulatory.db") == 0) ||
> + (strcmp(fw_priv->fw_name, "regulatory.db.p7s") == 0))
> + id = READING_FIRMWARE_REGULATORY_DB;
> +#endif
> fw_priv->size = 0;
> rc = kernel_read_file_from_path(path, &fw_priv->data, &size,
> msize, id);
>
> This is eye-soring, and in turn would mean adding yet more #ifdefs for any
> code on the kernel which open codes other firmware signing efforts with
> its own kconfig...
Agreed, adding ifdef's is ugly. As previously discussed, this code
will be removed.
To coordinate the signature verification, at build time either regdb
or IMA-appraisal firmware will be enabled. At runtime, in the case
that regdb is enabled and a custom policy requires IMA-appraisal
firmware signature verification, then both signature verification
methods will verify the signatures. If either fails, then the
signature verification will fail.
> I gather from reading the threads above that Eric's concerns are the re-use of
> an API for security to read files for something which is really not a file, but
> a binary blob of some sort and Casey's rebuttal is adding more hooks for small
> things is a bad idea.
>
> In light of all this I'll say that the concerns Eric has are unfortunately
> too late, that ship has sailed eons ago. The old non-fd API for module loading
> init_module() calls security_kernel_read_file(NULL, READING_MODULE). Your
> patch in this series adds security_kernel_read_file(NULL, READING_FIRMWARE_FALLBACK)
> for the old syfs loading facility.
It goes back even farther than that. Commit 2e72d51 ("security:
introduce kernel_module_from_file hook") introduced calling
security_kernel_module_from_file() in copy_module_from_user().
Commit a1db74209483 ("module: replace copy_module_from_fd with kernel
version") replaced it with the call to security_kernel_read_file().
> So in this regard, I think we have no other option but what you suggested, to
> add a wrapper, say a security_kernel_read_blob() wrapper that calls
> security_kernel_read_file(NULL, id); and make the following legacy calls use
> it:
>
> o kernel/module.c for init_module()
> o kexec_load()
> o firmware loader sysfs facility
>
> I think its fair then to add a new READING entry per functionality here
> *but* with the compromise that we *document* that such interfaces are
> discouraged, in preference for interfaces where at least the fd can be
> captured some how. This should hopefully put a stop gap to such interfaces.
Thanks! Eric, are you on board with this?
> Then as for my concern on extending and adding new READING_* ID tags
> for other future open-coded firmware calls, since:
>
> a) You seem to be working on a CONFIG_IMA_APPRAISE_FIRMWARE which would
> enable similar functionality as firmware signing but in userspace.
There are a number of different builtin policies. The "secure_boot"
builtin policy enables file signature verification for firmware,
kernel modules, kexec'ed image, and the IMA policy, but builtin
policies are enabled on the boot command line.
There are two problems:
- there's no way of configuring a builtin policy to verify firmware
signatures.
- CONFIG_IMA_APPRAISE is not fine enough grained.
The CONFIG_IMA_APPRAISE_FIRMWARE will be a Kconfig option. Similar
Kconfig options will require kernel modules, kexec'ed image, and the
IMA policy to be signed.
With this, option "d", below, will be possible.
> b) CONFIG_CFG80211_REQUIRE_SIGNED_REGDB was added to replace needing
> CRDA, with an in-kernel interface. CRDA just did a signature check on
> the regulatory.bin prior to tossing regulatory data over the kernel.
>
> c) We've taken a position to *not* implement generic firmware singing
> upstream in light of the fact that UEFI has pushed hw manufacturers
> to implement FW singing in hardware, and for devices outside of these
> we're happy to suggest IMA use.
There are a number of reasons that the kernel should be verifying
firmware signatures (eg. requiring a specific version of the firmware,
that was locally signed).
> d) It may be possible to have CONFIG_CFG80211_REQUIRE_SIGNED_REGDB
> depend on !CONFIG_IMA_APPRAISE_FIRMWARE this way we'd take a policy
> that CONFIG_IMA_APPRAISE_FIRMWARE replaces in-kernel open coded
> firmware singing facilities
>
> Then I think it makes sense to adapt a policy of being *very careful* allowing
> future open coded firmware signing efforts such as
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB, and recommend folks to do work for things
> like this through IMA with CONFIG_IMA_APPRAISE_FIRMWARE. This would limit our
> needs to extend READING_* ID tags for new open coded firmwares.
>
> Then as for the eye-sore you added for CONFIG_CFG80211_REQUIRE_SIGNED_REGDB,
> in light of all this, I accept we have no other way to deal with it but with
> #ifdefs.. however it could be dealt with, as helpers where if
> CONFIG_CFG80211_REQUIRE_SIGNED_REGDB is not set we just set the id to
> READING_FIRMWARE, ie, just keep the #ifdefs out of the actual code we read.
Assuming you agree with either REGDB or IMA-appraisal firmware being
configured at build, but allowing a custom policy to require firmware
signature verification based on IMA-appraisal at runtime, so that both
REGDB and IMA-appraisal firmware signature verification methods are
required, then the REGDB ifdef's can be removed.
> Perhaps it makes sense to throw this check into the helper:
>
> /* Already populated data member means we're loading into a buffer */
> if (fw_priv->data) {
> id = READING_FIRMWARE_PREALLOC_BUFFER;
> msize = fw_priv->allocated_size;
> }
Thanks, this looks good. What IMA-appraisal should do with
READING_FIRMWARE_PREALLOC_BUFFER still needs to be determined.
> PS: the work Alexei is doing with fork_usermode_blob() may sound similar
> to the above legacy cases, but as I have noted before -- it already uses
> an LSM hook to vet the data loaded as the data gets loaded as module.
Thank you for the clarification.
Mimi
--
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
More information about the Linux-security-module-archive
mailing list