diff mbox series

[RFC,1/2] lsm: introduce new hook security_vm_execstack

Message ID 20240315181032.645161-2-cgzones@googlemail.com (mailing list archive)
State New
Headers show
Series [RFC,1/2] lsm: introduce new hook security_vm_execstack | expand

Commit Message

Christian Göttsche March 15, 2024, 6:08 p.m. UTC
Add a new hook guarding instantiations of programs with executable
stack.  They are being warned about since commit 47a2ebb7f505 ("execve:
warn if process starts with executable stack").  Lets give LSMs the
ability to control their presence on a per application basis.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 fs/exec.c                     |  4 ++++
 include/linux/lsm_hook_defs.h |  1 +
 include/linux/security.h      |  6 ++++++
 security/security.c           | 13 +++++++++++++
 4 files changed, 24 insertions(+)

Comments

Casey Schaufler March 15, 2024, 6:22 p.m. UTC | #1
On 3/15/2024 11:08 AM, Christian Göttsche wrote:
> Add a new hook guarding instantiations of programs with executable
> stack.  They are being warned about since commit 47a2ebb7f505 ("execve:
> warn if process starts with executable stack").  Lets give LSMs the
> ability to control their presence on a per application basis.

This seems like a hideously expensive way to implement a flag
disallowing execution of programs with executable stacks. What's
wrong with adding a flag VM_NO_EXECUTABLE_STACK?

>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  fs/exec.c                     |  4 ++++
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/security.h      |  6 ++++++
>  security/security.c           | 13 +++++++++++++
>  4 files changed, 24 insertions(+)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 8cdd5b2dd09c..e6f9e980c6b1 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -829,6 +829,10 @@ int setup_arg_pages(struct linux_binprm *bprm,
>  	BUG_ON(prev != vma);
>  
>  	if (unlikely(vm_flags & VM_EXEC)) {
> +		ret = security_vm_execstack();
> +		if (ret)
> +			goto out_unlock;
> +
>  		pr_warn_once("process '%pD4' started with executable stack\n",
>  			     bprm->file);
>  	}
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 185924c56378..b31d0744e7e7 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -49,6 +49,7 @@ LSM_HOOK(int, 0, syslog, int type)
>  LSM_HOOK(int, 0, settime, const struct timespec64 *ts,
>  	 const struct timezone *tz)
>  LSM_HOOK(int, 1, vm_enough_memory, struct mm_struct *mm, long pages)
> +LSM_HOOK(int, 0, vm_execstack, void)
>  LSM_HOOK(int, 0, bprm_creds_for_exec, struct linux_binprm *bprm)
>  LSM_HOOK(int, 0, bprm_creds_from_file, struct linux_binprm *bprm, const struct file *file)
>  LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index d0eb20f90b26..084b96814970 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -294,6 +294,7 @@ int security_quota_on(struct dentry *dentry);
>  int security_syslog(int type);
>  int security_settime64(const struct timespec64 *ts, const struct timezone *tz);
>  int security_vm_enough_memory_mm(struct mm_struct *mm, long pages);
> +int security_vm_execstack(void);
>  int security_bprm_creds_for_exec(struct linux_binprm *bprm);
>  int security_bprm_creds_from_file(struct linux_binprm *bprm, const struct file *file);
>  int security_bprm_check(struct linux_binprm *bprm);
> @@ -624,6 +625,11 @@ static inline int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
>  	return __vm_enough_memory(mm, pages, cap_vm_enough_memory(mm, pages));
>  }
>  
> +static inline int security_vm_execstack(void)
> +{
> +	return 0;
> +}
> +
>  static inline int security_bprm_creds_for_exec(struct linux_binprm *bprm)
>  {
>  	return 0;
> diff --git a/security/security.c b/security/security.c
> index 0144a98d3712..f75240d0d99d 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1125,6 +1125,19 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
>  	return __vm_enough_memory(mm, pages, cap_sys_admin);
>  }
>  
> +/**
> + * security_vm_execstack() - Check if starting a program with executable stack
> + * is allowed
> + *
> + * Check whether starting a program with an executable stack is allowed.
> + *
> + * Return: Returns 0 if permission is granted.
> + */
> +int security_vm_execstack(void)
> +{
> +	return call_int_hook(vm_execstack);
> +}
> +
>  /**
>   * security_bprm_creds_for_exec() - Prepare the credentials for exec()
>   * @bprm: binary program information
Christian Göttsche March 15, 2024, 6:30 p.m. UTC | #2
On Fri, 15 Mar 2024 at 19:22, Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 3/15/2024 11:08 AM, Christian Göttsche wrote:
> > Add a new hook guarding instantiations of programs with executable
> > stack.  They are being warned about since commit 47a2ebb7f505 ("execve:
> > warn if process starts with executable stack").  Lets give LSMs the
> > ability to control their presence on a per application basis.
>
> This seems like a hideously expensive way to implement a flag
> disallowing execution of programs with executable stacks. What's
> wrong with adding a flag VM_NO_EXECUTABLE_STACK?

That would be global and not on a per application basis.
One might want to exempt known legacy programs.
Also is performance a concern for this today's rare occurrence?

> >
> > Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> > ---
> >  fs/exec.c                     |  4 ++++
> >  include/linux/lsm_hook_defs.h |  1 +
> >  include/linux/security.h      |  6 ++++++
> >  security/security.c           | 13 +++++++++++++
> >  4 files changed, 24 insertions(+)
> >
> > diff --git a/fs/exec.c b/fs/exec.c
> > index 8cdd5b2dd09c..e6f9e980c6b1 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -829,6 +829,10 @@ int setup_arg_pages(struct linux_binprm *bprm,
> >       BUG_ON(prev != vma);
> >
> >       if (unlikely(vm_flags & VM_EXEC)) {
> > +             ret = security_vm_execstack();
> > +             if (ret)
> > +                     goto out_unlock;
> > +
> >               pr_warn_once("process '%pD4' started with executable stack\n",
> >                            bprm->file);
> >       }
> > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > index 185924c56378..b31d0744e7e7 100644
> > --- a/include/linux/lsm_hook_defs.h
> > +++ b/include/linux/lsm_hook_defs.h
> > @@ -49,6 +49,7 @@ LSM_HOOK(int, 0, syslog, int type)
> >  LSM_HOOK(int, 0, settime, const struct timespec64 *ts,
> >        const struct timezone *tz)
> >  LSM_HOOK(int, 1, vm_enough_memory, struct mm_struct *mm, long pages)
> > +LSM_HOOK(int, 0, vm_execstack, void)
> >  LSM_HOOK(int, 0, bprm_creds_for_exec, struct linux_binprm *bprm)
> >  LSM_HOOK(int, 0, bprm_creds_from_file, struct linux_binprm *bprm, const struct file *file)
> >  LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm)
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index d0eb20f90b26..084b96814970 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -294,6 +294,7 @@ int security_quota_on(struct dentry *dentry);
> >  int security_syslog(int type);
> >  int security_settime64(const struct timespec64 *ts, const struct timezone *tz);
> >  int security_vm_enough_memory_mm(struct mm_struct *mm, long pages);
> > +int security_vm_execstack(void);
> >  int security_bprm_creds_for_exec(struct linux_binprm *bprm);
> >  int security_bprm_creds_from_file(struct linux_binprm *bprm, const struct file *file);
> >  int security_bprm_check(struct linux_binprm *bprm);
> > @@ -624,6 +625,11 @@ static inline int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> >       return __vm_enough_memory(mm, pages, cap_vm_enough_memory(mm, pages));
> >  }
> >
> > +static inline int security_vm_execstack(void)
> > +{
> > +     return 0;
> > +}
> > +
> >  static inline int security_bprm_creds_for_exec(struct linux_binprm *bprm)
> >  {
> >       return 0;
> > diff --git a/security/security.c b/security/security.c
> > index 0144a98d3712..f75240d0d99d 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -1125,6 +1125,19 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> >       return __vm_enough_memory(mm, pages, cap_sys_admin);
> >  }
> >
> > +/**
> > + * security_vm_execstack() - Check if starting a program with executable stack
> > + * is allowed
> > + *
> > + * Check whether starting a program with an executable stack is allowed.
> > + *
> > + * Return: Returns 0 if permission is granted.
> > + */
> > +int security_vm_execstack(void)
> > +{
> > +     return call_int_hook(vm_execstack);
> > +}
> > +
> >  /**
> >   * security_bprm_creds_for_exec() - Prepare the credentials for exec()
> >   * @bprm: binary program information
Casey Schaufler March 15, 2024, 6:41 p.m. UTC | #3
On 3/15/2024 11:30 AM, Christian Göttsche wrote:
> On Fri, 15 Mar 2024 at 19:22, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 3/15/2024 11:08 AM, Christian Göttsche wrote:
>>> Add a new hook guarding instantiations of programs with executable
>>> stack.  They are being warned about since commit 47a2ebb7f505 ("execve:
>>> warn if process starts with executable stack").  Lets give LSMs the
>>> ability to control their presence on a per application basis.
>> This seems like a hideously expensive way to implement a flag
>> disallowing execution of programs with executable stacks. What's
>> wrong with adding a flag VM_NO_EXECUTABLE_STACK?
> That would be global and not on a per application basis.
> One might want to exempt known legacy programs.

OK, I can see that.

> Also is performance a concern for this today's rare occurrence?

Performance is *always* a concern. You're adding a new hook list
for a "rare" case. You're extended SELinux policy to include the
case. This really should be a hardening feature, not an SELinux policy
feature. The hook makes no sense for an LSM like Smack, which only
implements subject+object controls. You could implement a stand alone
LSM that implements only this hook, but again, it's not really access
control, it's hardening.
Paul Moore March 15, 2024, 8:22 p.m. UTC | #4
On Fri, Mar 15, 2024 at 2:10 PM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Add a new hook guarding instantiations of programs with executable
> stack.  They are being warned about since commit 47a2ebb7f505 ("execve:
> warn if process starts with executable stack").  Lets give LSMs the
> ability to control their presence on a per application basis.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  fs/exec.c                     |  4 ++++
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/security.h      |  6 ++++++
>  security/security.c           | 13 +++++++++++++
>  4 files changed, 24 insertions(+)

Looking at the commit referenced above, I'm guessing the existing
security_file_mprotect() hook doesn't catch this?

> diff --git a/fs/exec.c b/fs/exec.c
> index 8cdd5b2dd09c..e6f9e980c6b1 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -829,6 +829,10 @@ int setup_arg_pages(struct linux_binprm *bprm,
>         BUG_ON(prev != vma);
>
>         if (unlikely(vm_flags & VM_EXEC)) {
> +               ret = security_vm_execstack();
> +               if (ret)
> +                       goto out_unlock;
> +
>                 pr_warn_once("process '%pD4' started with executable stack\n",
>                              bprm->file);
>         }

Instead of creating a new LSM hook, have you considered calling the
existing security_file_mprotect() hook?  The existing LSM controls
there may not be a great fit in this case, but I'd like to hear if
you've tried that, and if you have, what made you decide a new hook
was the better option?
Kees Cook March 16, 2024, 3:24 a.m. UTC | #5
On March 15, 2024 1:22:39 PM PDT, Paul Moore <paul@paul-moore.com> wrote:
>On Fri, Mar 15, 2024 at 2:10 PM Christian Göttsche
><cgzones@googlemail.com> wrote:
>>
>> Add a new hook guarding instantiations of programs with executable
>> stack.  They are being warned about since commit 47a2ebb7f505 ("execve:
>> warn if process starts with executable stack").  Lets give LSMs the
>> ability to control their presence on a per application basis.
>>
>> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>> ---
>>  fs/exec.c                     |  4 ++++
>>  include/linux/lsm_hook_defs.h |  1 +
>>  include/linux/security.h      |  6 ++++++
>>  security/security.c           | 13 +++++++++++++
>>  4 files changed, 24 insertions(+)
>
>Looking at the commit referenced above, I'm guessing the existing
>security_file_mprotect() hook doesn't catch this?
>
>> diff --git a/fs/exec.c b/fs/exec.c
>> index 8cdd5b2dd09c..e6f9e980c6b1 100644
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -829,6 +829,10 @@ int setup_arg_pages(struct linux_binprm *bprm,
>>         BUG_ON(prev != vma);
>>
>>         if (unlikely(vm_flags & VM_EXEC)) {
>> +               ret = security_vm_execstack();
>> +               if (ret)
>> +                       goto out_unlock;
>> +
>>                 pr_warn_once("process '%pD4' started with executable stack\n",
>>                              bprm->file);
>>         }
>
>Instead of creating a new LSM hook, have you considered calling the
>existing security_file_mprotect() hook?  The existing LSM controls
>there may not be a great fit in this case, but I'd like to hear if
>you've tried that, and if you have, what made you decide a new hook
>was the better option?

Also, can't MDWE handle this already?
https://git.kernel.org/linus/b507808ebce23561d4ff8c2aa1fb949fe402bc61

-Kees
Paul Moore March 19, 2024, 11:10 p.m. UTC | #6
On Fri, Mar 15, 2024 at 11:24 PM Kees Cook <kees@kernel.org> wrote:
> On March 15, 2024 1:22:39 PM PDT, Paul Moore <paul@paul-moore.com> wrote:
> >On Fri, Mar 15, 2024 at 2:10 PM Christian Göttsche
> ><cgzones@googlemail.com> wrote:
> >>
> >> Add a new hook guarding instantiations of programs with executable
> >> stack.  They are being warned about since commit 47a2ebb7f505 ("execve:
> >> warn if process starts with executable stack").  Lets give LSMs the
> >> ability to control their presence on a per application basis.
> >>
> >> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> >> ---
> >>  fs/exec.c                     |  4 ++++
> >>  include/linux/lsm_hook_defs.h |  1 +
> >>  include/linux/security.h      |  6 ++++++
> >>  security/security.c           | 13 +++++++++++++
> >>  4 files changed, 24 insertions(+)
> >
> >Looking at the commit referenced above, I'm guessing the existing
> >security_file_mprotect() hook doesn't catch this?
> >
> >> diff --git a/fs/exec.c b/fs/exec.c
> >> index 8cdd5b2dd09c..e6f9e980c6b1 100644
> >> --- a/fs/exec.c
> >> +++ b/fs/exec.c
> >> @@ -829,6 +829,10 @@ int setup_arg_pages(struct linux_binprm *bprm,
> >>         BUG_ON(prev != vma);
> >>
> >>         if (unlikely(vm_flags & VM_EXEC)) {
> >> +               ret = security_vm_execstack();
> >> +               if (ret)
> >> +                       goto out_unlock;
> >> +
> >>                 pr_warn_once("process '%pD4' started with executable stack\n",
> >>                              bprm->file);
> >>         }
> >
> >Instead of creating a new LSM hook, have you considered calling the
> >existing security_file_mprotect() hook?  The existing LSM controls
> >there may not be a great fit in this case, but I'd like to hear if
> >you've tried that, and if you have, what made you decide a new hook
> >was the better option?
>
> Also, can't MDWE handle this already?
> https://git.kernel.org/linus/b507808ebce23561d4ff8c2aa1fb949fe402bc61

It looks like it, but that doesn't mean there isn't also value in an
associated LSM hook as the LSM hook would admins and security policy
developers/analysts to incorporate this as part of the system's
security policy.  It's great that we have all of these cool knobs that
we can play with independent of each other, but sometimes you really
want a single unified security policy that you can look at, analyze,
and reason about.

Regardless, my previous comments still stand, I'd like to hear
verification that the existing security_file_mprotect() hook is not
sufficient, and if its current placement is lacking, why calling it
from a second location wasn't practical and required the creation of a
new LSM hook.
diff mbox series

Patch

diff --git a/fs/exec.c b/fs/exec.c
index 8cdd5b2dd09c..e6f9e980c6b1 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -829,6 +829,10 @@  int setup_arg_pages(struct linux_binprm *bprm,
 	BUG_ON(prev != vma);
 
 	if (unlikely(vm_flags & VM_EXEC)) {
+		ret = security_vm_execstack();
+		if (ret)
+			goto out_unlock;
+
 		pr_warn_once("process '%pD4' started with executable stack\n",
 			     bprm->file);
 	}
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 185924c56378..b31d0744e7e7 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -49,6 +49,7 @@  LSM_HOOK(int, 0, syslog, int type)
 LSM_HOOK(int, 0, settime, const struct timespec64 *ts,
 	 const struct timezone *tz)
 LSM_HOOK(int, 1, vm_enough_memory, struct mm_struct *mm, long pages)
+LSM_HOOK(int, 0, vm_execstack, void)
 LSM_HOOK(int, 0, bprm_creds_for_exec, struct linux_binprm *bprm)
 LSM_HOOK(int, 0, bprm_creds_from_file, struct linux_binprm *bprm, const struct file *file)
 LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm)
diff --git a/include/linux/security.h b/include/linux/security.h
index d0eb20f90b26..084b96814970 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -294,6 +294,7 @@  int security_quota_on(struct dentry *dentry);
 int security_syslog(int type);
 int security_settime64(const struct timespec64 *ts, const struct timezone *tz);
 int security_vm_enough_memory_mm(struct mm_struct *mm, long pages);
+int security_vm_execstack(void);
 int security_bprm_creds_for_exec(struct linux_binprm *bprm);
 int security_bprm_creds_from_file(struct linux_binprm *bprm, const struct file *file);
 int security_bprm_check(struct linux_binprm *bprm);
@@ -624,6 +625,11 @@  static inline int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
 	return __vm_enough_memory(mm, pages, cap_vm_enough_memory(mm, pages));
 }
 
+static inline int security_vm_execstack(void)
+{
+	return 0;
+}
+
 static inline int security_bprm_creds_for_exec(struct linux_binprm *bprm)
 {
 	return 0;
diff --git a/security/security.c b/security/security.c
index 0144a98d3712..f75240d0d99d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1125,6 +1125,19 @@  int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
 	return __vm_enough_memory(mm, pages, cap_sys_admin);
 }
 
+/**
+ * security_vm_execstack() - Check if starting a program with executable stack
+ * is allowed
+ *
+ * Check whether starting a program with an executable stack is allowed.
+ *
+ * Return: Returns 0 if permission is granted.
+ */
+int security_vm_execstack(void)
+{
+	return call_int_hook(vm_execstack);
+}
+
 /**
  * security_bprm_creds_for_exec() - Prepare the credentials for exec()
  * @bprm: binary program information