Re: [Fwd: [RFC PATCH v2] fw_lockdown: new micro LSM module to prevent loading unsigned firmware]

[Luis R. Rodriguez]

+ a few folks who might care + lkml.

On Mon, Nov 20, 2017 at 06:24:28PM -0500, Mimi Zohar wrote:
> Hi Matthew, David, Luis,
> 
> After Linus' comments I'm just not sure if it makes sense to re-post
> the patch.  To address Linus' comments, I could update the patch
> description as follows:
> 
>     Even if firmware comes signed from the manufacturer, the signature 
>     would be verified by the HW, not the kernel.

The concept that UEFI "kernel lockdown" implies that hardware requires
functionality on all or some peripherals which verifies firmware and mandates
that firmware is provided in a form which is signed was *news* to me. I'd like
to see this requirement carefully referenced, and think it would be good for us
then to *later* extend the documentation which David Howells is working on for
"kernel lockdown" to refer to these references.

Can someone provide a reference?

That said, even with this done, there are two circles of types of uses of
the firmware API that this UEFI requirement does not cover:

1) platforms which do not require UEFI, and some which *may* not require it but
perhaps want firmware signing. If such platforms are platforms which Red Hat or
SUSE care for, then the idea of signed firmware becomes a bit more sensible for
users of those platforms which may want also signed modules. If such folks
exist, they need to speak up. Otherwise I'd have to say we can live UEFI as a
compromise but also with IMA as a great alternative. Ie, we can shove IMA
down their throats.

I've frankly have grown tired of pushing firmware signing just for the sake of
the fact that I needed it for cfg80211, but now that its out of the way and
we open coded it, its no longer a requirement on my part. Folks who really
need this need to speak up or hold their piece for now, or just open code
the signing then for areas they need it, just as cfg80211 did for regulatory
data, or use IMA :)

I'll note that this UEFI requirement onto peripheral firmware signing *may*
mean that non-signed firmware, and the inherent latencies built-in to
revocation (or lack of such functionality) may be a detriment long term to
security and obviously software freedom, but this is not a new issue.

Revocation policies should be documented on the lock down documentation,
I didn't see that. Do have that well covered? Do we have revocation stuff
properly handled?

Not trusting the built-in keys on UEFI was an option, does that same knob
turn off this requirement on peripheral firmware verification?

2) There are uses for the firmware API for non-firmware, ie for data which
hardware won't use or cannot. The cfg80211 use for regulatory data was one
example, other uses are for EEPROM overrides and I'm certain there are
others. I'd say just open code those for now. cfg80211 already did it.

If we grow a list of users that do the same thing then maybe we can talk
about an API later. For now it doesn't make sense to me anymore given all
the above.

>  In some environments 
>     we might want to limit providing firmware, even firmware signed by
>     the manufacturer, to a specific instance.  Without IMA-appraisal
>     enabled, this is not possible. 

I think if you get what I put up above with references and indicate that
platforms / users that wish for similar enforcement can use IMA, that it
would be a good sell and alternative to UEFI + its theoretical implications
on peripheral hw verification.

> 
>     If the kernel is locked down and IMA-appraisal is not enabled, 
>     prevent loading of firmware.
> 
> Please let me know if you want me to re-post the patch with the two
> suggested changes and the above patch description.

Given all the above I think your patch makes sense still -- but IMHO the id tag
READING_FIRMWARE_PREALLOC_BUFFER should be considered as well *first*, and I
think one way forward is to evaluate removing it. The patch that added it was
not properly reviewed by stakeholders and last I looked while cleaning up this
firmware crap just recently [0] I could not identify the need for it [0].

[0] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20171117-firmware-flexible
Note: use commit aaec92fe3 ("test_firmware: test the 3 firmware kernel configs using debugfs")
as base for development, these patches were all posted for consideration
early for v4.16 now.

  Luis

> 
> thanks,
> 
> Mimi
> 
> -------- Forwarded Message --------
> From: Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx>
> To: David Howells <dhowells@xxxxxxxxxx>
> Cc: linux-integrity <linux-integrity@xxxxxxxxxxxxxxx>, linux-fsdevel <
> linux-fsdevel@xxxxxxxxxxxxxxx>, linux-kernel <linux-kernel@xxxxxxxxxxx
> .org>, Luis R. Rodriguez <mcgrof@xxxxxxxx>, "AKASHI, Takahiro" <takahi
> ro.akashi@xxxxxxxxxx>
> Subject: [RFC PATCH v2] fw_lockdown: new micro LSM module to prevent
> loading unsigned firmware
> Date: Mon, 13 Nov 2017 06:43:34 -0500
> 
> If the kernel is locked down and IMA-appraisal is not enabled, prevent
> loading of unsigned firmware.
> 
> Signed-off-by: Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx>
> ---
> Changelog v2:
> - Invert kernel_is_locked_down() test (Luis Rodriquez)
> - Increase LSM name maximum size (15 bytes + null) (Casey)
> 
> 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 +++++++++++++++++++++++++++++++++++++++++++
>  security/security.c           |  2 +-
>  6 files changed, 64 insertions(+), 1 deletion(-)
>  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..9a5472bc733f
> --- /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@xxxxxxxxxxxxxxxxxx>
> + *
> + * 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");
> diff --git a/security/security.c b/security/security.c
> index 4bf0f571b4ef..61a0c95ec687 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -32,7 +32,7 @@
>  #define MAX_LSM_EVM_XATTR	2
>  
>  /* Maximum number of letters for an LSM name string */
> -#define SECURITY_NAME_MAX	10
> +#define SECURITY_NAME_MAX	15
>  
>  struct security_hook_heads security_hook_heads __lsm_ro_after_init;
>  static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
> -- 
> 2.7.4
> 
> 

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg