-----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@xxxxxxxxxx>
>>>>>
>>>>> ---
>>>>>
>>>>> 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@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
