On Tue, Jul 13, 2021 at 07:17:00AM -0700, Casey Schaufler wrote: > On 7/12/2021 10:47 AM, Vivek Goyal wrote: > > On Mon, Jul 12, 2021 at 11:41:06AM -0400, J. Bruce Fields wrote: > >> On Mon, Jul 12, 2021 at 10:02:47AM -0400, Vivek Goyal wrote: > >>> On Fri, Jul 09, 2021 at 04:10:16PM -0400, Bruce Fields wrote: > >>>> On Fri, Jul 9, 2021 at 1:59 PM Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > >>>>> 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: > >>>> > >>>>> - 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.) > >>>> > >>>>> - 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
