setgid hardening

If an unprivileged program opens a setgid file for write and passes the fd to a privileged program and the privileged program writes to it, we currently fail to clear the setgid bit.  Fix it by checking f_cred instead of current's creds whenever a struct file is involved.

Quoted 5 lines · Andy Lutomirski

If an unprivileged program opens a setgid file for write and passes the fd to a privileged program and the privileged program writes to it, we currently fail to clear the setgid bit. Fix it by checking f_cred instead of current's creds whenever a struct file is involved.

[...]

What if, instead, a privileged program passes the fd to an un unprivileged program? It sounds like a bad idea to start with, but at least currently the unprivileged program is going to clear the setgid bit when it writes. This change would make that behaviour more dangerous.

Perhaps there should be a capability check on both the current credentials and file credentials? (I realise that we've considered file credential checks to be sufficient elsewhere, but those cases involved virtual files with special semantics, where it's clearer that a privileged process should not pass them to an unprivileged process.)

Quoted 5 lines · Ben Hutchings

Quoted 4 lines · Andy Lutomirski

If an unprivileged program opens a setgid file for write and passes the fd to a privileged program and the privileged program writes to it, we currently fail to clear the setgid bit. Fix it by checking f_cred instead of current's creds whenever a struct file is involved.

Quoted 7 lines

[...]

What if, instead, a privileged program passes the fd to an un unprivileged program? It sounds like a bad idea to start with, but at least currently the unprivileged program is going to clear the setgid bit when it writes. This change would make that behaviour more dangerous.

Hmm. Although, if a privileged program does something like:

(sudo -u nobody echo blah) >setuid_program

presumably it wanted to make the change.

Quoted 6 lines

Perhaps there should be a capability check on both the current credentials and file credentials? (I realise that we've considered file credential checks to be sufficient elsewhere, but those cases involved virtual files with special semantics, where it's clearer that a privileged process should not pass them to an unprivileged process.)

I could go either way.

What I really want to do is to write a third patch that isn't for -stable that just removes the capable() check entirely. I'm reasonably confident it won't break things for a silly reason: because it's capable() and not ns_capable(), anything it would break would also be broken in an unprivileged container, and I haven't seen any reports of package managers or similar breaking for this reason.

Quoted 5 lines · Andy Lutomirski

If an unprivileged program opens a setgid file for write and passes the fd to a privileged program and the privileged program writes to it, we currently fail to clear the setgid bit. Fix it by checking f_cred instead of current's creds whenever a struct file is involved.

[...]

What if, instead, a privileged program passes the fd to an un unprivileged program? It sounds like a bad idea to start with, but at least currently the unprivileged program is going to clear the setgid bit when it writes. This change would make that behaviour more dangerous.

Perhaps there should be a capability check on both the current credentials and file credentials? (I realise that we've considered file credential checks to be sufficient elsewhere, but those cases involved virtual files with special semantics, where it's clearer that a privileged process should not pass them to an unprivileged process.)

Currently, if you open("foo", O_WRONLY | O_CREAT | ..., 02777) in a directory that is setgid and owned by a different gid than current's fsgid, you end up with an SGID executable that is owned by the directory's GID.  This is a Bad Thing (tm).  Exploiting this is nontrivial because most ways of creating a new file create an empty file and empty executables aren't particularly interesting, but this is nevertheless quite dangerous.

Harden against this type of attack by detecting this particular corner case (unprivileged program creates SGID executable inode in SGID directory owned by a different GID) and clearing the new inode's SGID bit.

Quoted 1 line

Signed-off-by Andy Lutomirski <[email protected]>

---
 fs/inode.c | 21 +++++++++++++++++++--

 1 file changed, 19 insertions(+), 2 deletions(-)

fs/inode.c +0 -0

index f7029c40cfbd..d7e4b80470dd 100644

Quoted 27 lines · Andy Lutomirski

Currently, if you open("foo", O_WRONLY | O_CREAT | ..., 02777) in a directory that is setgid and owned by a different gid than current's fsgid, you end up with an SGID executable that is owned by the directory's GID. This is a Bad Thing (tm). Exploiting this is nontrivial because most ways of creating a new file create an empty file and empty executables aren't particularly interesting, but this is nevertheless quite dangerous.

Harden against this type of attack by detecting this particular corner case (unprivileged program creates SGID executable inode in SGID directory owned by a different GID) and clearing the new inode's SGID bit.

Quoted 1 line

Signed-off-by Andy Lutomirski <[email protected]>

---
 fs/inode.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

fs/inode.c +1 -0

index f7029c40cfbd..d7e4b80470dd 100644
@@ -2007,11 +2007,28 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
 {
      inode->i_uid = current_fsuid();
      if (dir && dir->i_mode & S_ISGID) {
+             bool changing_gid = !gid_eq(inode->i_gid, dir->i_gid);

[...]

inode->i_gid hasn't been initialised yet. This should compare with current_fsgid(), shouldn't it?

Ben.

--
Ben Hutchings
It is easier to write an incorrect program than to understand a correct one.

Currently, if you open("foo", O_WRONLY | O_CREAT | ..., 02777) in a directory that is setgid and owned by a different gid than current's

end up with an SGID executable that is owned by the directory's GID. This

a Bad Thing (tm). Exploiting this is nontrivial because most ways of

new file create an empty file and empty executables aren't particularly interesting, but this is nevertheless quite dangerous.

Harden against this type of attack by detecting this particular corner

(unprivileged program creates SGID executable inode in SGID directory owned by a different GID) and clearing the new inode's SGID bit.

Currently, if you open("foo", O_WRONLY | O_CREAT | ..., 02777) in a directory that is setgid and owned by a different gid than current's fsgid, you end up with an SGID executable that is owned by the directory's GID. This is a Bad Thing (tm). Exploiting this is nontrivial because most ways of creating a new file create an empty file and empty executables aren't particularly interesting, but this is nevertheless quite dangerous.

Harden against this type of attack by detecting this particular corner case (unprivileged program creates SGID executable inode in SGID directory owned by a different GID) and clearing the new inode's SGID bit.

Signed-off-by Andy Lutomirski <[email protected]>

---
 fs/inode.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

fs/inode.c +12 -2

index f7029c40cfbd..d7e4b80470dd 100644
@@ -2007,11 +2007,28 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
 {
 	inode->i_uid = current_fsuid();
 	if (dir && dir->i_mode & S_ISGID) {
+		bool changing_gid = !gid_eq(inode->i_gid, dir->i_gid);
+
 		inode->i_gid = dir->i_gid;
-		if (S_ISDIR(mode))
+		if (S_ISDIR(mode)) {
 			mode |= S_ISGID;
-	} else
+		} else if (((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))
+			   && S_ISREG(mode) && changing_gid
+			   && !capable(CAP_FSETID)) {
+			/*
+			 * Whoa there!  An unprivileged program just
+			 * tried to create a new executable with SGID
+			 * set in a directory with SGID set that belongs
+			 * to a different group.  Don't let this program
+			 * create a SGID executable that ends up owned

The kernel has some dangerous behavior involving the creation and modification of setgid executables. These issues aren't kernel security bugs per se, but they have been used to turn various filesystem permission oddities into reliably privilege escalation exploits.

See http://www.halfdog.net/Security/2015/SetgidDirectoryPrivilegeEscalation/
for a nice writeup.

Let's fix them for real.