selinux: new permission for chmod on suid or sgid files

This patch adds a new permission, setsuid, to the file class. This permission is checked any time a file has its attributes modified (setattr) and the setuid or setgid bits are involved. The purpose for this change is to further allow selinux to confine administrative duties. The example explains when the permission is needed and when it is not:

Start with a file with chmod 0644.

chmod u+s  (0644 -> 4644) { setattr setsuid }
chmod u-r  (4644 -> 4244) { setattr setsuid }
chmod u-s  (4244 -> 0244) { setattr setsuid }
chmod u+r  (0244 -> 0644) { setattr }

If either the starting or the final attributes will have the setuid or setgid bits set you need this permission.

Signed-off-by Eric Paris <[email protected]>

---

 security/selinux/hooks.c                     |   22 ++++++++++++++++++++--
 security/selinux/include/av_perm_to_string.h |    1 +
 security/selinux/include/av_permissions.h    |    1 +
 security/selinux/include/class_to_string.h   |    4 ++++
 security/selinux/include/security.h          |    2 ++
 security/selinux/selinuxfs.c                 |    3 ++-
 security/selinux/ss/services.c               |    3 +++
 7 files changed, 33 insertions(+), 3 deletions(-)

security/selinux/hooks.c +20 -2

index 576e511..58af86a 100644
@@ -2659,6 +2659,8 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	int rc;
+	unsigned int mode = dentry->d_inode->i_mode;
+	u32 av = 0;
 
 	rc = secondary_ops->inode_setattr(dentry, iattr);
 	if (rc)

@@ -2667,11 +2669,27 @@ static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 	if (iattr->ia_valid & ATTR_FORCE)
 		return 0;
 
+	/*
+	 * are we changing mode?
+	 * does policy support seperate suid/sgid checks?
+	 * is this a regular file?
+	 * do either the old inode->i_mode or the new iattr->i_mode have the
+	 * suid/sgid bits set?
+	 */
+	if ((iattr->ia_valid & ATTR_MODE) &&
+	    selinux_policycap_setsuidperm &&
+	    (S_ISREG(mode)) &&
+	    ((iattr->ia_mode & (S_ISUID | S_ISGID)) ||
+	     (mode & (S_ISUID | S_ISGID))))
+			av |= FILE__SETSUID;
+
 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
-		return dentry_has_perm(current, NULL, dentry, FILE__SETATTR);
+		av |= FILE__SETATTR;
+	else
+		av = FILE__WRITE;
 
-	return dentry_has_perm(current, NULL, dentry, FILE__WRITE);
+	return dentry_has_perm(current, NULL, dentry, av);
 }
 
 static int selinux_inode_getattr(struct vfsmount *mnt, struct dentry *dentry)

security/selinux/include/av_perm_to_string.h +1 -0

index 1223b4f..c6c5c0e 100644
@@ -19,6 +19,7 @@
    S_(SECCLASS_FILE, FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_FILE, FILE__EXECMOD, "execmod")
    S_(SECCLASS_FILE, FILE__OPEN, "open")
+   S_(SECCLASS_FILE, FILE__SETSUID, "setsuid")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECUTE_NO_TRANS, "execute_no_trans")
    S_(SECCLASS_CHR_FILE, CHR_FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECMOD, "execmod")

security/selinux/include/av_permissions.h +1 -0

index c4c5116..cd6d566 100644
@@ -101,6 +101,7 @@
 #define FILE__ENTRYPOINT                          0x00040000UL
 #define FILE__EXECMOD                             0x00080000UL
 #define FILE__OPEN                                0x00100000UL
+#define FILE__SETSUID                             0x00200000UL
 #define LNK_FILE__IOCTL                           0x00000001UL
 #define LNK_FILE__READ                            0x00000002UL
 #define LNK_FILE__WRITE                           0x00000004UL

security/selinux/include/class_to_string.h +4 -0

index bd813c3..0d11270 100644
@@ -72,3 +72,7 @@
     S_(NULL)
     S_("peer")
     S_("capability2")
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)

security/selinux/include/security.h +2 -0

index 7244737..a604f05 100644
@@ -56,12 +56,14 @@ extern int selinux_mls_enabled;
 enum {
 	POLICYDB_CAPABILITY_NETPEER,
 	POLICYDB_CAPABILITY_OPENPERM,
+	POLICYDB_CAPABILITY_SETSUIDPERM,
 	__POLICYDB_CAPABILITY_MAX
 };
 #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
 
 extern int selinux_policycap_netpeer;
 extern int selinux_policycap_openperm;
+extern int selinux_policycap_setsuidperm;
 
 /*
  * type_datum properties

security/selinux/selinuxfs.c +2 -1

index 69c9dcc..26ef62f 100644
@@ -42,7 +42,8 @@
 /* Policy capability filenames */
 static char *policycap_names[] = {
 	"network_peer_controls",
-	"open_perms"
+	"open_perms",
+	"setsuid_perms",
 };
 
 unsigned int selinux_checkreqprot = CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE;

security/selinux/ss/services.c +3 -0

index 343c8ab..9ecd8e7 100644
@@ -64,6 +64,7 @@ unsigned int policydb_loaded_version;
 
 int selinux_policycap_netpeer;
 int selinux_policycap_openperm;
+int selinux_policycap_setsuidperm;
 
 /*
  * This is declared in avc.c

@@ -1615,6 +1616,8 @@ static void security_load_policycaps(void)
 						  POLICYDB_CAPABILITY_NETPEER);
 	selinux_policycap_openperm = ebitmap_get_bit(&policydb.policycaps,
 						  POLICYDB_CAPABILITY_OPENPERM);
+	selinux_policycap_setsuidperm = ebitmap_get_bit(&policydb.policycaps,
+						  POLICYDB_CAPABILITY_SETSUIDPERM);
 }
 
 extern void selinux_complete_init(void);

Quoted 7 lines · Eric Paris

This patch adds a new permission, setsuid, to the file class. This permission is checked any time a file has its attributes modified (setattr) and the setuid or setgid bits are involved. The purpose for this change is to further allow selinux to confine administrative duties. The example explains when the permission is needed and when it is not:

Quoted 8 lines

Start with a file with chmod 0644.

chmod u+s  (0644 -> 4644) { setattr setsuid }
chmod u-r  (4644 -> 4244) { setattr setsuid }
chmod u-s  (4244 -> 0244) { setattr setsuid }
chmod u+r  (0244 -> 0644) { setattr }

If either the starting or the final attributes will have the setuid or setgid bits set you need this permission.

I'd like to understand better how this would be used.

Suppose that I want to control the ability to create/modify privileged executables on the system. Given that SELinux already controls capability usage, I can already do that by labeling privileged executable with an appropriate type and controlling the ability to create/modify/relabelto that type, without being concerned about the suid bit. Why do I need this check? And if I need this check, then don't I need a similar check on setting/clearing file capabilities on a given file?

Last concern I have is with this check not being adequately granular since it is a single check for setting or clearing the suid or sgid bit of a file owned by any user, whereas it seems more security-relevant to be setting the suid bit on a root-owned executable than to be clearing it or to be setting the suid bit on a sds-owned executable. I'm not sure how to write a useful policy that lets users do things that are harmless or of no interest while still enforcing a useful goal.

Quoted 148 lines

Signed-off-by Eric Paris <[email protected]>

---

 security/selinux/hooks.c                     |   22 ++++++++++++++++++++--
 security/selinux/include/av_perm_to_string.h |    1 +
 security/selinux/include/av_permissions.h    |    1 +
 security/selinux/include/class_to_string.h   |    4 ++++
 security/selinux/include/security.h          |    2 ++
 security/selinux/selinuxfs.c                 |    3 ++-
 security/selinux/ss/services.c               |    3 +++
 7 files changed, 33 insertions(+), 3 deletions(-)

security/selinux/hooks.c +20 -2

index 576e511..58af86a 100644
@@ -2659,6 +2659,8 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	int rc;
+	unsigned int mode = dentry->d_inode->i_mode;
+	u32 av = 0;
 
 	rc = secondary_ops->inode_setattr(dentry, iattr);
 	if (rc)

@@ -2667,11 +2669,27 @@ static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 	if (iattr->ia_valid & ATTR_FORCE)
 		return 0;
 
+	/*
+	 * are we changing mode?
+	 * does policy support seperate suid/sgid checks?
+	 * is this a regular file?
+	 * do either the old inode->i_mode or the new iattr->i_mode have the
+	 * suid/sgid bits set?
+	 */
+	if ((iattr->ia_valid & ATTR_MODE) &&
+	    selinux_policycap_setsuidperm &&
+	    (S_ISREG(mode)) &&
+	    ((iattr->ia_mode & (S_ISUID | S_ISGID)) ||
+	     (mode & (S_ISUID | S_ISGID))))
+			av |= FILE__SETSUID;
+
 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
-		return dentry_has_perm(current, NULL, dentry, FILE__SETATTR);
+		av |= FILE__SETATTR;
+	else
+		av = FILE__WRITE;
 
-	return dentry_has_perm(current, NULL, dentry, FILE__WRITE);
+	return dentry_has_perm(current, NULL, dentry, av);
 }
 
 static int selinux_inode_getattr(struct vfsmount *mnt, struct dentry *dentry)

security/selinux/include/av_perm_to_string.h +1 -0

index 1223b4f..c6c5c0e 100644
@@ -19,6 +19,7 @@
    S_(SECCLASS_FILE, FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_FILE, FILE__EXECMOD, "execmod")
    S_(SECCLASS_FILE, FILE__OPEN, "open")
+   S_(SECCLASS_FILE, FILE__SETSUID, "setsuid")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECUTE_NO_TRANS, "execute_no_trans")
    S_(SECCLASS_CHR_FILE, CHR_FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECMOD, "execmod")

security/selinux/include/av_permissions.h +1 -0

index c4c5116..cd6d566 100644
@@ -101,6 +101,7 @@
 #define FILE__ENTRYPOINT                          0x00040000UL
 #define FILE__EXECMOD                             0x00080000UL
 #define FILE__OPEN                                0x00100000UL
+#define FILE__SETSUID                             0x00200000UL
 #define LNK_FILE__IOCTL                           0x00000001UL
 #define LNK_FILE__READ                            0x00000002UL
 #define LNK_FILE__WRITE                           0x00000004UL

security/selinux/include/class_to_string.h +4 -0

index bd813c3..0d11270 100644
@@ -72,3 +72,7 @@
     S_(NULL)
     S_("peer")
     S_("capability2")
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)

security/selinux/include/security.h +2 -0

index 7244737..a604f05 100644
@@ -56,12 +56,14 @@ extern int selinux_mls_enabled;
 enum {
 	POLICYDB_CAPABILITY_NETPEER,
 	POLICYDB_CAPABILITY_OPENPERM,
+	POLICYDB_CAPABILITY_SETSUIDPERM,
 	__POLICYDB_CAPABILITY_MAX
 };
 #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
 
 extern int selinux_policycap_netpeer;
 extern int selinux_policycap_openperm;
+extern int selinux_policycap_setsuidperm;
 
 /*
  * type_datum properties

security/selinux/selinuxfs.c +2 -1

index 69c9dcc..26ef62f 100644
@@ -42,7 +42,8 @@
 /* Policy capability filenames */
 static char *policycap_names[] = {
 	"network_peer_controls",
-	"open_perms"
+	"open_perms",
+	"setsuid_perms",
 };
 
 unsigned int selinux_checkreqprot = CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE;

security/selinux/ss/services.c +3 -0

index 343c8ab..9ecd8e7 100644
@@ -64,6 +64,7 @@ unsigned int policydb_loaded_version;
 
 int selinux_policycap_netpeer;
 int selinux_policycap_openperm;
