[RFC PATCH 2/2] selinux: wire up new execstack LSM hook

[RFC PATCH 2/2] selinux: wire up new execstack LSM hook

[Christian Göttsche]

Perform a process { execstack } check unless virtual memory is marked
executable by default.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 security/selinux/hooks.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a0fde0641f77..daf901916836 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -113,6 +113,8 @@ struct selinux_state selinux_state;
 /* SECMARK reference count */
 static atomic_t selinux_secmark_refcount = ATOMIC_INIT(0);
 
+static int default_noexec __ro_after_init;
+
 #ifdef CONFIG_SECURITY_SELINUX_DEVELOP
 static int selinux_enforcing_boot __initdata;
 
@@ -2221,6 +2223,18 @@ static int selinux_vm_enough_memory(struct mm_struct *mm, long pages)
 	return cap_sys_admin;
 }
 
+static int selinux_vm_execstack(void)
+{
+	u32 sid;
+
+	if (!default_noexec)
+		return 0;
+
+	sid = current_sid();
+	return avc_has_perm(sid, sid, SECCLASS_PROCESS,
+			    PROCESS__EXECSTACK, NULL);
+}
+
 /* binprm security operations */
 
 static u32 ptrace_parent_sid(void)
@@ -3767,8 +3781,6 @@ static int selinux_file_ioctl_compat(struct file *file, unsigned int cmd,
 	return selinux_file_ioctl(file, cmd, arg);
 }
 
-static int default_noexec __ro_after_init;
-
 static int file_map_prot_check(struct file *file, unsigned long prot, int shared)
 {
 	const struct cred *cred = current_cred();
@@ -7120,6 +7132,7 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(quota_on, selinux_quota_on),
 	LSM_HOOK_INIT(syslog, selinux_syslog),
 	LSM_HOOK_INIT(vm_enough_memory, selinux_vm_enough_memory),
+	LSM_HOOK_INIT(vm_execstack, selinux_vm_execstack),
 
 	LSM_HOOK_INIT(netlink_send, selinux_netlink_send),
 
-- 
2.43.0

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

[Christian Göttsche]

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(+)

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

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

[Casey Schaufler]

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

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

[Christian Göttsche]

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

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

[Casey Schaufler]

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.

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

[Paul Moore]

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?

-- 
paul-moore.com

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

[Kees Cook]

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

-- 
Kees Cook

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

[Paul Moore]

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.

-- 
paul-moore.com