[d-kernel] AltHa: handle setcap binaries in the same way as setuid ones (Was: AltHa: nosuid handles capabilities as well)

[d-kernel] AltHa: handle setcap binaries in the same way as setuid ones (Was: AltHa: nosuid handles capabilities as well)

[Vladimir D. Seleznev]

Hi, d-kernel!

Here's a new version of the patch, built and tested against
std-def-5.15.40-alt1.

-- 
   WBR,
   Vladimir D. Seleznev

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

[Vladimir D. Seleznev]

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>
---
 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);
     * 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)
+{
+	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";
 	/* 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);
+
+		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);
+		}
+
+		/*
+		 * If no suid but it has any caps, change message string from
+		 * setuid to setcap.
+		 */
+		if (is_setcap)
+			setuidcap_srt = "setcap";
+
+		/* 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);
 		kfree(path_buffer);
 	}
 	return 0;
-- 
2.33.3

Re: [d-kernel] AltHa: handle setcap binaries in the same way as setuid ones (Was: AltHa: nosuid handles capabilities as well)

[Vitaly Chikunov]

Vladimir,

On Wed, May 18, 2022 at 03:24:57PM +0000, Vladimir D. Seleznev wrote:
> Hi, d-kernel!
> 
> Here's a new version of the patch, built and tested against
> std-def-5.15.40-alt1.

I wonder how can you send incorrect cover letter with git send-email.

It should say

  [PATCH v4 0/1] AltHa: handle setcap binaries in the same way as setuid ones

Also I suggest to add changelog in the future.

Thanks,

> 
> -- 
>    WBR,
>    Vladimir D. Seleznev
> 
> _______________________________________________
> devel-kernel mailing list
> devel-kernel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel-kernel

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

[Vitaly Chikunov]

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.


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

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

> +{
> +	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'.

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

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

> +		}
> +
> +		/*
> +		 * If no suid but it has any caps, change message string from
> +		 * setuid to setcap.

Isn't this comment is obvious?

> +		 */
> +		if (is_setcap)
> +			setuidcap_srt = "setcap";

Why not move this above when you set `is_setcap'.

> +
> +		/* 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? 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
> 
> _______________________________________________
> devel-kernel mailing list
> devel-kernel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel-kernel

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

[Vladimir D. Seleznev]

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

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

[Vitaly Chikunov]

Vladimir,

On Thu, May 19, 2022 at 04:24:17PM +0300, Vladimir D. Seleznev wrote:
> On Thu, May 19, 2022 at 03:09:23AM +0300, Vitaly Chikunov wrote:
> > >  	/* 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?

I'm not that perfectionist.

> 
> > > +
> > > +		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?

It's obvious only for people who know well what is bprm->cred at the time of
this LSM hook.

> > > +		/* If no suid and no caps detected, exit. */
> > > +		if (!is_setuid && !is_setcap)
> > > +			return 0;
> > >   ...
> > > +		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.

You are right.

Thanks,