+int selinux_policycap_setsuidperm;
 
 /*
  * This is declared in avc.c

@@ -1615,6 +1616,8 @@ static void security_load_policycaps(void)
 						  POLICYDB_CAPABILITY_NETPEER);
 	selinux_policycap_openperm = ebitmap_get_bit(&policydb.policycaps,
 						  POLICYDB_CAPABILITY_OPENPERM);
+	selinux_policycap_setsuidperm = ebitmap_get_bit(&policydb.policycaps,
+						  POLICYDB_CAPABILITY_SETSUIDPERM);
 }
 
 extern void selinux_complete_init(void);

Quoted 18 lines · Stephen Smalley

Quoted 7 lines · Eric Paris

This patch adds a new permission, setsuid, to the file class. This permission is checked any time a file has its attributes modified (setattr) and the setuid or setgid bits are involved. The purpose for this change is to further allow selinux to confine administrative duties. The example explains when the permission is needed and when it is not:

Quoted 8 lines

Start with a file with chmod 0644.

chmod u+s  (0644 -> 4644) { setattr setsuid }
chmod u-r  (4644 -> 4244) { setattr setsuid }
chmod u-s  (4244 -> 0244) { setattr setsuid }
chmod u+r  (0244 -> 0644) { setattr }

If either the starting or the final attributes will have the setuid or setgid bits set you need this permission.

I'd like to understand better how this would be used.

Quoted 8 lines

Suppose that I want to control the ability to create/modify privileged executables on the system. Given that SELinux already controls capability usage, I can already do that by labeling privileged executable with an appropriate type and controlling the ability to create/modify/relabelto that type, without being concerned about the suid bit. Why do I need this check? And if I need this check, then don't I need a similar check on setting/clearing file capabilities on a given file?

One other tidbit: I don't believe that this check will get applied in the case where a suid/sgid binary is overwritten and the suid/sgid bits are forcibly cleared (ATTR_FORCE), as the selinux_inode_setattr() hook returns immediately in that case as it must not fail.

Quoted 153 lines

Last concern I have is with this check not being adequately granular since it is a single check for setting or clearing the suid or sgid bit of a file owned by any user, whereas it seems more security-relevant to be setting the suid bit on a root-owned executable than to be clearing it or to be setting the suid bit on a sds-owned executable. I'm not sure how to write a useful policy that lets users do things that are harmless or of no interest while still enforcing a useful goal.

Quoted 145 lines

Signed-off-by Eric Paris <[email protected]>

---

 security/selinux/hooks.c                     |   22 ++++++++++++++++++++--
 security/selinux/include/av_perm_to_string.h |    1 +
 security/selinux/include/av_permissions.h    |    1 +
 security/selinux/include/class_to_string.h   |    4 ++++
 security/selinux/include/security.h          |    2 ++
 security/selinux/selinuxfs.c                 |    3 ++-
 security/selinux/ss/services.c               |    3 +++
 7 files changed, 33 insertions(+), 3 deletions(-)

security/selinux/hooks.c +20 -2

index 576e511..58af86a 100644
@@ -2659,6 +2659,8 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	int rc;
+	unsigned int mode = dentry->d_inode->i_mode;
+	u32 av = 0;
 
 	rc = secondary_ops->inode_setattr(dentry, iattr);
 	if (rc)

@@ -2667,11 +2669,27 @@ static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 	if (iattr->ia_valid & ATTR_FORCE)
 		return 0;
 
+	/*
+	 * are we changing mode?
+	 * does policy support seperate suid/sgid checks?
+	 * is this a regular file?
+	 * do either the old inode->i_mode or the new iattr->i_mode have the
+	 * suid/sgid bits set?
+	 */
+	if ((iattr->ia_valid & ATTR_MODE) &&
+	    selinux_policycap_setsuidperm &&
+	    (S_ISREG(mode)) &&
+	    ((iattr->ia_mode & (S_ISUID | S_ISGID)) ||
+	     (mode & (S_ISUID | S_ISGID))))
+			av |= FILE__SETSUID;
+
 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
-		return dentry_has_perm(current, NULL, dentry, FILE__SETATTR);
+		av |= FILE__SETATTR;
+	else
+		av = FILE__WRITE;
 
-	return dentry_has_perm(current, NULL, dentry, FILE__WRITE);
+	return dentry_has_perm(current, NULL, dentry, av);
 }
 
 static int selinux_inode_getattr(struct vfsmount *mnt, struct dentry *dentry)

security/selinux/include/av_perm_to_string.h +1 -0

index 1223b4f..c6c5c0e 100644
@@ -19,6 +19,7 @@
    S_(SECCLASS_FILE, FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_FILE, FILE__EXECMOD, "execmod")
    S_(SECCLASS_FILE, FILE__OPEN, "open")
+   S_(SECCLASS_FILE, FILE__SETSUID, "setsuid")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECUTE_NO_TRANS, "execute_no_trans")
    S_(SECCLASS_CHR_FILE, CHR_FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECMOD, "execmod")

security/selinux/include/av_permissions.h +1 -0

index c4c5116..cd6d566 100644
@@ -101,6 +101,7 @@
 #define FILE__ENTRYPOINT                          0x00040000UL
 #define FILE__EXECMOD                             0x00080000UL
 #define FILE__OPEN                                0x00100000UL
+#define FILE__SETSUID                             0x00200000UL
 #define LNK_FILE__IOCTL                           0x00000001UL
 #define LNK_FILE__READ                            0x00000002UL
 #define LNK_FILE__WRITE                           0x00000004UL

security/selinux/include/class_to_string.h +4 -0

index bd813c3..0d11270 100644
@@ -72,3 +72,7 @@
     S_(NULL)
     S_("peer")
     S_("capability2")
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)

security/selinux/include/security.h +2 -0

index 7244737..a604f05 100644
@@ -56,12 +56,14 @@ extern int selinux_mls_enabled;
 enum {
 	POLICYDB_CAPABILITY_NETPEER,
 	POLICYDB_CAPABILITY_OPENPERM,
+	POLICYDB_CAPABILITY_SETSUIDPERM,
 	__POLICYDB_CAPABILITY_MAX
 };
 #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
 
 extern int selinux_policycap_netpeer;
 extern int selinux_policycap_openperm;
+extern int selinux_policycap_setsuidperm;
 
 /*
  * type_datum properties

security/selinux/selinuxfs.c +2 -1

index 69c9dcc..26ef62f 100644
@@ -42,7 +42,8 @@
 /* Policy capability filenames */
 static char *policycap_names[] = {
 	"network_peer_controls",
-	"open_perms"
+	"open_perms",
+	"setsuid_perms",
 };
 
 unsigned int selinux_checkreqprot = CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE;

security/selinux/ss/services.c +3 -0

index 343c8ab..9ecd8e7 100644
@@ -64,6 +64,7 @@ unsigned int policydb_loaded_version;
 
 int selinux_policycap_netpeer;
 int selinux_policycap_openperm;
+int selinux_policycap_setsuidperm;
 
 /*
  * This is declared in avc.c

@@ -1615,6 +1616,8 @@ static void security_load_policycaps(void)
 						  POLICYDB_CAPABILITY_NETPEER);
 	selinux_policycap_openperm = ebitmap_get_bit(&policydb.policycaps,
 						  POLICYDB_CAPABILITY_OPENPERM);
+	selinux_policycap_setsuidperm = ebitmap_get_bit(&policydb.policycaps,
+						  POLICYDB_CAPABILITY_SETSUIDPERM);
 }
 

Quoted 2 lines

security/selinux/ss/services.c static void security_load_policycaps(void) · line 1623

Quoted 2 lines

security/selinux/ss/services.c +0 -0

 extern void selinux_complete_init(void);

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 186 lines · Stephen Smalley

Quoted 18 lines · Stephen Smalley

Quoted 7 lines · Eric Paris

This patch adds a new permission, setsuid, to the file class. This permission is checked any time a file has its attributes modified (setattr) and the setuid or setgid bits are involved. The purpose for this change is to further allow selinux to confine administrative duties. The example explains when the permission is needed and when it is not:

Quoted 8 lines

Start with a file with chmod 0644.

chmod u+s  (0644 -> 4644) { setattr setsuid }
chmod u-r  (4644 -> 4244) { setattr setsuid }
chmod u-s  (4244 -> 0244) { setattr setsuid }
chmod u+r  (0244 -> 0644) { setattr }

If either the starting or the final attributes will have the setuid or setgid bits set you need this permission.

I'd like to understand better how this would be used.

Quoted 8 lines

Suppose that I want to control the ability to create/modify privileged executables on the system. Given that SELinux already controls capability usage, I can already do that by labeling privileged executable with an appropriate type and controlling the ability to create/modify/relabelto that type, without being concerned about the suid bit. Why do I need this check? And if I need this check, then don't I need a similar check on setting/clearing file capabilities on a given file?

One other tidbit: I don't believe that this check will get applied in the case where a suid/sgid binary is overwritten and the suid/sgid bits are forcibly cleared (ATTR_FORCE), as the selinux_inode_setattr() hook returns immediately in that case as it must not fail.

Quoted 153 lines

Last concern I have is with this check not being adequately granular since it is a single check for setting or clearing the suid or sgid bit of a file owned by any user, whereas it seems more security-relevant to be setting the suid bit on a root-owned executable than to be clearing it or to be setting the suid bit on a sds-owned executable. I'm not sure how to write a useful policy that lets users do things that are harmless or of no interest while still enforcing a useful goal.

Quoted 145 lines

Signed-off-by Eric Paris <[email protected]>

---

 security/selinux/hooks.c                     |   22 ++++++++++++++++++++--
 security/selinux/include/av_perm_to_string.h |    1 +
 security/selinux/include/av_permissions.h    |    1 +
 security/selinux/include/class_to_string.h   |    4 ++++
 security/selinux/include/security.h          |    2 ++
 security/selinux/selinuxfs.c                 |    3 ++-
 security/selinux/ss/services.c               |    3 +++
 7 files changed, 33 insertions(+), 3 deletions(-)

security/selinux/hooks.c +20 -2

index 576e511..58af86a 100644
@@ -2659,6 +2659,8 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	int rc;
+	unsigned int mode = dentry->d_inode->i_mode;
+	u32 av = 0;
 
 	rc = secondary_ops->inode_setattr(dentry, iattr);
 	if (rc)

@@ -2667,11 +2669,27 @@ static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 	if (iattr->ia_valid & ATTR_FORCE)
 		return 0;
 
+	/*
+	 * are we changing mode?
+	 * does policy support seperate suid/sgid checks?
+	 * is this a regular file?
+	 * do either the old inode->i_mode or the new iattr->i_mode have the
+	 * suid/sgid bits set?
+	 */
+	if ((iattr->ia_valid & ATTR_MODE) &&
+	    selinux_policycap_setsuidperm &&
+	    (S_ISREG(mode)) &&
+	    ((iattr->ia_mode & (S_ISUID | S_ISGID)) ||
+	     (mode & (S_ISUID | S_ISGID))))
+			av |= FILE__SETSUID;
+
 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
-		return dentry_has_perm(current, NULL, dentry, FILE__SETATTR);
+		av |= FILE__SETATTR;
+	else
+		av = FILE__WRITE;
 
-	return dentry_has_perm(current, NULL, dentry, FILE__WRITE);
+	return dentry_has_perm(current, NULL, dentry, av);
 }
 
 static int selinux_inode_getattr(struct vfsmount *mnt, struct dentry *dentry)

security/selinux/include/av_perm_to_string.h +1 -0

index 1223b4f..c6c5c0e 100644
@@ -19,6 +19,7 @@
    S_(SECCLASS_FILE, FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_FILE, FILE__EXECMOD, "execmod")
    S_(SECCLASS_FILE, FILE__OPEN, "open")
+   S_(SECCLASS_FILE, FILE__SETSUID, "setsuid")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECUTE_NO_TRANS, "execute_no_trans")
    S_(SECCLASS_CHR_FILE, CHR_FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECMOD, "execmod")

security/selinux/include/av_permissions.h +1 -0

index c4c5116..cd6d566 100644
@@ -101,6 +101,7 @@
 #define FILE__ENTRYPOINT                          0x00040000UL
 #define FILE__EXECMOD                             0x00080000UL
 #define FILE__OPEN                                0x00100000UL
+#define FILE__SETSUID                             0x00200000UL
 #define LNK_FILE__IOCTL                           0x00000001UL
 #define LNK_FILE__READ                            0x00000002UL
 #define LNK_FILE__WRITE                           0x00000004UL

security/selinux/include/class_to_string.h +4 -0

index bd813c3..0d11270 100644
@@ -72,3 +72,7 @@
     S_(NULL)
     S_("peer")
     S_("capability2")
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)

security/selinux/include/security.h +2 -0

index 7244737..a604f05 100644
@@ -56,12 +56,14 @@ extern int selinux_mls_enabled;
 enum {
 	POLICYDB_CAPABILITY_NETPEER,
 	POLICYDB_CAPABILITY_OPENPERM,
+	POLICYDB_CAPABILITY_SETSUIDPERM,
 	__POLICYDB_CAPABILITY_MAX
 };
 #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
 
 extern int selinux_policycap_netpeer;
 extern int selinux_policycap_openperm;
+extern int selinux_policycap_setsuidperm;
 
 /*
  * type_datum properties

security/selinux/selinuxfs.c +2 -1

index 69c9dcc..26ef62f 100644
@@ -42,7 +42,8 @@
 /* Policy capability filenames */
 static char *policycap_names[] = {
 	"network_peer_controls",
-	"open_perms"
+	"open_perms",
+	"setsuid_perms",
 };
 
 unsigned int selinux_checkreqprot = CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE;

security/selinux/ss/services.c +3 -0

index 343c8ab..9ecd8e7 100644
@@ -64,6 +64,7 @@ unsigned int policydb_loaded_version;
 
 int selinux_policycap_netpeer;
 int selinux_policycap_openperm;
