* rfc: kernel: special enforcing on suid and setgid chmod @ 2007-09-17 18:01 Eric Paris 2007-09-17 19:23 ` Stephen Smalley 0 siblings, 1 reply; 5+ messages in thread From: Eric Paris @ 2007-09-17 18:01 UTC (permalink / raw) To: selinux; +Cc: dwalsh, sgrubb, sds, jmorris It was mentioned today that it might be useful to split the security check for setting DAC permissions such that the setuid and setgid bits could have special enforcement. This patch adds two new permissions to the file class and should explicitly check those. It doesn't do anything for the sticky bit, nor does this patch do anything for setuid and setgid on directories. Thoughts? couple questions: Are their other file types where those bits have meaning other than regular files and directories? Do I/policy people care about these bits on directories? (see man 2 stat) Am I allowed to add permissions to the set of common permissions without bumping the policy version number? I first thought about adding setuid and setgid to the common set for all files but then figured it would break something. This is uncompiled, untested, untrustworthy, about the only thing it is not un- of is worthy of a bit of discussion. If discussions goes well, I'll actually make a tested version of this patch when the userspace people who wanted to make use of it get to it. -Eric --- security/selinux/hooks.c | 9 ++++++++- security/selinux/include/av_perm_to_string.h | 2 ++ security/selinux/include/av_permissions.h | 2 ++ 3 files changed, 12 insertions(+), 1 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 3694662..6838e91 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2273,6 +2273,7 @@ static int selinux_inode_permission(struct inode *inode, int mask, static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr) { int rc; + u32 check = FILE__SETATTR; rc = secondary_ops->inode_setattr(dentry, iattr); if (rc) @@ -2281,9 +2282,15 @@ static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr) if (iattr->ia_valid & ATTR_FORCE) return 0; + if ((iattr->ia_valid & ATTR_MODE) & S_ISREG(dentry->d_inode->i_mode)) { + if (S_ISUID & iattr->ia_mode) + check |= FILE__SETUID; + if (S_ISGID & iattr->ia_mode) + check |= FILE__SETGID; + } 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); + return dentry_has_perm(current, NULL, dentry, check); return dentry_has_perm(current, NULL, dentry, FILE__WRITE); } diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h index 049bf69..11a40af 100644 --- a/security/selinux/include/av_perm_to_string.h +++ b/security/selinux/include/av_perm_to_string.h @@ -17,6 +17,8 @@ S_(SECCLASS_FILE, FILE__EXECUTE_NO_TRANS, "execute_no_trans") S_(SECCLASS_FILE, FILE__ENTRYPOINT, "entrypoint") S_(SECCLASS_FILE, FILE__EXECMOD, "execmod") + S_(SECCLASS_FILE, FILE__SETUID, "setuid") + S_(SECCLASS_FILE, FILE__SETGID, "setgid") 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 eda89a2..2d28e92 100644 --- a/security/selinux/include/av_permissions.h +++ b/security/selinux/include/av_permissions.h @@ -99,6 +99,8 @@ #define FILE__EXECUTE_NO_TRANS 0x00020000UL #define FILE__ENTRYPOINT 0x00040000UL #define FILE__EXECMOD 0x00080000UL +#define FILE__SETUID 0x00100000UL +#define FILE__SETGID 0x00200000UL #define LNK_FILE__IOCTL 0x00000001UL #define LNK_FILE__READ 0x00000002UL #define LNK_FILE__WRITE 0x00000004UL -- 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. ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: rfc: kernel: special enforcing on suid and setgid chmod 2007-09-17 18:01 rfc: kernel: special enforcing on suid and setgid chmod Eric Paris @ 2007-09-17 19:23 ` Stephen Smalley 2007-09-17 20:51 ` Daniel J Walsh 0 siblings, 1 reply; 5+ messages in thread From: Stephen Smalley @ 2007-09-17 19:23 UTC (permalink / raw) To: Eric Paris; +Cc: selinux, dwalsh, sgrubb, jmorris On Mon, 2007-09-17 at 14:01 -0400, Eric Paris wrote: > It was mentioned today that it might be useful to split the security > check for setting DAC permissions such that the setuid and setgid bits > could have special enforcement. This patch adds two new permissions to > the file class and should explicitly check those. It doesn't do > anything for the sticky bit, nor does this patch do anything for setuid > and setgid on directories. Thoughts? What's the rationale for it? How would you use it practically? Where do we draw the line for distinctions among different flavors of setattr? What are the implications for similar DAC-related extensions elsewhere? There is the obvious compatibility concern, which might be offset by your handle unknown patch but only if it goes into the kernel and policies well before this change does. > couple questions: > > Are their other file types where those bits have meaning other than > regular files and directories? > > Do I/policy people care about these bits on directories? (see man 2 > stat) > > Am I allowed to add permissions to the set of common permissions without > bumping the policy version number? No, it would alter the values of the per-class perms for classes that inherit from that common definition. And we don't have a framework today for remapping class/perm values upon such incompatible changes, even if you bump the version. Could be a good way to get you to implement dynamic lookup of class/perm values and remapping of them at runtime within the kernel ;) > I first thought about adding setuid > and setgid to the common set for all files but then figured it would > break something. > > This is uncompiled, untested, untrustworthy, about the only thing it is > not un- of is worthy of a bit of discussion. If discussions goes well, > I'll actually make a tested version of this patch when the userspace > people who wanted to make use of it get to it. > > -Eric > > --- > > security/selinux/hooks.c | 9 ++++++++- > security/selinux/include/av_perm_to_string.h | 2 ++ > security/selinux/include/av_permissions.h | 2 ++ > 3 files changed, 12 insertions(+), 1 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 3694662..6838e91 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2273,6 +2273,7 @@ static int selinux_inode_permission(struct inode *inode, int mask, > static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr) > { > int rc; > + u32 check = FILE__SETATTR; > > rc = secondary_ops->inode_setattr(dentry, iattr); > if (rc) > @@ -2281,9 +2282,15 @@ static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr) > if (iattr->ia_valid & ATTR_FORCE) > return 0; > > + if ((iattr->ia_valid & ATTR_MODE) & S_ISREG(dentry->d_inode->i_mode)) { > + if (S_ISUID & iattr->ia_mode) > + check |= FILE__SETUID; > + if (S_ISGID & iattr->ia_mode) > + check |= FILE__SETGID; > + } > 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); > + return dentry_has_perm(current, NULL, dentry, check); > > return dentry_has_perm(current, NULL, dentry, FILE__WRITE); > } > diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h > index 049bf69..11a40af 100644 > --- a/security/selinux/include/av_perm_to_string.h > +++ b/security/selinux/include/av_perm_to_string.h > @@ -17,6 +17,8 @@ > S_(SECCLASS_FILE, FILE__EXECUTE_NO_TRANS, "execute_no_trans") > S_(SECCLASS_FILE, FILE__ENTRYPOINT, "entrypoint") > S_(SECCLASS_FILE, FILE__EXECMOD, "execmod") > + S_(SECCLASS_FILE, FILE__SETUID, "setuid") > + S_(SECCLASS_FILE, FILE__SETGID, "setgid") > 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 eda89a2..2d28e92 100644 > --- a/security/selinux/include/av_permissions.h > +++ b/security/selinux/include/av_permissions.h > @@ -99,6 +99,8 @@ > #define FILE__EXECUTE_NO_TRANS 0x00020000UL > #define FILE__ENTRYPOINT 0x00040000UL > #define FILE__EXECMOD 0x00080000UL > +#define FILE__SETUID 0x00100000UL > +#define FILE__SETGID 0x00200000UL > #define LNK_FILE__IOCTL 0x00000001UL > #define LNK_FILE__READ 0x00000002UL > #define LNK_FILE__WRITE 0x00000004UL > > > > -- > 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. -- 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: rfc: kernel: special enforcing on suid and setgid chmod 2007-09-17 19:23 ` Stephen Smalley @ 2007-09-17 20:51 ` Daniel J Walsh 2007-09-18 16:58 ` Stephen Smalley 0 siblings, 1 reply; 5+ messages in thread From: Daniel J Walsh @ 2007-09-17 20:51 UTC (permalink / raw) To: Stephen Smalley; +Cc: Eric Paris, selinux, sgrubb, jmorris -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Stephen Smalley wrote: > On Mon, 2007-09-17 at 14:01 -0400, Eric Paris wrote: >> It was mentioned today that it might be useful to split the security >> check for setting DAC permissions such that the setuid and setgid bits >> could have special enforcement. This patch adds two new permissions to >> the file class and should explicitly check those. It doesn't do >> anything for the sticky bit, nor does this patch do anything for setuid >> and setgid on directories. Thoughts? > > What's the rationale for it? How would you use it practically? > Where do we draw the line for distinctions among different flavors of > setattr? What are the implications for similar DAC-related extensions > elsewhere? > I setup an account with a webadmr:webadm_t. Only allow this user to become root as webadm_t. User creates a setuid executable. Uses some other flaw to execute this new setuid app to get full root access. webadmin needs chmod in order to set proper permissions on cgi scripts. > There is the obvious compatibility concern, which might be offset by > your handle unknown patch but only if it goes into the kernel and > policies well before this change does. > >> couple questions: >> >> Are their other file types where those bits have meaning other than >> regular files and directories? >> >> Do I/policy people care about these bits on directories? (see man 2 >> stat) >> >> Am I allowed to add permissions to the set of common permissions without >> bumping the policy version number? > > No, it would alter the values of the per-class perms for classes that > inherit from that common definition. And we don't have a framework > today for remapping class/perm values upon such incompatible changes, > even if you bump the version. Could be a good way to get you to > implement dynamic lookup of class/perm values and remapping of them at > runtime within the kernel ;) > >> I first thought about adding setuid >> and setgid to the common set for all files but then figured it would >> break something. >> >> This is uncompiled, untested, untrustworthy, about the only thing it is >> not un- of is worthy of a bit of discussion. If discussions goes well, >> I'll actually make a tested version of this patch when the userspace >> people who wanted to make use of it get to it. >> >> -Eric >> >> --- >> >> security/selinux/hooks.c | 9 ++++++++- >> security/selinux/include/av_perm_to_string.h | 2 ++ >> security/selinux/include/av_permissions.h | 2 ++ >> 3 files changed, 12 insertions(+), 1 deletions(-) >> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> index 3694662..6838e91 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -2273,6 +2273,7 @@ static int selinux_inode_permission(struct inode *inode, int mask, >> static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr) >> { >> int rc; >> + u32 check = FILE__SETATTR; >> >> rc = secondary_ops->inode_setattr(dentry, iattr); >> if (rc) >> @@ -2281,9 +2282,15 @@ static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr) >> if (iattr->ia_valid & ATTR_FORCE) >> return 0; >> >> + if ((iattr->ia_valid & ATTR_MODE) & S_ISREG(dentry->d_inode->i_mode)) { >> + if (S_ISUID & iattr->ia_mode) >> + check |= FILE__SETUID; >> + if (S_ISGID & iattr->ia_mode) >> + check |= FILE__SETGID; >> + } >> 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); >> + return dentry_has_perm(current, NULL, dentry, check); >> >> return dentry_has_perm(current, NULL, dentry, FILE__WRITE); >> } >> diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h >> index 049bf69..11a40af 100644 >> --- a/security/selinux/include/av_perm_to_string.h >> +++ b/security/selinux/include/av_perm_to_string.h >> @@ -17,6 +17,8 @@ >> S_(SECCLASS_FILE, FILE__EXECUTE_NO_TRANS, "execute_no_trans") >> S_(SECCLASS_FILE, FILE__ENTRYPOINT, "entrypoint") >> S_(SECCLASS_FILE, FILE__EXECMOD, "execmod") >> + S_(SECCLASS_FILE, FILE__SETUID, "setuid") >> + S_(SECCLASS_FILE, FILE__SETGID, "setgid") >> 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 eda89a2..2d28e92 100644 >> --- a/security/selinux/include/av_permissions.h >> +++ b/security/selinux/include/av_permissions.h >> @@ -99,6 +99,8 @@ >> #define FILE__EXECUTE_NO_TRANS 0x00020000UL >> #define FILE__ENTRYPOINT 0x00040000UL >> #define FILE__EXECMOD 0x00080000UL >> +#define FILE__SETUID 0x00100000UL >> +#define FILE__SETGID 0x00200000UL >> #define LNK_FILE__IOCTL 0x00000001UL >> #define LNK_FILE__READ 0x00000002UL >> #define LNK_FILE__WRITE 0x00000004UL >> >> >> >> -- >> 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. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.7 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iD8DBQFG7ui7rlYvE4MpobMRAgS7AJ9NdCVwHd/Nq3HR6Z0Yi2AyUjzdqACeN/Vv T1Y7W2Sep6BYni36kQxI2yc= =ye8C -----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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: rfc: kernel: special enforcing on suid and setgid chmod 2007-09-17 20:51 ` Daniel J Walsh @ 2007-09-18 16:58 ` Stephen Smalley 2007-09-18 19:01 ` Daniel J Walsh 0 siblings, 1 reply; 5+ messages in thread From: Stephen Smalley @ 2007-09-18 16:58 UTC (permalink / raw) To: Daniel J Walsh; +Cc: Eric Paris, selinux, sgrubb, jmorris, Karl MacMillan On Mon, 2007-09-17 at 16:51 -0400, Daniel J Walsh wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Stephen Smalley wrote: > > On Mon, 2007-09-17 at 14:01 -0400, Eric Paris wrote: > >> It was mentioned today that it might be useful to split the security > >> check for setting DAC permissions such that the setuid and setgid bits > >> could have special enforcement. This patch adds two new permissions to > >> the file class and should explicitly check those. It doesn't do > >> anything for the sticky bit, nor does this patch do anything for setuid > >> and setgid on directories. Thoughts? > > > > What's the rationale for it? How would you use it practically? > > Where do we draw the line for distinctions among different flavors of > > setattr? What are the implications for similar DAC-related extensions > > elsewhere? > > > I setup an account with a webadmr:webadm_t. Only allow this user to > become root as webadm_t. User creates a setuid executable. Uses some > other flaw to execute this new setuid app to get full root access. Where full root access means what? Just having uid 0? Or having all capabilities? If the latter, we already can limit that based on domain, so the fact that the user can run a suid root shell doesn't mean that he gets capabilities unless his domain is allowed those capabilities. And if you want to distinguish such things on chmod, what about controlling the ability to actually perform the suid/sgid transition at execve time? I want to better understand the end game before we start making little changes one by one. Note too that past discussions of splitting setattr were oriented around separating the distinct operations (chmod vs. chown vs. utimes vs. truncate), so if we are splitting setattr at all (for suid/sgid), we should think about the full set of distinctions we want. Originally we always checked setattr permission, then later changed to only check that for chmod/chown/utimes with a specific timestamp and to check write for the rest (utimes to the current time, truncate). > webadmin needs chmod in order to set proper permissions on cgi scripts. > > > > > > There is the obvious compatibility concern, which might be offset by > > your handle unknown patch but only if it goes into the kernel and > > policies well before this change does. > > > >> couple questions: > >> > >> Are their other file types where those bits have meaning other than > >> regular files and directories? > >> > >> Do I/policy people care about these bits on directories? (see man 2 > >> stat) > >> > >> Am I allowed to add permissions to the set of common permissions without > >> bumping the policy version number? > > > > No, it would alter the values of the per-class perms for classes that > > inherit from that common definition. And we don't have a framework > > today for remapping class/perm values upon such incompatible changes, > > even if you bump the version. Could be a good way to get you to > > implement dynamic lookup of class/perm values and remapping of them at > > runtime within the kernel ;) > > > >> I first thought about adding setuid > >> and setgid to the common set for all files but then figured it would > >> break something. > >> > >> This is uncompiled, untested, untrustworthy, about the only thing it is > >> not un- of is worthy of a bit of discussion. If discussions goes well, > >> I'll actually make a tested version of this patch when the userspace > >> people who wanted to make use of it get to it. > >> > >> -Eric > >> > >> --- > >> > >> security/selinux/hooks.c | 9 ++++++++- > >> security/selinux/include/av_perm_to_string.h | 2 ++ > >> security/selinux/include/av_permissions.h | 2 ++ > >> 3 files changed, 12 insertions(+), 1 deletions(-) > >> > >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > >> index 3694662..6838e91 100644 > >> --- a/security/selinux/hooks.c > >> +++ b/security/selinux/hooks.c > >> @@ -2273,6 +2273,7 @@ static int selinux_inode_permission(struct inode *inode, int mask, > >> static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr) > >> { > >> int rc; > >> + u32 check = FILE__SETATTR; > >> > >> rc = secondary_ops->inode_setattr(dentry, iattr); > >> if (rc) > >> @@ -2281,9 +2282,15 @@ static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr) > >> if (iattr->ia_valid & ATTR_FORCE) > >> return 0; > >> > >> + if ((iattr->ia_valid & ATTR_MODE) & S_ISREG(dentry->d_inode->i_mode)) { > >> + if (S_ISUID & iattr->ia_mode) > >> + check |= FILE__SETUID; > >> + if (S_ISGID & iattr->ia_mode) > >> + check |= FILE__SETGID; > >> + } > >> 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); > >> + return dentry_has_perm(current, NULL, dentry, check); > >> > >> return dentry_has_perm(current, NULL, dentry, FILE__WRITE); > >> } > >> diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h > >> index 049bf69..11a40af 100644 > >> --- a/security/selinux/include/av_perm_to_string.h > >> +++ b/security/selinux/include/av_perm_to_string.h > >> @@ -17,6 +17,8 @@ > >> S_(SECCLASS_FILE, FILE__EXECUTE_NO_TRANS, "execute_no_trans") > >> S_(SECCLASS_FILE, FILE__ENTRYPOINT, "entrypoint") > >> S_(SECCLASS_FILE, FILE__EXECMOD, "execmod") > >> + S_(SECCLASS_FILE, FILE__SETUID, "setuid") > >> + S_(SECCLASS_FILE, FILE__SETGID, "setgid") > >> 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 eda89a2..2d28e92 100644 > >> --- a/security/selinux/include/av_permissions.h > >> +++ b/security/selinux/include/av_permissions.h > >> @@ -99,6 +99,8 @@ > >> #define FILE__EXECUTE_NO_TRANS 0x00020000UL > >> #define FILE__ENTRYPOINT 0x00040000UL > >> #define FILE__EXECMOD 0x00080000UL > >> +#define FILE__SETUID 0x00100000UL > >> +#define FILE__SETGID 0x00200000UL > >> #define LNK_FILE__IOCTL 0x00000001UL > >> #define LNK_FILE__READ 0x00000002UL > >> #define LNK_FILE__WRITE 0x00000004UL > >> > >> > >> > >> -- > >> 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. > > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.7 (GNU/Linux) > Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org > > iD8DBQFG7ui7rlYvE4MpobMRAgS7AJ9NdCVwHd/Nq3HR6Z0Yi2AyUjzdqACeN/Vv > T1Y7W2Sep6BYni36kQxI2yc= > =ye8C > -----END PGP SIGNATURE----- -- 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: rfc: kernel: special enforcing on suid and setgid chmod 2007-09-18 16:58 ` Stephen Smalley @ 2007-09-18 19:01 ` Daniel J Walsh 0 siblings, 0 replies; 5+ messages in thread From: Daniel J Walsh @ 2007-09-18 19:01 UTC (permalink / raw) To: Stephen Smalley; +Cc: Eric Paris, selinux, sgrubb, jmorris, Karl MacMillan -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Stephen Smalley wrote: > On Mon, 2007-09-17 at 16:51 -0400, Daniel J Walsh wrote: > Stephen Smalley wrote: >>>> On Mon, 2007-09-17 at 14:01 -0400, Eric Paris wrote: >>>>> It was mentioned today that it might be useful to split the security >>>>> check for setting DAC permissions such that the setuid and setgid bits >>>>> could have special enforcement. This patch adds two new permissions to >>>>> the file class and should explicitly check those. It doesn't do >>>>> anything for the sticky bit, nor does this patch do anything for setuid >>>>> and setgid on directories. Thoughts? >>>> What's the rationale for it? How would you use it practically? >>>> Where do we draw the line for distinctions among different flavors of >>>> setattr? What are the implications for similar DAC-related extensions >>>> elsewhere? >>>> > I setup an account with a webadmr:webadm_t. Only allow this user to > become root as webadm_t. User creates a setuid executable. Uses some > other flaw to execute this new setuid app to get full root access. > >> Where full root access means what? Just having uid 0? Or having all >> capabilities? If the latter, we already can limit that based on domain, >> so the fact that the user can run a suid root shell doesn't mean that he >> gets capabilities unless his domain is allowed those capabilities. > >> And if you want to distinguish such things on chmod, what about >> controlling the ability to actually perform the suid/sgid transition at >> execve time? I want to better understand the end game before we start >> making little changes one by one. > >> Note too that past discussions of splitting setattr were oriented around >> separating the distinct operations (chmod vs. chown vs. utimes vs. >> truncate), so if we are splitting setattr at all (for suid/sgid), we >> should think about the full set of distinctions we want. Originally we >> always checked setattr permission, then later changed to only check that >> for chmod/chown/utimes with a specific timestamp and to check write for >> the rest (utimes to the current time, truncate). > > webadmin needs chmod in order to set proper permissions on cgi scripts. > > > > >>>> There is the obvious compatibility concern, which might be offset by >>>> your handle unknown patch but only if it goes into the kernel and >>>> policies well before this change does. >>>> >>>>> couple questions: >>>>> >>>>> Are their other file types where those bits have meaning other than >>>>> regular files and directories? >>>>> >>>>> Do I/policy people care about these bits on directories? (see man 2 >>>>> stat) >>>>> >>>>> Am I allowed to add permissions to the set of common permissions without >>>>> bumping the policy version number? >>>> No, it would alter the values of the per-class perms for classes that >>>> inherit from that common definition. And we don't have a framework >>>> today for remapping class/perm values upon such incompatible changes, >>>> even if you bump the version. Could be a good way to get you to >>>> implement dynamic lookup of class/perm values and remapping of them at >>>> runtime within the kernel ;) >>>> >>>>> I first thought about adding setuid >>>>> and setgid to the common set for all files but then figured it would >>>>> break something. >>>>> >>>>> This is uncompiled, untested, untrustworthy, about the only thing it is >>>>> not un- of is worthy of a bit of discussion. If discussions goes well, >>>>> I'll actually make a tested version of this patch when the userspace >>>>> people who wanted to make use of it get to it. >>>>> >>>>> -Eric >>>>> >>>>> --- >>>>> >>>>> security/selinux/hooks.c | 9 ++++++++- >>>>> security/selinux/include/av_perm_to_string.h | 2 ++ >>>>> security/selinux/include/av_permissions.h | 2 ++ >>>>> 3 files changed, 12 insertions(+), 1 deletions(-) >>>>> >>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >>>>> index 3694662..6838e91 100644 >>>>> --- a/security/selinux/hooks.c >>>>> +++ b/security/selinux/hooks.c >>>>> @@ -2273,6 +2273,7 @@ static int selinux_inode_permission(struct inode *inode, int mask, >>>>> static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr) >>>>> { >>>>> int rc; >>>>> + u32 check = FILE__SETATTR; >>>>> >>>>> rc = secondary_ops->inode_setattr(dentry, iattr); >>>>> if (rc) >>>>> @@ -2281,9 +2282,15 @@ static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr) >>>>> if (iattr->ia_valid & ATTR_FORCE) >>>>> return 0; >>>>> >>>>> + if ((iattr->ia_valid & ATTR_MODE) & S_ISREG(dentry->d_inode->i_mode)) { >>>>> + if (S_ISUID & iattr->ia_mode) >>>>> + check |= FILE__SETUID; >>>>> + if (S_ISGID & iattr->ia_mode) >>>>> + check |= FILE__SETGID; >>>>> + } >>>>> 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); >>>>> + return dentry_has_perm(current, NULL, dentry, check); >>>>> >>>>> return dentry_has_perm(current, NULL, dentry, FILE__WRITE); >>>>> } >>>>> diff --git a/security/selinux/include/av_perm_to_string.h b/security/selinux/include/av_perm_to_string.h >>>>> index 049bf69..11a40af 100644 >>>>> --- a/security/selinux/include/av_perm_to_string.h >>>>> +++ b/security/selinux/include/av_perm_to_string.h >>>>> @@ -17,6 +17,8 @@ >>>>> S_(SECCLASS_FILE, FILE__EXECUTE_NO_TRANS, "execute_no_trans") >>>>> S_(SECCLASS_FILE, FILE__ENTRYPOINT, "entrypoint") >>>>> S_(SECCLASS_FILE, FILE__EXECMOD, "execmod") >>>>> + S_(SECCLASS_FILE, FILE__SETUID, "setuid") >>>>> + S_(SECCLASS_FILE, FILE__SETGID, "setgid") >>>>> 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 eda89a2..2d28e92 100644 >>>>> --- a/security/selinux/include/av_permissions.h >>>>> +++ b/security/selinux/include/av_permissions.h >>>>> @@ -99,6 +99,8 @@ >>>>> #define FILE__EXECUTE_NO_TRANS 0x00020000UL >>>>> #define FILE__ENTRYPOINT 0x00040000UL >>>>> #define FILE__EXECMOD 0x00080000UL >>>>> +#define FILE__SETUID 0x00100000UL >>>>> +#define FILE__SETGID 0x00200000UL >>>>> #define LNK_FILE__IOCTL 0x00000001UL >>>>> #define LNK_FILE__READ 0x00000002UL >>>>> #define LNK_FILE__WRITE 0x00000004UL >>>>> >>>>> >>>>> >>>>> -- >>>>> 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. I am thinking of the case where you define an application "login user" which has the ability to run setuid applications, but the administrator governs that he needs to go through sudo and run newrole to the webadm user. If while he is the webadm user he can create a setuid application, he can now circomvent the path. So maybe a bad example would be unconfined_t - unconfined_sudo_t - webadm_t If he is able to setuid some shell, he can then switch back to unconfined_t and run the shell to get through DAC permissions. I think as we move into constricting root administrator we are going to find other side paths for getting around these restrictions. I would argue that almost every confined domain, that currently has the ability to chmod on a file does not need the ability to set the setuid or gid bit. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.7 (GNU/Linux) Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org iD8DBQFG8CB7rlYvE4MpobMRAroYAJ9N0XIstPUj1HjgOW8dMdq61bG/EwCguAO+ L6UnymELGWZUA77QCz2SKtY= =pxin -----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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-09-18 19:01 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-09-17 18:01 rfc: kernel: special enforcing on suid and setgid chmod Eric Paris 2007-09-17 19:23 ` Stephen Smalley 2007-09-17 20:51 ` Daniel J Walsh 2007-09-18 16:58 ` Stephen Smalley 2007-09-18 19:01 ` Daniel J Walsh
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.