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

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

[Amerigo Wang]

V2 -> V3:
Call notify_change() before clearing suid/sgid.
Thanks to OGAWA Hirofumi.

V1 -> V2:
Introduce dentry_remove_suid(), and use it in do_truncate().
Thanks to Eric Paris.


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

---
diff --git a/fs/open.c b/fs/open.c
index dd98e80..159f5b3 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -213,11 +213,15 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
 		newattrs.ia_valid |= ATTR_FILE;
 	}
 
-	/* Remove suid/sgid on truncate too */
-	newattrs.ia_valid |= should_remove_suid(dentry);
-
 	mutex_lock(&dentry->d_inode->i_mutex);
 	err = notify_change(dentry, &newattrs);
+	if (err)
+		goto unlock;
+
+	/* Remove suid/sgid on truncate too */
+	err = dentry_remove_suid(dentry);
+
+ unlock:
 	mutex_unlock(&dentry->d_inode->i_mutex);
 	return err;
 }
diff --git a/include/linux/fs.h b/include/linux/fs.h
index a36ffa5..0481f11 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2167,6 +2167,7 @@ extern void destroy_inode(struct inode *);
 extern struct inode *new_inode(struct super_block *);
 extern int should_remove_suid(struct dentry *);
 extern int file_remove_suid(struct file *);
+extern int dentry_remove_suid(struct dentry *);
 
 extern void __insert_inode_hash(struct inode *, unsigned long hashval);
 extern void remove_inode_hash(struct inode *);
diff --git a/mm/filemap.c b/mm/filemap.c
index ccea3b6..33d94ad 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1839,9 +1839,11 @@ static int __remove_suid(struct dentry *dentry, int kill)
 	return notify_change(dentry, &newattrs);
 }
 
-int file_remove_suid(struct file *file)
+/*
+ * Note: you need to hold i_mutex before call this.
+ */
+int dentry_remove_suid(struct dentry *dentry)
 {
-	struct dentry *dentry = file->f_path.dentry;
 	int killsuid = should_remove_suid(dentry);
 	int killpriv = security_inode_need_killpriv(dentry);
 	int error = 0;
@@ -1855,6 +1857,12 @@ int file_remove_suid(struct file *file)
 
 	return error;
 }
+
+int file_remove_suid(struct file *file)
+{
+	struct dentry *dentry = file->f_path.dentry;
+	return dentry_remove_suid(dentry);
+}
 EXPORT_SYMBOL(file_remove_suid);
 
 static size_t __iovec_copy_from_user_inatomic(char *vaddr,

Re: [Patch v3] vfs: allow file truncations when both suid and write permissions set

[Eric Paris]

On Fri, 2009-08-07 at 06:05 -0400, Amerigo Wang wrote:
> V2 -> V3:
> Call notify_change() before clearing suid/sgid.
> Thanks to OGAWA Hirofumi.
> 
> V1 -> V2:
> Introduce dentry_remove_suid(), and use it in do_truncate().
> Thanks to Eric Paris.
> 
> 
> 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>
> 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: hirofumi@mail.parknet.co.jp

I was thinking about this and kept telling myself I was going to test v2
before I ack/nak.  Clearly we shouldn't for the dropping of SUID if the
process didn't have permission to change the ATTR_SIZE.

Acked-by: Eric Paris <eparis@redhat.com>

Re: [Patch v3] vfs: allow file truncations when both suid and write permissions set

[OGAWA Hirofumi]

Eric Paris <eparis@redhat.com> writes:

> On Fri, 2009-08-07 at 06:05 -0400, Amerigo Wang wrote:
>> V2 -> V3:
>> Call notify_change() before clearing suid/sgid.
>> Thanks to OGAWA Hirofumi.
>> 
>> V1 -> V2:
>> Introduce dentry_remove_suid(), and use it in do_truncate().
>> Thanks to Eric Paris.
>> 
>> 
>> 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>
>> 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: hirofumi@mail.parknet.co.jp
>
> I was thinking about this and kept telling myself I was going to test v2
> before I ack/nak.  Clearly we shouldn't for the dropping of SUID if the
> process didn't have permission to change the ATTR_SIZE.
>
> Acked-by: Eric Paris <eparis@redhat.com>

BTW, Do you know why doesn't security modules fix the handling of
do_truncate() (i.e. ATTR_MODE | ATTR_SIZE). And why doesn't it allow to
pass ATTR_FORCE for it?

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

Re: [Patch v3] vfs: allow file truncations when both suid and write permissions set

[Eric Paris]

On Sat, 2009-08-08 at 05:23 +0900, OGAWA Hirofumi wrote:
> Eric Paris <eparis@redhat.com> writes:
> 
> > On Fri, 2009-08-07 at 06:05 -0400, Amerigo Wang wrote:
> >> V2 -> V3:
> >> Call notify_change() before clearing suid/sgid.
> >> Thanks to OGAWA Hirofumi.
> >> 
> >> V1 -> V2:
> >> Introduce dentry_remove_suid(), and use it in do_truncate().
> >> Thanks to Eric Paris.
> >> 
> >> 
> >> 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>
> >> 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: hirofumi@mail.parknet.co.jp
> >
> > I was thinking about this and kept telling myself I was going to test v2
> > before I ack/nak.  Clearly we shouldn't for the dropping of SUID if the
> > process didn't have permission to change the ATTR_SIZE.
> >
> > Acked-by: Eric Paris <eparis@redhat.com>
> 
> BTW, Do you know why doesn't security modules fix the handling of
> do_truncate() (i.e. ATTR_MODE | ATTR_SIZE). And why doesn't it allow to
> pass ATTR_FORCE for it?

I'm not sure what you mean.  I understood ATTR_FORCE to mean 'I am magic
and get to override all security checks."  Which is why nothing should
ever be using ATTR_FORCE with things other than SUID.

I guess we could somehow force logic into the LSM to make it only apply
to SUID and friends but I'm not sure it buys us anything.

-Eric

Re: [Patch v3] vfs: allow file truncations when both suid and write permissions set

[OGAWA Hirofumi]

Eric Paris <eparis@redhat.com> writes:

>> > I was thinking about this and kept telling myself I was going to test v2
>> > before I ack/nak.  Clearly we shouldn't for the dropping of SUID if the
>> > process didn't have permission to change the ATTR_SIZE.
>> >
>> > Acked-by: Eric Paris <eparis@redhat.com>
>> 
>> BTW, Do you know why doesn't security modules fix the handling of
>> do_truncate() (i.e. ATTR_MODE | ATTR_SIZE). And why doesn't it allow to
>> pass ATTR_FORCE for it?
>
> I'm not sure what you mean.  I understood ATTR_FORCE to mean 'I am magic
> and get to override all security checks."  Which is why nothing should
> ever be using ATTR_FORCE with things other than SUID.
>
> I guess we could somehow force logic into the LSM to make it only apply
> to SUID and friends but I'm not sure it buys us anything.

Yes, I think it's good way. Don't we want to do the following?

	if (permission check of job)
		return error;
	if (do job at once)
        	return error;

But currently way is,

	if (permission check of first part)
        	return error
	if (do first part of job)
        	return error
	if (permission check of second part)
        	return error
	if (do second part of job)
        	return error

So, if second part was error, we may want to undo the job of first part
in theory. But, to undo is just hard and strange.

This is why I think ATTR_FORCE is good way. What do you think?

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

Re: [Patch v3] vfs: allow file truncations when both suid and write permissions set

[Amerigo Wang]

OGAWA Hirofumi wrote:
> Eric Paris <eparis@redhat.com> writes:
>
>   
>>>> I was thinking about this and kept telling myself I was going to test v2
>>>> before I ack/nak.  Clearly we shouldn't for the dropping of SUID if the
>>>> process didn't have permission to change the ATTR_SIZE.
>>>>
>>>> Acked-by: Eric Paris <eparis@redhat.com>
>>>>         
>>> BTW, Do you know why doesn't security modules fix the handling of
>>> do_truncate() (i.e. ATTR_MODE | ATTR_SIZE). And why doesn't it allow to
>>> pass ATTR_FORCE for it?
>>>       
>> I'm not sure what you mean.  I understood ATTR_FORCE to mean 'I am magic
>> and get to override all security checks."  Which is why nothing should
>> ever be using ATTR_FORCE with things other than SUID.
>>
>> I guess we could somehow force logic into the LSM to make it only apply
>> to SUID and friends but I'm not sure it buys us anything.
>>     
>
> Yes, I think it's good way. Don't we want to do the following?
>
> 	if (permission check of job)
> 		return error;
> 	if (do job at once)
>         	return error;
>
> But currently way is,
>
> 	if (permission check of first part)
>         	return error
> 	if (do first part of job)
>         	return error
> 	if (permission check of second part)
>         	return error
> 	if (do second part of job)
>         	return error
>
> So, if second part was error, we may want to undo the job of first part
> in theory. But, to undo is just hard and strange.
>   

Yeah, the problem is currently we don't have such wrappers, only 
notify_change(). :-/

Re: [Patch v3] vfs: allow file truncations when both suid and write permissions set

[OGAWA Hirofumi]

Amerigo Wang <amwang@redhat.com> writes:

> OGAWA Hirofumi wrote:
>> Eric Paris <eparis@redhat.com> writes:
>>
>>   
>>>>> I was thinking about this and kept telling myself I was going to test v2
>>>>> before I ack/nak.  Clearly we shouldn't for the dropping of SUID if the
>>>>> process didn't have permission to change the ATTR_SIZE.
>>>>>
>>>>> Acked-by: Eric Paris <eparis@redhat.com>
>>>>>         
>>>> BTW, Do you know why doesn't security modules fix the handling of
>>>> do_truncate() (i.e. ATTR_MODE | ATTR_SIZE). And why doesn't it allow to
>>>> pass ATTR_FORCE for it?
>>>>       
>>> I'm not sure what you mean.  I understood ATTR_FORCE to mean 'I am magic
>>> and get to override all security checks."  Which is why nothing should
>>> ever be using ATTR_FORCE with things other than SUID.
>>>
>>> I guess we could somehow force logic into the LSM to make it only apply
>>> to SUID and friends but I'm not sure it buys us anything.
>>>     
>>
>> Yes, I think it's good way. Don't we want to do the following?
>>
>> 	if (permission check of job)
>> 		return error;
>> 	if (do job at once)
>>         	return error;
>>
>> But currently way is,
>>
>> 	if (permission check of first part)
>>         	return error
>> 	if (do first part of job)
>>         	return error
>> 	if (permission check of second part)
>>         	return error
>> 	if (do second part of job)
>>         	return error
>>
>> So, if second part was error, we may want to undo the job of first part
>> in theory. But, to undo is just hard and strange.
>>   
>
> Yeah, the problem is currently we don't have such wrappers, only 
> notify_change(). :-/

I'm not sure you are meaning what wrappers though, I'm still thinking
changing LSM (or something) like Eric said is the way to do it easily
(and define ATTR_FORCE is not for ATTR_SIZE at least).

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

Re: [Patch v3] vfs: allow file truncations when both suid and write permissions set

[Stephen Smalley]

On Fri, 2009-08-07 at 16:38 -0400, Eric Paris wrote:
> On Sat, 2009-08-08 at 05:23 +0900, OGAWA Hirofumi wrote:
> > Eric Paris <eparis@redhat.com> writes:
> > 
> > > On Fri, 2009-08-07 at 06:05 -0400, Amerigo Wang wrote:
> > >> V2 -> V3:
> > >> Call notify_change() before clearing suid/sgid.
> > >> Thanks to OGAWA Hirofumi.
> > >> 
> > >> V1 -> V2:
> > >> Introduce dentry_remove_suid(), and use it in do_truncate().
> > >> Thanks to Eric Paris.
> > >> 
> > >> 
> > >> 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>
> > >> 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: hirofumi@mail.parknet.co.jp
> > >
> > > I was thinking about this and kept telling myself I was going to test v2
> > > before I ack/nak.  Clearly we shouldn't for the dropping of SUID if the
> > > process didn't have permission to change the ATTR_SIZE.
> > >
> > > Acked-by: Eric Paris <eparis@redhat.com>
> > 
> > BTW, Do you know why doesn't security modules fix the handling of
> > do_truncate() (i.e. ATTR_MODE | ATTR_SIZE). And why doesn't it allow to
> > pass ATTR_FORCE for it?
> 
> I'm not sure what you mean.  I understood ATTR_FORCE to mean 'I am magic
> and get to override all security checks."  Which is why nothing should
> ever be using ATTR_FORCE with things other than SUID.
> 
> I guess we could somehow force logic into the LSM to make it only apply
> to SUID and friends but I'm not sure it buys us anything.

SELinux shouldn't apply a permission check for the clearing of the suid
bit on write or truncate.  It should only apply a permission check for
the actual truncate or write operation, and then the clearing of the
suid bit should always be forced if that check passed.

-- 
Stephen Smalley
National Security Agency

Re: [Patch v3] vfs: allow file truncations when both suid and write permissions set

[OGAWA Hirofumi]

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

>> > > I was thinking about this and kept telling myself I was going to test v2
>> > > before I ack/nak.  Clearly we shouldn't for the dropping of SUID if the
>> > > process didn't have permission to change the ATTR_SIZE.
>> > >
>> > > Acked-by: Eric Paris <eparis@redhat.com>
>> > 
>> > BTW, Do you know why doesn't security modules fix the handling of
>> > do_truncate() (i.e. ATTR_MODE | ATTR_SIZE). And why doesn't it allow to
>> > pass ATTR_FORCE for it?
>> 
>> I'm not sure what you mean.  I understood ATTR_FORCE to mean 'I am magic
>> and get to override all security checks."  Which is why nothing should
>> ever be using ATTR_FORCE with things other than SUID.
>> 
>> I guess we could somehow force logic into the LSM to make it only apply
>> to SUID and friends but I'm not sure it buys us anything.
>
> SELinux shouldn't apply a permission check for the clearing of the suid
> bit on write or truncate.  It should only apply a permission check for
> the actual truncate or write operation, and then the clearing of the
> suid bit should always be forced if that check passed.

Ok. Yes. So, to do it efficiently without problem, I'm suggesting the
following or something (I don't know whether LSM should do this or not).

selinux_inode_setattr(),

	ia_valid = iattr->ia_valid;
	if (!(ia_valid & ATTR_FORCE) && (ia_valid & ATTR_FORCE_MASK)) {
		err = dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
		if (err)
                	return err;
		ia_valid &= ~ATTR_FORCE_MASK;
	}
	if (ia_valid & ATTR_NOT_FORCE_MASK)
	 	err = dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
	return err;

I guess ATTR_FORCE_MASK would be (ATTR_MODE | ATTR_UID | ATTR_GID |
			ATTR_ATIME_SET | ATTR_MTIME_SET) or something,
and ATTR_NOT_FORCE_MASK would be ATTR_SIZE or something.

I'm not sure this is the right code what selinux want to do though, but,
I hope it is clear what I want to say. (I'm assuming FILE__WRITE is for
check of ATTR_SIZE)

With this change, the caller can pass "(ATTR_SIZE | ATTR_MODE)" or
"(ATTR_SIZE | ATTR_MODE | ATTR_FORCE)" etc. for truncate().

[btw, "(ATTR_SIZE | ATTR_MODE)" is what do_truncate() does currently].

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

Re: [Patch v3] vfs: allow file truncations when both suid and write permissions set

[Stephen Smalley]

On Mon, 2009-08-10 at 21:43 +0900, OGAWA Hirofumi wrote:
> Stephen Smalley <sds@tycho.nsa.gov> writes:
> 
> >> > > I was thinking about this and kept telling myself I was going to test v2
> >> > > before I ack/nak.  Clearly we shouldn't for the dropping of SUID if the
> >> > > process didn't have permission to change the ATTR_SIZE.
> >> > >
> >> > > Acked-by: Eric Paris <eparis@redhat.com>
> >> > 
> >> > BTW, Do you know why doesn't security modules fix the handling of
> >> > do_truncate() (i.e. ATTR_MODE | ATTR_SIZE). And why doesn't it allow to
> >> > pass ATTR_FORCE for it?
> >> 
> >> I'm not sure what you mean.  I understood ATTR_FORCE to mean 'I am magic
> >> and get to override all security checks."  Which is why nothing should
> >> ever be using ATTR_FORCE with things other than SUID.
> >> 
> >> I guess we could somehow force logic into the LSM to make it only apply
> >> to SUID and friends but I'm not sure it buys us anything.
> >
> > SELinux shouldn't apply a permission check for the clearing of the suid
> > bit on write or truncate.  It should only apply a permission check for
> > the actual truncate or write operation, and then the clearing of the
> > suid bit should always be forced if that check passed.
> 
> Ok. Yes. So, to do it efficiently without problem, I'm suggesting the
> following or something (I don't know whether LSM should do this or not).
> 
> selinux_inode_setattr(),
> 
> 	ia_valid = iattr->ia_valid;
> 	if (!(ia_valid & ATTR_FORCE) && (ia_valid & ATTR_FORCE_MASK)) {
> 		err = dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
> 		if (err)
>                 	return err;
> 		ia_valid &= ~ATTR_FORCE_MASK;
> 	}
> 	if (ia_valid & ATTR_NOT_FORCE_MASK)
> 	 	err = dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
> 	return err;
> 
> I guess ATTR_FORCE_MASK would be (ATTR_MODE | ATTR_UID | ATTR_GID |
> 			ATTR_ATIME_SET | ATTR_MTIME_SET) or something,
> and ATTR_NOT_FORCE_MASK would be ATTR_SIZE or something.
> 
> I'm not sure this is the right code what selinux want to do though, but,
> I hope it is clear what I want to say. (I'm assuming FILE__WRITE is for
> check of ATTR_SIZE)

The logic is supposed to map certain attribute changes (mode, owner,
group, explicit setting of atime or mtime to a specific value rather
than the current time) to the SELinux setattr permission, while mapping
other attribute changes that occur naturally on a write (size, setting
of mtime to current time) to the SELinux write permission.  That doesn't
seem clear from using ATTR_FORCE_MASK vs ATTR_NOT_FORCE_MASK above - I'd
use different naming conventions for clarity.

> 
> With this change, the caller can pass "(ATTR_SIZE | ATTR_MODE)" or
> "(ATTR_SIZE | ATTR_MODE | ATTR_FORCE)" etc. for truncate().
> 
> [btw, "(ATTR_SIZE | ATTR_MODE)" is what do_truncate() does currently].

That was a change in do_truncate(), commit
7b82dc0e64e93f430182f36b46b79fcee87d3532.

It makes sense, but no one ever updated selinux_inode_setattr() to match
that change.

-- 
Stephen Smalley
National Security Agency

Re: [Patch v3] vfs: allow file truncations when both suid and write permissions set

[OGAWA Hirofumi]

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

>> > SELinux shouldn't apply a permission check for the clearing of the suid
>> > bit on write or truncate.  It should only apply a permission check for
>> > the actual truncate or write operation, and then the clearing of the
>> > suid bit should always be forced if that check passed.
>> 
>> Ok. Yes. So, to do it efficiently without problem, I'm suggesting the
>> following or something (I don't know whether LSM should do this or not).
>> 
>> selinux_inode_setattr(),
>> 
>> 	ia_valid = iattr->ia_valid;
>> 	if (!(ia_valid & ATTR_FORCE) && (ia_valid & ATTR_FORCE_MASK)) {
>> 		err = dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
>> 		if (err)
>>                 	return err;
>> 		ia_valid &= ~ATTR_FORCE_MASK;
>> 	}
>> 	if (ia_valid & ATTR_NOT_FORCE_MASK)
>> 	 	err = dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
>> 	return err;
>> 
>> I guess ATTR_FORCE_MASK would be (ATTR_MODE | ATTR_UID | ATTR_GID |
>> 			ATTR_ATIME_SET | ATTR_MTIME_SET) or something,
>> and ATTR_NOT_FORCE_MASK would be ATTR_SIZE or something.
>> 
>> I'm not sure this is the right code what selinux want to do though, but,
>> I hope it is clear what I want to say. (I'm assuming FILE__WRITE is for
>> check of ATTR_SIZE)
>
> The logic is supposed to map certain attribute changes (mode, owner,
> group, explicit setting of atime or mtime to a specific value rather
> than the current time) to the SELinux setattr permission, while mapping
> other attribute changes that occur naturally on a write (size, setting
> of mtime to current time) to the SELinux write permission.  That doesn't
> seem clear from using ATTR_FORCE_MASK vs ATTR_NOT_FORCE_MASK above - I'd
> use different naming conventions for clarity.

I see. Yes, the naming of this code doesn't matter at all. The code was
just intended to explain what I'm suggesting.

>> With this change, the caller can pass "(ATTR_SIZE | ATTR_MODE)" or
>> "(ATTR_SIZE | ATTR_MODE | ATTR_FORCE)" etc. for truncate().
>> 
>> [btw, "(ATTR_SIZE | ATTR_MODE)" is what do_truncate() does currently].
>
> That was a change in do_truncate(), commit
> 7b82dc0e64e93f430182f36b46b79fcee87d3532.
>
> It makes sense, but no one ever updated selinux_inode_setattr() to match
> that change.

I see. Yes, exactly. And for the user of non file owner case, I'm
thinking we would like to pass ATTR_FORCE too.

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

Re: [Patch v3] vfs: allow file truncations when both suid and write permissions set

[Amerigo Wang]

OGAWA Hirofumi wrote:
> Stephen Smalley <sds@tycho.nsa.gov> writes:
>
>   
>>>> SELinux shouldn't apply a permission check for the clearing of the suid
>>>> bit on write or truncate.  It should only apply a permission check for
>>>> the actual truncate or write operation, and then the clearing of the
>>>> suid bit should always be forced if that check passed.
>>>>         
>>> Ok. Yes. So, to do it efficiently without problem, I'm suggesting the
>>> following or something (I don't know whether LSM should do this or not).
>>>
>>> selinux_inode_setattr(),
>>>
>>> 	ia_valid = iattr->ia_valid;
>>> 	if (!(ia_valid & ATTR_FORCE) && (ia_valid & ATTR_FORCE_MASK)) {
>>> 		err = dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
>>> 		if (err)
>>>                 	return err;
>>> 		ia_valid &= ~ATTR_FORCE_MASK;
>>> 	}
>>> 	if (ia_valid & ATTR_NOT_FORCE_MASK)
>>> 	 	err = dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
>>> 	return err;
>>>
>>> I guess ATTR_FORCE_MASK would be (ATTR_MODE | ATTR_UID | ATTR_GID |
>>> 			ATTR_ATIME_SET | ATTR_MTIME_SET) or something,
>>> and ATTR_NOT_FORCE_MASK would be ATTR_SIZE or something.
>>>
>>> I'm not sure this is the right code what selinux want to do though, but,
>>> I hope it is clear what I want to say. (I'm assuming FILE__WRITE is for
>>> check of ATTR_SIZE)
>>>       
>> The logic is supposed to map certain attribute changes (mode, owner,
>> group, explicit setting of atime or mtime to a specific value rather
>> than the current time) to the SELinux setattr permission, while mapping
>> other attribute changes that occur naturally on a write (size, setting
>> of mtime to current time) to the SELinux write permission.  That doesn't
>> seem clear from using ATTR_FORCE_MASK vs ATTR_NOT_FORCE_MASK above - I'd
>> use different naming conventions for clarity.
>>     
>
> I see. Yes, the naming of this code doesn't matter at all. The code was
> just intended to explain what I'm suggesting.
>
>   
>>> With this change, the caller can pass "(ATTR_SIZE | ATTR_MODE)" or
>>> "(ATTR_SIZE | ATTR_MODE | ATTR_FORCE)" etc. for truncate().
>>>
>>> [btw, "(ATTR_SIZE | ATTR_MODE)" is what do_truncate() does currently].
>>>       
>> That was a change in do_truncate(), commit
>> 7b82dc0e64e93f430182f36b46b79fcee87d3532.
>>
>> It makes sense, but no one ever updated selinux_inode_setattr() to match
>> that change.
>>     
>
> I see. Yes, exactly. And for the user of non file owner case, I'm
> thinking we would like to pass ATTR_FORCE too.
>
>   
Ok, I know what you are talking about now...

Maybe I should make another patch to fix this... :-/ I need some time to 
understand SELinux, before I finish it, keeping this patch is fine.

Thanks!