[PATCH 0/2] setgid hardening

[PATCH 0/2] setgid hardening

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

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

 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, 35 insertions(+), 12 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>

[PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid()

[Andy Lutomirski]

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

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         | 16 +++++++++++-----
 fs/internal.h      |  2 +-
 fs/ocfs2/file.c    |  4 ++--
 fs/open.c          |  2 +-
 include/linux/fs.h |  2 +-
 5 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 88110fd0b282..f7029c40cfbd 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 ftruncate() or similar
+ * on that file.  If a file is not provided, we assume that no file
+ * descriptor is involved.
  */
-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,7 +1754,9 @@ 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)))
+	if (unlikely(kill && S_ISREG(mode) &&
+		     !(file ? file_ns_capable(file, &init_user_ns, CAP_FSETID) :
+		       capable(CAP_FSETID))))
 		return kill;
 
 	return 0;
@@ -1762,7 +1768,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 +1777,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 +1813,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>

Re: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid()

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

On Wed, 2017-01-25 at 13:06 -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 instead of current's creds whenever a struct file is
> involved.
[...]

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

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

Ben.

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid()

[Andy Lutomirski]

On Wed, Jan 25, 2017 at 1:43 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Wed, 2017-01-25 at 13:06 -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 instead of current's creds whenever a struct file is
>> involved.
> [...]
>
> What if, instead, a privileged program passes the fd to an un
> unprivileged program?  It sounds like a bad idea to start with, but at
> least currently the unprivileged program is going to clear the setgid
> bit when it writes.  This change would make that behaviour more
> dangerous.

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

(sudo -u nobody echo blah) >setuid_program

presumably it wanted to make the change.

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

I could go either way.

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

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

RE: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid()

[Frank Filz]

> On Wed, Jan 25, 2017 at 1:43 PM, Ben Hutchings <ben@decadent.org.uk>
> wrote:
> > On Wed, 2017-01-25 at 13:06 -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 instead of current's creds whenever a struct file is involved.
> > [...]
> >
> > What if, instead, a privileged program passes the fd to an un
> > unprivileged program?  It sounds like a bad idea to start with, but at
> > least currently the unprivileged program is going to clear the setgid
> > bit when it writes.  This change would make that behaviour more
> > dangerous.
>
> Hmm.  Although, if a privileged program does something like:
>
> (sudo -u nobody echo blah) >setuid_program
>
> presumably it wanted to make the change.

I'm not following all the intricacies here, though I need to...

What about a privileged program that drops privilege for certain operations?

Specifically the Ganesha user space NFS server runs as root, but sets fsuid/fsgid for specific threads performing I/O operations on behalf of NFS clients.

I want to make sure setgid bit handling is proper for these cases.

Ganesha does some permission checking, but this is one area I want to defer to the underlying  filesystem because it's not easy for Ganesha to get it right.

> > Perhaps there should be a capability check on both the current
> > credentials and file credentials?  (I realise that we've considered
> > file credential checks to be sufficient elsewhere, but those cases
> > involved virtual files with special semantics, where it's clearer that
> > a privileged process should not pass them to an unprivileged process.)
> >
>
> I could go either way.
>
> What I really want to do is to write a third patch that isn't for -stable that just
> removes the capable() check entirely.  I'm reasonably confident it won't
> break things for a silly reason: because it's capable() and not ns_capable(),
> anything it would break would also be broken in an unprivileged container,
> and I haven't seen any reports of package managers or similar breaking for
> this reason.

Frank


---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

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

Re: [PATCH 1/2] fs: Check f_cred instead of current's creds in should_remove_suid()

[Kees Cook]

On Wed, Jan 25, 2017 at 1:43 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Wed, 2017-01-25 at 13:06 -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 instead of current's creds whenever a struct file is
>> involved.
> [...]
>
> What if, instead, a privileged program passes the fd to an un
> unprivileged program?  It sounds like a bad idea to start with, but at
> least currently the unprivileged program is going to clear the setgid
> bit when it writes.  This change would make that behaviour more
> dangerous.
>
> Perhaps there should be a capability check on both the current
> credentials and file credentials?  (I realise that we've considered
> file credential checks to be sufficient elsewhere, but those cases
> involved virtual files with special semantics, where it's clearer that
> a privileged process should not pass them to an unprivileged process.)

We need a set of self-tests for this whole area. :( There are so many
corner cases. We still have an unfixed corner case with mmap writes
not clearing set*id bits that I tried to solve last year...

-Kees

-- 
Kees Cook
Nexus Security

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

[PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory

[Andy Lutomirski]

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

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

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

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

Re: [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1498 bytes --]

On Wed, 2017-01-25 at 13:06 -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.
> 
> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  fs/inode.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index f7029c40cfbd..d7e4b80470dd 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2007,11 +2007,28 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
>  {
>  	inode->i_uid = current_fsuid();
>  	if (dir && dir->i_mode & S_ISGID) {
> +		bool changing_gid = !gid_eq(inode->i_gid, dir->i_gid);
[...]

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

Ben.

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory

[Andy Lutomirski]

On Wed, Jan 25, 2017 at 1:31 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> On Wed, 2017-01-25 at 13:06 -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.
>>
>> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  fs/inode.c | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index f7029c40cfbd..d7e4b80470dd 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -2007,11 +2007,28 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
>>  {
>>       inode->i_uid = current_fsuid();
>>       if (dir && dir->i_mode & S_ISGID) {
>> +             bool changing_gid = !gid_eq(inode->i_gid, dir->i_gid);
> [...]
>
> inode->i_gid hasn't been initialised yet.  This should compare with
> current_fsgid(), shouldn't it?

Whoops.  In v2, I'll fix it by inode->i_gid first -- that'll simplify
the control flow.

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



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

RE: [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory

[Frank Filz]

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

Nasty.

I'd love to see a test for this in xfstests and/or pjdfstests...

Frank


---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

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

Re: [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory

[Willy Tarreau]

On Wed, Jan 25, 2017 at 01:06:52PM -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.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  fs/inode.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index f7029c40cfbd..d7e4b80470dd 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2007,11 +2007,28 @@ void inode_init_owner(struct inode *inode, const struct inode *dir,
>  {
>  	inode->i_uid = current_fsuid();
>  	if (dir && dir->i_mode & S_ISGID) {
> +		bool changing_gid = !gid_eq(inode->i_gid, dir->i_gid);
> +
>  		inode->i_gid = dir->i_gid;
> -		if (S_ISDIR(mode))
> +		if (S_ISDIR(mode)) {
>  			mode |= S_ISGID;
> -	} else
> +		} else if (((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))
> +			   && S_ISREG(mode) && changing_gid
> +			   && !capable(CAP_FSETID)) {
> +			/*
> +			 * Whoa there!  An unprivileged program just
> +			 * tried to create a new executable with SGID
> +			 * set in a directory with SGID set that belongs
> +			 * to a different group.  Don't let this program
> +			 * create a SGID executable that ends up owned
> +			 * by the wrong group.
> +			 */
> +			mode &= ~S_ISGID;
> +		}
> +
> +	} else {
>  		inode->i_gid = current_fsgid();
> +	}
>  	inode->i_mode = mode;
>  }

It seems to me like you're leaving inode->i_gid uninitialized when you
take the Woah branch here. Or at least it's not obvious to me. I'd
rather adjust it like this to make it easier to read (patched edited
by hand, sorry for the bad formating) and it also covers the case
where the gid_eq() check was apparently performed on something
uninitialized :

 {
 	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
+		} 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;
 }

Please ignore all this if I missed something.

Willy

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

Re: [PATCH 0/2] setgid hardening

[Willy Tarreau]

On Wed, Jan 25, 2017 at 01:06:50PM -0800, Andy Lutomirski 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.

BTW I like this. I vaguely remember having played with this when I
was a student 2 decades ago on a system where /var/spool/mail was
3777 (yes, setgid+sticky) and the mail files were 660. You could
deposit a shell there, then execute it with mail's permissions and
access any mailbox. That was quite odd as a design choice. The
impacts are often limited unless you find other ways to escalate
but generally it's not really clean.

Willy

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