+int selinux_policycap_setsuidperm;
 
 /*
  * This is declared in avc.c

@@ -1615,6 +1616,8 @@ static void security_load_policycaps(void)
 						  POLICYDB_CAPABILITY_NETPEER);
 	selinux_policycap_openperm = ebitmap_get_bit(&policydb.policycaps,
 						  POLICYDB_CAPABILITY_OPENPERM);
+	selinux_policycap_setsuidperm = ebitmap_get_bit(&policydb.policycaps,
+						  POLICYDB_CAPABILITY_SETSUIDPERM);
 }
 

Quoted 2 lines

Quoted 2 lines

security/selinux/ss/services.c static void security_load_policycaps(void) · line 1623

Quoted 2 lines

security/selinux/ss/services.c +0 -0

 extern void selinux_complete_init(void);

The reason I asked for this is for the confined administrator user.

Say I am webadmin of a system. I can run sudo to become webadm_r:webadmin_t as UID 0. I now want to change the permissions on a cgi script to be executable. If I can change it to setuid, or setgid, I might be able to find an alternate root to executing the app with raised privs.

I just see this as a risk, and I see little reason why a confined administrator would even need to setuid/setgid an application.

Quoted 193 lines · Daniel J Walsh

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 185 lines · Stephen Smalley

Quoted 18 lines · Stephen Smalley

Quoted 7 lines · Eric Paris

This patch adds a new permission, setsuid, to the file class. This permission is checked any time a file has its attributes modified (setattr) and the setuid or setgid bits are involved. The purpose for this change is to further allow selinux to confine administrative duties. The example explains when the permission is needed and when it is not:

Quoted 8 lines

Start with a file with chmod 0644.

chmod u+s  (0644 -> 4644) { setattr setsuid }
chmod u-r  (4644 -> 4244) { setattr setsuid }
chmod u-s  (4244 -> 0244) { setattr setsuid }
chmod u+r  (0244 -> 0644) { setattr }

If either the starting or the final attributes will have the setuid or setgid bits set you need this permission.

I'd like to understand better how this would be used.

Quoted 8 lines

Suppose that I want to control the ability to create/modify privileged executables on the system. Given that SELinux already controls capability usage, I can already do that by labeling privileged executable with an appropriate type and controlling the ability to create/modify/relabelto that type, without being concerned about the suid bit. Why do I need this check? And if I need this check, then don't I need a similar check on setting/clearing file capabilities on a given file?

One other tidbit: I don't believe that this check will get applied in the case where a suid/sgid binary is overwritten and the suid/sgid bits are forcibly cleared (ATTR_FORCE), as the selinux_inode_setattr() hook returns immediately in that case as it must not fail.

Quoted 153 lines

Last concern I have is with this check not being adequately granular since it is a single check for setting or clearing the suid or sgid bit of a file owned by any user, whereas it seems more security-relevant to be setting the suid bit on a root-owned executable than to be clearing it or to be setting the suid bit on a sds-owned executable. I'm not sure how to write a useful policy that lets users do things that are harmless or of no interest while still enforcing a useful goal.

Quoted 145 lines

Signed-off-by Eric Paris <[email protected]>

---

 security/selinux/hooks.c                     |   22 ++++++++++++++++++++--
 security/selinux/include/av_perm_to_string.h |    1 +
 security/selinux/include/av_permissions.h    |    1 +
 security/selinux/include/class_to_string.h   |    4 ++++
 security/selinux/include/security.h          |    2 ++
 security/selinux/selinuxfs.c                 |    3 ++-
 security/selinux/ss/services.c               |    3 +++
 7 files changed, 33 insertions(+), 3 deletions(-)

security/selinux/hooks.c +20 -2

index 576e511..58af86a 100644
@@ -2659,6 +2659,8 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	int rc;
+	unsigned int mode = dentry->d_inode->i_mode;
+	u32 av = 0;
 
 	rc = secondary_ops->inode_setattr(dentry, iattr);
 	if (rc)

@@ -2667,11 +2669,27 @@ static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 	if (iattr->ia_valid & ATTR_FORCE)
 		return 0;
 
+	/*
+	 * are we changing mode?
+	 * does policy support seperate suid/sgid checks?
+	 * is this a regular file?
+	 * do either the old inode->i_mode or the new iattr->i_mode have the
+	 * suid/sgid bits set?
+	 */
+	if ((iattr->ia_valid & ATTR_MODE) &&
+	    selinux_policycap_setsuidperm &&
+	    (S_ISREG(mode)) &&
+	    ((iattr->ia_mode & (S_ISUID | S_ISGID)) ||
+	     (mode & (S_ISUID | S_ISGID))))
+			av |= FILE__SETSUID;
+
 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
-		return dentry_has_perm(current, NULL, dentry, FILE__SETATTR);
+		av |= FILE__SETATTR;
+	else
+		av = FILE__WRITE;
 
-	return dentry_has_perm(current, NULL, dentry, FILE__WRITE);
+	return dentry_has_perm(current, NULL, dentry, av);
 }
 
 static int selinux_inode_getattr(struct vfsmount *mnt, struct dentry *dentry)

security/selinux/include/av_perm_to_string.h +1 -0

index 1223b4f..c6c5c0e 100644
@@ -19,6 +19,7 @@
    S_(SECCLASS_FILE, FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_FILE, FILE__EXECMOD, "execmod")
    S_(SECCLASS_FILE, FILE__OPEN, "open")
+   S_(SECCLASS_FILE, FILE__SETSUID, "setsuid")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECUTE_NO_TRANS, "execute_no_trans")
    S_(SECCLASS_CHR_FILE, CHR_FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECMOD, "execmod")

security/selinux/include/av_permissions.h +1 -0

index c4c5116..cd6d566 100644
@@ -101,6 +101,7 @@
 #define FILE__ENTRYPOINT                          0x00040000UL
 #define FILE__EXECMOD                             0x00080000UL
 #define FILE__OPEN                                0x00100000UL
+#define FILE__SETSUID                             0x00200000UL
 #define LNK_FILE__IOCTL                           0x00000001UL
 #define LNK_FILE__READ                            0x00000002UL
 #define LNK_FILE__WRITE                           0x00000004UL

security/selinux/include/class_to_string.h +4 -0

index bd813c3..0d11270 100644
@@ -72,3 +72,7 @@
     S_(NULL)
     S_("peer")
     S_("capability2")
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)

security/selinux/include/security.h +2 -0

index 7244737..a604f05 100644
@@ -56,12 +56,14 @@ extern int selinux_mls_enabled;
 enum {
 	POLICYDB_CAPABILITY_NETPEER,
 	POLICYDB_CAPABILITY_OPENPERM,
+	POLICYDB_CAPABILITY_SETSUIDPERM,
 	__POLICYDB_CAPABILITY_MAX
 };
 #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
 
 extern int selinux_policycap_netpeer;
 extern int selinux_policycap_openperm;
+extern int selinux_policycap_setsuidperm;
 
 /*
  * type_datum properties

security/selinux/selinuxfs.c +2 -1

index 69c9dcc..26ef62f 100644
@@ -42,7 +42,8 @@
 /* Policy capability filenames */
 static char *policycap_names[] = {
 	"network_peer_controls",
-	"open_perms"
+	"open_perms",
+	"setsuid_perms",
 };
 
 unsigned int selinux_checkreqprot = CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE;

security/selinux/ss/services.c +3 -0

index 343c8ab..9ecd8e7 100644
@@ -64,6 +64,7 @@ unsigned int policydb_loaded_version;
 
 int selinux_policycap_netpeer;
 int selinux_policycap_openperm;
+int selinux_policycap_setsuidperm;
 
 /*
  * This is declared in avc.c

@@ -1615,6 +1616,8 @@ static void security_load_policycaps(void)
 						  POLICYDB_CAPABILITY_NETPEER);
 	selinux_policycap_openperm = ebitmap_get_bit(&policydb.policycaps,
 						  POLICYDB_CAPABILITY_OPENPERM);
+	selinux_policycap_setsuidperm = ebitmap_get_bit(&policydb.policycaps,
+						  POLICYDB_CAPABILITY_SETSUIDPERM);
 }
 

Quoted 2 lines

Quoted 2 lines

security/selinux/ss/services.c static void security_load_policycaps(void) · line 1623

Quoted 2 lines

security/selinux/ss/services.c +0 -0

 extern void selinux_complete_init(void);

The reason I asked for this is for the confined administrator user.

Quoted 8 lines

Say I am webadmin of a system. I can run sudo to become webadm_r:webadmin_t as UID 0. I now want to change the permissions on a cgi script to be executable. If I can change it to setuid, or setgid, I might be able to find an alternate root to executing the app with raised privs.

I just see this as a risk, and I see little reason why a confined administrator would even need to setuid/setgid an application.

Well, as I noted when this came up as an RFC a year ago, the fact that they can create a setuid program doesn't mean that they can exercise any capabilities since we control that based on domain. And rather than preventing them from setting the suid bit, it might be more useful to prevent them from executing a setuid program (regardless of who made it setuid) or letting it execute but not changing its uid in that case (just as we do under ptrace).

http://marc.info/?l=selinux&m=119013530120434&w=2

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 211 lines · Stephen Smalley

Quoted 193 lines · Daniel J Walsh

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 185 lines · Stephen Smalley

Quoted 18 lines · Stephen Smalley

Quoted 7 lines · Eric Paris

This patch adds a new permission, setsuid, to the file class. This permission is checked any time a file has its attributes modified (setattr) and the setuid or setgid bits are involved. The purpose for this change is to further allow selinux to confine administrative duties. The example explains when the permission is needed and when it is not:

Quoted 8 lines

Start with a file with chmod 0644.

chmod u+s  (0644 -> 4644) { setattr setsuid }
chmod u-r  (4644 -> 4244) { setattr setsuid }
chmod u-s  (4244 -> 0244) { setattr setsuid }
chmod u+r  (0244 -> 0644) { setattr }

If either the starting or the final attributes will have the setuid or setgid bits set you need this permission.

I'd like to understand better how this would be used.

Quoted 8 lines

Suppose that I want to control the ability to create/modify privileged executables on the system. Given that SELinux already controls capability usage, I can already do that by labeling privileged executable with an appropriate type and controlling the ability to create/modify/relabelto that type, without being concerned about the suid bit. Why do I need this check? And if I need this check, then don't I need a similar check on setting/clearing file capabilities on a given file?

One other tidbit: I don't believe that this check will get applied in the case where a suid/sgid binary is overwritten and the suid/sgid bits are forcibly cleared (ATTR_FORCE), as the selinux_inode_setattr() hook returns immediately in that case as it must not fail.

Quoted 153 lines

Last concern I have is with this check not being adequately granular since it is a single check for setting or clearing the suid or sgid bit of a file owned by any user, whereas it seems more security-relevant to be setting the suid bit on a root-owned executable than to be clearing it or to be setting the suid bit on a sds-owned executable. I'm not sure how to write a useful policy that lets users do things that are harmless or of no interest while still enforcing a useful goal.

Quoted 145 lines

Signed-off-by Eric Paris <[email protected]>

---

 security/selinux/hooks.c                     |   22 ++++++++++++++++++++--
 security/selinux/include/av_perm_to_string.h |    1 +
 security/selinux/include/av_permissions.h    |    1 +
 security/selinux/include/class_to_string.h   |    4 ++++
 security/selinux/include/security.h          |    2 ++
 security/selinux/selinuxfs.c                 |    3 ++-
 security/selinux/ss/services.c               |    3 +++
 7 files changed, 33 insertions(+), 3 deletions(-)

security/selinux/hooks.c +20 -2

index 576e511..58af86a 100644
@@ -2659,6 +2659,8 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	int rc;
+	unsigned int mode = dentry->d_inode->i_mode;
+	u32 av = 0;
 
 	rc = secondary_ops->inode_setattr(dentry, iattr);
 	if (rc)

@@ -2667,11 +2669,27 @@ static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 	if (iattr->ia_valid & ATTR_FORCE)
 		return 0;
 
+	/*
+	 * are we changing mode?
+	 * does policy support seperate suid/sgid checks?
+	 * is this a regular file?
+	 * do either the old inode->i_mode or the new iattr->i_mode have the
+	 * suid/sgid bits set?
+	 */
+	if ((iattr->ia_valid & ATTR_MODE) &&
+	    selinux_policycap_setsuidperm &&
+	    (S_ISREG(mode)) &&
+	    ((iattr->ia_mode & (S_ISUID | S_ISGID)) ||
+	     (mode & (S_ISUID | S_ISGID))))
+			av |= FILE__SETSUID;
+
 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
-		return dentry_has_perm(current, NULL, dentry, FILE__SETATTR);
+		av |= FILE__SETATTR;
+	else
+		av = FILE__WRITE;
 
