nfs vs xfstests 193

nfs vs xfstests 193

[Christoph Hellwig]

[-- Attachment #1: Type: text/plain, Size: 536 bytes --]

I've noticed that xfstests 193 fails when run over NFS talking to an
XFS-based Linux server.  The test checks that we behave correctly
vs Posix 1003.1 for the various operations that end up in ->setattr.

Without the no_root_squash export flag we're not even able to run
something resembling the test as we get permission problems all through
it, see the first attachment for details.

With the root_squash export op we fail to clear the setuid/setgid
bits in various truncate and chown subtests, see the second attachment
for details.

[-- Attachment #2: 193.no_root_squash --]
[-- Type: text/plain, Size: 862 bytes --]

--- tests/generic/193.out	2013-10-24 15:41:33.000000000 +0000
+++ /root/xfstests/results//generic/193.out.bad	2013-11-06 11:55:39.000000000 +0000
@@ -35,26 +35,26 @@
 after:  -rw-r-Sr--
 with user exec perm
 before: -rwsr-Sr--
-after:  -rwxr-Sr--
+after:  -rwsr-Sr--
 with group exec perm
 before: -rwSr-sr--
-after:  -rw-r-xr--
+after:  -rwSr-sr--
 with user+group exec perm
 before: -rwsr-sr--
-after:  -rwxr-xr--
+after:  -rwsr-sr--
 check that suid/sgid bits are cleared after successful truncate...
 with no exec perm
 before: -rwSr-Sr--
-after:  -rw-r-Sr--
+after:  -rwSr-Sr--
 with user exec perm
 before: -rwsr-Sr--
-after:  -rwxr-Sr--
+after:  -rwsr-Sr--
 with group exec perm
 before: -rwSr-sr--
-after:  -rw-r-xr--
+after:  -rwSr-sr--
 with user+group exec perm
 before: -rwsr-sr--
-after:  -rwxr-xr--
+after:  -rwsr-sr--
 
 testing ATTR_*TIMES_SET
 

[-- Attachment #3: 193.root_squash --]
[-- Type: text/plain, Size: 7892 bytes --]

--- tests/generic/193.out	2013-10-24 15:41:33.000000000 +0000
+++ /root/xfstests/results//generic/193.out.bad	2013-11-06 11:52:05.000000000 +0000
@@ -2,63 +2,118 @@
 
 testing ATTR_UID
 
+touch: cannot touch '/mnt/nfs1/193.3516.root': Permission denied
+touch: cannot touch '/mnt/nfs1/193.3516.user': Permission denied
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 user: chown root owned file to qa_user (should fail)
-chown: changing ownership of 'test.root': Operation not permitted
+chown: cannot access 'test.root': No such file or directory
 user: chown root owned file to root (should fail)
-chown: changing ownership of 'test.root': Operation not permitted
+chown: cannot access 'test.root': No such file or directory
 user: chown qa_user owned file to qa_user (should succeed)
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 user: chown qa_user owned file to root (should fail)
-chown: changing ownership of 'test.user': Operation not permitted
+chown: cannot access 'test.user': No such file or directory
 
 testing ATTR_GID
 
+touch: cannot touch '/mnt/nfs1/193.3516.root': Permission denied
+touch: cannot touch '/mnt/nfs1/193.3516.user': Permission denied
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 user: chgrp root owned file to root (should fail)
-chgrp: changing group of 'test.root': Operation not permitted
+chgrp: cannot access 'test.root': No such file or directory
 user: chgrp qa_user owned file to root (should fail)
-chgrp: changing group of 'test.user': Operation not permitted
+chgrp: cannot access 'test.user': No such file or directory
 user: chgrp root owned file to qa_user (should fail)
-chgrp: changing group of 'test.root': Operation not permitted
+chgrp: cannot access 'test.root': No such file or directory
 user: chgrp qa_user owned file to qa_user (should succeed)
+chgrp: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 
 testing ATTR_MODE
 
+touch: cannot touch '/mnt/nfs1/193.3516.root': Permission denied
+touch: cannot touch '/mnt/nfs1/193.3516.user': Permission denied
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 user: chmod a+r on qa_user owned file (should succeed)
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 user: chmod a+r on root owned file (should fail)
-chmod: changing permissions of 'test.root': Operation not permitted
+chmod: cannot access 'test.root': No such file or directory
 check that the sgid bit is cleared
--rw-rw-rw-
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
 check that suid bit is not cleared
--rwSrw-rw-
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+touch: cannot touch '/mnt/nfs1/193.3516.root': Permission denied
+touch: cannot touch '/mnt/nfs1/193.3516.user': Permission denied
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 check that suid/sgid bits are cleared after successful chown...
 with no exec perm
-before: -rwSr-Sr--
-after:  -rw-r-Sr--
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+before: stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+after:  stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
 with user exec perm
-before: -rwsr-Sr--
-after:  -rwxr-Sr--
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+before: stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+after:  stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
 with group exec perm
-before: -rwSr-sr--
-after:  -rw-r-xr--
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+before: stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+after:  stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
 with user+group exec perm
-before: -rwsr-sr--
-after:  -rwxr-xr--
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+before: stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+after:  stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+touch: cannot touch '/mnt/nfs1/193.3516.root': Permission denied
+touch: cannot touch '/mnt/nfs1/193.3516.user': Permission denied
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 check that suid/sgid bits are cleared after successful truncate...
 with no exec perm
-before: -rwSr-Sr--
-after:  -rw-r-Sr--
+./tests/generic/193: line 242: /mnt/nfs1/193.3516.user: Permission denied
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+before: stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+bash: /mnt/nfs1/193.3516.user: Permission denied
+after:  stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
 with user exec perm
-before: -rwsr-Sr--
-after:  -rwxr-Sr--
+./tests/generic/193: line 249: /mnt/nfs1/193.3516.user: Permission denied
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+before: stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+bash: /mnt/nfs1/193.3516.user: Permission denied
+after:  stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
 with group exec perm
-before: -rwSr-sr--
-after:  -rw-r-xr--
+./tests/generic/193: line 257: /mnt/nfs1/193.3516.user: Permission denied
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+before: stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+bash: /mnt/nfs1/193.3516.user: Permission denied
+after:  stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
 with user+group exec perm
-before: -rwsr-sr--
-after:  -rwxr-xr--
+./tests/generic/193: line 266: /mnt/nfs1/193.3516.user: Permission denied
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+before: stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+bash: /mnt/nfs1/193.3516.user: Permission denied
+after:  stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
 
 testing ATTR_*TIMES_SET
 
+touch: cannot touch '/mnt/nfs1/193.3516.root': Permission denied
+touch: cannot touch '/mnt/nfs1/193.3516.user': Permission denied
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 user: touch qa_user file (should succeed)
+touch: cannot touch '/mnt/nfs1/193.3516.user': Permission denied
 user: touch root file (should fail)
 touch: cannot touch 'test.root': Permission denied
 *** done

[-- Attachment #4: Type: text/plain, Size: 121 bytes --]

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

nfs vs xfstests 193

[Christoph Hellwig]

[-- Attachment #1: Type: text/plain, Size: 536 bytes --]

I've noticed that xfstests 193 fails when run over NFS talking to an
XFS-based Linux server.  The test checks that we behave correctly
vs Posix 1003.1 for the various operations that end up in ->setattr.

Without the no_root_squash export flag we're not even able to run
something resembling the test as we get permission problems all through
it, see the first attachment for details.

With the root_squash export op we fail to clear the setuid/setgid
bits in various truncate and chown subtests, see the second attachment
for details.

[-- Attachment #2: 193.no_root_squash --]
[-- Type: text/plain, Size: 862 bytes --]

--- tests/generic/193.out	2013-10-24 15:41:33.000000000 +0000
+++ /root/xfstests/results//generic/193.out.bad	2013-11-06 11:55:39.000000000 +0000
@@ -35,26 +35,26 @@
 after:  -rw-r-Sr--
 with user exec perm
 before: -rwsr-Sr--
-after:  -rwxr-Sr--
+after:  -rwsr-Sr--
 with group exec perm
 before: -rwSr-sr--
-after:  -rw-r-xr--
+after:  -rwSr-sr--
 with user+group exec perm
 before: -rwsr-sr--
-after:  -rwxr-xr--
+after:  -rwsr-sr--
 check that suid/sgid bits are cleared after successful truncate...
 with no exec perm
 before: -rwSr-Sr--
-after:  -rw-r-Sr--
+after:  -rwSr-Sr--
 with user exec perm
 before: -rwsr-Sr--
-after:  -rwxr-Sr--
+after:  -rwsr-Sr--
 with group exec perm
 before: -rwSr-sr--
-after:  -rw-r-xr--
+after:  -rwSr-sr--
 with user+group exec perm
 before: -rwsr-sr--
-after:  -rwxr-xr--
+after:  -rwsr-sr--
 
 testing ATTR_*TIMES_SET
 

[-- Attachment #3: 193.root_squash --]
[-- Type: text/plain, Size: 7892 bytes --]

--- tests/generic/193.out	2013-10-24 15:41:33.000000000 +0000
+++ /root/xfstests/results//generic/193.out.bad	2013-11-06 11:52:05.000000000 +0000
@@ -2,63 +2,118 @@
 
 testing ATTR_UID
 
+touch: cannot touch '/mnt/nfs1/193.3516.root': Permission denied
+touch: cannot touch '/mnt/nfs1/193.3516.user': Permission denied
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 user: chown root owned file to qa_user (should fail)
-chown: changing ownership of 'test.root': Operation not permitted
+chown: cannot access 'test.root': No such file or directory
 user: chown root owned file to root (should fail)
-chown: changing ownership of 'test.root': Operation not permitted
+chown: cannot access 'test.root': No such file or directory
 user: chown qa_user owned file to qa_user (should succeed)
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 user: chown qa_user owned file to root (should fail)
-chown: changing ownership of 'test.user': Operation not permitted
+chown: cannot access 'test.user': No such file or directory
 
 testing ATTR_GID
 
+touch: cannot touch '/mnt/nfs1/193.3516.root': Permission denied
+touch: cannot touch '/mnt/nfs1/193.3516.user': Permission denied
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 user: chgrp root owned file to root (should fail)
-chgrp: changing group of 'test.root': Operation not permitted
+chgrp: cannot access 'test.root': No such file or directory
 user: chgrp qa_user owned file to root (should fail)
-chgrp: changing group of 'test.user': Operation not permitted
+chgrp: cannot access 'test.user': No such file or directory
 user: chgrp root owned file to qa_user (should fail)
-chgrp: changing group of 'test.root': Operation not permitted
+chgrp: cannot access 'test.root': No such file or directory
 user: chgrp qa_user owned file to qa_user (should succeed)
+chgrp: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 
 testing ATTR_MODE
 
+touch: cannot touch '/mnt/nfs1/193.3516.root': Permission denied
+touch: cannot touch '/mnt/nfs1/193.3516.user': Permission denied
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 user: chmod a+r on qa_user owned file (should succeed)
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 user: chmod a+r on root owned file (should fail)
-chmod: changing permissions of 'test.root': Operation not permitted
+chmod: cannot access 'test.root': No such file or directory
 check that the sgid bit is cleared
--rw-rw-rw-
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
 check that suid bit is not cleared
--rwSrw-rw-
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+touch: cannot touch '/mnt/nfs1/193.3516.root': Permission denied
+touch: cannot touch '/mnt/nfs1/193.3516.user': Permission denied
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 check that suid/sgid bits are cleared after successful chown...
 with no exec perm
-before: -rwSr-Sr--
-after:  -rw-r-Sr--
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+before: stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+after:  stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
 with user exec perm
-before: -rwsr-Sr--
-after:  -rwxr-Sr--
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+before: stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+after:  stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
 with group exec perm
-before: -rwSr-sr--
-after:  -rw-r-xr--
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+before: stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+after:  stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
 with user+group exec perm
-before: -rwsr-sr--
-after:  -rwxr-xr--
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+before: stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+after:  stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+touch: cannot touch '/mnt/nfs1/193.3516.root': Permission denied
+touch: cannot touch '/mnt/nfs1/193.3516.user': Permission denied
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 check that suid/sgid bits are cleared after successful truncate...
 with no exec perm
-before: -rwSr-Sr--
-after:  -rw-r-Sr--
+./tests/generic/193: line 242: /mnt/nfs1/193.3516.user: Permission denied
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+before: stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+bash: /mnt/nfs1/193.3516.user: Permission denied
+after:  stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
 with user exec perm
-before: -rwsr-Sr--
-after:  -rwxr-Sr--
+./tests/generic/193: line 249: /mnt/nfs1/193.3516.user: Permission denied
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+before: stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+bash: /mnt/nfs1/193.3516.user: Permission denied
+after:  stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
 with group exec perm
-before: -rwSr-sr--
-after:  -rw-r-xr--
+./tests/generic/193: line 257: /mnt/nfs1/193.3516.user: Permission denied
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+before: stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+bash: /mnt/nfs1/193.3516.user: Permission denied
+after:  stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
 with user+group exec perm
-before: -rwsr-sr--
-after:  -rwxr-xr--
+./tests/generic/193: line 266: /mnt/nfs1/193.3516.user: Permission denied
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+chmod: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
+before: stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
+bash: /mnt/nfs1/193.3516.user: Permission denied
+after:  stat: cannot stat '/mnt/nfs1/193.3516.user': No such file or directory
 
 testing ATTR_*TIMES_SET
 
+touch: cannot touch '/mnt/nfs1/193.3516.root': Permission denied
+touch: cannot touch '/mnt/nfs1/193.3516.user': Permission denied
+chown: cannot access '/mnt/nfs1/193.3516.user': No such file or directory
 user: touch qa_user file (should succeed)
+touch: cannot touch '/mnt/nfs1/193.3516.user': Permission denied
 user: touch root file (should fail)
 touch: cannot touch 'test.root': Permission denied
 *** done

Re: nfs vs xfstests 193

[Stanislav Kholmanskikh]

On 11/06/2013 03:56 PM, Christoph Hellwig wrote:
> I've noticed that xfstests 193 fails when run over NFS talking to an
> XFS-based Linux server.  The test checks that we behave correctly
> vs Posix 1003.1 for the various operations that end up in ->setattr.
>
> Without the no_root_squash export flag we're not even able to run
> something resembling the test as we get permission problems all through
> it, see the first attachment for details.
>
> With the root_squash export op we fail to clear the setuid/setgid
> bits in various truncate and chown subtests, see the second attachment
> for details.
Hi!

I've come across  the same issue. But NFS server is backed by ext4 file 
system in my environment.

The test case quotes POSIX:

"If the specified file is a regular file, one or more of the S_IXUSR, 
S_IXGRP, or S_IXOTH bits of the
file mode are set, and the process has appropriate privileges, it is 
implementation-defined whether the set-user-ID and set-group-ID
bits are altered."

So the difference that what we have now:

between nfs:
~# touch file; chmod ug+s file; chmod u+x file; ls -l file; chown root 
file; ls -l file; rm -f file
-rwsr-Sr-- 1 root root 0 Dec  6 04:49 file
-rwsr-Sr-- 1 root root 0 Dec  6 04:49 file

and ext3, ext4, xfs, btrfs:
~# touch file; chmod ug+s file; chmod u+x file; ls -l file; chown root 
file; ls -l file; rm -f file
-rwsr-Sr-- 1 root root 0 Dec  6 04:49 file
-rwxr-Sr-- 1 root root 0 Dec  6 04:49 file

is not a violation of this POSIX statement. But It's just an 
"implementation-defined" behaviour.

I suppose that the difference raises because of this part of code in 
fs/nfsd/vfs.c:

         /* Revoke setuid/setgid on chown */
         if (!S_ISDIR(inode->i_mode) &&
             (((iap->ia_valid & ATTR_UID) && !uid_eq(iap->ia_uid, 
inode->i_uid)) ||
              ((iap->ia_valid & ATTR_GID) && !gid_eq(iap->ia_gid, 
inode->i_gid)))) {
                 iap->ia_valid |= ATTR_KILL_PRIV;
                 if (iap->ia_valid & ATTR_MODE) {
                         /* we're setting mode too, just clear the s*id 
bits */
                         iap->ia_mode &= ~S_ISUID;
                         if (iap->ia_mode & S_IXGRP)
                                 iap->ia_mode &= ~S_ISGID;
                 } else {
                         /* set ATTR_KILL_* bits and let VFS handle it */
                         iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID);
                 }
         }

uid_eq() and gid_eq() checkings allow removal of s*id bits only if the 
owner/group of the file is changed during chown().

I.e. on nfs:
~# touch file; chmod ug+s file; chmod u+x file; ls -l file; chown bin 
file; ls -l file; rm -f file
-rwsr-Sr-- 1 root root 0 Dec  6 05:02 file
-rwxr-Sr-- 1 bin root 0 Dec  6 05:02 file

Is it acceptable to change NFS kernel server behaviour by removal of 
!uid_eq(iap->ia_uid, inode->i_uid) and !gid_eq(iap->ia_gid, 
inode->i_gid) from the condition above?

Just to make the behaviour more consistent between NFS and other "local" 
file systems as It was done by
commit 
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=0953e620de0538cbd081f1b45126f6098112a598

Thank you!



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: nfs vs xfstests 193

[Stanislav Kholmanskikh]

On 11/06/2013 03:56 PM, Christoph Hellwig wrote:
> I've noticed that xfstests 193 fails when run over NFS talking to an
> XFS-based Linux server.  The test checks that we behave correctly
> vs Posix 1003.1 for the various operations that end up in ->setattr.
>
> Without the no_root_squash export flag we're not even able to run
> something resembling the test as we get permission problems all through
> it, see the first attachment for details.
>
> With the root_squash export op we fail to clear the setuid/setgid
> bits in various truncate and chown subtests, see the second attachment
> for details.
Hi!

I've come across  the same issue. But NFS server is backed by ext4 file 
system in my environment.

The test case quotes POSIX:

"If the specified file is a regular file, one or more of the S_IXUSR, 
S_IXGRP, or S_IXOTH bits of the
file mode are set, and the process has appropriate privileges, it is 
implementation-defined whether the set-user-ID and set-group-ID
bits are altered."

So the difference that what we have now:

between nfs:
~# touch file; chmod ug+s file; chmod u+x file; ls -l file; chown root 
file; ls -l file; rm -f file
-rwsr-Sr-- 1 root root 0 Dec  6 04:49 file
-rwsr-Sr-- 1 root root 0 Dec  6 04:49 file

and ext3, ext4, xfs, btrfs:
~# touch file; chmod ug+s file; chmod u+x file; ls -l file; chown root 
file; ls -l file; rm -f file
-rwsr-Sr-- 1 root root 0 Dec  6 04:49 file
-rwxr-Sr-- 1 root root 0 Dec  6 04:49 file

is not a violation of this POSIX statement. But It's just an 
"implementation-defined" behaviour.

I suppose that the difference raises because of this part of code in 
fs/nfsd/vfs.c:

         /* Revoke setuid/setgid on chown */
         if (!S_ISDIR(inode->i_mode) &&
             (((iap->ia_valid & ATTR_UID) && !uid_eq(iap->ia_uid, 
inode->i_uid)) ||
              ((iap->ia_valid & ATTR_GID) && !gid_eq(iap->ia_gid, 
inode->i_gid)))) {
                 iap->ia_valid |= ATTR_KILL_PRIV;
                 if (iap->ia_valid & ATTR_MODE) {
                         /* we're setting mode too, just clear the s*id 
bits */
                         iap->ia_mode &= ~S_ISUID;
                         if (iap->ia_mode & S_IXGRP)
                                 iap->ia_mode &= ~S_ISGID;
                 } else {
                         /* set ATTR_KILL_* bits and let VFS handle it */
                         iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID);
                 }
         }

uid_eq() and gid_eq() checkings allow removal of s*id bits only if the 
owner/group of the file is changed during chown().

I.e. on nfs:
~# touch file; chmod ug+s file; chmod u+x file; ls -l file; chown bin 
file; ls -l file; rm -f file
-rwsr-Sr-- 1 root root 0 Dec  6 05:02 file
-rwxr-Sr-- 1 bin root 0 Dec  6 05:02 file

Is it acceptable to change NFS kernel server behaviour by removal of 
!uid_eq(iap->ia_uid, inode->i_uid) and !gid_eq(iap->ia_gid, 
inode->i_gid) from the condition above?

Just to make the behaviour more consistent between NFS and other "local" 
file systems as It was done by
commit 
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=0953e620de0538cbd081f1b45126f6098112a598

Thank you!

Re: nfs vs xfstests 193

[Christoph Hellwig]

On Fri, Dec 06, 2013 at 05:20:34PM +0400, Stanislav Kholmanskikh wrote:
> Just to make the behaviour more consistent between NFS and other
> "local" file systems as It was done by
> commit https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=0953e620de0538cbd081f1b45126f6098112a598

Seems like we got others in line with XFS behavior.  I'd prefer to
have NFS follow this as well.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: nfs vs xfstests 193

[Christoph Hellwig]

On Fri, Dec 06, 2013 at 05:20:34PM +0400, Stanislav Kholmanskikh wrote:
> Just to make the behaviour more consistent between NFS and other
> "local" file systems as It was done by
> commit https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=0953e620de0538cbd081f1b45126f6098112a598

Seems like we got others in line with XFS behavior.  I'd prefer to
have NFS follow this as well.

Re: nfs vs xfstests 193

[J. Bruce Fields]

On Fri, Dec 06, 2013 at 10:08:58AM -0800, Christoph Hellwig wrote:
> On Fri, Dec 06, 2013 at 05:20:34PM +0400, Stanislav Kholmanskikh wrote:
> > Just to make the behaviour more consistent between NFS and other
> > "local" file systems as It was done by
> > commit https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=0953e620de0538cbd081f1b45126f6098112a598
> 
> Seems like we got others in line with XFS behavior.

But, not having tested the behavior, it looks like fs/open.c has a
simlar !S_ISDIR() check.  Where's that behavior implemented?

> I'd prefer to have NFS follow this as well.

Huh.  Sachin, do you remember if there was any other motivation behind
that patch?

--b.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: nfs vs xfstests 193

[J. Bruce Fields]

On Fri, Dec 06, 2013 at 10:08:58AM -0800, Christoph Hellwig wrote:
> On Fri, Dec 06, 2013 at 05:20:34PM +0400, Stanislav Kholmanskikh wrote:
> > Just to make the behaviour more consistent between NFS and other
> > "local" file systems as It was done by
> > commit https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=0953e620de0538cbd081f1b45126f6098112a598
> 
> Seems like we got others in line with XFS behavior.

But, not having tested the behavior, it looks like fs/open.c has a
simlar !S_ISDIR() check.  Where's that behavior implemented?

> I'd prefer to have NFS follow this as well.

Huh.  Sachin, do you remember if there was any other motivation behind
that patch?

--b.

Re: nfs vs xfstests 193

[J. Bruce Fields]

On Fri, Dec 06, 2013 at 03:44:04PM -0500, bfields wrote:
> On Fri, Dec 06, 2013 at 10:08:58AM -0800, Christoph Hellwig wrote:
> > On Fri, Dec 06, 2013 at 05:20:34PM +0400, Stanislav Kholmanskikh wrote:
> > > Just to make the behaviour more consistent between NFS and other
> > > "local" file systems as It was done by
> > > commit https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=0953e620de0538cbd081f1b45126f6098112a598
> > 
> > Seems like we got others in line with XFS behavior.
> 
> But, not having tested the behavior, it looks like fs/open.c has a
> simlar !S_ISDIR() check.  Where's that behavior implemented?
> 
> > I'd prefer to have NFS follow this as well.
> 
> Huh.  Sachin, do you remember if there was any other motivation behind
> that patch?

Never mind, I see, the complaint is about the case where the id's don't
change, not about the directory case.  So Sachin's
0953e620de0538cbd081f1b45126f6098112a598 doesn't actually have anything
to do with this.

I'm fine with removing the id comparisons and changing the nfsd behavior
to match local filesystems.

--b.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: nfs vs xfstests 193

[J. Bruce Fields]

On Fri, Dec 06, 2013 at 03:44:04PM -0500, bfields wrote:
> On Fri, Dec 06, 2013 at 10:08:58AM -0800, Christoph Hellwig wrote:
> > On Fri, Dec 06, 2013 at 05:20:34PM +0400, Stanislav Kholmanskikh wrote:
> > > Just to make the behaviour more consistent between NFS and other
> > > "local" file systems as It was done by
> > > commit https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=0953e620de0538cbd081f1b45126f6098112a598
> > 
> > Seems like we got others in line with XFS behavior.
> 
> But, not having tested the behavior, it looks like fs/open.c has a
> simlar !S_ISDIR() check.  Where's that behavior implemented?
> 
> > I'd prefer to have NFS follow this as well.
> 
> Huh.  Sachin, do you remember if there was any other motivation behind
> that patch?

Never mind, I see, the complaint is about the case where the id's don't
change, not about the directory case.  So Sachin's
0953e620de0538cbd081f1b45126f6098112a598 doesn't actually have anything
to do with this.

I'm fine with removing the id comparisons and changing the nfsd behavior
to match local filesystems.

--b.

Re: nfs vs xfstests 193

[Stanislav Kholmanskikh]

On 12/07/2013 12:47 AM, J. Bruce Fields wrote:
> On Fri, Dec 06, 2013 at 03:44:04PM -0500, bfields wrote:
>> On Fri, Dec 06, 2013 at 10:08:58AM -0800, Christoph Hellwig wrote:
>>> On Fri, Dec 06, 2013 at 05:20:34PM +0400, Stanislav Kholmanskikh wrote:
>>>> Just to make the behaviour more consistent between NFS and other
>>>> "local" file systems as It was done by
>>>> commit https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=0953e620de0538cbd081f1b45126f6098112a598
>>>
>>> Seems like we got others in line with XFS behavior.
>>
>> But, not having tested the behavior, it looks like fs/open.c has a
>> simlar !S_ISDIR() check.  Where's that behavior implemented?
>>
>>> I'd prefer to have NFS follow this as well.
>>
>> Huh.  Sachin, do you remember if there was any other motivation behind
>> that patch?
>
> Never mind, I see, the complaint is about the case where the id's don't
> change, not about the directory case.  So Sachin's
> 0953e620de0538cbd081f1b45126f6098112a598 doesn't actually have anything
> to do with this.
>
> I'm fine with removing the id comparisons and changing the nfsd behavior
> to match local filesystems.

Great.

I will try to produce a patch for this.

>
> --b.
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: nfs vs xfstests 193

[Stanislav Kholmanskikh]

On 12/07/2013 12:47 AM, J. Bruce Fields wrote:
> On Fri, Dec 06, 2013 at 03:44:04PM -0500, bfields wrote:
>> On Fri, Dec 06, 2013 at 10:08:58AM -0800, Christoph Hellwig wrote:
>>> On Fri, Dec 06, 2013 at 05:20:34PM +0400, Stanislav Kholmanskikh wrote:
>>>> Just to make the behaviour more consistent between NFS and other
>>>> "local" file systems as It was done by
>>>> commit https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/commit/?id=0953e620de0538cbd081f1b45126f6098112a598
>>>
>>> Seems like we got others in line with XFS behavior.
>>
>> But, not having tested the behavior, it looks like fs/open.c has a
>> simlar !S_ISDIR() check.  Where's that behavior implemented?
>>
>>> I'd prefer to have NFS follow this as well.
>>
>> Huh.  Sachin, do you remember if there was any other motivation behind
>> that patch?
>
> Never mind, I see, the complaint is about the case where the id's don't
> change, not about the directory case.  So Sachin's
> 0953e620de0538cbd081f1b45126f6098112a598 doesn't actually have anything
> to do with this.
>
> I'm fine with removing the id comparisons and changing the nfsd behavior
> to match local filesystems.

Great.

I will try to produce a patch for this.

>
> --b.
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
>

[PATCH] nfsd: revoking of suid/sgid bits after chown() in a consistent way

[Stanislav Kholmanskikh]

There is an inconsistency in the handling of SUID/SGID file
bits after chown() between NFS and other local file systems.

Local file systems (for example, ext3, ext4, xfs, btrfs) revoke
SUID/SGID bits after chown() on a regular file even if
the owner/group of the file has not been changed:

~# touch file; chmod ug+s file; chmod u+x file
~# ls -l file
-rwsr-Sr-- 1 root root 0 Dec  6 04:49 file
~# chown root file; ls -l file
-rwxr-Sr-- 1 root root 0 Dec  6 04:49 file

but NFS doesn't do that:

~# touch file; chmod ug+s file; chmod u+x file
~# ls -l file
-rwsr-Sr-- 1 root root 0 Dec  6 04:49 file
~# chown root file; ls -l file
-rwsr-Sr-- 1 root root 0 Dec  6 04:49 file

NFS does that only if the owner/group has been changed:

~# touch file; chmod ug+s file; chmod u+x file
~# ls -l file
-rwsr-Sr-- 1 root root 0 Dec  6 05:02 file
~# chown bin file; ls -l file
-rwxr-Sr-- 1 bin root 0 Dec  6 05:02 file

See: http://pubs.opengroup.org/onlinepubs/9699919799/functions/chown.html

 "If the specified file is a regular file, one or more of
 the S_IXUSR, S_IXGRP, or S_IXOTH bits of the file mode are set,
 and the process has appropriate privileges, it is
 implementation-defined whether the set-user-ID and set-group-ID
 bits are altered."

So both variants are acceptable by POSIX.

This patch makes NFS to behave like local file systems.

Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
---
 fs/nfsd/vfs.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 72cb28e..8226991 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -348,8 +348,7 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
 
 	/* Revoke setuid/setgid on chown */
 	if (!S_ISDIR(inode->i_mode) &&
-	    (((iap->ia_valid & ATTR_UID) && !uid_eq(iap->ia_uid, inode->i_uid)) ||
-	     ((iap->ia_valid & ATTR_GID) && !gid_eq(iap->ia_gid, inode->i_gid)))) {
+	    ((iap->ia_valid & ATTR_UID) || (iap->ia_valid & ATTR_GID))) {
 		iap->ia_valid |= ATTR_KILL_PRIV;
 		if (iap->ia_valid & ATTR_MODE) {
 			/* we're setting mode too, just clear the s*id bits */
-- 
1.7.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH] nfsd: revoking of suid/sgid bits after chown() in a consistent way

[Stanislav Kholmanskikh]

There is an inconsistency in the handling of SUID/SGID file
bits after chown() between NFS and other local file systems.

Local file systems (for example, ext3, ext4, xfs, btrfs) revoke
SUID/SGID bits after chown() on a regular file even if
the owner/group of the file has not been changed:

~# touch file; chmod ug+s file; chmod u+x file
~# ls -l file
-rwsr-Sr-- 1 root root 0 Dec  6 04:49 file
~# chown root file; ls -l file
-rwxr-Sr-- 1 root root 0 Dec  6 04:49 file

but NFS doesn't do that:

~# touch file; chmod ug+s file; chmod u+x file
~# ls -l file
-rwsr-Sr-- 1 root root 0 Dec  6 04:49 file
~# chown root file; ls -l file
-rwsr-Sr-- 1 root root 0 Dec  6 04:49 file

NFS does that only if the owner/group has been changed:

~# touch file; chmod ug+s file; chmod u+x file
~# ls -l file
-rwsr-Sr-- 1 root root 0 Dec  6 05:02 file
~# chown bin file; ls -l file
-rwxr-Sr-- 1 bin root 0 Dec  6 05:02 file

See: http://pubs.opengroup.org/onlinepubs/9699919799/functions/chown.html

 "If the specified file is a regular file, one or more of
 the S_IXUSR, S_IXGRP, or S_IXOTH bits of the file mode are set,
 and the process has appropriate privileges, it is
 implementation-defined whether the set-user-ID and set-group-ID
 bits are altered."

So both variants are acceptable by POSIX.

This patch makes NFS to behave like local file systems.

Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
---
 fs/nfsd/vfs.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 72cb28e..8226991 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -348,8 +348,7 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
 
 	/* Revoke setuid/setgid on chown */
 	if (!S_ISDIR(inode->i_mode) &&
-	    (((iap->ia_valid & ATTR_UID) && !uid_eq(iap->ia_uid, inode->i_uid)) ||
-	     ((iap->ia_valid & ATTR_GID) && !gid_eq(iap->ia_gid, inode->i_gid)))) {
+	    ((iap->ia_valid & ATTR_UID) || (iap->ia_valid & ATTR_GID))) {
 		iap->ia_valid |= ATTR_KILL_PRIV;
 		if (iap->ia_valid & ATTR_MODE) {
 			/* we're setting mode too, just clear the s*id bits */
-- 
1.7.1

Re: [PATCH] nfsd: revoking of suid/sgid bits after chown() in a consistent way

[Stanislav Kholmanskikh]

On 12/11/2013 02:16 PM, Stanislav Kholmanskikh wrote:
[cut off]
>
> This patch makes NFS to behave like local file systems.
>
[cut off]

This patch allows to run generic/193 without any issues with NFSv3.

With NFSv4 generic/193 fails (but with the other issues, which existed 
even before the patch).

generic/193 expects that suid/sgid bits are cleared after the file 
truncation:

touch file
chown fsgqa:fsgqa file
chmod u+s file
echo 'xyz' > file
ls -l file
su fsgqa -c 'echo > file'
ls -l file

With ext4 (for example), we have expectable results:
-rwSr--r-- 1 fsgqa fsgqa 4 Dec 11 05:21 file
-rw-r--r-- 1 fsgqa fsgqa 1 Dec 11 05:22 file

With NFSv3 as well:
-rwSr--r-- 1 fsgqa fsgqa 4 Dec 11 05:24 file
-rw-r--r-- 1 fsgqa fsgqa 1 Dec 11 05:25 file

But with NFSv4 the bits are not cleared:
-rwSr--r-- 1 fsgqa fsgqa 1 Dec 11 05:19 file
-rwSr--r-- 1 fsgqa fsgqa 1 Dec 11 05:21 file

'echo > file' issues:

open("file", O_WRONLY|O_CREAT|O_TRUNC, 0666)

Can it be because of design differences between NFSv3 and NFSv4?

Thank you.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] nfsd: revoking of suid/sgid bits after chown() in a consistent way

[Stanislav Kholmanskikh]

On 12/11/2013 02:16 PM, Stanislav Kholmanskikh wrote:
[cut off]
>
> This patch makes NFS to behave like local file systems.
>
[cut off]

This patch allows to run generic/193 without any issues with NFSv3.

With NFSv4 generic/193 fails (but with the other issues, which existed 
even before the patch).

generic/193 expects that suid/sgid bits are cleared after the file 
truncation:

touch file
chown fsgqa:fsgqa file
chmod u+s file
echo 'xyz' > file
ls -l file
su fsgqa -c 'echo > file'
ls -l file

With ext4 (for example), we have expectable results:
-rwSr--r-- 1 fsgqa fsgqa 4 Dec 11 05:21 file
-rw-r--r-- 1 fsgqa fsgqa 1 Dec 11 05:22 file

With NFSv3 as well:
-rwSr--r-- 1 fsgqa fsgqa 4 Dec 11 05:24 file
-rw-r--r-- 1 fsgqa fsgqa 1 Dec 11 05:25 file

But with NFSv4 the bits are not cleared:
-rwSr--r-- 1 fsgqa fsgqa 1 Dec 11 05:19 file
-rwSr--r-- 1 fsgqa fsgqa 1 Dec 11 05:21 file

'echo > file' issues:

open("file", O_WRONLY|O_CREAT|O_TRUNC, 0666)

Can it be because of design differences between NFSv3 and NFSv4?

Thank you.

Re: [PATCH] nfsd: revoking of suid/sgid bits after chown() in a consistent way

[J. Bruce Fields]

On Wed, Dec 11, 2013 at 03:00:22PM +0400, Stanislav Kholmanskikh wrote:
> 
> 
> On 12/11/2013 02:16 PM, Stanislav Kholmanskikh wrote:
> [cut off]
> >
> >This patch makes NFS to behave like local file systems.
> >
> [cut off]
> 
> This patch allows to run generic/193 without any issues with NFSv3.
> 
> With NFSv4 generic/193 fails (but with the other issues, which
> existed even before the patch).
> 
> generic/193 expects that suid/sgid bits are cleared after the file
> truncation:
> 
> touch file
> chown fsgqa:fsgqa file
> chmod u+s file
> echo 'xyz' > file
> ls -l file
> su fsgqa -c 'echo > file'
> ls -l file
> 
> With ext4 (for example), we have expectable results:
> -rwSr--r-- 1 fsgqa fsgqa 4 Dec 11 05:21 file
> -rw-r--r-- 1 fsgqa fsgqa 1 Dec 11 05:22 file
> 
> With NFSv3 as well:
> -rwSr--r-- 1 fsgqa fsgqa 4 Dec 11 05:24 file
> -rw-r--r-- 1 fsgqa fsgqa 1 Dec 11 05:25 file
> 
> But with NFSv4 the bits are not cleared:
> -rwSr--r-- 1 fsgqa fsgqa 1 Dec 11 05:19 file
> -rwSr--r-- 1 fsgqa fsgqa 1 Dec 11 05:21 file
> 
> 'echo > file' issues:
> 
> open("file", O_WRONLY|O_CREAT|O_TRUNC, 0666)
> 
> Can it be because of design differences between NFSv3 and NFSv4?

In the v3 case I'd expect the open O_TRUNC to result in a SETATTR rpc,
in the v4 case an OPEN rpc.  Both result in a call to nfsd_setattr,
though I only see nfsd_setattr turning off the SUID/SGID bits in the
chown case.  Are you sure it isn't the subsequent write that clears
those bits?

But looks to me like nfsd_vfs_write (used in both v3 & v4 cases) clears
suid & guid, so I still don't see it.

--b.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] nfsd: revoking of suid/sgid bits after chown() in a consistent way

[J. Bruce Fields]

On Wed, Dec 11, 2013 at 03:00:22PM +0400, Stanislav Kholmanskikh wrote:
> 
> 
> On 12/11/2013 02:16 PM, Stanislav Kholmanskikh wrote:
> [cut off]
> >
> >This patch makes NFS to behave like local file systems.
> >
> [cut off]
> 
> This patch allows to run generic/193 without any issues with NFSv3.
> 
> With NFSv4 generic/193 fails (but with the other issues, which
> existed even before the patch).
> 
> generic/193 expects that suid/sgid bits are cleared after the file
> truncation:
> 
> touch file
> chown fsgqa:fsgqa file
> chmod u+s file
> echo 'xyz' > file
> ls -l file
> su fsgqa -c 'echo > file'
> ls -l file
> 
> With ext4 (for example), we have expectable results:
> -rwSr--r-- 1 fsgqa fsgqa 4 Dec 11 05:21 file
> -rw-r--r-- 1 fsgqa fsgqa 1 Dec 11 05:22 file
> 
> With NFSv3 as well:
> -rwSr--r-- 1 fsgqa fsgqa 4 Dec 11 05:24 file
> -rw-r--r-- 1 fsgqa fsgqa 1 Dec 11 05:25 file
> 
> But with NFSv4 the bits are not cleared:
> -rwSr--r-- 1 fsgqa fsgqa 1 Dec 11 05:19 file
> -rwSr--r-- 1 fsgqa fsgqa 1 Dec 11 05:21 file
> 
> 'echo > file' issues:
> 
> open("file", O_WRONLY|O_CREAT|O_TRUNC, 0666)
> 
> Can it be because of design differences between NFSv3 and NFSv4?

In the v3 case I'd expect the open O_TRUNC to result in a SETATTR rpc,
in the v4 case an OPEN rpc.  Both result in a call to nfsd_setattr,
though I only see nfsd_setattr turning off the SUID/SGID bits in the
chown case.  Are you sure it isn't the subsequent write that clears
those bits?

But looks to me like nfsd_vfs_write (used in both v3 & v4 cases) clears
suid & guid, so I still don't see it.

--b.

Re: [PATCH] nfsd: revoking of suid/sgid bits after chown() in a consistent way

[Christoph Hellwig]

On Wed, Dec 11, 2013 at 10:38:59PM -0500, J. Bruce Fields wrote:
> In the v3 case I'd expect the open O_TRUNC to result in a SETATTR rpc,
> in the v4 case an OPEN rpc.  Both result in a call to nfsd_setattr,
> though I only see nfsd_setattr turning off the SUID/SGID bits in the
> chown case.  Are you sure it isn't the subsequent write that clears
> those bits?

We've traditionally cleared the suid bits for O_TRUNC for local
filesystem, although this is more a convention than a real security
need.  It would still be good if NFSv4 would follow the general
semantics.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] nfsd: revoking of suid/sgid bits after chown() in a consistent way

[Christoph Hellwig]

On Wed, Dec 11, 2013 at 10:38:59PM -0500, J. Bruce Fields wrote:
> In the v3 case I'd expect the open O_TRUNC to result in a SETATTR rpc,
> in the v4 case an OPEN rpc.  Both result in a call to nfsd_setattr,
> though I only see nfsd_setattr turning off the SUID/SGID bits in the
> chown case.  Are you sure it isn't the subsequent write that clears
> those bits?

We've traditionally cleared the suid bits for O_TRUNC for local
filesystem, although this is more a convention than a real security
need.  It would still be good if NFSv4 would follow the general
semantics.

Re: [PATCH] nfsd: revoking of suid/sgid bits after chown() in a consistent way

[Stanislav Kholmanskikh]

On 12/12/2013 07:38 AM, J. Bruce Fields wrote:
> On Wed, Dec 11, 2013 at 03:00:22PM +0400, Stanislav Kholmanskikh wrote:
>>
>>
>> On 12/11/2013 02:16 PM, Stanislav Kholmanskikh wrote:
>> [cut off]
>>>
>>> This patch makes NFS to behave like local file systems.
>>>
>> [cut off]
>>
>> This patch allows to run generic/193 without any issues with NFSv3.
>>
>> With NFSv4 generic/193 fails (but with the other issues, which
>> existed even before the patch).
>>
>> generic/193 expects that suid/sgid bits are cleared after the file
>> truncation:
>>
>> touch file
>> chown fsgqa:fsgqa file
>> chmod u+s file
>> echo 'xyz' > file
>> ls -l file
>> su fsgqa -c 'echo > file'
>> ls -l file
>>
>> With ext4 (for example), we have expectable results:
>> -rwSr--r-- 1 fsgqa fsgqa 4 Dec 11 05:21 file
>> -rw-r--r-- 1 fsgqa fsgqa 1 Dec 11 05:22 file
>>
>> With NFSv3 as well:
>> -rwSr--r-- 1 fsgqa fsgqa 4 Dec 11 05:24 file
>> -rw-r--r-- 1 fsgqa fsgqa 1 Dec 11 05:25 file
>>
>> But with NFSv4 the bits are not cleared:
>> -rwSr--r-- 1 fsgqa fsgqa 1 Dec 11 05:19 file
>> -rwSr--r-- 1 fsgqa fsgqa 1 Dec 11 05:21 file
>>
>> 'echo > file' issues:
>>
>> open("file", O_WRONLY|O_CREAT|O_TRUNC, 0666)
>>
>> Can it be because of design differences between NFSv3 and NFSv4?
>
> In the v3 case I'd expect the open O_TRUNC to result in a SETATTR rpc,
> in the v4 case an OPEN rpc.  Both result in a call to nfsd_setattr,
> though I only see nfsd_setattr turning off the SUID/SGID bits in the
> chown case.  Are you sure it isn't the subsequent write that clears
> those bits?

Actually, in the above test script I occasionally swapped positions of 
"echo 'xyz' > file" and "chmod u+s file". Of course, chmod should be 
after the writing. Sorry.

But here we are:

rm -f file; touch file
chown fsgqa:fsgqa file
echo 'xyz' > file
chmod u+s file
ls -l file
su fsgqa -c 'echo -n > file'  # open(O_TRUNC), close()
ls -l file
su fsgqa -c 'echo > file'     # open(O_TRUNC), write("\n"), close()
ls -l file

With NFSv3 suid is cleared after write:
-rwSr--r-- 1 fsgqa fsgqa 4 Dec 12 05:24 file
-rwSr--r-- 1 fsgqa fsgqa 0 Dec 12 05:24 file
-rw-r--r-- 1 fsgqa fsgqa 1 Dec 12 05:25 file

With NFSv4 suid is not cleared after write("\n"):
-rwSr--r-- 1 fsgqa fsgqa 4 Dec 12 05:26 file
-rwSr--r-- 1 fsgqa fsgqa 0 Dec 12 05:26 file
-rwSr--r-- 1 fsgqa fsgqa 1 Dec 12 05:27 file

but if we issue "su fsgqa -c 'echo -n b > file'", we will clear it:
-rw-r--r-- 1 fsgqa fsgqa 1 Dec 12 05:28 file

So if "file" is a file on NFSv4:
-rwSr--r-- 1 fsgqa fsgqa 4 Dec 12 05:26 file
and we do:

fd = open(file, O_WRONLY);

then write(fd, "\n", 1) will not clear suid bit, but write(fd, "b", 1) 
will do.

With ext4 suid is cleared after open(O_TRUNC):
-rwSr--r-- 1 fsgqa fsgqa 4 Dec 12 05:29 file
-rw-r--r-- 1 fsgqa fsgqa 0 Dec 12 05:29 file
-rw-r--r-- 1 fsgqa fsgqa 1 Dec 12 05:30 file

Execution of (via 'su fsgqa -c ...'):
fd = open("file", O_WRONLY);
close(fd);
if "file" is on ext4 file system and has suid bit on will not clear suid 
bit.

Execution of (via 'su fsgqa -c ...'):
fd = open("file", O_WRONLY);
write(fd, "a", 1);
close(fd);
if "file" is on ext4 file system and has suid bit on will clear suid bit.

To conclude:
1. With NFS suid is not cleared after open(O_TRUNC)
This may be solved by addition of ATTR_SIZE handling in nfsd_setattr 
(i.e nfsd_sanitize_attrs). Right?

2. NFSv4 treats "\n" on write() specially.
No ideas by the moment.

>
> But looks to me like nfsd_vfs_write (used in both v3 & v4 cases) clears
> suid & guid, so I still don't see it.
>
> --b.
>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] nfsd: revoking of suid/sgid bits after chown() in a consistent way

[Stanislav Kholmanskikh]

On 12/12/2013 07:38 AM, J. Bruce Fields wrote:
> On Wed, Dec 11, 2013 at 03:00:22PM +0400, Stanislav Kholmanskikh wrote:
>>
>>
>> On 12/11/2013 02:16 PM, Stanislav Kholmanskikh wrote:
>> [cut off]
>>>
>>> This patch makes NFS to behave like local file systems.
>>>
>> [cut off]
>>
>> This patch allows to run generic/193 without any issues with NFSv3.
>>
>> With NFSv4 generic/193 fails (but with the other issues, which
>> existed even before the patch).
>>
>> generic/193 expects that suid/sgid bits are cleared after the file
>> truncation:
>>
>> touch file
>> chown fsgqa:fsgqa file
>> chmod u+s file
>> echo 'xyz' > file
>> ls -l file
>> su fsgqa -c 'echo > file'
>> ls -l file
>>
>> With ext4 (for example), we have expectable results:
>> -rwSr--r-- 1 fsgqa fsgqa 4 Dec 11 05:21 file
>> -rw-r--r-- 1 fsgqa fsgqa 1 Dec 11 05:22 file
>>
>> With NFSv3 as well:
>> -rwSr--r-- 1 fsgqa fsgqa 4 Dec 11 05:24 file
>> -rw-r--r-- 1 fsgqa fsgqa 1 Dec 11 05:25 file
>>
>> But with NFSv4 the bits are not cleared:
>> -rwSr--r-- 1 fsgqa fsgqa 1 Dec 11 05:19 file
>> -rwSr--r-- 1 fsgqa fsgqa 1 Dec 11 05:21 file
>>
>> 'echo > file' issues:
>>
>> open("file", O_WRONLY|O_CREAT|O_TRUNC, 0666)
>>
>> Can it be because of design differences between NFSv3 and NFSv4?
>
> In the v3 case I'd expect the open O_TRUNC to result in a SETATTR rpc,
> in the v4 case an OPEN rpc.  Both result in a call to nfsd_setattr,
> though I only see nfsd_setattr turning off the SUID/SGID bits in the
> chown case.  Are you sure it isn't the subsequent write that clears
> those bits?

Actually, in the above test script I occasionally swapped positions of 
"echo 'xyz' > file" and "chmod u+s file". Of course, chmod should be 
after the writing. Sorry.

But here we are:

rm -f file; touch file
chown fsgqa:fsgqa file
echo 'xyz' > file
chmod u+s file
ls -l file
su fsgqa -c 'echo -n > file'  # open(O_TRUNC), close()
ls -l file
su fsgqa -c 'echo > file'     # open(O_TRUNC), write("\n"), close()
ls -l file

With NFSv3 suid is cleared after write:
-rwSr--r-- 1 fsgqa fsgqa 4 Dec 12 05:24 file
-rwSr--r-- 1 fsgqa fsgqa 0 Dec 12 05:24 file
-rw-r--r-- 1 fsgqa fsgqa 1 Dec 12 05:25 file

With NFSv4 suid is not cleared after write("\n"):
-rwSr--r-- 1 fsgqa fsgqa 4 Dec 12 05:26 file
-rwSr--r-- 1 fsgqa fsgqa 0 Dec 12 05:26 file
-rwSr--r-- 1 fsgqa fsgqa 1 Dec 12 05:27 file

but if we issue "su fsgqa -c 'echo -n b > file'", we will clear it:
-rw-r--r-- 1 fsgqa fsgqa 1 Dec 12 05:28 file

So if "file" is a file on NFSv4:
-rwSr--r-- 1 fsgqa fsgqa 4 Dec 12 05:26 file
and we do:

fd = open(file, O_WRONLY);

then write(fd, "\n", 1) will not clear suid bit, but write(fd, "b", 1) 
will do.

With ext4 suid is cleared after open(O_TRUNC):
-rwSr--r-- 1 fsgqa fsgqa 4 Dec 12 05:29 file
-rw-r--r-- 1 fsgqa fsgqa 0 Dec 12 05:29 file
-rw-r--r-- 1 fsgqa fsgqa 1 Dec 12 05:30 file

Execution of (via 'su fsgqa -c ...'):
fd = open("file", O_WRONLY);
close(fd);
if "file" is on ext4 file system and has suid bit on will not clear suid 
bit.

Execution of (via 'su fsgqa -c ...'):
fd = open("file", O_WRONLY);
write(fd, "a", 1);
close(fd);
if "file" is on ext4 file system and has suid bit on will clear suid bit.

To conclude:
1. With NFS suid is not cleared after open(O_TRUNC)
This may be solved by addition of ATTR_SIZE handling in nfsd_setattr 
(i.e nfsd_sanitize_attrs). Right?

2. NFSv4 treats "\n" on write() specially.
No ideas by the moment.

>
> But looks to me like nfsd_vfs_write (used in both v3 & v4 cases) clears
> suid & guid, so I still don't see it.
>
> --b.
>

Re: [PATCH] nfsd: revoking of suid/sgid bits after chown() in a consistent way

[J. Bruce Fields]

Thanks, applying for 3.14.--b.

On Wed, Dec 11, 2013 at 02:16:36PM +0400, Stanislav Kholmanskikh wrote:
> There is an inconsistency in the handling of SUID/SGID file
> bits after chown() between NFS and other local file systems.
> 
> Local file systems (for example, ext3, ext4, xfs, btrfs) revoke
> SUID/SGID bits after chown() on a regular file even if
> the owner/group of the file has not been changed:
> 
> ~# touch file; chmod ug+s file; chmod u+x file
> ~# ls -l file
> -rwsr-Sr-- 1 root root 0 Dec  6 04:49 file
> ~# chown root file; ls -l file
> -rwxr-Sr-- 1 root root 0 Dec  6 04:49 file
> 
> but NFS doesn't do that:
> 
> ~# touch file; chmod ug+s file; chmod u+x file
> ~# ls -l file
> -rwsr-Sr-- 1 root root 0 Dec  6 04:49 file
> ~# chown root file; ls -l file
> -rwsr-Sr-- 1 root root 0 Dec  6 04:49 file
> 
> NFS does that only if the owner/group has been changed:
> 
> ~# touch file; chmod ug+s file; chmod u+x file
> ~# ls -l file
> -rwsr-Sr-- 1 root root 0 Dec  6 05:02 file
> ~# chown bin file; ls -l file
> -rwxr-Sr-- 1 bin root 0 Dec  6 05:02 file
> 
> See: http://pubs.opengroup.org/onlinepubs/9699919799/functions/chown.html
> 
>  "If the specified file is a regular file, one or more of
>  the S_IXUSR, S_IXGRP, or S_IXOTH bits of the file mode are set,
>  and the process has appropriate privileges, it is
>  implementation-defined whether the set-user-ID and set-group-ID
>  bits are altered."
> 
> So both variants are acceptable by POSIX.
> 
> This patch makes NFS to behave like local file systems.
> 
> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
> ---
>  fs/nfsd/vfs.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 72cb28e..8226991 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -348,8 +348,7 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
>  
>  	/* Revoke setuid/setgid on chown */
>  	if (!S_ISDIR(inode->i_mode) &&
> -	    (((iap->ia_valid & ATTR_UID) && !uid_eq(iap->ia_uid, inode->i_uid)) ||
> -	     ((iap->ia_valid & ATTR_GID) && !gid_eq(iap->ia_gid, inode->i_gid)))) {
> +	    ((iap->ia_valid & ATTR_UID) || (iap->ia_valid & ATTR_GID))) {
>  		iap->ia_valid |= ATTR_KILL_PRIV;
>  		if (iap->ia_valid & ATTR_MODE) {
>  			/* we're setting mode too, just clear the s*id bits */
> -- 
> 1.7.1
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] nfsd: revoking of suid/sgid bits after chown() in a consistent way

[J. Bruce Fields]

Thanks, applying for 3.14.--b.

On Wed, Dec 11, 2013 at 02:16:36PM +0400, Stanislav Kholmanskikh wrote:
> There is an inconsistency in the handling of SUID/SGID file
> bits after chown() between NFS and other local file systems.
> 
> Local file systems (for example, ext3, ext4, xfs, btrfs) revoke
> SUID/SGID bits after chown() on a regular file even if
> the owner/group of the file has not been changed:
> 
> ~# touch file; chmod ug+s file; chmod u+x file
> ~# ls -l file
> -rwsr-Sr-- 1 root root 0 Dec  6 04:49 file
> ~# chown root file; ls -l file
> -rwxr-Sr-- 1 root root 0 Dec  6 04:49 file
> 
> but NFS doesn't do that:
> 
> ~# touch file; chmod ug+s file; chmod u+x file
> ~# ls -l file
> -rwsr-Sr-- 1 root root 0 Dec  6 04:49 file
> ~# chown root file; ls -l file
> -rwsr-Sr-- 1 root root 0 Dec  6 04:49 file
> 
> NFS does that only if the owner/group has been changed:
> 
> ~# touch file; chmod ug+s file; chmod u+x file
> ~# ls -l file
> -rwsr-Sr-- 1 root root 0 Dec  6 05:02 file
> ~# chown bin file; ls -l file
> -rwxr-Sr-- 1 bin root 0 Dec  6 05:02 file
> 
> See: http://pubs.opengroup.org/onlinepubs/9699919799/functions/chown.html
> 
>  "If the specified file is a regular file, one or more of
>  the S_IXUSR, S_IXGRP, or S_IXOTH bits of the file mode are set,
>  and the process has appropriate privileges, it is
>  implementation-defined whether the set-user-ID and set-group-ID
>  bits are altered."
> 
> So both variants are acceptable by POSIX.
> 
> This patch makes NFS to behave like local file systems.
> 
> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
> ---
>  fs/nfsd/vfs.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 72cb28e..8226991 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -348,8 +348,7 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
>  
>  	/* Revoke setuid/setgid on chown */
>  	if (!S_ISDIR(inode->i_mode) &&
> -	    (((iap->ia_valid & ATTR_UID) && !uid_eq(iap->ia_uid, inode->i_uid)) ||
> -	     ((iap->ia_valid & ATTR_GID) && !gid_eq(iap->ia_gid, inode->i_gid)))) {
> +	    ((iap->ia_valid & ATTR_UID) || (iap->ia_valid & ATTR_GID))) {
>  		iap->ia_valid |= ATTR_KILL_PRIV;
>  		if (iap->ia_valid & ATTR_MODE) {
>  			/* we're setting mode too, just clear the s*id bits */
> -- 
> 1.7.1
>