Skip to contentSkip to conversation

fix file truncations when both suid and write permissions set

PATCH Superseded Amerigo Wang 3 participants 17 messages Aug 17, 2009 – Aug 19, 2009

Subsystems (2)
VFS SELinux security module

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Amerigo Wang

author
#0 lore raw

v4 cover fix 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.

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

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

Thanks!

author
#1 lore raw

PATCH v4 1/2 selinux: ajust rules for ATTR_FORCE

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 OGAWA Hirofumi <[email protected]>
Cc Stephen Smalley <[email protected]>
Cc Eric Paris <[email protected]>
Signed-off-by WANG Cong <[email protected]>

---

security/selinux/hooks.c +14 -8
index 1e8cfc4..3ee3365 100644
@@ -2708,18 +2708,24 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 			      file_mask_to_av(inode->i_mode, mask), NULL);
 }
 
+#define SELINUX_FORCED_MASK	(ATTR_MODE | ATTR_UID | ATTR_GID | \
+				ATTR_ATIME_SET | ATTR_MTIME_SET)
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	const struct cred *cred = current_cred();
+	unsigned int ia_valid = iattr->ia_valid;
+	int err = 0;
 
-	if (iattr->ia_valid & ATTR_FORCE)
-		return 0;
-
-	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
-			       ATTR_ATIME_SET | ATTR_MTIME_SET))
-		return dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
-
-	return dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
+	if ((ia_valid & ATTR_FORCE) && (ia_valid & SELINUX_FORCED_MASK)) {
+		err = dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
+		if (err)
+			return err;
+		ia_valid &= ~SELINUX_FORCED_MASK;
+		ia_valid &= ~ATTR_FORCE;
+	}
+	if (ia_valid)
+		err = dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
+	return err;
 }
 
 static int selinux_inode_getattr(struct vfsmount *mnt, struct dentry *dentry)
author
#2 lore raw

Re: [Patch 1/2] selinux: ajust rules for ATTR_FORCE

Oops! A typo in $subject...

s/ajust/adjust/

:-)

NAK
#3 lore raw

Re: [Patch 1/2] selinux: ajust rules for ATTR_FORCE

In reply to Amerigo Wang
Quoted 31 lines · Amerigo Wang

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 OGAWA Hirofumi <[email protected]>
Cc Stephen Smalley <[email protected]>
Cc Eric Paris <[email protected]>
Signed-off-by WANG Cong <[email protected]>

---

security/selinux/hooks.c +4 -5
index 1e8cfc4..3ee3365 100644
@@ -2708,18 +2708,24 @@ static int selinux_inode_permission(struct inode *inode, int mask)
 			      file_mask_to_av(inode->i_mode, mask), NULL);
 }
 
+#define SELINUX_FORCED_MASK	(ATTR_MODE | ATTR_UID | ATTR_GID | \
+				ATTR_ATIME_SET | ATTR_MTIME_SET)
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	const struct cred *cred = current_cred();
+	unsigned int ia_valid = iattr->ia_valid;
+	int err = 0;
 
-	if (iattr->ia_valid & ATTR_FORCE)
-		return 0;
-
-	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
-			       ATTR_ATIME_SET | ATTR_MTIME_SET))

security/selinux/hooks.c static int selinux_inode_permission(struct inode *inode, int mask) · line 2725

Quoted 10 lines
security/selinux/hooks.c +7 -3
-		return dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
-
-	return dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
+	if ((ia_valid & ATTR_FORCE) && (ia_valid & SELINUX_FORCED_MASK)) {
+		err = dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
+		if (err)
+			return err;
+		ia_valid &= ~SELINUX_FORCED_MASK;
+		ia_valid &= ~ATTR_FORCE;
+	}

This will only apply the setattr check if ATTR_FORCE was specified, which is not the current behavior nor what we want.

NAK.

Quoted 6 lines
security/selinux/hooks.c +3 -0
+	if (ia_valid)
+		err = dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
+	return err;
 }
 
 static int selinux_inode_getattr(struct vfsmount *mnt, struct dentry *dentry)
Signature · 2 lines
Stephen Smalley
National Security Agency
#4 lore raw

Re: [Patch 1/2] selinux: ajust rules for ATTR_FORCE

Quoted 4 lines · Stephen Smalley
Quoted 3 lines · Amerigo Wang

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.

[...]

Quoted 5 lines

This will only apply the setattr check if ATTR_FORCE was specified, which is not the current behavior nor what we want.

NAK.

How about this? I tweaked Amerigo's patch, and it is based on the original code is doing. This is only compile-test though.

[I'm still not sure what selinux want to do. normally inode_permission() should check truncate() permission, and this FILE__SIZE checks something again...? And we want to check FILE__WRITE for ATTR_[AMC]TIME?]

Thanks.

Signature · 61 lines
OGAWA Hirofumi <[email protected]>

[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: Stephen Smalley <[email protected]>
Cc: Eric Paris <[email protected]>
Signed-off-by: WANG Cong <[email protected]>
[tweaks]
Signed-off-by: OGAWA Hirofumi <[email protected]>
---

 security/selinux/hooks.c |   28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff -puN security/selinux/hooks.c~selinux-truncate-fix security/selinux/hooks.c
--- linux-2.6/security/selinux/hooks.c~selinux-truncate-fix	2009-08-18 01:01:13.000000000 +0900
+++ linux-2.6-hirofumi/security/selinux/hooks.c	2009-08-18 03:23:52.000000000 +0900
@@ -2710,16 +2710,28 @@ static int selinux_inode_permission(stru
 
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
-	const struct cred *cred = current_cred();
-
-	if (iattr->ia_valid & ATTR_FORCE)
-		return 0;
+#define SELINUX_F_SETATTR_MASK	(ATTR_MODE | ATTR_UID | ATTR_GID |	\
+				 ATTR_ATIME_SET | ATTR_MTIME_SET |	\
+				 ATTR_TIMES_SET)
+#define SELINUX_F_WRITE_MASK	(ATTR_CTIME | ATTR_MTIME | ATTR_ATIME | \
+				 ATTR_SIZE)
 
-	if (iattr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID |
-			       ATTR_ATIME_SET | ATTR_MTIME_SET))
-		return dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
+	const struct cred *cred = current_cred();
+	unsigned int ia_valid = iattr->ia_valid;
 
-	return dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
+	if (!(ia_valid & ATTR_FORCE) && (ia_valid & SELINUX_F_SETATTR_MASK)) {
+		int err = dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
+		if (err)
+			return err;
+		/* Assume timestamp was changed by attributes or utimes(). */
+		ia_valid &= ~(ATTR_CTIME | ATTR_MTIME | ATTR_ATIME);
+	}
+	if (ia_valid & SELINUX_F_WRITE_MASK) {
+		int err = dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
+		if (err)
+			return err;
+	}
+	return 0;
 }
 
 static int selinux_inode_getattr(struct vfsmount *mnt, struct dentry *dentry)
_
#5 lore raw

Re: [Patch 1/2] selinux: ajust rules for ATTR_FORCE

Quoted 13 lines · OGAWA Hirofumi
Quoted 4 lines · Stephen Smalley
Quoted 3 lines · Amerigo Wang

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.

[...]

Quoted 4 lines

This will only apply the setattr check if ATTR_FORCE was specified, which is not the current behavior nor what we want.

Quoted 8 lines
Quoted 1 line

NAK.

How about this? I tweaked Amerigo's patch, and it is based on the original code is doing. This is only compile-test though.

