[PATCH v2 1/9] ima: based on policy verify firmware signatures (pre-allocated buffer)
Don't differentiate between kernel_read_file_id READING_FIRMWARE and
READING_FIRMWARE_PREALLOC_BUFFER enumerations.
Fixes: a098ecd firmware: support loading into a pre-allocated buffer (since 4.8)
Signed-off-by: Mimi Zohar
Cc: Luis R. Rodriguez
Cc: David Howells
Cc: Kees Cook
Cc: Serge E. Hallyn
Cc: Stephen Boyd
---
security/integrity/ima/ima_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index e771b736aa03..83f84928ad76 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -447,6 +447,7 @@ int ima_read_file(struct file *file, enum
kernel_read_file_id read_id)
static int read_idmap[READING_MAX_ID] = {
[READING_FIRMWARE] = FIRMWARE_CHECK,
+ [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
[READING_MODULE] = MODULE_CHECK,
[READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
[READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
--
2.7.5
[PATCH v2 3/9] security: define security_kernel_read_blob() wrapper
In order for LSMs and IMA-appraisal to differentiate between the original
and new syscalls (eg. kexec, kernel modules, firmware), both the original
and new syscalls must call an LSM hook.
Commit 2e72d51b4ac3 ("security: introduce kernel_module_from_file hook")
introduced calling security_kernel_module_from_file() in both the original
and new syscalls. Commit a1db74209483 ("module: replace
copy_module_from_fd with kernel version") replaced these LSM calls with
security_kernel_read_file().
Commit e40ba6d56b41 ("firmware: replace call to fw_read_file_contents()
with kernel version") and commit b804defe4297 ("kexec: replace call to
copy_file_from_fd() with kernel version") replaced their own version of
reading a file from the kernel with the generic
kernel_read_file_from_path/fd() versions, which call the pre and post
security_kernel_read_file LSM hooks.
Missing are LSM calls in the original kexec syscall and firmware sysfs
fallback method. From a technical perspective there is no justification
for defining a new LSM hook, as the existing security_kernel_read_file()
works just fine. The original syscalls, however, do not read a file, so
the security hook name is inappropriate. Instead of defining a new LSM
hook, this patch defines security_kernel_read_blob() as a wrapper for
the existing LSM security_kernel_file_read() hook.
Signed-off-by: Mimi Zohar
Cc: Eric Biederman
Cc: Luis R. Rodriguez
Cc: Kees Cook
Cc: David Howells
Cc: Casey Schaufler
Changelog v2:
- Define a generic wrapper named security_kernel_read_blob() for
security_kernel_read_file().
Changelog v1:
- Define and call security_kexec_load(), a wrapper for
security_kernel_read_file().
---
include/linux/security.h | 6 ++
security/security.c | 6 ++
2 files changed, 12 insertions(+)
diff --git a/include/linux/security.h b/include/linux/security.h
index 63030c85ee19..4db1967a688b 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -323,6 +323,7 @@ int security_kernel_module_request(char *kmod_name);
int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
enum kernel_read_file_id id);
+int security_kernel_read_blob(enum kernel_read_file_id id);
int security_task_fix_setuid(struct cred *new, const struct cred *old,
int flags);
int security_task_setpgid(struct task_struct *p, pid_t pgid);
@@ -922,6 +923,11 @@ static inline int security_kernel_post_read_file(struct
file *file,
return 0;
}
+static inline int security_kernel_read_blob(enum kernel_read_file_id id)
+{
+ return 0;
+}
+
static inline int security_task_fix_setuid(struct cred *new,
const struct cred *old,
int flags)
diff --git a/security/security.c b/security/security.c
index 68f46d849abe..8f199b2bf4a2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1044,6 +1044,12 @@ int security_kernel_read_file(struct file *file, enum
kernel_read_file_id id)
}
EXPORT_SYMBOL_GPL(security_kernel_read_file);
+int security_kernel_read_blob(enum kernel_read_file_id id)
+{
+ return security_kernel_read_file(NULL, id);
+}
+EXPORT_SYMBOL_GPL(security_kernel_read_blob);
+
int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
enum kernel_read_file_id id)
{
--
2.7.5
[PATCH v2 4/9] kexec: add call to LSM hook in original kexec_load syscall
In order for LSMs and IMA-appraisal to differentiate between the
original and new syscalls, both the original and new syscalls must call
an LSM hook. This patch adds a call to security_kernel_read_blob() in
the original kexec syscall.
Signed-off-by: Mimi Zohar
Cc: Eric Biederman
Cc: Luis R. Rodriguez
Cc: Kees Cook
Cc: David Howells
---
kernel/kexec.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/kernel/kexec.c b/kernel/kexec.c
index aed8fb2564b3..64cebe92a690 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -195,11 +196,21 @@ static int do_kexec_load(unsigned long entry, unsigned
long nr_segments,
static inline int kexec_load_check(unsigned long nr_segments,
unsigned long flags)
{
+ int result;
+
/* We only trust the superuser with rebooting the system. */
if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
return -EPERM;
/*
+* Allow LSMs and IMA to differentiate between kexec_load and
+* kexec_file_load syscalls.
+*/
+ result = security_kernel_read_blob(READING_KEXEC_IMAGE);
+ if (result < 0)
+ return result;
+
+ /*
* Verify we have a legal set of flags
* This leaves us room for future extensions.
*/
--
2.7.5
[PATCH v2 9/9] ima: based on policy prevent loading firmware (pre-allocated buffer)
Question: can the device access the pre-allocated buffer at any time?
By allowing devices to request firmware be loaded directly into a
pre-allocated buffer, will this allow the device access to the firmware
before the kernel has verified the firmware signature?
Is it dependent on the type of buffer allocated (eg. DMA)? For example,
qcom_mdt_load() -> qcom_scm_pas_init_image() -> dma_alloc_coherent().
With an IMA policy requiring signed firmware, this patch would prevent
loading firmware into a pre-allocated buffer.
Signed-off-by: Mimi Zohar
Cc: Luis R. Rodriguez
Cc: David Howells
Cc: Kees Cook
Cc: Serge E. Hallyn
Cc: Stephen Boyd
---
security/integrity/ima/ima_main.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 29d1a929af5c..6224468845e6 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -452,6 +452,15 @@ int ima_read_file(struct file *file, enum
kernel_read_file_id read_id)
return 0;
}
+ if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
+ if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
+ (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+ pr_err("Prevent device from accessing firmware prior to
verifying the firmware signature.\n");
+ return -EACCES;
+ }
+ return 0;
+ }
+
if (read_id == READING_FIRMWARE_FALLBACK_SYSFS) {
if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
(ima_appraise & IMA_APPRAISE_ENFORCE)) {
--
2.7.5
[PATCH v2 8/9] ima: add build time policy
IMA by default does not measure, appraise or audit files, but can be
enabled at runtime by specifying a builtin policy on the boot command line
or by loading a custom policy.
This patch defines a build time policy, which verifies kernel modules,
firmware, kexec image, and/or the IMA policy signatures. This build time
policy is automatically enabled at runtime. The build time policy rules
persist after loading a custom policy.
Signed-off-by: Mimi Zohar
---
security/integrity/ima/Kconfig | 58 +
security/integrity/ima/ima_policy.c | 46 +++--
2 files changed, 101 insertions(+), 3 deletions(-)
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 6a8f67714c83..004919d9bf09 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -156,6 +156,64 @@ config IMA_APPRAISE
<http://linux-ima.sourceforge.net>
If unsure, say N.
+config IMA_APPRAISE_BUILD_POLICY
+ bool "IMA build time configured policy rules"
+ depends on IMA_APPRAISE && INTEGRITY_ASYMMETRIC_KEYS
+ default n
+ help
+ This option defines an IMA appraisal policy at build time, which
+ is enforced at run time without having to specify a builtin
+ policy name on the boot command line. The build time appraisal
+ policy rules persist after loading a custom policy.
+
+ Depending on the rules configured, this policy may require kernel
+ modules, firmware, the kexec kernel image, and/or the IMA policy
+ to be signed. Unsigned files might prevent the system from
+ booting or applications from working properly.
+
+config IMA_APPRAISE_REQUIRE_FIRMWARE_SIGS
+ bool "Appraise firmware signatures"
+ depends on IMA_APPRAISE_BUILD_POLICY
+ default n
+ help
+ This option defines a policy requiring all firmware to be signed,
+ including the regulatory.db. If both this option and
+ CFG80211_REQUIRE_SIGNED_REGDB are enabled, then both signature
+ verification methods are necessary.
+
+config IMA_APPRAISE_REQUIRE_KEXEC_SIGS
+ bool "Appraise kexec kernel image signatures"
+ depends on IMA_APPRAISE_BUILD_POLICY
+ default n
+ help
+ Enabling this rule will require all kexec'ed kernel images to
+ be signed and verified by a public key on the trusted IMA
+ keyring.
+
+ Kernel image signatures can not be verified by the original
+ kexec_load syscall. Enabling this rule will prevent its
+ usage.
+
+config IMA_APPRAISE_REQUIRE_MODULE_SIGS
+ bool "Appraise kernel modules signatures"
+ depends on IMA_APPRAISE_BUILD_POLICY
+ default n
+ help
+ Enabling this rule will require all kernel modules to be signed
+ and verified by a public key on the trusted IMA keyring.
+
+ Kernel module signatures can only be verified by IMA-appraisal,
+ via the finit_module syscall. Enabling this rule will prevent
+ the usage of the init_module syscall.
+
+config IMA_APPRAISE_REQUIRE_POLICY_SIGS
+ bool "Appraise IMA policy signature"
+ depends on IMA_APPRAISE_BUILD_POLICY
+ default n
+ help
+ Enabling this rule will require the IMA policy to be signed and
+ and verified by a key on the trusted IMA keyring.
+
config IMA_APPRAISE_BOOTPARAM
bool "ima_appraise boot parameter"
depends on IMA_APPRAISE
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index c27f6993b07a..3c0bc8a1a88e 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -49,6 +49,7 @@
int ima_policy_flag;
static int temp_ima_appraise;
+static int build_ima_appraise __ro_after_init;
#define MAX_LSM_RULES 6
enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
@@ -162,6 +163,25 @@ static struct ima_rule_entry default_appraise_rules[]
__ro_after_init = {
#endif
};
+static struct ima_rule_entry build_appraise_rules[] __ro_after_init = {
+#ifdef CONFIG_IMA_APPRAISE_REQUIRE_MODULE_SIGS
+ {.action = APPRAISE, .func = MODULE_CHECK,
+.flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+#endif
+#ifdef CONFIG_IMA_APPRAISE_REQUIRE_FIRMWARE_SIGS
+ {.action = APPRAISE, .func = FIRMWARE_CHECK,
+.flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+#endif
+#ifdef CONFIG_IMA_APPRAISE_REQUIRE_KEXEC_SIGS
+ {.action = APPRAISE, .func = KEXEC_KERNEL_CHECK,
+.flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+#endif
+#ifdef CONFIG_IMA_APPRAISE_REQUIRE_POLICY_SIGS
+ {.action = APPRAISE, .func = POLICY_CHECK,
+.flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+#endif
+};
+
static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
{.action = APPRAISE, .func = MODULE_CHE
[PATCH v2 6/9] firmware: add call to LSM hook before firmware sysfs fallback
Add an LSM hook prior to allowing firmware sysfs fallback loading.
Signed-off-by: Mimi Zohar
Cc: Luis R. Rodriguez
Cc: David Howells
Cc: Kees Cook
Changelog:
- call security_kernel_read_blob()
- rename the READING_FIRMWARE_FALLBACK kernel_read_file_id enumeration to
READING_FIRMWARE_FALLBACK_SYSFS.
---
drivers/base/firmware_loader/fallback.c | 7 +++
include/linux/fs.h | 1 +
2 files changed, 8 insertions(+)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index 358354148dec..6ec97e19c017 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -651,6 +651,8 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
static bool fw_run_sysfs_fallback(unsigned int opt_flags)
{
+ int ret;
+
if (fw_fallback_config.ignore_sysfs_fallback) {
pr_info_once("Ignoring firmware sysfs fallback due to sysctl
knob\n");
return false;
@@ -659,6 +661,11 @@ static bool fw_run_sysfs_fallback(unsigned int opt_flags)
if ((opt_flags & FW_OPT_NOFALLBACK))
return false;
+ /* Also permit LSMs and IMA to fail firmware sysfs fallback */
+ ret = security_kernel_read_blob(READING_FIRMWARE_FALLBACK_SYSFS);
+ if (ret < 0)
+ return ret;
+
return fw_force_sysfs_fallback(opt_flags);
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 760d8da1b6c7..6e31d9207435 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2810,6 +2810,7 @@ extern int do_pipe_flags(int *, int);
id(UNKNOWN, unknown)\
id(FIRMWARE, firmware) \
id(FIRMWARE_PREALLOC_BUFFER, firmware) \
+ id(FIRMWARE_FALLBACK_SYSFS, firmware) \
id(MODULE, kernel-module) \
id(KEXEC_IMAGE, kexec-image)\
id(KEXEC_INITRAMFS, kexec-initramfs)\
--
2.7.5
[PATCH v2 7/9] ima: based on policy require signed firmware (sysfs fallback)
With an IMA policy requiring signed firmware, this patch prevents
the sysfs fallback method of loading firmware.
Signed-off-by: Mimi Zohar
Cc: Luis R. Rodriguez
Cc: David Howells
Cc: Matthew Garrett
---
security/integrity/ima/ima_main.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 7e1a127f18fe..29d1a929af5c 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -451,7 +451,17 @@ int ima_read_file(struct file *file, enum
kernel_read_file_id read_id)
}
return 0;
}
+
+ if (read_id == READING_FIRMWARE_FALLBACK_SYSFS) {
+ if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
+ (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+ pr_err("Prevent firmware sysfs fallback loading.\n");
+ return -EACCES;
+ }
+ return 0;
+ }
return 0;
+
}
static int read_idmap[READING_MAX_ID] = {
--
2.7.5
[PATCH v2 0/9] kexec/firmware: support system wide policy requiring signatures
IMA-appraisal is mostly being used in the embedded or single purpose closed system environments. In these environments, both the Kconfig options and the userspace tools can be modified appropriately to limit syscalls. For stock kernels, userspace applications need to continue to work with older kernels as well as with newer kernels. In this environment, the customer needs the ability to define a system wide IMA policy, such as requiring all kexec'ed images, firmware, kernel modules to be signed, without being dependent on either the Kconfig options or the userspace tools.[1] This patch set allows the customer to define a policy which requires the kexec'ed kernel images, firmware, and/or kernel modules to be signed. New to this patch set is the ability to configure a build time IMA policy, which is automatically loaded at run time without needing to specify it on the boot command line. The build time policy rules persist after loading a custom kernel policy. [1] kexec-tools suupports the new syscall based on a flag (-s). Changelog v3: - combined "kexec: limit kexec_load syscall" and "firmware: kernel signature verification" patch sets. - add support for build time policy. - defined generic security_kernel_read_blob() wrapper for security_kernel_read_file(). Suggested by Luis. - removed the CONFIG_CFG80211_REQUIRE_SIGNED_REGDB ifdef. If both REGDB and an IMA-appraisal policy require signed firmware, for now require both signatures. Subsequent patches might change this. - Still unclear if the pre-allocated firmware buffer can be accessed prior to the signature verification completes. Mimi Zohar (9): ima: based on policy verify firmware signatures (pre-allocated buffer) ima: fix updating the ima_appraise flag security: define security_kernel_read_blob() wrapper kexec: add call to LSM hook in original kexec_load syscall ima: based on policy require signed kexec kernel images firmware: add call to LSM hook before firmware sysfs fallback ima: based on policy require signed firmware (sysfs fallback) ima: add build time policy ima: based on policy prevent loading firmware (pre-allocated buffer) drivers/base/firmware_loader/fallback.c | 7 +++ include/linux/fs.h | 1 + include/linux/security.h| 6 +++ kernel/kexec.c | 11 + security/integrity/ima/Kconfig | 58 + security/integrity/ima/ima.h| 1 + security/integrity/ima/ima_main.c | 29 + security/integrity/ima/ima_policy.c | 76 +++-- security/security.c | 6 +++ 9 files changed, 183 insertions(+), 12 deletions(-) -- 2.7.5
Re: [PATCH] ima: Fix pr_fmt() redefinition
Hi Petr,
On Thu, 2018-05-17 at 12:47 +0200, Petr Vorel wrote:
> Previous definition was too late and caused problems in powerpc allyesconfig:
> security/integrity/ima/ima_kexec.c:18:0: warning: "pr_fmt" redefined
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> In file included from include/linux/kernel.h:14:0,
> from include/asm-generic/bug.h:18,
> from arch/powerpc/include/asm/bug.h:128,
> from include/linux/bug.h:5,
> from include/linux/seq_file.h:7,
> from security/integrity/ima/ima_kexec.c:13:
> include/linux/printk.h:288:0: note: this is the location of the previous
> definition
> #define pr_fmt(fmt) fmt
>
> Fixes: 3dea0d93d257 ("ima: Unify logging")
>
> Signed-off-by: Petr Vorel
> Reported-by: Stephen Rothwell
As the original patch hasn't been upstreamed yet, I'll squash this fix
with the original patch.
thanks!
Mimi
> ---
> security/integrity/ima/ima_kexec.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/ima/ima_kexec.c
> b/security/integrity/ima/ima_kexec.c
> index db0de585f6c0..16bd18747cfa 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -10,13 +10,13 @@
> * the Free Software Foundation; either version 2 of the License, or
> * (at your option) any later version.
> */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> #include
> #include
> #include
> #include "ima.h"
>
> -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> -
> #ifdef CONFIG_IMA_KEXEC
> static int ima_dump_measurement_list(unsigned long *buffer_size, void
> **buffer,
>unsigned long segment_size)
Re: [RFC PATCH v4 3/5] ima: differentiate auditing policy rules from "audit" actions
On Wed, 2018-05-16 at 16:28 -0400, Stefan Berger wrote: > On 05/15/2018 09:40 AM, Mimi Zohar wrote: > > Hi Stefan, > > > > On Fri, 2018-05-11 at 10:42 -0400, Stefan Berger wrote: > >> From: Mimi Zohar > >> > >> The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and > >> the IMA "audit" policy action. This patch defines AUDIT_INTEGRITY_POLICY > >> to reflect the IMA policy rules. > >> > >> Signed-off-by: Mimi Zohar > > We do need to separate out auditing the IMA policy rules from the > > "IMA-audit" messages. Based on the IMA policy rule aspect of the > > discussions [1], I would really appreciate if you could work with > > Richard and Steve on the new IMA policy rule audit format. > Is your patch below still valid for splitting it up into 'two distinct > audit record types' ? We need to separate the IMA policy audit rules from the IMA-audit messages. As we're changing the audit numbers, we need to take into account Richard's and Steve's comments about the IMA policy record format at the same time. This patch is incomplete and needs to address their comments. Mimi > > > > This change can be upstreamed independently of either the IMA > > namespacing or the audit containerid patch sets. The sooner we make > > this change and upstream it, the better. > > > > [1] https://www.redhat.com/archives/linux-audit/2018-March/msg00092.html > > > > thanks, > > > > Mimi > > > >> --- > >> include/uapi/linux/audit.h | 3 ++- > >> security/integrity/ima/ima_policy.c | 2 +- > >> 2 files changed, 3 insertions(+), 2 deletions(-) > >> > >> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > >> index 4e61a9e05132..8966e7ff1c4c 100644 > >> --- a/include/uapi/linux/audit.h > >> +++ b/include/uapi/linux/audit.h > >> @@ -146,7 +146,8 @@ > >> #define AUDIT_INTEGRITY_STATUS 1802 /* Integrity enable status */ > >> #define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ > >> #define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */ > >> -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ > >> +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy > >> msgs */ > >> +#define AUDIT_INTEGRITY_POLICY1806 /* IMA policy rules */ > >> > >> #define AUDIT_KERNEL 2000/* Asynchronous audit record. > >> NOT A REQUEST. */ > >> > >> diff --git a/security/integrity/ima/ima_policy.c > >> b/security/integrity/ima/ima_policy.c > >> index 915f5572c6ff..3a1412db02a3 100644 > >> --- a/security/integrity/ima/ima_policy.c > >> +++ b/security/integrity/ima/ima_policy.c > >> @@ -619,7 +619,7 @@ static int ima_parse_rule(char *rule, struct > >> ima_rule_entry *entry) > >>bool uid_token; > >>int result = 0; > >> > >> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE); > >> + ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_POLICY); > >> > >>entry->uid = INVALID_UID; > >>entry->fowner = INVALID_UID; > >
Re: [RFC PATCH v4 3/5] ima: differentiate auditing policy rules from "audit" actions
Hi Stefan, On Fri, 2018-05-11 at 10:42 -0400, Stefan Berger wrote: > From: Mimi Zohar > > The AUDIT_INTEGRITY_RULE is used for auditing IMA policy rules and > the IMA "audit" policy action. This patch defines AUDIT_INTEGRITY_POLICY > to reflect the IMA policy rules. > > Signed-off-by: Mimi Zohar We do need to separate out auditing the IMA policy rules from the "IMA-audit" messages. Based on the IMA policy rule aspect of the discussions [1], I would really appreciate if you could work with Richard and Steve on the new IMA policy rule audit format. This change can be upstreamed independently of either the IMA namespacing or the audit containerid patch sets. The sooner we make this change and upstream it, the better. [1] https://www.redhat.com/archives/linux-audit/2018-March/msg00092.html thanks, Mimi > --- > include/uapi/linux/audit.h | 3 ++- > security/integrity/ima/ima_policy.c | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > index 4e61a9e05132..8966e7ff1c4c 100644 > --- a/include/uapi/linux/audit.h > +++ b/include/uapi/linux/audit.h > @@ -146,7 +146,8 @@ > #define AUDIT_INTEGRITY_STATUS 1802 /* Integrity enable status */ > #define AUDIT_INTEGRITY_HASH 1803 /* Integrity HASH type */ > #define AUDIT_INTEGRITY_PCR 1804 /* PCR invalidation msgs */ > -#define AUDIT_INTEGRITY_RULE 1805 /* policy rule */ > +#define AUDIT_INTEGRITY_RULE 1805 /* IMA "audit" action policy msgs */ > +#define AUDIT_INTEGRITY_POLICY 1806 /* IMA policy rules */ > > #define AUDIT_KERNEL 2000/* Asynchronous audit record. NOT A > REQUEST. */ > > diff --git a/security/integrity/ima/ima_policy.c > b/security/integrity/ima/ima_policy.c > index 915f5572c6ff..3a1412db02a3 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -619,7 +619,7 @@ static int ima_parse_rule(char *rule, struct > ima_rule_entry *entry) > bool uid_token; > int result = 0; > > - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE); > + ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_POLICY); > > entry->uid = INVALID_UID; > entry->fowner = INVALID_UID;
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Tue, 2018-05-15 at 08:32 -0400, Josh Boyer wrote: > One aspect that was always a concern to some is whether the firmware files > were modified directly to have the signature attached to them. That may > run afoul of the "no modification" license that most blobs are shipped > under. Does IMA have the signatures for the files stored in xattrs or in > some other detached manner? They're stored as xattrs. RPM has support for including file signatures in the RPM header. Mimi
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Mon, 2018-05-14 at 19:28 +, Luis R. Rodriguez wrote:
[...]
> > 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.
>
> OK so you're saying that if CONFIG_IMA_APPRAISE_FIRMWARE is disabled you can
> still end up with CONFIG_CFG80211_REQUIRE_SIGNED_REGDB as enabled *and* a
> custom policy which requires IMA-appraisal for the certain firmware signature
> verifications?
Right
> > There are two problems:
> > - there's no way of configuring a builtin policy to verify firmware
> > signatures.
>
> I'm not too familiar with IMA however it sounds like you can extend the IMA
> built-in policy on the boot command line.
No, there are a couple of policies predefined in the kernel that can
be loaded by specifying them on the boot command line. A custom
policy can be loaded later. Only after specifying a policy on the
boot command line or loading a custom policy, does IMA do anything.
> > - 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.
>
> Sure, it is still unclear to me if CONFIG_IMA_APPRAISE_FIRMWARE will be
> doing firmware verification in userspace or in the kernel.
The kernel is verifying signatures.
> > 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).
>
> Oh I agree, Linux enterprise distributions also have a strong reason to
> have this, so that for instance we only trust and run vendor-approved
> signed firmware. Otherwise the driver should reject the firmware. Every
> now and then enterprise distros may run into cases were certain customers
> may run oddball firmwares, and its unclear if we expect proper functionality
> with that firmware. Having some form of firmware signing would help with
> this pipeline, but this is currently dealt with at the packaging, and
> noting other than logs ensures the driver is using an intended firmware.
> But these needs *IMHO* have not been enough to push to generalize a kernel
> firmware signing facility.
In order for IMA-appraisal to verify firmware signatures, the
signatures need to be distributed with the firmware. Perhaps this
will be enough of an incentive for distros to start including firmware
signatures in the packages.
> If CONFIG_IMA_APPRAISE_FIRMWARE is going to provide this functionality somehow
> I'm happy to hear it.
The functionality has been there since commit 5a9196d ("ima: add
support for measuring and appraising firmware"). The
security_kernel_fw_from_file() hook was later replaced with the
generic security_kernel_read_file() hook.
Mimi
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Fri, 2018-05-11 at 21:52 +, 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 +, 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 +, 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(f
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Thu, 2018-05-10 at 23:26 +, 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 +, 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
Re: [PATCH v2 2/4] tpm: Move eventlog files to a subdirectory
On Wed, 2018-05-09 at 16:51 +0200, Petr Vorel wrote: > Hi Mimi, > > > [Cc'ing Petr Vorel and the ltp mailing list] > > > Hi Jarrko, > > > On Fri, 2018-04-20 at 08:39 +0300, Jarkko Sakkinen wrote: > > > On Thu, Apr 12, 2018 at 12:13:48PM +0200, Thiebaud Weksteen wrote: > > > > Signed-off-by: Thiebaud Weksteen > > > > Suggested-by: Jarkko Sakkinen > > > > Reviewed-by: Jarkko Sakkinen > > > Tested-by: Jarkko Sakkinen > > > I just noticed this is queued in your next branch. > > > Petr Vorel has been updating the IMA LTP tests. One of those IMA LTP > > tests includes walking the TPM binary_runtime_measurements in order to > > calculate the IMA boot-aggregate. The IMA boot-aggregate is the first > > measurement in the IMA measurement list. > > Did you meant that this commit ([2]) in linux-tpmdd/next changed location of > binary_runtime_measurements in sysfs? > > IMHO these commits ([1], [2], [3]) just put source code into eventlog/ > subdirectory: I mistakenly thought the eventlog itself was being moved, not the source code. thanks, Mimi > > Kind regards, > Petr > > [1] 75d647f5de69 tpm: Move eventlog declarations to its own header > [2] 9b01b5356629 tpm: Move shared eventlog functions to common.c > [3] 0bfb23746052 tpm: Move eventlog files to a subdirectory >
[PATCH 1/3] ima: based on the "secure_boot" policy limit syscalls
The builtin "secure_boot" policy adds IMA appraisal rules requiring kernel
modules (finit_module syscall), direct firmware load, kexec kernel image
(kexec_file_load syscall), and the IMA policy to be signed, but did not
prevent the other syscalls/methods from working. Loading an equivalent
custom policy containing these same rules would have prevented the other
syscalls/methods from working.
This patch refactors the code to load custom policies, defining a new
function named ima_appraise_flag(). The new function is called either
when loading the builtin "secure_boot" or custom policies.
Fixes: 503ceaef8e2e ("ima: define a set of appraisal rules requiring file
signatures")
Signed-off-by: Mimi Zohar
---
security/integrity/ima/ima_policy.c | 25 ++---
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index 03cbba423e59..df3e45878a87 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -440,6 +440,17 @@ void ima_update_policy_flag(void)
ima_policy_flag &= ~IMA_APPRAISE;
}
+static int ima_appraise_flag(enum ima_hooks func)
+{
+ if (func == MODULE_CHECK)
+ return IMA_APPRAISE_MODULES;
+ else if (func == FIRMWARE_CHECK)
+ return IMA_APPRAISE_FIRMWARE;
+ else if (func == POLICY_CHECK)
+ return IMA_APPRAISE_POLICY;
+ return 0;
+}
+
/**
* ima_init_policy - initialize the default measure rules.
*
@@ -478,9 +489,12 @@ void __init ima_init_policy(void)
* Insert the appraise rules requiring file signatures, prior to
* any other appraise rules.
*/
- for (i = 0; i < secure_boot_entries; i++)
+ for (i = 0; i < secure_boot_entries; i++) {
list_add_tail(&secure_boot_rules[i].list,
&ima_default_rules);
+ temp_ima_appraise |=
+ ima_appraise_flag(secure_boot_rules[i].func);
+ }
for (i = 0; i < appraise_entries; i++) {
list_add_tail(&default_appraise_rules[i].list,
@@ -934,12 +948,9 @@ static int ima_parse_rule(char *rule, struct
ima_rule_entry *entry)
}
if (!result && (entry->action == UNKNOWN))
result = -EINVAL;
- else if (entry->func == MODULE_CHECK)
- temp_ima_appraise |= IMA_APPRAISE_MODULES;
- else if (entry->func == FIRMWARE_CHECK)
- temp_ima_appraise |= IMA_APPRAISE_FIRMWARE;
- else if (entry->func == POLICY_CHECK)
- temp_ima_appraise |= IMA_APPRAISE_POLICY;
+ else if (entry->action == APPRAISE)
+ temp_ima_appraise |= ima_appraise_flag(entry->func);
+
audit_log_format(ab, "res=%d", !result);
audit_log_end(ab);
return result;
--
2.7.5
[PATCH 0/3] kexec: limit kexec_load syscall
IMA-appraisal is mostly being used in the embedded or single purpose closed system environments. In these environments, both the Kconfig options and the userspace tools can be modified appropriately to limit syscalls. For stock kernels, userspace applications need to continue to work with older kernels as well as with newer kernels. In this environment, the customer needs the ability to define a system wide IMA runtime policy, such as requiring all kexec'ed images (or firmware) to be signed, without being dependent on either the Kconfig options or the userspace tools. This patch set allows the customer to define a policy which requires kexec'ed kernels to be signed. Mimi Zohar (3): ima: based on the "secure_boot" policy limit syscalls kexec: call LSM hook for kexec_load syscall ima: based on policy require signed kexec kernel images include/linux/security.h| 6 ++ kernel/kexec.c | 11 +++ security/integrity/ima/ima.h| 1 + security/integrity/ima/ima_main.c | 9 + security/integrity/ima/ima_policy.c | 27 --- security/security.c | 6 ++ 6 files changed, 53 insertions(+), 7 deletions(-) -- 2.7.5
[PATCH 3/3] ima: based on policy require signed kexec kernel images
The original kexec_load syscall can not verify file signatures. This
patch differentiates between the kexec_load and kexec_file_load
syscalls.
Signed-off-by: Mimi Zohar
Cc: Eric Biederman
Cc: David Howells
Cc: Kees Cook
Cc: Matthew Garrett
---
security/integrity/ima/ima.h| 1 +
security/integrity/ima/ima_main.c | 9 +
security/integrity/ima/ima_policy.c | 2 ++
3 files changed, 12 insertions(+)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 35fe91aa1fc9..03c9c37ee345 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -233,6 +233,7 @@ int ima_policy_show(struct seq_file *m, void *v);
#define IMA_APPRAISE_MODULES 0x08
#define IMA_APPRAISE_FIRMWARE 0x10
#define IMA_APPRAISE_POLICY0x20
+#define IMA_APPRAISE_KEXEC 0x40
#ifdef CONFIG_IMA_APPRAISE
int ima_appraise_measurement(enum ima_hooks func,
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 74d0bd7e76d7..754ece08e1c6 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -444,6 +444,15 @@ int ima_read_file(struct file *file, enum
kernel_read_file_id read_id)
}
return 0; /* We rely on module signature checking */
}
+
+ if (!file && read_id == READING_KEXEC_IMAGE) {
+ if ((ima_appraise & IMA_APPRAISE_KEXEC) &&
+ (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+ pr_err("impossible to appraise a kernel image without a
file descriptor; try using kexec_file syscall.\n");
+ return -EACCES; /* INTEGRITY_UNKNOWN */
+ }
+ return 0;
+ }
return 0;
}
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index df3e45878a87..a9e96a884867 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -448,6 +448,8 @@ static int ima_appraise_flag(enum ima_hooks func)
return IMA_APPRAISE_FIRMWARE;
else if (func == POLICY_CHECK)
return IMA_APPRAISE_POLICY;
+ else if (func == KEXEC_KERNEL_CHECK)
+ return IMA_APPRAISE_KEXEC;
return 0;
}
--
2.7.5
[PATCH 2/3] kexec: call LSM hook for kexec_load syscall
In order for LSMs and IMA-appraisal to differentiate between the
kexec_load and kexec_file_load_syscalls, an LSM call needs to be added
to the original kexec_load syscall. From a technical perspective there
is no need for defining a new LSM hook, as the existing
security_kernel_kexec_load() works just fine. However, the name is
confusing. For this reason, instead of defining a new LSM hook, this
patch defines security_kexec_load() as a wrapper for the existing LSM
security_kernel_file_read() hook.
Signed-off-by: Mimi Zohar
Cc: Eric Biederman
Cc: Kees Cook
Cc: David Howells
Cc: Matthew Garrett
Cc: Casey Schaufler
Changelog v1:
- Define and call security_kexec_load(), a wrapper for
security_kernel_read_file().
---
include/linux/security.h | 6 ++
kernel/kexec.c | 11 +++
security/security.c | 6 ++
3 files changed, 23 insertions(+)
diff --git a/include/linux/security.h b/include/linux/security.h
index 63030c85ee19..26f6d85903ed 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -323,6 +323,7 @@ int security_kernel_module_request(char *kmod_name);
int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
enum kernel_read_file_id id);
+int security_kexec_load(void);
int security_task_fix_setuid(struct cred *new, const struct cred *old,
int flags);
int security_task_setpgid(struct task_struct *p, pid_t pgid);
@@ -922,6 +923,11 @@ static inline int security_kernel_post_read_file(struct
file *file,
return 0;
}
+static inline int security_kexec_load(void)
+{
+ return 0;
+}
+
static inline int security_task_fix_setuid(struct cred *new,
const struct cred *old,
int flags)
diff --git a/kernel/kexec.c b/kernel/kexec.c
index aed8fb2564b3..6b44b0e9a60b 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -195,11 +196,21 @@ static int do_kexec_load(unsigned long entry, unsigned
long nr_segments,
static inline int kexec_load_check(unsigned long nr_segments,
unsigned long flags)
{
+ int result;
+
/* We only trust the superuser with rebooting the system. */
if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
return -EPERM;
/*
+* Allow LSMs and IMA to differentiate between kexec_load and
+* kexec_file_load syscalls.
+*/
+ result = security_kexec_load();
+ if (result < 0)
+ return result;
+
+ /*
* Verify we have a legal set of flags
* This leaves us room for future extensions.
*/
diff --git a/security/security.c b/security/security.c
index 68f46d849abe..0f339156 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1044,6 +1044,12 @@ int security_kernel_read_file(struct file *file, enum
kernel_read_file_id id)
}
EXPORT_SYMBOL_GPL(security_kernel_read_file);
+int security_kexec_load()
+{
+ return security_kernel_read_file(NULL, READING_KEXEC_IMAGE);
+}
+EXPORT_SYMBOL_GPL(security_kexec_load);
+
int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
enum kernel_read_file_id id)
{
--
2.7.5
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Wed, 2018-05-09 at 23:48 +, 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 +, 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/msg5.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 >
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Wed, 2018-05-09 at 21:22 +, 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 +, 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
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Wed, 2018-05-09 at 19:15 +, 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
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Tue, 2018-05-08 at 17:34 +, 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 +, 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 > > > > Cc: Luis R. Rodriguez > > > > Cc: David Howells > > > > Cc: Kees Cook > > > > Cc: Seth Forshee > > > > Cc: Johannes Berg > > > > --- > > > > 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_
Re: [PATCH v2 2/4] tpm: Move eventlog files to a subdirectory
[Cc'ing Petr Vorel and the ltp mailing list] Hi Jarrko, On Fri, 2018-04-20 at 08:39 +0300, Jarkko Sakkinen wrote: > On Thu, Apr 12, 2018 at 12:13:48PM +0200, Thiebaud Weksteen wrote: > > Signed-off-by: Thiebaud Weksteen > > Suggested-by: Jarkko Sakkinen > > Reviewed-by: Jarkko Sakkinen > Tested-by: Jarkko Sakkinen I just noticed this is queued in your next branch. Petr Vorel has been updating the IMA LTP tests. One of those IMA LTP tests includes walking the TPM binary_runtime_measurements in order to calculate the IMA boot-aggregate. The IMA boot-aggregate is the first measurement in the IMA measurement list. Mimi
Re: [PATCH 0/3] kexec: limit kexec_load syscall
On Thu, 2018-05-03 at 18:03 -0500, Eric W. Biederman wrote: > Mimi Zohar writes: > > > On Thu, 2018-05-03 at 16:38 -0500, Eric W. Biederman wrote: > >> Mimi Zohar writes: > >> > >> > [Cc'ing Kees and kernel-hardening] > >> > > >> > On Thu, 2018-05-03 at 15:13 -0500, Eric W. Biederman wrote: > >> >> Mimi Zohar writes: > >> >> > >> >> > In environments that require the kexec kernel image to be signed, > >> >> > prevent > >> >> > using the kexec_load syscall. In order for LSMs and IMA to > >> >> > differentiate > >> >> > between kexec_load and kexec_file_load syscalls, this patch set adds a > >> >> > call to security_kernel_read_file() in kexec_load_check(). > >> >> > >> >> Having thought about it some more this justification for these changes > >> >> does not work. The functionality of kexec_load is already root-only. > >> >> So in environments that require the kernel image to be signed just don't > >> >> use kexec_load. Possibly even compile kexec_load out to save space > >> >> because you will never need it. You don't need a new security hook to > >> >> do any of that. Userspace is a very fine mechanism for being the > >> >> instrument of policy. > >> > > >> > True, for those building their own kernel, they can disable the old > >> > syscalls. The concern is not for those building their own kernels, > >> > but for those using stock kernels. > >> > > >> > By adding an LSM hook here in the kexec_load syscall, as opposed to an > >> > IMA specific hook, other LSMs can piggy back on top of it. Currently, > >> > both load_pin and SELinux are gating the kernel module syscalls based > >> > on security_kernel_read_file. > >> > > >> > If there was a similar option for the kernel image, I'm pretty sure > >> > other LSMs would use it. > >> > > >> > From an IMA perspective, there needs to be some method for only > >> > allowing signed code to be loaded, executed, etc. - kernel modules, > >> > kernel image/initramfs, firmware, policies. > >> > >> What is the IMA perspective. Why can't IMA trust appropriately > >> authorized userspace? > > > > Suppose a system owner wants to define a system wide policy that > > requires all code be signed - kernel modules, firmware, kexec image & > > initramfs, executables, mmapped files, etc - without having to rebuild > > the kernel. Without a call in kexec_load that isn't possible. > > Of course it is. You just make it a requirement that before an > executable will be signed it will be audited to see that it doesn't > call sys_kexec_load. Signing presumably means something. So it should > not be hard to enforce a policy like that on a specialty system call > that most applications will never call. Initially I'm hoping that files will simply come signed, providing file provenance. Anything else is gravy. > >> >> If you don't trust userspace that needs to be spelled out very clearly. > >> >> You need to talk about what your threat models are. > >> >> > >> >> If the only justification is so that that we can't boot windows if > >> >> someone hacks into userspace it has my nack because that is another kind > >> >> of complete non-sense. > >> > > >> > The usecase is the ability to gate the kexec_load usage in stock > >> > kernels. > >> > >> But kexec_load is already gated. It requires CAP_SYS_BOOT. > > > > It isn't a matter of kexec_load already being gated, but of wanting a > > single place for defining a system wide policy, as described above. > > Signing is only a tool to enforce a policy. Signing by itself is not a > policy. Enforcing any quality controls in the signed executables should > trivially prevent kexec_load from being used. Existing kernels might not support the newer kexec_file_load syscall, so packages are currently being built to try one syscall and fallback to using the other one. In this case, it has nothing to do with quality control. Mimi
Re: [PATCH 1/6] firmware: permit LSMs and IMA to fail firmware sysfs fallback loading
On Fri, 2018-05-04 at 00:02 +, Luis R. Rodriguez wrote:
> If you can add Andres Rodriguez , and Greg to your Cc list
> in the future patches that'd be appreciated.
>
> On Tue, May 01, 2018 at 09:48:18AM -0400, Mimi Zohar wrote:
> > Add an LSM hook prior to allowing firmware sysfs fallback loading.
> >
> > Signed-off-by: Mimi Zohar
> > Cc: Luis R. Rodriguez
> > Cc: David Howells
> > Cc: Kees Cook
> > Cc: Matthew Garrett
> > ---
> > drivers/base/firmware_loader/fallback.c | 7 +++
> > include/linux/fs.h | 1 +
> > 2 files changed, 8 insertions(+)
> >
> > diff --git a/drivers/base/firmware_loader/fallback.c
> > b/drivers/base/firmware_loader/fallback.c
> > index 31b5015b59fe..23d2af30474e 100644
> > --- a/drivers/base/firmware_loader/fallback.c
> > +++ b/drivers/base/firmware_loader/fallback.c
> > @@ -651,6 +651,8 @@ static bool fw_force_sysfs_fallback(unsigned int
> > opt_flags)
> >
> > static bool fw_run_sysfs_fallback(unsigned int opt_flags)
> > {
> > + int ret;
> > +
> > if (fw_fallback_config.ignore_sysfs_fallback) {
> > pr_info_once("Ignoring firmware sysfs fallback due to sysctl
> > knob\n");
> > return false;
> > @@ -659,6 +661,11 @@ static bool fw_run_sysfs_fallback(unsigned int
> > opt_flags)
> > if ((opt_flags & FW_OPT_NOFALLBACK))
> > return false;
> >
> > + /* Also permit LSMs and IMA to fail firmware sysfs fallback */
> > + ret = security_kernel_read_file(NULL, READING_FIRMWARE_FALLBACK);
> > + if (ret < 0)
> > + return ret;
> > +
> > return fw_force_sysfs_fallback(opt_flags);
> > }
> >
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 760d8da1b6c7..dc16a73c3d38 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2810,6 +2810,7 @@ extern int do_pipe_flags(int *, int);
> > id(UNKNOWN, unknown)\
> > id(FIRMWARE, firmware) \
> > id(FIRMWARE_PREALLOC_BUFFER, firmware) \
> > + id(FIRMWARE_FALLBACK, firmware) \
>
> If you're going to add this perhaps FIRMWARE_FALLBACK_SYSFS as we may later
> get FIRMWARE_FALLBACK_EFI.
>From an IMA signature verification perspective, both are buffer based.
The file signature is stored as a security xattr. Without a file
descriptor, the kernel cannot verify the firmware signature.
I don't have a problem with defining another enumeration. Perhaps
other LSMs will want to be able to differentiate between sysfs and EFI
fallback methods.
Mimi
Re: [PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
On Fri, 2018-05-04 at 00:07 +, 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 > > Cc: Luis R. Rodriguez > > Cc: David Howells > > Cc: Kees Cook > > Cc: Seth Forshee > > Cc: Johannes Berg > > --- > > 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 > > > > >
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
On Thu, 2018-05-03 at 22:23 +, Luis R. Rodriguez wrote: > On Tue, May 01, 2018 at 03:27:27PM -0400, Mimi Zohar wrote: > > On Tue, 2018-05-01 at 21:11 +0200, Hans de Goede wrote: > > > Only the pre hook? I believe the post-hook should still be called too, > > > right? So that we've hashes of all loaded firmwares in the IMA core. > > > > Good catch! Right, if IMA-measurement is enabled, then we would want > > to add the measurement. > > Mimi, just a heads up, we only use the post hook for the syfs fallback > mechanism, ie, we don't even use the post hook for direct fs lookup. > Do we want that there? The direct fs lookup calls kernel_read_file_from_path(), which calls the security_kernel_read_file() and security_kernel_post_read_file() hooks. So there is no need to add a direct call to either of these security calls. Mimi
Re: [PATCH 0/3] kexec: limit kexec_load syscall
On Thu, 2018-05-03 at 16:38 -0500, Eric W. Biederman wrote: > Mimi Zohar writes: > > > [Cc'ing Kees and kernel-hardening] > > > > On Thu, 2018-05-03 at 15:13 -0500, Eric W. Biederman wrote: > >> Mimi Zohar writes: > >> > >> > In environments that require the kexec kernel image to be signed, prevent > >> > using the kexec_load syscall. In order for LSMs and IMA to differentiate > >> > between kexec_load and kexec_file_load syscalls, this patch set adds a > >> > call to security_kernel_read_file() in kexec_load_check(). > >> > >> Having thought about it some more this justification for these changes > >> does not work. The functionality of kexec_load is already root-only. > >> So in environments that require the kernel image to be signed just don't > >> use kexec_load. Possibly even compile kexec_load out to save space > >> because you will never need it. You don't need a new security hook to > >> do any of that. Userspace is a very fine mechanism for being the > >> instrument of policy. > > > > True, for those building their own kernel, they can disable the old > > syscalls. The concern is not for those building their own kernels, > > but for those using stock kernels. > > > > By adding an LSM hook here in the kexec_load syscall, as opposed to an > > IMA specific hook, other LSMs can piggy back on top of it. Currently, > > both load_pin and SELinux are gating the kernel module syscalls based > > on security_kernel_read_file. > > > > If there was a similar option for the kernel image, I'm pretty sure > > other LSMs would use it. > > > > From an IMA perspective, there needs to be some method for only > > allowing signed code to be loaded, executed, etc. - kernel modules, > > kernel image/initramfs, firmware, policies. > > What is the IMA perspective. Why can't IMA trust appropriately > authorized userspace? Suppose a system owner wants to define a system wide policy that requires all code be signed - kernel modules, firmware, kexec image & initramfs, executables, mmapped files, etc - without having to rebuild the kernel. Without a call in kexec_load that isn't possible. > > >> If you don't trust userspace that needs to be spelled out very clearly. > >> You need to talk about what your threat models are. > >> > >> If the only justification is so that that we can't boot windows if > >> someone hacks into userspace it has my nack because that is another kind > >> of complete non-sense. > > > > The usecase is the ability to gate the kexec_load usage in stock > > kernels. > > But kexec_load is already gated. It requires CAP_SYS_BOOT. It isn't a matter of kexec_load already being gated, but of wanting a single place for defining a system wide policy, as described above. Mimi > > >> Given that you are not trusting userspace this changeset also probably > >> needs to have the kernel-hardening list cc'd. Because the only possible > >> justification I can imagine for something like this is kernel-hardening. > > > > Sure, Cc'ing linux-hardening and Kees. > > > > Mimi >
Re: [PATCH v6 0/4] Certificate insertion support for x86 bzImages
On Fri, 2018-05-04 at 03:11 +1000, James Morris wrote: > On Wed, 2 May 2018, Mehmet Kayaalp wrote: > > > These patches add support for modifying the reserved space for extra > > certificates in a compressed bzImage in x86. This allows separating the > > system keyring certificate from the kernel build process. After the kernel > > image is distributed, the insert-sys-cert script can be used to insert the > > certificate for x86. > > Can you provide more explanation of how this is useful and who would use > it? I'm involved in a number projects that rely on a kernel build group to actually build kernels for their systems. Reserving memory for additional public keys, allows product groups to insert public keys post build. Initially the product groups might insert development keys, but eventually they would insert the product's public key. Mimi
Re: [PATCH 0/3] kexec: limit kexec_load syscall
[Cc'ing Kees and kernel-hardening] On Thu, 2018-05-03 at 15:13 -0500, Eric W. Biederman wrote: > Mimi Zohar writes: > > > In environments that require the kexec kernel image to be signed, prevent > > using the kexec_load syscall. In order for LSMs and IMA to differentiate > > between kexec_load and kexec_file_load syscalls, this patch set adds a > > call to security_kernel_read_file() in kexec_load_check(). > > Having thought about it some more this justification for these changes > does not work. The functionality of kexec_load is already root-only. > So in environments that require the kernel image to be signed just don't > use kexec_load. Possibly even compile kexec_load out to save space > because you will never need it. You don't need a new security hook to > do any of that. Userspace is a very fine mechanism for being the > instrument of policy. True, for those building their own kernel, they can disable the old syscalls. The concern is not for those building their own kernels, but for those using stock kernels. By adding an LSM hook here in the kexec_load syscall, as opposed to an IMA specific hook, other LSMs can piggy back on top of it. Currently, both load_pin and SELinux are gating the kernel module syscalls based on security_kernel_read_file. If there was a similar option for the kernel image, I'm pretty sure other LSMs would use it. >From an IMA perspective, there needs to be some method for only allowing signed code to be loaded, executed, etc. - kernel modules, kernel image/initramfs, firmware, policies. > If you don't trust userspace that needs to be spelled out very clearly. > You need to talk about what your threat models are. > > If the only justification is so that that we can't boot windows if > someone hacks into userspace it has my nack because that is another kind > of complete non-sense. The usecase is the ability to gate the kexec_load usage in stock kernels. > > Given that you are not trusting userspace this changeset also probably > needs to have the kernel-hardening list cc'd. Because the only possible > justification I can imagine for something like this is kernel-hardening. Sure, Cc'ing linux-hardening and Kees. Mimi
Re: [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
On Thu, 2018-05-03 at 11:42 -0500, Eric W. Biederman wrote: > Casey Schaufler writes: > > > On 5/3/2018 8:51 AM, Eric W. Biederman wrote: > >> Mimi Zohar writes: > >> > >>> On Wed, 2018-05-02 at 09:45 -0500, Eric W. Biederman wrote: > >>>> Mimi Zohar writes: > >>>> > >>>>> Allow LSMs and IMA to differentiate between the kexec_load and > >>>>> kexec_file_load syscalls by adding an "unnecessary" call to > >>>>> security_kernel_read_file() in kexec_load. This would be similar to the > >>>>> existing init_module syscall calling security_kernel_read_file(). > >>>> Given the reasonable desire to load a policy that ensures everything > >>>> has a signature I don't have fundamental objections. > >>>> > >>>> security_kernel_read_file as a hook seems an odd choice. At the very > >>>> least it has a bad name because there is no file reading going on here. > >>>> > >>>> I am concerned that I don't see CONFIG_KEXEC_VERIFY_SIG being tested > >>>> anywhere. Which means I could have a kernel compiled without that and I > >>>> would be allowed to use kexec_file_load without signature checking. > >>>> While kexec_load would be denied. > >>>> > >>>> Am I missing something here? > >>> The kexec_file_load() calls kernel_read_file_from_fd(), which in turn > >>> calls security_kernel_read_file(). So kexec_file_load and kexec_load > >>> syscall would be using the same method for enforcing signature > >>> verification. > >> Having looked at your patches and the kernel a little more I think > >> this should be a separate security hook that does not take a file > >> parameter. > >> > >> Right now every other security module assumes !file is init_module. > >> So I think this change has the potential to confuse other security > >> modules, with the result of unintended policy being applied. > >> > >> So just for good security module hygeine I think this needs a dedicated > >> kexec_load security hook. > > > > I would rather see the existing modules updated than a new > > hook added. Too many hooks spoil the broth. Two hooks with > > trivial differences just add to the clutter and make it harder > > for non-lsm developers to figure out what to use in their > > code. > > These are not non-trivial differences. There is absolutely nothing > file related about kexec_load. Nor for init_module for that matter. > > If something is called security_kernel_read_file I think it is wholly > appropriate for code that processes such a hook to assume file is > non-NULL. > > When you have to dance a jig (which is what I see the security modules > doing) to figure out who is calling a lsm hook for what purpose I think > it is a maintenance problem waiting to happen and that the hook is badly > designed. > > At this point I don't care what the lsm's do with the hooks but the > hooks need to make sense for people outside of the lsm's and something > about reading a file in a syscall that doesn't read files is complete > and utter nonsense. Sure, we can define a wrapper around the security_kernel_read_file() hook, calling it security_non-fd_syscall() or even security_old_syscall(). Mimi
Re: [PATCH] evm: Don't update hmacs in user ns mounts
On Wed, 2018-05-02 at 16:49 -0500, Eric W. Biederman wrote: > From: Seth Forshee > Date: Fri, 22 Dec 2017 15:32:35 +0100 > > The kernel should not calculate new hmacs for mounts done by > non-root users. Update evm_calc_hmac_or_hash() to refuse to > calculate new hmacs for mounts for non-init user namespaces. > > Cc: linux-integr...@vger.kernel.org > Cc: linux-security-mod...@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: James Morris > Cc: Mimi Zohar > Cc: "Serge E. Hallyn" > Signed-off-by: Seth Forshee > Signed-off-by: Dongsu Park > Signed-off-by: Eric W. Biederman > --- > > Mimi this patch has been floating around for a while and it appears to > be the only piece missing from the vfs to make unprivileged mounts safe > (at least semantically). Do you want to merge this through your integrity > tree or should merge this through my userns tree? Matthew's EVM patches don't conflict with this change, so either way is fine. Mimi > > security/integrity/evm/evm_crypto.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > > diff --git a/security/integrity/evm/evm_crypto.c > b/security/integrity/evm/evm_crypto.c > index a46fba322340..facf9cdd577d 100644 > --- a/security/integrity/evm/evm_crypto.c > +++ b/security/integrity/evm/evm_crypto.c > @@ -200,7 +200,8 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry, > int size; > bool ima_present = false; > > - if (!(inode->i_opflags & IOP_XATTR)) > + if (!(inode->i_opflags & IOP_XATTR) || > + inode->i_sb->s_user_ns != &init_user_ns) > return -EOPNOTSUPP; > > desc = init_desc(type);
Re: [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
On Wed, 2018-05-02 at 09:45 -0500, Eric W. Biederman wrote:
> Mimi Zohar writes:
>
> > Allow LSMs and IMA to differentiate between the kexec_load and
> > kexec_file_load syscalls by adding an "unnecessary" call to
> > security_kernel_read_file() in kexec_load. This would be similar to the
> > existing init_module syscall calling security_kernel_read_file().
>
> Given the reasonable desire to load a policy that ensures everything
> has a signature I don't have fundamental objections.
>
> security_kernel_read_file as a hook seems an odd choice. At the very
> least it has a bad name because there is no file reading going on here.
>
> I am concerned that I don't see CONFIG_KEXEC_VERIFY_SIG being tested
> anywhere. Which means I could have a kernel compiled without that and I
> would be allowed to use kexec_file_load without signature checking.
> While kexec_load would be denied.
>
> Am I missing something here?
The kexec_file_load() calls kernel_read_file_from_fd(), which in turn
calls security_kernel_read_file(). So kexec_file_load and kexec_load
syscall would be using the same method for enforcing signature
verification.
This is independent of the architecture specific method for verifying
signatures. The coordination between these two methods was included
in the lockdown patch set, but is being removed, as well the gating of
kexec_load syscall. Instead of being based on the lockdown flag, I
assume the coordination between the two methods will reappear based on
a secure boot flag of some sort.
Mimi
>
> > Signed-off-by: Mimi Zohar
> > ---
> > kernel/kexec.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/kernel/kexec.c b/kernel/kexec.c
> > index aed8fb2564b3..d1386cfc6796 100644
> > --- a/kernel/kexec.c
> > +++ b/kernel/kexec.c
> > @@ -11,6 +11,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -195,11 +196,21 @@ static int do_kexec_load(unsigned long entry,
> > unsigned long nr_segments,
> > static inline int kexec_load_check(unsigned long nr_segments,
> >unsigned long flags)
> > {
> > + int result;
> > +
> > /* We only trust the superuser with rebooting the system. */
> > if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
> > return -EPERM;
> >
> > /*
> > +* Allow LSMs and IMA to differentiate between kexec_load and
> > +* kexec_file_load syscalls.
> > +*/
> > + result = security_kernel_read_file(NULL, READING_KEXEC_IMAGE);
> > + if (result < 0)
> > + return result;
> > +
> > + /*
> > * Verify we have a legal set of flags
> > * This leaves us room for future extensions.
> > */
>
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
On Tue, 2018-05-01 at 21:11 +0200, Hans de Goede wrote:
> Hi,
>
> On 01-05-18 16:36, Mimi Zohar wrote:
> > [Cc'ing linux-security]
> >
> > On Sun, 2018-04-29 at 11:35 +0200, Hans de Goede wrote:
> > [...]
> >> diff --git a/drivers/base/firmware_loader/fallback_efi.c
> >> b/drivers/base/firmware_loader/fallback_efi.c
> >> new file mode 100644
> >> index ..82ba82f48a79
> >> --- /dev/null
> >> +++ b/drivers/base/firmware_loader/fallback_efi.c
> >> @@ -0,0 +1,51 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +
> >> +#include "fallback.h"
> >> +#include "firmware.h"
> >> +
> >> +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
> >> + enum fw_opt *opt_flags, int ret)
> >> +{
> >> + enum kernel_read_file_id id = READING_FIRMWARE;
> >
> > Please define a new kernel_read_file_id for this (eg.
> > READING_FIRMWARE_EFI_EMBEDDED).
>
> Are you sure, I wonder how useful it is to add a new
> kernel_read_file_id every time a new way to get firmware
> comes up?
>
> I especially wonder about the sense in adding a new id
> given that the quite old FIRMWARE_PREALLOC_BUFFER is
> still not supported / checked properly by the security code.
I posted patches earlier today[1], which address this. Patch 5/6 just
makes it equivalent to READING_FIRMWARE. Patch 6/6 questions whether
the device has access to the pre-allocated buffer *before* the
signature has been verified.
[1] kernsec.org/pipermail/linux-security-module-archive/2018-May/006639.html
>
> Anyways I can add a new id if you want me to, what about
> when fw_get_efi_embedded_fw is reading into a driver allocated
> buffer, do you want a separate EADING_FIRMWARE_EFI_EMBEDDED_PREALLOC_BUFFER
> for that ?
Without the kernel being able to verify the firmware's signature, I'm
not sure it makes much of a difference.
>
> >
> >> + size_t size, max = INT_MAX;
> >> + int rc;
> >> +
> >> + if (!dev)
> >> + return ret;
> >> +
> >> + if (!device_property_read_bool(dev, "efi-embedded-firmware"))
> >> + return ret;
> >
> > Instead of calling security_kernel_post_read_file(), either in
> > device_property_read_bool() or here call security_kernel_read_file().
> >
> > The pre read call is for deciding whether to allow this call
> > independent of the firmware being loaded, whereas the post security
> > call is currently being used by IMA-appraisal for verifying a
> > signature. There might be other LSMs using the post hook as well. As
> > there is no kernel signature associated with this firmware, use the
> > security pre read_file hook.
>
> Only the pre hook? I believe the post-hook should still be called too,
> right? So that we've hashes of all loaded firmwares in the IMA core.
Good catch! Right, if IMA-measurement is enabled, then we would want
to add the measurement.
Mimi
Re: [PATCH v5 2/5] efi: Add embedded peripheral firmware support
[Cc'ing linux-security]
On Sun, 2018-04-29 at 11:35 +0200, Hans de Goede wrote:
[...]
> diff --git a/drivers/base/firmware_loader/fallback_efi.c
> b/drivers/base/firmware_loader/fallback_efi.c
> new file mode 100644
> index ..82ba82f48a79
> --- /dev/null
> +++ b/drivers/base/firmware_loader/fallback_efi.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include "fallback.h"
> +#include "firmware.h"
> +
> +int fw_get_efi_embedded_fw(struct device *dev, struct fw_priv *fw_priv,
> +enum fw_opt *opt_flags, int ret)
> +{
> + enum kernel_read_file_id id = READING_FIRMWARE;
Please define a new kernel_read_file_id for this (eg.
READING_FIRMWARE_EFI_EMBEDDED).
> + size_t size, max = INT_MAX;
> + int rc;
> +
> + if (!dev)
> + return ret;
> +
> + if (!device_property_read_bool(dev, "efi-embedded-firmware"))
> + return ret;
Instead of calling security_kernel_post_read_file(), either in
device_property_read_bool() or here call security_kernel_read_file().
The pre read call is for deciding whether to allow this call
independent of the firmware being loaded, whereas the post security
call is currently being used by IMA-appraisal for verifying a
signature. There might be other LSMs using the post hook as well. As
there is no kernel signature associated with this firmware, use the
security pre read_file hook.
thanks,
Mimi
> +
> + *opt_flags |= FW_OPT_NO_WARN | FW_OPT_NOCACHE | FW_OPT_NOFALLBACK;
> +
> + /* Already populated data member means we're loading into a buffer */
> + if (fw_priv->data) {
> + id = READING_FIRMWARE_PREALLOC_BUFFER;
> + max = fw_priv->allocated_size;
> + }
> +
> + rc = efi_get_embedded_fw(fw_priv->fw_name, &fw_priv->data, &size, max);
> + if (rc) {
> + dev_warn(dev, "Firmware %s not in EFI\n", fw_priv->fw_name);
> + return ret;
> + }
> +
> + rc = security_kernel_post_read_file(NULL, fw_priv->data, size, id);
> + if (rc) {
> + if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
> + vfree(fw_priv->data);
> + fw_priv->data = NULL;
> + }
> + return rc;
> + }
> +
> + dev_dbg(dev, "using efi-embedded fw %s\n", fw_priv->fw_name);
> + fw_priv->size = size;
> + fw_state_done(fw_priv);
> + return 0;
> +}
[PATCH 2/6] ima: prevent sysfs fallback firmware loading
With an IMA policy requiring signed firmware, this patch prevents
the sysfs fallback method of loading firmware.
Signed-off-by: Mimi Zohar
Cc: Luis R. Rodriguez
Cc: David Howells
Cc: Matthew Garrett
---
security/integrity/ima/ima_main.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 754ece08e1c6..8759280dccf6 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -453,7 +453,17 @@ int ima_read_file(struct file *file, enum
kernel_read_file_id read_id)
}
return 0;
}
+
+ if (read_id == READING_FIRMWARE_FALLBACK) {
+ if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
+ (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+ pr_err("Prevent firmware sysfs fallback loading.\n");
+ return -EACCES;
+ }
+ return 0;
+ }
return 0;
+
}
static int read_idmap[READING_MAX_ID] = {
--
2.7.5
[PATCH 4/6] ima: coordinate with signed regulatory.db
Based on IMA policy, measure and appraise regulatory.db firmware as
usual, but on signature verification failure rely on regdb signature.
For systems wanting IMA-appraisal enforcement on all firmware, including
regdb, do not enable CONFIG_CFG80211_REQUIRE_SIGNED_REGDB.
Signed-off-by: Mimi Zohar
Cc: Luis R. Rodriguez
Cc: David Howells
Cc: Kees Cook
Cc: Seth Forshee
Cc: Johannes Berg
---
security/integrity/ima/ima_main.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 8759280dccf6..71b5a51c6709 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -468,6 +468,7 @@ int ima_read_file(struct file *file, enum
kernel_read_file_id read_id)
static int read_idmap[READING_MAX_ID] = {
[READING_FIRMWARE] = FIRMWARE_CHECK,
+ [READING_FIRMWARE_REGULATORY_DB] = FIRMWARE_CHECK,
[READING_MODULE] = MODULE_CHECK,
[READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
[READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
@@ -515,8 +516,12 @@ int ima_post_read_file(struct file *file, void *buf,
loff_t size,
func = read_idmap[read_id] ?: FILE_CHECK;
security_task_getsecid(current, &secid);
- return process_measurement(file, current_cred(), secid, buf, size,
- MAY_READ, func, 0);
+ ret = process_measurement(file, current_cred(), secid, buf, size,
+ MAY_READ, func, 0);
+
+ /* Co-ordination with signed regdb */
+ if (ret < -EACCES && read_id == READING_FIRMWARE_REGULATORY_DB)
+ return 0;
}
static int __init init_ima(void)
--
2.7.5
[PATCH 5/6] ima: verify kernel firmware signatures when using a preallocated buffer
Don't differentiate between kernel_read_file_id READING_FIRMWARE and
READING_FIRMWARE_PREALLOC_BUFFER enumerations.
Fixes: a098ecd firmware: support loading into a pre-allocated buffer (since 4.8)
Signed-off-by: Mimi Zohar
Cc: Luis R. Rodriguez
Cc: David Howells
Cc: Kees Cook
Cc: Serge E. Hallyn
Cc: Stephen Boyd
---
security/integrity/ima/ima_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 71b5a51c6709..eb9c273ab81d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -469,6 +469,7 @@ int ima_read_file(struct file *file, enum
kernel_read_file_id read_id)
static int read_idmap[READING_MAX_ID] = {
[READING_FIRMWARE] = FIRMWARE_CHECK,
[READING_FIRMWARE_REGULATORY_DB] = FIRMWARE_CHECK,
+ [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
[READING_MODULE] = MODULE_CHECK,
[READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
[READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
--
2.7.5
[RFC PATCH 6/6] ima: prevent loading firmware into a pre-allocated buffer
Question: can the device access the pre-allocated buffer at any time?
By allowing devices to request firmware be loaded directly into a
pre-allocated buffer, will this allow the device access to the firmware
before the kernel has verified the firmware signature?
Is it dependent on the type of buffer allocated (eg. DMA)? For example,
qcom_mdt_load() -> qcom_scm_pas_init_image() -> dma_alloc_coherent().
With an IMA policy requiring signed firmware, this patch would prevent
loading firmware into a pre-allocated buffer.
Signed-off-by: Mimi Zohar
Cc: Luis R. Rodriguez
Cc: David Howells
Cc: Kees Cook
Cc: Serge E. Hallyn
Cc: Stephen Boyd
---
security/integrity/ima/ima_main.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index eb9c273ab81d..3098131f77c4 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -454,6 +454,15 @@ int ima_read_file(struct file *file, enum
kernel_read_file_id read_id)
return 0;
}
+ if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
+ if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
+ (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+ pr_err("Prevent device from accessing firmware prior to
verifying the firmware signature.\n");
+ return -EACCES;
+ }
+ return 0;
+ }
+
if (read_id == READING_FIRMWARE_FALLBACK) {
if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
(ima_appraise & IMA_APPRAISE_ENFORCE)) {
--
2.7.5
[PATCH 3/6] firmware: differentiate between signed regulatory.db and other firmware
Allow LSMs and IMA to differentiate between signed regulatory.db and other firmware. Signed-off-by: Mimi Zohar Cc: Luis R. Rodriguez Cc: David Howells Cc: Kees Cook Cc: Seth Forshee Cc: Johannes Berg --- 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 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)\ -- 2.7.5
[PATCH 1/6] firmware: permit LSMs and IMA to fail firmware sysfs fallback loading
Add an LSM hook prior to allowing firmware sysfs fallback loading.
Signed-off-by: Mimi Zohar
Cc: Luis R. Rodriguez
Cc: David Howells
Cc: Kees Cook
Cc: Matthew Garrett
---
drivers/base/firmware_loader/fallback.c | 7 +++
include/linux/fs.h | 1 +
2 files changed, 8 insertions(+)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index 31b5015b59fe..23d2af30474e 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -651,6 +651,8 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
static bool fw_run_sysfs_fallback(unsigned int opt_flags)
{
+ int ret;
+
if (fw_fallback_config.ignore_sysfs_fallback) {
pr_info_once("Ignoring firmware sysfs fallback due to sysctl
knob\n");
return false;
@@ -659,6 +661,11 @@ static bool fw_run_sysfs_fallback(unsigned int opt_flags)
if ((opt_flags & FW_OPT_NOFALLBACK))
return false;
+ /* Also permit LSMs and IMA to fail firmware sysfs fallback */
+ ret = security_kernel_read_file(NULL, READING_FIRMWARE_FALLBACK);
+ if (ret < 0)
+ return ret;
+
return fw_force_sysfs_fallback(opt_flags);
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 760d8da1b6c7..dc16a73c3d38 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2810,6 +2810,7 @@ extern int do_pipe_flags(int *, int);
id(UNKNOWN, unknown)\
id(FIRMWARE, firmware) \
id(FIRMWARE_PREALLOC_BUFFER, firmware) \
+ id(FIRMWARE_FALLBACK, firmware) \
id(MODULE, kernel-module) \
id(KEXEC_IMAGE, kexec-image)\
id(KEXEC_INITRAMFS, kexec-initramfs)\
--
2.7.5
[PATCH 0/6] firmware: kernel signature verification
Allow LSMs and IMA to differentiate between different methods of firmware loading (eg. direct loading, sysfs fallback) and to differentiate/coordinate between signature verification methods (eg. regdb, IMA-appraisal). In addition, the last two patches address the pre-allocated buffer. The first of these patches doesn't differentiate between reading the firmware first into kernel memory and verifying the kernel signature, versus reading the firmware directly into a pre-allocated buffer. The last patch, which is posted as an RFC, questions whether the device can access the pre-allocated buffer before the kernel signature has been verified. Mimi Zohar (6): firmware: permit LSMs and IMA to fail firmware sysfs fallback loading ima: prevent sysfs fallback firmware loading firmware: differentiate between signed regulatory.db and other firmware ima: coordinate with signed regulatory.db ima: verify kernel firmware signatures when using a preallocated buffer ima: prevent loading firmware into a pre-allocated buffer drivers/base/firmware_loader/fallback.c | 7 +++ drivers/base/firmware_loader/main.c | 5 + include/linux/fs.h | 2 ++ security/integrity/ima/ima_main.c | 29 +++-- 4 files changed, 41 insertions(+), 2 deletions(-) -- 2.7.5
[PATCH] ima: define a new policy condition based on the filesystem name
If/when file data signatures are distributed with the file data, this
patch will not be needed. In the current environment where only some
files are signed, the ability to differentiate between file systems is
needed. Some file systems consider the file system magic number
internal to the file system.
This patch defines a new IMA policy condition named "fsname", based on
the superblock's file_system_type (sb->s_type) name. This allows policy
rules to be expressed in terms of the filesystem name.
The following sample rules require file signatures on rootfs files
executed or mmap'ed.
appraise func=BPRM_CHECK fsname=rootfs appraise_type=imasig
appraise func=FILE_MMAP fsname=rootfs appraise_type=imasig
Signed-off-by: Mimi Zohar
Cc: Dave Chinner
Cc: Theodore Ts'o
---
Documentation/ABI/testing/ima_policy | 2 +-
security/integrity/ima/ima_policy.c | 25 -
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy
b/Documentation/ABI/testing/ima_policy
index b8465e00ba5f..74c6702de74e 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -21,7 +21,7 @@ Description:
audit | hash | dont_hash
condition:= base | lsm [option]
base: [[func=] [mask=] [fsmagic=] [fsuuid=] [uid=]
- [euid=] [fowner=]]
+ [euid=] [fowner=] [fsname=]]
lsm:[[subj_user=] [subj_role=] [subj_type=]
[obj_user=] [obj_role=] [obj_type=]]
option: [[appraise_type=]] [permit_directio]
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index d89bebf85421..03cbba423e59 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -33,6 +33,7 @@
#define IMA_INMASK 0x0040
#define IMA_EUID 0x0080
#define IMA_PCR0x0100
+#define IMA_FSNAME 0x0200
#define UNKNOWN0
#define MEASURE0x0001 /* same as IMA_MEASURE */
@@ -74,6 +75,7 @@ struct ima_rule_entry {
void *args_p; /* audit value */
int type; /* audit type */
} lsm[MAX_LSM_RULES];
+ char *fsname;
};
/*
@@ -273,6 +275,9 @@ static bool ima_match_rules(struct ima_rule_entry *rule,
struct inode *inode,
if ((rule->flags & IMA_FSMAGIC)
&& rule->fsmagic != inode->i_sb->s_magic)
return false;
+ if ((rule->flags & IMA_FSNAME)
+ && strcmp(rule->fsname, inode->i_sb->s_type->name))
+ return false;
if ((rule->flags & IMA_FSUUID) &&
!uuid_equal(&rule->fsuuid, &inode->i_sb->s_uuid))
return false;
@@ -540,7 +545,7 @@ enum {
Opt_audit, Opt_hash, Opt_dont_hash,
Opt_obj_user, Opt_obj_role, Opt_obj_type,
Opt_subj_user, Opt_subj_role, Opt_subj_type,
- Opt_func, Opt_mask, Opt_fsmagic,
+ Opt_func, Opt_mask, Opt_fsmagic, Opt_fsname,
Opt_fsuuid, Opt_uid_eq, Opt_euid_eq, Opt_fowner_eq,
Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
@@ -565,6 +570,7 @@ static match_table_t policy_tokens = {
{Opt_func, "func=%s"},
{Opt_mask, "mask=%s"},
{Opt_fsmagic, "fsmagic=%s"},
+ {Opt_fsname, "fsname=%s"},
{Opt_fsuuid, "fsuuid=%s"},
{Opt_uid_eq, "uid=%s"},
{Opt_euid_eq, "euid=%s"},
@@ -776,6 +782,17 @@ static int ima_parse_rule(char *rule, struct
ima_rule_entry *entry)
if (!result)
entry->flags |= IMA_FSMAGIC;
break;
+ case Opt_fsname:
+ ima_log_string(ab, "fsname", args[0].from);
+
+ entry->fsname = kstrdup(args[0].from, GFP_KERNEL);
+ if (!entry->fsname) {
+ result = -ENOMEM;
+ break;
+ }
+ result = 0;
+ entry->flags |= IMA_FSNAME;
+ break;
case Opt_fsuuid:
ima_log_string(ab, "fsuuid", args[0].from);
@@ -1104,6 +1121,12 @@ int ima_policy_show(struct seq_file *m, void *v)
seq_puts(m, " ");
}
+ if (entry->flags & IMA_FSNAME) {
+ snprintf(tbuf, sizeof(tbuf), "%s", entry->fsname);
+ seq_printf(m, pt(Opt_fsname), tbuf);
+ seq_puts(m, " ");
+ }
+
if (entry->flags & IMA_PCR) {
snprintf(tbuf, sizeof(tbuf), "%d", entry->pcr);
seq_printf(m, pt(Opt_pcr), tbuf);
--
2.7.5
Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support
On Tue, 2018-04-24 at 23:42 +, Luis R. Rodriguez wrote:
> On Tue, Apr 24, 2018 at 12:07:01PM -0400, Mimi Zohar wrote:
> > On Tue, 2018-04-24 at 17:09 +0200, Hans de Goede wrote:
> > > Hi,
> > >
> > > On 23-04-18 23:11, Luis R. Rodriguez wrote:
> > > > Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a
> > > > new ID
> > > > and security for this type of request so IMA can reject it if the
> > > > policy is
> > > > configured for it.
> > >
> > > Hmm, interesting, actually it seems like the whole existence
> > > of READING_FIRMWARE_PREALLOC_BUFFER is a mistake,
>
> request_firmware_into_buf() was merged without my own review, however,
> the ID thing did get review from Mimi:
>
> https://patchwork.kernel.org/patch/9074611/
>
> The ID is not for IMA alone, its for any LSM to decide what to do.
> Note Mimi asked for READING_FIRMWARE_DMA if such buffer was in DMA,
> otherise READING_FIRMWARE_PREALLOC_BUFFER was suggested.
>
> > > the IMA
> > > framework really does not care if we are loading the firmware
> > > into memory allocated by the firmware-loader code, or into
> > > memory allocated by the device-driver requesting the firmware.
>
> That's up to LSM folks to decide. We have these so far:
>
> #define __kernel_read_file_id(id) \
>
> id(UNKNOWN, unknown)\
>
> id(FIRMWARE, firmware) \
>
> id(FIRMWARE_PREALLOC_BUFFER, firmware) \
>
> id(MODULE, kernel-module) \
>
> id(KEXEC_IMAGE, kexec-image)\
>
> id(KEXEC_INITRAMFS, kexec-initramfs)\
>
> id(POLICY, security-policy) \
>
> id(X509_CERTIFICATE, x509-certificate) \
>
> id(MAX_ID, )
>
> The first type of IDs added was about type of files the kernel
> LSMs may want to do different things for.
>
> Mimi why did you want a separate ID for it back before?
The point of commit a098ecd2fa7d ("firmware: support loading into a
pre-allocated buffer") is to avoid reading the firmware into kernel
memory and then copying it "to it's final resting place". My concern
is that if the device driver has access to the buffer, it could access
the buffer prior to the firmware's signature having been verified by
the kernel.
In tightly controlled environments interested in limiting which signed
firmware version is loaded, require's the device driver not having
access to the buffer until after the signature has been verified by
the kernel (eg. IMA-appraisal).
>
> I should note now that request_firmware_into_buf() and its
> READING_FIRMWARE_PREALLOC_BUFFER was to enable a driver on memory constrained
> devices. The files are large (commit says 16 MiB).
>
> I've heard of larger possible files with remoteproc and with Android using
> the custom fallback mechanism -- which could mean a proprietary tool
> fetching firmware from a random special place on a device.
>
> I could perhaps imagine an LSM which may be aware of such type of
> arrangement may want to do its own vetting of some sort, but this
> would not be specific to READING_FIRMWARE_PREALLOC_BUFFER, but rather
> the custom fallback mechaism.
>
> Whether or not the buffer was preallocated by the driver seems a little
> odd for security folks to do something different with it. Security LSM
> folks please chime in.
>
> I could see a bit more of a use case for an ID for firmware scraped
> from EFI, which Hans' patch will provide. But that *also* should get
> good review from other LSM folks.
>
> One of the issues with accepting more IDs loosely is where do we
> stop though? If no one really is using READING_FIRMWARE_PREALLOC_BUFFER
> I'd say lets remove it. Likewise, for this EFI thing I'd like an idea
> if we really are going to have users for it.
>
> If its of any help --
>
> drivers/soc/qcom/mdt_loader.c is the only driver currently using
> request_firmware_into_buf() however I'll note qcom_mdt_load() is used in many
> other drivers so they are wrappers around request_firmware_into_buf():
>
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c: * adreno_request_fw() handles this,
> but qcom_mdt_load() does
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c: ret = qcom_mdt_load(dev, fw,
> fwname, GPU_PAS_ID
Re: [PATCH v3 2/5] efi: Add embedded peripheral firmware support
On Tue, 2018-04-24 at 17:09 +0200, Hans de Goede wrote:
> Hi,
>
> On 23-04-18 23:11, Luis R. Rodriguez wrote:
> > Hans, please see use of READING_FIRMWARE_PREALLOC_BUFFER, we'll need a new
> > ID
> > and security for this type of request so IMA can reject it if the policy is
> > configured for it.
>
> Hmm, interesting, actually it seems like the whole existence
> of READING_FIRMWARE_PREALLOC_BUFFER is a mistake, the IMA
> framework really does not care if we are loading the firmware
> into memory allocated by the firmware-loader code, or into
> memory allocated by the device-driver requesting the firmware.
>
> As such the current IMA code (from v4.17-rc2) actually does
> not handle READING_FIRMWARE_PREALLOC_BUFFER at all,
Right, it doesn't yet address READING_FIRMWARE_PREALLOC_BUFFER, but
should.
Depending on whether the device requesting the firmware has access to
the DMA memory, before the signature verification, will determine how
IMA-appraisal addresses READING_FIRMWARE_PREALLOC_BUFFER.
Mimi
> here
> are bits of code from: security/integrity/ima/ima_main.c:
>
> static int read_idmap[READING_MAX_ID] = {
> [READING_FIRMWARE] = FIRMWARE_CHECK,
> [READING_MODULE] = MODULE_CHECK,
> [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
> [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
> [READING_POLICY] = POLICY_CHECK
> };
>
> int ima_post_read_file(struct file *file, void *buf, loff_t size,
> ...
> if (!file && read_id == READING_FIRMWARE) {
> if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> (ima_appraise & IMA_APPRAISE_ENFORCE))
> return -EACCES; /* INTEGRITY_UNKNOWN */
> return 0;
> }
>
> Which show that the IMA code is not handling
> READING_FIRMWARE_PREALLOC_BUFFER as it should (I believe it
> should handle it the same as READING_FIRMWARE).
>
> Now we could fix that, but the only user of
> READING_FIRMWARE_PREALLOC_BUFFER is the code which originally
> introduced it:
>
> https://patchwork.kernel.org/patch/9162011/
>
> So I believe it might be better to instead replace it
> with just READING_FIRMWARE and find another way to tell
> kernel_read_file() that there is a pre-allocated buffer,
> perhaps the easiest way there is that *buf must be
> NULL when the caller wants kernel_read_file() to
> vmalloc the mem. This would of course require auditing
> all callers that the buf which the pass in is initialized
> to NULL.
>
> Either way adding a third READING_FIRMWARE_FOO to the
> kernel_read_file_id enum seems like a bad idea, from
> the IMA pov firmware is firmware.
>
> What this whole exercise has shown me though is that
> I need to call security_kernel_post_read_file() when
> loading EFI embedded firmware. I will add a call to
> security_kernel_post_read_file() for v4 of the patch-set.
>
> > Please Cc Kees in future patches.
>
> Will do.
>
> Regards,
>
> Hans
>
Re: [RFC PATCH v3 1/3] ima: extend clone() with IMA namespace support
On Wed, 2018-04-18 at 15:12 -0500, Eric W. Biederman wrote: > Mimi Zohar writes: > > > On Wed, 2018-04-18 at 09:09 -0700, John Johansen wrote: > >> On 04/13/2018 09:25 AM, Mimi Zohar wrote: > >> > [Cc'ing John Johansen] > >> > > >> > On Tue, 2018-03-27 at 18:01 -0500, Eric W. Biederman wrote: > >> > [...] > >> >> As such I expect the best way to create the ima namespace is by simply > >> >> writing to securityfs/imafs. Possibly before the user namespace is > >> >> even unshared. That would allow IMA to keep track of things from > >> >> before a container is created. > >> > > >> > >> I do think this is generally the right approach for LSMs when looking > >> forward to LSM stacking and more LSMs. > >> > >> > >> > My initial thought was to stage IMA namespacing with just IMA-audit > >> > first, followed by either IMA-measurement or IMA-appraisal. This > >> > would allow us to get the basic IMA namespacing framework working and > >> > defer dealing with the securityfs related namespacing of the IMA > >> > policy and measurement list issues to later. > >> > > >> > By tying IMA namespacing to a securityfs ima/unshare file, we would > >> > need to address the securityfs issues first. > >> > > >> > >> well it depends on what you want to do. It would be possible to have > >> a simple file (not a jump link) within securityfs that IMA could use > >> without having to deal with all the securityfs issues first. However it > >> does require that securityfs (not necessarily imafs) be visible within > >> the mount namespace of the task doing the setup. > > > > Eric, would you be OK with that? > > Roughly. My understanding is that you have to have a write to some > filesystem to set the ima policy. > > I was expecting having to write an "create ima namespace" value > to the filesystem would not be any special effort. > > Now it sounds like providing the "create an ima namespace" is going to > be a special case, and that does not sound correct. This is not any different than any of the other security/ima/ files (eg. policy, ascii_runtime_measurements, ...). The next IMA namespacing stage would add support for these files. Mimi
Re: [RFC PATCH v3 1/3] ima: extend clone() with IMA namespace support
On Wed, 2018-04-18 at 09:09 -0700, John Johansen wrote: > On 04/13/2018 09:25 AM, Mimi Zohar wrote: > > [Cc'ing John Johansen] > > > > On Tue, 2018-03-27 at 18:01 -0500, Eric W. Biederman wrote: > > [...] > >> As such I expect the best way to create the ima namespace is by simply > >> writing to securityfs/imafs. Possibly before the user namespace is > >> even unshared. That would allow IMA to keep track of things from > >> before a container is created. > > > > I do think this is generally the right approach for LSMs when looking > forward to LSM stacking and more LSMs. > > > > My initial thought was to stage IMA namespacing with just IMA-audit > > first, followed by either IMA-measurement or IMA-appraisal. This > > would allow us to get the basic IMA namespacing framework working and > > defer dealing with the securityfs related namespacing of the IMA > > policy and measurement list issues to later. > > > > By tying IMA namespacing to a securityfs ima/unshare file, we would > > need to address the securityfs issues first. > > > > well it depends on what you want to do. It would be possible to have > a simple file (not a jump link) within securityfs that IMA could use > without having to deal with all the securityfs issues first. However it > does require that securityfs (not necessarily imafs) be visible within > the mount namespace of the task doing the setup. Eric, would you be OK with that? Mimi
Re: [PATCH v2 2/2] tpm: reduce polling time to usecs for even finer granularity
On Tue, 2018-04-17 at 09:12 -0400, Nayna Jain wrote:
> The TPM burstcount and status commands are supposed to return very
> quickly [1][2]. This patch further reduces the TPM poll sleep time to usecs
> in get_burstcount() and wait_for_tpm_stat() by calling usleep_range()
> directly.
>
> After this change, performance on a TPM 1.2 with an 8 byte burstcount for
> 1000 extends improved from ~10.7 sec to ~7 sec.
>
> [1] From TCG Specification "TCG PC Client Specific TPM Interface
> Specification (TIS), Family 1.2":
>
> "NOTE : It takes roughly 330 ns per byte transfer on LPC. 256 bytes would
> take 84 us, which is a long time to stall the CPU. Chipsets may not be
> designed to post this much data to LPC; therefore, the CPU itself is
> stalled for much of this time. Sending 1 kB would take 350 μs. Therefore,
> even if the TPM_STS_x.burstCount field is a high value, software SHOULD
> be interruptible during this period."
>
> [2] From TCG Specification 2.0, "TCG PC Client Platform TPM Profile
> (PTP) Specification":
>
> "It takes roughly 330 ns per byte transfer on LPC. 256 bytes would take
> 84 us. Chipsets may not be designed to post this much data to LPC;
> therefore, the CPU itself is stalled for much of this time. Sending 1 kB
> would take 350 us. Therefore, even if the TPM_STS_x.burstCount field is a
> high value, software should be interruptible during this period. For SPI,
> assuming 20MHz clock and 64-byte transfers, it would take about 120 usec
> to move 256B of data. Sending 1kB would take about 500 usec. If the
> transactions are done using 4 bytes at a time, then it would take about
> 1 msec. to transfer 1kB of data."
>
> Signed-off-by: Nayna Jain
Reviewed-by: Mimi Zohar
> ---
> drivers/char/tpm/tpm.h | 4 +++-
> drivers/char/tpm/tpm_tis_core.c | 5 +++--
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 7e797377e1eb..f0e4d290c347 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -54,7 +54,9 @@ enum tpm_timeout {
> TPM_TIMEOUT = 5,/* msecs */
> TPM_TIMEOUT_RETRY = 100, /* msecs */
> TPM_TIMEOUT_RANGE_US = 300, /* usecs */
> - TPM_TIMEOUT_POLL = 1/* msecs */
> + TPM_TIMEOUT_POLL = 1, /* msecs */
> + TPM_TIMEOUT_USECS_MIN = 100, /* usecs */
> + TPM_TIMEOUT_USECS_MAX = 500 /* usecs */
> };
>
> /* TPM addresses */
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 021e6b68f2db..5bba5c662423 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -84,7 +84,8 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
> }
> } else {
> do {
> - tpm_msleep(TPM_TIMEOUT_POLL);
> + usleep_range(TPM_TIMEOUT_USECS_MIN,
> + TPM_TIMEOUT_USECS_MAX);
> status = chip->ops->status(chip);
> if ((status & mask) == mask)
> return 0;
> @@ -226,7 +227,7 @@ static int get_burstcount(struct tpm_chip *chip)
> burstcnt = (value >> 8) & 0x;
> if (burstcnt)
> return burstcnt;
> - tpm_msleep(TPM_TIMEOUT_POLL);
> + usleep_range(TPM_TIMEOUT_USECS_MIN, TPM_TIMEOUT_USECS_MAX);
> } while (time_before(jiffies, stop));
> return -EBUSY;
> }
Re: [PATCH v2 1/2] tpm: reduce poll sleep time in tpm_transmit()
On Tue, 2018-04-17 at 09:12 -0400, Nayna Jain wrote:
> The TPM polling code in tpm_transmit sleeps in each loop iteration for
> 5 msecs. However, the TPM might return earlier, and thus waiting for
> 5 msecs adds an unnecessary delay. This patch reduces the polling sleep
> time in tpm_transmit() from 5 msecs to 1 msecs.
>
> Additionally, this patch renames TPM_POLL_SLEEP and moves it to tpm.h as
> an enum value.
>
> After this change, performance on a TPM 1.2 with an 8 byte burstcount
> for 1000 extends improved from ~14 sec to ~10.7 sec.
>
> Signed-off-by: Nayna Jain
Reviewed-by: Mimi Zohar
> ---
> drivers/char/tpm/tpm-interface.c | 2 +-
> drivers/char/tpm/tpm.h | 3 ++-
> drivers/char/tpm/tpm_tis_core.c | 10 ++
> 3 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c
> b/drivers/char/tpm/tpm-interface.c
> index 9e80a953d693..a676d8ad5992 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -470,7 +470,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, struct
> tpm_space *space,
> goto out;
> }
>
> - tpm_msleep(TPM_TIMEOUT);
> + tpm_msleep(TPM_TIMEOUT_POLL);
> rmb();
> } while (time_before(jiffies, stop));
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f895fba4e20d..7e797377e1eb 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -53,7 +53,8 @@ enum tpm_const {
> enum tpm_timeout {
> TPM_TIMEOUT = 5,/* msecs */
> TPM_TIMEOUT_RETRY = 100, /* msecs */
> - TPM_TIMEOUT_RANGE_US = 300 /* usecs */
> + TPM_TIMEOUT_RANGE_US = 300, /* usecs */
> + TPM_TIMEOUT_POLL = 1/* msecs */
> };
>
> /* TPM addresses */
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index da074e3db19b..021e6b68f2db 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -31,12 +31,6 @@
> #include "tpm.h"
> #include "tpm_tis_core.h"
>
> -/* This is a polling delay to check for status and burstcount.
> - * As per ddwg input, expectation is that status check and burstcount
> - * check should return within few usecs.
> - */
> -#define TPM_POLL_SLEEP 1 /* msec */
> -
> static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value);
>
> static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
> @@ -90,7 +84,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
> }
> } else {
> do {
> - tpm_msleep(TPM_POLL_SLEEP);
> + tpm_msleep(TPM_TIMEOUT_POLL);
> status = chip->ops->status(chip);
> if ((status & mask) == mask)
> return 0;
> @@ -232,7 +226,7 @@ static int get_burstcount(struct tpm_chip *chip)
> burstcnt = (value >> 8) & 0x;
> if (burstcnt)
> return burstcnt;
> - tpm_msleep(TPM_POLL_SLEEP);
> + tpm_msleep(TPM_TIMEOUT_POLL);
> } while (time_before(jiffies, stop));
> return -EBUSY;
> }
Re: [RFC PATCH] rootfs: force mounting rootfs as tmpfs
Hi Rob, On Thu, 2018-02-01 at 17:34 -0600, Rob Landley wrote: > > On 02/01/2018 04:41 PM, Taras Kondratiuk wrote: > > Quoting Mimi Zohar (2018-02-01 13:51:52) > >> On Thu, 2018-02-01 at 11:09 -0600, Rob Landley wrote: > >>> On 02/01/2018 09:55 AM, Mimi Zohar wrote: > >>>> On Thu, 2018-02-01 at 09:20 -0600, Rob Landley wrote: > >>>> > >>>>>> With your patch and specifying "root=tmpfs", dracut is complaining: > >>>>>> > >>>>>> dracut: FATAL: Don't know how to handle 'root=tmpfs' > >>>>>> dracut: refusing to continue > >>>>> > >>>>> [googles]... I do not understand why this package exists. > >>>>> > >>>>> If you're switching to another root filesystem, using a tool that > >>>>> wikipedia[citation needed] says has no purpose but to switch to another > >>>>> root filesystem, (so let's reproduce the kernel infrastructure in > >>>>> userspace while leaving it the kernel too)... why do you need initramfs > >>>>> to be tmpfs? You're using it for half a second, then discarding it, > >>>>> what's the point of it being tmpfs? > >>>> > >>>> Unlike the kernel image which is signed by the distros, the initramfs > >>>> doesn't come signed, because it is built on the target system. Even > >>>> if the initramfs did come signed, it is beneficial to measure and > >>>> appraise the individual files in the initramfs. > >>> > >>> You can still shoot yourself in the foot with tmpfs. People mount a /run > >>> and a /tmp and then as a normal user you can go > >>> https://twitter.com/landley/status/959103235305951233 and maybe the > >>> default should be a little more clever there... > >>> > >>> I'll throw it on the todo heap. :) > >>> > >>>>> Sigh. If people are ok with having rootfs just be tmpfs whenever tmpfs > >>>>> is configured in, even when you're then going to overmount it with > >>>>> something else like you're doing, let's just _remove_ the test. If it > >>>>> can be tmpfs, have it be tmpfs. > >>>> > >>>> Very much appreciated! > >>> > >>> Not yet tested, but something like the attached? (Sorry for the > >>> half-finished doc changes in there, I'm at work and have a 5 minute > >>> break. I can test properly this evening if you don't get to it...) > >> > >> Yes, rootfs is being mounted as tmpfs. > > > > I don't think you can unconditionally replace ramfs with initramfs by > > default. Their behavior is different in some cases (e.g. pivot_root vs > > switch_root) > > Both are switch_root, you can't pivot_root off of either one. (Yes, I > hit that bug and reported it, and they fixed it, back in the day... > http://lists.busybox.net/pipermail/busybox/2006-March/053529.html ) > > > and it can break many systems that expect ramfs by default. > > The use case I told Mimi about off-list (since they stopped cc:ing the > list in one of their replies but the conversation continued) was the guy > who was extracting an initramfs bigger than 50% of system memory, which > worked with initramfs but failed with initmpfs. A quick google didn't > find the original message but it resulted in this blog entry from the > affected party: > > http://www.lightofdawn.org/blog/?viewDetailed=00128 > > I.E. yeah, I know, I need to redo these patches tonight. I'd really like to be able to have rootfs be a tmpfs filesystem. Any time estimate on this patch? thanks! Mimi
Re: [RFC PATCH v3 1/3] ima: extend clone() with IMA namespace support
[Cc'ing John Johansen] On Tue, 2018-03-27 at 18:01 -0500, Eric W. Biederman wrote: [...] > As such I expect the best way to create the ima namespace is by simply > writing to securityfs/imafs. Possibly before the user namespace is > even unshared. That would allow IMA to keep track of things from > before a container is created. My initial thought was to stage IMA namespacing with just IMA-audit first, followed by either IMA-measurement or IMA-appraisal. This would allow us to get the basic IMA namespacing framework working and defer dealing with the securityfs related namespacing of the IMA policy and measurement list issues to later. By tying IMA namespacing to a securityfs ima/unshare file, we would need to address the securityfs issues first. Mimi
[PATCH] lockdown: fix coordination of kernel module signature verification
If both IMA-appraisal and sig_enforce are enabled, then both signatures
are currently required. If the IMA-appraisal signature verification
fails, it could rely on the appended signature verification; but with the
lockdown patch set, the appended signature verification assumes that if
IMA-appraisal is enabled, it has verified the signature. Basically each
signature verification method would be relying on the other to verify the
kernel module signature.
This patch addresses the problem of requiring both kernel module signature
verification methods, when both are enabled, by verifying just the
appended signature.
Signed-off-by: Mimi Zohar
---
kernel/module.c | 4 +---
security/integrity/ima/ima_main.c | 7 ++-
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index 9c1709a05037..60861eb7bc4d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2803,9 +2803,7 @@ static int module_sig_check(struct load_info *info, int
flags,
if (sig_enforce) {
pr_notice("%s is rejected\n", reason);
return -EKEYREJECTED;
- }
-
- if (can_do_ima_check && is_ima_appraise_enabled())
+ } else if (can_do_ima_check && is_ima_appraise_enabled())
return 0;
if (kernel_is_locked_down(reason))
return -EPERM;
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 754ece08e1c6..2155b1f316a4 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -480,6 +480,7 @@ static int read_idmap[READING_MAX_ID] = {
int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id read_id)
{
+ bool sig_enforce = is_module_sig_enforced();
enum ima_hooks func;
u32 secid;
@@ -490,7 +491,11 @@ int ima_post_read_file(struct file *file, void *buf,
loff_t size,
return 0;
}
- if (!file && read_id == READING_MODULE) /* MODULE_SIG_FORCE enabled */
+ /*
+* If both IMA-appraisal and appended signature verification are
+* enabled, rely on the appended signature verification.
+*/
+ if (sig_enforce && read_id == READING_MODULE)
return 0;
/* permit signed certs */
--
2.7.5
[PATCH 3/3] ima: based on policy require signed kexec kernel images
The original kexec_load syscall can not verify file signatures. This
patch differentiates between the kexec_load and kexec_file_load
syscalls.
Signed-off-by: Mimi Zohar
---
security/integrity/ima/ima.h| 1 +
security/integrity/ima/ima_main.c | 9 +
security/integrity/ima/ima_policy.c | 2 ++
3 files changed, 12 insertions(+)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 35fe91aa1fc9..03c9c37ee345 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -233,6 +233,7 @@ int ima_policy_show(struct seq_file *m, void *v);
#define IMA_APPRAISE_MODULES 0x08
#define IMA_APPRAISE_FIRMWARE 0x10
#define IMA_APPRAISE_POLICY0x20
+#define IMA_APPRAISE_KEXEC 0x40
#ifdef CONFIG_IMA_APPRAISE
int ima_appraise_measurement(enum ima_hooks func,
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 74d0bd7e76d7..754ece08e1c6 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -444,6 +444,15 @@ int ima_read_file(struct file *file, enum
kernel_read_file_id read_id)
}
return 0; /* We rely on module signature checking */
}
+
+ if (!file && read_id == READING_KEXEC_IMAGE) {
+ if ((ima_appraise & IMA_APPRAISE_KEXEC) &&
+ (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+ pr_err("impossible to appraise a kernel image without a
file descriptor; try using kexec_file syscall.\n");
+ return -EACCES; /* INTEGRITY_UNKNOWN */
+ }
+ return 0;
+ }
return 0;
}
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index 1bdb5bc57568..3444352caf80 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -443,6 +443,8 @@ static int ima_appraise_flag(enum ima_hooks func)
return IMA_APPRAISE_FIRMWARE;
else if (func == POLICY_CHECK)
return IMA_APPRAISE_POLICY;
+ else if (func == KEXEC_KERNEL_CHECK)
+ return IMA_APPRAISE_KEXEC;
return 0;
}
--
2.7.5
[PATCH 2/3] kexec: call LSM hook for kexec_load syscall
Allow LSMs and IMA to differentiate between the kexec_load and
kexec_file_load syscalls by adding an "unnecessary" call to
security_kernel_read_file() in kexec_load. This would be similar to the
existing init_module syscall calling security_kernel_read_file().
Signed-off-by: Mimi Zohar
---
kernel/kexec.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/kernel/kexec.c b/kernel/kexec.c
index aed8fb2564b3..d1386cfc6796 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -195,11 +196,21 @@ static int do_kexec_load(unsigned long entry, unsigned
long nr_segments,
static inline int kexec_load_check(unsigned long nr_segments,
unsigned long flags)
{
+ int result;
+
/* We only trust the superuser with rebooting the system. */
if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
return -EPERM;
/*
+* Allow LSMs and IMA to differentiate between kexec_load and
+* kexec_file_load syscalls.
+*/
+ result = security_kernel_read_file(NULL, READING_KEXEC_IMAGE);
+ if (result < 0)
+ return result;
+
+ /*
* Verify we have a legal set of flags
* This leaves us room for future extensions.
*/
--
2.7.5
[PATCH 0/3] kexec: limit kexec_load syscall
In environments that require the kexec kernel image to be signed, prevent using the kexec_load syscall. In order for LSMs and IMA to differentiate between kexec_load and kexec_file_load syscalls, this patch set adds a call to security_kernel_read_file() in kexec_load_check(). Signed-off-by: Mimi Zohar Mimi Zohar (3): ima: based on the "secure_boot" policy limit syscalls kexec: call LSM hook for kexec_load syscall ima: based on policy require signed kexec kernel images kernel/kexec.c | 11 +++ security/integrity/ima/ima.h| 1 + security/integrity/ima/ima_main.c | 9 + security/integrity/ima/ima_policy.c | 27 --- 4 files changed, 41 insertions(+), 7 deletions(-) -- 2.7.5
[PATCH 1/3] ima: based on the "secure_boot" policy limit syscalls
The builtin "secure_boot" policy adds IMA appraisal rules requiring kernel
modules (finit_module syscall), direct firmware load, kexec kernel image
(kexec_file_load syscall), and the IMA policy to be signed, but did not
prevent the other syscalls/methods from working. Loading an equivalent
custom policy containing these same rules would have prevented the other
syscalls/methods from working.
This patch refactors the code to load custom policies, defining a new
function named ima_appraise_flag(). The new function is called either
when loading the builtin "secure_boot" or custom policies.
Fixes: 503ceaef8e2e ("ima: define a set of appraisal rules requiring file
signatures")
Signed-off-by: Mimi Zohar
---
security/integrity/ima/ima_policy.c | 25 ++---
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index d89bebf85421..1bdb5bc57568 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -435,6 +435,17 @@ void ima_update_policy_flag(void)
ima_policy_flag &= ~IMA_APPRAISE;
}
+static int ima_appraise_flag(enum ima_hooks func)
+{
+ if (func == MODULE_CHECK)
+ return IMA_APPRAISE_MODULES;
+ else if (func == FIRMWARE_CHECK)
+ return IMA_APPRAISE_FIRMWARE;
+ else if (func == POLICY_CHECK)
+ return IMA_APPRAISE_POLICY;
+ return 0;
+}
+
/**
* ima_init_policy - initialize the default measure rules.
*
@@ -473,9 +484,12 @@ void __init ima_init_policy(void)
* Insert the appraise rules requiring file signatures, prior to
* any other appraise rules.
*/
- for (i = 0; i < secure_boot_entries; i++)
+ for (i = 0; i < secure_boot_entries; i++) {
list_add_tail(&secure_boot_rules[i].list,
&ima_default_rules);
+ temp_ima_appraise |=
+ ima_appraise_flag(secure_boot_rules[i].func);
+ }
for (i = 0; i < appraise_entries; i++) {
list_add_tail(&default_appraise_rules[i].list,
@@ -917,12 +931,9 @@ static int ima_parse_rule(char *rule, struct
ima_rule_entry *entry)
}
if (!result && (entry->action == UNKNOWN))
result = -EINVAL;
- else if (entry->func == MODULE_CHECK)
- temp_ima_appraise |= IMA_APPRAISE_MODULES;
- else if (entry->func == FIRMWARE_CHECK)
- temp_ima_appraise |= IMA_APPRAISE_FIRMWARE;
- else if (entry->func == POLICY_CHECK)
- temp_ima_appraise |= IMA_APPRAISE_POLICY;
+ else if (entry->action == APPRAISE)
+ temp_ima_appraise |= ima_appraise_flag(entry->func);
+
audit_log_format(ab, "res=%d", !result);
audit_log_end(ab);
return result;
--
2.7.5
Re: [PATCH 06/24] kexec_load: Disable at runtime if the kernel is locked down
On Wed, 2018-04-11 at 16:09 -0400, Mimi Zohar wrote: > On Wed, 2018-04-11 at 14:00 -0500, Eric W. Biederman wrote: > > David Howells writes: > > > > > From: Matthew Garrett > > > > > > The kexec_load() syscall permits the loading and execution of arbitrary > > > code in ring 0, which is something that lock-down is meant to prevent. It > > > makes sense to disable kexec_load() in this situation. > > > > > > This does not affect kexec_file_load() syscall which can check for a > > > signature on the image to be booted. > > > > Maybing I am missing it but I am not seeing anything that would require > > kexec_file_load be configured such that it checks the loaded kernel. > > > > Without that I don't see the point of disabling kexec_load. > > > > Nacked-by: "Eric W. Biederman" > > The IMA "secure boot" policy requires the kexec image to be signed. > This call to kernel_is_locked_down() could be replaced with a call > to security_kernel_read_file(NULL, READING_KEXEC_IMAGE). > > It would be similar to the existing init_module syscall calling > security_kernel_read_file(). David, enabling the IMA-appraisal "secure boot" policy should probably not be dependent on lockdown either. Mimi
Re: [PATCH 06/24] kexec_load: Disable at runtime if the kernel is locked down
On Wed, 2018-04-11 at 14:00 -0500, Eric W. Biederman wrote: > David Howells writes: > > > From: Matthew Garrett > > > > The kexec_load() syscall permits the loading and execution of arbitrary > > code in ring 0, which is something that lock-down is meant to prevent. It > > makes sense to disable kexec_load() in this situation. > > > > This does not affect kexec_file_load() syscall which can check for a > > signature on the image to be booted. > > Maybing I am missing it but I am not seeing anything that would require > kexec_file_load be configured such that it checks the loaded kernel. > > Without that I don't see the point of disabling kexec_load. > > Nacked-by: "Eric W. Biederman" The IMA "secure boot" policy requires the kexec image to be signed. This call to kernel_is_locked_down() could be replaced with a call to security_kernel_read_file(NULL, READING_KEXEC_IMAGE). It would be similar to the existing init_module syscall calling security_kernel_read_file(). Mimi
Re: [PATCH] tpm: moves the delay_msec increment after sleep in tpm_transmit()
On Sat, 2018-04-07 at 13:36 +0300, Jarkko Sakkinen wrote:
> On Fri, Apr 06, 2018 at 02:03:37PM +0530, Nayna Jain wrote:
> > On 04/05/2018 03:42 PM, Jarkko Sakkinen wrote:
> > > On Mon, Apr 02, 2018 at 09:50:06PM +0530, Nayna Jain wrote:
> > > > Commit e2fb992d82c6 ("tpm: add retry logic") introduced a new loop to
> > > > handle the TPM2_RC_RETRY error. The loop retries the command after
> > > > sleeping for the specified time, which is incremented exponentially in
> > > > every iteration. This patch fixes the initial sleep to be the default
> > > > sleep time.
> > > I think I understand the code change but do not understand what the
> > > long description.
> >
> > It tells that the first sleep is delay_msec * 2 and not delay_msec.
>
> So the problem is that the loop doubles the time before sleeping
> for the first time. This is missing from the description. Please
> refine it in some way.
Sure, how about replacing the last line of the patch description with:
Unfortunately, the loop doubles the time before sleeping, causing the
initial sleep to be doubled. This patch fixes the initial sleep time.
If this change is acceptable, do you want to make the change or should Nayna
repost the patch?
thanks,
Mimi
Re: An actual suggestion (Re: [GIT PULL] Kernel lockdown for secure boot)
On Thu, 2018-04-05 at 10:16 +0800, joeyli wrote: > Hi David, > > On Wed, Apr 04, 2018 at 05:17:24PM +0100, David Howells wrote: > > Andy Lutomirski wrote: > > > > > Since this thread has devolved horribly, I'm going to propose a solution. > > > > > > 1. Split the "lockdown" state into three levels: (please don't > > > bikeshed about the names right now.) > > > > > > LOCKDOWN_NONE: normal behavior > > > > > > LOCKDOWN_PROTECT_INTEGREITY: kernel tries to keep root from writing to > > > kernel memory > > > > > > LOCKDOWN_PROTECT_INTEGRITY_AND_SECRECY: kernel tries to keep root from > > > reading or writing kernel memory. > > > > In theory, it's good idea, but in practice it's not as easy to implement as > > I > > think you think. > > > > Let me list here the things that currently get restricted by lockdown: > > > [...snip] > > (5) Kexec. > > > > About IMA with kernel module signing and kexec(not on x86_64 yet)... Only carrying the measurement list across kexec is architecture specific, but everything else should work. > Because IMA can be used to verify the integrity of kernel module or even > the image for kexec. I think that the > IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY must be enabled at runtime > when kernel is locked-down. I think we need to understand the problem a bit better. Is the problem that you're using the secondary keyring and loading the UEFI keys onto the secondary keyring? > Because the root can enroll master key to keyring then IMA trusts the ima key > derived from master key. It causes that the arbitrary signed module can be > loaded > when the root compromised. With only the builtin keyring, only keys signed by a builtin key can be added to the IMA keyring. Mimi
Re: [PATCH v6 04/12] ima: Introduce is_ima_sig()
On Fri, 2018-03-16 at 17:38 -0300, Thiago Jung Bauermann wrote:
> With the introduction of another IMA signature type (modsig), some places
> will need to check for both of them. It is cleaner to do that if there's a
> helper function to tell whether an xattr_value represents an IMA
> signature.
Initially the function name "is_ima_sig" is fine, since it reflects
the 'imasig' type. Having a more generic function name would be
better when adding 'modsig' support. As long as the function is
locally define, we can drop 'ima' from the name. Perhaps something
like has_signature or is_signed() would be preferable.
Mimi
>
> Suggested-by: Mimi Zohar
> Signed-off-by: Thiago Jung Bauermann
> ---
> security/integrity/ima/ima.h | 5 +
> security/integrity/ima/ima_appraise.c | 7 +++
> security/integrity/ima/ima_template_lib.c | 2 +-
> 3 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 35fe91aa1fc9..4bafa6a97967 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -155,6 +155,11 @@ unsigned long ima_get_binary_runtime_size(void);
> int ima_init_template(void);
> void ima_init_template_list(void);
>
> +static inline bool is_ima_sig(const struct evm_ima_xattr_data *xattr_value)
> +{
> + return xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG;
> +}
> +
> /*
> * used to protect h_table and sha_table
> */
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index a6b2995b7d0b..01172eab297b 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -325,15 +325,14 @@ int ima_appraise_measurement(enum ima_hooks func,
> } else if (status != INTEGRITY_PASS) {
> /* Fix mode, but don't replace file signatures. */
> if ((ima_appraise & IMA_APPRAISE_FIX) &&
> - (!xattr_value ||
> - xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> + !is_ima_sig(xattr_value)) {
> if (!ima_fix_xattr(dentry, iint))
> status = INTEGRITY_PASS;
> }
>
> /* Permit new files with file signatures, but without data. */
> if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
> - xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
> + is_ima_sig(xattr_value)) {
> status = INTEGRITY_PASS;
> }
>
> @@ -448,7 +447,7 @@ int ima_inode_setxattr(struct dentry *dentry, const char
> *xattr_name,
> if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
> return -EINVAL;
> ima_reset_appraise_flags(d_backing_inode(dentry),
> - xvalue->type == EVM_IMA_XATTR_DIGSIG);
> + is_ima_sig(xvalue));
> result = 0;
> }
> return result;
> diff --git a/security/integrity/ima/ima_template_lib.c
> b/security/integrity/ima/ima_template_lib.c
> index 5afaa53decc5..afb52a90e532 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -380,7 +380,7 @@ int ima_eventsig_init(struct ima_event_data *event_data,
> {
> struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
>
> - if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG))
> + if (!is_ima_sig(xattr_value))
> return 0;
>
> return ima_write_template_field_data(xattr_value, event_data->xattr_len,
>
Re: [PATCH v6 11/12] ima: Implement support for module-style appended signatures
On Fri, 2018-03-16 at 17:38 -0300, Thiago Jung Bauermann wrote:
> This patch actually implements the appraise_type=imasig|modsig option,
> allowing IMA to read and verify modsig signatures.
>
> In case both are present in the same file, IMA will first check whether the
> key used by the xattr signature is present in the kernel keyring. If not,
> it will try the appended signature.
Yes, this sounds right.
>
> Signed-off-by: Thiago Jung Bauermann
> ---
> security/integrity/ima/ima.h | 11 +++-
> security/integrity/ima/ima_appraise.c | 53
> +++
> security/integrity/ima/ima_main.c | 21 +++---
> 3 files changed, 74 insertions(+), 11 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 49aef56dc96d..c11ccb7c5bfb 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -157,7 +157,8 @@ void ima_init_template_list(void);
>
> static inline bool is_ima_sig(const struct evm_ima_xattr_data *xattr_value)
> {
> - return xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG;
> + return xattr_value && (xattr_value->type == EVM_IMA_XATTR_DIGSIG ||
> +xattr_value->type == IMA_MODSIG);
> }
>
> /*
> @@ -253,6 +254,8 @@ enum integrity_status ima_get_cache_status(struct
> integrity_iint_cache *iint,
> enum ima_hooks func);
> enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
>int xattr_len);
> +bool ima_xattr_sig_known_key(const struct evm_ima_xattr_data *xattr_value,
> + int xattr_len);
> int ima_read_xattr(struct dentry *dentry,
> struct evm_ima_xattr_data **xattr_value);
>
> @@ -291,6 +294,12 @@ ima_get_hash_algo(struct evm_ima_xattr_data
> *xattr_value, int xattr_len)
> return ima_hash_algo;
> }
>
> +static inline bool ima_xattr_sig_known_key(const struct evm_ima_xattr_data
> +*xattr_value, int xattr_len)
> +{
> + return false;
> +}
> +
> static inline int ima_read_xattr(struct dentry *dentry,
>struct evm_ima_xattr_data **xattr_value)
> {
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index 01172eab297b..84e0fd5a19c8 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -189,6 +189,22 @@ enum hash_algo ima_get_hash_algo(struct
> evm_ima_xattr_data *xattr_value,
> return ima_hash_algo;
> }
>
> +bool ima_xattr_sig_known_key(const struct evm_ima_xattr_data *xattr_value,
> + int xattr_len)
> +{
> + struct key *keyring;
> +
> + if (xattr_value->type != EVM_IMA_XATTR_DIGSIG)
> + return false;
> +
> + keyring = integrity_keyring_from_id(INTEGRITY_KEYRING_IMA);
> + if (IS_ERR(keyring))
> + return false;
> +
> + return asymmetric_sig_has_known_key(keyring, (const char *) xattr_value,
> + xattr_len);
> +}
> +
> int ima_read_xattr(struct dentry *dentry,
> struct evm_ima_xattr_data **xattr_value)
> {
> @@ -221,8 +237,12 @@ int ima_appraise_measurement(enum ima_hooks func,
> struct inode *inode = d_backing_inode(dentry);
> enum integrity_status status = INTEGRITY_UNKNOWN;
> int rc = xattr_len, hash_start = 0;
> + size_t xattr_contents_len;
> + void *xattr_contents;
>
> - if (!(inode->i_opflags & IOP_XATTR))
> + /* If not appraising a modsig, we need an xattr. */
> + if ((xattr_value == NULL || xattr_value->type != IMA_MODSIG) &&
> + !(inode->i_opflags & IOP_XATTR))
> return INTEGRITY_UNKNOWN;
>
> if (rc <= 0) {
> @@ -241,13 +261,29 @@ int ima_appraise_measurement(enum ima_hooks func,
> goto out;
> }
>
> - status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
> + /*
> + * If it's a modsig, we don't have the xattr contents to pass to
> + * evm_verifyxattr().
> + */
> + if (xattr_value->type == IMA_MODSIG) {
> + xattr_contents = NULL;
> + xattr_contents_len = 0;
> + } else {
> + xattr_contents = xattr_value;
> + xattr_contents_len = xattr_len;
> + }
> +
> + status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_contents,
> + xattr_contents_len, iint);
> switch (status) {
> case INTEGRITY_PASS:
> case INTEGRITY_PASS_IMMUTABLE:
> case INTEGRITY_UNKNOWN:
> break;
> case INTEGRITY_NOXATTRS:/* No EVM protected xattrs. */
> + /* It's fine not to have xattrs when using a modsig. */
> + if (xattr_value->type == IMA_MODSIG)
> + break;
> case INTEGRITY_NOLABEL: /* No security.evm xa
Re: [PATCH v6 12/12] ima: Write modsig to the measurement list
On Fri, 2018-03-16 at 17:38 -0300, Thiago Jung Bauermann wrote:
> Define new "d-sig" template field which holds the digest that is expected
> to match the one contained in the modsig.
>
> Also add modsig support to the "sig" template field, allowing the the
> contents of the modsig to be included in the measurement list.
Although including the appended signature in the template data doesn't
make sense on its own, as the file digest (without the appended
signature) is needed to validate the appended signature, defining a
new template field and its usage should be independent of other
changes.
Mimi
>
> Suggested-by: Mimi Zohar
> Signed-off-by: Thiago Jung Bauermann
> ---
> Documentation/security/IMA-templates.rst | 5
> security/integrity/ima/ima_template.c | 4 ++-
> security/integrity/ima/ima_template_lib.c | 47
> +--
> security/integrity/ima/ima_template_lib.h | 2 ++
> 4 files changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/security/IMA-templates.rst
> b/Documentation/security/IMA-templates.rst
> index 2cd0e273cc9a..f2a0f4225857 100644
> --- a/Documentation/security/IMA-templates.rst
> +++ b/Documentation/security/IMA-templates.rst
> @@ -68,6 +68,11 @@ descriptors by adding their identifier to the format string
> - 'd-ng': the digest of the event, calculated with an arbitrary hash
> algorithm (field format: [:]digest, where the digest
> prefix is shown only if the hash algorithm is not SHA1 or MD5);
> + - 'd-sig': the digest of the event for files that have an appended modsig.
> This
> + field is calculated without including the modsig and thus will differ from
> + the total digest of the file, but it is what should match the digest
> + contained in the modsig (if it doesn't, the signature is invalid). It is
> + shown in the same format as 'd-ng';
> - 'n-ng': the name of the event, without size limitations;
> - 'sig': the file signature.
>
> diff --git a/security/integrity/ima/ima_template.c
> b/security/integrity/ima/ima_template.c
> index 30db39b23804..36fc32f538b5 100644
> --- a/security/integrity/ima/ima_template.c
> +++ b/security/integrity/ima/ima_template.c
> @@ -43,8 +43,10 @@ static struct ima_template_field supported_fields[] = {
>.field_show = ima_show_template_string},
> {.field_id = "sig", .field_init = ima_eventsig_init,
>.field_show = ima_show_template_sig},
> + {.field_id = "d-sig", .field_init = ima_eventdigest_sig_init,
> + .field_show = ima_show_template_digest_ng},
> };
> -#define MAX_TEMPLATE_NAME_LEN 15
> +#define MAX_TEMPLATE_NAME_LEN 24
>
> static struct ima_template_desc *ima_template;
> static struct ima_template_desc *lookup_template_desc(const char *name);
> diff --git a/security/integrity/ima/ima_template_lib.c
> b/security/integrity/ima/ima_template_lib.c
> index afb52a90e532..1dca082cce43 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -220,7 +220,8 @@ int ima_parse_buf(void *bufstartp, void *bufendp, void
> **bufcurp,
> return 0;
> }
>
> -static int ima_eventdigest_init_common(u8 *digest, u32 digestsize, u8
> hash_algo,
> +static int ima_eventdigest_init_common(const u8 *digest, u32 digestsize,
> +u8 hash_algo,
> struct ima_field_data *field_data)
> {
> /*
> @@ -323,6 +324,35 @@ int ima_eventdigest_ng_init(struct ima_event_data
> *event_data,
> hash_algo, field_data);
> }
>
> +/*
> + * This function writes the digest of the file which is expected to match the
> + * digest contained in the file's embedded signature.
> + */
> +int ima_eventdigest_sig_init(struct ima_event_data *event_data,
> + struct ima_field_data *field_data)
> +{
> + struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
> + enum hash_algo hash_algo = HASH_ALGO_SHA1;
> + const u8 *cur_digest = NULL;
> + u8 cur_digestsize = 0;
> + int ret;
> +
> + if (!xattr_value || xattr_value->type != IMA_MODSIG)
> + return 0;
> +
> + if (event_data->violation) /* recording a violation. */
> + goto out;
> +
> + ret = ima_get_modsig_hash(xattr_value, &hash_algo, &cur_digest,
> + &cur_digestsize);
> + if (ret)
> + return ret;
> +
> + out:
> + return ima_eventdigest_init_common(cur_digest, cur_digestsize,
> +
Re: [PATCH v6 03/12] PKCS#7: Introduce pkcs7_get_digest()
On Fri, 2018-03-16 at 17:38 -0300, Thiago Jung Bauermann wrote:
> IMA will need to access the digest of the PKCS7 message (as calculated by
> the kernel) before the signature is verified, so introduce
> pkcs7_get_digest() for that purpose.
>
> Also, modify pkcs7_digest() to detect when the digest was already
> calculated so that it doesn't have to do redundant work. Verifying that
> sinfo->sig->digest isn't NULL is sufficient because both places which
> allocate sinfo->sig (pkcs7_parse_message() and pkcs7_note_signed_info())
> use kzalloc() so sig->digest is always initialized to zero.
>
> Signed-off-by: Thiago Jung Bauermann
> Cc: David Howells
> Cc: Herbert Xu
> Cc: "David S. Miller"
Reviewed-by: Mimi Zohar
> ---
> crypto/asymmetric_keys/pkcs7_verify.c | 25 +
> include/crypto/pkcs7.h| 3 +++
> 2 files changed, 28 insertions(+)
>
> diff --git a/crypto/asymmetric_keys/pkcs7_verify.c
> b/crypto/asymmetric_keys/pkcs7_verify.c
> index 39e6de0c2761..bd02360f8be5 100644
> --- a/crypto/asymmetric_keys/pkcs7_verify.c
> +++ b/crypto/asymmetric_keys/pkcs7_verify.c
> @@ -33,6 +33,10 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
>
> kenter(",%u,%s", sinfo->index, sinfo->sig->hash_algo);
>
> + /* The digest was calculated already. */
> + if (sig->digest)
> + return 0;
> +
> if (!sinfo->sig->hash_algo)
> return -ENOPKG;
>
> @@ -122,6 +126,27 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
> return ret;
> }
>
> +int pkcs7_get_digest(struct pkcs7_message *pkcs7, const u8 **buf, u8 *len)
> +{
> + struct pkcs7_signed_info *sinfo = pkcs7->signed_infos;
> + int ret;
> +
> + /*
> + * This function doesn't support messages with more than one signature.
> + */
> + if (sinfo == NULL || sinfo->next != NULL)
> + return -EBADMSG;
> +
> + ret = pkcs7_digest(pkcs7, sinfo);
> + if (ret)
> + return ret;
> +
> + *buf = sinfo->sig->digest;
> + *len = sinfo->sig->digest_size;
> +
> + return 0;
> +}
> +
> /*
> * Find the key (X.509 certificate) to use to verify a PKCS#7 message.
> PKCS#7
> * uses the issuer's name and the issuing certificate serial number for
> diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h
> index 6f51d0cb6d12..cfaea9c37f4a 100644
> --- a/include/crypto/pkcs7.h
> +++ b/include/crypto/pkcs7.h
> @@ -46,4 +46,7 @@ extern int pkcs7_verify(struct pkcs7_message *pkcs7,
> extern int pkcs7_supply_detached_data(struct pkcs7_message *pkcs7,
> const void *data, size_t datalen);
>
> +extern int pkcs7_get_digest(struct pkcs7_message *pkcs7, const u8 **buf,
> + u8 *len);
> +
> #endif /* _CRYPTO_PKCS7_H */
>
Re: [PATCH v6 02/12] PKCS#7: Introduce pkcs7_get_message_sig() and verify_pkcs7_message_sig()
Hi Thiago,
On Fri, 2018-03-16 at 17:38 -0300, Thiago Jung Bauermann wrote:
> IMA will need to know the key that signed a given PKCS#7 message, so add
> pkcs7_get_message_sig().
>
> It will also need to verify an already parsed PKCS#7 message. For this
> purpose, add verify_pkcs7_message_sig() which takes a struct pkcs7_message
> for verification instead of the raw bytes that verify_pkcs7_signature()
> takes.
The title "PKCS#7: refactor verify_pkcs7_signature()" might be more
appropriate. The patch description would then explain why it needs to
be refactored. In this case, verify_pkcs7_signature() verifies the
signature using keys on the builtin and secondary keyrings. IMA-
appraisal needs to verify the signature using keys on its keyring.
The patch itself looks good!
Reviewed-by: Mimi Zohar
> Signed-off-by: Thiago Jung Bauermann
> Cc: David Howells
> Cc: David Woodhouse
> Cc: Herbert Xu
> Cc: "David S. Miller"
> ---
> certs/system_keyring.c| 61
> ++-
> crypto/asymmetric_keys/pkcs7_parser.c | 16 +
> include/crypto/pkcs7.h| 2 ++
> include/linux/verification.h | 10 ++
> 4 files changed, 73 insertions(+), 16 deletions(-)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 6251d1b27f0c..7ddc8b7a3062 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -190,33 +190,27 @@ late_initcall(load_system_certificate_list);
> #ifdef CONFIG_SYSTEM_DATA_VERIFICATION
>
> /**
> - * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
> + * verify_pkcs7_message_sig - Verify a PKCS#7-based signature on system data.
> * @data: The data to be verified (NULL if expecting internal data).
> * @len: Size of @data.
> - * @raw_pkcs7: The PKCS#7 message that is the signature.
> - * @pkcs7_len: The size of @raw_pkcs7.
> + * @pkcs7: The PKCS#7 message that is the signature.
> * @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
> * (void *)1UL for all trusted keys).
> * @usage: The use to which the key is being put.
> * @view_content: Callback to gain access to content.
> * @ctx: Context for callback.
> */
> -int verify_pkcs7_signature(const void *data, size_t len,
> -const void *raw_pkcs7, size_t pkcs7_len,
> -struct key *trusted_keys,
> -enum key_being_used_for usage,
> -int (*view_content)(void *ctx,
> -const void *data, size_t len,
> -size_t asn1hdrlen),
> -void *ctx)
> +int verify_pkcs7_message_sig(const void *data, size_t len,
> + struct pkcs7_message *pkcs7,
> + struct key *trusted_keys,
> + enum key_being_used_for usage,
> + int (*view_content)(void *ctx,
> + const void *data, size_t len,
> + size_t asn1hdrlen),
> + void *ctx)
> {
> - struct pkcs7_message *pkcs7;
> int ret;
>
> - pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
> - if (IS_ERR(pkcs7))
> - return PTR_ERR(pkcs7);
> -
> /* The data should be detached - so we need to supply it. */
> if (data && pkcs7_supply_detached_data(pkcs7, data, len) < 0) {
> pr_err("PKCS#7 signature with non-detached data\n");
> @@ -258,6 +252,41 @@ int verify_pkcs7_signature(const void *data, size_t len,
> }
>
> error:
> + pr_devel("<==%s() = %d\n", __func__, ret);
> + return ret;
> +}
> +
> +/**
> + * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
> + * @data: The data to be verified (NULL if expecting internal data).
> + * @len: Size of @data.
> + * @raw_pkcs7: The PKCS#7 message that is the signature.
> + * @pkcs7_len: The size of @raw_pkcs7.
> + * @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
> + * (void *)1UL for all trusted keys).
> + * @usage: The use to which the key is being put.
> + * @view_content: Callback to gain access to content.
> + * @ctx: Context for callback.
> + */
> +int verify_pkcs7_signature(const void *data, size_t len,
> +const void *raw_pkcs7, size_t pkcs7_len,
> +struct key *trusted_keys,
> +enum key_being_used_for usage,
> +
Re: [PATCH v6 07/12] integrity: Select CONFIG_KEYS instead of depending on it
On Fri, 2018-03-16 at 17:38 -0300, Thiago Jung Bauermann wrote: > This avoids a dependency cycle in CONFIG_IMA_APPRAISE_MODSIG (introduced by > a later patch in this series): it will select CONFIG_MODULE_SIG_FORMAT > which in turn selects CONFIG_KEYS. Kconfig then complains that > CONFIG_INTEGRITY_SIGNATURE depends on CONFIG_KEYS. > > Signed-off-by: Thiago Jung Bauermann Signed-off-by: Mimi Zohar > --- > security/integrity/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig > index da9565891738..0d642e0317c7 100644 > --- a/security/integrity/Kconfig > +++ b/security/integrity/Kconfig > @@ -17,8 +17,8 @@ if INTEGRITY > > config INTEGRITY_SIGNATURE > bool "Digital signature verification using multiple keyrings" > - depends on KEYS > default n > + select KEYS > select SIGNATURE > help > This option enables digital signature verification support >
Re: [PATCH v6 06/12] integrity: Introduce asymmetric_sig_has_known_key()
On Fri, 2018-03-16 at 17:38 -0300, Thiago Jung Bauermann wrote:
> IMA will only look for a modsig if the xattr sig references a key which is
> not in the expected kernel keyring. To that end, introduce
> asymmetric_sig_has_known_key().
>
> The logic of extracting the key used in the xattr sig is factored out from
> asymmetric_verify() so that it can be used by the new function.
>
> Signed-off-by: Thiago Jung Bauermann
Signed-off-by: Mimi Zohar
> ---
> security/integrity/digsig_asymmetric.c | 44
> +-
> security/integrity/integrity.h | 8 +++
> 2 files changed, 41 insertions(+), 11 deletions(-)
>
> diff --git a/security/integrity/digsig_asymmetric.c
> b/security/integrity/digsig_asymmetric.c
> index ab6a029062a1..241647970c19 100644
> --- a/security/integrity/digsig_asymmetric.c
> +++ b/security/integrity/digsig_asymmetric.c
> @@ -79,26 +79,48 @@ static struct key *request_asymmetric_key(struct key
> *keyring, uint32_t keyid)
> return key;
> }
>
> -int asymmetric_verify(struct key *keyring, const char *sig,
> - int siglen, const char *data, int datalen)
> +static struct key *asymmetric_key_from_sig(struct key *keyring, const char
> *sig,
> +int siglen)
> {
> - struct public_key_signature pks;
> - struct signature_v2_hdr *hdr = (struct signature_v2_hdr *)sig;
> - struct key *key;
> - int ret = -ENOMEM;
> + const struct signature_v2_hdr *hdr = (struct signature_v2_hdr *) sig;
>
> if (siglen <= sizeof(*hdr))
> - return -EBADMSG;
> + return ERR_PTR(-EBADMSG);
>
> siglen -= sizeof(*hdr);
>
> if (siglen != be16_to_cpu(hdr->sig_size))
> - return -EBADMSG;
> + return ERR_PTR(-EBADMSG);
>
> if (hdr->hash_algo >= HASH_ALGO__LAST)
> - return -ENOPKG;
> + return ERR_PTR(-ENOPKG);
> +
> + return request_asymmetric_key(keyring, be32_to_cpu(hdr->keyid));
> +}
> +
> +bool asymmetric_sig_has_known_key(struct key *keyring, const char *sig,
> + int siglen)
> +{
> + struct key *key;
> +
> + key = asymmetric_key_from_sig(keyring, sig, siglen);
> + if (IS_ERR_OR_NULL(key))
> + return false;
> +
> + key_put(key);
> +
> + return true;
> +}
> +
> +int asymmetric_verify(struct key *keyring, const char *sig,
> + int siglen, const char *data, int datalen)
> +{
> + struct public_key_signature pks;
> + struct signature_v2_hdr *hdr = (struct signature_v2_hdr *)sig;
> + struct key *key;
> + int ret = -ENOMEM;
>
> - key = request_asymmetric_key(keyring, be32_to_cpu(hdr->keyid));
> + key = asymmetric_key_from_sig(keyring, sig, siglen);
> if (IS_ERR(key))
> return PTR_ERR(key);
>
> @@ -109,7 +131,7 @@ int asymmetric_verify(struct key *keyring, const char
> *sig,
> pks.digest = (u8 *)data;
> pks.digest_size = datalen;
> pks.s = hdr->sig;
> - pks.s_size = siglen;
> + pks.s_size = siglen - sizeof(*hdr);
> ret = verify_signature(key, &pks);
> key_put(key);
> pr_debug("%s() = %d\n", __func__, ret);
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 2d245f44ca26..4c381b992e11 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -179,12 +179,20 @@ static inline int integrity_init_keyring(const unsigned
> int id)
> #ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
> int asymmetric_verify(struct key *keyring, const char *sig,
> int siglen, const char *data, int datalen);
> +bool asymmetric_sig_has_known_key(struct key *keyring, const char *sig,
> + int siglen);
> #else
> static inline int asymmetric_verify(struct key *keyring, const char *sig,
> int siglen, const char *data, int datalen)
> {
> return -EOPNOTSUPP;
> }
> +
> +static inline bool asymmetric_sig_has_known_key(struct key *keyring,
> + const char *sig, int siglen)
> +{
> + return false;
> +}
> #endif
>
> #ifdef CONFIG_IMA_LOAD_X509
>
Re: [PATCH v6 05/12] integrity: Introduce integrity_keyring_from_id()
On Fri, 2018-03-16 at 17:38 -0300, Thiago Jung Bauermann wrote:
> IMA will need to obtain the keyring used to verify file signatures so that
> it can verify the module-style signature appended to files.
>
> Signed-off-by: Thiago Jung Bauermann
Signed-off-by: Mimi Zohar
> ---
> security/integrity/digsig.c| 28 +---
> security/integrity/integrity.h | 6 ++
> 2 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 6f9e4ce568cd..e641a67b9fc7 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -48,11 +48,10 @@ static bool init_keyring __initdata;
> #define restrict_link_to_ima restrict_link_by_builtin_trusted
> #endif
>
> -int integrity_digsig_verify(const unsigned int id, const char *sig, int
> siglen,
> - const char *digest, int digestlen)
> +struct key *integrity_keyring_from_id(const unsigned int id)
> {
> - if (id >= INTEGRITY_KEYRING_MAX || siglen < 2)
> - return -EINVAL;
> + if (id >= INTEGRITY_KEYRING_MAX)
> + return ERR_PTR(-EINVAL);
>
> if (!keyring[id]) {
> keyring[id] =
> @@ -61,17 +60,32 @@ int integrity_digsig_verify(const unsigned int id, const
> char *sig, int siglen,
> int err = PTR_ERR(keyring[id]);
> pr_err("no %s keyring: %d\n", keyring_name[id], err);
> keyring[id] = NULL;
> - return err;
> + return ERR_PTR(err);
> }
> }
>
> + return keyring[id];
> +}
> +
> +int integrity_digsig_verify(const unsigned int id, const char *sig, int
> siglen,
> + const char *digest, int digestlen)
> +{
> + struct key *keyring;
> +
> + if (siglen < 2)
> + return -EINVAL;
> +
> + keyring = integrity_keyring_from_id(id);
> + if (IS_ERR(keyring))
> + return PTR_ERR(keyring);
> +
> switch (sig[1]) {
> case 1:
> /* v1 API expect signature without xattr type */
> - return digsig_verify(keyring[id], sig + 1, siglen - 1,
> + return digsig_verify(keyring, sig + 1, siglen - 1,
>digest, digestlen);
> case 2:
> - return asymmetric_verify(keyring[id], sig, siglen,
> + return asymmetric_verify(keyring, sig, siglen,
>digest, digestlen);
> }
>
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 79799a0d9195..2d245f44ca26 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -150,6 +150,7 @@ int integrity_kernel_read(struct file *file, loff_t
> offset,
>
> #ifdef CONFIG_INTEGRITY_SIGNATURE
>
> +struct key *integrity_keyring_from_id(const unsigned int id);
> int integrity_digsig_verify(const unsigned int id, const char *sig, int
> siglen,
> const char *digest, int digestlen);
>
> @@ -157,6 +158,11 @@ int __init integrity_init_keyring(const unsigned int id);
> int __init integrity_load_x509(const unsigned int id, const char *path);
> #else
>
> +static inline struct key *integrity_keyring_from_id(const unsigned int id)
> +{
> + return ERR_PTR(-EINVAL);
> +}
> +
> static inline int integrity_digsig_verify(const unsigned int id,
> const char *sig, int siglen,
> const char *digest, int digestlen)
>
Re: [RFC PATCH v2 1/3] ima: extend clone() with IMA namespace support
On Thu, 2018-03-15 at 15:35 -0500, Eric W. Biederman wrote: > Stefan Berger writes: > > On 03/15/2018 03:20 PM, Eric W. Biederman wrote: [..] > >> From previous conversations I remember that there is a legitimate > >> bootstrap problem for IMA. That needs to be looked at, and I am not > >> seeing that mentioned. > > > > IMA's log should not have a gap. So ideally we shouldn't have to write > > something > > into sysfs to spawn a new IMA namespace so that we don't miss whatever > > setup may > > have happened to get there, including the writing into procfs. IMA should be > > there right from the start. So a clone flag would be ideal for that. > > Please make that securityfs not sysfs. Sysfs should be about the > hardware not these higher level software details. I really don't want > to have to namespace sysfs more than I already have. > > As for the no gaps requirement. That is a powerful lever for ruling out > solutions that don't work as well. IMA-measurement and IMA-audit need to be enabled from the very beginning. The only reason we differentiate between IMA-measurement and IMA-audit from IMA-appraisal is simply because the initramfs doesn't include xattrs. Once support for CPIO xattrs is upstreamed, IMA-appraisal could then also be enabled from the very beginning. For now, we rely on the initramfs being measured (and appraised) and enable IMA-appraisal before any files are accessed from real root. Systems with a custom /init today already can enable IMA-appraisal from the very beginning. In terms of IMA namespacing, we shouldn't need to differentiate between IMA-measurement and IMA-audit from IMA-appraisal. All of them should be initialized from the very beginning to capture all measurements in the measurement list, audit the measurements and appraise all files. Requiring IMA namespacing to be joined to another namespace complicates things, like the unnecessary creation of IMA namespaces. Just as there is an "owning" namespace for other namespaces, there should be an "owning" IMA namespace, which is independent of either the mount or user namespace. (I hope I'm using the term "owning" properly here.) Mimi
Re: [PATCH 0/9] KEYS: Blacklisting & UEFI database load
On Sun, 2018-03-11 at 11:20 +0800, joeyli wrote: > On Wed, Mar 07, 2018 at 07:28:37AM -0800, James Bottomley wrote: > > On Wed, 2018-03-07 at 08:18 -0500, Mimi Zohar wrote: > > > On Tue, 2018-03-06 at 15:05 +0100, Jiri Slaby wrote: > > > > what's the status of this please? Distributors (I checked SUSE, > > > > RedHat and Ubuntu) have to carry these patches and every of them > > > > have to forward-port the patches to new kernels. So are you going > > > > to resend the PR to have this merged? > > [...] > > > Just because I trust the platform keys prior to booting the kernel, > > > doesn't mean that I *want* to trust those keys once booted. There > > > are, however, places where we need access to those keys to verify a > > > signature (eg. kexec kernel image). > > > > Which is essentially the reason I always give when these patches come > > back > > > > Josh Boyer's "MODSIGN: Allow the "db" UEFI variable to be suppressed" > patch checks MokIgnoreDB variable to ignore db: > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-uefi&id=7c395b30a33a617c5cc2cdd419300af71277b79a > > I think that we can consider to use MokAllowDB. Which means that kernel > ignores DB by default. Not all systems have a shim layer. This design is really x86 specific. Allowing shim keys, but ignoring DB, does not address those systems. > > > Nayna Jain's "certs: define a trusted platform keyring" patch set > > > introduces a new, separate keyring for these platform keys. > > > > Perhaps, to break the deadlock, we should ask Jiří what the reason is > > the distros want these keys to be trusted. Apart from the Microsoft > > key, it will also give you an OEM key in your trusted keyring. Is it > > something to do with OEM supplied modules? > > > > As I remember that some manufacturers uses certificate in db to > sign their kernel module. We need to discuss with them for switching > to mok. Currently I do not know all use cases for using db. > > There have some benefits for using db: > > - User does not need to deal with shim-mokmanager to enroll mok. >Target machine doesn't need to reboot and user doesn't need to >face to mokmanager UI. The reason for trusting enrolled shim keys is because it requires physical presence. (I kind of remember hearing that this changed. There is some method of accepting enrolled keys that does not require physical presence.) > - The db is a authenticated variable, it's still secure when secure >boot is disabled. >The db is a authenticated variable that it can only be modified >by manufacturer's key. Kernel can trust it when secure boot >is disabled. It's useful for we do not need to taint kernel >for loading a manufacturer's kernel module even secure boot is >disabled. > > - Do not need to worry about the space of NVRAM and the EFI firmware >implementation for writing a boot time variable. > > But I also agree that we should not trust all keys (like Microsoft key) > in db by default. Between requiring a shim layer and relying on physical presence, I'm not convinced this is the best solution. Do we really want to support different methods for different architectures? Mimi
Re: [PATCH] security: Fix IMA Kconfig for dependencies on ARM64
On Thu, 2018-03-15 at 10:29 -0700, James Bottomley wrote: > On Thu, 2018-03-15 at 13:14 -0400, Mimi Zohar wrote: > > On Thu, 2018-03-15 at 10:08 -0700, James Bottomley wrote: > > > > > > On Thu, 2018-03-15 at 12:19 -0400, Mimi Zohar wrote: > > > > > > > > > > > > > If EFI is extending the TPM, will the events be added to the TPM > > > > event log or to the IMA measurement list? > > > > > > I'm not proposing any changes to the tpm_pcr_extend API. At the > > > moment it does an extend without logging, so that's what it will do > > > in the EFI driver case as well. That means logging is still the > > > responsibility of the caller. > > > > Does EFI support extending multiple TPM banks? > > The specs are here: > > https://trustedcomputinggroup.org/tcg-efi-protocol-specification/ > > As I said, I'm not planning to change the tpm_pcr_.. API. At the > moment for a TPM2 we extend all banks in the tpm_pcr_extend() API, so > that's what we'll continue to do ... including extending the sha256 > banks with the sha1 hash, which seems to be our current practice. Thanks, what you're planning on doing is a lot clearer now. Mimi
Re: [PATCH v3 3/5] tpm: migrate tpm2_probe() to use struct tpm_buf
On Fri, 2018-03-16 at 14:21 +0200, Jarkko Sakkinen wrote: > On Mon, Mar 05, 2018 at 05:52:24PM -0500, Mimi Zohar wrote: > > Hi Jarrko, > > > > On Mon, 2018-03-05 at 18:56 +0200, Jarkko Sakkinen wrote: > > > In order to make struct tpm_buf the first class object for constructing > > > TPM > > > commands, migrate tpm2_probe() to use it. > > > > > > Signed-off-by: Jarkko Sakkinen > > > > With this patch, the Pi doesn't find the TPM. I'm seeing the > > following line in dmesg. > > > > [1.087414] tpm_tis_spi: probe of spi0.0 failed with error 256 > > Thank you for reporting this Mimi. Does it have TPM1/TPM2? The pi has a TPM 2.0 attached to the GPIO. James pointed out the change in return codes. + if (be16_to_cpu(out->tag) == TPM2_ST_NO_SESSIONS) chip->flags |= TPM_CHIP_FLAG_TPM2; - - return 0; +out: + tpm_buf_destroy(&buf); + return rc; } EXPORT_SYMBOL_GPL(tpm2_probe); Mimi
Re: [PATCH 0/4] Code improvements in integrity and IMA
Hi Thiago, On Wed, 2018-03-14 at 17:20 -0300, Thiago Jung Bauermann wrote: > Hello, > > These patches come from the "appended signatures support for IMA appraisal" > series. They are code improvements and cleanups and I decided to submit > them separately for a couple of reasons: > > 1. they stand on their own and could be included in v4.17; > > 2. this will simplify the original patch series a bit, by having it contain >only the patches actually necessary to implement the feature. Agreed. The first 3 patches are applied. The last one, we'll see about. Mimi > > These are the changes made to them since v5 of the modsig series: > > - Patch "integrity: Remove unused macro IMA_ACTION_RULE_FLAGS" > - New patch. > > - Patch "ima: Improvements in ima_appraise_measurement()" > - Moved is_ima_sig() to its own patch (not in this series). > > Mimi Zohar (1): > ima: Improvements in ima_appraise_measurement() > > Thiago Jung Bauermann (3): > integrity: Remove unused macro IMA_ACTION_RULE_FLAGS > ima: Simplify ima_eventsig_init() > integrity: Introduce struct evm_xattr > > security/integrity/evm/evm_crypto.c | 4 ++-- > security/integrity/evm/evm_main.c | 10 > security/integrity/ima/ima_appraise.c | 40 > ++- > security/integrity/ima/ima_template_lib.c | 11 +++-- > security/integrity/integrity.h| 6 - > 5 files changed, 39 insertions(+), 32 deletions(-) >
Re: [PATCH 3/4] ima: Improvements in ima_appraise_measurement()
On Wed, 2018-03-14 at 21:03 -0300, Thiago Jung Bauermann wrote:
> Hello Serge,
>
> Thanks for quickly reviewing these patches!
>
> Serge E. Hallyn writes:
>
> > Quoting Thiago Jung Bauermann (bauer...@linux.vnet.ibm.com):
> >> From: Mimi Zohar
> >> @@ -241,16 +241,20 @@ int ima_appraise_measurement(enum ima_hooks func,
> >>}
> >>
> >>status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
> >> - if ((status != INTEGRITY_PASS) &&
> >> - (status != INTEGRITY_PASS_IMMUTABLE) &&
> >> - (status != INTEGRITY_UNKNOWN)) {
> >> - if ((status == INTEGRITY_NOLABEL)
> >> - || (status == INTEGRITY_NOXATTRS))
> >> - cause = "missing-HMAC";
> >> - else if (status == INTEGRITY_FAIL)
> >> - cause = "invalid-HMAC";
> >> + switch (status) {
> >> + case INTEGRITY_PASS:
> >> + case INTEGRITY_PASS_IMMUTABLE:
> >> + case INTEGRITY_UNKNOWN:
> >
> > Wouldn't it be more future-proof to replace this with a 'default', or
> > to at least add a "default: BUG()" to catch new status values?
>
> I agree. I like the "default: BUG()" option.
Agreed. I would put it at the end after INTEGRITY_FAIL.
>
> >> + break;
> >> + case INTEGRITY_NOXATTRS:/* No EVM protected xattrs. */
> >> + case INTEGRITY_NOLABEL: /* No security.evm xattr. */
> >> + cause = "missing-HMAC";
> >> + goto out;
> >> + case INTEGRITY_FAIL:/* Invalid HMAC/signature. */
> >> + cause = "invalid-HMAC";
> >>goto out;
> >>}
> >> +
> >>switch (xattr_value->type) {
> >>case IMA_XATTR_DIGEST_NG:
> >>/* first byte contains algorithm id */
> >> @@ -316,17 +320,20 @@ int ima_appraise_measurement(enum ima_hooks func,
> >>integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> >>op, cause, rc, 0);
> >>} else if (status != INTEGRITY_PASS) {
> >> + /* Fix mode, but don't replace file signatures. */
> >>if ((ima_appraise & IMA_APPRAISE_FIX) &&
> >>(!xattr_value ||
> >> xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> >>if (!ima_fix_xattr(dentry, iint))
> >>status = INTEGRITY_PASS;
> >> - } else if ((inode->i_size == 0) &&
> >> - (iint->flags & IMA_NEW_FILE) &&
> >> - (xattr_value &&
> >> - xattr_value->type == EVM_IMA_XATTR_DIGSIG)) {
> >> + }
> >> +
> >> + /* Permit new files with file signatures, but without data. */
> >> + if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
> >
> > This may be correct, but it's not identical to what you're replacing.
> > Since in either case you're setting status to INTEGRITY_PASS the final
> > result is the same, but with a few extra possible steps. Not sure
> > whether that matters.
>
> Good point. I'll have to defer this one to Mimi though.
The end result is the same, but add some needed comments.
Mimi
>
> >
> >> + xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
> >>status = INTEGRITY_PASS;
> >>}
> >> +
> >>integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> >>op, cause, rc, 0);
> >>} else {
>
>
Re: [PATCH] security: Fix IMA Kconfig for dependencies on ARM64
On Thu, 2018-03-15 at 10:08 -0700, James Bottomley wrote: > On Thu, 2018-03-15 at 12:19 -0400, Mimi Zohar wrote: > > If EFI is extending the TPM, will the events be added to the TPM > > event log or to the IMA measurement list? > > I'm not proposing any changes to the tpm_pcr_extend API. At the moment > it does an extend without logging, so that's what it will do in the EFI > driver case as well. That means logging is still the responsibility of > the caller. Does EFI support extending multiple TPM banks? Mimi
Re: [PATCH] security: Fix IMA Kconfig for dependencies on ARM64
On Wed, 2018-03-14 at 10:25 -0700, James Bottomley wrote: > On Wed, 2018-03-14 at 13:08 -0400, Mimi Zohar wrote: [..] > > Adding additional support for post IMA-initialization for TPM's built > > as kernel modules is clearly not optimal for all of the reasons > > provided to now and will be confusing, but could be supported. This > > delayed loading of the TPM needs to be clearly indicated in both the > > audit log and in IMA's measurement list. > > Why if the measurement chain isn't broken? The way I'm thinking of > implementing it, IMA wouldn't even know. I'm not sure this is good news. > What would happen is that a > NULL tpm chip in tpm_pcr_read/tpm_pcr_extend would trigger the usual > search for the first TPM but if none were found and we'd booted on an > EFI system, we'd just use the EFI driver to do perform the operation. If EFI is extending the TPM, will the events be added to the TPM event log or to the IMA measurement list? Up to now the IMA boot aggregate record includes PCRs from 0 - 7. With these PCRs, the boot aggregate wouldn't change when booting the same kernel. Would you change the boot-aggregate to include these other PCRs? > There's probably a bit of additional subtlety making the kernel and EFI > agree which TPM they're using in a multi-TPM situation. Agreed > The EFI driver isn't full featured: it only does measurement and > logging, but it looks like that's all IMA needs. What happens for non EFI systems, when you can't extend the TPM? Mimi
Re: [PATCH] security: Fix IMA Kconfig for dependencies on ARM64
On Wed, 2018-03-14 at 07:41 -0700, James Bottomley wrote: > On Tue, 2018-03-13 at 12:57 +, Safford, David (GE Global Research, > US) wrote: > > > > > > -Original Message- > > > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com > > > ] > > > Sent: Monday, March 12, 2018 8:07 PM > > > To: Mimi Zohar ; Jiandi An > [...] > > > > > The key question is not whether the component could > > > > > theoretically > > > > > access the files but whether it actually does so. > > > > > > > > As much as you might think you know what is included in the > > > > initramfs, IMA-measurement is your safety net, including > > > > everything accessed in the TCB. > > > > > > The initrd *is* part of the Trusted Computing Base because it's > > > part of the boot custody chain. That's really my point. If I > > > don't know what's in my initrd, I've broken the chain there and IMA > > > can't fix it. > > > > > > James > > > > That's exactly the point - how do you know what's in your initrd? > > The initrd is normally built on the possibly compromised system in > > question. > > The point of deploying security measures is to make sure my system > isn't compromised. I realise the institutional view is "we didn't > build the initrd" and my individual view is well, I built my own kernel > as well, so what's the difference? But the initrd in both models is > still part of the chain. > > > It's not signed as a whole by someone trusted. How can the > > attestation server say a given hash of the initrd as a whole is good? > > I trust myself. I can get the hash at build time. In the same way as > I sign my own kernel at build time for secure boot. > > > If IMA is running from the very start, it can at least measure (and > > eventually appraise) every individual file in the initrd. Given this > > more detailed measurement list, the attestation server can verify all > > the components in the initrd, even when it is assembled on the > > untrusted system. > > > > On many embedded systems, there is no initrd, and IMA has to start > > measuring and appraising immediately, anyway. > > > > Perhaps there is a use case where there is a known set of initrd > > images, and so the bootloader's measurement of the initrd is > > sufficient for verification. I've not run into that situation yet. If > > you want an option for this use case, that's fine, (I'm all for > > choice) but it should not be the default for IMA. > > Actually, we seem to have wandered away from the main concern which was > trying to select built in TPM drivers. IMA selecting the generic TPM drivers has never been an issue. As I mentioned previously, this is simply a convenience. Anyone building a kernel can configure the vendor specific TPM drivers as builtin. In terms of distro's, configuring vendor specific TPMs as builtin is a TPM vandor/distro discussion. On OpenPower, we've requested the Atmel, Infineon, and Nuvoton TPM drivers be builtin by the different distro's. > What about a compromise: we > already get the boot loader to do measurements and PCR extensions using > the BIOS TPM driver, there's no reason why we can't do the same in the > early kernel until a real TPM driver is found. Your proposal requires changes to the existing boot loaders, not all of which are X86 based. Grub, to the best of my knowledge, is not interested in having anything to do with TPM based measurements. Many attempts have been made to upstream trusted boot patches, but none have been accepted. Any support would need to piggyback on the callback hooks introduced for secure boot and then be carried by the distros. As for Linux based boot loaders, the driver needs to be builtin for these measurements. So this wouldn't help you. > That way IMA would have > no dependency on any built in TPM driver ... is that an acceptable > compromise to everyone? Adding additional support for post IMA-initialization for TPM's built as kernel modules is clearly not optimal for all of the reasons provided to now and will be confusing, but could be supported. This delayed loading of the TPM needs to be clearly indicated in both the audit log and in IMA's measurement list. In terms of attestation, if a measurement policy is enabled before the TPM is loaded, any records up to the delayed TPM entry in the IMA measurement list would need to be ignored. In addition, your changes should not in any way change the existing IMA-measurement initialization. Mimi
Re: [RFC PATCH linux-next] evm: evm_hmac can be static
On Wed, 2018-03-14 at 01:42 +0800, kbuild test robot wrote:
> Fixes: c49fc264be98 ("evm: Move evm_hmac and evm_hash from evm_main.c to
> evm_crypto.c")
> Signed-off-by: Fengguang Wu
Thanks!
> ---
> evm_crypto.c |4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/evm/evm_crypto.c
> b/security/integrity/evm/evm_crypto.c
> index fdde9cb..a46fba3 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -37,8 +37,8 @@ static DEFINE_MUTEX(mutex);
>
> static unsigned long evm_set_key_flags;
>
> -char * const evm_hmac = "hmac(sha1)";
> -char * const evm_hash = "sha1";
> +static char * const evm_hmac = "hmac(sha1)";
> +static char * const evm_hash = "sha1";
>
> /**
> * evm_set_key() - set EVM HMAC key from the kernel
Re: [PATCH] security: Fix IMA Kconfig for dependencies on ARM64
On Mon, 2018-03-12 at 15:30 -0700, James Bottomley wrote: > On Mon, 2018-03-12 at 17:53 -0400, Mimi Zohar wrote: [...] > > - This use case, when the TPM is not builtin and unavailable before > > IMA is initialized. > > > > I would classify this use case as an IMA testing/debugging > > environment, when it cannot, for whatever reason, be builtin the > > kernel or initialized before IMA. > > > > From Dave Safford: > > For the TCG chain of trust to have any meaning, all files have to > > be measured and extended into the TPM before they are accessed. > > If > > the TPM driver is loaded after any unmeasured file, the chain is > > broken, and IMA is useless for any use case or any threat model. > > I don't think this is quite the correct characterisation. In principle > the kernel could also touch the files before IMA is loaded. However, > we know from the way the kernel operates that it doesn't. We basically > trust that the kernel measurement tells us this. The same thing can be > made to apply to the initrd. With the builtin "tcb" policy, IMA-measurement is enabled from the very beginning. Afterwards, the system can transition to a custom policy based on finer grain LSM labels, which aren't available on boot. > The key question is not whether the component could theoretically > access the files but whether it actually does so. As much as you might think you know what is included in the initramfs, IMA-measurement is your safety net, including everything accessed in the TCB. Mimi
Re: [PATCH] security: Fix IMA Kconfig for dependencies on ARM64
On Mon, 2018-03-12 at 17:05 -0600, Jason Gunthorpe wrote: > On Mon, Mar 12, 2018 at 06:58:45PM -0400, Mimi Zohar wrote: > > On Mon, 2018-03-12 at 15:59 -0600, Jason Gunthorpe wrote: > > > On Mon, Mar 12, 2018 at 05:53:18PM -0400, Mimi Zohar wrote: > > > > > > > Using Kconfig to force the TPM to be builtin is not required, but > > > > helpful. Users interested in IMA-measurement could configure the TPM > > > > as builtin themselves. Without the TPM builtin, IMA goes into TPM- > > > > bypass mode. > > > > > > This issues, broadly speaking, we have lots of TPM drivers, selecting > > > only some to actually support IMA shows we have some kind of problem > > > here. > > > > True, IMA is not selecting the older TPM vendor specific modules, but > > only the newer TPM_TIS and now TPM_CRB modules. That doesn't imply > > that IMA only supports some TPMs. It means that by default, these > > TPMs are builtin. Anyone building a kernel, can select the vendor > > specific TPM to be builtin. > > That doesn't help distros, which is the main point of the complaint > with this scheme :) Years ago because of faulty TPM drivers, IMA was disabled in one of the main distro's. Deciding which vendor specific TPMs should be builtin, is a discussion between the distro's and TPM vendors. Mimi
Re: [PATCH] security: Fix IMA Kconfig for dependencies on ARM64
On Mon, 2018-03-12 at 15:59 -0600, Jason Gunthorpe wrote: > On Mon, Mar 12, 2018 at 05:53:18PM -0400, Mimi Zohar wrote: > > > Using Kconfig to force the TPM to be builtin is not required, but > > helpful. Users interested in IMA-measurement could configure the TPM > > as builtin themselves. Without the TPM builtin, IMA goes into TPM- > > bypass mode. > > This issues, broadly speaking, we have lots of TPM drivers, selecting > only some to actually support IMA shows we have some kind of problem > here. True, IMA is not selecting the older TPM vendor specific modules, but only the newer TPM_TIS and now TPM_CRB modules. That doesn't imply that IMA only supports some TPMs. It means that by default, these TPMs are builtin. Anyone building a kernel, can select the vendor specific TPM to be builtin. Mimi
Re: [PATCH] security: Fix IMA Kconfig for dependencies on ARM64
On Fri, 2018-03-09 at 09:11 -0800, James Bottomley wrote: > On Thu, 2018-03-08 at 12:42 -0600, Jiandi An wrote: > [...] > > I'm no expert on IMA and its driver. James, will you be kind enough > > to look into overhauling the IMA driver to not measure until after > > initrd phase if that's the consensus on resolving this? > > I'll add it to my todo list. > > Since my TPM 2.0 test environment is a VM with a tpm that has a network > connection to an emulator on my host, it's impossible to set it up so > that it's built in (because you need the network config before you init > the TPM) so I might accelerate if I suddenly need to debug IMA issues > in this configuration. There are a number of different issues being discussed. - When IMA is enabled, unlike some other TPM device drivers, the TPM 2.0 is not forced to be builtin. This is addressed by Jiandi's patch. - Jason's comment questioning having Kconfig force the TPM to be builtin. Using Kconfig to force the TPM to be builtin is not required, but helpful. Users interested in IMA-measurement could configure the TPM as builtin themselves. Without the TPM builtin, IMA goes into TPM- bypass mode. Extending a TPM with IMA measurements, which was not builtin, but loaded at some unspecified point in time, changes the existing meaning of the IMA-measurement list. - This use case, when the TPM is not builtin and unavailable before IMA is initialized. I would classify this use case as an IMA testing/debugging environment, when it cannot, for whatever reason, be builtin the kernel or initialized before IMA. >From Dave Safford: For the TCG chain of trust to have any meaning, all files have to be measured and extended into the TPM before they are accessed. If the TPM driver is loaded after any unmeasured file, the chain is broken, and IMA is useless for any use case or any threat model. While the initramfs may be measured by the bootloader, there are two problems: 1. IMA has no way of knowing if the kernel or initramfs has accessed any unmeasured files before TPM driver loading and IMA initialization. 2. Even if we can somehow guarantee that nothing outside the initramfs has been accessed prior to IMA initialization, it is difficult if not impossible for the attestation server to know what a good initramfs measurement should be, as the initramfs is built on the suspect device in the first place. We can sort of trust the initramfs measurement in the reference manifest, but after that, the attestation server has no way to trust a reported initramfs measurement. Mimi
Re: [PATCH] security: Fix IMA Kconfig for dependencies on ARM64
On Tue, 2018-03-06 at 23:26 -0600, Jiandi An wrote: > TPM_CRB driver is the TPM support for ARM64. If it > is built as module, TPM chip is registered after IMA > init. tpm_pcr_read() in IMA driver would fail and > display the following message even though eventually > there is TPM chip on the system: > > ima: No TPM chip found, activating TPM-bypass! (rc=-19) > > Fix IMA Kconfig to select TPM_CRB so TPM_CRB driver is > built in kernel and initializes before IMA driver. > > Signed-off-by: Jiandi An Thanks, this patch has been applied. Mimi > --- > security/integrity/ima/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig > index 35ef693..6a8f677 100644 > --- a/security/integrity/ima/Kconfig > +++ b/security/integrity/ima/Kconfig > @@ -10,6 +10,7 @@ config IMA > select CRYPTO_HASH_INFO > select TCG_TPM if HAS_IOMEM && !UML > select TCG_TIS if TCG_TPM && X86 > + select TCG_CRB if TCG_TPM && ACPI > select TCG_IBMVTPM if TCG_TPM && PPC_PSERIES > help > The Trusted Computing Group(TCG) runtime Integrity
Re: [PATCH v3] ima: drop vla in ima_audit_measurement()
On Thu, 2018-03-08 at 16:08 -0700, Tycho Andersen wrote:
> In keeping with the directive to get rid of VLAs [1], let's drop the VLA
> from ima_audit_measurement(). We need to adjust the return type of
> ima_audit_measurement, because now this function can fail if an allocation
> fails.
>
> [1]: https://lkml.org/lkml/2018/3/7/621
>
> v2: just use audit_log_format instead of doing a second allocation
> v3: ignore failures in ima_audit_measurement()
>
> Signed-off-by: Tycho Andersen
Thanks, this patch has been applied.
Mimi
> ---
> security/integrity/ima/ima_api.c | 16 ++--
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/security/integrity/ima/ima_api.c
> b/security/integrity/ima/ima_api.c
> index 08fe405338e1..2aab9170ef68 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -308,14 +308,17 @@ void ima_audit_measurement(struct integrity_iint_cache
> *iint,
> const unsigned char *filename)
> {
> struct audit_buffer *ab;
> - char hash[(iint->ima_hash->length * 2) + 1];
> + char *hash;
> const char *algo_name = hash_algo_name[iint->ima_hash->algo];
> - char algo_hash[sizeof(hash) + strlen(algo_name) + 2];
> int i;
>
> if (iint->flags & IMA_AUDITED)
> return;
>
> + hash = kzalloc((iint->ima_hash->length * 2) + 1, GFP_KERNEL);
> + if (!hash)
> + return;
> +
> for (i = 0; i < iint->ima_hash->length; i++)
> hex_byte_pack(hash + (i * 2), iint->ima_hash->digest[i]);
> hash[i * 2] = '\0';
> @@ -323,18 +326,19 @@ void ima_audit_measurement(struct integrity_iint_cache
> *iint,
> ab = audit_log_start(current->audit_context, GFP_KERNEL,
>AUDIT_INTEGRITY_RULE);
> if (!ab)
> - return;
> + goto out;
>
> audit_log_format(ab, "file=");
> audit_log_untrustedstring(ab, filename);
> - audit_log_format(ab, " hash=");
> - snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash);
> - audit_log_untrustedstring(ab, algo_hash);
> + audit_log_format(ab, " hash=\"%s:%s\"", algo_name, hash);
>
> audit_log_task_info(ab, current);
> audit_log_end(ab);
>
> iint->flags |= IMA_AUDITED;
> +out:
> + kfree(hash);
> + return;
> }
>
> /*
Re: [PATCH 1/2] security: evm: Move evm_hmac and evm_hash from evm_main.c to evm_crypto.c
On Tue, 2018-02-27 at 19:16 -0300, Hernán Gonzalez wrote:
> Note: This is compile only tested.
> This variable was not used where it was defined, there was no point in
> declaring it there as extern, thus it got moved and constified saving up 2
> bytes.
>
> Function old new delta
> init_desc273 271 -2
> Total: Before=2112094, After=2112092, chg -0.00%
>
> Signed-off-by: Hernán Gonzalez
Thanks, both patches have been applied.
Mimi
> ---
> security/integrity/evm/evm.h| 2 --
> security/integrity/evm/evm_crypto.c | 3 +++
> security/integrity/evm/evm_main.c | 2 --
> 3 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
> index 0482539..45c4a89 100644
> --- a/security/integrity/evm/evm.h
> +++ b/security/integrity/evm/evm.h
> @@ -31,8 +31,6 @@
> EVM_ALLOW_METADATA_WRITES)
>
> extern int evm_initialized;
> -extern char *evm_hmac;
> -extern char *evm_hash;
>
> #define EVM_ATTR_FSUUID 0x0001
>
> diff --git a/security/integrity/evm/evm_crypto.c
> b/security/integrity/evm/evm_crypto.c
> index 691f3e0..fdde9cb 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -37,6 +37,9 @@ static DEFINE_MUTEX(mutex);
>
> static unsigned long evm_set_key_flags;
>
> +char * const evm_hmac = "hmac(sha1)";
> +char * const evm_hash = "sha1";
> +
> /**
> * evm_set_key() - set EVM HMAC key from the kernel
> * @key: pointer to a buffer with the key data
> diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> index a8d5028..826926d 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -33,8 +33,6 @@ int evm_initialized;
> static char *integrity_status_msg[] = {
> "pass", "pass_immutable", "fail", "no_label", "no_xattrs", "unknown"
> };
> -char *evm_hmac = "hmac(sha1)";
> -char *evm_hash = "sha1";
> int evm_hmac_attrs;
>
> char *evm_config_xattrnames[] = {
Re: [PATCH v2] exec: Set file unwritable before LSM check
On Fri, 2018-03-09 at 11:54 -0800, Kees Cook wrote: > On Fri, Mar 9, 2018 at 11:47 AM, Linus Torvalds > wrote: > > On Fri, Mar 9, 2018 at 11:30 AM, Kees Cook wrote: > >> The LSM check should happen after the file has been confirmed to be > >> unchanging. Without this, we could have a race between the Time of Check > >> (the call to security_kernel_read_file() which could read the file and > >> make access policy decisions) and the Time of Use (starting with > >> kernel_read_file()'s reading of the file contents). In theory, file > >> contents could change between the two. For files opened by userspace, IMA refers to the problem as "Time of Measure, Time of Use" (ToMToU) and emits an audit message. security_kernel_read_file() is being called by the kernel to read the kexec kernel image and initramfs, kernel modules (new syscall), ima_policy, EVM x509 certificate, and firmware. If these files are signed, like they should be, then IMA prevents them from being opened for write. Modifying the file via the filesystem should not be possible. Other sorts of attacks, would probably be possible. If these files aren't signed, then in terms of IMA-measurement the file measured, might not be the file used. The ToMToU audit message is not being generated for these files. > > I'm going to assume I get this for 4.17 from the security tree. > > > > Because I'm guessing there are actually no existing users that care? > > selinux seems to just look at file state, not actually at contents or > > anything that write access denial would care about. > > > > And the only other security module that even registers this is > > loadpin, and again it just seems to check things like "on the right > > filesystem" that aren't actually impacted by write access (in fact, > > the documented reason is to check that it's a read-only filesystem so > > that write access is simply _irrelevant_). > > > > So this issue seems to be mainly a cleanliness thing, not an actual bug. > > That is my assumption too (I left off the Cc: stable as a result). I'm > much less familiar with IMA, though, but it's a caller of > kernel_read_file(), not hooking it, etc. Please add my reviewed-by. Mimi
Re: [PATCH v2 1/3] certs: define a trusted platform keyring
On Fri, 2018-03-09 at 21:08 +0530, Nayna Jain wrote: > The kernel can be supplied in SEEPROM or lockable flash memory in embedded > devices. Some devices may not support secure boot, but the kernel is > trusted because the image is stored in protected memory. That kernel may > need to kexec additional kernels, it may be used as a bootloader, for > example, or it may need to kexec a crashdump kernel. In such cases, it may > want to verify the signature of the next kernel. > > The kernel, however, cannot directly verify platform keys, and an > administrator may therefore not want to trust them for arbitrary usage. > In order to differentiate platform keys from other keys and provide the > necessary separation of trust, the kernel needs an additional keyring to > store platform keys. > > This patch implements a built-in list of certificates that are loaded onto > the trusted platform keyring named ".platform_keys" to facilitate signature > verification during kexec. Because the platform keyring are builtin, it > cannot be updated from userspace. > > This keyring can be enabled by setting CONFIG_PLATFORM_KEYRING. The > platform certificate can be provided using CONFIG_PLATFORM_TRUSTED_KEYS. > > Signed-off-by: Nayna Jain Please add my Reviewed-by: Mimi Zohar on this and 2/3. Mimi > --- > Changelog: > > v2: > > * Include David Howell's feedback: > * Fix the indentation > * Fix the patch description per line length as suggested by Mimi > > certs/Kconfig | 17 ++ > certs/Makefile | 13 +++ > certs/system_certificates.S | 20 + > certs/system_keyring.c | 55 > ++--- > 4 files changed, 97 insertions(+), 8 deletions(-) > > diff --git a/certs/Kconfig b/certs/Kconfig > index 5f7663df6e8e..608a4358a25e 100644 > --- a/certs/Kconfig > +++ b/certs/Kconfig > @@ -83,4 +83,21 @@ config SYSTEM_BLACKLIST_HASH_LIST > wrapper to incorporate the list into the kernel. Each should > be a string of hex digits. > > +config PLATFORM_KEYRING > +bool "Provide keyring for platform trusted keys" > +depends on KEYS > +depends on ASYMMETRIC_KEY_TYPE > +help > + Provide a separate, distinct keyring for platform trusted keys, which > + the kernel automatically populates during initialization from values > + embedded during build, used for verifying the kexec'ed kernel image > + and, possibly, the initramfs signature. > + > +config PLATFORM_TRUSTED_KEYS > + string "Platform/Firmware trusted X.509 certs." > + depends on PLATFORM_KEYRING > + help > + Provide the filename of a PEM-formatted file containing the platform > + trusted X.509 certificates to be loaded in the platform keyring. > + > endmenu > diff --git a/certs/Makefile b/certs/Makefile > index 5d0999b9e21b..680903725031 100644 > --- a/certs/Makefile > +++ b/certs/Makefile > @@ -104,3 +104,16 @@ targets += signing_key.x509 > $(obj)/signing_key.x509: scripts/extract-cert $(X509_DEP) FORCE > $(call > if_changed,extract_certs,$(MODULE_SIG_KEY_SRCPREFIX)$(CONFIG_MODULE_SIG_KEY)) > endif # CONFIG_MODULE_SIG > + > + > +ifeq ($(CONFIG_PLATFORM_KEYRING),y) > + > +$(eval $(call config_filename,PLATFORM_TRUSTED_KEYS)) > + > +# GCC doesn't include .incbin files in -MD generated dependencies (PR#66871) > +$(obj)/system_certificates.o: $(obj)/platform_certificate_list > + > +targets += platform_certificate_list > +$(obj)/platform_certificate_list: scripts/extract-cert > $(PLATFORM_TRUSTED_KEYS_FILENAME) FORCE > + $(call if_changed,extract_certs,$(CONFIG_PLATFORM_TRUSTED_KEYS)) > +endif # CONFIG_PLATFORM_KEYRING > diff --git a/certs/system_certificates.S b/certs/system_certificates.S > index 3918ff7235ed..b0eb448ee617 100644 > --- a/certs/system_certificates.S > +++ b/certs/system_certificates.S > @@ -14,6 +14,15 @@ __cert_list_start: > .incbin "certs/x509_certificate_list" > __cert_list_end: > > +#ifdef CONFIG_PLATFORM_KEYRING > + .align 8 > + .globl VMLINUX_SYMBOL(platform_certificate_list) > +VMLINUX_SYMBOL(platform_certificate_list): > +__platform_cert_list_start: > + .incbin "certs/platform_certificate_list" > +__platform_cert_list_end: > +#endif /* CONFIG_PLATFORM_KEYRING */ > + > #ifdef CONFIG_SYSTEM_EXTRA_CERTIFICATE > .globl VMLINUX_SYMBOL(system_extra_cert) > .size system_extra_cert, CONFIG_SYSTEM_EXTRA_CERTIFICATE_SIZE > @@ -35,3 +44,14 @@ VMLINUX_SYMBOL(system_certificate_list_size): > #else > .long __cert
Re: [PATCH v2 3/3] ima: support platform keyring for kernel appraisal
On Fri, 2018-03-09 at 21:08 +0530, Nayna Jain wrote:
> Distros may sign the kernel images and, possibly, the initramfs with
> platform trusted keys. On secure boot enabled systems or embedded devices,
> these signatures are to be validated using keys on the platform keyring.
>
> This patch enables IMA-appraisal to access the platform keyring, based on a
> new Kconfig option "IMA_USE_PLATFORM_KEYRING".
>
> Signed-off-by: Nayna Jain
Thanks, Nayna!
Signed-off-by: Mimi Zohar
> ---
> Changelog:
>
> v2:
> * Rename integrity_load_keyring() to integrity_find_keyring()
> * Fix the patch description per line length as suggested by Mimi
>
> security/integrity/digsig.c | 15 +++
> security/integrity/ima/Kconfig| 10 ++
> security/integrity/ima/ima_appraise.c | 22 +-
> security/integrity/ima/ima_init.c | 4
> security/integrity/integrity.h| 17 -
> 5 files changed, 62 insertions(+), 6 deletions(-)
>
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 6f9e4ce568cd..cfeb977bced9 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -34,6 +34,8 @@ static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
> ".ima",
> #endif
> "_module",
> + ".platform_keys",
> +
> };
>
> #ifdef CONFIG_INTEGRITY_TRUSTED_KEYRING
> @@ -78,6 +80,19 @@ int integrity_digsig_verify(const unsigned int id, const
> char *sig, int siglen,
> return -EOPNOTSUPP;
> }
>
> +#ifdef CONFIG_IMA_USE_PLATFORM_KEYRING
> +int __init integrity_find_keyring(const unsigned int id)
> +{
> +
> + keyring[id] = find_keyring_by_name(keyring_name[id], 0);
> + if (IS_ERR(keyring[id]))
> + if (PTR_ERR(keyring[id]) != -ENOKEY)
> + return PTR_ERR(keyring[id]);
> + return 0;
> +
> +}
> +#endif
> +
> int __init integrity_init_keyring(const unsigned int id)
> {
> const struct cred *cred = current_cred();
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 35ef69312811..2e89d4f8a364 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -227,3 +227,13 @@ config IMA_APPRAISE_SIGNED_INIT
> default n
> help
> This option requires user-space init to be signed.
> +
> +config IMA_USE_PLATFORM_KEYRING
> + bool "IMA uses keys from Platform Keyring for verification"
> + depends on PLATFORM_KEYRING
> + depends on IMA_APPRAISE
> + depends on INTEGRITY_ASYMMETRIC_KEYS
> + default n
> + help
> + This option enables IMA appraisal to look for the platform
> + trusted keys in .platform_keys keyring.
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index f2803a40ff82..5fec29f40595 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -276,13 +276,25 @@ int ima_appraise_measurement(enum ima_hooks func,
>(const char *)xattr_value, rc,
>iint->ima_hash->digest,
>iint->ima_hash->length);
> - if (rc == -EOPNOTSUPP) {
> - status = INTEGRITY_UNKNOWN;
> - } else if (rc) {
> + if (rc) {
> + if (rc == -EOPNOTSUPP) {
> + status = INTEGRITY_UNKNOWN;
> + break;
> + }
> + if (func == KEXEC_KERNEL_CHECK) {
> + rc = integrity_digsig_verify(
> + INTEGRITY_KEYRING_PLATFORM,
> + (const char *)xattr_value,
> + xattr_len,
> + iint->ima_hash->digest,
> + iint->ima_hash->length);
> + if (!rc) {
> + status = INTEGRITY_PASS;
> + break;
> + }
> + }
> cause = "invalid-signature";
> status = INTEGRITY_FAIL;
> - } else {
> - status = INTEGRITY_PASS;
> }
> break;
> default:
> diff --git a/security/integrity/ima/ima_init.c
> b/sec
Re: [PATCH v2] ima: drop vla in ima_audit_measurement()
On Thu, 2018-03-08 at 15:15 -0700, Tycho Andersen wrote:
> Hi Mimi,
>
> On Thu, Mar 08, 2018 at 05:05:40PM -0500, Mimi Zohar wrote:
> > On Thu, 2018-03-08 at 14:45 -0700, Tycho Andersen wrote:
> > > Hi Mimi,
> > >
> > > On Thu, Mar 08, 2018 at 03:36:14PM -0500, Mimi Zohar wrote:
> > > > On Thu, 2018-03-08 at 13:23 -0700, Tycho Andersen wrote:
> > > >
> > > > > /*
> > > > > diff --git a/security/integrity/ima/ima_main.c
> > > > > b/security/integrity/ima/ima_main.c
> > > > > index 2cfb0c714967..356faae6f09c 100644
> > > > > --- a/security/integrity/ima/ima_main.c
> > > > > +++ b/security/integrity/ima/ima_main.c
> > > > > @@ -288,8 +288,11 @@ static int process_measurement(struct file
> > > > > *file, char *buf, loff_t size,
> > > > > xattr_value, xattr_len,
> > > > > opened);
> > > > > inode_unlock(inode);
> > > > > }
> > > > > - if (action & IMA_AUDIT)
> > > > > - ima_audit_measurement(iint, pathname);
> > > > > + if (action & IMA_AUDIT) {
> > > > > + rc = ima_audit_measurement(iint, pathname);
> > > > > + if (rc < 0)
> > > > > + goto out_locked;
> > > > > + }
> > > > >
> > > > > if ((file->f_flags & O_DIRECT) && (iint->flags &
> > > > > IMA_PERMIT_DIRECTIO))
> > > > > rc = 0;
> > > >
> > > > Only when IMA-appraisal is enforcing file data integrity should
> > > > process_measurement() ever fail. Other errors can be logged/audited.
> > >
> > > Ok, so previously in ima_audit_measurement() when allocation failed,
> > > there was nothing logged. If we just keep this behavior like below,
> > > does that look good?
> >
> > Before the IMA locking change that were just upstreamed, there were
> > problems with measuring/appraising files that were opened with the
> > O_DIRECT flag. Unless the IMA policy specified permit_directio, the
> > measurement/appraisal failed. With the new locking, opening files
> > with the O_DIRECTIO flag shouldn't be a problem. It just needs to be
> > fully tested before removing this code.
> >
> > On failure, the code below tests the ima_audit_measurement() result
> > and skips the IMA_PERMIT_DIRECTIO test. Unless I'm missing something,
> > I don't see the point.
>
> It skips the IMA_PERMIT_DIRECTIO test because it's already going to
> fail: we're in enforce mode and we got an allocation failure and so we
> can't audit this access (note: there is another allocation failure in
> ima_audit_measurement() which is still ignored after this patch, so
> maybe ignoring failures is ok; seems like it's not, though
By the time we get here, we've already verified the file's integrity,
if it is in policy. At this point, we're attempting to add the file
hash to the audit log. If for some reason the audit fails, there's
not much we can do.
> I'm not sure I really understand the rest of your message though. Can
> you suggest what the patch should do here? Should we just ignore all
> failures as before?
I would leave it as it is.
Mimi
Re: [PATCH v2] ima: drop vla in ima_audit_measurement()
On Thu, 2018-03-08 at 14:45 -0700, Tycho Andersen wrote:
> Hi Mimi,
>
> On Thu, Mar 08, 2018 at 03:36:14PM -0500, Mimi Zohar wrote:
> > On Thu, 2018-03-08 at 13:23 -0700, Tycho Andersen wrote:
> >
> > > /*
> > > diff --git a/security/integrity/ima/ima_main.c
> > > b/security/integrity/ima/ima_main.c
> > > index 2cfb0c714967..356faae6f09c 100644
> > > --- a/security/integrity/ima/ima_main.c
> > > +++ b/security/integrity/ima/ima_main.c
> > > @@ -288,8 +288,11 @@ static int process_measurement(struct file *file,
> > > char *buf, loff_t size,
> > > xattr_value, xattr_len, opened);
> > > inode_unlock(inode);
> > > }
> > > - if (action & IMA_AUDIT)
> > > - ima_audit_measurement(iint, pathname);
> > > + if (action & IMA_AUDIT) {
> > > + rc = ima_audit_measurement(iint, pathname);
> > > + if (rc < 0)
> > > + goto out_locked;
> > > + }
> > >
> > > if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO))
> > > rc = 0;
> >
> > Only when IMA-appraisal is enforcing file data integrity should
> > process_measurement() ever fail. Other errors can be logged/audited.
>
> Ok, so previously in ima_audit_measurement() when allocation failed,
> there was nothing logged. If we just keep this behavior like below,
> does that look good?
Before the IMA locking change that were just upstreamed, there were
problems with measuring/appraising files that were opened with the
O_DIRECT flag. Unless the IMA policy specified permit_directio, the
measurement/appraisal failed. With the new locking, opening files
with the O_DIRECTIO flag shouldn't be a problem. It just needs to be
fully tested before removing this code.
On failure, the code below tests the ima_audit_measurement() result
and skips the IMA_PERMIT_DIRECTIO test. Unless I'm missing something,
I don't see the point.
Mimi
> Thanks!
>
> Tycho
>
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 356faae6f09c..4e699bc7adc5 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -289,9 +289,13 @@ static int process_measurement(struct file *file, char
> *buf, loff_t size,
> inode_unlock(inode);
> }
> if (action & IMA_AUDIT) {
> - rc = ima_audit_measurement(iint, pathname);
> - if (rc < 0)
> + int ret;
> +
> + ret = ima_audit_measurement(iint, pathname);
> + if (ret < 0 && ima_appraise & IMA_APPRAISE_ENFORCE) {
> + rc = ret;
> goto out_locked;
> + }
> }
>
> if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO))
>
Re: [PATCH v2] ima: drop vla in ima_audit_measurement()
On Thu, 2018-03-08 at 13:23 -0700, Tycho Andersen wrote:
> /*
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 2cfb0c714967..356faae6f09c 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -288,8 +288,11 @@ static int process_measurement(struct file *file, char
> *buf, loff_t size,
> xattr_value, xattr_len, opened);
> inode_unlock(inode);
> }
> - if (action & IMA_AUDIT)
> - ima_audit_measurement(iint, pathname);
> + if (action & IMA_AUDIT) {
> + rc = ima_audit_measurement(iint, pathname);
> + if (rc < 0)
> + goto out_locked;
> + }
>
> if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO))
> rc = 0;
Only when IMA-appraisal is enforcing file data integrity should
process_measurement() ever fail. Other errors can be logged/audited.
Mimi
Re: [PATCH] security: Fix IMA Kconfig for dependencies on ARM64
On Thu, 2018-03-08 at 12:42 -0600, Jiandi An wrote: > So from the discussion, I hear James suggests to overhaul the current > IMA driver to not do measurement (calling tpm_pcr_read(), etc) until > after initrd phase so TPM drivers can be built as modules. > > I hear Mimi insists TPM drivers should be built-in when IMA driver is > enabled and set to Y in Kconfig. > > Do we have a consensus on which way we should go? > > I'm no expert on IMA and its driver. James, will you be kind enough > to look into overhauling the IMA driver to not measure until after > initrd phase if that's the consensus on resolving this? IMA selecting the TPM forces the TPM to be builtin. There's nothing keeping you from directly configuring the TPM driver as builtin. For remote attestation to validate the IMA measurement list against the PCRs, the existing "ima_tcb" and "ima_policy=tcb" builtin policies require the TPM to be builtin. Not building the TPM into the kernel will also affect EVM. I don't have a problem accepting your patch now; and if/when there is a real use case for building the TPM driver as a kernel module for use with IMA-measurement, accepting those changes then. Mimi
Re: [PATCH] ima: drop vla in ima_audit_measurement()
On Thu, 2018-03-08 at 12:47 -0700, Tycho Andersen wrote: > On Thu, Mar 08, 2018 at 02:20:17PM -0500, Mimi Zohar wrote: > > On Thu, 2018-03-08 at 12:04 -0700, Tycho Andersen wrote: > > > On Thu, Mar 08, 2018 at 01:50:30PM -0500, Mimi Zohar wrote: > > > > On Thu, 2018-03-08 at 11:37 -0700, Tycho Andersen wrote: > > > > > On Thu, Mar 08, 2018 at 07:47:37PM +0200, Andy Shevchenko wrote: > > > > > > On Thu, Mar 8, 2018 at 7:14 PM, Tycho Andersen > > > > > > wrote: > > > > > > > In keeping with the directive to get rid of VLAs [1], let's drop > > > > > > > the VLA > > > > > > > from ima_audit_measurement(). We need to adjust the return type of > > > > > > > ima_audit_measurement, because now this function can fail if an > > > > > > > allocation > > > > > > > fails. > > > > > > > > > > > > > > > > > > > > > > > > > + algo_hash_len = hash_len + strlen(algo_name) + 2; > > > > > > > + algo_hash = kzalloc(algo_hash_len, GFP_KERNEL); > > > > > > > > > > > > > - snprintf(algo_hash, sizeof(algo_hash), "%s:%s", > > > > > > > algo_name, hash); > > > > > > > + snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, > > > > > > > hash); > > > > > > > > > > > > kasprintf() ? > > > > > > > > > > Sure, in fact I think we could just do: > > > > > > > > > > - snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash); > > > > > - audit_log_untrustedstring(ab, algo_hash); > > > > > + audit_log_untrustedstring(ab, algo_name); > > > > > + audit_log_format(ab, ":"); > > > > > + audit_log_untrustedstring(ab, hash); > > > > > > > > > > and get rid of the allocation entirely. I'll test and make sure it > > > > > works and then re-send. > > > > > > > > The hash algorithm name is an enumeration that comes from the kernel. > > > > It's defined in crypto/hash_info.c: hash_algo_name. Why do we need > > > > to use audit_log_untrustedstring()? > > > > > > Yes, I suppose we don't need it for the hash either, since we're > > > generating that and we know it's just hex digits and not any audit > > > control characters or "s or anything. > > > > > > It looks like we could get rid of the other allocation too by just > > > using audit_log_n_hex, but that uses hex_byte_pack_upper, vs. the > > > hex_byte_pack that's currently in use in this function. Is that too > > > much of a breakage? > > > > Based on the discussion with Richard Briggs, we need to differentiate > > between the ima_audit_measurement() and the ima_parse_rule() usage of > > AUDIT_INTEGRITY_RULE. The ima_parse_rule() will continue to use > > AUDIT_INTEGRITY_RULE. ima_audit_measurement() will need to define and > > use a new number. Auidt name suggestions would be appreciated. > > > > When we make that sort of change, any other changes are insignificant. > > How different are the two formats? > > It's just uppercase and lowercase in the hash value, so: > > Mar 8 16:56:46 ima kernel: [ 104.922927] audit: type=1805 > audit(1520528206.082:53): file="/bin/cat" > hash="sha1:79e52322102f073684e2dd0ab7653c7c6fcc49b4" ppid=2049 pid=2123 > auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 > ses=1 comm="bash" exe="/bin/bash" > > vs. > > Mar 8 19:45:12 ima kernel: [ 207.124383] audit: type=1805 > audit(1520538312.740:239): file="/root/.viminfo" > hash="sha1:3322BE0C00190AB0D20C47574575842EC3020BF5" ppid=2045 pid=2195 > auid=1000 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 > ses=1 comm="vi" exe="/usr/bin/vim.basic" > > I'm happy to do either way. If you're willing to wait until the container-id issue is resolved/upstreamed, then either way is fine. If you want the change to go in sooner, then keep it as it currently is. Mimi
Re: [PATCH] ima: drop vla in ima_audit_measurement()
On Thu, 2018-03-08 at 12:04 -0700, Tycho Andersen wrote: > On Thu, Mar 08, 2018 at 01:50:30PM -0500, Mimi Zohar wrote: > > On Thu, 2018-03-08 at 11:37 -0700, Tycho Andersen wrote: > > > On Thu, Mar 08, 2018 at 07:47:37PM +0200, Andy Shevchenko wrote: > > > > On Thu, Mar 8, 2018 at 7:14 PM, Tycho Andersen wrote: > > > > > In keeping with the directive to get rid of VLAs [1], let's drop the > > > > > VLA > > > > > from ima_audit_measurement(). We need to adjust the return type of > > > > > ima_audit_measurement, because now this function can fail if an > > > > > allocation > > > > > fails. > > > > > > > > > > > > > > > > > + algo_hash_len = hash_len + strlen(algo_name) + 2; > > > > > + algo_hash = kzalloc(algo_hash_len, GFP_KERNEL); > > > > > > > > > - snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, > > > > > hash); > > > > > + snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash); > > > > > > > > kasprintf() ? > > > > > > Sure, in fact I think we could just do: > > > > > > - snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash); > > > - audit_log_untrustedstring(ab, algo_hash); > > > + audit_log_untrustedstring(ab, algo_name); > > > + audit_log_format(ab, ":"); > > > + audit_log_untrustedstring(ab, hash); > > > > > > and get rid of the allocation entirely. I'll test and make sure it > > > works and then re-send. > > > > The hash algorithm name is an enumeration that comes from the kernel. > > It's defined in crypto/hash_info.c: hash_algo_name. Why do we need > > to use audit_log_untrustedstring()? > > Yes, I suppose we don't need it for the hash either, since we're > generating that and we know it's just hex digits and not any audit > control characters or "s or anything. > > It looks like we could get rid of the other allocation too by just > using audit_log_n_hex, but that uses hex_byte_pack_upper, vs. the > hex_byte_pack that's currently in use in this function. Is that too > much of a breakage? Based on the discussion with Richard Briggs, we need to differentiate between the ima_audit_measurement() and the ima_parse_rule() usage of AUDIT_INTEGRITY_RULE. The ima_parse_rule() will continue to use AUDIT_INTEGRITY_RULE. ima_audit_measurement() will need to define and use a new number. Auidt name suggestions would be appreciated. When we make that sort of change, any other changes are insignificant. How different are the two formats? Mimi
Re: [PATCH] ima: drop vla in ima_audit_measurement()
On Thu, 2018-03-08 at 11:37 -0700, Tycho Andersen wrote: > On Thu, Mar 08, 2018 at 07:47:37PM +0200, Andy Shevchenko wrote: > > On Thu, Mar 8, 2018 at 7:14 PM, Tycho Andersen wrote: > > > In keeping with the directive to get rid of VLAs [1], let's drop the VLA > > > from ima_audit_measurement(). We need to adjust the return type of > > > ima_audit_measurement, because now this function can fail if an allocation > > > fails. > > > > > > > > > + algo_hash_len = hash_len + strlen(algo_name) + 2; > > > + algo_hash = kzalloc(algo_hash_len, GFP_KERNEL); > > > > > - snprintf(algo_hash, sizeof(algo_hash), "%s:%s", algo_name, hash); > > > + snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash); > > > > kasprintf() ? > > Sure, in fact I think we could just do: > > - snprintf(algo_hash, algo_hash_len, "%s:%s", algo_name, hash); > - audit_log_untrustedstring(ab, algo_hash); > + audit_log_untrustedstring(ab, algo_name); > + audit_log_format(ab, ":"); > + audit_log_untrustedstring(ab, hash); > > and get rid of the allocation entirely. I'll test and make sure it > works and then re-send. The hash algorithm name is an enumeration that comes from the kernel. It's defined in crypto/hash_info.c: hash_algo_name. Why do we need to use audit_log_untrustedstring()? Mimi
Re: [PATCH] audit: add containerid support for IMA-audit
On Thu, 2018-03-08 at 06:21 -0500, Richard Guy Briggs wrote: > On 2018-03-05 09:24, Mimi Zohar wrote: > > On Mon, 2018-03-05 at 08:50 -0500, Richard Guy Briggs wrote: > > > On 2018-03-05 08:43, Mimi Zohar wrote: > > > > Hi Richard, > > > > > > > > This patch has been compiled, but not runtime tested. > > > > > > Ok, great, thank you. I assume you are offering this patch to be > > > included in this patchset? > > > > Yes, thank you. > > > > > I'll have a look to see where it fits in the > > > IMA record. It might be better if it were an AUDIT_CONTAINER_INFO > > > auxiliary record, but I'll have a look at the circumstances of the > > > event. > > I had a look at the context of this record to see if adding the contid > field to it made sense. I think the only records for which the contid > field makes sense are the two newly proposed records, AUDIT_CONTAINER > which introduces the container ID and the and AUDIT_CONTAINER_INFO which > documents the presence of the container ID in a process event (or > process-less network event). All others should use the auxiliary record > AUDIT_CONTAINER_INFO rather than include the contid field directly > itself. There are several reasons for this including record length, the > ability to filter unwanted records, the difficulty of changing the order > of or removing fields in the future. > > Syscalls get this information automatically if the container ID is set > for a task via the AUDIT_CONTAINER_INFO auxiliary record. Generally a > syscall event is one that uses the task's audit_context while a > standalone event uses NULL or builds a local audit_context that is > discarded immediately after the local use. > > Looking at the two cases of AUDIT_INTEGRITY_RULE record generation, it > appears that they should be split into two distinct audit record types. > > The record created in ima_audit_measurement() is a syscall record that > could possibly stand on its own since the subject attributes are > present. If it remains a syscall auxiliary record it will automatically > have the AUDIT_CONTAINER_INFO record accompany it anyways. If it is > decided to detach it (which would save cpu/netlink/disk bandwidth but is > not recommended due to not wanting to throw away any other syscall > information or other involved records (PATH, CWD, etc...) then a local > audit_context would be created for the AUDIT_INTEGRITY_RULE and > AUDIT_CONTAINERID_INFO records only and immediately discarded. > > The record created in ima_parse_rule() is not currently a syscall record > since it is passed an audit_context of NULL and it has a very different > format that does not include any subject attributes (except subj_*=). > At first glance it appears this one should be a syscall accompanied > auxiliary record. Either way it should have an AUDIT_CONTAINER_INFO > auxiliary record either by being converted to a syscall auxiliary record > by using current->audit_context rather than NULL when calling > audit_log_start(), or creating a local audit_context and calling > audit_log_container_info() then releasing the local context. This > version of the record has additional concerns covered here: > https://github.com/linux-audit/audit-kernel/issues/52 > > Can you briefly describe the circumstances under which these two > different identically-numbered records are produced as a first step > towards splitting them into two distict records? Agreed, the two uses should really be separated. ima_parse_rule() generates audit messages, when the IMA policy is initially loaded, replaced, or extended, the policy rules are included in the audit log. When IMA is namespaced, there will be a host policy and namespace policies. We'll need to be able differentiate between the host policy rules and IMA namespaced policy rules, and between IMA namespaced policy rules. The audit messages produced by ima_audit_measurement() were originally upstreamed for forensics, and as seen by the FireEye blog are now used to augment existing security analytics. These records are probably being used independently of any other audit records. A single record is generated per file, per system. With IMA namespacing, these records need to be generated once per file, per namespace as well. In order to differentiate the records between the host and namespace, and between namespaces, these records should include the container id. To disambiguate between these audit messages and the policy rule messages, we could rename these audit messages to AUDIT_INTEGRITY_IMA. > The four AUDIT_INTEGRITY _METADATA, _PCR, _DATA and _STATUS records > appear to be already properly covered for AUDIT_CONTAINER_INFO records > by being syscall auxiliary records. The AUDIT_INTEGRITY_HASH record > appears to be unused. Ok Mimi
Re: [PATCH] security: Fix IMA Kconfig for dependencies on ARM64
On Wed, 2018-03-07 at 11:41 -0800, James Bottomley wrote: > On Wed, 2018-03-07 at 14:21 -0500, Mimi Zohar wrote: > > On Wed, 2018-03-07 at 11:08 -0800, James Bottomley wrote: > > > > > > On Wed, 2018-03-07 at 13:55 -0500, Mimi Zohar wrote: > > > > > > > > On Wed, 2018-03-07 at 11:51 -0700, Jason Gunthorpe wrote: > > > > > > > > > > > > > > > On Tue, Mar 06, 2018 at 11:26:26PM -0600, Jiandi An wrote: > > > > > > > > > > > > > > > > > > TPM_CRB driver is the TPM support for ARM64. If it > > > > > > is built as module, TPM chip is registered after IMA > > > > > > init. tpm_pcr_read() in IMA driver would fail and > > > > > > display the following message even though eventually > > > > > > there is TPM chip on the system: > > > > > > > > > > > > ima: No TPM chip found, activating TPM-bypass! (rc=-19) > > > > > > > > > > > > Fix IMA Kconfig to select TPM_CRB so TPM_CRB driver is > > > > > > built in kernel and initializes before IMA driver. > > > > > > > > > > > > Signed-off-by: Jiandi An > > > > > > security/integrity/ima/Kconfig | 1 + > > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > > > diff --git a/security/integrity/ima/Kconfig > > > > > > b/security/integrity/ima/Kconfig > > > > > > index 35ef693..6a8f677 100644 > > > > > > +++ b/security/integrity/ima/Kconfig > > > > > > @@ -10,6 +10,7 @@ config IMA > > > > > > select CRYPTO_HASH_INFO > > > > > > select TCG_TPM if HAS_IOMEM && !UML > > > > > > select TCG_TIS if TCG_TPM && X86 > > > > > > Well, this explains why IMA doesn't work on one of my X86 systems: > > > it's got a non i2c infineon TPM. > > > > > > > > > > > > > > > > > > > > > > > > + select TCG_CRB if TCG_TPM && ACPI > > > > > > select TCG_IBMVTPM if TCG_TPM && PPC_PSERIES > > > > > > help > > > > > > The Trusted Computing Group(TCG) runtime Integrity > > > > > > > > > > This seems really weird, why are any specific TPM drivers > > > > > linked to IMA config, we have lots of drivers.. > > > > > > > > > > I don't think I've ever seen this pattern in Kconfig before? > > > > > > > > As you've seen by the current discussions, the TPM driver needs > > > > to be initialized prior to IMA. Otherwise IMA goes into TPM- > > > > bypass mode. That implies that the TPM must be builtin to the > > > > kernel, and not as a kernel module. > > > > > > Actually, that's not necessarily true: If we don't begin appraisal > > > until after the initrd phase, then the initrd can load TPM modules > > > before IMA starts. > > > > > > This would involve a bit of code rejigging to not require a TPM > > > until IMA wants to write its first measurement, but it looks doable > > > and would get us out of having to second guess TPM selections. > > > > The question is about measurement, not appraisal. Although the > > initramfs might be measured, the initramfs can access files on the > > real root filesystem. Those files need to be measured, before they > > are used/accessed. > > Isn't it a question of threat model? Because the initrd is measured, > you know it's the one you specified and you should know its security > properties, so measurement doesn't really need to begin until the root > pivots. Perhaps in the case where the initramfs is signed and the signature is verified, I would agree that I know the security properties of the initramfs. That still doesn't negate the fact that the initramfs could access files on real root, without first measuring them. > At that point you pick up the boot aggregate so the log now is > tied to the initrd measurement. Conversely, I can't really see a > threat model where you could trick a correctly measured initrd into > subverting IMA, especially because listening network daemons aren't > usually active at this stage. Linux based boot loaders can be configured to download remote kernel images and initramfs files - network boot. > I'm not saying there isn't a use case for wanting your TPM built in, > I'm just saying I don't think it needs to be required for everyone who > uses IMA. If the TPM module is not builtin, there are no guarantees when it was loaded. There could be a disconnect between the IMA measurement list and the TPM PCRs. If someone has a special use case, then I agree with you, that we could theoretically support it, but I don't think we want to confuse distros or anyone else. The TPM should be builtin, so that IMA measurements can begin before accessing real root. Mimi
Re: [PATCH] security: Fix IMA Kconfig for dependencies on ARM64
On Wed, 2018-03-07 at 11:08 -0800, James Bottomley wrote: > On Wed, 2018-03-07 at 13:55 -0500, Mimi Zohar wrote: > > On Wed, 2018-03-07 at 11:51 -0700, Jason Gunthorpe wrote: > > > > > > On Tue, Mar 06, 2018 at 11:26:26PM -0600, Jiandi An wrote: > > > > > > > > TPM_CRB driver is the TPM support for ARM64. If it > > > > is built as module, TPM chip is registered after IMA > > > > init. tpm_pcr_read() in IMA driver would fail and > > > > display the following message even though eventually > > > > there is TPM chip on the system: > > > > > > > > ima: No TPM chip found, activating TPM-bypass! (rc=-19) > > > > > > > > Fix IMA Kconfig to select TPM_CRB so TPM_CRB driver is > > > > built in kernel and initializes before IMA driver. > > > > > > > > Signed-off-by: Jiandi An > > > > security/integrity/ima/Kconfig | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/security/integrity/ima/Kconfig > > > > b/security/integrity/ima/Kconfig > > > > index 35ef693..6a8f677 100644 > > > > +++ b/security/integrity/ima/Kconfig > > > > @@ -10,6 +10,7 @@ config IMA > > > > select CRYPTO_HASH_INFO > > > > select TCG_TPM if HAS_IOMEM && !UML > > > > select TCG_TIS if TCG_TPM && X86 > > Well, this explains why IMA doesn't work on one of my X86 systems: it's > got a non i2c infineon TPM. > > > > > + select TCG_CRB if TCG_TPM && ACPI > > > > select TCG_IBMVTPM if TCG_TPM && PPC_PSERIES > > > > help > > > > The Trusted Computing Group(TCG) runtime Integrity > > > > > > This seems really weird, why are any specific TPM drivers linked to > > > IMA config, we have lots of drivers.. > > > > > > I don't think I've ever seen this pattern in Kconfig before? > > > > As you've seen by the current discussions, the TPM driver needs to be > > initialized prior to IMA. Otherwise IMA goes into TPM-bypass mode. > > That implies that the TPM must be builtin to the kernel, and not as > > a kernel module. > > Actually, that's not necessarily true: If we don't begin appraisal > until after the initrd phase, then the initrd can load TPM modules > before IMA starts. > > This would involve a bit of code rejigging to not require a TPM until > IMA wants to write its first measurement, but it looks doable and would > get us out of having to second guess TPM selections. The question is about measurement, not appraisal. Although the initramfs might be measured, the initramfs can access files on the real root filesystem. Those files need to be measured, before they are used/accessed. Mimi
