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.
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.
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.
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
> [email protected]
> http://oss.sgi.com/mailman/listinfo/xfs
>
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.
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 <[email protected]>
---
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
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.
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.
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.
>
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 <[email protected]>
> ---
> 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
>
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!
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.