Re: [PATCH] NFSD: Checking whether kill_suid by should_remove_suid()

[Kinglong Mee <kinglongmee@gmail.com>]

On 2014/4/18 21:02, J. Bruce Fields wrote:
> On Sun, Apr 13, 2014 at 11:11:39PM +0800, Kinglong Mee wrote:
>> As local filesystem, writing data to the file by non-owner will
>> clears the SUID+SGID, owner will not.
>
> Are you sure about this?  (Do you have a test case that fails?)

Sorry, maybe there are some fault of the comment,
and also, there are some problems needs rechecking.
Please ignore this patch.

I test it using command line with (root, local ext4),
touch test; chmod 4777 test; stat test; echo 1234 > test; stat test;
   File: ‘test’
   Size: 0               Blocks: 0          IO Block: 4096   regular 
empty file
Device: 803h/2051d      Inode: 1200137     Links: 1
Access: (4777/-rwsrwxrwx)  Uid: (    0/    root)   Gid: (    0/    root)
Context: unconfined_u:object_r:admin_home_t:s0
Access: 2014-04-18 21:36:31.016029014 +0800
Modify: 2014-04-18 21:36:31.016029014 +0800
Change: 2014-04-18 21:36:31.026030285 +0800
  Birth: -
   File: ‘test’
   Size: 5               Blocks: 8          IO Block: 4096   regular file
Device: 803h/2051d      Inode: 1200137     Links: 1
Access: (4777/-rwsrwxrwx)  Uid: (    0/    root)   Gid: (    0/    root)
Context: unconfined_u:object_r:admin_home_t:s0
Access: 2014-04-18 21:36:31.016029014 +0800
Modify: 2014-04-18 21:36:31.040032065 +0800
Change: 2014-04-18 21:36:31.040032065 +0800
  Birth: -

With no_root_squash, (root, remote ext4),
should not change the sgid/suid, but got

   File: ‘test’
   Size: 0               Blocks: 0          IO Block: 262144 regular 
empty file
Device: 24h/36d Inode: 786439      Links: 1
Access: (4777/-rwsrwxrwx)  Uid: ( 1000/    test)   Gid: ( 1000/    test)
Context: system_u:object_r:nfs_t:s0
Access: 2014-04-18 21:45:32.155805097 +0800
Modify: 2014-04-18 21:45:32.155805097 +0800
Change: 2014-04-18 21:45:32.168806749 +0800
  Birth: -
   File: ‘test’
   Size: 5               Blocks: 8          IO Block: 262144 regular file
Device: 24h/36d Inode: 786439      Links: 1
Access: (0777/-rwxrwxrwx)  Uid: ( 1000/    test)   Gid: ( 1000/    test)
Context: system_u:object_r:nfs_t:s0
Access: 2014-04-18 21:45:32.155805097 +0800
Modify: 2014-04-18 21:45:32.184808783 +0800
Change: 2014-04-18 21:45:32.184808783 +0800
  Birth: -

thanks,
Kinglong Mee

> I don't see an owner check in should_remove_suid.
>
> And I think that an nfsd thread will always have CAP_FSETID set (see
> cap_raise_nfsd_set and the definition of CAP_NFSD_SET), so that
> should_remove_suid() will always return 0.
>
> --b.
>
>>
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>>   fs/nfsd/vfs.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>> index 16f0673..19c0931 100644
>> --- a/fs/nfsd/vfs.c
>> +++ b/fs/nfsd/vfs.c
>> @@ -943,7 +943,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh
>> *fhp, struct file *file,
>>   	fsnotify_modify(file);
>>
>>   	/* clear setuid/setgid flag after write */
>> -	if (inode->i_mode & (S_ISUID | S_ISGID))
>> +	if (should_remove_suid(dentry))
>>   		kill_suid(dentry);
>>
>>   	if (stable) {
>> --
>> 1.9.0
>>
>