Without `default_permissions`, cached permissions are only checked on first access #15

Open
jpandre opened this Issue Jan 30, 2016 · 18 comments

Projects

None yet

5 participants

@jpandre
jpandre commented Jan 30, 2016

When the inodes are cached by fuse (non-zero attr_timeout), and the standard_permissions are used, the following sequence :

chmod 755 trydir
rm -rf trydir
mkdir trydir
echo file > trydir/file
ls -li trydir/file    
chmod 000 trydir
chmod 444 trydir/file
ls -li trydir/file

outputs :

293 -rw-rw-r-- 1 linux linux 5 2009-08-25 17:50 trydir/file
293 -r--r--r-- 1 linux linux 5 2009-08-25 17:50 trydir/file

The "chmod 000" should have led to an EACCES error for the subsequent commands.

This was reported in 2009 (http://article.gmane.org/gmane.comp.file-systems.fuse.devel/8099), and according to http://article.gmane.org/gmane.comp.file-systems.fuse.devel/15300 some work is still needed on it.

This means that for being Posix compliant, fuse file systems cannot either use the cache or have the permissions checked by the kernel.

@satgs
satgs commented Jan 31, 2016

Would this fix http://sourceforge.net/p/fuse/mailman/message/34801725/ too. Because the attr-timeout has nothing to do with the issue reported (the entry timeout has to be > 0). By standard permissions do you mean default permissions turned off ?

@jpandre
jpandre commented Jan 31, 2016

Hi,

Satya G S wrote:

Would this fix http://sourceforge.net/p/fuse/mailman/message/34801725/
too. Because the attr-timeout has nothing to do with the issue reported
(the entry timeout has to be > 0). By standard permissions do you mean
default permissions turned off ?

I do not fully understand what you mean (what has
nothing to do with what ?, what fix are you mentioning ?),
so I explain my point of view on the issue :

When the kernel needs to dig into a directory on
behalf of some user (typically in a readdir()), it
makes a lookup on the directory, and the result (either
successful or failed because of permissions) is saved
in some cached structure. Now if the cached structure
is invalid either because the permissions have changed
or the request is done by another user with different
rights, the kernel is expected to update its own data.

Your results show this is not yet the case.

attr-timeout (I mean the last argument of fuse_reply_attr())
does not need to be positive. If it is zero, the kernel
is not allowed to use cached data and it has to redo
the lookups on the full path to check what is allowed
to the requesting user.

On Linux, ntfs-3g usually uses default_permissions (yes,
that is what I meant) but it uses a zero attr-timeout
for the above reason, and repeating the posted test
sequence yields the expected result :

[linux@dimension linux]$ ./try.sh
136 -rw-r--r-- 1 linux linux 5 Jan 31 11:06 trydir/file
chmod: cannot access ‘trydir/file’: Permission denied
ls: cannot access trydir/file: Permission denied

On OpenIndiana (same fuse library), ntfs-3g can use
the fuse cache, and the cache is compatible with using
hard links at the high level interface.

Note : the proper behavior of permissions is checked by
the Posix compliance test :
http://sourceforge.net/p/ntfs-3g/pjd-fstest/ci/master/tree/

Regards

Jean-Pierre

@satgs
satgs commented Jan 31, 2016

ls -l seems to working fine (doesn't display children attributes) if default permissions is turned on. When default permissions is turned off and entry timeout is > 0 ls -l display the children attributes for the duration of entry timeout which is a bug.

Thanks,
Satya.

@jpandre
jpandre commented Jan 31, 2016

Hum, yes you are right. The issue occurs with default_permissions not enabled, and with permissions checked by the user-level file system. I have recompiled ntfs-3g with a timeout of 10 seconds (both attr and entry timeouts), and executed as root the following script :

rm -rf trydir
umask 077
mkdir trydir
umask 022
echo file > trydir/file
echo "ls -li trydir/file" > user.sh
chmod 755 user.sh
ls -ldi trydir trydir/file user.sh     
su linux -c ./user.sh

Note that this script never changes the permissions on the directory "trydir". The expected output is :

[root@dimension linux]# ./try2.sh
144 drwx------ 1 root root 144 Jan 31 21:26 trydir
146 -rw-r--r-- 1 root root   5 Jan 31 21:26 trydir/file
126 -rwxr-xr-x 1 root root  19 Jan 31 21:26 user.sh
trydir/file not found

The error on last line is caused by the user ("linux") not being allowed to list "trydir".
With the timeouts enabled, I get :

[root@dimension linux]# ./try2.sh
123 drwx------ 1 root root 144 Jan 31 21:47 trydir
127 -rw-r--r-- 1 root root   5 Jan 31 21:47 trydir/file
126 -rwxr-xr-x 1 root root  19 Jan 31 21:47 user.sh
127 r--  31 Jan 16  21:47         5  trydir/file

And trydir could be listed...
So this is not a bug for not refreshing the cache (the permissions on trydir are never changed), it is a bug for not checking the permissions on the cached directory when the current user is not the one for which the directory was cached in the first place. In the debug output, I can see that there is no lookup from directory tryfile after the inner script user.sh is opened, so the user-level file system has no opportunity to check whether a lookup by the non-root user would be valid.

Also note the user-level file system has no opportunity to invalidate an entry. There is no action triggering a change.

I have replayed the shell script shown in the original post, and I can confirm the bug is still present, but only when the option default_permissions is NOT used.

for Nikolaus : maybe you can change the title to "The directory entries cached by fuse are not checked against the current user permissions"

@jpandre jpandre closed this Jan 31, 2016
@jpandre
jpandre commented Jan 31, 2016

Sorry, I wanted to close the comment, ... not the issue.

@jpandre jpandre reopened this Jan 31, 2016
@satgs
satgs commented Feb 1, 2016

Jean,

Thank you for confirming the behaviour. This issue hurts every filesystem that has support for ACLs. Setting the entry timeout to zero will result in too many up calls to filesystem.

Isn't this a place to track library issues. This looks like a kernel bug to me.

Thanks,
Satya.

@jpandre
jpandre commented Feb 1, 2016

I agree, not using the cache is inefficient because it implies making more calls to the user-level file system. The same goes for not using default_permissions.

So I am not insisting on this bug to be fixed, I would rather call for default_permissions to support the same permission checks as ext2/3/4.

I also call for the cache to support hard links, and this is specific to the high-level library interface, there is no problem with the low-level one.

These are the two reasons why ntfs-3g gave up using the cache on Linux (except when using the low-level interface and not using the Posix ACLs).

@Nikratio Nikratio changed the title from Bug : The inodes entries cached by fuse are not refreshed upon returning from setattr(). to Cached directory entry permissions are only checked for first accessing user Feb 1, 2016
@Nikratio
Contributor
Nikratio commented Feb 1, 2016

So this is not a bug for not refreshing the cache (the permissions on trydir are never changed), it is a bug for not checking the permissions on the cached directory when the current user is not the one for which the directory was cached in the first place

Yes, when writing the kernel code there was probably an implicit assumption that allow_other" would not be used (or maybe it didn't exist at that point). With allow_other, this becomes a security bug.

Isn't this a place to track library issues. This looks like a kernel bug to me.

You are right. I asked Jean-Pierre to file it here so that there's at least a permanent record (give that this was first reported on the kernel ml in 2006).

@Nikratio Nikratio added a commit that referenced this issue Feb 1, 2016
@Nikratio Nikratio Document bug #15. 43138fb
@Nikratio Nikratio added a commit that referenced this issue Feb 1, 2016
@Nikratio Nikratio Document bug #15. 9775c70
@Nikratio
Contributor
Nikratio commented Feb 1, 2016

I've added this to the README, see commit 43138fb

@jpandre
jpandre commented Feb 2, 2016

Nikolaus Rath wrote:

I've added this to the README, see commit 43138fb
43138fb

+bug #15: the
+permission to access a cached directory entry is only checked for the
+first user that accesses it. As long as the directory entry is cached,
+accesses by other users are made with the permissions of the first
+user.

I think this is not correct : in my first example and
the situation met by Satya the initial access and the
second one are done by the same user (with permissions
changed), and yes, in my second example there is a user
change with permissions unchanged.

So my feeling is that there is no check at all.

My text change proposal :

+bug #15: the
+permission to access a cached directory entry rely on the check
+done for the current user at the moment it was cached.
+As long as the directory entry is cached, subsequent
+accesses by the same user or by other users are not checked.


Reply to this email directly or view it on GitHub
#15 (comment).

@Nikratio Nikratio added a commit that referenced this issue Feb 2, 2016
@Nikratio Nikratio Fix description of bug #15. 85f3ff4
@Nikratio Nikratio added a commit that referenced this issue Feb 2, 2016
@Nikratio Nikratio Fix description of bug #15. c41b1a5
@Nikratio
Contributor
Nikratio commented Feb 2, 2016

Thanks for re-checking. How do you like the version in commit 85f3ff4?

@jpandre
jpandre commented Feb 2, 2016

Nikolaus Rath wrote:

Thanks for re-checking. How do you like the version in commit c41b?

Looks good to me.

Jean-Pierre

@Nikratio Nikratio added a commit that referenced this issue Mar 1, 2016
@Nikratio Nikratio Improve description of issue #15. 86fe71c
@Nikratio Nikratio changed the title from Cached directory entry permissions are only checked for first accessing user to Without `default_permissions`, cached permissions are only checked on first access Mar 1, 2016
@Nikratio Nikratio added a commit that referenced this issue Mar 1, 2016
@Nikratio Nikratio Improve description of issue #15. b3b452e
@0day-ci 0day-ci pushed a commit to 0day-ci/linux that referenced this issue May 31, 2016
@ashishsangwan2 @fengguang ashishsangwan2 + fengguang fuse: callback for invalidating cached children dentries for a directory
This patch solves a long standing bug.
"([bug #15](libfuse/libfuse#15)): if the
`default_permissions` mount option is not used, the results of the
first permission check performed by the file system for a directory
entry will be re-used for subsequent accesses as long as the inode of
the accessed entry is present in the kernel cache - even if the
permissions have since changed, and even if the subsequent access is
made by a different user.
This bug needs to be fixed in the Linux kernel and has been known
since 2006 but unfortunately no fix has been applied yet. If you
depend on correct permission handling for FUSE file systems, the only
workaround is to use `default_permissions` (which does not currently
support ACLs), or to completely disable caching of directory entry
attributes. Alternatively, the severity of the bug can be somewhat
reduced by not using the `allow_other` mount option."

This patch introduce a callback which the user space implementation can use
to invalidate the cached entries of a parent directory, for example when
the execute permissions are revoked and force real lookup.

Signed-off-by: Ashish Sangwan <ashishsangwan2@gmail.com>
db8cff5
@rfjakob rfjakob referenced this issue in rfjakob/gocryptfs Aug 12, 2016
Closed

Need workaround for libfuse ticket #15 #33

@rfjakob
rfjakob commented Aug 16, 2016

gocryptfs developer here. I don't understand why this is declared a kernel bug (but on the other hand I can't click the gmane links because it has been shut down).

Isn't just that the user-space filesystem fails to check the permissions on READDIR? (This is the opportunity to check the permissions, there is no need for an extra LOOKUP)

@jpandre
jpandre commented Aug 16, 2016

@rfjakob : see my example try2.sh, there is no readdir at all (no wildcards). Creating "trydir/file" is just a create() in write-only directory "trydir" (which is allowed, though directory is unreadable). Later on, for executing "trydir/file", there is no lookup from "trydir", and the user space has no chance of checking whether "trydir" is readable. When "file" is opened, the user space does not know from which directory it is opened (a file may be referenced in several directories with different permissions).
The fuse kernel module has been said to accept the lookup because it does not know who is the user requesting the file... which can be translated to a bad design of the interface between the fuse kernel module and the kernel proper.

@rfjakob
rfjakob commented Aug 16, 2016 edited

Ok, I think I get it. When there are multiple hard links of one file in different directories, user-space cannot decide which directory to check. It's quite a corner-case, but still.

Note that the kernel knows about the user and this info is also passed to user-space in every request:
https://github.com/libfuse/libfuse/blob/d6d3e50a51fb97d9f9188ceb1bed01cfbc8133a0/include/fuse_kernel.h#L695

@jpandre
jpandre commented Aug 16, 2016

@rfjakob
In "the kernel knows about the user and this info is also passed to user-space in every request" you miss the point. We are in the situation where user-space does not receive any request at all for checking whether directory is readable. This has been said to be a consequence of the fuse kernel module not getting the user description in some particular situation.
AFAIK the only way for the user space to get around this is by setting a null duration for the attributes cached in the kernel. This forces a lookup to user-space.

@rfjakob
rfjakob commented Aug 16, 2016
@brauner brauner referenced this issue in lxc/lxcfs Aug 21, 2016
Open

Use default_permissions #128

@Nikratio Nikratio added the confirmed label Oct 26, 2016
@AdminOfLife

I'm trying to compile under Windows using сygwin, but eventually do make-j8 command error occurs

> ../include/fuse_kernel.h:759:41: error: expected expression before 'uint32_t'
>   #define FUSE_DEV_IOC_CLONE _IOR (229, 0, uint32_t).

Please help deal with this problem, which is already just trying to build library, but this error does not allow it to make

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment