[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 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
Mimi Zohar writes: > 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(). 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 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
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 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
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. Eric ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
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. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
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. > 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 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
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. > > */ > ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
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? Eric > 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. >*/ ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 2/3] kexec: call LSM hook for kexec_load syscall
Hi Eric, I'd really appreciate your reviewing/ack'ing this patch. thanks, Mimi On Thu, 2018-04-12 at 18:41 -0400, Mimi Zohar wrote: > 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. >*/ ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
[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 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec