[PATCH] selinux: new permission for chmod on suid or sgid files

[PATCH] selinux: new permission for chmod on suid or sgid files

[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:

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 <eparis@redhat.com>

---

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

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 576e511..58af86a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -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)
diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h
index 1223b4f..c6c5c0e 100644
--- a/security/selinux/include/av_perm_to_string.h
+++ b/security/selinux/include/av_perm_to_string.h
@@ -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")
diff --git a/security/selinux/include/av_permissions.h b/security/selinux/include/av_permissions.h
index c4c5116..cd6d566 100644
--- a/security/selinux/include/av_permissions.h
+++ b/security/selinux/include/av_permissions.h
@@ -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
diff --git a/security/selinux/include/class_to_string.h b/security/selinux/include/class_to_string.h
index bd813c3..0d11270 100644
--- a/security/selinux/include/class_to_string.h
+++ b/security/selinux/include/class_to_string.h
@@ -72,3 +72,7 @@
     S_(NULL)
     S_("peer")
     S_("capability2")
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)
+    S_(NULL)
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 7244737..a604f05 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -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
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 69c9dcc..26ef62f 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -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;
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 343c8ab..9ecd8e7 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -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);



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Stephen Smalley]

On Fri, 2008-10-17 at 11:40 -0400, Eric Paris wrote:
> 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.

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.

> 
> Signed-off-by: Eric Paris <eparis@redhat.com>
> 
> ---
> 
>  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(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 576e511..58af86a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -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)
> diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h
> index 1223b4f..c6c5c0e 100644
> --- a/security/selinux/include/av_perm_to_string.h
> +++ b/security/selinux/include/av_perm_to_string.h
> @@ -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")
> diff --git a/security/selinux/include/av_permissions.h b/security/selinux/include/av_permissions.h
> index c4c5116..cd6d566 100644
> --- a/security/selinux/include/av_permissions.h
> +++ b/security/selinux/include/av_permissions.h
> @@ -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
> diff --git a/security/selinux/include/class_to_string.h b/security/selinux/include/class_to_string.h
> index bd813c3..0d11270 100644
> --- a/security/selinux/include/class_to_string.h
> +++ b/security/selinux/include/class_to_string.h
> @@ -72,3 +72,7 @@
>      S_(NULL)
>      S_("peer")
>      S_("capability2")
> +    S_(NULL)
> +    S_(NULL)
> +    S_(NULL)
> +    S_(NULL)
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index 7244737..a604f05 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -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
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 69c9dcc..26ef62f 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -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;
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 343c8ab..9ecd8e7 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -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);
> 
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Stephen Smalley]

On Fri, 2008-10-17 at 12:52 -0400, Stephen Smalley wrote:
> On Fri, 2008-10-17 at 11:40 -0400, Eric Paris wrote:
> > 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.
> 
> 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?

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.

> 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.
> 
> > 
> > Signed-off-by: Eric Paris <eparis@redhat.com>
> > 
> > ---
> > 
> >  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(-)
> > 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 576e511..58af86a 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -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)
> > diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h
> > index 1223b4f..c6c5c0e 100644
> > --- a/security/selinux/include/av_perm_to_string.h
> > +++ b/security/selinux/include/av_perm_to_string.h
> > @@ -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")
> > diff --git a/security/selinux/include/av_permissions.h b/security/selinux/include/av_permissions.h
> > index c4c5116..cd6d566 100644
> > --- a/security/selinux/include/av_permissions.h
> > +++ b/security/selinux/include/av_permissions.h
> > @@ -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
> > diff --git a/security/selinux/include/class_to_string.h b/security/selinux/include/class_to_string.h
> > index bd813c3..0d11270 100644
> > --- a/security/selinux/include/class_to_string.h
> > +++ b/security/selinux/include/class_to_string.h
> > @@ -72,3 +72,7 @@
> >      S_(NULL)
> >      S_("peer")
> >      S_("capability2")
> > +    S_(NULL)
> > +    S_(NULL)
> > +    S_(NULL)
> > +    S_(NULL)
> > diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> > index 7244737..a604f05 100644
> > --- a/security/selinux/include/security.h
> > +++ b/security/selinux/include/security.h
> > @@ -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
> > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > index 69c9dcc..26ef62f 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -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;
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index 343c8ab..9ecd8e7 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -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);
> > 
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Daniel J Walsh]

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

Stephen Smalley wrote:
> On Fri, 2008-10-17 at 12:52 -0400, Stephen Smalley wrote:
>> On Fri, 2008-10-17 at 11:40 -0400, Eric Paris wrote:
>>> 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.
>> 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?
> 
> 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.
> 
>> 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.
>>
>>> Signed-off-by: Eric Paris <eparis@redhat.com>
>>>
>>> ---
>>>
>>>  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(-)
>>>
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 576e511..58af86a 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -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)
>>> diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h
>>> index 1223b4f..c6c5c0e 100644
>>> --- a/security/selinux/include/av_perm_to_string.h
>>> +++ b/security/selinux/include/av_perm_to_string.h
>>> @@ -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")
>>> diff --git a/security/selinux/include/av_permissions.h b/security/selinux/include/av_permissions.h
>>> index c4c5116..cd6d566 100644
>>> --- a/security/selinux/include/av_permissions.h
>>> +++ b/security/selinux/include/av_permissions.h
>>> @@ -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
>>> diff --git a/security/selinux/include/class_to_string.h b/security/selinux/include/class_to_string.h
>>> index bd813c3..0d11270 100644
>>> --- a/security/selinux/include/class_to_string.h
>>> +++ b/security/selinux/include/class_to_string.h
>>> @@ -72,3 +72,7 @@
>>>      S_(NULL)
>>>      S_("peer")
>>>      S_("capability2")
>>> +    S_(NULL)
>>> +    S_(NULL)
>>> +    S_(NULL)
>>> +    S_(NULL)
>>> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
>>> index 7244737..a604f05 100644
>>> --- a/security/selinux/include/security.h
>>> +++ b/security/selinux/include/security.h
>>> @@ -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
>>> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
>>> index 69c9dcc..26ef62f 100644
>>> --- a/security/selinux/selinuxfs.c
>>> +++ b/security/selinux/selinuxfs.c
>>> @@ -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;
>>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
>>> index 343c8ab..9ecd8e7 100644
>>> --- a/security/selinux/ss/services.c
>>> +++ b/security/selinux/ss/services.c
>>> @@ -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);
>>>
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.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iEYEARECAAYFAkj43OMACgkQrlYvE4MpobMU2gCgmUv9CJojTN/f9Ivg9ZTFBdt5
LgMAoN8MvV5BJ4lsC52fdlny0lddjXJV
=5Yyx
-----END PGP SIGNATURE-----

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Stephen Smalley]

On Fri, 2008-10-17 at 14:43 -0400, Daniel J Walsh wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Stephen Smalley wrote:
> > On Fri, 2008-10-17 at 12:52 -0400, Stephen Smalley wrote:
> >> On Fri, 2008-10-17 at 11:40 -0400, Eric Paris wrote:
> >>> 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.
> >> 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?
> > 
> > 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.
> > 
> >> 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.
> >>
> >>> Signed-off-by: Eric Paris <eparis@redhat.com>
> >>>
> >>> ---
> >>>
> >>>  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(-)
> >>>
> >>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >>> index 576e511..58af86a 100644
> >>> --- a/security/selinux/hooks.c
> >>> +++ b/security/selinux/hooks.c
> >>> @@ -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)
> >>> diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h
> >>> index 1223b4f..c6c5c0e 100644
> >>> --- a/security/selinux/include/av_perm_to_string.h
> >>> +++ b/security/selinux/include/av_perm_to_string.h
> >>> @@ -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")
> >>> diff --git a/security/selinux/include/av_permissions.h b/security/selinux/include/av_permissions.h
> >>> index c4c5116..cd6d566 100644
> >>> --- a/security/selinux/include/av_permissions.h
> >>> +++ b/security/selinux/include/av_permissions.h
> >>> @@ -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
> >>> diff --git a/security/selinux/include/class_to_string.h b/security/selinux/include/class_to_string.h
> >>> index bd813c3..0d11270 100644
> >>> --- a/security/selinux/include/class_to_string.h
> >>> +++ b/security/selinux/include/class_to_string.h
> >>> @@ -72,3 +72,7 @@
> >>>      S_(NULL)
> >>>      S_("peer")
> >>>      S_("capability2")
> >>> +    S_(NULL)
> >>> +    S_(NULL)
> >>> +    S_(NULL)
> >>> +    S_(NULL)
> >>> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> >>> index 7244737..a604f05 100644
> >>> --- a/security/selinux/include/security.h
> >>> +++ b/security/selinux/include/security.h
> >>> @@ -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
> >>> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> >>> index 69c9dcc..26ef62f 100644
> >>> --- a/security/selinux/selinuxfs.c
> >>> +++ b/security/selinux/selinuxfs.c
> >>> @@ -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;
> >>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> >>> index 343c8ab..9ecd8e7 100644
> >>> --- a/security/selinux/ss/services.c
> >>> +++ b/security/selinux/ss/services.c
> >>> @@ -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);
> >>>
> 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.

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

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Daniel J Walsh]

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

Stephen Smalley wrote:
> On Fri, 2008-10-17 at 14:43 -0400, Daniel J Walsh wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Stephen Smalley wrote:
>>> On Fri, 2008-10-17 at 12:52 -0400, Stephen Smalley wrote:
>>>> On Fri, 2008-10-17 at 11:40 -0400, Eric Paris wrote:
>>>>> 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.
>>>> 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?
>>> 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.
>>>
>>>> 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.
>>>>
>>>>> Signed-off-by: Eric Paris <eparis@redhat.com>
>>>>>
>>>>> ---
>>>>>
>>>>>  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(-)
>>>>>
>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>>> index 576e511..58af86a 100644
>>>>> --- a/security/selinux/hooks.c
>>>>> +++ b/security/selinux/hooks.c
>>>>> @@ -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)
>>>>> diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h
>>>>> index 1223b4f..c6c5c0e 100644
>>>>> --- a/security/selinux/include/av_perm_to_string.h
>>>>> +++ b/security/selinux/include/av_perm_to_string.h
>>>>> @@ -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")
>>>>> diff --git a/security/selinux/include/av_permissions.h b/security/selinux/include/av_permissions.h
>>>>> index c4c5116..cd6d566 100644
>>>>> --- a/security/selinux/include/av_permissions.h
>>>>> +++ b/security/selinux/include/av_permissions.h
>>>>> @@ -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
>>>>> diff --git a/security/selinux/include/class_to_string.h b/security/selinux/include/class_to_string.h
>>>>> index bd813c3..0d11270 100644
>>>>> --- a/security/selinux/include/class_to_string.h
>>>>> +++ b/security/selinux/include/class_to_string.h
>>>>> @@ -72,3 +72,7 @@
>>>>>      S_(NULL)
>>>>>      S_("peer")
>>>>>      S_("capability2")
>>>>> +    S_(NULL)
>>>>> +    S_(NULL)
>>>>> +    S_(NULL)
>>>>> +    S_(NULL)
>>>>> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
>>>>> index 7244737..a604f05 100644
>>>>> --- a/security/selinux/include/security.h
>>>>> +++ b/security/selinux/include/security.h
>>>>> @@ -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
>>>>> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
>>>>> index 69c9dcc..26ef62f 100644
>>>>> --- a/security/selinux/selinuxfs.c
>>>>> +++ b/security/selinux/selinuxfs.c
>>>>> @@ -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;
>>>>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
>>>>> index 343c8ab..9ecd8e7 100644
>>>>> --- a/security/selinux/ss/services.c
>>>>> +++ b/security/selinux/ss/services.c
>>>>> @@ -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);
>>>>>
>> 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.
> 
> 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
> 
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.



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iEYEARECAAYFAkj44zMACgkQrlYvE4MpobNj+QCfaQaoJ11g12D5nTHEOBhp1jz8
enwAn2ddpQbSrXDq1aVWbg4MeHL0bf1w
=5aec
-----END PGP SIGNATURE-----

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Stephen Smalley]

On Fri, 2008-10-17 at 15:10 -0400, Daniel J Walsh wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Stephen Smalley wrote:
> > On Fri, 2008-10-17 at 14:43 -0400, Daniel J Walsh wrote:
> >> -----BEGIN PGP SIGNED MESSAGE-----
> >> Hash: SHA1
> >>
> >> Stephen Smalley wrote:
> >>> On Fri, 2008-10-17 at 12:52 -0400, Stephen Smalley wrote:
> >>>> On Fri, 2008-10-17 at 11:40 -0400, Eric Paris wrote:
> >>>>> 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.
> >>>> 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?
> >>> 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.
> >>>
> >>>> 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.
> >>>>
> >>>>> Signed-off-by: Eric Paris <eparis@redhat.com>
> >>>>>
> >>>>> ---
> >>>>>
> >>>>>  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(-)
> >>>>>
> >>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >>>>> index 576e511..58af86a 100644
> >>>>> --- a/security/selinux/hooks.c
> >>>>> +++ b/security/selinux/hooks.c
> >>>>> @@ -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)
> >>>>> diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h
> >>>>> index 1223b4f..c6c5c0e 100644
> >>>>> --- a/security/selinux/include/av_perm_to_string.h
> >>>>> +++ b/security/selinux/include/av_perm_to_string.h
> >>>>> @@ -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")
> >>>>> diff --git a/security/selinux/include/av_permissions.h b/security/selinux/include/av_permissions.h
> >>>>> index c4c5116..cd6d566 100644
> >>>>> --- a/security/selinux/include/av_permissions.h
> >>>>> +++ b/security/selinux/include/av_permissions.h
> >>>>> @@ -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
> >>>>> diff --git a/security/selinux/include/class_to_string.h b/security/selinux/include/class_to_string.h
> >>>>> index bd813c3..0d11270 100644
> >>>>> --- a/security/selinux/include/class_to_string.h
> >>>>> +++ b/security/selinux/include/class_to_string.h
> >>>>> @@ -72,3 +72,7 @@
> >>>>>      S_(NULL)
> >>>>>      S_("peer")
> >>>>>      S_("capability2")
> >>>>> +    S_(NULL)
> >>>>> +    S_(NULL)
> >>>>> +    S_(NULL)
> >>>>> +    S_(NULL)
> >>>>> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> >>>>> index 7244737..a604f05 100644
> >>>>> --- a/security/selinux/include/security.h
> >>>>> +++ b/security/selinux/include/security.h
> >>>>> @@ -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
> >>>>> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> >>>>> index 69c9dcc..26ef62f 100644
> >>>>> --- a/security/selinux/selinuxfs.c
> >>>>> +++ b/security/selinux/selinuxfs.c
> >>>>> @@ -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;
> >>>>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> >>>>> index 343c8ab..9ecd8e7 100644
> >>>>> --- a/security/selinux/ss/services.c
> >>>>> +++ b/security/selinux/ss/services.c
> >>>>> @@ -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);
> >>>>>
> >> 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.
> > 
> > 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
> > 
> 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.

Going back to your earlier explanation though, it sounds like you want
to prevent a user from creating their own entrypoint programs into their
own user identity.  In that case, I don't see why you care about
clearing the bit or changing other parts of the mode.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Eric Paris]

On Fri, 2008-10-17 at 15:39 -0400, Stephen Smalley wrote:

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

-Eric


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Stephen Smalley]

On Fri, 2008-10-17 at 16:04 -0400, Eric Paris wrote:
> On Fri, 2008-10-17 at 15:39 -0400, Stephen Smalley wrote:
> 
> > 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.

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

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

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

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

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Eric Paris]

On Fri, 2008-10-17 at 16:09 -0400, Stephen Smalley wrote:
> On Fri, 2008-10-17 at 16:04 -0400, Eric Paris wrote:

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

I think this is our main point of contention.  I'm not making strong
claims about the uncircumventable nature of things.  But, you aren't
allowed to on one hand claim this is useless because policy is perfect
and on the other talk about how to circumvent this protection based on
what would have to be a terrible confinement of your webadmin.  I've yet
to see a confined admin that could do many useful things without having
some method to escape the confinement.  Just because it isn't perfect
doesn't mean it's useless or even a bad idea.

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. 

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

Confined domain was the wrong word.  Absolutely.  Let me reword.
"should the webadmin really be allowed to easily override that system
policy because he wasnts to use su - to get to his admin account instead
of sudo" ??

Things like user seperation are the responsibility of DAC in the real
world.  Hardening DAC enforcement should be one of our goals.

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

Obivously his 'web admin' job is going to require root access.  So he
would be perfectly capable of calling chown reguser:reguser /sbin/su.
(I'm not in this particular example discussing the usefullness of the
activity.)  And thats exactly my point.  chowning is going to clear the
suid.  Since suid is set I claim that clearing that bit is against the
DAC security policy and thus chown should be forbidden as well.  My
current patch doesn't do that but I said I'd be inclined to add it.

> Come on, guys, you can do better.

Come on, Stephen, exaplin why hardening systems against confined admins
being able to override DAC system security policies is a bad idea...

-Eric


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Eric Paris]

FAIL ALERT!

On Fri, Oct 17, 2008 at 4:38 PM, Eric Paris <eparis@redhat.com> wrote:
Yes, DAC sercurity
> 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.

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Daniel J Walsh]

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

Stephen Smalley wrote:
> On Fri, 2008-10-17 at 14:43 -0400, Daniel J Walsh wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Stephen Smalley wrote:
>>> On Fri, 2008-10-17 at 12:52 -0400, Stephen Smalley wrote:
>>>> On Fri, 2008-10-17 at 11:40 -0400, Eric Paris wrote:
>>>>> 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.
>>>> 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?
>>> 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.
>>>
>>>> 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.
>>>>
>>>>> Signed-off-by: Eric Paris <eparis@redhat.com>
>>>>>
>>>>> ---
>>>>>
>>>>>  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(-)
>>>>>
>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>>> index 576e511..58af86a 100644
>>>>> --- a/security/selinux/hooks.c
>>>>> +++ b/security/selinux/hooks.c
>>>>> @@ -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)
>>>>> diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h
>>>>> index 1223b4f..c6c5c0e 100644
>>>>> --- a/security/selinux/include/av_perm_to_string.h
>>>>> +++ b/security/selinux/include/av_perm_to_string.h
>>>>> @@ -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")
>>>>> diff --git a/security/selinux/include/av_permissions.h b/security/selinux/include/av_permissions.h
>>>>> index c4c5116..cd6d566 100644
>>>>> --- a/security/selinux/include/av_permissions.h
>>>>> +++ b/security/selinux/include/av_permissions.h
>>>>> @@ -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
>>>>> diff --git a/security/selinux/include/class_to_string.h b/security/selinux/include/class_to_string.h
>>>>> index bd813c3..0d11270 100644
>>>>> --- a/security/selinux/include/class_to_string.h
>>>>> +++ b/security/selinux/include/class_to_string.h
>>>>> @@ -72,3 +72,7 @@
>>>>>      S_(NULL)
>>>>>      S_("peer")
>>>>>      S_("capability2")
>>>>> +    S_(NULL)
>>>>> +    S_(NULL)
>>>>> +    S_(NULL)
>>>>> +    S_(NULL)
>>>>> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
>>>>> index 7244737..a604f05 100644
>>>>> --- a/security/selinux/include/security.h
>>>>> +++ b/security/selinux/include/security.h
>>>>> @@ -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
>>>>> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
>>>>> index 69c9dcc..26ef62f 100644
>>>>> --- a/security/selinux/selinuxfs.c
>>>>> +++ b/security/selinux/selinuxfs.c
>>>>> @@ -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;
>>>>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
>>>>> index 343c8ab..9ecd8e7 100644
>>>>> --- a/security/selinux/ss/services.c
>>>>> +++ b/security/selinux/ss/services.c
>>>>> @@ -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);
>>>>>
>> 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.
> 
> 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
> 
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.)


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iEYEARECAAYFAkj4+qEACgkQrlYvE4MpobOlMgCeKbBaLjhhYuPmEymIMCene1aH
SP0An3VaV+6UnL0fhl2pPcqVtfE3cdGT
=iAhK
-----END PGP SIGNATURE-----

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Daniel J Walsh]

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

Stephen Smalley wrote:
> On Fri, 2008-10-17 at 16:04 -0400, Eric Paris wrote:
>> On Fri, 2008-10-17 at 15:39 -0400, Stephen Smalley wrote:
>>
>>> 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.
> 
>> 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.
> 
>> 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.
> 
>> 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.
> 
>>   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.
> 
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.



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iEYEARECAAYFAkj5AQkACgkQrlYvE4MpobPUJQCg5qRoFCBYh/bj1XU509v63F0H
UawAmwX5xiCLM85U8ZxupBzj5XEd68Jp
=paj/
-----END PGP SIGNATURE-----

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Stephen Smalley]

On Fri, 2008-10-17 at 16:50 -0400, Daniel J Walsh wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Stephen Smalley wrote:
> > On Fri, 2008-10-17 at 14:43 -0400, Daniel J Walsh wrote:
> >> -----BEGIN PGP SIGNED MESSAGE-----
> >> Hash: SHA1
> >>
> >> Stephen Smalley wrote:
> >>> On Fri, 2008-10-17 at 12:52 -0400, Stephen Smalley wrote:
> >>>> On Fri, 2008-10-17 at 11:40 -0400, Eric Paris wrote:
> >>>>> 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.
> >>>> 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?
> >>> 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.
> >>>
> >>>> 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.
> >>>>
> >>>>> Signed-off-by: Eric Paris <eparis@redhat.com>
> >>>>>
> >>>>> ---
> >>>>>
> >>>>>  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(-)
> >>>>>
> >>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >>>>> index 576e511..58af86a 100644
> >>>>> --- a/security/selinux/hooks.c
> >>>>> +++ b/security/selinux/hooks.c
> >>>>> @@ -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)
> >>>>> diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h
> >>>>> index 1223b4f..c6c5c0e 100644
> >>>>> --- a/security/selinux/include/av_perm_to_string.h
> >>>>> +++ b/security/selinux/include/av_perm_to_string.h
> >>>>> @@ -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")
> >>>>> diff --git a/security/selinux/include/av_permissions.h b/security/selinux/include/av_permissions.h
> >>>>> index c4c5116..cd6d566 100644
> >>>>> --- a/security/selinux/include/av_permissions.h
> >>>>> +++ b/security/selinux/include/av_permissions.h
> >>>>> @@ -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
> >>>>> diff --git a/security/selinux/include/class_to_string.h b/security/selinux/include/class_to_string.h
> >>>>> index bd813c3..0d11270 100644
> >>>>> --- a/security/selinux/include/class_to_string.h
> >>>>> +++ b/security/selinux/include/class_to_string.h
> >>>>> @@ -72,3 +72,7 @@
> >>>>>      S_(NULL)
> >>>>>      S_("peer")
> >>>>>      S_("capability2")
> >>>>> +    S_(NULL)
> >>>>> +    S_(NULL)
> >>>>> +    S_(NULL)
> >>>>> +    S_(NULL)
> >>>>> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> >>>>> index 7244737..a604f05 100644
> >>>>> --- a/security/selinux/include/security.h
> >>>>> +++ b/security/selinux/include/security.h
> >>>>> @@ -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
> >>>>> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> >>>>> index 69c9dcc..26ef62f 100644
> >>>>> --- a/security/selinux/selinuxfs.c
> >>>>> +++ b/security/selinux/selinuxfs.c
> >>>>> @@ -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;
> >>>>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> >>>>> index 343c8ab..9ecd8e7 100644
> >>>>> --- a/security/selinux/ss/services.c
> >>>>> +++ b/security/selinux/ss/services.c
> >>>>> @@ -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);
> >>>>>
> >> 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.
> > 
> > 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
> > 
> 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.)

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.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Stephen Smalley]

On Fri, 2008-10-17 at 17:18 -0400, Daniel J Walsh wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Stephen Smalley wrote:
> > On Fri, 2008-10-17 at 16:04 -0400, Eric Paris wrote:
> >> On Fri, 2008-10-17 at 15:39 -0400, Stephen Smalley wrote:
> >>
> >>> 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.
> > 
> >> 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.
> > 
> >> 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.
> > 
> >> 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.
> > 
> >>   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.
> > 
> 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.

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.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Stephen Smalley]

On Fri, 2008-10-17 at 16:38 -0400, Eric Paris wrote:
> 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.

DAC by definition is subject to control by the user, not by the
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.

It isn't about being provable but rather providing any actual security
benefit.

However, I'm open to the notion of controlling making additional
entrypoints to a user identity, as described in my response to Dan, but
I don't believe that this patch is the way to achieve that.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Daniel J Walsh]

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

Stephen Smalley wrote:
> On Fri, 2008-10-17 at 17:18 -0400, Daniel J Walsh wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Stephen Smalley wrote:
>>> On Fri, 2008-10-17 at 16:04 -0400, Eric Paris wrote:
>>>> On Fri, 2008-10-17 at 15:39 -0400, Stephen Smalley wrote:
>>>>
>>>>> 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.
>>>
>>>> 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.
>>>
>>>> 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.
>>>
>>>> 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.
>>>
>>>>   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.
>>>
>> 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.
> 
> 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.  :^)

> 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.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iEYEARECAAYFAkj820UACgkQrlYvE4MpobMpBgCg6bgdShjBt9SNMibqrUos+Wqt
UHsAnjKornOi6HKExM5ktKNZbPRSz1tC
=rsXR
-----END PGP SIGNATURE-----

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Daniel J Walsh]

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

Stephen Smalley wrote:
> On Fri, 2008-10-17 at 16:50 -0400, Daniel J Walsh wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Stephen Smalley wrote:
>>> On Fri, 2008-10-17 at 14:43 -0400, Daniel J Walsh wrote:
>>>> -----BEGIN PGP SIGNED MESSAGE-----
>>>> Hash: SHA1
>>>>
>>>> Stephen Smalley wrote:
>>>>> On Fri, 2008-10-17 at 12:52 -0400, Stephen Smalley wrote:
>>>>>> On Fri, 2008-10-17 at 11:40 -0400, Eric Paris wrote:
>>>>>>> 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.
>>>>>> 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?
>>>>> 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.
>>>>>
>>>>>> 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.
>>>>>>
>>>>>>> Signed-off-by: Eric Paris <eparis@redhat.com>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>>  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(-)
>>>>>>>
>>>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>>>>> index 576e511..58af86a 100644
>>>>>>> --- a/security/selinux/hooks.c
>>>>>>> +++ b/security/selinux/hooks.c
>>>>>>> @@ -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)
>>>>>>> diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h
>>>>>>> index 1223b4f..c6c5c0e 100644
>>>>>>> --- a/security/selinux/include/av_perm_to_string.h
>>>>>>> +++ b/security/selinux/include/av_perm_to_string.h
>>>>>>> @@ -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")
>>>>>>> diff --git a/security/selinux/include/av_permissions.h b/security/selinux/include/av_permissions.h
>>>>>>> index c4c5116..cd6d566 100644
>>>>>>> --- a/security/selinux/include/av_permissions.h
>>>>>>> +++ b/security/selinux/include/av_permissions.h
>>>>>>> @@ -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
>>>>>>> diff --git a/security/selinux/include/class_to_string.h b/security/selinux/include/class_to_string.h
>>>>>>> index bd813c3..0d11270 100644
>>>>>>> --- a/security/selinux/include/class_to_string.h
>>>>>>> +++ b/security/selinux/include/class_to_string.h
>>>>>>> @@ -72,3 +72,7 @@
>>>>>>>      S_(NULL)
>>>>>>>      S_("peer")
>>>>>>>      S_("capability2")
>>>>>>> +    S_(NULL)
>>>>>>> +    S_(NULL)
>>>>>>> +    S_(NULL)
>>>>>>> +    S_(NULL)
>>>>>>> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
>>>>>>> index 7244737..a604f05 100644
>>>>>>> --- a/security/selinux/include/security.h
>>>>>>> +++ b/security/selinux/include/security.h
>>>>>>> @@ -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
>>>>>>> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
>>>>>>> index 69c9dcc..26ef62f 100644
>>>>>>> --- a/security/selinux/selinuxfs.c
>>>>>>> +++ b/security/selinux/selinuxfs.c
>>>>>>> @@ -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;
>>>>>>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
>>>>>>> index 343c8ab..9ecd8e7 100644
>>>>>>> --- a/security/selinux/ss/services.c
>>>>>>> +++ b/security/selinux/ss/services.c
>>>>>>> @@ -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);
>>>>>>>
>>>> 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.
>>> 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
>>>
>> 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.)
> 
> 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.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org

iEYEARECAAYFAkj83pUACgkQrlYvE4MpobNCngCfbIO9itGMvOEu/00rTZMEuPif
98sAniARbXki0BBJgTPRijUA3c46HamM
=4Cwo
-----END PGP SIGNATURE-----

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Stephen Smalley]

On Mon, 2008-10-20 at 15:25 -0400, Daniel J Walsh wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Stephen Smalley wrote:
> > On Fri, 2008-10-17 at 17:18 -0400, Daniel J Walsh wrote:
> >> -----BEGIN PGP SIGNED MESSAGE-----
> >> Hash: SHA1
> >>
> >> Stephen Smalley wrote:
> >>> On Fri, 2008-10-17 at 16:04 -0400, Eric Paris wrote:
> >>>> On Fri, 2008-10-17 at 15:39 -0400, Stephen Smalley wrote:
> >>>>
> >>>>> 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.
> >>>
> >>>> 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.
> >>>
> >>>> 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.
> >>>
> >>>> 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.
> >>>
> >>>>   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.
> >>>
> >> 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.
> > 
> > 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.  :^)
> 
> > 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.

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

The first one controls what processes can create new entrypoints into a
uid/gid.  The second one controls the ability to invoke a suid/sgid or
file-caps program at all.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Eric Paris]

On Mon, Oct 20, 2008 at 4:04 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On Mon, 2008-10-20 at 15:25 -0400, Daniel J Walsh wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Stephen Smalley wrote:
>> > On Fri, 2008-10-17 at 17:18 -0400, Daniel J Walsh wrote:
>> >> -----BEGIN PGP SIGNED MESSAGE-----
>> >> Hash: SHA1
>> >>
>> >> Stephen Smalley wrote:
>> >>> On Fri, 2008-10-17 at 16:04 -0400, Eric Paris wrote:
>> >>>> On Fri, 2008-10-17 at 15:39 -0400, Stephen Smalley wrote:
>> >>>>
>> >>>>> 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.
>> >>>
>> >>>> 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.
>> >>>
>> >>>> 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.
>> >>>
>> >>>> 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.
>> >>>
>> >>>>   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.
>> >>>
>> >> 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.
>> >
>> > 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.  :^)
>>
>> > 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.
>
> 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).

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

-Eric

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Stephen Smalley]

On Mon, 2008-10-20 at 16:12 -0400, Eric Paris wrote:
> On Mon, Oct 20, 2008 at 4:04 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > On Mon, 2008-10-20 at 15:25 -0400, Daniel J Walsh wrote:
> >> -----BEGIN PGP SIGNED MESSAGE-----
> >> Hash: SHA1
> >>
> >> Stephen Smalley wrote:
> >> > On Fri, 2008-10-17 at 17:18 -0400, Daniel J Walsh wrote:
> >> >> -----BEGIN PGP SIGNED MESSAGE-----
> >> >> Hash: SHA1
> >> >>
> >> >> Stephen Smalley wrote:
> >> >>> On Fri, 2008-10-17 at 16:04 -0400, Eric Paris wrote:
> >> >>>> On Fri, 2008-10-17 at 15:39 -0400, Stephen Smalley wrote:
> >> >>>>
> >> >>>>> 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.
> >> >>>
> >> >>>> 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.
> >> >>>
> >> >>>> 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.
> >> >>>
> >> >>>> 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.
> >> >>>
> >> >>>>   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.
> >> >>>
> >> >> 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.
> >> >
> >> > 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.  :^)
> >>
> >> > 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.
> >
> > 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).
> 
> 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.

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[Stephen Smalley]

On Mon, 2008-10-20 at 16:24 -0400, Stephen Smalley wrote:
> On Mon, 2008-10-20 at 16:12 -0400, Eric Paris wrote:
> > On Mon, Oct 20, 2008 at 4:04 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > > On Mon, 2008-10-20 at 15:25 -0400, Daniel J Walsh wrote:
> > >> -----BEGIN PGP SIGNED MESSAGE-----
> > >> Hash: SHA1
> > >>
> > >> Stephen Smalley wrote:
> > >> > On Fri, 2008-10-17 at 17:18 -0400, Daniel J Walsh wrote:
> > >> >> -----BEGIN PGP SIGNED MESSAGE-----
> > >> >> Hash: SHA1
> > >> >>
> > >> >> Stephen Smalley wrote:
> > >> >>> On Fri, 2008-10-17 at 16:04 -0400, Eric Paris wrote:
> > >> >>>> On Fri, 2008-10-17 at 15:39 -0400, Stephen Smalley wrote:
> > >> >>>>
> > >> >>>>> 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.
> > >> >>>
> > >> >>>> 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.
> > >> >>>
> > >> >>>> 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.
> > >> >>>
> > >> >>>> 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.
> > >> >>>
> > >> >>>>   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.
> > >> >>>
> > >> >> 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.
> > >> >
> > >> > 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.  :^)
> > >>
> > >> > 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.
> > >
> > > 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).
> > 
> > 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

Sorry - s/cap_effective/cap_post_exec_permitted/

> 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.
> 
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: new permission for chmod on suid or sgid files

[James Morris]

On Mon, 20 Oct 2008, Daniel J Walsh wrote:


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

Can you guys please trim your replies?  It's not much fun scrolling 
through 14K of quoted email at nine levels of indentation just to read two 
lines of actual content.

Thanks,


- James
-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.