Re: [d-kernel] [PATCH] AltHa: handle setcap binaries in the same way as setuid ones

["Vladimir D. Seleznev" <vseleznv@altlinux.org>]

On Thu, May 19, 2022 at 03:09:23AM +0300, Vitaly Chikunov wrote:
> Vladimir,
> 
> On Wed, May 18, 2022 at 03:24:58PM +0000, Vladimir D. Seleznev wrote:
> > altha.nosuid facility controls what binaries can raise user privilleges.
> > Prior to this commit it only handled setuid binaries, but it was still
> > possible to raise privilleges via setcaps. Now it handles both setuid
> > and setcap binaries.
> > 
> > Signed-off-by: Vladimir D. Seleznev <vseleznv@altlinux.org>
> > ---
> 
> You don't need to send cover letter for a single patch if you add your
> comments here, after '---'. But you still need to add v5 next time.

OK.

> >  Documentation/admin-guide/LSM/AltHa.rst |  6 ++--
> >  security/altha/altha_lsm.c              | 48 ++++++++++++++++++++-----
> >  2 files changed, 43 insertions(+), 11 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/LSM/AltHa.rst b/Documentation/admin-guide/LSM/AltHa.rst
> > index be698709d3f0..beda40601c9e 100644
> > --- a/Documentation/admin-guide/LSM/AltHa.rst
> > +++ b/Documentation/admin-guide/LSM/AltHa.rst
> > @@ -3,7 +3,7 @@ AltHa
> >  ====
> >  
> >  AltHa is a Linux Security Module currently has three userspace hardening options:
> > -    * ignore SUID on binaries (with exceptions possible);
> > +    * ignore SUID and setcaps on binaries (with exceptions possible);
> 
> Perhaps, description of SECURITY_ALTHA in Kconfig should be
> updated too, if I'm counting correctly.

OK.

> >      * prevent running selected script interpreters in interactive mode;
> >      * disable open file unlinking in selected dirs.
> >      * enable kiosk mode
> > @@ -15,12 +15,12 @@ through sysctls in ``/proc/sys/kernel/altha``.
> >  
> >  NoSUID
> >  ============
> > -Modern Linux systems can be used with minimal (or even zero at least for OWL and ALT) usage of SUID programms, but in many cases in full-featured desktop or server systems there are plenty of them: uncounted and sometimes unnecessary. Privileged programms are always an attack surface, but mounting filesystems with ``nosuid`` flag doesn't provide enough granularity in SUID binaries management. This LSM module provides a single control point for all SUID binaries. When this submodule is enabled, SUID bits on all binaries except explicitly listed are system-wide ignored.
> > +Modern Linux systems can be used with minimal (or even zero at least for OWL and ALT) usage of SUID programms, but in many cases in full-featured desktop or server systems there are plenty of them: uncounted and sometimes unnecessary. Privileged programms are always an attack surface, but mounting filesystems with ``nosuid`` flag doesn't provide enough granularity in SUID binaries management. This LSM module provides a single control point for all SUID and setcap binaries. When this submodule is enabled, SUID and setcap bits on all binaries except explicitly listed are system-wide ignored.
> >  
> >  Sysctl parameters and defaults:
> >  
> >  * ``kernel.altha.nosuid.enabled = 0``, set to 1 to enable
> > -* ``kernel.altha.nosuid.exceptions =``, colon-separated list of enabled SUID binaries, for example: ``/bin/su:/usr/libexec/hasher-priv/hasher-priv``
> > +* ``kernel.altha.nosuid.exceptions =``, colon-separated list of enabled SUID and setcap binaries, for example: ``/bin/su:/usr/libexec/hasher-priv/hasher-priv``
> >  
> >  RestrScript
> >  ============
> > diff --git a/security/altha/altha_lsm.c b/security/altha/altha_lsm.c
> > index c670ad7ed458..4f6b309445c0 100644
> > --- a/security/altha/altha_lsm.c
> > +++ b/security/altha/altha_lsm.c
> > @@ -11,6 +11,7 @@
> >  
> >  #include <linux/lsm_hooks.h>
> >  #include <linux/cred.h>
> > +#include <linux/capability.h>
> >  #include <linux/sysctl.h>
> >  #include <linux/binfmts.h>
> >  #include <linux/file.h>
> > @@ -237,10 +238,19 @@ int is_olock_dir(struct inode *inode)
> >  	return 0;
> >  }
> >  
> > +static int has_any_caps(struct cred *cred)
> 
> Why helper for a single use? Also, it checks definitely not for 'any'
> caps.