-	return dentry_has_perm(current, NULL, dentry, FILE__WRITE);
+	return dentry_has_perm(current, NULL, dentry, av);
 }
 
 static int selinux_inode_getattr(struct vfsmount *mnt, struct dentry *dentry)

security/selinux/include/av_perm_to_string.h +1 -0

index 1223b4f..c6c5c0e 100644
@@ -19,6 +19,7 @@
    S_(SECCLASS_FILE, FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_FILE, FILE__EXECMOD, "execmod")
    S_(SECCLASS_FILE, FILE__OPEN, "open")
+   S_(SECCLASS_FILE, FILE__SETSUID, "setsuid")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECUTE_NO_TRANS, "execute_no_trans")
    S_(SECCLASS_CHR_FILE, CHR_FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECMOD, "execmod")

security/selinux/include/av_permissions.h +1 -0

index c4c5116..cd6d566 100644
@@ -101,6 +101,7 @@
 #define FILE__ENTRYPOINT                          0x00040000UL
 #define FILE__EXECMOD                             0x00080000UL
 #define FILE__OPEN                                0x00100000UL
+#define FILE__SETSUID                             0x00200000UL
 #define LNK_FILE__IOCTL                           0x00000001UL
 #define LNK_FILE__READ                            0x00000002UL
 #define LNK_FILE__WRITE                           0x00000004UL

security/selinux/include/class_to_string.h +4 -0

index bd813c3..0d11270 100644
@@ -72,3 +72,7 @@
     S_(NULL)
     S_("peer")
     S_("capability2")
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)

security/selinux/include/security.h +2 -0

index 7244737..a604f05 100644
@@ -56,12 +56,14 @@ extern int selinux_mls_enabled;
 enum {
 	POLICYDB_CAPABILITY_NETPEER,
 	POLICYDB_CAPABILITY_OPENPERM,
+	POLICYDB_CAPABILITY_SETSUIDPERM,
 	__POLICYDB_CAPABILITY_MAX
 };
 #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
 
 extern int selinux_policycap_netpeer;
 extern int selinux_policycap_openperm;
+extern int selinux_policycap_setsuidperm;
 
 /*
  * type_datum properties

security/selinux/selinuxfs.c +2 -1

index 69c9dcc..26ef62f 100644
@@ -42,7 +42,8 @@
 /* Policy capability filenames */
 static char *policycap_names[] = {
 	"network_peer_controls",
-	"open_perms"
+	"open_perms",
+	"setsuid_perms",
 };
 
 unsigned int selinux_checkreqprot = CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE;

security/selinux/ss/services.c +3 -0

index 343c8ab..9ecd8e7 100644
@@ -64,6 +64,7 @@ unsigned int policydb_loaded_version;
 
 int selinux_policycap_netpeer;
 int selinux_policycap_openperm;
