Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sticky bit causes operation not permitted when exported over NFS #6889

Closed
telnoratti opened this issue Nov 20, 2017 · 11 comments
Closed

Sticky bit causes operation not permitted when exported over NFS #6889

telnoratti opened this issue Nov 20, 2017 · 11 comments

Comments

@telnoratti
Copy link

System information

Type Version/Name
Distribution Name RedHat Enterprise Linux
Distribution Version 6.9
Linux Kernel 2.6.32-696.6.3.el6.x86_64
Architecture x86_64
ZFS Version 0.7.2-1
SPL Version 0.7.1-1

Describe the problem you're observing

When exporting zfs over nfs, setting the sticky bit on a directory gives access denied incorrectly when trying to remove files. On the same system I tested exporting an xfs file system and it did not have the same issue.

Supposedly this changed sometime in the last few months, but we didn't notice this until today. We rebuilt the zpool a few weeks ago, if that's relevant.

Selinux is already permissive.

Describe how to reproduce the problem

Have an nfs server setup and running and change directory to a zfs filesystem, then run the following as root.

chown nonrootuser testexport/sticky
chmod +t testexport/sticky
echo "$PWD/testexport 127.0.0.1(no_root_squash,rw)" >> /etc/exports
exportfs -av
mkdir testmount
mount 127.0.0.1:$PWD/testexport testmount/
sudo -u nonrootuser touch testmount/sticky/foo
sudo -u nonrootuser rm testmount/sticky/foo
sudo -u nonrootuser touch testexport/sticky/bar
sudo -u nonrootuser rm testexport/sticky/bar

The rm testmount/sticky/foo fails with

