All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] setgid hardening
@ 2017-01-28  2:49 ` Andy Lutomirski
  0 siblings, 0 replies; 22+ messages in thread
From: Andy Lutomirski @ 2017-01-28  2:49 UTC (permalink / raw)
  To: security
  Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau,
	linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List,
	Jan Kara, Linux FS Devel, Frank Filz, Andy Lutomirski

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.

Changes from v1:
 - Fix uninitialized variable issue (Willy, Ben)
 - Also check current creds in should_remove_suid() (Ben)

Andy Lutomirski (2):
  fs: Check f_cred as well as of current's creds in should_remove_suid()
  fs: Harden against open(..., O_CREAT, 02777) in a setgid directory

 fs/inode.c         | 61 ++++++++++++++++++++++++++++++++++++++++++++++--------
 fs/internal.h      |  2 +-
 fs/ocfs2/file.c    |  4 ++--
 fs/open.c          |  2 +-
 include/linux/fs.h |  2 +-
 5 files changed, 57 insertions(+), 14 deletions(-)

-- 
2.9.3

^ permalink raw reply	[flat|nested] 22+ messages in thread
* [PATCH v2 0/2] setgid hardening
@ 2017-01-28  2:49 ` Andy Lutomirski
  0 siblings, 0 replies; 22+ messages in thread
From: Andy Lutomirski @ 2017-01-28  2:49 UTC (permalink / raw)
  To: security
  Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau,
	linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List,
	Jan Kara, Linux FS Devel, Frank Filz, Andy Lutomirski

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.

Changes from v1:
 - Fix uninitialized variable issue (Willy, Ben)
 - Also check current creds in should_remove_suid() (Ben)

Andy Lutomirski (2):
  fs: Check f_cred as well as of current's creds in should_remove_suid()
  fs: Harden against open(..., O_CREAT, 02777) in a setgid directory

 fs/inode.c         | 61 ++++++++++++++++++++++++++++++++++++++++++++++--------
 fs/internal.h      |  2 +-
 fs/ocfs2/file.c    |  4 ++--
 fs/open.c          |  2 +-
 include/linux/fs.h |  2 +-
 5 files changed, 57 insertions(+), 14 deletions(-)

-- 
2.9.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 22+ messages in thread
* [PATCH v2 1/2] fs: Check f_cred as well as of current's creds in should_remove_suid()
  2017-01-28  2:49 ` Andy Lutomirski
@ 2017-01-28  2:49   ` Andy Lutomirski
  -1 siblings, 0 replies; 22+ messages in thread
From: Andy Lutomirski @ 2017-01-28  2:49 UTC (permalink / raw)
  To: security
  Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau,
	linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List,
	Jan Kara, Linux FS Devel, Frank Filz, Andy Lutomirski, stable

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 in addition to current's creds whenever a struct file is
involved.

I'm checking both because I'm nervous about preserving the SUID and
SGID bits in any situation in which they're not currently preserved
and because Ben Hutchings suggested doing it this way.

I don't know why we check capabilities at all, and we could probably
get away with clearing the setgid bit regardless of capabilities,
but this change should be less likely to break some weird program.

This mitigates exploits that take advantage of world-writable setgid
files or directories.

Cc: stable@vger.kernel.org
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 fs/inode.c         | 37 +++++++++++++++++++++++++++++++------
 fs/internal.h      |  2 +-
 fs/ocfs2/file.c    |  4 ++--
 fs/open.c          |  2 +-
 include/linux/fs.h |  2 +-
 5 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 88110fd0b282..0e1e141b094c 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1733,8 +1733,12 @@ EXPORT_SYMBOL(touch_atime);
  *
  *	if suid or (sgid and xgrp)
  *		remove privs
+ *
+ * If a file is provided, we assume that this is write(), ftruncate() or
+ * similar on that file.  If a file is not provided, we assume that no
+ * file descriptor is involved (e.g. truncate()).
  */
-int should_remove_suid(struct dentry *dentry)
+int should_remove_suid(struct dentry *dentry, struct file *file)
 {
 	umode_t mode = d_inode(dentry)->i_mode;
 	int kill = 0;
@@ -1750,8 +1754,29 @@ int should_remove_suid(struct dentry *dentry)
 	if (unlikely((mode & S_ISGID) && (mode & S_IXGRP)))
 		kill |= ATTR_KILL_SGID;
 
-	if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode)))
-		return kill;
+	if (unlikely(kill && S_ISREG(mode))) {
+		/*
+		 * To minimize the degree to which this code works differently
+		 * from Linux 4.9 and below, we kill SUID/SGID if the writer
+		 * is unprivileged even if the file was opened by a privileged
+		 * process.  Yes, this is a hack and is a technical violation
+		 * of the "write(2) doesn't check current_cred()" rule.
+		 *
+		 * Ideally we would just kill the SUID bit regardless
+		 * of capabilities.
+		 */
+		if (!capable(CAP_FSETID))
+			return kill;
+
+		/*
+		 * To avoid abuse of stdout/stderr redirection, we need to
+		 * kill SUID/SGID if the file was opened by an unprivileged
+		 * task.
+		 */
+		if (file && file->f_cred != current_cred() &&
+		    !file_ns_capable(file, &init_user_ns, CAP_FSETID))
+			return kill;
+	}
 
 	return 0;
 }
@@ -1762,7 +1787,7 @@ EXPORT_SYMBOL(should_remove_suid);
  * response to write or truncate. Return 0 if nothing has to be changed.
  * Negative value on error (change should be denied).
  */
-int dentry_needs_remove_privs(struct dentry *dentry)
+int dentry_needs_remove_privs(struct dentry *dentry, struct file *file)
 {
 	struct inode *inode = d_inode(dentry);
 	int mask = 0;
@@ -1771,7 +1796,7 @@ int dentry_needs_remove_privs(struct dentry *dentry)
 	if (IS_NOSEC(inode))
 		return 0;
 
-	mask = should_remove_suid(dentry);
+	mask = should_remove_suid(dentry, file);
 	ret = security_inode_need_killpriv(dentry);
 	if (ret < 0)
 		return ret;
@@ -1807,7 +1832,7 @@ int file_remove_privs(struct file *file)
 	if (IS_NOSEC(inode))
 		return 0;
 
-	kill = dentry_needs_remove_privs(dentry);
+	kill = dentry_needs_remove_privs(dentry, file);
 	if (kill < 0)
 		return kill;
 	if (kill)
diff --git a/fs/internal.h b/fs/internal.h
index b63cf3af2dc2..c467ad502cac 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -119,7 +119,7 @@ extern struct file *filp_clone_open(struct file *);
  */
 extern long prune_icache_sb(struct super_block *sb, struct shrink_control *sc);
 extern void inode_add_lru(struct inode *inode);
-extern int dentry_needs_remove_privs(struct dentry *dentry);
+extern int dentry_needs_remove_privs(struct dentry *dentry, struct file *file);
 
 extern bool __atime_needs_update(const struct path *, struct inode *, bool);
 static inline bool atime_needs_update_rcu(const struct path *path,
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index c4889655d32b..db6efd940ac0 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -1903,7 +1903,7 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
 		}
 	}
 
-	if (file && should_remove_suid(file->f_path.dentry)) {
+	if (file && should_remove_suid(file->f_path.dentry, file)) {
 		ret = __ocfs2_write_remove_suid(inode, di_bh);
 		if (ret) {
 			mlog_errno(ret);
@@ -2132,7 +2132,7 @@ static int ocfs2_prepare_inode_for_write(struct file *file,
 		 * inode. There's also the dinode i_size state which
 		 * can be lost via setattr during extending writes (we
 		 * set inode->i_size at the end of a write. */
-		if (should_remove_suid(dentry)) {
+		if (should_remove_suid(dentry, file)) {
 			if (meta_level == 0) {
 				ocfs2_inode_unlock(inode, meta_level);
 				meta_level = 1;
diff --git a/fs/open.c b/fs/open.c
index d3ed8171e8e0..8f54f34d1e3e 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -52,7 +52,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
 	}
 
 	/* Remove suid, sgid, and file capabilities on truncate too */
-	ret = dentry_needs_remove_privs(dentry);
+	ret = dentry_needs_remove_privs(dentry, filp);
 	if (ret < 0)
 		return ret;
 	if (ret)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2ba074328894..87654fb21158 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2718,7 +2718,7 @@ extern void __destroy_inode(struct inode *);
 extern struct inode *new_inode_pseudo(struct super_block *sb);
 extern struct inode *new_inode(struct super_block *sb);
 extern void free_inode_nonrcu(struct inode *inode);
-extern int should_remove_suid(struct dentry *);
+extern int should_remove_suid(struct dentry *, struct file *);
 extern int file_remove_privs(struct file *);
 
 extern void __insert_inode_hash(struct inode *, unsigned long hashval);
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH v2 1/2] fs: Check f_cred as well as of current's creds in should_remove_suid()
@ 2017-01-28  2:49   ` Andy Lutomirski
  0 siblings, 0 replies; 22+ messages in thread
From: Andy Lutomirski @ 2017-01-28  2:49 UTC (permalink / raw)
  To: security
  Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau,
	linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List,
	Jan Kara, Linux FS Devel, Frank Filz, Andy Lutomirski, stable

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 in addition to current's creds whenever a struct file is
involved.

I'm checking both because I'm nervous about preserving the SUID and
SGID bits in any situation in which they're not currently preserved
and because Ben Hutchings suggested doing it this way.

I don't know why we check capabilities at all, and we could probably
get away with clearing the setgid bit regardless of capabilities,
but this change should be less likely to break some weird program.

This mitigates exploits that take advantage of world-writable setgid
files or directories.

Cc: stable@vger.kernel.org
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 fs/inode.c         | 37 +++++++++++++++++++++++++++++++------
 fs/internal.h      |  2 +-
 fs/ocfs2/file.c    |  4 ++--
 fs/open.c          |  2 +-
 include/linux/fs.h |  2 +-
 5 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 88110fd0b282..0e1e141b094c 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1733,8 +1733,12 @@ EXPORT_SYMBOL(touch_atime);
  *
  *	if suid or (sgid and xgrp)
  *		remove privs
+ *
+ * If a file is provided, we assume that this is write(), ftruncate() or
+ * similar on that file.  If a file is not provided, we assume that no
+ * file descriptor is involved (e.g. truncate()).
  */
-int should_remove_suid(struct dentry *dentry)
+int should_remove_suid(struct dentry *dentry, struct file *file)
 {
 	umode_t mode = d_inode(dentry)->i_mode;
 	int kill = 0;
@@ -1750,8 +1754,29 @@ int should_remove_suid(struct dentry *dentry)
 	if (unlikely((mode & S_ISGID) && (mode & S_IXGRP)))
 		kill |= ATTR_KILL_SGID;
 
-	if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode)))
-		return kill;
+	if (unlikely(kill && S_ISREG(mode))) {
+		/*
+		 * To minimize the degree to which this code works differently
+		 * from Linux 4.9 and below, we kill SUID/SGID if the writer
+		 * is unprivileged even if the file was opened by a privileged
+		 * process.  Yes, this is a hack and is a technical violation
+		 * of the "write(2) doesn't check current_cred()" rule.
+		 *
+		 * Ideally we would just kill the SUID bit regardless
+		 * of capabilities.
+		 */
+		if (!capable(CAP_FSETID))
+			return kill;
+
+		/*
+		 * To avoid abuse of stdout/stderr redirection, we need to
+		 * kill SUID/SGID if the file was opened by an unprivileged
+		 * task.
+		 */
+		if (file && file->f_cred != current_cred() &&
+		    !file_ns_capable(file, &init_user_ns, CAP_FSETID))
+			return kill;
+	}
 
 	return 0;
 }
@@ -1762,7 +1787,7 @@ EXPORT_SYMBOL(should_remove_suid);
  * response to write or truncate. Return 0 if nothing has to be changed.
  * Negative value on error (change should be denied).
  */
-int dentry_needs_remove_privs(struct dentry *dentry)
+int dentry_needs_remove_privs(struct dentry *dentry, struct file *file)
 {
 	struct inode *inode = d_inode(dentry);
 	int mask = 0;
@@ -1771,7 +1796,7 @@ int dentry_needs_remove_privs(struct dentry *dentry)
 	if (IS_NOSEC(inode))
 		return 0;
 
-	mask = should_remove_suid(dentry);
+	mask = should_remove_suid(dentry, file);
 	ret = security_inode_need_killpriv(dentry);
 	if (ret < 0)
 		return ret;
@@ -1807,7 +1832,7 @@ int file_remove_privs(struct file *file)
 	if (IS_NOSEC(inode))
 		return 0;
 
-	kill = dentry_needs_remove_privs(dentry);
+	kill = dentry_needs_remove_privs(dentry, file);
 	if (kill < 0)
 		return kill;
 	if (kill)
diff --git a/fs/internal.h b/fs/internal.h
index b63cf3af2dc2..c467ad502cac 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -119,7 +119,7 @@ extern struct file *filp_clone_open(struct file *);
  */
 extern long prune_icache_sb(struct super_block *sb, struct shrink_control *sc);
 extern void inode_add_lru(struct inode *inode);
-extern int dentry_needs_remove_privs(struct dentry *dentry);
+extern int dentry_needs_remove_privs(struct dentry *dentry, struct file *file);
 
 extern bool __atime_needs_update(const struct path *, struct inode *, bool);
 static inline bool atime_needs_update_rcu(const struct path *path,
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index c4889655d32b..db6efd940ac0 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -1903,7 +1903,7 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
 		}
 	}
 
-	if (file && should_remove_suid(file->f_path.dentry)) {
+	if (file && should_remove_suid(file->f_path.dentry, file)) {
 		ret = __ocfs2_write_remove_suid(inode, di_bh);
 		if (ret) {
 			mlog_errno(ret);
@@ -2132,7 +2132,7 @@ static int ocfs2_prepare_inode_for_write(struct file *file,
 		 * inode. There's also the dinode i_size state which
 		 * can be lost via setattr during extending writes (we
 		 * set inode->i_size at the end of a write. */
-		if (should_remove_suid(dentry)) {
+		if (should_remove_suid(dentry, file)) {
 			if (meta_level == 0) {
 				ocfs2_inode_unlock(inode, meta_level);
 				meta_level = 1;
diff --git a/fs/open.c b/fs/open.c
index d3ed8171e8e0..8f54f34d1e3e 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -52,7 +52,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
 	}
 
 	/* Remove suid, sgid, and file capabilities on truncate too */
-	ret = dentry_needs_remove_privs(dentry);
+	ret = dentry_needs_remove_privs(dentry, filp);
 	if (ret < 0)
 		return ret;
 	if (ret)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2ba074328894..87654fb21158 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2718,7 +2718,7 @@ extern void __destroy_inode(struct inode *);
 extern struct inode *new_inode_pseudo(struct super_block *sb);
 extern struct inode *new_inode(struct super_block *sb);
 extern void free_inode_nonrcu(struct inode *inode);
-extern int should_remove_suid(struct dentry *);
+extern int should_remove_suid(struct dentry *, struct file *);
 extern int file_remove_privs(struct file *);
 
 extern void __insert_inode_hash(struct inode *, unsigned long hashval);
-- 
2.9.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH v2 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory
  2017-01-28  2:49 ` Andy Lutomirski
@ 2017-01-28  2:49   ` Andy Lutomirski
  -1 siblings, 0 replies; 22+ messages in thread
From: Andy Lutomirski @ 2017-01-28  2:49 UTC (permalink / raw)
  To: security
  Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau,
	linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List,
	Jan Kara, Linux FS Devel, Frank Filz, Andy Lutomirski, stable

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.

Cc: stable@vger.kernel.org
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 fs/inode.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 0e1e141b094c..f6acb9232263 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2025,12 +2025,30 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
 			umode_t mode)
 {
 	inode->i_uid = current_fsuid();
+	inode->i_gid = current_fsgid();
+
 	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
-		inode->i_gid = current_fsgid();
+		} 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
+			 * by the wrong group.
+			 */
+			mode &= ~S_ISGID;
+		}
+	}
+
 	inode->i_mode = mode;
 }
 EXPORT_SYMBOL(inode_init_owner);
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH v2 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory
@ 2017-01-28  2:49   ` Andy Lutomirski
  0 siblings, 0 replies; 22+ messages in thread
From: Andy Lutomirski @ 2017-01-28  2:49 UTC (permalink / raw)
  To: security
  Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau,
	linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List,
	Jan Kara, Linux FS Devel, Frank Filz, Andy Lutomirski, stable

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.

Cc: stable@vger.kernel.org
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 fs/inode.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 0e1e141b094c..f6acb9232263 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2025,12 +2025,30 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
 			umode_t mode)
 {
 	inode->i_uid = current_fsuid();
+	inode->i_gid = current_fsgid();
+
 	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
-		inode->i_gid = current_fsgid();
+		} 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
+			 * by the wrong group.
+			 */
+			mode &= ~S_ISGID;
+		}
+	}
+
 	inode->i_mode = mode;
 }
 EXPORT_SYMBOL(inode_init_owner);
-- 
2.9.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 0/2] setgid hardening
  2017-01-28  2:49 ` Andy Lutomirski
@ 2017-01-31  3:49   ` Michael Kerrisk
  -1 siblings, 0 replies; 22+ messages in thread
From: Michael Kerrisk @ 2017-01-31  3:49 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: security, Konstantin Khlebnikov, Alexander Viro, Kees Cook,
	Willy Tarreau, linux-mm, Andrew Morton, yalin wang,
	Linux Kernel Mailing List, Jan Kara, Linux FS Devel, Frank Filz,
	Linux API

[CC += linux-api@]

Andy, this is an API change!

On Sat, Jan 28, 2017 at 3:49 PM, Andy Lutomirski <luto@kernel.org> wrote:
> 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.
>
> Changes from v1:
>  - Fix uninitialized variable issue (Willy, Ben)
>  - Also check current creds in should_remove_suid() (Ben)
>
> Andy Lutomirski (2):
>   fs: Check f_cred as well as of current's creds in should_remove_suid()
>   fs: Harden against open(..., O_CREAT, 02777) in a setgid directory
>
>  fs/inode.c         | 61 ++++++++++++++++++++++++++++++++++++++++++++++--------
>  fs/internal.h      |  2 +-
>  fs/ocfs2/file.c    |  4 ++--
>  fs/open.c          |  2 +-
>  include/linux/fs.h |  2 +-
>  5 files changed, 57 insertions(+), 14 deletions(-)
>
> --
> 2.9.3
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>



-- 
Michael Kerrisk Linux man-pages maintainer;
http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface", http://blog.man7.org/

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 0/2] setgid hardening
@ 2017-01-31  3:49   ` Michael Kerrisk
  0 siblings, 0 replies; 22+ messages in thread
From: Michael Kerrisk @ 2017-01-31  3:49 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: security, Konstantin Khlebnikov, Alexander Viro, Kees Cook,
	Willy Tarreau, linux-mm, Andrew Morton, yalin wang,
	Linux Kernel Mailing List, Jan Kara, Linux FS Devel, Frank Filz,
	Linux API

[CC += linux-api@]

Andy, this is an API change!

On Sat, Jan 28, 2017 at 3:49 PM, Andy Lutomirski <luto@kernel.org> wrote:
> 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.
>
> Changes from v1:
>  - Fix uninitialized variable issue (Willy, Ben)
>  - Also check current creds in should_remove_suid() (Ben)
>
> Andy Lutomirski (2):
>   fs: Check f_cred as well as of current's creds in should_remove_suid()
>   fs: Harden against open(..., O_CREAT, 02777) in a setgid directory
>
>  fs/inode.c         | 61 ++++++++++++++++++++++++++++++++++++++++++++++--------
>  fs/internal.h      |  2 +-
>  fs/ocfs2/file.c    |  4 ++--
>  fs/open.c          |  2 +-
>  include/linux/fs.h |  2 +-
>  5 files changed, 57 insertions(+), 14 deletions(-)
>
> --
> 2.9.3
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>



-- 
Michael Kerrisk Linux man-pages maintainer;
http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface", http://blog.man7.org/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/2] fs: Check f_cred as well as of current's creds in should_remove_suid()
  2017-01-28  2:49   ` Andy Lutomirski
@ 2017-01-31  3:50     ` Michael Kerrisk
  -1 siblings, 0 replies; 22+ messages in thread
From: Michael Kerrisk @ 2017-01-31  3:50 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: security, Konstantin Khlebnikov, Alexander Viro, Kees Cook,
	Willy Tarreau, linux-mm, Andrew Morton, yalin wang,
	Linux Kernel Mailing List, Jan Kara, Linux FS Devel, Frank Filz,
	stable, Linux API

[CC += linux-api@]

On Sat, Jan 28, 2017 at 3:49 PM, Andy Lutomirski <luto@kernel.org> wrote:
> 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 in addition to current's creds whenever a struct file is
> involved.
>
> I'm checking both because I'm nervous about preserving the SUID and
> SGID bits in any situation in which they're not currently preserved
> and because Ben Hutchings suggested doing it this way.
>
> I don't know why we check capabilities at all, and we could probably
> get away with clearing the setgid bit regardless of capabilities,
> but this change should be less likely to break some weird program.
>
> This mitigates exploits that take advantage of world-writable setgid
> files or directories.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  fs/inode.c         | 37 +++++++++++++++++++++++++++++++------
>  fs/internal.h      |  2 +-
>  fs/ocfs2/file.c    |  4 ++--
>  fs/open.c          |  2 +-
>  include/linux/fs.h |  2 +-
>  5 files changed, 36 insertions(+), 11 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 88110fd0b282..0e1e141b094c 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1733,8 +1733,12 @@ EXPORT_SYMBOL(touch_atime);
>   *
>   *     if suid or (sgid and xgrp)
>   *             remove privs
> + *
> + * If a file is provided, we assume that this is write(), ftruncate() or
> + * similar on that file.  If a file is not provided, we assume that no
> + * file descriptor is involved (e.g. truncate()).
>   */
> -int should_remove_suid(struct dentry *dentry)
> +int should_remove_suid(struct dentry *dentry, struct file *file)
>  {
>         umode_t mode = d_inode(dentry)->i_mode;
>         int kill = 0;
> @@ -1750,8 +1754,29 @@ int should_remove_suid(struct dentry *dentry)
>         if (unlikely((mode & S_ISGID) && (mode & S_IXGRP)))
>                 kill |= ATTR_KILL_SGID;
>
> -       if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode)))
> -               return kill;
> +       if (unlikely(kill && S_ISREG(mode))) {
> +               /*
> +                * To minimize the degree to which this code works differently
> +                * from Linux 4.9 and below, we kill SUID/SGID if the writer
> +                * is unprivileged even if the file was opened by a privileged
> +                * process.  Yes, this is a hack and is a technical violation
> +                * of the "write(2) doesn't check current_cred()" rule.
> +                *
> +                * Ideally we would just kill the SUID bit regardless
> +                * of capabilities.
> +                */
> +               if (!capable(CAP_FSETID))
> +                       return kill;
> +
> +               /*
> +                * To avoid abuse of stdout/stderr redirection, we need to
> +                * kill SUID/SGID if the file was opened by an unprivileged
> +                * task.
> +                */
> +               if (file && file->f_cred != current_cred() &&
> +                   !file_ns_capable(file, &init_user_ns, CAP_FSETID))
> +                       return kill;
> +       }
>
>         return 0;
>  }
> @@ -1762,7 +1787,7 @@ EXPORT_SYMBOL(should_remove_suid);
>   * response to write or truncate. Return 0 if nothing has to be changed.
>   * Negative value on error (change should be denied).
>   */
> -int dentry_needs_remove_privs(struct dentry *dentry)
> +int dentry_needs_remove_privs(struct dentry *dentry, struct file *file)
>  {
>         struct inode *inode = d_inode(dentry);
>         int mask = 0;
> @@ -1771,7 +1796,7 @@ int dentry_needs_remove_privs(struct dentry *dentry)
>         if (IS_NOSEC(inode))
>                 return 0;
>
> -       mask = should_remove_suid(dentry);
> +       mask = should_remove_suid(dentry, file);
>         ret = security_inode_need_killpriv(dentry);
>         if (ret < 0)
>                 return ret;
> @@ -1807,7 +1832,7 @@ int file_remove_privs(struct file *file)
>         if (IS_NOSEC(inode))
>                 return 0;
>
> -       kill = dentry_needs_remove_privs(dentry);
> +       kill = dentry_needs_remove_privs(dentry, file);
>         if (kill < 0)
>                 return kill;
>         if (kill)
> diff --git a/fs/internal.h b/fs/internal.h
> index b63cf3af2dc2..c467ad502cac 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -119,7 +119,7 @@ extern struct file *filp_clone_open(struct file *);
>   */
>  extern long prune_icache_sb(struct super_block *sb, struct shrink_control *sc);
>  extern void inode_add_lru(struct inode *inode);
> -extern int dentry_needs_remove_privs(struct dentry *dentry);
> +extern int dentry_needs_remove_privs(struct dentry *dentry, struct file *file);
>
>  extern bool __atime_needs_update(const struct path *, struct inode *, bool);
>  static inline bool atime_needs_update_rcu(const struct path *path,
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index c4889655d32b..db6efd940ac0 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -1903,7 +1903,7 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
>                 }
>         }
>
> -       if (file && should_remove_suid(file->f_path.dentry)) {
> +       if (file && should_remove_suid(file->f_path.dentry, file)) {
>                 ret = __ocfs2_write_remove_suid(inode, di_bh);
>                 if (ret) {
>                         mlog_errno(ret);
> @@ -2132,7 +2132,7 @@ static int ocfs2_prepare_inode_for_write(struct file *file,
>                  * inode. There's also the dinode i_size state which
>                  * can be lost via setattr during extending writes (we
>                  * set inode->i_size at the end of a write. */
> -               if (should_remove_suid(dentry)) {
> +               if (should_remove_suid(dentry, file)) {
>                         if (meta_level == 0) {
>                                 ocfs2_inode_unlock(inode, meta_level);
>                                 meta_level = 1;
> diff --git a/fs/open.c b/fs/open.c
> index d3ed8171e8e0..8f54f34d1e3e 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -52,7 +52,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
>         }
>
>         /* Remove suid, sgid, and file capabilities on truncate too */
> -       ret = dentry_needs_remove_privs(dentry);
> +       ret = dentry_needs_remove_privs(dentry, filp);
>         if (ret < 0)
>                 return ret;
>         if (ret)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2ba074328894..87654fb21158 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2718,7 +2718,7 @@ extern void __destroy_inode(struct inode *);
>  extern struct inode *new_inode_pseudo(struct super_block *sb);
>  extern struct inode *new_inode(struct super_block *sb);
>  extern void free_inode_nonrcu(struct inode *inode);
> -extern int should_remove_suid(struct dentry *);
> +extern int should_remove_suid(struct dentry *, struct file *);
>  extern int file_remove_privs(struct file *);
>
>  extern void __insert_inode_hash(struct inode *, unsigned long hashval);
> --
> 2.9.3
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>



-- 
Michael Kerrisk Linux man-pages maintainer;
http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface", http://blog.man7.org/

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/2] fs: Check f_cred as well as of current's creds in should_remove_suid()
@ 2017-01-31  3:50     ` Michael Kerrisk
  0 siblings, 0 replies; 22+ messages in thread
From: Michael Kerrisk @ 2017-01-31  3:50 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: security, Konstantin Khlebnikov, Alexander Viro, Kees Cook,
	Willy Tarreau, linux-mm, Andrew Morton, yalin wang,
	Linux Kernel Mailing List, Jan Kara, Linux FS Devel, Frank Filz,
	stable, Linux API

[CC += linux-api@]

On Sat, Jan 28, 2017 at 3:49 PM, Andy Lutomirski <luto@kernel.org> wrote:
> 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 in addition to current's creds whenever a struct file is
> involved.
>
> I'm checking both because I'm nervous about preserving the SUID and
> SGID bits in any situation in which they're not currently preserved
> and because Ben Hutchings suggested doing it this way.
>
> I don't know why we check capabilities at all, and we could probably
> get away with clearing the setgid bit regardless of capabilities,
> but this change should be less likely to break some weird program.
>
> This mitigates exploits that take advantage of world-writable setgid
> files or directories.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  fs/inode.c         | 37 +++++++++++++++++++++++++++++++------
>  fs/internal.h      |  2 +-
>  fs/ocfs2/file.c    |  4 ++--
>  fs/open.c          |  2 +-
>  include/linux/fs.h |  2 +-
>  5 files changed, 36 insertions(+), 11 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 88110fd0b282..0e1e141b094c 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1733,8 +1733,12 @@ EXPORT_SYMBOL(touch_atime);
>   *
>   *     if suid or (sgid and xgrp)
>   *             remove privs
> + *
> + * If a file is provided, we assume that this is write(), ftruncate() or
> + * similar on that file.  If a file is not provided, we assume that no
> + * file descriptor is involved (e.g. truncate()).
>   */
> -int should_remove_suid(struct dentry *dentry)
> +int should_remove_suid(struct dentry *dentry, struct file *file)
>  {
>         umode_t mode = d_inode(dentry)->i_mode;
>         int kill = 0;
> @@ -1750,8 +1754,29 @@ int should_remove_suid(struct dentry *dentry)
>         if (unlikely((mode & S_ISGID) && (mode & S_IXGRP)))
>                 kill |= ATTR_KILL_SGID;
>
> -       if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode)))
> -               return kill;
> +       if (unlikely(kill && S_ISREG(mode))) {
> +               /*
> +                * To minimize the degree to which this code works differently
> +                * from Linux 4.9 and below, we kill SUID/SGID if the writer
> +                * is unprivileged even if the file was opened by a privileged
> +                * process.  Yes, this is a hack and is a technical violation
> +                * of the "write(2) doesn't check current_cred()" rule.
> +                *
> +                * Ideally we would just kill the SUID bit regardless
> +                * of capabilities.
> +                */
> +               if (!capable(CAP_FSETID))
> +                       return kill;
> +
> +               /*
> +                * To avoid abuse of stdout/stderr redirection, we need to
> +                * kill SUID/SGID if the file was opened by an unprivileged
> +                * task.
> +                */
> +               if (file && file->f_cred != current_cred() &&
> +                   !file_ns_capable(file, &init_user_ns, CAP_FSETID))
> +                       return kill;
> +       }
>
>         return 0;
>  }
> @@ -1762,7 +1787,7 @@ EXPORT_SYMBOL(should_remove_suid);
>   * response to write or truncate. Return 0 if nothing has to be changed.
>   * Negative value on error (change should be denied).
>   */
> -int dentry_needs_remove_privs(struct dentry *dentry)
> +int dentry_needs_remove_privs(struct dentry *dentry, struct file *file)
>  {
>         struct inode *inode = d_inode(dentry);
>         int mask = 0;
> @@ -1771,7 +1796,7 @@ int dentry_needs_remove_privs(struct dentry *dentry)
>         if (IS_NOSEC(inode))
>                 return 0;
>
> -       mask = should_remove_suid(dentry);
> +       mask = should_remove_suid(dentry, file);
>         ret = security_inode_need_killpriv(dentry);
>         if (ret < 0)
>                 return ret;
> @@ -1807,7 +1832,7 @@ int file_remove_privs(struct file *file)
>         if (IS_NOSEC(inode))
>                 return 0;
>
> -       kill = dentry_needs_remove_privs(dentry);
> +       kill = dentry_needs_remove_privs(dentry, file);
>         if (kill < 0)
>                 return kill;
>         if (kill)
> diff --git a/fs/internal.h b/fs/internal.h
> index b63cf3af2dc2..c467ad502cac 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -119,7 +119,7 @@ extern struct file *filp_clone_open(struct file *);
>   */
>  extern long prune_icache_sb(struct super_block *sb, struct shrink_control *sc);
>  extern void inode_add_lru(struct inode *inode);
> -extern int dentry_needs_remove_privs(struct dentry *dentry);
> +extern int dentry_needs_remove_privs(struct dentry *dentry, struct file *file);
>
>  extern bool __atime_needs_update(const struct path *, struct inode *, bool);
>  static inline bool atime_needs_update_rcu(const struct path *path,
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index c4889655d32b..db6efd940ac0 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -1903,7 +1903,7 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
>                 }
>         }
>
> -       if (file && should_remove_suid(file->f_path.dentry)) {
> +       if (file && should_remove_suid(file->f_path.dentry, file)) {
>                 ret = __ocfs2_write_remove_suid(inode, di_bh);
>                 if (ret) {
>                         mlog_errno(ret);
> @@ -2132,7 +2132,7 @@ static int ocfs2_prepare_inode_for_write(struct file *file,
>                  * inode. There's also the dinode i_size state which
>                  * can be lost via setattr during extending writes (we
>                  * set inode->i_size at the end of a write. */
> -               if (should_remove_suid(dentry)) {
> +               if (should_remove_suid(dentry, file)) {
>                         if (meta_level == 0) {
>                                 ocfs2_inode_unlock(inode, meta_level);
>                                 meta_level = 1;
> diff --git a/fs/open.c b/fs/open.c
> index d3ed8171e8e0..8f54f34d1e3e 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -52,7 +52,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
>         }
>
>         /* Remove suid, sgid, and file capabilities on truncate too */
> -       ret = dentry_needs_remove_privs(dentry);
> +       ret = dentry_needs_remove_privs(dentry, filp);
>         if (ret < 0)
>                 return ret;
>         if (ret)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2ba074328894..87654fb21158 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2718,7 +2718,7 @@ extern void __destroy_inode(struct inode *);
>  extern struct inode *new_inode_pseudo(struct super_block *sb);
>  extern struct inode *new_inode(struct super_block *sb);
>  extern void free_inode_nonrcu(struct inode *inode);
> -extern int should_remove_suid(struct dentry *);
> +extern int should_remove_suid(struct dentry *, struct file *);
>  extern int file_remove_privs(struct file *);
>
>  extern void __insert_inode_hash(struct inode *, unsigned long hashval);
> --
> 2.9.3
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>



-- 
Michael Kerrisk Linux man-pages maintainer;
http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface", http://blog.man7.org/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory
  2017-01-28  2:49   ` Andy Lutomirski
@ 2017-01-31  3:50     ` Michael Kerrisk
  -1 siblings, 0 replies; 22+ messages in thread
From: Michael Kerrisk @ 2017-01-31  3:50 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: security, Konstantin Khlebnikov, Alexander Viro, Kees Cook,
	Willy Tarreau, linux-mm, Andrew Morton, yalin wang,
	Linux Kernel Mailing List, Jan Kara, Linux FS Devel, Frank Filz,
	stable, Linux API

[CC += linux-api@]

On Sat, Jan 28, 2017 at 3:49 PM, Andy Lutomirski <luto@kernel.org> wrote:
> 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.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  fs/inode.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 0e1e141b094c..f6acb9232263 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2025,12 +2025,30 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
>                         umode_t mode)
>  {
>         inode->i_uid = current_fsuid();
> +       inode->i_gid = current_fsgid();
> +
>         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
> -               inode->i_gid = current_fsgid();
> +               } 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
> +                        * by the wrong group.
> +                        */
> +                       mode &= ~S_ISGID;
> +               }
> +       }
> +
>         inode->i_mode = mode;
>  }
>  EXPORT_SYMBOL(inode_init_owner);
> --
> 2.9.3
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>



-- 
Michael Kerrisk Linux man-pages maintainer;
http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface", http://blog.man7.org/

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory
@ 2017-01-31  3:50     ` Michael Kerrisk
  0 siblings, 0 replies; 22+ messages in thread
From: Michael Kerrisk @ 2017-01-31  3:50 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: security, Konstantin Khlebnikov, Alexander Viro, Kees Cook,
	Willy Tarreau, linux-mm, Andrew Morton, yalin wang,
	Linux Kernel Mailing List, Jan Kara, Linux FS Devel, Frank Filz,
	stable, Linux API

[CC += linux-api@]

On Sat, Jan 28, 2017 at 3:49 PM, Andy Lutomirski <luto@kernel.org> wrote:
> 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.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  fs/inode.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 0e1e141b094c..f6acb9232263 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2025,12 +2025,30 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
>                         umode_t mode)
>  {
>         inode->i_uid = current_fsuid();
> +       inode->i_gid = current_fsgid();
> +
>         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
> -               inode->i_gid = current_fsgid();
> +               } 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
> +                        * by the wrong group.
> +                        */
> +                       mode &= ~S_ISGID;
> +               }
> +       }
> +
>         inode->i_mode = mode;
>  }
>  EXPORT_SYMBOL(inode_init_owner);
> --
> 2.9.3
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>



-- 
Michael Kerrisk Linux man-pages maintainer;
http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface", http://blog.man7.org/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 0/2] setgid hardening
  2017-01-31  3:49   ` Michael Kerrisk
@ 2017-01-31  3:56     ` Andy Lutomirski
  -1 siblings, 0 replies; 22+ messages in thread
From: Andy Lutomirski @ 2017-01-31  3:56 UTC (permalink / raw)
  To: Michael Kerrisk
  Cc: Andy Lutomirski, security, Konstantin Khlebnikov, Alexander Viro,
	Kees Cook, Willy Tarreau, linux-mm, Andrew Morton, yalin wang,
	Linux Kernel Mailing List, Jan Kara, Linux FS Devel, Frank Filz,
	Linux API

On Mon, Jan 30, 2017 at 7:49 PM, Michael Kerrisk <mtk.manpages@gmail.com> wrote:
> [CC += linux-api@]
>
> Andy, this is an API change!

Indeed.  I should be ashamed of myself!

>
> On Sat, Jan 28, 2017 at 3:49 PM, Andy Lutomirski <luto@kernel.org> wrote:
>> 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.
>>
>> Changes from v1:
>>  - Fix uninitialized variable issue (Willy, Ben)
>>  - Also check current creds in should_remove_suid() (Ben)
>>
>> Andy Lutomirski (2):
>>   fs: Check f_cred as well as of current's creds in should_remove_suid()
>>   fs: Harden against open(..., O_CREAT, 02777) in a setgid directory
>>
>>  fs/inode.c         | 61 ++++++++++++++++++++++++++++++++++++++++++++++--------
>>  fs/internal.h      |  2 +-
>>  fs/ocfs2/file.c    |  4 ++--
>>  fs/open.c          |  2 +-
>>  include/linux/fs.h |  2 +-
>>  5 files changed, 57 insertions(+), 14 deletions(-)
>>
>> --
>> 2.9.3
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>
>
>
> --
> Michael Kerrisk Linux man-pages maintainer;
> http://www.kernel.org/doc/man-pages/
> Author of "The Linux Programming Interface", http://blog.man7.org/



-- 
Andy Lutomirski
AMA Capital Management, LLC

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 0/2] setgid hardening
@ 2017-01-31  3:56     ` Andy Lutomirski
  0 siblings, 0 replies; 22+ messages in thread
From: Andy Lutomirski @ 2017-01-31  3:56 UTC (permalink / raw)
  To: Michael Kerrisk
  Cc: Andy Lutomirski, security, Konstantin Khlebnikov, Alexander Viro,
	Kees Cook, Willy Tarreau, linux-mm, Andrew Morton, yalin wang,
	Linux Kernel Mailing List, Jan Kara, Linux FS Devel, Frank Filz,
	Linux API

On Mon, Jan 30, 2017 at 7:49 PM, Michael Kerrisk <mtk.manpages@gmail.com> wrote:
> [CC += linux-api@]
>
> Andy, this is an API change!

Indeed.  I should be ashamed of myself!

>
> On Sat, Jan 28, 2017 at 3:49 PM, Andy Lutomirski <luto@kernel.org> wrote:
>> 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.
>>
>> Changes from v1:
>>  - Fix uninitialized variable issue (Willy, Ben)
>>  - Also check current creds in should_remove_suid() (Ben)
>>
>> Andy Lutomirski (2):
>>   fs: Check f_cred as well as of current's creds in should_remove_suid()
>>   fs: Harden against open(..., O_CREAT, 02777) in a setgid directory
>>
>>  fs/inode.c         | 61 ++++++++++++++++++++++++++++++++++++++++++++++--------
>>  fs/internal.h      |  2 +-
>>  fs/ocfs2/file.c    |  4 ++--
>>  fs/open.c          |  2 +-
>>  include/linux/fs.h |  2 +-
>>  5 files changed, 57 insertions(+), 14 deletions(-)
>>
>> --
>> 2.9.3
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>
>
>
> --
> Michael Kerrisk Linux man-pages maintainer;
> http://www.kernel.org/doc/man-pages/
> Author of "The Linux Programming Interface", http://blog.man7.org/



-- 
Andy Lutomirski
AMA Capital Management, LLC

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/2] fs: Check f_cred as well as of current's creds in should_remove_suid()
  2017-01-28  2:49   ` Andy Lutomirski
  (?)
@ 2017-01-31 11:43     ` Jeff Layton
  -1 siblings, 0 replies; 22+ messages in thread
From: Jeff Layton @ 2017-01-31 11:43 UTC (permalink / raw)
  To: Andy Lutomirski, security
  Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau,
	linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List,
	Jan Kara, Linux FS Devel, Frank Filz, stable

On Fri, 2017-01-27 at 18:49 -0800, Andy Lutomirski wrote:
> 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 in addition to current's creds whenever a struct file is
> involved.
> 
> I'm checking both because I'm nervous about preserving the SUID and
> SGID bits in any situation in which they're not currently preserved
> and because Ben Hutchings suggested doing it this way.
> 
> I don't know why we check capabilities at all, and we could probably
> get away with clearing the setgid bit regardless of capabilities,
> but this change should be less likely to break some weird program.
> 
> This mitigates exploits that take advantage of world-writable setgid
> files or directories.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  fs/inode.c         | 37 +++++++++++++++++++++++++++++++------
>  fs/internal.h      |  2 +-
>  fs/ocfs2/file.c    |  4 ++--
>  fs/open.c          |  2 +-
>  include/linux/fs.h |  2 +-
>  5 files changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 88110fd0b282..0e1e141b094c 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1733,8 +1733,12 @@ EXPORT_SYMBOL(touch_atime);
>   *
>   *	if suid or (sgid and xgrp)
>   *		remove privs
> + *
> + * If a file is provided, we assume that this is write(), ftruncate() or
> + * similar on that file.  If a file is not provided, we assume that no
> + * file descriptor is involved (e.g. truncate()).
>   */
> -int should_remove_suid(struct dentry *dentry)
> +int should_remove_suid(struct dentry *dentry, struct file *file)
>  {
>  	umode_t mode = d_inode(dentry)->i_mode;
>  	int kill = 0;
> @@ -1750,8 +1754,29 @@ int should_remove_suid(struct dentry *dentry)
>  	if (unlikely((mode & S_ISGID) && (mode & S_IXGRP)))
>  		kill |= ATTR_KILL_SGID;
>  
> -	if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode)))
> -		return kill;
> +	if (unlikely(kill && S_ISREG(mode))) {
> +		/*
> +		 * To minimize the degree to which this code works differently
> +		 * from Linux 4.9 and below, we kill SUID/SGID if the writer
> +		 * is unprivileged even if the file was opened by a privileged
> +		 * process.  Yes, this is a hack and is a technical violation
> +		 * of the "write(2) doesn't check current_cred()" rule.
> +		 *
> +		 * Ideally we would just kill the SUID bit regardless
> +		 * of capabilities.
> +		 */
> +		if (!capable(CAP_FSETID))
> +			return kill;
> +
> +		/*
> +		 * To avoid abuse of stdout/stderr redirection, we need to
> +		 * kill SUID/SGID if the file was opened by an unprivileged
> +		 * task.
> +		 */
> +		if (file && file->f_cred != current_cred() &&
> +		    !file_ns_capable(file, &init_user_ns, CAP_FSETID))
> +			return kill;
> +	}
>  
>  	return 0;
>  }
> @@ -1762,7 +1787,7 @@ EXPORT_SYMBOL(should_remove_suid);
>   * response to write or truncate. Return 0 if nothing has to be changed.
>   * Negative value on error (change should be denied).
>   */
> -int dentry_needs_remove_privs(struct dentry *dentry)
> +int dentry_needs_remove_privs(struct dentry *dentry, struct file *file)
>  {
>  	struct inode *inode = d_inode(dentry);
>  	int mask = 0;
> @@ -1771,7 +1796,7 @@ int dentry_needs_remove_privs(struct dentry *dentry)
>  	if (IS_NOSEC(inode))
>  		return 0;
>  
> -	mask = should_remove_suid(dentry);
> +	mask = should_remove_suid(dentry, file);
>  	ret = security_inode_need_killpriv(dentry);
>  	if (ret < 0)
>  		return ret;
> @@ -1807,7 +1832,7 @@ int file_remove_privs(struct file *file)
>  	if (IS_NOSEC(inode))
>  		return 0;
>  
> -	kill = dentry_needs_remove_privs(dentry);
> +	kill = dentry_needs_remove_privs(dentry, file);
>  	if (kill < 0)
>  		return kill;
>  	if (kill)
> diff --git a/fs/internal.h b/fs/internal.h
> index b63cf3af2dc2..c467ad502cac 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -119,7 +119,7 @@ extern struct file *filp_clone_open(struct file *);
>   */
>  extern long prune_icache_sb(struct super_block *sb, struct shrink_control *sc);
>  extern void inode_add_lru(struct inode *inode);
> -extern int dentry_needs_remove_privs(struct dentry *dentry);
> +extern int dentry_needs_remove_privs(struct dentry *dentry, struct file *file);
>  
>  extern bool __atime_needs_update(const struct path *, struct inode *, bool);
>  static inline bool atime_needs_update_rcu(const struct path *path,
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index c4889655d32b..db6efd940ac0 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -1903,7 +1903,7 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
>  		}
>  	}
>  
> -	if (file && should_remove_suid(file->f_path.dentry)) {
> +	if (file && should_remove_suid(file->f_path.dentry, file)) {
>  		ret = __ocfs2_write_remove_suid(inode, di_bh);
>  		if (ret) {
>  			mlog_errno(ret);
> @@ -2132,7 +2132,7 @@ static int ocfs2_prepare_inode_for_write(struct file *file,
>  		 * inode. There's also the dinode i_size state which
>  		 * can be lost via setattr during extending writes (we
>  		 * set inode->i_size at the end of a write. */
> -		if (should_remove_suid(dentry)) {
> +		if (should_remove_suid(dentry, file)) {
>  			if (meta_level == 0) {
>  				ocfs2_inode_unlock(inode, meta_level);
>  				meta_level = 1;
> diff --git a/fs/open.c b/fs/open.c
> index d3ed8171e8e0..8f54f34d1e3e 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -52,7 +52,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
>  	}
>  
>  	/* Remove suid, sgid, and file capabilities on truncate too */
> -	ret = dentry_needs_remove_privs(dentry);
> +	ret = dentry_needs_remove_privs(dentry, filp);
>  	if (ret < 0)
>  		return ret;
>  	if (ret)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2ba074328894..87654fb21158 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2718,7 +2718,7 @@ extern void __destroy_inode(struct inode *);
>  extern struct inode *new_inode_pseudo(struct super_block *sb);
>  extern struct inode *new_inode(struct super_block *sb);
>  extern void free_inode_nonrcu(struct inode *inode);
> -extern int should_remove_suid(struct dentry *);
> +extern int should_remove_suid(struct dentry *, struct file *);
>  extern int file_remove_privs(struct file *);
>  
>  extern void __insert_inode_hash(struct inode *, unsigned long hashval);

Reviewed-by: Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/2] fs: Check f_cred as well as of current's creds in should_remove_suid()
@ 2017-01-31 11:43     ` Jeff Layton
  0 siblings, 0 replies; 22+ messages in thread
From: Jeff Layton @ 2017-01-31 11:43 UTC (permalink / raw)
  To: Andy Lutomirski, security
  Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau,
	linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List,
	Jan Kara, Linux FS Devel, Frank Filz, stable

On Fri, 2017-01-27 at 18:49 -0800, Andy Lutomirski wrote:
> 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 in addition to current's creds whenever a struct file is
> involved.
> 
> I'm checking both because I'm nervous about preserving the SUID and
> SGID bits in any situation in which they're not currently preserved
> and because Ben Hutchings suggested doing it this way.
> 
> I don't know why we check capabilities at all, and we could probably
> get away with clearing the setgid bit regardless of capabilities,
> but this change should be less likely to break some weird program.
> 
> This mitigates exploits that take advantage of world-writable setgid
> files or directories.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  fs/inode.c         | 37 +++++++++++++++++++++++++++++++------
>  fs/internal.h      |  2 +-
>  fs/ocfs2/file.c    |  4 ++--
>  fs/open.c          |  2 +-
>  include/linux/fs.h |  2 +-
>  5 files changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 88110fd0b282..0e1e141b094c 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1733,8 +1733,12 @@ EXPORT_SYMBOL(touch_atime);
>   *
>   *	if suid or (sgid and xgrp)
>   *		remove privs
> + *
> + * If a file is provided, we assume that this is write(), ftruncate() or
> + * similar on that file.  If a file is not provided, we assume that no
> + * file descriptor is involved (e.g. truncate()).
>   */
> -int should_remove_suid(struct dentry *dentry)
> +int should_remove_suid(struct dentry *dentry, struct file *file)
>  {
>  	umode_t mode = d_inode(dentry)->i_mode;
>  	int kill = 0;
> @@ -1750,8 +1754,29 @@ int should_remove_suid(struct dentry *dentry)
>  	if (unlikely((mode & S_ISGID) && (mode & S_IXGRP)))
>  		kill |= ATTR_KILL_SGID;
>  
> -	if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode)))
> -		return kill;
> +	if (unlikely(kill && S_ISREG(mode))) {
> +		/*
> +		 * To minimize the degree to which this code works differently
> +		 * from Linux 4.9 and below, we kill SUID/SGID if the writer
> +		 * is unprivileged even if the file was opened by a privileged
> +		 * process.  Yes, this is a hack and is a technical violation
> +		 * of the "write(2) doesn't check current_cred()" rule.
> +		 *
> +		 * Ideally we would just kill the SUID bit regardless
> +		 * of capabilities.
> +		 */
> +		if (!capable(CAP_FSETID))
> +			return kill;
> +
> +		/*
> +		 * To avoid abuse of stdout/stderr redirection, we need to
> +		 * kill SUID/SGID if the file was opened by an unprivileged
> +		 * task.
> +		 */
> +		if (file && file->f_cred != current_cred() &&
> +		    !file_ns_capable(file, &init_user_ns, CAP_FSETID))
> +			return kill;
> +	}
>  
>  	return 0;
>  }
> @@ -1762,7 +1787,7 @@ EXPORT_SYMBOL(should_remove_suid);
>   * response to write or truncate. Return 0 if nothing has to be changed.
>   * Negative value on error (change should be denied).
>   */
> -int dentry_needs_remove_privs(struct dentry *dentry)
> +int dentry_needs_remove_privs(struct dentry *dentry, struct file *file)
>  {
>  	struct inode *inode = d_inode(dentry);
>  	int mask = 0;
> @@ -1771,7 +1796,7 @@ int dentry_needs_remove_privs(struct dentry *dentry)
>  	if (IS_NOSEC(inode))
>  		return 0;
>  
> -	mask = should_remove_suid(dentry);
> +	mask = should_remove_suid(dentry, file);
>  	ret = security_inode_need_killpriv(dentry);
>  	if (ret < 0)
>  		return ret;
> @@ -1807,7 +1832,7 @@ int file_remove_privs(struct file *file)
>  	if (IS_NOSEC(inode))
>  		return 0;
>  
> -	kill = dentry_needs_remove_privs(dentry);
> +	kill = dentry_needs_remove_privs(dentry, file);
>  	if (kill < 0)
>  		return kill;
>  	if (kill)
> diff --git a/fs/internal.h b/fs/internal.h
> index b63cf3af2dc2..c467ad502cac 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -119,7 +119,7 @@ extern struct file *filp_clone_open(struct file *);
>   */
>  extern long prune_icache_sb(struct super_block *sb, struct shrink_control *sc);
>  extern void inode_add_lru(struct inode *inode);
> -extern int dentry_needs_remove_privs(struct dentry *dentry);
> +extern int dentry_needs_remove_privs(struct dentry *dentry, struct file *file);
>  
>  extern bool __atime_needs_update(const struct path *, struct inode *, bool);
>  static inline bool atime_needs_update_rcu(const struct path *path,
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index c4889655d32b..db6efd940ac0 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -1903,7 +1903,7 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
>  		}
>  	}
>  
> -	if (file && should_remove_suid(file->f_path.dentry)) {
> +	if (file && should_remove_suid(file->f_path.dentry, file)) {
>  		ret = __ocfs2_write_remove_suid(inode, di_bh);
>  		if (ret) {
>  			mlog_errno(ret);
> @@ -2132,7 +2132,7 @@ static int ocfs2_prepare_inode_for_write(struct file *file,
>  		 * inode. There's also the dinode i_size state which
>  		 * can be lost via setattr during extending writes (we
>  		 * set inode->i_size at the end of a write. */
> -		if (should_remove_suid(dentry)) {
> +		if (should_remove_suid(dentry, file)) {
>  			if (meta_level == 0) {
>  				ocfs2_inode_unlock(inode, meta_level);
>  				meta_level = 1;
> diff --git a/fs/open.c b/fs/open.c
> index d3ed8171e8e0..8f54f34d1e3e 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -52,7 +52,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
>  	}
>  
>  	/* Remove suid, sgid, and file capabilities on truncate too */
> -	ret = dentry_needs_remove_privs(dentry);
> +	ret = dentry_needs_remove_privs(dentry, filp);
>  	if (ret < 0)
>  		return ret;
>  	if (ret)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2ba074328894..87654fb21158 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2718,7 +2718,7 @@ extern void __destroy_inode(struct inode *);
>  extern struct inode *new_inode_pseudo(struct super_block *sb);
>  extern struct inode *new_inode(struct super_block *sb);
>  extern void free_inode_nonrcu(struct inode *inode);
> -extern int should_remove_suid(struct dentry *);
> +extern int should_remove_suid(struct dentry *, struct file *);
>  extern int file_remove_privs(struct file *);
>  
>  extern void __insert_inode_hash(struct inode *, unsigned long hashval);

Reviewed-by: Jeff Layton <jlayton@redhat.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/2] fs: Check f_cred as well as of current's creds in should_remove_suid()
@ 2017-01-31 11:43     ` Jeff Layton
  0 siblings, 0 replies; 22+ messages in thread
From: Jeff Layton @ 2017-01-31 11:43 UTC (permalink / raw)
  To: Andy Lutomirski, security
  Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau,
	linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List,
	Jan Kara, Linux FS Devel, Frank Filz, stable

On Fri, 2017-01-27 at 18:49 -0800, Andy Lutomirski wrote:
> 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 in addition to current's creds whenever a struct file is
> involved.
> 
> I'm checking both because I'm nervous about preserving the SUID and
> SGID bits in any situation in which they're not currently preserved
> and because Ben Hutchings suggested doing it this way.
> 
> I don't know why we check capabilities at all, and we could probably
> get away with clearing the setgid bit regardless of capabilities,
> but this change should be less likely to break some weird program.
> 
> This mitigates exploits that take advantage of world-writable setgid
> files or directories.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  fs/inode.c         | 37 +++++++++++++++++++++++++++++++------
>  fs/internal.h      |  2 +-
>  fs/ocfs2/file.c    |  4 ++--
>  fs/open.c          |  2 +-
>  include/linux/fs.h |  2 +-
>  5 files changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 88110fd0b282..0e1e141b094c 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1733,8 +1733,12 @@ EXPORT_SYMBOL(touch_atime);
>   *
>   *	if suid or (sgid and xgrp)
>   *		remove privs
> + *
> + * If a file is provided, we assume that this is write(), ftruncate() or
> + * similar on that file.  If a file is not provided, we assume that no
> + * file descriptor is involved (e.g. truncate()).
>   */
> -int should_remove_suid(struct dentry *dentry)
> +int should_remove_suid(struct dentry *dentry, struct file *file)
>  {
>  	umode_t mode = d_inode(dentry)->i_mode;
>  	int kill = 0;
> @@ -1750,8 +1754,29 @@ int should_remove_suid(struct dentry *dentry)
>  	if (unlikely((mode & S_ISGID) && (mode & S_IXGRP)))
>  		kill |= ATTR_KILL_SGID;
>  
> -	if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode)))
> -		return kill;
> +	if (unlikely(kill && S_ISREG(mode))) {
> +		/*
> +		 * To minimize the degree to which this code works differently
> +		 * from Linux 4.9 and below, we kill SUID/SGID if the writer
> +		 * is unprivileged even if the file was opened by a privileged
> +		 * process.  Yes, this is a hack and is a technical violation
> +		 * of the "write(2) doesn't check current_cred()" rule.
> +		 *
> +		 * Ideally we would just kill the SUID bit regardless
> +		 * of capabilities.
> +		 */
> +		if (!capable(CAP_FSETID))
> +			return kill;
> +
> +		/*
> +		 * To avoid abuse of stdout/stderr redirection, we need to
> +		 * kill SUID/SGID if the file was opened by an unprivileged
> +		 * task.
> +		 */
> +		if (file && file->f_cred != current_cred() &&
> +		    !file_ns_capable(file, &init_user_ns, CAP_FSETID))
> +			return kill;
> +	}
>  
>  	return 0;
>  }
> @@ -1762,7 +1787,7 @@ EXPORT_SYMBOL(should_remove_suid);
>   * response to write or truncate. Return 0 if nothing has to be changed.
>   * Negative value on error (change should be denied).
>   */
> -int dentry_needs_remove_privs(struct dentry *dentry)
> +int dentry_needs_remove_privs(struct dentry *dentry, struct file *file)
>  {
>  	struct inode *inode = d_inode(dentry);
>  	int mask = 0;
> @@ -1771,7 +1796,7 @@ int dentry_needs_remove_privs(struct dentry *dentry)
>  	if (IS_NOSEC(inode))
>  		return 0;
>  
> -	mask = should_remove_suid(dentry);
> +	mask = should_remove_suid(dentry, file);
>  	ret = security_inode_need_killpriv(dentry);
>  	if (ret < 0)
>  		return ret;
> @@ -1807,7 +1832,7 @@ int file_remove_privs(struct file *file)
>  	if (IS_NOSEC(inode))
>  		return 0;
>  
> -	kill = dentry_needs_remove_privs(dentry);
> +	kill = dentry_needs_remove_privs(dentry, file);
>  	if (kill < 0)
>  		return kill;
>  	if (kill)
> diff --git a/fs/internal.h b/fs/internal.h
> index b63cf3af2dc2..c467ad502cac 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -119,7 +119,7 @@ extern struct file *filp_clone_open(struct file *);
>   */
>  extern long prune_icache_sb(struct super_block *sb, struct shrink_control *sc);
>  extern void inode_add_lru(struct inode *inode);
> -extern int dentry_needs_remove_privs(struct dentry *dentry);
> +extern int dentry_needs_remove_privs(struct dentry *dentry, struct file *file);
>  
>  extern bool __atime_needs_update(const struct path *, struct inode *, bool);
>  static inline bool atime_needs_update_rcu(const struct path *path,
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index c4889655d32b..db6efd940ac0 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -1903,7 +1903,7 @@ static int __ocfs2_change_file_space(struct file *file, struct inode *inode,
>  		}
>  	}
>  
> -	if (file && should_remove_suid(file->f_path.dentry)) {
> +	if (file && should_remove_suid(file->f_path.dentry, file)) {
>  		ret = __ocfs2_write_remove_suid(inode, di_bh);
>  		if (ret) {
>  			mlog_errno(ret);
> @@ -2132,7 +2132,7 @@ static int ocfs2_prepare_inode_for_write(struct file *file,
>  		 * inode. There's also the dinode i_size state which
>  		 * can be lost via setattr during extending writes (we
>  		 * set inode->i_size at the end of a write. */
> -		if (should_remove_suid(dentry)) {
> +		if (should_remove_suid(dentry, file)) {
>  			if (meta_level == 0) {
>  				ocfs2_inode_unlock(inode, meta_level);
>  				meta_level = 1;
> diff --git a/fs/open.c b/fs/open.c
> index d3ed8171e8e0..8f54f34d1e3e 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -52,7 +52,7 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
>  	}
>  
>  	/* Remove suid, sgid, and file capabilities on truncate too */
> -	ret = dentry_needs_remove_privs(dentry);
> +	ret = dentry_needs_remove_privs(dentry, filp);
>  	if (ret < 0)
>  		return ret;
>  	if (ret)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2ba074328894..87654fb21158 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2718,7 +2718,7 @@ extern void __destroy_inode(struct inode *);
>  extern struct inode *new_inode_pseudo(struct super_block *sb);
>  extern struct inode *new_inode(struct super_block *sb);
>  extern void free_inode_nonrcu(struct inode *inode);
> -extern int should_remove_suid(struct dentry *);
> +extern int should_remove_suid(struct dentry *, struct file *);
>  extern int file_remove_privs(struct file *);
>  
>  extern void __insert_inode_hash(struct inode *, unsigned long hashval);

Reviewed-by: Jeff Layton <jlayton@redhat.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory
  2017-01-28  2:49   ` Andy Lutomirski
  (?)
@ 2017-01-31 11:43     ` Jeff Layton
  -1 siblings, 0 replies; 22+ messages in thread
From: Jeff Layton @ 2017-01-31 11:43 UTC (permalink / raw)
  To: Andy Lutomirski, security
  Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau,
	linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List,
	Jan Kara, Linux FS Devel, Frank Filz, stable

On Fri, 2017-01-27 at 18:49 -0800, Andy Lutomirski wrote:
> 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.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  fs/inode.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 0e1e141b094c..f6acb9232263 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2025,12 +2025,30 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
>  			umode_t mode)
>  {
>  	inode->i_uid = current_fsuid();
> +	inode->i_gid = current_fsgid();
> +
>  	if (dir && dir->i_mode & S_ISGID) {

I'm surprised the compiler doesn't complain about ambiguous order of ops
in the above if statement. Might be nice to add some parenthesis there
since you're in here, just for clarity.

> +		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
> -		inode->i_gid = current_fsgid();
> +		} 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
> +			 * by the wrong group.
> +			 */
> +			mode &= ~S_ISGID;
> +		}
> +	}
> +
>  	inode->i_mode = mode;
>  }
>  EXPORT_SYMBOL(inode_init_owner);

It's hard to picture any applications that would rely on the legacy
behavior, but if they come out of the woodwork, we could always add a
"make my kernel unsafe" command-line or compile time switch to bring it
back.

I think this is reasonable thing to do, but Michael K. is correct that
we should document the behavior changes in the relevant manpages.

Reviewed-by: Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory
@ 2017-01-31 11:43     ` Jeff Layton
  0 siblings, 0 replies; 22+ messages in thread
From: Jeff Layton @ 2017-01-31 11:43 UTC (permalink / raw)
  To: Andy Lutomirski, security
  Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau,
	linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List,
	Jan Kara, Linux FS Devel, Frank Filz, stable

On Fri, 2017-01-27 at 18:49 -0800, Andy Lutomirski wrote:
> 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.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  fs/inode.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 0e1e141b094c..f6acb9232263 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2025,12 +2025,30 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
>  			umode_t mode)
>  {
>  	inode->i_uid = current_fsuid();
> +	inode->i_gid = current_fsgid();
> +
>  	if (dir && dir->i_mode & S_ISGID) {

I'm surprised the compiler doesn't complain about ambiguous order of ops
in the above if statement. Might be nice to add some parenthesis there
since you're in here, just for clarity.

> +		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
> -		inode->i_gid = current_fsgid();
> +		} 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
> +			 * by the wrong group.
> +			 */
> +			mode &= ~S_ISGID;
> +		}
> +	}
> +
>  	inode->i_mode = mode;
>  }
>  EXPORT_SYMBOL(inode_init_owner);

It's hard to picture any applications that would rely on the legacy
behavior, but if they come out of the woodwork, we could always add a
"make my kernel unsafe" command-line or compile time switch to bring it
back.

I think this is reasonable thing to do, but Michael K. is correct that
we should document the behavior changes in the relevant manpages.

Reviewed-by: Jeff Layton <jlayton@redhat.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory
@ 2017-01-31 11:43     ` Jeff Layton
  0 siblings, 0 replies; 22+ messages in thread
From: Jeff Layton @ 2017-01-31 11:43 UTC (permalink / raw)
  To: Andy Lutomirski, security
  Cc: Konstantin Khlebnikov, Alexander Viro, Kees Cook, Willy Tarreau,
	linux-mm, Andrew Morton, yalin wang, Linux Kernel Mailing List,
	Jan Kara, Linux FS Devel, Frank Filz, stable

On Fri, 2017-01-27 at 18:49 -0800, Andy Lutomirski wrote:
> 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.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  fs/inode.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 0e1e141b094c..f6acb9232263 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2025,12 +2025,30 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
>  			umode_t mode)
>  {
>  	inode->i_uid = current_fsuid();
> +	inode->i_gid = current_fsgid();
> +
>  	if (dir && dir->i_mode & S_ISGID) {

I'm surprised the compiler doesn't complain about ambiguous order of ops
in the above if statement. Might be nice to add some parenthesis there
since you're in here, just for clarity.

> +		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
> -		inode->i_gid = current_fsgid();
> +		} 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
> +			 * by the wrong group.
> +			 */
> +			mode &= ~S_ISGID;
> +		}
> +	}
> +
>  	inode->i_mode = mode;
>  }
>  EXPORT_SYMBOL(inode_init_owner);

It's hard to picture any applications that would rely on the legacy
behavior, but if they come out of the woodwork, we could always add a
"make my kernel unsafe" command-line or compile time switch to bring it
back.

I think this is reasonable thing to do, but Michael K. is correct that
we should document the behavior changes in the relevant manpages.

Reviewed-by: Jeff Layton <jlayton@redhat.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory
  2017-01-31 11:43     ` Jeff Layton
@ 2017-01-31 16:51       ` Andy Lutomirski
  -1 siblings, 0 replies; 22+ messages in thread
From: Andy Lutomirski @ 2017-01-31 16:51 UTC (permalink / raw)
  To: Jeff Layton
  Cc: Andy Lutomirski, security, Konstantin Khlebnikov, Alexander Viro,
	Kees Cook, Willy Tarreau, linux-mm, Andrew Morton, yalin wang,
	Linux Kernel Mailing List, Jan Kara, Linux FS Devel, Frank Filz,
	stable

On Tue, Jan 31, 2017 at 3:43 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Fri, 2017-01-27 at 18:49 -0800, Andy Lutomirski wrote:
>> 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.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  fs/inode.c | 24 +++++++++++++++++++++---
>>  1 file changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 0e1e141b094c..f6acb9232263 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -2025,12 +2025,30 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
>>                       umode_t mode)
>>  {
>>       inode->i_uid = current_fsuid();
>> +     inode->i_gid = current_fsgid();
>> +
>>       if (dir && dir->i_mode & S_ISGID) {
>
> I'm surprised the compiler doesn't complain about ambiguous order of ops
> in the above if statement. Might be nice to add some parenthesis there
> since you're in here, just for clarity.

I'll keep that in mind if I do further cleanups here.

>
>> +             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
>> -             inode->i_gid = current_fsgid();
>> +             } 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
>> +                      * by the wrong group.
>> +                      */
>> +                     mode &= ~S_ISGID;
>> +             }
>> +     }
>> +
>>       inode->i_mode = mode;
>>  }
>>  EXPORT_SYMBOL(inode_init_owner);
>
> It's hard to picture any applications that would rely on the legacy
> behavior, but if they come out of the woodwork, we could always add a
> "make my kernel unsafe" command-line or compile time switch to bring it
> back.

I'm having trouble thinking of any legitimate use.  Sure, some package
manager or untar-like tool could create a setgid file like this, but
as soon as it tries to write to the file, unless it exploits a
different bug, the setgid bit would be cleared.

--Andy

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory
@ 2017-01-31 16:51       ` Andy Lutomirski
  0 siblings, 0 replies; 22+ messages in thread
From: Andy Lutomirski @ 2017-01-31 16:51 UTC (permalink / raw)
  To: Jeff Layton
  Cc: Andy Lutomirski, security, Konstantin Khlebnikov, Alexander Viro,
	Kees Cook, Willy Tarreau, linux-mm, Andrew Morton, yalin wang,
	Linux Kernel Mailing List, Jan Kara, Linux FS Devel, Frank Filz,
	stable

On Tue, Jan 31, 2017 at 3:43 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Fri, 2017-01-27 at 18:49 -0800, Andy Lutomirski wrote:
>> 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.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  fs/inode.c | 24 +++++++++++++++++++++---
>>  1 file changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 0e1e141b094c..f6acb9232263 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -2025,12 +2025,30 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
>>                       umode_t mode)
>>  {
>>       inode->i_uid = current_fsuid();
>> +     inode->i_gid = current_fsgid();
>> +
>>       if (dir && dir->i_mode & S_ISGID) {
>
> I'm surprised the compiler doesn't complain about ambiguous order of ops
> in the above if statement. Might be nice to add some parenthesis there
> since you're in here, just for clarity.

I'll keep that in mind if I do further cleanups here.

>
>> +             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
>> -             inode->i_gid = current_fsgid();
>> +             } 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
>> +                      * by the wrong group.
>> +                      */
>> +                     mode &= ~S_ISGID;
>> +             }
>> +     }
>> +
>>       inode->i_mode = mode;
>>  }
>>  EXPORT_SYMBOL(inode_init_owner);
>
> It's hard to picture any applications that would rely on the legacy
> behavior, but if they come out of the woodwork, we could always add a
> "make my kernel unsafe" command-line or compile time switch to bring it
> back.

I'm having trouble thinking of any legitimate use.  Sure, some package
manager or untar-like tool could create a setgid file like this, but
as soon as it tries to write to the file, unless it exploits a
different bug, the setgid bit would be cleared.

--Andy

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 22+ messages in thread
end of thread, other threads:[~2017-01-31 17:01 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-28  2:49 [PATCH v2 0/2] setgid hardening Andy Lutomirski
2017-01-28  2:49 ` Andy Lutomirski
2017-01-28  2:49 ` [PATCH v2 1/2] fs: Check f_cred as well as of current's creds in should_remove_suid() Andy Lutomirski
2017-01-28  2:49   ` Andy Lutomirski
2017-01-31  3:50   ` Michael Kerrisk
2017-01-31  3:50     ` Michael Kerrisk
2017-01-31 11:43   ` Jeff Layton
2017-01-31 11:43     ` Jeff Layton
2017-01-31 11:43     ` Jeff Layton
2017-01-28  2:49 ` [PATCH v2 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory Andy Lutomirski
2017-01-28  2:49   ` Andy Lutomirski
2017-01-31  3:50   ` Michael Kerrisk
2017-01-31  3:50     ` Michael Kerrisk
2017-01-31 11:43   ` Jeff Layton
2017-01-31 11:43     ` Jeff Layton
2017-01-31 11:43     ` Jeff Layton
2017-01-31 16:51     ` Andy Lutomirski
2017-01-31 16:51       ` Andy Lutomirski
2017-01-31  3:49 ` [PATCH v2 0/2] setgid hardening Michael Kerrisk
2017-01-31  3:49   ` Michael Kerrisk
2017-01-31  3:56   ` Andy Lutomirski
2017-01-31  3:56     ` Andy Lutomirski

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.