Skip to contentSkip to conversation

fix file truncations when both suid and write permissions set

PATCH New Amerigo Wang 1 participant 3 messages Aug 19, 2009

Subsystems (2)
VFS SELinux security module
lore
Share & tools
lore
author
#1 lore raw

PATCH v5 1/2 selinux: adjust rules for ATTR_FORCE

In reply to Amerigo Wang

[PATCH] selinux: adjust rules for ATTR_FORCE

From: Amerigo Wang <[email protected]>

As suggested by OGAWA Hirofumi in thread: http://lkml.org/lkml/2009/8/7/132,
we should let selinux_inode_setattr() to match our ATTR_* rules. ATTR_FORCE should not force things like ATTR_SIZE.

Cc Eric Paris <[email protected]>
Signed-off-by WANG Cong <[email protected]>
[tweaked-by: OGAWA Hirofumi <[email protected]> ]
Signed-off-by OGAWA Hirofumi <[email protected]>
Acked-by Stephen Smalley <[email protected]>
---
 security/selinux/hooks.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
security/selinux/hooks.c renamed from security/selinux/hooks.c~selinux-truncate-fix +10 -4
diff -puN security/selinux/hooks.c~selinux-truncate-fix security/selinux/hooks.c
@@ -2711,12 +2711,18 @@ static int selinux_inode_permission(stru
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	const struct cred *cred = current_cred();
+	unsigned int ia_valid = iattr->ia_valid;
 
-	if (iattr->ia_valid & ATTR_FORCE)
-		return 0;
+	/* ATTR_FORCE is just used for ATTR_KILL_S[UG]ID. */
+	if (ia_valid & ATTR_FORCE) {
+		ia_valid &= ~(ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_MODE |
+			      ATTR_FORCE);
+		if (!ia_valid)
+			return 0;
+	}
 
-	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
-			       ATTR_ATIME_SET | ATTR_MTIME_SET))
+	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
+			ATTR_ATIME_SET | ATTR_MTIME_SET | ATTR_TIMES_SET))
 		return dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
 
 	return dentry_has_perm(cred, NULL, dentry, FILE__WRITE);

_

Amerigo Wang

author
#0 lore raw

v5 cover fix file truncations when both suid and write permissions set

Andrew, these two patches can replace the -mm patch "vfs-allow-file-truncations-when-both-suid-and-write-permissions-set.patch".

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.

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

Thanks!

author
#2 lore raw

PATCH v5 2/2 vfs: allow file truncations when both suid and write permissions set

With patch 1/2 applied, now we can just simply add ATTR_FORCE flag, to remove suid on truncation.

Signed-off-by WANG Cong <[email protected]>
Cc Eric Sandeen <[email protected]>
Cc Eric Paris <[email protected]>
Cc Eugene Teo <[email protected]>
Cc Al Viro <[email protected]>
Cc OGAWA Hirofumi <[email protected]>

---

fs/open.c +6 -4
index dd98e80..40d1fa2 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. */

@@ -214,12 +214,14 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
 	}
 
 	/* 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;
 
 	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)