rm: cannot remove `testmount/sticky/foo': Operation not permitted

but trying to rm directly at testexport succeeds.

If you run these same commands on a different underlying file system (I've only tested xfs) gives no error.

Include any warning/errors/backtraces from the system logs

I'm happy to include any logs you think are relevant, but there was nothing in selinux logs (selinux is permissive), messages, secure, or dmesg.

Thanks in advance.

@dcrdev
Copy link

dcrdev commented Nov 21, 2017

As of two releases ago, I've also been experiencing similar issues with sticky bits. For me it's not over nfs though - I'm getting operation not permitted errors with .NET core applications that implement the File.OpenRead method.

Only affects files with sticky bit and does not occur if the underlying filesystem is xfs - only started happening 2 releases ago, the files are unchanged and nothing else has changed on my end configuration-wise.

I'm on CentOS 7.4.

@jap
Copy link

jap commented Nov 22, 2017

I can confirm this happening with the jonathonf zfsonlinux packages on ubuntu-xenial with a stock kernel (4.4) as well, when trying to access an nfs share backed by zfs.

@dcrdev
Copy link

dcrdev commented Nov 22, 2017

Should have mentioned - I'm on the stock kernel also.

@jap
Copy link

jap commented Nov 22, 2017

Playing some more on my testing system shows the following (/srv/startmail is a mounted share, backed by zfs on the server):

jasper @ vlt01.be1.ams1:/srv/startmail/tmp/a $ ls -la
total 1
drwxrwxrwt 2 root root 2 Nov 22 15:46 .
drwxrwxrwt 4 root root 6 Nov 22 15:46 ..
jasper @ vlt01.be1.ams1:/srv/startmail/tmp/a $ touch a
jasper @ vlt01.be1.ams1:/srv/startmail/tmp/a $ rm a
jasper @ vlt01.be1.ams1:/srv/startmail/tmp/a $ sudo chown jasper .
jasper @ vlt01.be1.ams1:/srv/startmail/tmp/a $ touch a
jasper @ vlt01.be1.ams1:/srv/startmail/tmp/a $ rm a
rm: cannot remove 'a': Operation not permitted
jasper @ vlt01.be1.ams1:/srv/startmail/tmp/a $ sudo touch a
jasper @ vlt01.be1.ams1:/srv/startmail/tmp/a $ ls -la
total 2
drwxrwxrwt 2 jasper root   3 Nov 22 15:47 .
drwxrwxrwt 4 root   root   6 Nov 22 15:46 ..
-rw-rw-r-- 1 jasper jasper 0 Nov 22 15:53 a
jasper @ vlt01.be1.ams1:/srv/startmail/tmp/a $ rm a
rm: cannot remove 'a': Operation not permitted
[1] jasper @ vlt01.be1.ams1:/srv/startmail/tmp/a $ sudo chown root .
jasper @ vlt01.be1.ams1:/srv/startmail/tmp/a $ rm a
jasper @ vlt01.be1.ams1:/srv/startmail/tmp/a $

So if either the directory or the thing to be removed is owned by root, removal is allowed.
I'm starting to suspect module/zfs/zfs_dir.c:1143 which contains the following line:
if ((uid = crgetuid(cr)) == downer || uid == fowner || ...
It seems uid = 0 when called from an NFS context.

@dcrdev
Copy link

dcrdev commented Nov 23, 2017

Also affecting the setgid bit on directories.

@MarkGavalda
Copy link

Possibly the same issue as #6800 ?

behlendorf added a commit to behlendorf/zfs that referenced this issue Nov 29, 2017
When zfs_sticky_remove_access() was originally adapted for Linux
a typo was made which altered the desired behavior.  As described
in the block comment, permission should be granted when the entry
is a regular file and you have write access.  S_ISREG should have
been used here instead of S_ISDIR.

Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#6889
@behlendorf
Copy link
Contributor

I've open PR #6910 with a proposed fix for this issue. Independent verification that the patch resolves the issue as intended would be great.

@jap
Copy link

jap commented Nov 29, 2017

Thanks for this patch, I'll try it out tomorrow or the day after! (It's about midnight in my timezone)

@telnoratti
Copy link
Author

PR #6910 fixes the issue for me. Thanks!

behlendorf added a commit to tonyhutter/zfs that referenced this issue Dec 1, 2017
When zfs_sticky_remove_access() was originally adapted for Linux
a typo was made which altered the desired behavior.  As described
in the block comment, permission should be granted when the entry
is a regular file and you have write access.  S_ISREG should have
been used here instead of S_ISDIR.

Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#6889
@jap
Copy link

jap commented Dec 1, 2017

This fix appears to be wrong - it does fix this specific case, but there's a regression:

spaans@client:/srv/shared/foo$ ls -l
total 0
spaans@client:/srv/shared/foo$ ls -la
total 1
drwxrwxrwt 2 spaans root 2 Dec  1 11:02 .
drwxr-xr-x 4 root   root 4 Dec  1 11:03 ..
spaans@client:/srv/shared/foo$ mkdir a
spaans@client:/srv/shared/foo$ rmdir a
rmdir: failed to remove 'a': Operation not permitted

(client is a vm which has nfs-mounted a share on a server running zfs with this patch)

Note that running rmdir with this user on the server side works as expected - that is, it removes the dir.

behlendorf added a commit to behlendorf/zfs that referenced this issue Dec 1, 2017
When zfs_sticky_remove_access() was originally adapted for Linux
a typo was made which altered the intended behavior.  As described
in the block comment, the intended behavior is that permission
should be granted when the entry is a regular file and you have
write access.  That is, S_ISREG should have been used instead of
S_ISDIR.

Restricting permission to regular files made good sense for older
systems where setting the bit on executable files would instruct
the system to save the program's text segment on the swap device.

On modern systems this behavior has been replaced by the sticky
bit acting as a restricted deletion flag and the plain file
restriction has been relaxed.

Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#6889
@behlendorf
Copy link
Contributor

@jap thanks for verifying the updated patch. I've refreshed it again to relax the file type restriction which is no longer correct when it's used in the modern context as a restricted deletion flag.

behlendorf added a commit to tonyhutter/zfs that referenced this issue Dec 1, 2017
When zfs_sticky_remove_access() was originally adapted for Linux
a typo was made which altered the intended behavior.  As described
in the block comment, the intended behavior is that permission
should be granted when the entry is a regular file and you have
write access.  That is, S_ISREG should have been used instead of
S_ISDIR.

Restricting permission to regular files made good sense for older
systems where setting the bit on executable files would instruct
the system to save the program's text segment on the swap device.

On modern systems this behavior has been replaced by the sticky
bit acting as a restricted deletion flag and the plain file
restriction has been relaxed.

Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#6889
behlendorf added a commit to tonyhutter/zfs that referenced this issue Dec 1, 2017
When zfs_sticky_remove_access() was originally adapted for Linux
a typo was made which altered the intended behavior.  As described
in the block comment, the intended behavior is that permission
should be granted when the entry is a regular file and you have
write access.  That is, S_ISREG should have been used instead of
S_ISDIR.

Restricting permission to regular files made good sense for older
systems where setting the bit on executable files would instruct
the system to save the program's text segment on the swap device.

On modern systems this behavior has been replaced by the sticky
bit acting as a restricted deletion flag and the plain file
restriction has been relaxed.

Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#6889
behlendorf added a commit to tonyhutter/zfs that referenced this issue Dec 1, 2017
When zfs_sticky_remove_access() was originally adapted for Linux
a typo was made which altered the intended behavior.  As described
in the block comment, the intended behavior is that permission
should be granted when the entry is a regular file and you have
write access.  That is, S_ISREG should have been used instead of
S_ISDIR.

Restricting permission to regular files made good sense for older
systems where setting the bit on executable files would instruct
the system to save the program's text segment on the swap device.

On modern systems this behavior has been replaced by the sticky
bit acting as a restricted deletion flag and the plain file
restriction has been relaxed.

Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#6889
tonyhutter pushed a commit that referenced this issue Dec 5, 2017
When zfs_sticky_remove_access() was originally adapted for Linux
a typo was made which altered the intended behavior.  As described
in the block comment, the intended behavior is that permission
should be granted when the entry is a regular file and you have
write access.  That is, S_ISREG should have been used instead of
S_ISDIR.

Restricting permission to regular files made good sense for older
systems where setting the bit on executable files would instruct
the system to save the program's text segment on the swap device.

On modern systems this behavior has been replaced by the sticky
bit acting as a restricted deletion flag and the plain file
restriction has been relaxed.

Reviewed-by: George Melikov <[email protected]>
Reviewed-by: Giuseppe Di Natale <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes #6889
Closes #6910
tonyhutter pushed a commit to tonyhutter/zfs that referenced this issue Dec 5, 2017
When zfs_sticky_remove_access() was originally adapted for Linux
a typo was made which altered the intended behavior.  As described
in the block comment, the intended behavior is that permission
should be granted when the entry is a regular file and you have
write access.  That is, S_ISREG should have been used instead of
S_ISDIR.

Restricting permission to regular files made good sense for older
systems where setting the bit on executable files would instruct
the system to save the program's text segment on the swap device.

On modern systems this behavior has been replaced by the sticky
bit acting as a restricted deletion flag and the plain file
restriction has been relaxed.

Reviewed-by: George Melikov <[email protected]>
Reviewed-by: Giuseppe Di Natale <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#6889
Closes openzfs#6910
Nasf-Fan pushed a commit to Nasf-Fan/zfs that referenced this issue Jan 29, 2018
When zfs_sticky_remove_access() was originally adapted for Linux
a typo was made which altered the intended behavior.  As described
in the block comment, the intended behavior is that permission
should be granted when the entry is a regular file and you have
write access.  That is, S_ISREG should have been used instead of
S_ISDIR.

Restricting permission to regular files made good sense for older
systems where setting the bit on executable files would instruct
the system to save the program's text segment on the swap device.

On modern systems this behavior has been replaced by the sticky
bit acting as a restricted deletion flag and the plain file
restriction has been relaxed.

Reviewed-by: George Melikov <[email protected]>
Reviewed-by: Giuseppe Di Natale <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#6889
Closes openzfs#6910
Nasf-Fan pushed a commit to Nasf-Fan/zfs that referenced this issue Feb 13, 2018
When zfs_sticky_remove_access() was originally adapted for Linux
a typo was made which altered the intended behavior.  As described
in the block comment, the intended behavior is that permission
should be granted when the entry is a regular file and you have
write access.  That is, S_ISREG should have been used instead of
S_ISDIR.

Restricting permission to regular files made good sense for older
systems where setting the bit on executable files would instruct
the system to save the program's text segment on the swap device.

On modern systems this behavior has been replaced by the sticky
bit acting as a restricted deletion flag and the plain file
restriction has been relaxed.

Reviewed-by: George Melikov <[email protected]>
Reviewed-by: Giuseppe Di Natale <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#6889
Closes openzfs#6910
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants