[V4 Patch 0/2] fix file truncations when both suid and write permissions set

[V4 Patch 0/2] fix file truncations when both suid and write permissions set

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

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 <amwang@redhat.com>

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

Thanks!

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

[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 <hirofumi@mail.parknet.co.jp>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Eric Paris <eparis@redhat.com>
Signed-off-by: WANG Cong <amwang@redhat.com>

---
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1e8cfc4..3ee3365 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -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)

[Patch 2/2] vfs: allow file truncations when both suid and write permissions set

[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 <amwang@redhat.com>
Cc: Eric Sandeen <esandeen@redhat.com>
Cc: Eric Paris <eparis@redhat.com>
Cc: Eugene Teo <eteo@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

---
diff --git a/fs/open.c b/fs/open.c
index dd98e80..cea9abf 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -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)

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

[Amerigo Wang]

Oops! A typo in $subject...

s/ajust/adjust/

:-)

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

[Stephen Smalley]

On Mon, 2009-08-17 at 03:07 -0400, Amerigo Wang wrote:
> 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 <hirofumi@mail.parknet.co.jp>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: Eric Paris <eparis@redhat.com>
> Signed-off-by: WANG Cong <amwang@redhat.com>
> 
> ---
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1e8cfc4..3ee3365 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -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;
> +	}

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

NAK.

> +	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)
-- 
Stephen Smalley
National Security Agency

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

[OGAWA Hirofumi]

Stephen Smalley <sds@tycho.nsa.gov> writes:

> On Mon, 2009-08-17 at 03:07 -0400, Amerigo Wang wrote:
>> 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.

[...]

>
> 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.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

[PATCH] selinux: adjust rules for ATTR_FORCE

From: Amerigo Wang <amwang@redhat.com>

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 <sds@tycho.nsa.gov>
Cc: Eric Paris <eparis@redhat.com>
Signed-off-by: WANG Cong <amwang@redhat.com>
[tweaks]
Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

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

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

[Stephen Smalley]

On Tue, 2009-08-18 at 03:46 +0900, OGAWA Hirofumi wrote:
> Stephen Smalley <sds@tycho.nsa.gov> writes:
> 
> > On Mon, 2009-08-17 at 03:07 -0400, Amerigo Wang wrote:
> >> 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.
> 
> [...]
> 
> >
> > 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?]

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.

-- 
Stephen Smalley
National Security Agency

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

[OGAWA Hirofumi]

Stephen Smalley <sds@tycho.nsa.gov> writes:

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

> 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.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

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

[Stephen Smalley]

On Tue, 2009-08-18 at 04:46 +0900, OGAWA Hirofumi wrote:
> Stephen Smalley <sds@tycho.nsa.gov> writes:
> 
> >> [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?

No, just setattr.

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

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

-- 
Stephen Smalley
National Security Agency

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

[OGAWA Hirofumi]

Stephen Smalley <sds@tycho.nsa.gov> writes:

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

>> > 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.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

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

[OGAWA Hirofumi]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:

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

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.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>


From: Amerigo Wang <amwang@redhat.com>

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 <sds@tycho.nsa.gov>
Cc: Eric Paris <eparis@redhat.com>
Signed-off-by: WANG Cong <amwang@redhat.com>
[tweaks]
Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 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);
_

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

[Amerigo Wang]

OGAWA Hirofumi wrote:
> 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.
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>
>
> From: Amerigo Wang <amwang@redhat.com>
>
> 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 <sds@tycho.nsa.gov>
> Cc: Eric Paris <eparis@redhat.com>
> Signed-off-by: WANG Cong <amwang@redhat.com>
> [tweaks]
> Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>   

Great!

Some comments below.

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

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

> +	}
>  
> -	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);
> _
>
>   
I am not sure about ATTR_TIMES_SET here, but looks fine. :-/

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

[OGAWA Hirofumi]

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

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

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>


[PATCH] selinux: adjust rules for ATTR_FORCE

From: Amerigo Wang <amwang@redhat.com>

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 <sds@tycho.nsa.gov>
Cc: Eric Paris <eparis@redhat.com>
Signed-off-by: WANG Cong <amwang@redhat.com>
[tweaks]
Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 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);
_

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

[Amerigo Wang]

OGAWA Hirofumi wrote:
> [Sorry if this killed thread. My ISP seems to be stopping email server
> now. I've read this email from web archive.]
>
>   
>>> @@ -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;
>>>   
>>>       
>> 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.
>   

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

Thank you!

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

[Stephen Smalley]

On Tue, 2009-08-18 at 16:39 +0900, OGAWA Hirofumi wrote:
> [Sorry if this killed thread. My ISP seems to be stopping email server
> now. I've read this email from web archive.]
> 
> >> @@ -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;
> >>   
> > 
> > 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 <sds@tycho.nsa.gov>

-- 
Stephen Smalley
National Security Agency

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

[OGAWA Hirofumi]

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

>>> 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 <sds@tycho.nsa.gov>

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.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

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

[Amerigo Wang]

OGAWA Hirofumi wrote:
> [My ISP still seems to be stopping email server :-/]
>
>   
>>>> 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 <sds@tycho.nsa.gov>
>>     
>
> 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.
> 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;
>   

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

Thanks!