| Message ID | 1510347775.3549.2.camel@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 10, 2017 at 04:02:55PM -0500, Mimi Zohar wrote: > If the kernel is locked down and IMA-appraisal is not enabled, prevent > loading of unsigned firmware. > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > --- > > Changelog v1: > - Lots of minor changes Kconfig, Makefile, fw_lsm.c for such a small patch <-- snip --> > diff --git a/security/fw_lockdown/fw_lsm.c b/security/fw_lockdown/fw_lsm.c > new file mode 100644 > index 000000000000..cce03a5c5280 > --- /dev/null > +++ b/security/fw_lockdown/fw_lsm.c > @@ -0,0 +1,51 @@ > +/* > + * fw_lockdown security module > + * > + * Copyright (C) 2017 IBM Corporation > + * > + * Authors: > + * Mimi Zohar <zohar@linux.vnet.ibm.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#define pr_fmt(fmt) "fw_lockdown: " fmt > + > +#include <linux/module.h> > +#include <linux/ima.h> > +#include <linux/lsm_hooks.h> > + > +/** > + * fw_lockdown_read_file - prevent loading of unsigned firmware > + * @file: pointer to firmware > + * @read_id: caller identifier > + * > + * Prevent loading of unsigned firmware in lockdown mode. > + */ > +static int fw_lockdown_read_file(struct file *file, enum kernel_read_file_id id) > +{ > + if (id == READING_FIRMWARE) { > + if (!is_ima_appraise_enabled() && > + !kernel_is_locked_down("Loading of unsigned firmware")) > + return -EACCES; I don't have kernel_is_locked_down() but I found it here: https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-lockdown&id=0ff5adf1e81135c4ed914d0a8b70124caed04142 1) I don't see it taking any arguments 2) Don't we want: if (!is_ima_appraise_enabled() && kernel_is_locked_down()) return -EACCES; If, only if IMA appraisal is enabled would we enable kernel lockdown. Luis
On 11/10/2017 1:02 PM, Mimi Zohar wrote: > If the kernel is locked down and IMA-appraisal is not enabled, prevent > loading of unsigned firmware. > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > --- > > Changelog v1: > - Lots of minor changes Kconfig, Makefile, fw_lsm.c for such a small patch > > security/Kconfig | 1 + > security/Makefile | 2 ++ > security/fw_lockdown/Kconfig | 6 +++++ > security/fw_lockdown/Makefile | 3 +++ > security/fw_lockdown/fw_lsm.c | 51 +++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 63 insertions(+) > create mode 100644 security/fw_lockdown/Kconfig > create mode 100644 security/fw_lockdown/Makefile > create mode 100644 security/fw_lockdown/fw_lsm.c > > diff --git a/security/Kconfig b/security/Kconfig > index a4fa8b826039..6e7e5888f823 100644 > --- a/security/Kconfig > +++ b/security/Kconfig > @@ -243,6 +243,7 @@ source security/tomoyo/Kconfig > source security/apparmor/Kconfig > source security/loadpin/Kconfig > source security/yama/Kconfig > +source security/fw_lockdown/Kconfig > > source security/integrity/Kconfig > > diff --git a/security/Makefile b/security/Makefile > index 8c4a43e3d4e0..58852dee5e22 100644 > --- a/security/Makefile > +++ b/security/Makefile > @@ -9,6 +9,7 @@ subdir-$(CONFIG_SECURITY_TOMOYO) += tomoyo > subdir-$(CONFIG_SECURITY_APPARMOR) += apparmor > subdir-$(CONFIG_SECURITY_YAMA) += yama > subdir-$(CONFIG_SECURITY_LOADPIN) += loadpin > +subdir-$(CONFIG_SECURITY_FW_LOCKDOWN) += fw_lockdown > > # always enable default capabilities > obj-y += commoncap.o > @@ -24,6 +25,7 @@ obj-$(CONFIG_SECURITY_TOMOYO) += tomoyo/ > obj-$(CONFIG_SECURITY_APPARMOR) += apparmor/ > obj-$(CONFIG_SECURITY_YAMA) += yama/ > obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/ > +obj-$(CONFIG_SECURITY_FW_LOCKDOWN) += fw_lockdown/ > obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o > > # Object integrity file lists > diff --git a/security/fw_lockdown/Kconfig b/security/fw_lockdown/Kconfig > new file mode 100644 > index 000000000000..d6aef6ce8fee > --- /dev/null > +++ b/security/fw_lockdown/Kconfig > @@ -0,0 +1,6 @@ > +config SECURITY_FW_LOCKDOWN > + bool "Prevent loading unsigned firmware" > + depends on LOCK_DOWN_KERNEL > + default y > + help > + Prevent loading unsigned firmware in lockdown mode, > diff --git a/security/fw_lockdown/Makefile b/security/fw_lockdown/Makefile > new file mode 100644 > index 000000000000..3a16757fd35d > --- /dev/null > +++ b/security/fw_lockdown/Makefile > @@ -0,0 +1,3 @@ > +obj-$(CONFIG_SECURITY_FW_LOCKDOWN) += fw_lockdown.o > + > +fw_lockdown-y := fw_lsm.o > diff --git a/security/fw_lockdown/fw_lsm.c b/security/fw_lockdown/fw_lsm.c > new file mode 100644 > index 000000000000..cce03a5c5280 > --- /dev/null > +++ b/security/fw_lockdown/fw_lsm.c > @@ -0,0 +1,51 @@ > +/* > + * fw_lockdown security module > + * > + * Copyright (C) 2017 IBM Corporation > + * > + * Authors: > + * Mimi Zohar <zohar@linux.vnet.ibm.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#define pr_fmt(fmt) "fw_lockdown: " fmt > + > +#include <linux/module.h> > +#include <linux/ima.h> > +#include <linux/lsm_hooks.h> > + > +/** > + * fw_lockdown_read_file - prevent loading of unsigned firmware > + * @file: pointer to firmware > + * @read_id: caller identifier > + * > + * Prevent loading of unsigned firmware in lockdown mode. > + */ > +static int fw_lockdown_read_file(struct file *file, enum kernel_read_file_id id) > +{ > + if (id == READING_FIRMWARE) { > + if (!is_ima_appraise_enabled() && > + !kernel_is_locked_down("Loading of unsigned firmware")) > + return -EACCES; > + } > + return 0; > +} > + > +static struct security_hook_list fw_lockdown_hooks[] = { > + LSM_HOOK_INIT(kernel_read_file, fw_lockdown_read_file) > +}; > + > +static int __init init_fw_lockdown(void) > +{ > + security_add_hooks(fw_lockdown_hooks, ARRAY_SIZE(fw_lockdown_hooks), > + "fw_lockdown"); SECURITY_NAME_MAX is 10. Either pick a shorter name or increase this value. I slightly favor an increase to 16. > + pr_info("initialized\n"); > + return 0; > +} > + > +late_initcall(init_fw_lockdown); > +MODULE_LICENSE("GPL");
On Fri, 2017-11-10 at 23:39 +0100, Luis R. Rodriguez wrote: > On Fri, Nov 10, 2017 at 04:02:55PM -0500, Mimi Zohar wrote: > > If the kernel is locked down and IMA-appraisal is not enabled, prevent > > loading of unsigned firmware. > > > > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > > --- > > > > Changelog v1: > > - Lots of minor changes Kconfig, Makefile, fw_lsm.c for such a small patch > > <-- snip --> > > > diff --git a/security/fw_lockdown/fw_lsm.c b/security/fw_lockdown/fw_lsm.c > > new file mode 100644 > > index 000000000000..cce03a5c5280 > > --- /dev/null > > +++ b/security/fw_lockdown/fw_lsm.c > > @@ -0,0 +1,51 @@ > > +/* > > + * fw_lockdown security module > > + * > > + * Copyright (C) 2017 IBM Corporation > > + * > > + * Authors: > > + * Mimi Zohar <zohar@linux.vnet.ibm.com> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + */ > > + > > +#define pr_fmt(fmt) "fw_lockdown: " fmt > > + > > +#include <linux/module.h> > > +#include <linux/ima.h> > > +#include <linux/lsm_hooks.h> > > + > > +/** > > + * fw_lockdown_read_file - prevent loading of unsigned firmware > > + * @file: pointer to firmware > > + * @read_id: caller identifier > > + * > > + * Prevent loading of unsigned firmware in lockdown mode. > > + */ > > +static int fw_lockdown_read_file(struct file *file, enum kernel_read_file_id id) > > +{ > > + if (id == READING_FIRMWARE) { > > + if (!is_ima_appraise_enabled() && > > + !kernel_is_locked_down("Loading of unsigned firmware")) > > + return -EACCES; > > I don't have kernel_is_locked_down() but I found it here: > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-lockdown&id=0ff5adf1e81135c4ed914d0a8b70124caed04142 > > 1) I don't see it taking any arguments David reposted the patches https://lkml.org/lkml/2017/11/9/659 with the change. The latest version of the patches are in his efi-lock- down branch. > 2) Don't we want: > > if (!is_ima_appraise_enabled() && kernel_is_locked_down()) > return -EACCES; > > If, only if IMA appraisal is enabled would we enable kernel lockdown. Yes, the kernel_is_locked_down() test needs to be inverted. Mimi
On 11/10/2017 01:02 PM, Mimi Zohar wrote: > If the kernel is locked down and IMA-appraisal is not enabled, prevent > loading of unsigned firmware. > diff --git a/security/fw_lockdown/Kconfig b/security/fw_lockdown/Kconfig > new file mode 100644 > index 000000000000..d6aef6ce8fee > --- /dev/null > +++ b/security/fw_lockdown/Kconfig > @@ -0,0 +1,6 @@ > +config SECURITY_FW_LOCKDOWN > + bool "Prevent loading unsigned firmware" > + depends on LOCK_DOWN_KERNEL > + default y > + help > + Prevent loading unsigned firmware in lockdown mode, Please be honest about what this does. This option makes your system useless if you don't use IMA-Appraisal and it offers a particular security benefit if you do you IMA-Appraisal. How about making it depend on IMA-Appraisal? Change the name to SECURITY_ONLY_LOAD_IMA_APPRAISED_FIRMWARE and adjust the text accordingly, please. > +/** > + * fw_lockdown_read_file - prevent loading of unsigned firmware > + * @file: pointer to firmware > + * @read_id: caller identifier > + * > + * Prevent loading of unsigned firmware in lockdown mode. That comment gives a highly misleading impression of what this function does. > + */ > +static int fw_lockdown_read_file(struct file *file, enum kernel_read_file_id id)
On Mon, Apr 02, 2018 at 05:42:22PM -0700, Andy Lutomirski wrote: > On 11/10/2017 01:02 PM, Mimi Zohar wrote: > > If the kernel is locked down and IMA-appraisal is not enabled, prevent > > loading of unsigned firmware. > > > diff --git a/security/fw_lockdown/Kconfig b/security/fw_lockdown/Kconfig > > new file mode 100644 > > index 000000000000..d6aef6ce8fee > > --- /dev/null > > +++ b/security/fw_lockdown/Kconfig > > @@ -0,0 +1,6 @@ > > +config SECURITY_FW_LOCKDOWN > > + bool "Prevent loading unsigned firmware" > > + depends on LOCK_DOWN_KERNEL > > + default y > > + help > > + Prevent loading unsigned firmware in lockdown mode, > > Please be honest about what this does. This option makes your system > useless if you don't use IMA-Appraisal and it offers a particular security > benefit if you do you IMA-Appraisal. How about making it depend on > IMA-Appraisal? Change the name to SECURITY_ONLY_LOAD_IMA_APPRAISED_FIRMWARE > and adjust the text accordingly, please. Mimi is on vacation right now so I'll address this. All the above makes sense, but just keep in mind the patch was posted just as an illustration of how IMA *can* live up to the original intent proposed by David Howells on kernel lockdown. David's old kernel lockdown documentation stipulated that a requirement was to also prevent loading unsigned firmware. Does the kernel lockdown documentation still have a section indicating signed firmware is a requirement? Or was that removed in the end? Mimi's RFC was at a time when we still had on our roadmap the prospect of adding generic signed firmware support into Linux. Mimi's point and RFC patch was an illustration then on how IMA users can already meet these assumed requirements on 'kernel lockdown', and that if we used a micro LSM hook this could be easily managed, given the firmware signing support we intended on adding would not be needed for IMA systems. Her point was that with IMA you can already meet the definition and IMA systems would not have to wait for "generic firmware signing" to be merged. The biggest thing which has changed since then is that we decided to *not* support or streamline generic firmware signing (non-IMA) for now for a few reasons [0] [1] which are important to re-iterate as these are easy to forget, and AFAICT not documented anywhere. One was that my own requirement for it was already done -- cfg80211 already open codes firmware signing checking on its own through the new CONFIG_CFG80211_REQUIRE_SIGNED_REGDB thereby also not requiring CRDA anymore, the userspace component which used to read the signed regulatory database and then send regulatory content to the wireless subsystem. So CRDA is no longer needed if you enable CONFIG_CFG80211_REQUIRE_SIGNED_REGDB. Second, is is that the whole UEFI movement pushed hardware vendors to start embracing requiring signed firmware on peripherals themselves. So a lot of hardware these days *does* do firmware signing already and a generic kernel firmware signing facility would then just make us do double work then. There are exceptions and there are a few reasons for this. For instance USB devices are not allowed to pee on random memory (thanks for the analogy Alan!), so not all USB devices have support for checking their own firmware signature. This is one reason also why why some operating systems do not allow users to use random USB devices. If companies *really* need to vet for these they have a few options, one is to use IMA, the other is to have the driver open code the firmware signing component just as we did with cfg80211 through the new CONFIG_CFG80211_REQUIRE_SIGNED_REGDB. If we end up later with enough open coded firmware checking users we can later revisit adding a generic firmware signing facility. Third is that there is no strong evidence of any security issue with having firmware in /lib/firmware not be signed. I can attest to this, in my research of all the history of this, I cannot find a single incident which calls bloody murder for unsigned firmware. There are concerns and known backdoors on firmware for storage drives for instance, but these are not /lib/firmware files, for instance. If you also consider the second reason above, it may also help explain why perhaps there is also any serious threat on this front. Granted, there are known cases know of a third party using some signing key to sign malicious firmware and using that to exploit an OS, but if they did that and we trusted each vendor signing key as well we'd be just as vulnerable. Fourth, if you want signed /lib/firmware, you might want signed binaries, and if you want signed binaries, we already have IMA. So.. until there is no real strong reason to support and maintain a generic firmware signing mechanism, no need to add it. We can wait for the open coded boom, and if that really does happen we'll revisit later. So the kernel lockdown definition may want to revise all the above, review its documentation and if the "firmware signing" aspect is still there, update it somehow to reflect or ignore the above. If we want to keep "signed firmware" as a requirement for "kernel lockdown" then we have no option to also amend the definition I think to consider signed executables, and imply or recommend the use of IMA. If the assumption is that peripherals or drivers all have hardware firmware signing support and are OK with random USB gadgets, then perhaps we are OK in removing (if its not already removed) the "signed firmware" requirement from the "kernel lockdown" definition, and the concept of signed firmware or IMA can live on as additional effort for now. However, *iff* we *really* want to push again for generic "signed firmware", we want a good justification for it considering all the above. To be fair I'll note that I don't recall a super strong justification for signed modules back in the day, other than Rusty saying 'its happening', 'its being merged', and that some companies wanted it. I think we do have the case of companies wanting signed firmware in this case as well... however the issue is the foundations may be on a warm fuzzy feeling only, and a big difference is that with modules you don't have peripherals checking for the signed bits, with firmware we do have this practice growing these days. If we *do* end up moving forward with generic "signed firmware" later though, the architecture for it would be through a micro LSM as Mimi suggested in this patch, and it would whitelists IMA. The patch would also need to be updated to whitelist the new CONFIG_CFG80211_REQUIRE_SIGNED_REGDB and any other open coded signed firmware bits which may get merged later. Luis > > +/** > > + * fw_lockdown_read_file - prevent loading of unsigned firmware > > + * @file: pointer to firmware > > + * @read_id: caller identifier > > + * > > + * Prevent loading of unsigned firmware in lockdown mode. > > That comment gives a highly misleading impression of what this function > does. > > > + */ > > +static int fw_lockdown_read_file(struct file *file, enum kernel_read_file_id id) >
On Tue, Apr 3, 2018 at 9:56 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote: > The biggest thing which has changed since then is that we decided to *not* > support or streamline generic firmware signing (non-IMA) for now for a few > reasons [0] [1] which are important to re-iterate as these are easy to forget, > and AFAICT not documented anywhere. And the URLs... [0] https://lkml.kernel.org/r/20171204195155.GU729@wotan.suse.de [1] https://lkml.kernel.org/r/20171207153209.5da771a9@alans-desktop Luis
diff --git a/security/Kconfig b/security/Kconfig index a4fa8b826039..6e7e5888f823 100644 --- a/security/Kconfig +++ b/security/Kconfig @@ -243,6 +243,7 @@ source security/tomoyo/Kconfig source security/apparmor/Kconfig source security/loadpin/Kconfig source security/yama/Kconfig +source security/fw_lockdown/Kconfig source security/integrity/Kconfig diff --git a/security/Makefile b/security/Makefile index 8c4a43e3d4e0..58852dee5e22 100644 --- a/security/Makefile +++ b/security/Makefile @@ -9,6 +9,7 @@ subdir-$(CONFIG_SECURITY_TOMOYO) += tomoyo subdir-$(CONFIG_SECURITY_APPARMOR) += apparmor subdir-$(CONFIG_SECURITY_YAMA) += yama subdir-$(CONFIG_SECURITY_LOADPIN) += loadpin +subdir-$(CONFIG_SECURITY_FW_LOCKDOWN) += fw_lockdown # always enable default capabilities obj-y += commoncap.o @@ -24,6 +25,7 @@ obj-$(CONFIG_SECURITY_TOMOYO) += tomoyo/ obj-$(CONFIG_SECURITY_APPARMOR) += apparmor/ obj-$(CONFIG_SECURITY_YAMA) += yama/ obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/ +obj-$(CONFIG_SECURITY_FW_LOCKDOWN) += fw_lockdown/ obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o # Object integrity file lists diff --git a/security/fw_lockdown/Kconfig b/security/fw_lockdown/Kconfig new file mode 100644 index 000000000000..d6aef6ce8fee --- /dev/null +++ b/security/fw_lockdown/Kconfig @@ -0,0 +1,6 @@ +config SECURITY_FW_LOCKDOWN + bool "Prevent loading unsigned firmware" + depends on LOCK_DOWN_KERNEL + default y + help + Prevent loading unsigned firmware in lockdown mode, diff --git a/security/fw_lockdown/Makefile b/security/fw_lockdown/Makefile new file mode 100644 index 000000000000..3a16757fd35d --- /dev/null +++ b/security/fw_lockdown/Makefile @@ -0,0 +1,3 @@ +obj-$(CONFIG_SECURITY_FW_LOCKDOWN) += fw_lockdown.o + +fw_lockdown-y := fw_lsm.o diff --git a/security/fw_lockdown/fw_lsm.c b/security/fw_lockdown/fw_lsm.c new file mode 100644 index 000000000000..cce03a5c5280 --- /dev/null +++ b/security/fw_lockdown/fw_lsm.c @@ -0,0 +1,51 @@ +/* + * fw_lockdown security module + * + * Copyright (C) 2017 IBM Corporation + * + * Authors: + * Mimi Zohar <zohar@linux.vnet.ibm.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#define pr_fmt(fmt) "fw_lockdown: " fmt + +#include <linux/module.h> +#include <linux/ima.h> +#include <linux/lsm_hooks.h> + +/** + * fw_lockdown_read_file - prevent loading of unsigned firmware + * @file: pointer to firmware + * @read_id: caller identifier + * + * Prevent loading of unsigned firmware in lockdown mode. + */ +static int fw_lockdown_read_file(struct file *file, enum kernel_read_file_id id) +{ + if (id == READING_FIRMWARE) { + if (!is_ima_appraise_enabled() && + !kernel_is_locked_down("Loading of unsigned firmware")) + return -EACCES; + } + return 0; +} + +static struct security_hook_list fw_lockdown_hooks[] = { + LSM_HOOK_INIT(kernel_read_file, fw_lockdown_read_file) +}; + +static int __init init_fw_lockdown(void) +{ + security_add_hooks(fw_lockdown_hooks, ARRAY_SIZE(fw_lockdown_hooks), + "fw_lockdown"); + pr_info("initialized\n"); + return 0; +} + +late_initcall(init_fw_lockdown); +MODULE_LICENSE("GPL");
If the kernel is locked down and IMA-appraisal is not enabled, prevent loading of unsigned firmware. Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com> --- Changelog v1: - Lots of minor changes Kconfig, Makefile, fw_lsm.c for such a small patch security/Kconfig | 1 + security/Makefile | 2 ++ security/fw_lockdown/Kconfig | 6 +++++ security/fw_lockdown/Makefile | 3 +++ security/fw_lockdown/fw_lsm.c | 51 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 63 insertions(+) create mode 100644 security/fw_lockdown/Kconfig create mode 100644 security/fw_lockdown/Makefile create mode 100644 security/fw_lockdown/fw_lsm.c