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

[d-kernel] [PATCH v5] 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/Kconfig                  |  2 +-
 security/altha/altha_lsm.c              | 47 ++++++++++++++++++++-----
 3 files changed, 43 insertions(+), 12 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/Kconfig b/security/altha/Kconfig
index 4bafdef4e58e..cd1dd69cc48d 100644
--- a/security/altha/Kconfig
+++ b/security/altha/Kconfig
@@ -4,7 +4,7 @@ config SECURITY_ALTHA
 	default n
 	help
 	  Some hardening options:
-	    * ignore SUID on binaries (with exceptions possible);
+	    * ignore SUID and setcap on binaries (with exceptions possible);
 	    * prevent running selected script interprers in interactive move;
 	    * WxorX for filesystems (with exceptions possible);
 
diff --git a/security/altha/altha_lsm.c b/security/altha/altha_lsm.c
index c670ad7ed458..e597d722ab04 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>
@@ -241,6 +242,7 @@ int is_olock_dir(struct inode *inode)
 static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * fi)
 {
 	struct altha_list_struct *node;
+	char *setuidcap_str = "setuid";
 	/* when it's not a shebang issued script interpreter */
 	if (rstrscript_enabled && bprm->executable == bprm->interpreter) {
 		char *path_p;
@@ -267,11 +269,37 @@ 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;
+
+		/*
+		 * While nosuid is supposed to prevent switching to superuser,
+		 * it does not check swtiching to a non-privileged user because
+		 * it is almost never user.
+		 */
+		is_setuid = !uid_eq(bprm->cred->uid, bprm->cred->euid);
+
+		if (!is_setuid) {
+			cur_euid = from_kuid(bprm->cred->user_ns, bprm->cred->euid);
+			/*
+			 * Check capabilities only for effectivly non-superuser
+			 * processes: superuser processess always have
+			 * capabilities, should keep them so these processes
+			 * continue working correctly.
+			 */
+			if (cur_euid != (uid_t) 0)
+				is_setcap = !(cap_isclear(bprm->cred->cap_permitted)
+						& cap_isclear(bprm->cred->cap_effective));
+
+			setuidcap_str = "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 +311,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_str, cur_uid);
 				up_read(&nosuid_exceptions_sem);
 				kfree(path_buffer);
 				return 0;
@@ -292,9 +320,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_str, 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] [PATCH v5] AltHa: handle setcap binaries in the same way as setuid ones

[Vladimir D. Seleznev]

On Mon, May 23, 2022 at 01:44:04PM +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>
> ---
>  Documentation/admin-guide/LSM/AltHa.rst |  6 ++--
>  security/altha/Kconfig                  |  2 +-
>  security/altha/altha_lsm.c              | 47 ++++++++++++++++++++-----
>  3 files changed, 43 insertions(+), 12 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/Kconfig b/security/altha/Kconfig
> index 4bafdef4e58e..cd1dd69cc48d 100644
> --- a/security/altha/Kconfig
> +++ b/security/altha/Kconfig
> @@ -4,7 +4,7 @@ config SECURITY_ALTHA
>  	default n
>  	help
>  	  Some hardening options:
> -	    * ignore SUID on binaries (with exceptions possible);
> +	    * ignore SUID and setcap on binaries (with exceptions possible);
>  	    * prevent running selected script interprers in interactive move;
>  	    * WxorX for filesystems (with exceptions possible);
>  
> diff --git a/security/altha/altha_lsm.c b/security/altha/altha_lsm.c
> index c670ad7ed458..e597d722ab04 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>
> @@ -241,6 +242,7 @@ int is_olock_dir(struct inode *inode)
>  static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * fi)
>  {
>  	struct altha_list_struct *node;
> +	char *setuidcap_str = "setuid";
>  	/* when it's not a shebang issued script interpreter */
>  	if (rstrscript_enabled && bprm->executable == bprm->interpreter) {
>  		char *path_p;
> @@ -267,11 +269,37 @@ 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;
> +
> +		/*
> +		 * While nosuid is supposed to prevent switching to superuser,
> +		 * it does not check swtiching to a non-privileged user because
> +		 * it is almost never user.
> +		 */
> +		is_setuid = !uid_eq(bprm->cred->uid, bprm->cred->euid);
> +
> +		if (!is_setuid) {
> +			cur_euid = from_kuid(bprm->cred->user_ns, bprm->cred->euid);
> +			/*
> +			 * Check capabilities only for effectivly non-superuser
> +			 * processes: superuser processess always have
> +			 * capabilities, should keep them so these processes
> +			 * continue working correctly.
> +			 */
> +			if (cur_euid != (uid_t) 0)
> +				is_setcap = !(cap_isclear(bprm->cred->cap_permitted)
> +						& cap_isclear(bprm->cred->cap_effective));
> +
> +			setuidcap_str = "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 +311,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_str, cur_uid);
>  				up_read(&nosuid_exceptions_sem);
>  				kfree(path_buffer);
>  				return 0;
> @@ -292,9 +320,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_str, 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

Ping

-- 
   WBR,
   Vladimir D. Seleznev

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

[Vitaly Chikunov]

Vladimir,

On Mon, May 30, 2022 at 02:48:56PM +0300, Vladimir D. Seleznev wrote:
> On Mon, May 23, 2022 at 01:44:04PM +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>
> > ---
> >  Documentation/admin-guide/LSM/AltHa.rst |  6 ++--
> >  security/altha/Kconfig                  |  2 +-
> >  security/altha/altha_lsm.c              | 47 ++++++++++++++++++++-----
> >  3 files changed, 43 insertions(+), 12 deletions(-)
> > 
> 
> Ping

What about tests?

ps. I also have additional thoughts about this protection concept itself.

> 
> -- 
>    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 v5] AltHa: handle setcap binaries in the same way as setuid ones

[Vladimir D. Seleznev]

On Mon, May 30, 2022 at 06:11:25PM +0300, Vitaly Chikunov wrote:
> Vladimir,
> 
> On Mon, May 30, 2022 at 02:48:56PM +0300, Vladimir D. Seleznev wrote:
> > On Mon, May 23, 2022 at 01:44:04PM +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>
> > > ---
> > >  Documentation/admin-guide/LSM/AltHa.rst |  6 ++--
> > >  security/altha/Kconfig                  |  2 +-
> > >  security/altha/altha_lsm.c              | 47 ++++++++++++++++++++-----
> > >  3 files changed, 43 insertions(+), 12 deletions(-)
> > > 
> > 
> > Ping
> 
> What about tests?

I'm not ready to put efforts for tests at this moment. Please apply the
patch, the tests can be a future work for this module.

> ps. I also have additional thoughts about this protection concept itself.

-- 
   WBR,
   Vladimir D. Seleznev

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

[Dmitry V. Levin]

On Mon, May 30, 2022 at 06:24:12PM +0300, Vladimir D. Seleznev wrote:
> On Mon, May 30, 2022 at 06:11:25PM +0300, Vitaly Chikunov wrote:
[...]
> > What about tests?
> 
> I'm not ready to put efforts for tests at this moment. Please apply the
> patch, the tests can be a future work for this module.

In the absence of tests, how can we make sure the new feature works properly?


-- 
ldv

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

[Andrey Savchenko]

[-- Attachment #1: Type: text/plain, Size: 5064 bytes --]

On Mon, 23 May 2022 13:44:04 +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>
> ---
>  Documentation/admin-guide/LSM/AltHa.rst |  6 ++--
>  security/altha/Kconfig                  |  2 +-
>  security/altha/altha_lsm.c              | 47 ++++++++++++++++++++-----
>  3 files changed, 43 insertions(+), 12 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/Kconfig b/security/altha/Kconfig
> index 4bafdef4e58e..cd1dd69cc48d 100644
> --- a/security/altha/Kconfig
> +++ b/security/altha/Kconfig
> @@ -4,7 +4,7 @@ config SECURITY_ALTHA
>  	default n
>  	help
>  	  Some hardening options:
> -	    * ignore SUID on binaries (with exceptions possible);
> +	    * ignore SUID and setcap on binaries (with exceptions possible);
>  	    * prevent running selected script interprers in interactive move;
>  	    * WxorX for filesystems (with exceptions possible);
>  
> diff --git a/security/altha/altha_lsm.c b/security/altha/altha_lsm.c
> index c670ad7ed458..e597d722ab04 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>
> @@ -241,6 +242,7 @@ int is_olock_dir(struct inode *inode)
>  static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * fi)
>  {
>  	struct altha_list_struct *node;
> +	char *setuidcap_str = "setuid";
>  	/* when it's not a shebang issued script interpreter */
>  	if (rstrscript_enabled && bprm->executable == bprm->interpreter) {
>  		char *path_p;
> @@ -267,11 +269,37 @@ 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;
> +
> +		/*
> +		 * While nosuid is supposed to prevent switching to superuser,
> +		 * it does not check swtiching to a non-privileged user because
> +		 * it is almost never user.

Looks like a typo. Did you mean "almost never used"?

Best regards,
Andrew Savchenko

[-- Attachment #2: Type: application/pgp-signature, Size: 833 bytes --]

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

[Vladimir D. Seleznev]

On Mon, May 30, 2022 at 06:45:09PM +0300, Dmitry V. Levin wrote:
> On Mon, May 30, 2022 at 06:24:12PM +0300, Vladimir D. Seleznev wrote:
> > On Mon, May 30, 2022 at 06:11:25PM +0300, Vitaly Chikunov wrote:
> [...]
> > > What about tests?
> > 
> > I'm not ready to put efforts for tests at this moment. Please apply the
> > patch, the tests can be a future work for this module.
> 
> In the absence of tests, how can we make sure the new feature works properly?

I tested it. The tests are good, but how do you know they are correct? I
don't mind to write tests but not now.

-- 
   WBR,
   Vladimir D. Seleznev

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

[Vladimir D. Seleznev]

On Mon, May 30, 2022 at 08:08:12PM +0300, Andrey Savchenko wrote:
> On Mon, 23 May 2022 13:44:04 +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>
> > ---
> >  Documentation/admin-guide/LSM/AltHa.rst |  6 ++--
> >  security/altha/Kconfig                  |  2 +-
> >  security/altha/altha_lsm.c              | 47 ++++++++++++++++++++-----
> >  3 files changed, 43 insertions(+), 12 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/Kconfig b/security/altha/Kconfig
> > index 4bafdef4e58e..cd1dd69cc48d 100644
> > --- a/security/altha/Kconfig
> > +++ b/security/altha/Kconfig
> > @@ -4,7 +4,7 @@ config SECURITY_ALTHA
> >  	default n
> >  	help
> >  	  Some hardening options:
> > -	    * ignore SUID on binaries (with exceptions possible);
> > +	    * ignore SUID and setcap on binaries (with exceptions possible);
> >  	    * prevent running selected script interprers in interactive move;
> >  	    * WxorX for filesystems (with exceptions possible);
> >  
> > diff --git a/security/altha/altha_lsm.c b/security/altha/altha_lsm.c
> > index c670ad7ed458..e597d722ab04 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>
> > @@ -241,6 +242,7 @@ int is_olock_dir(struct inode *inode)
> >  static int altha_bprm_creds_from_file(struct linux_binprm *bprm, struct file * fi)
> >  {
> >  	struct altha_list_struct *node;
> > +	char *setuidcap_str = "setuid";
> >  	/* when it's not a shebang issued script interpreter */
> >  	if (rstrscript_enabled && bprm->executable == bprm->interpreter) {
> >  		char *path_p;
> > @@ -267,11 +269,37 @@ 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;
> > +
> > +		/*
> > +		 * While nosuid is supposed to prevent switching to superuser,
> > +		 * it does not check swtiching to a non-privileged user because
> > +		 * it is almost never user.
> 
> Looks like a typo. Did you mean "almost never used"?

It is. Yes, I mean that, thank you! I'll fix this.

-- 
   WBR,
   Vladimir D. Seleznev

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

[Dmitry V. Levin]

On Tue, May 31, 2022 at 12:28:19AM +0300, Vladimir D. Seleznev wrote:
> On Mon, May 30, 2022 at 06:45:09PM +0300, Dmitry V. Levin wrote:
> > On Mon, May 30, 2022 at 06:24:12PM +0300, Vladimir D. Seleznev wrote:
> > > On Mon, May 30, 2022 at 06:11:25PM +0300, Vitaly Chikunov wrote:
> > [...]
> > > > What about tests?
> > > 
> > > I'm not ready to put efforts for tests at this moment. Please apply the
> > > patch, the tests can be a future work for this module.
> > 
> > In the absence of tests, how can we make sure the new feature works properly?
> 
> I tested it. The tests are good, but how do you know they are correct? I
> don't mind to write tests but not now.

Tests are crucial in proving that new features work properly.
In some projects, e.g. strace, a new feature is not merged
until accompanied with a proper test.

What's stopping you from supplying a test now?


-- 
ldv

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

[Vladimir D. Seleznev]

On Tue, May 31, 2022 at 09:45:03AM +0300, Dmitry V. Levin wrote:
> On Tue, May 31, 2022 at 12:28:19AM +0300, Vladimir D. Seleznev wrote:
> > On Mon, May 30, 2022 at 06:45:09PM +0300, Dmitry V. Levin wrote:
> > > On Mon, May 30, 2022 at 06:24:12PM +0300, Vladimir D. Seleznev wrote:
> > > > On Mon, May 30, 2022 at 06:11:25PM +0300, Vitaly Chikunov wrote:
> > > [...]
> > > > > What about tests?
> > > > 
> > > > I'm not ready to put efforts for tests at this moment. Please apply the
> > > > patch, the tests can be a future work for this module.
> > > 
> > > In the absence of tests, how can we make sure the new feature works properly?
> > 
> > I tested it. The tests are good, but how do you know they are correct? I
> > don't mind to write tests but not now.
> 
> Tests are crucial in proving that new features work properly.
> In some projects, e.g. strace, a new feature is not merged
> until accompanied with a proper test.
> 
> What's stopping you from supplying a test now?

I'm not an expert in kernel tests framework, so I need to involve time
to learn it. vt@ suggested taking as an example kiosk test, but as far
as I understand it does not use kernel test facility, instead it is
written independently, and it poorly suitable for altha nosuid
functionality. Besides, the kiost test does not run during build process
(correct me if I'm wrong), so its presence does not make things any
better.

AltHa code is almost stand-alone and does not change any part of the
vanilla kernel. I think if it does not work as expected, users will
report that (and I use it).

-- 
   WBR,
   Vladimir D. Seleznev

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

[Vitaly Chikunov]

Vladimir,

On Wed, Jun 01, 2022 at 01:47:27AM +0300, Vladimir D. Seleznev wrote:
> On Tue, May 31, 2022 at 09:45:03AM +0300, Dmitry V. Levin wrote:
> > On Tue, May 31, 2022 at 12:28:19AM +0300, Vladimir D. Seleznev wrote:
> > > On Mon, May 30, 2022 at 06:45:09PM +0300, Dmitry V. Levin wrote:
> > > > On Mon, May 30, 2022 at 06:24:12PM +0300, Vladimir D. Seleznev wrote:
> > > > > On Mon, May 30, 2022 at 06:11:25PM +0300, Vitaly Chikunov wrote:
> > > > [...]
> > > > > > What about tests?
> > > > > 
> > > > > I'm not ready to put efforts for tests at this moment. Please apply the
> > > > > patch, the tests can be a future work for this module.
> > > > 
> > > > In the absence of tests, how can we make sure the new feature works properly?
> > > 
> > > I tested it. The tests are good, but how do you know they are correct? I
> > > don't mind to write tests but not now.
> > 
> > Tests are crucial in proving that new features work properly.
> > In some projects, e.g. strace, a new feature is not merged
> > until accompanied with a proper test.
> > 
> > What's stopping you from supplying a test now?
> 
> I'm not an expert in kernel tests framework, so I need to involve time
> to learn it.

You'll be disappointed be the kernel test framework. Basically it's just
convention to print in TAP format. So for shell scripts you're on your
own. And I'm suggesting you to write just a simple and understandable
test shell script. Kiosk test is just an example of such script and not
"framework" suggestion. It's as suitable as bash.

Only properties we need for such test script it that it's easily runnable,
understandable, and cover positive and negative cases.

I will run it at least once. Perhaps, no need to run it on every kernel
build, but it's possible, since we run some LTP tests anyway, it'll
depend on how heavy the test is. Testers can run it too (if they wish).

You don't need to integrate it into kselftests, since we don't run it
anyway, and upstream will not run your tests either, so why bother?
Simple literate bash script is enough.

> vt@ suggested taking as an example kiosk test, but as far
> as I understand it does not use kernel test facility, instead it is
> written independently, and it poorly suitable for altha nosuid
> functionality. Besides, the kiost test does not run during build process
> (correct me if I'm wrong), so its presence does not make things any
> better.

Perhaps, more people should be able to understand what test does than
what LSM code does. So the test is supposed to increase confidence in
nosuid feature, and show there's no regressions anywhere in the future
we wish to be reassured.

The test will not "make things any better" if you absolutely confident
in flawlessness of your code.

Thanks,

> 
> AltHa code is almost stand-alone and does not change any part of the
> vanilla kernel. I think if it does not work as expected, users will
> report that (and I use it).
> 
> -- 
>    WBR,
>    Vladimir D. Seleznev
> _______________________________________________
> devel-kernel mailing list
> devel-kernel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel-kernel