allow file truncations when both suid and write permissions set

When suid is set and the non-owner user has write permission, any writing into this file should be allowed and suid should be removed after that.

However, current kernel only allows writing without truncations, when we do truncations on that file, we get EPERM. This is a bug.

Steps to reproduce this bug:

% ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1 % echo h > rootdir/file1
zsh: operation not permitted: rootdir/file1 % ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1 % echo h >> rootdir/file1
% ls -l rootdir/file1
-rwxrwxrwx 1 root root 5 Jun 25 16:34 rootdir/file1

This patch fixes it.

(Need to Cc stable?)

Signed-off-by WANG Cong <[email protected]>

Cc Eric Sandeen <[email protected]>

Cc Eugene Teo <[email protected]>

Cc Al Viro <[email protected]>

---

fs/open.c +1 -1

index dd98e80..ebf6d8d 100644
@@ -199,7 +199,7 @@ out:
 int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
 	struct file *filp)
 {
-	int err;
+	int ret;
 	struct iattr newattrs;
 
 	/* Not pretty: "inode->i_size" shouldn't really be signed. But it is. */

fs/open.c +2 -2

 	mutex_lock(&dentry->d_inode->i_mutex);
-	err = notify_change(dentry, &newattrs);
+	ret = notify_change(dentry, &newattrs);
 	mutex_unlock(&dentry->d_inode->i_mutex);
-	return err;
+	return ret;
 }
 
 static long do_sys_truncate(const char __user *pathname, loff_t length)

Quoted 6 lines · Amerigo Wang

When suid is set and the non-owner user has write permission, any writing into this file should be allowed and suid should be removed after that.

However, current kernel only allows writing without truncations, when we do truncations on that file, we get EPERM. This is a bug.

Quoted 7 lines · Eric Sandeen

Quoted 6 lines · Amerigo Wang

When suid is set and the non-owner user has write permission, any writing into this file should be allowed and suid should be removed after that.

However, current kernel only allows writing without truncations, when we do truncations on that file, we get EPERM. This is a bug.

...

Quoted 11 lines

So I think the main problem here is simply that we didn't set ATTR_FORCE, right...

Seems a little odd to |= with ret, -then- check if it's non-0.  Maybe:

      /* Remove suid/sgid on truncate too */
-     newattrs.ia_valid |= should_remove_suid(dentry);
+     ret = should_remove_suid(dentry);
+     if (ret)
+             newattrs.ia_valid |= (ret | ATTR_FORCE);

On second thought, and after talking w/ eparis, I think this probably needs a security_inode_killpriv() too... it seems like it might be best to change file_remove_suid(*file) to dentry_remove_suid(*dentry) and just call that from do_truncate()?

-Eric

Quoted 28 lines · Eric Sandeen

Quoted 7 lines · Eric Sandeen

Quoted 6 lines · Amerigo Wang

When suid is set and the non-owner user has write permission, any writing into this file should be allowed and suid should be removed after that.

However, current kernel only allows writing without truncations, when we do truncations on that file, we get EPERM. This is a bug.

...

fs/open.c int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,

Quoted 11 lines

So I think the main problem here is simply that we didn't set ATTR_FORCE, right...

Seems a little odd to |= with ret, -then- check if it's non-0. Maybe:

      /* Remove suid/sgid on truncate too */

fs/open.c +3 -1

-     newattrs.ia_valid |= should_remove_suid(dentry);
+     ret = should_remove_suid(dentry);
+     if (ret)
+             newattrs.ia_valid |= (ret | ATTR_FORCE);

On second thought, and after talking w/ eparis, I think this probably needs a security_inode_killpriv() too... it seems like it might be best to change file_remove_suid(*file) to dentry_remove_suid(*dentry) and just call that from do_truncate()?

Quoted 1 line

-Eric

All of this stuff seems horribly complex.... I'm trying to wrap my
head around everything going on here as well....

So NAK on both Amerigo's and Eric's patch. ATTR_FORCE is wrong. ATTR_FORCE here is going to force the security system to accept ALL of the attr changes, not just the ATTR_KILL_SUID and/or ATTR_KILL_SGID you |= in from should_remove_suid.
You need to follow esandeen's recommendation, change file_remove_suid() to dentry_remove_suid() and then use dentry_remove_suid() in do_truncate instead of what you are currently trying to do (and I think that's supposed to be done under the i_mutex right?)

Quoted 9 lines · Eric Paris

So NAK on both Amerigo's and Eric's patch. ATTR_FORCE is wrong. ATTR_FORCE here is going to force the security system to accept ALL of the attr changes, not just the ATTR_KILL_SUID and/or ATTR_KILL_SGID you |= in from should_remove_suid.
You need to follow esandeen's recommendation, change file_remove_suid() to dentry_remove_suid() and then use dentry_remove_suid() in do_truncate instead of what you are currently trying to do (and I think that's supposed to be done under the i_mutex right?)

But file_remove_suid() actually adds ATTR_FORCE too, in __remove_suid()...

Quoted 12 lines · Amerigo Wang

Quoted 10 lines · Eric Paris

So NAK on both Amerigo's and Eric's patch. ATTR_FORCE is wrong. ATTR_FORCE here is going to force the security system to accept ALL of the attr changes, not just the ATTR_KILL_SUID and/or ATTR_KILL_SGID you |= in from should_remove_suid.
You need to follow esandeen's recommendation, change file_remove_suid() to dentry_remove_suid() and then use dentry_remove_suid() in do_truncate instead of what you are currently trying to do (and I think that's supposed to be done under the i_mutex right?)

Quoted 2 lines

But file_remove_suid() actually adds ATTR_FORCE too, in __remove_suid()...

The difference being that it adds it to a private ia_valid that ONLY contains the SUID/SGID bits that we need to force the removal of. Not to the ia_valid that contains ATTR_SIZE and ATTR_FILE.