Re: NFSv3 may inappropriately return EPERM for fsetxattr

[NeilBrown <neilb@suse.com>]

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

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.

Thanks,
NeilBrown

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