linux-kernel - Re: FUSE: regression when clearing setuid bits on chown

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161206100243.GF2622@veci.piliscsaba.szeredi.hu>
Date:   Tue, 6 Dec 2016 11:02:43 +0100
From:   Miklos Szeredi <miklos@...redi.hu>
To:     Jeff Layton <jlayton@...hat.com>
Cc:     linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: FUSE: regression when clearing setuid bits on chown

On Mon, Dec 05, 2016 at 01:21:15PM -0500, Jeff Layton wrote:
> Hi Miklos,
> 
> I think we've found a "regression" that has crept in due to this patch:
> 
> commit a09f99eddef44035ec764075a37bace8181bec38
> Author: Miklos Szeredi <mszeredi@...hat.com>
> Date:   Sat Oct 1 07:32:32 2016 +0200
> 
>     fuse: fix killing s[ug]id in setattr
>     

Thanks for the report.  Below patch should fix it.  Not sure what I've been
thinking when adding all that complexity, when the issue here was not whether to
clear the suid or not, but how stale the mode used is.

Thanks,
Miklos

---
From: Miklos Szeredi <mszeredi@...hat.com>
Subject: fuse: fix clearing suid, sgid for chown()

Basically, the pjdfstests set the ownership of a file to 06555, and then
chowns it (as root) to a new uid/gid. Prior to commit a09f99eddef4 ("fuse:
fix killing s[ug]id in setattr"), fuse would send down a setattr with both
the uid/gid change and a new mode.  Now, it just sends down the uid/gid
change.

Technically this is NOTABUG, since POSIX doesn't _require_ that we clear
these bits for a privileged process, but Linux (wisely) has done that and I
think we don't want to change that behavior here.

This is caused by the use of should_remove_suid(), which will always return
0 when the process has CAP_FSETID.

In fact we really don't need to be calling should_remove_suid() at all,
since we've already been indicated that we should remove the suid, we just
don't want to use a (very) stale mode for that.

This patch should fix the above as well as simplify the logic.

Reported-by: Jeff Layton <jlayton@...hat.com> 
Signed-off-by: Miklos Szeredi <mszeredi@...hat.com>
Fixes: a09f99eddef4 ("fuse: fix killing s[ug]id in setattr")
Cc: <stable@...r.kernel.org>
---
 fs/fuse/dir.c |   14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -1739,8 +1739,6 @@ static int fuse_setattr(struct dentry *e
 		 * This should be done on write(), truncate() and chown().
 		 */
 		if (!fc->handle_killpriv) {
-			int kill;
-
 			/*
 			 * ia_mode calculation may have used stale i_mode.
 			 * Refresh and recalculate.
@@ -1749,16 +1747,8 @@ static int fuse_setattr(struct dentry *e
 			if (ret)
 				return ret;

-			attr->ia_mode = inode->i_mode;
-			kill = should_remove_suid(entry);
-			if (kill & ATTR_KILL_SUID) {
-				attr->ia_valid |= ATTR_MODE;
-				attr->ia_mode &= ~S_ISUID;
-			}
-			if (kill & ATTR_KILL_SGID) {
-				attr->ia_valid |= ATTR_MODE;
-				attr->ia_mode &= ~S_ISGID;
-			}
+			attr->ia_mode = inode->i_mode & ~(S_ISUID | S_ISGID);
+			attr->ia_valid |= ATTR_MODE;
 		}
 	}
 	if (!attr->ia_valid)