Re: NFSv3 may inappropriately return EPERM for fsetxattr

[Bruce Fields <bfields@fieldses.org>]

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.

--b.