LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/3] kexec: limit kexec_load syscall 
@ 2018-05-11  1:36 Mimi Zohar
  2018-05-11  1:36 ` [PATCH 1/3] ima: based on the "secure_boot" policy limit syscalls Mimi Zohar
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Mimi Zohar @ 2018-05-11  1:36 UTC (permalink / raw)
  To: linux-integrity
  Cc: Eric Biederman, David Howells, Mimi Zohar, linux-security-module,
	kexec, linux-kernel

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/3] ima: based on the "secure_boot" policy limit syscalls
  2018-05-11  1:36 [PATCH 0/3] kexec: limit kexec_load syscall Mimi Zohar
@ 2018-05-11  1:36 ` Mimi Zohar
  2018-05-11  1:36 ` [PATCH 2/3] kexec: call LSM hook for kexec_load syscall Mimi Zohar
  2018-05-11  1:36 ` [PATCH 3/3] ima: based on policy require signed kexec kernel images Mimi Zohar
  2 siblings, 0 replies; 12+ messages in thread
From: Mimi Zohar @ 2018-05-11  1:36 UTC (permalink / raw)
  To: linux-integrity
  Cc: Eric Biederman, David Howells, Mimi Zohar, linux-security-module,
	kexec, linux-kernel

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 <zohar@linux.vnet.ibm.com>
---
 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
  2018-05-11  1:36 [PATCH 0/3] kexec: limit kexec_load syscall Mimi Zohar
  2018-05-11  1:36 ` [PATCH 1/3] ima: based on the "secure_boot" policy limit syscalls Mimi Zohar
@ 2018-05-11  1:36 ` Mimi Zohar
  2018-05-11  1:36 ` [PATCH 3/3] ima: based on policy require signed kexec kernel images Mimi Zohar
  2 siblings, 0 replies; 12+ messages in thread
From: Mimi Zohar @ 2018-05-11  1:36 UTC (permalink / raw)
  To: linux-integrity
  Cc: Eric Biederman, David Howells, Mimi Zohar, linux-security-module,
	kexec, linux-kernel, Kees Cook, Matthew Garrett, Casey Schaufler

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 <zohar@linux.vnet.ibm.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Matthew Garrett <mjg59@google.com>
Cc: Casey Schaufler <casey@schaufler-ca.com>

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 <linux/capability.h>
 #include <linux/mm.h>
 #include <linux/file.h>
+#include <linux/security.h>
 #include <linux/kexec.h>
 #include <linux/mutex.h>
 #include <linux/list.h>
@@ -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..0f3390000156 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 3/3] ima: based on policy require signed kexec kernel images
  2018-05-11  1:36 [PATCH 0/3] kexec: limit kexec_load syscall Mimi Zohar
  2018-05-11  1:36 ` [PATCH 1/3] ima: based on the "secure_boot" policy limit syscalls Mimi Zohar
  2018-05-11  1:36 ` [PATCH 2/3] kexec: call LSM hook for kexec_load syscall Mimi Zohar
@ 2018-05-11  1:36 ` Mimi Zohar
  2 siblings, 0 replies; 12+ messages in thread
From: Mimi Zohar @ 2018-05-11  1:36 UTC (permalink / raw)
  To: linux-integrity
  Cc: Eric Biederman, David Howells, Mimi Zohar, linux-security-module,
	kexec, linux-kernel, Kees Cook, Matthew Garrett

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 <zohar@linux.vnet.ibm.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Matthew Garrett <mjg59@google.com>
---
 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_POLICY	0x20
+#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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
  2018-05-03 21:06             ` Mimi Zohar
@ 2018-05-03 21:36               ` Eric W. Biederman
  0 siblings, 0 replies; 12+ messages in thread
From: Eric W. Biederman @ 2018-05-03 21:36 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Casey Schaufler, David Howells, Matthew Garrett, linux-integrity,
	linux-security-module, kexec, linux-kernel

Mimi Zohar <zohar@linux.vnet.ibm.com> writes:

> On Thu, 2018-05-03 at 11:42 -0500, Eric W. Biederman wrote:
>> Casey Schaufler <casey@schaufler-ca.com> writes:
>> 
>> > On 5/3/2018 8:51 AM, Eric W. Biederman wrote:
>> >> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
>> >>
>> >>> On Wed, 2018-05-02 at 09:45 -0500, Eric W. Biederman wrote:
>> >>>> Mimi Zohar <zohar@linux.vnet.ibm.com> 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().

I really don't see why you want to use the same hook.

I just read through the code of all three users.  None of them.
Especially IMA shares any significant code between the !file case and
the file case.

Eric


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
  2018-05-03 16:42           ` Eric W. Biederman
@ 2018-05-03 21:06             ` Mimi Zohar
  2018-05-03 21:36               ` Eric W. Biederman
  0 siblings, 1 reply; 12+ messages in thread
From: Mimi Zohar @ 2018-05-03 21:06 UTC (permalink / raw)
  To: Eric W. Biederman, Casey Schaufler
  Cc: David Howells, Matthew Garrett, linux-integrity,
	linux-security-module, kexec, linux-kernel

On Thu, 2018-05-03 at 11:42 -0500, Eric W. Biederman wrote:
> Casey Schaufler <casey@schaufler-ca.com> writes:
> 
> > On 5/3/2018 8:51 AM, Eric W. Biederman wrote:
> >> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> >>
> >>> On Wed, 2018-05-02 at 09:45 -0500, Eric W. Biederman wrote:
> >>>> Mimi Zohar <zohar@linux.vnet.ibm.com> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
  2018-05-03 16:05         ` Casey Schaufler
@ 2018-05-03 16:42           ` Eric W. Biederman
  2018-05-03 21:06             ` Mimi Zohar
  0 siblings, 1 reply; 12+ messages in thread
From: Eric W. Biederman @ 2018-05-03 16:42 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Mimi Zohar, David Howells, Matthew Garrett, linux-integrity,
	linux-security-module, kexec, linux-kernel

Casey Schaufler <casey@schaufler-ca.com> writes:

> On 5/3/2018 8:51 AM, Eric W. Biederman wrote:
>> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
>>
>>> On Wed, 2018-05-02 at 09:45 -0500, Eric W. Biederman wrote:
>>>> Mimi Zohar <zohar@linux.vnet.ibm.com> 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.

Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
  2018-05-03 15:51       ` Eric W. Biederman
@ 2018-05-03 16:05         ` Casey Schaufler
  2018-05-03 16:42           ` Eric W. Biederman
  0 siblings, 1 reply; 12+ messages in thread
From: Casey Schaufler @ 2018-05-03 16:05 UTC (permalink / raw)
  To: Eric W. Biederman, Mimi Zohar
  Cc: David Howells, Matthew Garrett, linux-integrity,
	linux-security-module, kexec, linux-kernel

On 5/3/2018 8:51 AM, Eric W. Biederman wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
>
>> On Wed, 2018-05-02 at 09:45 -0500, Eric W. Biederman wrote:
>>> Mimi Zohar <zohar@linux.vnet.ibm.com> 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. 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
  2018-05-02 15:45     ` Mimi Zohar
@ 2018-05-03 15:51       ` Eric W. Biederman
  2018-05-03 16:05         ` Casey Schaufler
  0 siblings, 1 reply; 12+ messages in thread
From: Eric W. Biederman @ 2018-05-03 15:51 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: David Howells, Matthew Garrett, linux-integrity,
	linux-security-module, kexec, linux-kernel

Mimi Zohar <zohar@linux.vnet.ibm.com> writes:

> On Wed, 2018-05-02 at 09:45 -0500, Eric W. Biederman wrote:
>> Mimi Zohar <zohar@linux.vnet.ibm.com> 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.


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

I was blind there for a moment.  Yes this is all about the ima xattrs
allowing a file to be loaded.

Eric

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
  2018-05-02 14:45   ` Eric W. Biederman
@ 2018-05-02 15:45     ` Mimi Zohar
  2018-05-03 15:51       ` Eric W. Biederman
  0 siblings, 1 reply; 12+ messages in thread
From: Mimi Zohar @ 2018-05-02 15:45 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: David Howells, Matthew Garrett, linux-integrity,
	linux-security-module, kexec, linux-kernel

On Wed, 2018-05-02 at 09:45 -0500, Eric W. Biederman wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> 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 <zohar@linux.vnet.ibm.com>
> > ---
> >  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 <linux/capability.h>
> >  #include <linux/mm.h>
> >  #include <linux/file.h>
> > +#include <linux/security.h>
> >  #include <linux/kexec.h>
> >  #include <linux/mutex.h>
> >  #include <linux/list.h>
> > @@ -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.
> >  	 */
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
  2018-04-12 22:41 ` [PATCH 2/3] kexec: call LSM hook for " Mimi Zohar
@ 2018-05-02 14:45   ` Eric W. Biederman
  2018-05-02 15:45     ` Mimi Zohar
  0 siblings, 1 reply; 12+ messages in thread
From: Eric W. Biederman @ 2018-05-02 14:45 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: David Howells, Matthew Garrett, linux-integrity,
	linux-security-module, kexec, linux-kernel

Mimi Zohar <zohar@linux.vnet.ibm.com> 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?

Eric



> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> ---
>  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 <linux/capability.h>
>  #include <linux/mm.h>
>  #include <linux/file.h>
> +#include <linux/security.h>
>  #include <linux/kexec.h>
>  #include <linux/mutex.h>
>  #include <linux/list.h>
> @@ -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.
>  	 */

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
  2018-04-12 22:41 [PATCH 0/3] kexec: limit kexec_load syscall Mimi Zohar
@ 2018-04-12 22:41 ` Mimi Zohar
  2018-05-02 14:45   ` Eric W. Biederman
  0 siblings, 1 reply; 12+ messages in thread
From: Mimi Zohar @ 2018-04-12 22:41 UTC (permalink / raw)
  To: David Howells
  Cc: Matthew Garrett, Mimi Zohar, linux-integrity,
	linux-security-module, Eric Biederman, kexec, linux-kernel

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 <zohar@linux.vnet.ibm.com>
---
 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 <linux/capability.h>
 #include <linux/mm.h>
 #include <linux/file.h>
+#include <linux/security.h>
 #include <linux/kexec.h>
 #include <linux/mutex.h>
 #include <linux/list.h>
@@ -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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-05-11  1:37 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-11  1:36 [PATCH 0/3] kexec: limit kexec_load syscall Mimi Zohar
2018-05-11  1:36 ` [PATCH 1/3] ima: based on the "secure_boot" policy limit syscalls Mimi Zohar
2018-05-11  1:36 ` [PATCH 2/3] kexec: call LSM hook for kexec_load syscall Mimi Zohar
2018-05-11  1:36 ` [PATCH 3/3] ima: based on policy require signed kexec kernel images Mimi Zohar
  -- strict thread matches above, loose matches on Subject: below --
2018-04-12 22:41 [PATCH 0/3] kexec: limit kexec_load syscall Mimi Zohar
2018-04-12 22:41 ` [PATCH 2/3] kexec: call LSM hook for " Mimi Zohar
2018-05-02 14:45   ` Eric W. Biederman
2018-05-02 15:45     ` Mimi Zohar
2018-05-03 15:51       ` Eric W. Biederman
2018-05-03 16:05         ` Casey Schaufler
2018-05-03 16:42           ` Eric W. Biederman
2018-05-03 21:06             ` Mimi Zohar
2018-05-03 21:36               ` Eric W. Biederman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).