On Tue, Jul 13, 2021 at 07:17:00AM -0700, Casey Schaufler wrote:

On 7/12/2021 10:47 AM, Vivek Goyal wrote:

quoted

On Mon, Jul 12, 2021 at 11:41:06AM -0400, J. Bruce Fields wrote:

quoted

On Mon, Jul 12, 2021 at 10:02:47AM -0400, Vivek Goyal wrote:

quoted

On Fri, Jul 09, 2021 at 04:10:16PM -0400, Bruce Fields wrote:

quoted

On Fri, Jul 9, 2021 at 1:59 PM Vivek Goyal [off-list ref] wrote:

quoted

nfs seems to have some issues.

I'm not sure what the expected behavior is for nfs.  All I have for
now is some generic troubleshooting ideas, sorry:

quoted

- I can set user.foo xattr on symlink and query it back using xattr name.

  getfattr -h -n user.foo foo-link.txt

  But when I try to dump all xattrs on this file, user.foo is being
  filtered out it looks like. Not sure why.

Logging into the server and seeing what's set there could help confirm
whether it's the client or server that's at fault.  (Or watching the
traffic in wireshark; there are GET/SET/LISTXATTR ops that should be
easy to spot.)

quoted

- I can't set "user.foo" xattr on a device node on nfs and I get
  "Permission denied". I am assuming nfs server is returning this.

Wireshark should tell you whether it's the server or client doing that.

The RFC is https://datatracker.ietf.org/doc/html/rfc8276, and I don't
see any explicit statement about what the server should do in the case
of symlinks or device nodes, but I do see "Any regular file or
directory may have a set of extended attributes", so that was clearly
the assumption.  Also, NFS4ERR_WRONG_TYPE is listed as a possible
error return for the xattr ops.  But on a quick skim I don't see any
explicit checks in the nfsd code, so I *think* it's just relying on
the vfs for any file type checks.

Hi Bruce,

Thanks for the response. I am just trying to do set a user.foo xattr on
a device node on nfs.

setfattr -n "user.foo" -v "bar" /mnt/nfs/test-dev

and I get -EACCESS.

I put some printk() statements and EACCESS is being returned from here.

nfs4_xattr_set_nfs4_user() {
        if (!nfs_access_get_cached(inode, current_cred(), &cache, true)) {
                if (!(cache.mask & NFS_ACCESS_XAWRITE)) {
                        return -EACCES;
                }
        }
}

Value of cache.mask=0xd at the time of error.

Looks like 0xd is what the server returns to access on a device node
with mode bits rw- for the caller.

Commit c11d7fd1b317 "nfsd: take xattr bits into account for permission
checks" added the ACCESS_X* bits for regular files and directories but
not others.

But you don't want to determine permission from the mode bits anyway,
you want it to depend on the owner,

Thinking more about this part. Current implementation of my patch is
effectively doing both the checks. It checks that you are owner or
have CAP_FOWNER in xattr_permission() and then goes on to call
inode_permission(). And that means file mode bits will also play a
role. If caller does not have write permission on the file, it will
be denied setxattr().

If I don't call inode_permission(), and just return 0 right away for
file owner (for symlinks and special files), then just being owner
is enough to write user.* xattr. And then even security modules will
not get a chance to block that operation.

That isn't going to fly. SELinux and Smack don't rely on ownership
as a criteria for access. Being the owner of a symlink conveys no
special privilege. The LSM must be consulted to determine if the
module's policy allows the access.

Getting back to this thread after a while. Sorry got busy in other
things.

I noticed that if we skip calling inode_permission() for special files,
then we will skip calling security_inode_permission() but we will
still call security hooks for setxattr/getxattr/removexattr etc.

security_inode_setxattr()
security_inode_getxattr()
security_inode_removexattr()

So LSMs will still get a chance whether to allow/disallow this operation
or not.

And skipping security_inode_permission() kind of makes sense that for
special files, I am not writing to device. So taking permission from
LSMs, will not make much sense.

Thanks
Vivek