[I'm still not sure what selinux want to do. normally inode_permission() should check truncate() permission, and this FILE__SIZE checks something again...? And we want to check FILE__WRITE for ATTR_[AMC]TIME?]

Explicit setting of mode, owner, group, or timestamps is to be checked by the setattr permission, while implicit setting of timestamps or size is mediated by the write permission. Permission needs to be revalidated on use to address potential file relabeling or policy change. ATTR_FORCE is supposed to suppress permission checking altogether, and shouldn't be mixed with multiple attribute changes if some should be subject to permission checks while others should not.

Signature · 2 lines
Stephen Smalley
National Security Agency
#6 lore raw

Re: [Patch 1/2] selinux: ajust rules for ATTR_FORCE

Quoted 7 lines · Stephen Smalley
Quoted 3 lines

[I'm still not sure what selinux want to do. normally inode_permission() should check truncate() permission, and this FILE__SIZE checks something again...? And we want to check FILE__WRITE for ATTR_[AMC]TIME?]

Explicit setting of mode, owner, group, or timestamps is to be checked by the setattr permission, while implicit setting of timestamps or size is mediated by the write permission.

E.g. mode change has implicit ATTR_CTIME change. So it meant, we should check the both of FILE__SETATTR and FILE__WRITE?

Quoted 3 lines

ATTR_FORCE is supposed to suppress permission checking altogether, and shouldn't be mixed with multiple attribute changes if some should be subject to permission checks while others should not.

I disagree. In fact, ATTR_FORCE is just used for ATTR_KILL_S[UG]ID, and notify_change() is disallowing the mixed ATTR_MODE and ATTR_KILL_*. I think it should be enough.

If ATTR_FORCE is confusable, I think we can just add new ATTR_FORCE_MODE or ATTR_FORCE_KILL, and replace with current ATTR_FORCE. I'm ok either way. But, with this change, ATTR_FORCE has no users.

Thanks.

Signature · 1 line
OGAWA Hirofumi <[email protected]>
#7 lore raw

Re: [Patch 1/2] selinux: ajust rules for ATTR_FORCE

Quoted 6 lines · OGAWA Hirofumi
Quoted 4 lines · Stephen Smalley
Quoted 3 lines

[I'm still not sure what selinux want to do. normally inode_permission() should check truncate() permission, and this FILE__SIZE checks something again...? And we want to check FILE__WRITE for ATTR_[AMC]TIME?]

Quoted 6 lines
Quoted 3 lines

Explicit setting of mode, owner, group, or timestamps is to be checked by the setattr permission, while implicit setting of timestamps or size is mediated by the write permission.

E.g. mode change has implicit ATTR_CTIME change. So it meant, we should check the both of FILE__SETATTR and FILE__WRITE?

No, just setattr.

Quoted 7 lines
Quoted 3 lines

ATTR_FORCE is supposed to suppress permission checking altogether, and shouldn't be mixed with multiple attribute changes if some should be subject to permission checks while others should not.

I disagree. In fact, ATTR_FORCE is just used for ATTR_KILL_S[UG]ID, and notify_change() is disallowing the mixed ATTR_MODE and ATTR_KILL_*. I think it should be enough.

Ok, then we just need to adjust selinux_inode_setattr to understand that ATTR_FORCE only means to bypass checking on ATTR_MODE.

Quoted 3 lines

If ATTR_FORCE is confusable, I think we can just add new ATTR_FORCE_MODE or ATTR_FORCE_KILL, and replace with current ATTR_FORCE. I'm ok either way. But, with this change, ATTR_FORCE has no users.

Signature · 2 lines
Stephen Smalley
National Security Agency
#8 lore raw

Re: [Patch 1/2] selinux: ajust rules for ATTR_FORCE

Quoted 4 lines · Stephen Smalley
Quoted 2 lines

E.g. mode change has implicit ATTR_CTIME change. So it meant, we should check the both of FILE__SETATTR and FILE__WRITE?

No, just setattr.

Ok.

Quoted 10 lines
Quoted 7 lines
Quoted 3 lines

ATTR_FORCE is supposed to suppress permission checking altogether, and shouldn't be mixed with multiple attribute changes if some should be subject to permission checks while others should not.

I disagree. In fact, ATTR_FORCE is just used for ATTR_KILL_S[UG]ID, and notify_change() is disallowing the mixed ATTR_MODE and ATTR_KILL_*. I think it should be enough.

Ok, then we just need to adjust selinux_inode_setattr to understand that ATTR_FORCE only means to bypass checking on ATTR_MODE.

Ok, sure. I'll try it.

Thanks.

Signature · 1 line
OGAWA Hirofumi <[email protected]>
#9 lore raw

Re: [Patch 1/2] selinux: ajust rules for ATTR_FORCE

Quoted 4 lines · OGAWA Hirofumi
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines

ATTR_FORCE is supposed to suppress permission checking altogether, and shouldn't be mixed with multiple attribute changes if some should be subject to permission checks while others should not.

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

I disagree. In fact, ATTR_FORCE is just used for ATTR_KILL_S[UG]ID, and notify_change() is disallowing the mixed ATTR_MODE and ATTR_KILL_*. I think it should be enough.

Ok, then we just need to adjust selinux_inode_setattr to understand that ATTR_FORCE only means to bypass checking on ATTR_MODE.

Ok, sure. I'll try it.

Could you review this? I've added ATTR_TIMES_SET to check explicit utimes(), and tried it with minimum change.

[I'm not sure this handles notify_change() usage of nfsd (and perhaps other network fs too) correctly, and whether selinux may want to check it. I guess network fs _may_ try to change the multiple attributes at a time. Well, even if it's true, it would be another topic...]

Thanks.

Signature · 45 lines
OGAWA Hirofumi <[email protected]>


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: Stephen Smalley <[email protected]>
Cc: Eric Paris <[email protected]>
Signed-off-by: WANG Cong <[email protected]>
[tweaks]
Signed-off-by: OGAWA Hirofumi <[email protected]>
---

 security/selinux/hooks.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff -puN security/selinux/hooks.c~selinux-truncate-fix security/selinux/hooks.c
--- linux-2.6/security/selinux/hooks.c~selinux-truncate-fix	2009-08-18 03:50:09.000000000 +0900
+++ linux-2.6-hirofumi/security/selinux/hooks.c	2009-08-18 05:35:11.000000000 +0900
@@ -2711,12 +2711,17 @@ 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);
+		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);
_
author
#10 lore raw

Re: [Patch 1/2] selinux: ajust rules for ATTR_FORCE

Quoted 9 lines · OGAWA Hirofumi

Could you review this? I've added ATTR_TIMES_SET to check explicit utimes(), and tried it with minimum change.

[I'm not sure this handles notify_change() usage of nfsd (and perhaps other network fs too) correctly, and whether selinux may want to check it. I guess network fs _may_ try to change the multiple attributes at a time. Well, even if it's true, it would be another topic...]

Ah, according to the discussion here, I know I misunderstood ATTR_FORCE... sorry.

Quoted 9 lines

OGAWA Hirofumi <[email protected]>

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.

Quoted 6 lines
Cc Stephen Smalley <[email protected]>
Cc Eric Paris <[email protected]>
Signed-off-by WANG Cong <[email protected]>
[tweaks]
Signed-off-by OGAWA Hirofumi <[email protected]>

Great!

Some comments below.

Quoted 12 lines

---

 security/selinux/hooks.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff -puN security/selinux/hooks.c~selinux-truncate-fix security/selinux/hooks.c

security/selinux/hooks.c renamed from security/selinux/hooks.c~selinux-truncate-fix +0 -0
@@ -2711,12 +2711,17 @@ static int selinux_inode_permission(stru
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	const struct cred *cred = current_cred();

security/selinux/hooks.c static int selinux_inode_permission(stru · line 2721

Quoted 10 lines
security/selinux/hooks.c renamed from security/selinux/hooks.c~selinux-truncate-fix +6 -2
+	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);
+		if (!ia_valid)
+			return 0;

So if I read this correctly, (ATTR_FORCE| ATTR_KILL_SUID|ATTR_MODE) will not return here, since 'ia_valid' will be ATTR_FORCE finally.

I think you forgot to clear ATTR_FORCE here...

Quoted 8 lines
security/selinux/hooks.c renamed from security/selinux/hooks.c~selinux-truncate-fix +3 -2
+	}
 
-	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);
Quoted 4 lines

return dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
_

I am not sure about ATTR_TIMES_SET here, but looks fine. :-/

#11 lore raw

Re: [Patch 1/2] selinux: ajust rules for ATTR_FORCE

In reply to OGAWA Hirofumi

[Sorry if this killed thread. My ISP seems to be stopping email server now. I've read this email from web archive.]

Quoted 18 lines
Quoted 4 lines
(quoted diff) +0 -0
@@ -2711,12 +2711,17 @@ static int selinux_inode_permission(stru
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	const struct cred *cred = current_cred();

Review static int selinux_inode_permission(stru · line 2721

Quoted 10 lines
(quoted diff) +6 -2
+	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);
+		if (!ia_valid)
+			return 0;

So if I read this correctly, (ATTR_FORCE| ATTR_KILL_SUID|ATTR_MODE) will not return here, since 'ia_valid' will be ATTR_FORCE finally.

Quoted 1 line

I think you forgot to clear ATTR_FORCE here...

Whoops, good catch. Fortunately, it doesn't seem to have actual problem, but it's bug obviously, and sorry for that. Fixed patch was attached.

Thanks.

Signature · 48 lines
OGAWA Hirofumi <[email protected]>


[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: Stephen Smalley <[email protected]>
Cc: Eric Paris <[email protected]>
Signed-off-by: WANG Cong <[email protected]>
[tweaks]
Signed-off-by: OGAWA Hirofumi <[email protected]>
---

 security/selinux/hooks.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff -puN security/selinux/hooks.c~selinux-truncate-fix security/selinux/hooks.c
--- linux-2.6/security/selinux/hooks.c~selinux-truncate-fix	2009-08-18 06:27:58.000000000 +0900
+++ linux-2.6-hirofumi/security/selinux/hooks.c	2009-08-18 16:10:50.000000000 +0900
@@ -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);
_
author
#12 lore raw

Re: [Patch 1/2] selinux: ajust rules for ATTR_FORCE

Quoted 21 lines · OGAWA Hirofumi

[Sorry if this killed thread. My ISP seems to be stopping email server now. I've read this email from web archive.]

Quoted 17 lines
Quoted 5 lines
(quoted diff) +1 -0
@@ -2711,12 +2711,17 @@ 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;

Review static int selinux_inode_permission(stru · line 2722

Quoted 10 lines
(quoted diff) +5 -2
 
-	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);
+		if (!ia_valid)
+			return 0;

So if I read this correctly, (ATTR_FORCE| ATTR_KILL_SUID|ATTR_MODE) will not return here, since 'ia_valid' will be ATTR_FORCE finally.

Quoted 6 lines
Quoted 2 lines

I think you forgot to clear ATTR_FORCE here...

Whoops, good catch. Fortunately, it doesn't seem to have actual problem, but it's bug obviously, and sorry for that. Fixed patch was attached.

This version looks OK for me! Stephen, any objections?

Thank you!

Acked-by
#13 lore raw

Re: [Patch 1/2] selinux: ajust rules for ATTR_FORCE

In reply to OGAWA Hirofumi
Quoted 21 lines · OGAWA Hirofumi

[Sorry if this killed thread. My ISP seems to be stopping email server now. I've read this email from web archive.]

Quoted 18 lines
Quoted 4 lines
(quoted diff) +0 -0
@@ -2711,12 +2711,17 @@ static int selinux_inode_permission(stru
 static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
 {
 	const struct cred *cred = current_cred();

Review static int selinux_inode_permission(stru · line 2721

Quoted 10 lines
(quoted diff) +6 -2
+	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);
+		if (!ia_valid)
+			return 0;

So if I read this correctly, (ATTR_FORCE| ATTR_KILL_SUID|ATTR_MODE) will not return here, since 'ia_valid' will be ATTR_FORCE finally.

Quoted 4 lines
Quoted 1 line

I think you forgot to clear ATTR_FORCE here...

Whoops, good catch. Fortunately, it doesn't seem to have actual problem, but it's bug obviously, and sorry for that. Fixed patch was attached.

You can add my:

Acked-by Stephen Smalley <[email protected]>
Signature · 2 lines
Stephen Smalley
National Security Agency
#14 lore raw

Re: [Patch 1/2] selinux: ajust rules for ATTR_FORCE

In reply to OGAWA Hirofumi

[My ISP still seems to be stopping email server :-/]

Quoted 10 lines
Quoted 7 lines
Quoted 4 lines

So if I read this correctly, (ATTR_FORCE| ATTR_KILL_SUID|ATTR_MODE) will not return here, since 'ia_valid' will be ATTR_FORCE finally.

I think you forgot to clear ATTR_FORCE here...

Whoops, good catch. Fortunately, it doesn't seem to have actual problem, but it's bug obviously, and sorry for that. Fixed patch was attached.

You can add my:

Acked-by Stephen Smalley <[email protected]>

Thanks.

Amerigo, could you handle that patch with his ack for the remaining work?

BTW, I think [Patch 2/2] of

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

should be

	killsuid = should_remove_suid(dentry);
	if (killsuid)
		newattrs.ia_valid |= killsuid | ATTR_FORCE;

or something (someone pointed out it) on earlier thread, IIRC.

Thanks.

Signature · 1 line
OGAWA Hirofumi <[email protected]>
author
#15 lore raw

Re: [Patch 1/2] selinux: ajust rules for ATTR_FORCE

Quoted 10 lines · OGAWA Hirofumi

[My ISP still seems to be stopping email server :-/]

Quoted 7 lines
Quoted 7 lines
Quoted 4 lines

So if I read this correctly, (ATTR_FORCE| ATTR_KILL_SUID|ATTR_MODE) will not return here, since 'ia_valid' will be ATTR_FORCE finally.

I think you forgot to clear ATTR_FORCE here...

Whoops, good catch. Fortunately, it doesn't seem to have actual problem, but it's bug obviously, and sorry for that. Fixed patch was attached.

Quoted 8 lines
Quoted 3 lines

You can add my:

Acked-by Stephen Smalley <[email protected]>

Thanks.

Amerigo, could you handle that patch with his ack for the remaining work?

No problem, I will resend the newest patch with his Ack.

Quoted 8 lines

BTW, I think [Patch 2/2] of

(quoted diff) +4 -1
-	newattrs.ia_valid |= should_remove_suid(dentry);
+	ret = should_remove_suid(dentry);
+	newattrs.ia_valid |= ret;
+	if (ret)
+		newattrs.ia_valid |= ATTR_FORCE;
Quoted 6 lines

should be

	killsuid = should_remove_suid(dentry);
	if (killsuid)
		newattrs.ia_valid |= killsuid | ATTR_FORCE;

Hmm, I almost forgot this... This only saves one statement if 'ret == 0', but that is OK, I will change it.

Thanks!

author
#16 lore raw

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

In reply to Amerigo Wang

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 +7 -4
index dd98e80..cea9abf 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,15 @@ 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);
+	newattrs.ia_valid |= ret;
+	if (ret)
+		newattrs.ia_valid |= 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)