It makes code look cleaner. But OK I'll remove this helper.

> > +{
> > +	return !cap_isclear(cred->cap_permitted) ||
> > +	       !cap_isclear(cred->cap_effective);
> > +
> > +	return 0;
> > +}
> > +
> >  /* Hooks */
> >  static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * fi)
> >  {
> >  	struct altha_list_struct *node;
> > +	char *setuidcap_srt = "setuid";
> 
> What is 'srt'? Please rename if it means 'str'.

OK.

> >  	/* when it's not a shebang issued script interpreter */
> >  	if (rstrscript_enabled && bprm->executable == bprm->interpreter) {
> >  		char *path_p;
> > @@ -267,11 +277,30 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f
> >  		up_read(&interpreters_sem);
> >  		kfree(path_buffer);
> >  	}
> > -	if (unlikely(nosuid_enabled &&
> > -		     !uid_eq(bprm->cred->uid, bprm->cred->euid))) {
> > +	if (nosuid_enabled) {
> >  		char *path_p;
> >  		char *path_buffer;
> > -		uid_t cur_uid;
> > +		int is_setuid = 0, is_setcap = 0;
> > +		uid_t cur_uid, cur_euid;
> > +
> > +		is_setuid = !uid_eq(bprm->cred->uid, bprm->cred->euid);
> 
> It seems we want to restrict root to suid into user too, because this
> way of switching users is never used. Perhaps, this decision should be
> documented in comments.

Or we can restrict only switching to superuser. What do you think would
be a correct way?

> > +
> > +		if (!is_setuid) {
> > +			cur_euid = from_kuid(bprm->cred->user_ns, bprm->cred->euid);
> > +			if (cur_euid != (uid_t) 0)
> > +				is_setcap = has_any_caps(bprm->cred);
> 
> Perhaps, this should also be documented in comment why such complicated
> logic of setting `is_setcap`. -- Because, exec by root always have
> capabilities which does not imply setcap and you want to avoid this
> situation and accidental drop of legitimate root capabilities.

Isn't that obvious?

> > +		}
> > +
> > +		/*
> > +		 * If no suid but it has any caps, change message string from
> > +		 * setuid to setcap.
> 
> Isn't this comment is obvious?

It is.

> > +		 */
> > +		if (is_setcap)
> > +			setuidcap_srt = "setcap";
> 
> Why not move this above when you set `is_setcap'.

OK.

> > +
> > +		/* If no suid and no caps detected, exit. */
> > +		if (!is_setuid && !is_setcap)
> > +			return 0;
> >  
> >  		path_buffer = kmalloc(PATH_MAX, GFP_KERNEL);
> >  		if (!path_buffer)
> > @@ -283,8 +312,8 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f
> >  		list_for_each_entry(node, &nosuid_exceptions_list, list) {
> >  			if (strcmp(path_p, node->spath) == 0) {
> >  				pr_notice_ratelimited
> > -				    ("AltHa/NoSUID: %s permitted to setuid from %d\n",
> > -				     bprm->filename, cur_uid);
> > +				    ("AltHa/NoSUID: %s permitted to %s from %d\n",
> > +				     bprm->filename, setuidcap_srt, cur_uid);
> >  				up_read(&nosuid_exceptions_sem);
> >  				kfree(path_buffer);
> >  				return 0;
> > @@ -292,9 +321,12 @@ static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * f
> >  		}
> >  		up_read(&nosuid_exceptions_sem);
> >  		pr_notice_ratelimited
> > -		    ("AltHa/NoSUID: %s prevented to setuid from %d\n",
> > -		     bprm->filename, cur_uid);
> > -		bprm->cred->euid = bprm->cred->uid;
> > +		    ("AltHa/NoSUID: %s prevented to %s from %d\n",
> > +		     bprm->filename, setuidcap_srt, cur_uid);
> > +		if (is_setuid)
> > +			bprm->cred->euid = bprm->cred->uid;
> > +		cap_clear(bprm->cred->cap_permitted);
> > +		cap_clear(bprm->cred->cap_effective);
> 
> Any exec under root will drop privileges, is it intended? 

No, this code does not run if there is no either setuid or setcap.
Everything is fine.

> I think it isn't. For example, run dmesg under root when
> kernel.altha.nosuid.enabled=1 is set.
> 
> I strongly suggest adding tests to this change.
> 
> Thanks,
> 
> 
> >  		kfree(path_buffer);
> >  	}
> >  	return 0;
> > -- 
> > 2.33.3
> > 

-- 
   WBR,
   Vladimir D. Seleznev