Re: NFSv3 may inappropriately return EPERM for fsetxattr

[NeilBrown <neilb@suse.com>]

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

On Sun, Aug 12 2018, Bruce Fields wrote:

> On Sun, Aug 12, 2018 at 08:28:00AM +1000, NeilBrown wrote:
>> On Fri, Aug 10 2018, Bruce Fields wrote:
>> 
>> > On Fri, Aug 10, 2018 at 01:00:27PM -0400, Bruce Fields wrote:
>> >> On Fri, Aug 10, 2018 at 11:29:33AM +1000, NeilBrown wrote:
>> >> > On Mon, Mar 21 2016, Nelson Elhage wrote:
>> >> > 
>> >> > > That's correct. The other detail that seems to be important is that
>> >> > > the user making the call must be different from the user owning the
>> >> > > file. We've also been using user remapping on the server, so that
>> >> > > non-xattr calls succeed in that configuration.
>> >> > >
>> >> > > The reproducer James added in the bugzilla is:
>> >> > >
>> >> > > (on machine with IP address 10.1.1.1)
>> >> > > sudo mkdir /nfs_test
>> >> > > sudo useradd -u 10000 test_user
>> >> > > sudo chown test_user /nfs_test
>> >> > > echo "/nfs_test 10.1.1.2(rw,all_squash,anonuid=10000)" | sudo tee -a
>> >> > > /etc/exports
>> >> > > sudo exportfs -a
>> >> > >
>> >> > > (on machine with IP address 10.1.1.2)
>> >> > > sudo mkdir /nfs_test
>> >> > > sudo mount -t nfs -o vers=3,noacl 10.1.1.1:/nfs_test /nfs_test
>> >> > > touch /nfs_test/foo
>> >> > > install -m 755 /nfs_test/foo /nfs_test/bar
>> >> > 
>> >> > Did anything ever happen about this?
>> >> > I have a customer with a similar problem (in 4.4) but I cannot see any
>> >> > evidence of fixes landing in mainline.
>> >> > 
>> >> > Problem happens with you have uid mapping on the server
>> >> > (e.g. anonuid=10000 as above) and a user with a different uid on the
>> >> > client attempts setacl on a file with that user.
>> >> > As anon is mapped to the owner of the file, setacl should be allowed.
>> >> > However set_posix_acl() calls inode_owner_or_capable() which checks if
>> >> > the client-side uid matches the visible inode->i_uid - they don't.
>> >> > 
>> >> > Testing i_uid on the client is always incorrect for permission checking
>> >> > with NFS - the client should always ask the server, either with ACCESS
>> >> > or, in this case, by simply attempting the operation.
>> >> > 
>> >> > Any suggestions how best to fix this?
>> >> > - We could move the responsibility for permission checking into
>> >> >   i_op->set_acl, but that would be a large change and might make it too
>> >> >   easy for other filesystems to get it wrong.
>> >> > - we could have some sort of flag asking set_posix_acl(), but that's
>> >> >   rather clumsy.... maybe if i_op->set_acl_check_perm use that without
>> >> >   testing ownership first??
>> >> > - we could copy
>> >> >     posic_acl_xattr_{get,set,list} into nfs together with functions
>> >> >     they call, modify set_posix_acl() to not test ownership,
>> >> >     and provide a local 'struct xattr_handler' structure for NFS.
>> >> > 
>> >> > I don't really like any of those suggestions.  Can someone else do any
>> >> > better?
>> >> 
>> >> Do we have important callers of inode_owner_or_capable() in the vfs (as
>> >> opposed to in individual filesystems), and do any of them pose a similar
>> >> problem for network filesystems?
>> >
>> > do_linkat()->may_linkat() looks kinda suspicious to me.  Or what about
>> > the O_NOATIME check in map_open()?  Just engaging in dumb grepping
>> > here....
>> >
>> > --b.
>> 
>> NOATIME, both in open and fcntl, is rejected on NFS.  This seems valid
>> as there is no way in the protocol to ask the server to no update the
>> atime.
>> 
>> Others I found we just short-cuts to avoid calling i_op->permission() if
>> the caller was an owner.  I don't *think* that would affect NFS much
>> ... though if an owner didn't have write permission, some things might
>> be incorrectly forbidden.  Maybe.
>
> OK, so not too important.  Still, it sounds like
> inode_owner_or_capable() is something people expect to work for any
> filesystem, so I wonder if there's a way to do that.  Or at least
> disable it.

We could add a new flag - MAY_OWN (or something) - to the flags
recognised by inode_permission() and i_op->permission().

If ->permission isn't set, inode_permission() uses
inode_owner_or_capable().
If it is, it gets to call that, or do whatever is appropriate.

Is this flag the same as NFS_MAY_OWNER_OVERRIDE or not....??

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]