| Message ID | 534AA92B.8010805@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?) 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >> > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?) > > 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. You are right, should_remove_suid always return 0, nfsd will never call kill_suid(). Coincidentally, that's the fix for bug of root clears the SUID/SGID after writing data. The right fix should drops the kill_suid(), because vfs_writev() have do it correctly. I have push a new patch. thanks, Kinglong Mee -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
As local filesystem, writing data to the file by non-owner will clears the SUID+SGID, owner will not. Signed-off-by: Kinglong Mee <kinglongmee@gmail.com> --- fs/nfsd/vfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) if (stable) {