+int selinux_policycap_setsuidperm;
 
 /*
  * This is declared in avc.c

@@ -1615,6 +1616,8 @@ static void security_load_policycaps(void)
 						  POLICYDB_CAPABILITY_NETPEER);
 	selinux_policycap_openperm = ebitmap_get_bit(&policydb.policycaps,
 						  POLICYDB_CAPABILITY_OPENPERM);
+	selinux_policycap_setsuidperm = ebitmap_get_bit(&policydb.policycaps,
+						  POLICYDB_CAPABILITY_SETSUIDPERM);
 }
 

Quoted 2 lines

Quoted 2 lines

security/selinux/ss/services.c static void security_load_policycaps(void) · line 1623

Quoted 2 lines

security/selinux/ss/services.c +0 -0

 extern void selinux_complete_init(void);

The reason I asked for this is for the confined administrator user.

Quoted 8 lines

Say I am webadmin of a system. I can run sudo to become webadm_r:webadmin_t as UID 0. I now want to change the permissions on a cgi script to be executable. If I can change it to setuid, or setgid, I might be able to find an alternate root to executing the app with raised privs.

I just see this as a risk, and I see little reason why a confined administrator would even need to setuid/setgid an application.

Well, as I noted when this came up as an RFC a year ago, the fact that they can create a setuid program doesn't mean that they can exercise any capabilities since we control that based on domain. And rather than preventing them from setting the suid bit, it might be more useful to prevent them from executing a setuid program (regardless of who made it setuid) or letting it execute but not changing its uid in that case (just as we do under ptrace).

Quoted 2 lines

http://marc.info/?l=selinux&m=119013530120434&w=2

But I see the problem not with webadm_t executing the setuid app, but potentially another domain that he can trigger. Say apache or samba or ftp. If one of these apps has "setuid" capability they could execute the setuid app and potentially escalate privs.

sesearch --allow | grep setuid | wc
WARNING: This policy contained disabled aliases; they have been removed.

    197    3947   33797

197 domains have the ability to setuid, if the bad admin figures a way to get one of these confined domains to run his application, he might be able to do something evil.

I don't follow the above. First, the CAP_SETUID capability controls the ability to use set*uid() system calls, not to execute setuid binaries (aside from a special case for shared state). Second, if some other confined domain executes a setuid binary created by this user, it is still limited by the permissions granted to that original confined domain as far as SELinux is concerned.

Quoted 6 lines · Stephen Smalley

I don't follow the above. First, the CAP_SETUID capability controls the ability to use set*uid() system calls, not to execute setuid binaries (aside from a special case for shared state). Second, if some other confined domain executes a setuid binary created by this user, it is still limited by the permissions granted to that original confined domain as far as SELinux is concerned.

He's saying there are almost 200 domains that can run setuid apps.

Limiting the number of domains that can create new setuid apps limits the number of places that these domains can go. Clearly they are all still confined to their domain and whatever it allows, but allowing them to gain root priveledges may give them the ability to attack other parts

of the system normally controlled by dac. This certainly doesn't lessen the MAC confinement. Lets assume an audited system in which we are certain the only suid app untrusted users are allowed to run is ping. So the users have the right to run suid apps. They are protected from each other by DAC.

Confined webadmin writes a program which is clears out other users public_html when they get a spurious DMCA takedown notice. He then (because he is a lazy bumbling idiot) makes his script suid so he does not have to go into his confined webadmin account constantly to delete users webpages.

Normal DAC protects user2 from being attacked by user1. Because of the bumbling incompetance of confined webadmin user1 can now use this setuid app to do things which he is allow by selinux policy but denied by normal DAC permissions.

Why did webadmin need to make a setuid app to begin with? file caps are already protected by CAP_SETFCAP. Lets assume system policy says that su should not be setuid. Should the webadmin really be allowed to easily override that system policy because he wants to use su - to get to his confined domain instead of sudo?

She bumbling idiot really be
allowed to say add o+x to su - when the system policy only really wanted group wheel to be able to run su? Should the bumbling idiot be able to remove the suid flag from a program and not be able to fix it?

I think there are some real gains that can be made by limiting how confined admins or untrusted users can deal with suid apps.

I'd probably be inclined to add changing the uid/gid/other things that clear setuid to be things which require this permission. I don't see a reason to allow my webadmin to chown su to himself...

Quoted 6 lines · Eric Paris

of the system normally controlled by dac. This certainly doesn't lessen the MAC confinement. Lets assume an audited system in which we are certain the only suid app untrusted users are allowed to run is ping. So the users have the right to run suid apps. They are protected from each other by DAC.

Quoted 5 lines

Confined webadmin writes a program which is clears out other users public_html when they get a spurious DMCA takedown notice. He then (because he is a lazy bumbling idiot) makes his script suid so he does not have to go into his confined webadmin account constantly to delete users webpages.

He could also make a daemon that runs under his uid and accepts commands via local socket.

Quoted 5 lines

Normal DAC protects user2 from being attacked by user1. Because of the bumbling incompetance of confined webadmin user1 can now use this setuid app to do things which he is allow by selinux policy but denied by normal DAC permissions.

Quoted 5 lines

Why did webadmin need to make a setuid app to begin with? file caps are already protected by CAP_SETFCAP. Lets assume system policy says that su should not be setuid. Should the webadmin really be allowed to easily override that system policy because he wants to use su - to get to his confined domain instead of sudo?

He can't get to his confined domain via some other program unless policy says he can. He can only get to some other uid that way.

Quoted 8 lines

She bumbling idiot really be
allowed to say add o+x to su - when the system policy only really wanted group wheel to be able to run su? Should the bumbling idiot be able to remove the suid flag from a program and not be able to fix it?

I think there are some real gains that can be made by limiting how confined admins or untrusted users can deal with suid apps.

Quoted 3 lines

I'd probably be inclined to add changing the uid/gid/other things that clear setuid to be things which require this permission. I don't see a reason to allow my webadmin to chown su to himself...

He can't do that unless he owns su (or that itself is subject to fowner). And chown'ing or writing to a suid app clears the suid bit forcibly.

Come on, guys, you can do better.

policy that I'm talking about is !!NOT!!! mathmatically provable and that's the part that I'm sure you initially reject, but it doesn't make it pointless or even detractive.

Anyway, I'm not arguing this is going to provide greater MAC confinement and I'm not able to describe security goals in terms of MAC domains. I'm merely stating that a SELinux confined admin is easily able change DAC security policy as defined by an organization.

Yes, DAC sercurity
policy that I'm talking about is mathmatically provable and that's the part that I'm sure you initially reject, but it doesn't make it pointless or even detractive.

Quoted 9 lines · Eric Paris

Quoted 6 lines · Stephen Smalley

I don't follow the above. First, the CAP_SETUID capability controls the ability to use set*uid() system calls, not to execute setuid binaries (aside from a special case for shared state). Second, if some other confined domain executes a setuid binary created by this user, it is still limited by the permissions granted to that original confined domain as far as SELinux is concerned.

He's saying there are almost 200 domains that can run setuid apps.

As I said, CAP_SETUID isn't about whether or not you can run a setuid app. You can do that without CAP_SETUID. Obviously since a normal unprivileged user shell can run a setuid program in the first place.

Quoted 4 lines

Limiting the number of domains that can create new setuid apps limits the number of places that these domains can go. Clearly they are all still confined to their domain and whatever it allows, but allowing them to gain root priveledges may give them the ability to attack other parts

Wait - how did they gain root privileges? root privileges are capabilities, and we control capabilities based on domain.

Quoted 11 lines

of the system normally controlled by dac. This certainly doesn't lessen the MAC confinement. Lets assume an audited system in which we are certain the only suid app untrusted users are allowed to run is ping. So the users have the right to run suid apps. They are protected from each other by DAC.

Confined webadmin writes a program which is clears out other users public_html when they get a spurious DMCA takedown notice. He then (because he is a lazy bumbling idiot) makes his script suid so he does not have to go into his confined webadmin account constantly to delete users webpages.

He could also make a daemon that runs under his uid and accepts commands via local socket.

Quoted 10 lines

Normal DAC protects user2 from being attacked by user1. Because of the bumbling incompetance of confined webadmin user1 can now use this setuid app to do things which he is allow by selinux policy but denied by normal DAC permissions.

Why did webadmin need to make a setuid app to begin with? file caps are already protected by CAP_SETFCAP. Lets assume system policy says that su should not be setuid. Should the webadmin really be allowed to easily override that system policy because he wants to use su - to get to his confined domain instead of sudo?

He can't get to his confined domain via some other program unless policy says he can. He can only get to some other uid that way.

Quoted 11 lines

She bumbling idiot really be
allowed to say add o+x to su - when the system policy only really wanted group wheel to be able to run su? Should the bumbling idiot be able to remove the suid flag from a program and not be able to fix it?

I think there are some real gains that can be made by limiting how confined admins or untrusted users can deal with suid apps.

I'd probably be inclined to add changing the uid/gid/other things that clear setuid to be things which require this permission. I don't see a reason to allow my webadmin to chown su to himself...

He can't do that unless he owns su (or that itself is subject to fowner). And chown'ing or writing to a suid app clears the suid bit forcibly.

Come on, guys, you can do better.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 64 lines · Stephen Smalley

Quoted 9 lines · Eric Paris

Quoted 6 lines · Stephen Smalley

I don't follow the above. First, the CAP_SETUID capability controls the ability to use set*uid() system calls, not to execute setuid binaries (aside from a special case for shared state). Second, if some other confined domain executes a setuid binary created by this user, it is still limited by the permissions granted to that original confined domain as far as SELinux is concerned.

He's saying there are almost 200 domains that can run setuid apps.

As I said, CAP_SETUID isn't about whether or not you can run a setuid app. You can do that without CAP_SETUID. Obviously since a normal unprivileged user shell can run a setuid program in the first place.

Quoted 4 lines

Limiting the number of domains that can create new setuid apps limits the number of places that these domains can go. Clearly they are all still confined to their domain and whatever it allows, but allowing them to gain root priveledges may give them the ability to attack other parts

Wait - how did they gain root privileges? root privileges are capabilities, and we control capabilities based on domain.

Quoted 11 lines

of the system normally controlled by dac. This certainly doesn't lessen the MAC confinement. Lets assume an audited system in which we are certain the only suid app untrusted users are allowed to run is ping. So the users have the right to run suid apps. They are protected from each other by DAC.

Confined webadmin writes a program which is clears out other users public_html when they get a spurious DMCA takedown notice. He then (because he is a lazy bumbling idiot) makes his script suid so he does not have to go into his confined webadmin account constantly to delete users webpages.

He could also make a daemon that runs under his uid and accepts commands via local socket.

Quoted 10 lines

Normal DAC protects user2 from being attacked by user1. Because of the bumbling incompetance of confined webadmin user1 can now use this setuid app to do things which he is allow by selinux policy but denied by normal DAC permissions.

Why did webadmin need to make a setuid app to begin with? file caps are already protected by CAP_SETFCAP. Lets assume system policy says that su should not be setuid. Should the webadmin really be allowed to easily override that system policy because he wants to use su - to get to his confined domain instead of sudo?

He can't get to his confined domain via some other program unless policy says he can. He can only get to some other uid that way.

Quoted 11 lines

She bumbling idiot really be
allowed to say add o+x to su - when the system policy only really wanted group wheel to be able to run su? Should the bumbling idiot be able to remove the suid flag from a program and not be able to fix it?

I think there are some real gains that can be made by limiting how confined admins or untrusted users can deal with suid apps.

I'd probably be inclined to add changing the uid/gid/other things that clear setuid to be things which require this permission. I don't see a reason to allow my webadmin to chown su to himself...

Quoted 6 lines

He can't do that unless he owns su (or that itself is subject to fowner). And chown'ing or writing to a suid app clears the suid bit forcibly.

Come on, guys, you can do better.

This hole discussion has actually opened me up to a new understanding. SELinux right now does not prevent the execution of setuid applications if you do not have setuid capability, it only prevents you from running apps that actually execute the setuid() call.

I think this is a problem. One of the things I have been saying in my presentations is that running as staff_t will prevent you from running any setuid application unless a transition is defined. Turns out this is wrong and I was lying.

# cp /usr/bin/id /usr/bin/myid
# chmod 4755 /usr/bin/myid

$ myid
uid=3267(dwalsh) gid=3267(dwalsh) euid=0(root) groups=3267(dwalsh)
context=staff_u:staff_r:staff_t:s0

So running a chmod o+s file as staff_t will run as EUID 0. I think this is something SELinux should block. At least treat this the same way a file system mounted nosetuid would.

Quoted 86 lines · Daniel J Walsh

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 56 lines · Stephen Smalley

Quoted 9 lines · Eric Paris

Quoted 6 lines · Stephen Smalley

I don't follow the above. First, the CAP_SETUID capability controls the ability to use set*uid() system calls, not to execute setuid binaries (aside from a special case for shared state). Second, if some other confined domain executes a setuid binary created by this user, it is still limited by the permissions granted to that original confined domain as far as SELinux is concerned.

He's saying there are almost 200 domains that can run setuid apps.

As I said, CAP_SETUID isn't about whether or not you can run a setuid app. You can do that without CAP_SETUID. Obviously since a normal unprivileged user shell can run a setuid program in the first place.

Quoted 4 lines

Limiting the number of domains that can create new setuid apps limits the number of places that these domains can go. Clearly they are all still confined to their domain and whatever it allows, but allowing them to gain root priveledges may give them the ability to attack other parts

Wait - how did they gain root privileges? root privileges are capabilities, and we control capabilities based on domain.

Quoted 11 lines

of the system normally controlled by dac. This certainly doesn't lessen the MAC confinement. Lets assume an audited system in which we are certain the only suid app untrusted users are allowed to run is ping. So the users have the right to run suid apps. They are protected from each other by DAC.

Confined webadmin writes a program which is clears out other users public_html when they get a spurious DMCA takedown notice. He then (because he is a lazy bumbling idiot) makes his script suid so he does not have to go into his confined webadmin account constantly to delete users webpages.

He could also make a daemon that runs under his uid and accepts commands via local socket.

Quoted 10 lines

Normal DAC protects user2 from being attacked by user1. Because of the bumbling incompetance of confined webadmin user1 can now use this setuid app to do things which he is allow by selinux policy but denied by normal DAC permissions.

Why did webadmin need to make a setuid app to begin with? file caps are already protected by CAP_SETFCAP. Lets assume system policy says that su should not be setuid. Should the webadmin really be allowed to easily override that system policy because he wants to use su - to get to his confined domain instead of sudo?

He can't get to his confined domain via some other program unless policy says he can. He can only get to some other uid that way.

Quoted 8 lines

She bumbling idiot really be
allowed to say add o+x to su - when the system policy only really wanted group wheel to be able to run su? Should the bumbling idiot be able to remove the suid flag from a program and not be able to fix it?

I think there are some real gains that can be made by limiting how confined admins or untrusted users can deal with suid apps.

Quoted 9 lines

Quoted 3 lines

I'd probably be inclined to add changing the uid/gid/other things that clear setuid to be things which require this permission. I don't see a reason to allow my webadmin to chown su to himself...

He can't do that unless he owns su (or that itself is subject to fowner). And chown'ing or writing to a suid app clears the suid bit forcibly.

Come on, guys, you can do better.

This hole discussion has actually opened me up to a new understanding. SELinux right now does not prevent the execution of setuid applications if you do not have setuid capability, it only prevents you from running apps that actually execute the setuid() call.

I think this is a problem. One of the things I have been saying in my presentations is that running as staff_t will prevent you from running any setuid application unless a transition is defined. Turns out this is wrong and I was lying.

# cp /usr/bin/id /usr/bin/myid
# chmod 4755 /usr/bin/myid

$ myid
uid=3267(dwalsh) gid=3267(dwalsh) euid=0(root) groups=3267(dwalsh)
context=staff_u:staff_r:staff_t:s0

Quoted 3 lines

So running a chmod o+s file as staff_t will run as EUID 0. I think this is something SELinux should block. At least treat this the same way a file system mounted nosetuid would.

That would require a control on the exec path instead of chmod/setattr - which is what I suggested a year ago in response to the original RFC.

But note that gaining EUID 0 does not automatically grant privilege under SELinux - your capabilities are still limited based on domain and your ability to read/write even root-owned files is controlled based on the (domain, type, file class) triple.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 87 lines · Stephen Smalley

Quoted 86 lines · Daniel J Walsh

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 56 lines · Stephen Smalley

Quoted 9 lines · Eric Paris

Quoted 6 lines · Stephen Smalley

I don't follow the above. First, the CAP_SETUID capability controls the ability to use set*uid() system calls, not to execute setuid binaries (aside from a special case for shared state). Second, if some other confined domain executes a setuid binary created by this user, it is still limited by the permissions granted to that original confined domain as far as SELinux is concerned.

He's saying there are almost 200 domains that can run setuid apps.

As I said, CAP_SETUID isn't about whether or not you can run a setuid app. You can do that without CAP_SETUID. Obviously since a normal unprivileged user shell can run a setuid program in the first place.

Quoted 4 lines

Limiting the number of domains that can create new setuid apps limits the number of places that these domains can go. Clearly they are all still confined to their domain and whatever it allows, but allowing them to gain root priveledges may give them the ability to attack other parts

Wait - how did they gain root privileges? root privileges are capabilities, and we control capabilities based on domain.

Quoted 11 lines

of the system normally controlled by dac. This certainly doesn't lessen the MAC confinement. Lets assume an audited system in which we are certain the only suid app untrusted users are allowed to run is ping. So the users have the right to run suid apps. They are protected from each other by DAC.

Confined webadmin writes a program which is clears out other users public_html when they get a spurious DMCA takedown notice. He then (because he is a lazy bumbling idiot) makes his script suid so he does not have to go into his confined webadmin account constantly to delete users webpages.

He could also make a daemon that runs under his uid and accepts commands via local socket.

Quoted 10 lines

Normal DAC protects user2 from being attacked by user1. Because of the bumbling incompetance of confined webadmin user1 can now use this setuid app to do things which he is allow by selinux policy but denied by normal DAC permissions.

Why did webadmin need to make a setuid app to begin with? file caps are already protected by CAP_SETFCAP. Lets assume system policy says that su should not be setuid. Should the webadmin really be allowed to easily override that system policy because he wants to use su - to get to his confined domain instead of sudo?

He can't get to his confined domain via some other program unless policy says he can. He can only get to some other uid that way.

Quoted 8 lines

She bumbling idiot really be
allowed to say add o+x to su - when the system policy only really wanted group wheel to be able to run su? Should the bumbling idiot be able to remove the suid flag from a program and not be able to fix it?

I think there are some real gains that can be made by limiting how confined admins or untrusted users can deal with suid apps.

Quoted 9 lines

Quoted 3 lines

I'd probably be inclined to add changing the uid/gid/other things that clear setuid to be things which require this permission. I don't see a reason to allow my webadmin to chown su to himself...

He can't do that unless he owns su (or that itself is subject to fowner). And chown'ing or writing to a suid app clears the suid bit forcibly.

Come on, guys, you can do better.

This hole discussion has actually opened me up to a new understanding. SELinux right now does not prevent the execution of setuid applications if you do not have setuid capability, it only prevents you from running apps that actually execute the setuid() call.

I think this is a problem. One of the things I have been saying in my presentations is that running as staff_t will prevent you from running any setuid application unless a transition is defined. Turns out this is wrong and I was lying.

# cp /usr/bin/id /usr/bin/myid
# chmod 4755 /usr/bin/myid

$ myid
uid=3267(dwalsh) gid=3267(dwalsh) euid=0(root) groups=3267(dwalsh)
context=staff_u:staff_r:staff_t:s0

Quoted 7 lines

Quoted 3 lines

So running a chmod o+s file as staff_t will run as EUID 0. I think this is something SELinux should block. At least treat this the same way a file system mounted nosetuid would.

That would require a control on the exec path instead of chmod/setattr - which is what I suggested a year ago in response to the original RFC.

Sounds like a good idea, I guess you should have pushed it. :^)

Quoted 5 lines

But note that gaining EUID 0 does not automatically grant privilege under SELinux - your capabilities are still limited based on domain and your ability to read/write even root-owned files is controlled based on the (domain, type, file class) triple.

Understood. But we still have the risc of having a fairly wide open user type like staff_t tripping on a setuid app that could exculate privs. Or the ability for two staff_t users to attach each other through setuid apps.

Quoted 106 lines · Daniel J Walsh

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 87 lines · Stephen Smalley

Quoted 86 lines · Daniel J Walsh

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 56 lines · Stephen Smalley

Quoted 9 lines · Eric Paris

Quoted 6 lines · Stephen Smalley

I don't follow the above. First, the CAP_SETUID capability controls the ability to use set*uid() system calls, not to execute setuid binaries (aside from a special case for shared state). Second, if some other confined domain executes a setuid binary created by this user, it is still limited by the permissions granted to that original confined domain as far as SELinux is concerned.

He's saying there are almost 200 domains that can run setuid apps.

As I said, CAP_SETUID isn't about whether or not you can run a setuid app. You can do that without CAP_SETUID. Obviously since a normal unprivileged user shell can run a setuid program in the first place.

Quoted 4 lines

Limiting the number of domains that can create new setuid apps limits the number of places that these domains can go. Clearly they are all still confined to their domain and whatever it allows, but allowing them to gain root priveledges may give them the ability to attack other parts

Wait - how did they gain root privileges? root privileges are capabilities, and we control capabilities based on domain.

Quoted 11 lines

of the system normally controlled by dac. This certainly doesn't lessen the MAC confinement. Lets assume an audited system in which we are certain the only suid app untrusted users are allowed to run is ping. So the users have the right to run suid apps. They are protected from each other by DAC.

Confined webadmin writes a program which is clears out other users public_html when they get a spurious DMCA takedown notice. He then (because he is a lazy bumbling idiot) makes his script suid so he does not have to go into his confined webadmin account constantly to delete users webpages.

He could also make a daemon that runs under his uid and accepts commands via local socket.

Quoted 10 lines

Normal DAC protects user2 from being attacked by user1. Because of the bumbling incompetance of confined webadmin user1 can now use this setuid app to do things which he is allow by selinux policy but denied by normal DAC permissions.

Why did webadmin need to make a setuid app to begin with? file caps are already protected by CAP_SETFCAP. Lets assume system policy says that su should not be setuid. Should the webadmin really be allowed to easily override that system policy because he wants to use su - to get to his confined domain instead of sudo?

He can't get to his confined domain via some other program unless policy says he can. He can only get to some other uid that way.

Quoted 8 lines

She bumbling idiot really be
allowed to say add o+x to su - when the system policy only really wanted group wheel to be able to run su? Should the bumbling idiot be able to remove the suid flag from a program and not be able to fix it?

I think there are some real gains that can be made by limiting how confined admins or untrusted users can deal with suid apps.

Quoted 9 lines

Quoted 3 lines

I'd probably be inclined to add changing the uid/gid/other things that clear setuid to be things which require this permission. I don't see a reason to allow my webadmin to chown su to himself...

He can't do that unless he owns su (or that itself is subject to fowner). And chown'ing or writing to a suid app clears the suid bit forcibly.

Come on, guys, you can do better.

This hole discussion has actually opened me up to a new understanding. SELinux right now does not prevent the execution of setuid applications if you do not have setuid capability, it only prevents you from running apps that actually execute the setuid() call.

I think this is a problem. One of the things I have been saying in my presentations is that running as staff_t will prevent you from running any setuid application unless a transition is defined. Turns out this is wrong and I was lying.

# cp /usr/bin/id /usr/bin/myid
# chmod 4755 /usr/bin/myid

$ myid
uid=3267(dwalsh) gid=3267(dwalsh) euid=0(root) groups=3267(dwalsh)
context=staff_u:staff_r:staff_t:s0

Quoted 7 lines

Quoted 3 lines

So running a chmod o+s file as staff_t will run as EUID 0. I think this is something SELinux should block. At least treat this the same way a file system mounted nosetuid would.

That would require a control on the exec path instead of chmod/setattr - which is what I suggested a year ago in response to the original RFC.

Sounds like a good idea, I guess you should have pushed it. :^)

Quoted 5 lines

But note that gaining EUID 0 does not automatically grant privilege under SELinux - your capabilities are still limited based on domain and your ability to read/write even root-owned files is controlled based on the (domain, type, file class) triple.

Quoted 4 lines

Understood. But we still have the risc of having a fairly wide open user type like staff_t tripping on a setuid app that could exculate privs. Or the ability for two staff_t users to attach each other through setuid apps.

Ok, then I think the path forward is:
- a setsuid file permission check only applied when setting suid/sgid in selinux_inode_setattr (and maybe when adding wider execute access, although that's harder if you want to take ACLs into account),

- a new process permission check in selinux_bprm_set_security() when bprm->e_uid != current->euid or bprm->e_gid != current->egid or ! cap_issubset(bprm->cap_post_exec_permitted, current->cap_permitted).

Quoted 117 lines · Stephen Smalley

Quoted 106 lines · Daniel J Walsh

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 87 lines · Stephen Smalley

Quoted 86 lines · Daniel J Walsh

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 56 lines · Stephen Smalley

Quoted 9 lines · Eric Paris

Quoted 6 lines · Stephen Smalley

I don't follow the above. First, the CAP_SETUID capability controls the ability to use set*uid() system calls, not to execute setuid binaries (aside from a special case for shared state). Second, if some other confined domain executes a setuid binary created by this user, it is still limited by the permissions granted to that original confined domain as far as SELinux is concerned.

He's saying there are almost 200 domains that can run setuid apps.

As I said, CAP_SETUID isn't about whether or not you can run a setuid app. You can do that without CAP_SETUID. Obviously since a normal unprivileged user shell can run a setuid program in the first place.

Quoted 4 lines

Limiting the number of domains that can create new setuid apps limits the number of places that these domains can go. Clearly they are all still confined to their domain and whatever it allows, but allowing them to gain root priveledges may give them the ability to attack other parts

Wait - how did they gain root privileges? root privileges are capabilities, and we control capabilities based on domain.

Quoted 11 lines

of the system normally controlled by dac. This certainly doesn't lessen the MAC confinement. Lets assume an audited system in which we are certain the only suid app untrusted users are allowed to run is ping. So the users have the right to run suid apps. They are protected from each other by DAC.

Confined webadmin writes a program which is clears out other users public_html when they get a spurious DMCA takedown notice. He then (because he is a lazy bumbling idiot) makes his script suid so he does not have to go into his confined webadmin account constantly to delete users webpages.

He could also make a daemon that runs under his uid and accepts commands via local socket.

Quoted 10 lines

Normal DAC protects user2 from being attacked by user1. Because of the bumbling incompetance of confined webadmin user1 can now use this setuid app to do things which he is allow by selinux policy but denied by normal DAC permissions.

Why did webadmin need to make a setuid app to begin with? file caps are already protected by CAP_SETFCAP. Lets assume system policy says that su should not be setuid. Should the webadmin really be allowed to easily override that system policy because he wants to use su - to get to his confined domain instead of sudo?

He can't get to his confined domain via some other program unless policy says he can. He can only get to some other uid that way.

Quoted 8 lines

She bumbling idiot really be
allowed to say add o+x to su - when the system policy only really wanted group wheel to be able to run su? Should the bumbling idiot be able to remove the suid flag from a program and not be able to fix it?

I think there are some real gains that can be made by limiting how confined admins or untrusted users can deal with suid apps.

Quoted 9 lines

Quoted 3 lines

I'd probably be inclined to add changing the uid/gid/other things that clear setuid to be things which require this permission. I don't see a reason to allow my webadmin to chown su to himself...

He can't do that unless he owns su (or that itself is subject to fowner). And chown'ing or writing to a suid app clears the suid bit forcibly.

Come on, guys, you can do better.

This hole discussion has actually opened me up to a new understanding. SELinux right now does not prevent the execution of setuid applications if you do not have setuid capability, it only prevents you from running apps that actually execute the setuid() call.

I think this is a problem. One of the things I have been saying in my presentations is that running as staff_t will prevent you from running any setuid application unless a transition is defined. Turns out this is wrong and I was lying.

# cp /usr/bin/id /usr/bin/myid
# chmod 4755 /usr/bin/myid

$ myid
uid=3267(dwalsh) gid=3267(dwalsh) euid=0(root) groups=3267(dwalsh)
context=staff_u:staff_r:staff_t:s0

Quoted 7 lines

Quoted 3 lines

So running a chmod o+s file as staff_t will run as EUID 0. I think this is something SELinux should block. At least treat this the same way a file system mounted nosetuid would.

That would require a control on the exec path instead of chmod/setattr - which is what I suggested a year ago in response to the original RFC.

Sounds like a good idea, I guess you should have pushed it. :^)

Quoted 5 lines

But note that gaining EUID 0 does not automatically grant privilege under SELinux - your capabilities are still limited based on domain and your ability to read/write even root-owned files is controlled based on the (domain, type, file class) triple.

Quoted 4 lines

Understood. But we still have the risc of having a fairly wide open user type like staff_t tripping on a setuid app that could exculate privs. Or the ability for two staff_t users to attach each other through setuid apps.

Ok, then I think the path forward is:
- a setsuid file permission check only applied when setting suid/sgid in selinux_inode_setattr (and maybe when adding wider execute access, although that's harder if you want to take ACLs into account),

Quoted 3 lines

- a new process permission check in selinux_bprm_set_security() when bprm->e_uid != current->euid or bprm->e_gid != current->egid or ! cap_issubset(bprm->cap_post_exec_permitted, current->cap_permitted).

I'll take a look. At that point is current->cap_permitted still that of the original execve process or has it already been intersected with
cap_inheritable.... /me has spent all day looking at how to audit
fcaps and I realized I got that wrong the first time...

Quoted 122 lines · Eric Paris

Quoted 120 lines · Stephen Smalley

Quoted 106 lines · Daniel J Walsh

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 87 lines · Stephen Smalley

Quoted 86 lines · Daniel J Walsh

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 56 lines · Stephen Smalley

Quoted 9 lines · Eric Paris

Quoted 6 lines · Stephen Smalley

I don't follow the above. First, the CAP_SETUID capability controls the ability to use set*uid() system calls, not to execute setuid binaries (aside from a special case for shared state). Second, if some other confined domain executes a setuid binary created by this user, it is still limited by the permissions granted to that original confined domain as far as SELinux is concerned.

He's saying there are almost 200 domains that can run setuid apps.

As I said, CAP_SETUID isn't about whether or not you can run a setuid app. You can do that without CAP_SETUID. Obviously since a normal unprivileged user shell can run a setuid program in the first place.

Quoted 4 lines

Limiting the number of domains that can create new setuid apps limits the number of places that these domains can go. Clearly they are all still confined to their domain and whatever it allows, but allowing them to gain root priveledges may give them the ability to attack other parts

Wait - how did they gain root privileges? root privileges are capabilities, and we control capabilities based on domain.

Quoted 11 lines

of the system normally controlled by dac. This certainly doesn't lessen the MAC confinement. Lets assume an audited system in which we are certain the only suid app untrusted users are allowed to run is ping. So the users have the right to run suid apps. They are protected from each other by DAC.

Confined webadmin writes a program which is clears out other users public_html when they get a spurious DMCA takedown notice. He then (because he is a lazy bumbling idiot) makes his script suid so he does not have to go into his confined webadmin account constantly to delete users webpages.

He could also make a daemon that runs under his uid and accepts commands via local socket.

Quoted 10 lines

Normal DAC protects user2 from being attacked by user1. Because of the bumbling incompetance of confined webadmin user1 can now use this setuid app to do things which he is allow by selinux policy but denied by normal DAC permissions.

Why did webadmin need to make a setuid app to begin with? file caps are already protected by CAP_SETFCAP. Lets assume system policy says that su should not be setuid. Should the webadmin really be allowed to easily override that system policy because he wants to use su - to get to his confined domain instead of sudo?

He can't get to his confined domain via some other program unless policy says he can. He can only get to some other uid that way.

Quoted 8 lines

She bumbling idiot really be
allowed to say add o+x to su - when the system policy only really wanted group wheel to be able to run su? Should the bumbling idiot be able to remove the suid flag from a program and not be able to fix it?

I think there are some real gains that can be made by limiting how confined admins or untrusted users can deal with suid apps.

Quoted 9 lines

Quoted 3 lines

I'd probably be inclined to add changing the uid/gid/other things that clear setuid to be things which require this permission. I don't see a reason to allow my webadmin to chown su to himself...

He can't do that unless he owns su (or that itself is subject to fowner). And chown'ing or writing to a suid app clears the suid bit forcibly.

Come on, guys, you can do better.

This hole discussion has actually opened me up to a new understanding. SELinux right now does not prevent the execution of setuid applications if you do not have setuid capability, it only prevents you from running apps that actually execute the setuid() call.

I think this is a problem. One of the things I have been saying in my presentations is that running as staff_t will prevent you from running any setuid application unless a transition is defined. Turns out this is wrong and I was lying.

# cp /usr/bin/id /usr/bin/myid
# chmod 4755 /usr/bin/myid

$ myid
uid=3267(dwalsh) gid=3267(dwalsh) euid=0(root) groups=3267(dwalsh)
context=staff_u:staff_r:staff_t:s0

Quoted 7 lines

Quoted 3 lines

So running a chmod o+s file as staff_t will run as EUID 0. I think this is something SELinux should block. At least treat this the same way a file system mounted nosetuid would.

That would require a control on the exec path instead of chmod/setattr - which is what I suggested a year ago in response to the original RFC.

Sounds like a good idea, I guess you should have pushed it. :^)

Quoted 5 lines

But note that gaining EUID 0 does not automatically grant privilege under SELinux - your capabilities are still limited based on domain and your ability to read/write even root-owned files is controlled based on the (domain, type, file class) triple.

Quoted 4 lines

Understood. But we still have the risc of having a fairly wide open user type like staff_t tripping on a setuid app that could exculate privs. Or the ability for two staff_t users to attach each other through setuid apps.

Ok, then I think the path forward is:
- a setsuid file permission check only applied when setting suid/sgid in selinux_inode_setattr (and maybe when adding wider execute access, although that's harder if you want to take ACLs into account),

- a new process permission check in selinux_bprm_set_security() when bprm->e_uid != current->euid or bprm->e_gid != current->egid or ! cap_issubset(bprm->cap_post_exec_permitted, current->cap_permitted).

Quoted 4 lines

I'll take a look. At that point is current->cap_permitted still that of the original execve process or has it already been intersected with
cap_inheritable.... /me has spent all day looking at how to audit
fcaps and I realized I got that wrong the first time...

In selinux_bprm_set_security(), after the call to the secondary_ops hook, we should have the new permitted set in brpm->cap_effective while

current's credentials are still those of the caller (unchanged) - the changes are not applied until bprm_apply_creds. The inheritable mask is factored into the computation of bprm->cap_post_exec_permitted during cap_bprm_set_security.

Quoted 193 lines · Daniel J Walsh

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 185 lines · Stephen Smalley

Quoted 18 lines · Stephen Smalley

Quoted 7 lines · Eric Paris

This patch adds a new permission, setsuid, to the file class. This permission is checked any time a file has its attributes modified (setattr) and the setuid or setgid bits are involved. The purpose for this change is to further allow selinux to confine administrative duties. The example explains when the permission is needed and when it is not:

Quoted 8 lines

Start with a file with chmod 0644.

chmod u+s  (0644 -> 4644) { setattr setsuid }
chmod u-r  (4644 -> 4244) { setattr setsuid }
chmod u-s  (4244 -> 0244) { setattr setsuid }
chmod u+r  (0244 -> 0644) { setattr }

If either the starting or the final attributes will have the setuid or setgid bits set you need this permission.

I'd like to understand better how this would be used.

Quoted 8 lines

Suppose that I want to control the ability to create/modify privileged executables on the system. Given that SELinux already controls capability usage, I can already do that by labeling privileged executable with an appropriate type and controlling the ability to create/modify/relabelto that type, without being concerned about the suid bit. Why do I need this check? And if I need this check, then don't I need a similar check on setting/clearing file capabilities on a given file?

One other tidbit: I don't believe that this check will get applied in the case where a suid/sgid binary is overwritten and the suid/sgid bits are forcibly cleared (ATTR_FORCE), as the selinux_inode_setattr() hook returns immediately in that case as it must not fail.

Quoted 153 lines

Last concern I have is with this check not being adequately granular since it is a single check for setting or clearing the suid or sgid bit of a file owned by any user, whereas it seems more security-relevant to be setting the suid bit on a root-owned executable than to be clearing it or to be setting the suid bit on a sds-owned executable. I'm not sure how to write a useful policy that lets users do things that are harmless or of no interest while still enforcing a useful goal.

Quoted 145 lines

Signed-off-by Eric Paris <[email protected]>

---

 security/selinux/hooks.c                     |   22 ++++++++++++++++++++--
 security/selinux/include/av_perm_to_string.h |    1 +
 security/selinux/include/av_permissions.h    |    1 +
 security/selinux/include/class_to_string.h   |    4 ++++
 security/selinux/include/security.h          |    2 ++
 security/selinux/selinuxfs.c                 |    3 ++-
 security/selinux/ss/services.c               |    3 +++
 7 files changed, 33 insertions(+), 3 deletions(-)

security/selinux/hooks.c +20 -2

index 576e511..58af86a 100644
@@ -2659,6 +2659,8 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	int rc;
+	unsigned int mode = dentry->d_inode->i_mode;
+	u32 av = 0;
 
 	rc = secondary_ops->inode_setattr(dentry, iattr);
 	if (rc)

@@ -2667,11 +2669,27 @@ static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 	if (iattr->ia_valid & ATTR_FORCE)
 		return 0;
 
+	/*
+	 * are we changing mode?
+	 * does policy support seperate suid/sgid checks?
+	 * is this a regular file?
+	 * do either the old inode->i_mode or the new iattr->i_mode have the
+	 * suid/sgid bits set?
+	 */
+	if ((iattr->ia_valid & ATTR_MODE) &&
+	    selinux_policycap_setsuidperm &&
+	    (S_ISREG(mode)) &&
+	    ((iattr->ia_mode & (S_ISUID | S_ISGID)) ||
+	     (mode & (S_ISUID | S_ISGID))))
+			av |= FILE__SETSUID;
+
 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
-		return dentry_has_perm(current, NULL, dentry, FILE__SETATTR);
+		av |= FILE__SETATTR;
+	else
+		av = FILE__WRITE;
 
-	return dentry_has_perm(current, NULL, dentry, FILE__WRITE);
+	return dentry_has_perm(current, NULL, dentry, av);
 }
 
 static int selinux_inode_getattr(struct vfsmount *mnt, struct dentry *dentry)

security/selinux/include/av_perm_to_string.h +1 -0

index 1223b4f..c6c5c0e 100644
@@ -19,6 +19,7 @@
    S_(SECCLASS_FILE, FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_FILE, FILE__EXECMOD, "execmod")
    S_(SECCLASS_FILE, FILE__OPEN, "open")
+   S_(SECCLASS_FILE, FILE__SETSUID, "setsuid")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECUTE_NO_TRANS, "execute_no_trans")
    S_(SECCLASS_CHR_FILE, CHR_FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECMOD, "execmod")

security/selinux/include/av_permissions.h +1 -0

index c4c5116..cd6d566 100644
@@ -101,6 +101,7 @@
 #define FILE__ENTRYPOINT                          0x00040000UL
 #define FILE__EXECMOD                             0x00080000UL
 #define FILE__OPEN                                0x00100000UL
+#define FILE__SETSUID                             0x00200000UL
 #define LNK_FILE__IOCTL                           0x00000001UL
 #define LNK_FILE__READ                            0x00000002UL
 #define LNK_FILE__WRITE                           0x00000004UL

security/selinux/include/class_to_string.h +4 -0

index bd813c3..0d11270 100644
@@ -72,3 +72,7 @@
     S_(NULL)
     S_("peer")
     S_("capability2")
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)

security/selinux/include/security.h +2 -0

index 7244737..a604f05 100644
@@ -56,12 +56,14 @@ extern int selinux_mls_enabled;
 enum {
 	POLICYDB_CAPABILITY_NETPEER,
 	POLICYDB_CAPABILITY_OPENPERM,
+	POLICYDB_CAPABILITY_SETSUIDPERM,
 	__POLICYDB_CAPABILITY_MAX
 };
 #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
 
 extern int selinux_policycap_netpeer;
 extern int selinux_policycap_openperm;
+extern int selinux_policycap_setsuidperm;
 
 /*
  * type_datum properties

security/selinux/selinuxfs.c +2 -1

index 69c9dcc..26ef62f 100644
@@ -42,7 +42,8 @@
 /* Policy capability filenames */
 static char *policycap_names[] = {
 	"network_peer_controls",
-	"open_perms"
+	"open_perms",
+	"setsuid_perms",
 };
 
 unsigned int selinux_checkreqprot = CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE;

security/selinux/ss/services.c +3 -0

index 343c8ab..9ecd8e7 100644
@@ -64,6 +64,7 @@ unsigned int policydb_loaded_version;
 
 int selinux_policycap_netpeer;
 int selinux_policycap_openperm;
+int selinux_policycap_setsuidperm;
 
 /*
  * This is declared in avc.c

@@ -1615,6 +1616,8 @@ static void security_load_policycaps(void)
 						  POLICYDB_CAPABILITY_NETPEER);
 	selinux_policycap_openperm = ebitmap_get_bit(&policydb.policycaps,
 						  POLICYDB_CAPABILITY_OPENPERM);
+	selinux_policycap_setsuidperm = ebitmap_get_bit(&policydb.policycaps,
+						  POLICYDB_CAPABILITY_SETSUIDPERM);
 }
 

Quoted 2 lines

Quoted 2 lines

security/selinux/ss/services.c static void security_load_policycaps(void) · line 1623

Quoted 2 lines

security/selinux/ss/services.c +0 -0

 extern void selinux_complete_init(void);

The reason I asked for this is for the confined administrator user.

Quoted 8 lines

Say I am webadmin of a system. I can run sudo to become webadm_r:webadmin_t as UID 0. I now want to change the permissions on a cgi script to be executable. If I can change it to setuid, or setgid, I might be able to find an alternate root to executing the app with raised privs.

I just see this as a risk, and I see little reason why a confined administrator would even need to setuid/setgid an application.

Well, as I noted when this came up as an RFC a year ago, the fact that they can create a setuid program doesn't mean that they can exercise any capabilities since we control that based on domain. And rather than preventing them from setting the suid bit, it might be more useful to prevent them from executing a setuid program (regardless of who made it setuid) or letting it execute but not changing its uid in that case (just as we do under ptrace).

http://marc.info/?l=selinux&m=119013530120434&w=2

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 211 lines · Stephen Smalley

Quoted 193 lines · Daniel J Walsh

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 185 lines · Stephen Smalley

Quoted 18 lines · Stephen Smalley

Quoted 7 lines · Eric Paris

This patch adds a new permission, setsuid, to the file class. This permission is checked any time a file has its attributes modified (setattr) and the setuid or setgid bits are involved. The purpose for this change is to further allow selinux to confine administrative duties. The example explains when the permission is needed and when it is not:

Quoted 8 lines

Start with a file with chmod 0644.

chmod u+s  (0644 -> 4644) { setattr setsuid }
chmod u-r  (4644 -> 4244) { setattr setsuid }
chmod u-s  (4244 -> 0244) { setattr setsuid }
chmod u+r  (0244 -> 0644) { setattr }

If either the starting or the final attributes will have the setuid or setgid bits set you need this permission.

I'd like to understand better how this would be used.

Quoted 8 lines

Suppose that I want to control the ability to create/modify privileged executables on the system. Given that SELinux already controls capability usage, I can already do that by labeling privileged executable with an appropriate type and controlling the ability to create/modify/relabelto that type, without being concerned about the suid bit. Why do I need this check? And if I need this check, then don't I need a similar check on setting/clearing file capabilities on a given file?

One other tidbit: I don't believe that this check will get applied in the case where a suid/sgid binary is overwritten and the suid/sgid bits are forcibly cleared (ATTR_FORCE), as the selinux_inode_setattr() hook returns immediately in that case as it must not fail.

Quoted 153 lines

Last concern I have is with this check not being adequately granular since it is a single check for setting or clearing the suid or sgid bit of a file owned by any user, whereas it seems more security-relevant to be setting the suid bit on a root-owned executable than to be clearing it or to be setting the suid bit on a sds-owned executable. I'm not sure how to write a useful policy that lets users do things that are harmless or of no interest while still enforcing a useful goal.

Quoted 145 lines

Signed-off-by Eric Paris <[email protected]>

---

 security/selinux/hooks.c                     |   22 ++++++++++++++++++++--
 security/selinux/include/av_perm_to_string.h |    1 +
 security/selinux/include/av_permissions.h    |    1 +
 security/selinux/include/class_to_string.h   |    4 ++++
 security/selinux/include/security.h          |    2 ++
 security/selinux/selinuxfs.c                 |    3 ++-
 security/selinux/ss/services.c               |    3 +++
 7 files changed, 33 insertions(+), 3 deletions(-)

security/selinux/hooks.c +20 -2

index 576e511..58af86a 100644
@@ -2659,6 +2659,8 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	int rc;
+	unsigned int mode = dentry->d_inode->i_mode;
+	u32 av = 0;
 
 	rc = secondary_ops->inode_setattr(dentry, iattr);
 	if (rc)

@@ -2667,11 +2669,27 @@ static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 	if (iattr->ia_valid & ATTR_FORCE)
 		return 0;
 
+	/*
+	 * are we changing mode?
+	 * does policy support seperate suid/sgid checks?
+	 * is this a regular file?
+	 * do either the old inode->i_mode or the new iattr->i_mode have the
+	 * suid/sgid bits set?
+	 */
+	if ((iattr->ia_valid & ATTR_MODE) &&
+	    selinux_policycap_setsuidperm &&
+	    (S_ISREG(mode)) &&
+	    ((iattr->ia_mode & (S_ISUID | S_ISGID)) ||
+	     (mode & (S_ISUID | S_ISGID))))
+			av |= FILE__SETSUID;
+
 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
-		return dentry_has_perm(current, NULL, dentry, FILE__SETATTR);
+		av |= FILE__SETATTR;
+	else
+		av = FILE__WRITE;
 
-	return dentry_has_perm(current, NULL, dentry, FILE__WRITE);
+	return dentry_has_perm(current, NULL, dentry, av);
 }
 
 static int selinux_inode_getattr(struct vfsmount *mnt, struct dentry *dentry)

security/selinux/include/av_perm_to_string.h +1 -0

index 1223b4f..c6c5c0e 100644
@@ -19,6 +19,7 @@
    S_(SECCLASS_FILE, FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_FILE, FILE__EXECMOD, "execmod")
    S_(SECCLASS_FILE, FILE__OPEN, "open")
+   S_(SECCLASS_FILE, FILE__SETSUID, "setsuid")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECUTE_NO_TRANS, "execute_no_trans")
    S_(SECCLASS_CHR_FILE, CHR_FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECMOD, "execmod")

security/selinux/include/av_permissions.h +1 -0

index c4c5116..cd6d566 100644
@@ -101,6 +101,7 @@
 #define FILE__ENTRYPOINT                          0x00040000UL
 #define FILE__EXECMOD                             0x00080000UL
 #define FILE__OPEN                                0x00100000UL
+#define FILE__SETSUID                             0x00200000UL
 #define LNK_FILE__IOCTL                           0x00000001UL
 #define LNK_FILE__READ                            0x00000002UL
 #define LNK_FILE__WRITE                           0x00000004UL

security/selinux/include/class_to_string.h +4 -0

index bd813c3..0d11270 100644
@@ -72,3 +72,7 @@
     S_(NULL)
     S_("peer")
     S_("capability2")
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)

security/selinux/include/security.h +2 -0

index 7244737..a604f05 100644
@@ -56,12 +56,14 @@ extern int selinux_mls_enabled;
 enum {
 	POLICYDB_CAPABILITY_NETPEER,
 	POLICYDB_CAPABILITY_OPENPERM,
+	POLICYDB_CAPABILITY_SETSUIDPERM,
 	__POLICYDB_CAPABILITY_MAX
 };
 #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
 
 extern int selinux_policycap_netpeer;
 extern int selinux_policycap_openperm;
+extern int selinux_policycap_setsuidperm;
 
 /*
  * type_datum properties

security/selinux/selinuxfs.c +2 -1

index 69c9dcc..26ef62f 100644
@@ -42,7 +42,8 @@
 /* Policy capability filenames */
 static char *policycap_names[] = {
 	"network_peer_controls",
-	"open_perms"
+	"open_perms",
+	"setsuid_perms",
 };
 
 unsigned int selinux_checkreqprot = CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE;

security/selinux/ss/services.c +3 -0

index 343c8ab..9ecd8e7 100644
@@ -64,6 +64,7 @@ unsigned int policydb_loaded_version;
 
 int selinux_policycap_netpeer;
 int selinux_policycap_openperm;
+int selinux_policycap_setsuidperm;
 
 /*
  * This is declared in avc.c

@@ -1615,6 +1616,8 @@ static void security_load_policycaps(void)
 						  POLICYDB_CAPABILITY_NETPEER);
 	selinux_policycap_openperm = ebitmap_get_bit(&policydb.policycaps,
 						  POLICYDB_CAPABILITY_OPENPERM);
+	selinux_policycap_setsuidperm = ebitmap_get_bit(&policydb.policycaps,
+						  POLICYDB_CAPABILITY_SETSUIDPERM);
 }
 

Quoted 2 lines

Quoted 2 lines

security/selinux/ss/services.c static void security_load_policycaps(void) · line 1623

Quoted 2 lines

security/selinux/ss/services.c +0 -0

 extern void selinux_complete_init(void);

The reason I asked for this is for the confined administrator user.

Quoted 8 lines

Say I am webadmin of a system. I can run sudo to become webadm_r:webadmin_t as UID 0. I now want to change the permissions on a cgi script to be executable. If I can change it to setuid, or setgid, I might be able to find an alternate root to executing the app with raised privs.

I just see this as a risk, and I see little reason why a confined administrator would even need to setuid/setgid an application.

Well, as I noted when this came up as an RFC a year ago, the fact that they can create a setuid program doesn't mean that they can exercise any capabilities since we control that based on domain. And rather than preventing them from setting the suid bit, it might be more useful to prevent them from executing a setuid program (regardless of who made it setuid) or letting it execute but not changing its uid in that case (just as we do under ptrace).

Quoted 2 lines

http://marc.info/?l=selinux&m=119013530120434&w=2

Lets take another path to this.

nsplugin executes programs that want to setattr to /home/dwalsh/.recently-used.xbel

Why I don't know. But audit2allow tells me to add a rule

allow nsplugin_t user_home_t:file setattr;

Which I might just allow, since maybe changing the ownership on a file is required. (I am dont auditing it.) If nsplugin_t was allowed setattr, it could be used to chmod 4755 ~/myexe, and now another user on the system would be able to run ~dwalsh/myexe as dwalsh. (If they had the same user type.)

Quoted 223 lines · Daniel J Walsh

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 210 lines · Stephen Smalley

Quoted 193 lines · Daniel J Walsh

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 185 lines · Stephen Smalley

Quoted 18 lines · Stephen Smalley

Quoted 7 lines · Eric Paris

This patch adds a new permission, setsuid, to the file class. This permission is checked any time a file has its attributes modified (setattr) and the setuid or setgid bits are involved. The purpose for this change is to further allow selinux to confine administrative duties. The example explains when the permission is needed and when it is not:

Quoted 8 lines

Start with a file with chmod 0644.

chmod u+s  (0644 -> 4644) { setattr setsuid }
chmod u-r  (4644 -> 4244) { setattr setsuid }
chmod u-s  (4244 -> 0244) { setattr setsuid }
chmod u+r  (0244 -> 0644) { setattr }

If either the starting or the final attributes will have the setuid or setgid bits set you need this permission.

I'd like to understand better how this would be used.

Quoted 8 lines

Suppose that I want to control the ability to create/modify privileged executables on the system. Given that SELinux already controls capability usage, I can already do that by labeling privileged executable with an appropriate type and controlling the ability to create/modify/relabelto that type, without being concerned about the suid bit. Why do I need this check? And if I need this check, then don't I need a similar check on setting/clearing file capabilities on a given file?

One other tidbit: I don't believe that this check will get applied in the case where a suid/sgid binary is overwritten and the suid/sgid bits are forcibly cleared (ATTR_FORCE), as the selinux_inode_setattr() hook returns immediately in that case as it must not fail.

Quoted 153 lines

Last concern I have is with this check not being adequately granular since it is a single check for setting or clearing the suid or sgid bit of a file owned by any user, whereas it seems more security-relevant to be setting the suid bit on a root-owned executable than to be clearing it or to be setting the suid bit on a sds-owned executable. I'm not sure how to write a useful policy that lets users do things that are harmless or of no interest while still enforcing a useful goal.

Quoted 145 lines

Signed-off-by Eric Paris <[email protected]>

---

 security/selinux/hooks.c                     |   22 ++++++++++++++++++++--
 security/selinux/include/av_perm_to_string.h |    1 +
 security/selinux/include/av_permissions.h    |    1 +
 security/selinux/include/class_to_string.h   |    4 ++++
 security/selinux/include/security.h          |    2 ++
 security/selinux/selinuxfs.c                 |    3 ++-
 security/selinux/ss/services.c               |    3 +++
 7 files changed, 33 insertions(+), 3 deletions(-)

security/selinux/hooks.c +20 -2

index 576e511..58af86a 100644
@@ -2659,6 +2659,8 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	int rc;
+	unsigned int mode = dentry->d_inode->i_mode;
+	u32 av = 0;
 
 	rc = secondary_ops->inode_setattr(dentry, iattr);
 	if (rc)

@@ -2667,11 +2669,27 @@ static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 	if (iattr->ia_valid & ATTR_FORCE)
 		return 0;
 
+	/*
+	 * are we changing mode?
+	 * does policy support seperate suid/sgid checks?
+	 * is this a regular file?
+	 * do either the old inode->i_mode or the new iattr->i_mode have the
+	 * suid/sgid bits set?
+	 */
+	if ((iattr->ia_valid & ATTR_MODE) &&
+	    selinux_policycap_setsuidperm &&
+	    (S_ISREG(mode)) &&
+	    ((iattr->ia_mode & (S_ISUID | S_ISGID)) ||
+	     (mode & (S_ISUID | S_ISGID))))
+			av |= FILE__SETSUID;
+
 	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
 			       ATTR_ATIME_SET | ATTR_MTIME_SET))
-		return dentry_has_perm(current, NULL, dentry, FILE__SETATTR);
+		av |= FILE__SETATTR;
+	else
+		av = FILE__WRITE;
 
-	return dentry_has_perm(current, NULL, dentry, FILE__WRITE);
+	return dentry_has_perm(current, NULL, dentry, av);
 }
 
 static int selinux_inode_getattr(struct vfsmount *mnt, struct dentry *dentry)

security/selinux/include/av_perm_to_string.h +1 -0

index 1223b4f..c6c5c0e 100644
@@ -19,6 +19,7 @@
    S_(SECCLASS_FILE, FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_FILE, FILE__EXECMOD, "execmod")
    S_(SECCLASS_FILE, FILE__OPEN, "open")
+   S_(SECCLASS_FILE, FILE__SETSUID, "setsuid")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECUTE_NO_TRANS, "execute_no_trans")
    S_(SECCLASS_CHR_FILE, CHR_FILE__ENTRYPOINT, "entrypoint")
    S_(SECCLASS_CHR_FILE, CHR_FILE__EXECMOD, "execmod")

security/selinux/include/av_permissions.h +1 -0

index c4c5116..cd6d566 100644
@@ -101,6 +101,7 @@
 #define FILE__ENTRYPOINT                          0x00040000UL
 #define FILE__EXECMOD                             0x00080000UL
 #define FILE__OPEN                                0x00100000UL
+#define FILE__SETSUID                             0x00200000UL
 #define LNK_FILE__IOCTL                           0x00000001UL
 #define LNK_FILE__READ                            0x00000002UL
 #define LNK_FILE__WRITE                           0x00000004UL

security/selinux/include/class_to_string.h +4 -0

index bd813c3..0d11270 100644
@@ -72,3 +72,7 @@
     S_(NULL)
     S_("peer")
     S_("capability2")
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)

security/selinux/include/security.h +2 -0

index 7244737..a604f05 100644
@@ -56,12 +56,14 @@ extern int selinux_mls_enabled;
 enum {
 	POLICYDB_CAPABILITY_NETPEER,
 	POLICYDB_CAPABILITY_OPENPERM,
+	POLICYDB_CAPABILITY_SETSUIDPERM,
 	__POLICYDB_CAPABILITY_MAX
 };
 #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1)
 
 extern int selinux_policycap_netpeer;
 extern int selinux_policycap_openperm;
+extern int selinux_policycap_setsuidperm;
 
 /*
  * type_datum properties

security/selinux/selinuxfs.c +2 -1

index 69c9dcc..26ef62f 100644
@@ -42,7 +42,8 @@
 /* Policy capability filenames */
 static char *policycap_names[] = {
 	"network_peer_controls",
-	"open_perms"
+	"open_perms",
+	"setsuid_perms",
 };
 
 unsigned int selinux_checkreqprot = CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE;

security/selinux/ss/services.c +3 -0

index 343c8ab..9ecd8e7 100644
@@ -64,6 +64,7 @@ unsigned int policydb_loaded_version;
 
 int selinux_policycap_netpeer;
 int selinux_policycap_openperm;
+int selinux_policycap_setsuidperm;
 
 /*
  * This is declared in avc.c

@@ -1615,6 +1616,8 @@ static void security_load_policycaps(void)
 						  POLICYDB_CAPABILITY_NETPEER);
 	selinux_policycap_openperm = ebitmap_get_bit(&policydb.policycaps,
 						  POLICYDB_CAPABILITY_OPENPERM);
+	selinux_policycap_setsuidperm = ebitmap_get_bit(&policydb.policycaps,
+						  POLICYDB_CAPABILITY_SETSUIDPERM);
 }
 

Quoted 2 lines

Quoted 2 lines

security/selinux/ss/services.c static void security_load_policycaps(void) · line 1623

Quoted 2 lines

security/selinux/ss/services.c +0 -0

 extern void selinux_complete_init(void);

The reason I asked for this is for the confined administrator user.

Quoted 8 lines

Say I am webadmin of a system. I can run sudo to become webadm_r:webadmin_t as UID 0. I now want to change the permissions on a cgi script to be executable. If I can change it to setuid, or setgid, I might be able to find an alternate root to executing the app with raised privs.

I just see this as a risk, and I see little reason why a confined administrator would even need to setuid/setgid an application.

Well, as I noted when this came up as an RFC a year ago, the fact that they can create a setuid program doesn't mean that they can exercise any capabilities since we control that based on domain. And rather than preventing them from setting the suid bit, it might be more useful to prevent them from executing a setuid program (regardless of who made it setuid) or letting it execute but not changing its uid in that case (just as we do under ptrace).

Quoted 2 lines

http://marc.info/?l=selinux&m=119013530120434&w=2

Lets take another path to this.

nsplugin executes programs that want to setattr to /home/dwalsh/.recently-used.xbel

Why I don't know. But audit2allow tells me to add a rule

Quoted 7 lines

allow nsplugin_t user_home_t:file setattr;

Which I might just allow, since maybe changing the ownership on a file is required. (I am dont auditing it.) If nsplugin_t was allowed setattr, it could be used to chmod 4755 ~/myexe, and now another user on the system would be able to run ~dwalsh/myexe as dwalsh. (If they had the same user type.)

Ok, so what you want to do is control the ability to create additional "entrypoints" into a user identity. In which case you want to control the following actions:
- making an executable suid (chmod with suid bit set),
- adding further execute access to an existing suid executable (chmod +x of a suid executable _or_ modifying the ACL of a suid executable).

But I don't see the benefit to you of controlling the ability to clear the suid bit or modifying the rw mode bits on a suid executable (writing to a suid executable forcibly clears the suid bit, so you end up with a program that is no longer suid at all). Using the same permission for all of the actions covered in Eric's patch just increases the likelihood that you'll have to grant it in some cases where you don't want to allow the particular two actions above.

Fine, I don't care about this either. So setting the setuid bit on would be the only thing we watch.