linux-kernel - Re: [PATCH v2 1/1] xattr: Allow user.* xattr on symlink and special files

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 12 Jul 2021 17:22:01 -0400
From:   Vivek Goyal <vgoyal@...hat.com>
To:     "J. Bruce Fields" <bfields@...ldses.org>
Cc:     Bruce Fields <bfields@...hat.com>,
        Casey Schaufler <casey@...aufler-ca.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        viro@...iv.linux.org.uk, virtio-fs@...hat.com, dwalsh@...hat.com,
        dgilbert@...hat.com, casey.schaufler@...el.com,
        linux-security-module@...r.kernel.org, selinux@...r.kernel.org,
        tytso@....edu, miklos@...redi.hu, gscrivan@...hat.com,
        jack@...e.cz, Christoph Hellwig <hch@...radead.org>
Subject: Re: [PATCH v2 1/1] xattr: Allow user.* xattr on symlink and special
 files

On Mon, Jul 12, 2021 at 03:31:39PM -0400, J. Bruce Fields wrote:
> On Mon, Jul 12, 2021 at 01:47:59PM -0400, Vivek Goyal wrote:
> > On Mon, Jul 12, 2021 at 11:41:06AM -0400, J. Bruce Fields wrote:
> > > 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. IOW, if you are owner of
> > a symlink or special file, you can write as many user.* xattr as you
> > like and except quota does not look like anything else can block
> > it. I am wondering if this approach is ok?
> 
> Yeah, I'd expect security modules to get a say, and I wouldn't expect
> mode bits on device nodes to be useful for deciding whether it makes
> sense for xattrs to be readable or writeable.

Actually, calling inode_permission() for symlinks probably should be
fine.

Its the device node which is problematic. Because we started with the
assumption that mode bits there represent access writes for read/writing
to device (and not to the filesystem). 

> 
> But, I don't really know.

> 
> Do we have any other use cases besides this case of storing security
> labels in user xattrs?

Storing security label was one example. In case of virtiofs, there is
a good chance that we will end up remapping all the guest xattrs and
prefix these with "user.virtiofsd".

fuse-overlay is another use case. They are storing real uid/gid in 
user.* xattrs for files over NFS.

I think overlayfs can be another benefeciary in some form. Now there
is support for unpriviliged mouting of overlayfs from inside a user
namespace. And that uses xattrs "user.overlay" on upper files for
overlayfs specific metadata. Device nodes are not copied up. But
they might have an issue with symlinks. Miklos, will know more.

Thanks
Vivek