* Vivek Goyal (vgo...@redhat.com) wrote: > On Tue, Jun 15, 2021 at 04:46:45PM +0100, Daniel P. Berrangé wrote: > > On Fri, Jun 11, 2021 at 11:42:22AM -0400, Vivek Goyal wrote: > > > On Fri, Jun 11, 2021 at 01:04:27PM +0100, Daniel P. Berrangé wrote: > > > > Different guest xattr prefixes have distinct access control rules > > > > applied > > > > by the guest. When remapping a guest xattr care must be taken that the > > > > remapping does not allow the a guest user to bypass guest kernel access > > > > control rules. > > > > > > > > For example if 'trusted.*' which requires CAP_SYS_ADMIN is remapped > > > > to 'user.virtiofs.trusted.*', an unprivileged guest user which can > > > > write to 'user.*' can bypass the CAP_SYS_ADMIN control. Thus the > > > > target of any remapping must be explicitly blocked from read/writes > > > > by the guest, to prevent access control bypass. > > > > > > > > The examples shown in the virtiofsd man page already do the right > > > > thing and ensure safety, but the security implications of getting > > > > this wrong were not made explicit. This could lead to host admins > > > > and apps unwittingly creating insecure configurations. > > > > > > > > Signed-off-by: Daniel P. Berrangé <berra...@redhat.com> > > > > --- > > > > docs/tools/virtiofsd.rst | 55 ++++++++++++++++++++++++++++++++++++---- > > > > 1 file changed, 50 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/docs/tools/virtiofsd.rst b/docs/tools/virtiofsd.rst > > > > index 00554c75bd..6370ab927c 100644 > > > > --- a/docs/tools/virtiofsd.rst > > > > +++ b/docs/tools/virtiofsd.rst > > > > @@ -127,8 +127,8 @@ Options > > > > timeout. ``always`` sets a long cache lifetime at the expense of > > > > coherency. > > > > The default is ``auto``. > > > > > > > > -xattr-mapping > > > > -------------- > > > > +Extended attribute (xattr) mapping > > > > +---------------------------------- > > > > > > > > By default the name of xattr's used by the client are passed through > > > > to the server > > > > file system. This can be a problem where either those xattr names are > > > > used > > > > @@ -136,6 +136,9 @@ by something on the server (e.g. selinux > > > > client/server confusion) or if the > > > > virtiofsd is running in a container with restricted privileges where > > > > it cannot > > > > access some attributes. > > > > > > > > +Mapping syntax > > > > +~~~~~~~~~~~~~~ > > > > + > > > > A mapping of xattr names can be made using -o xattrmap=mapping where > > > > the ``mapping`` > > > > string consists of a series of rules. > > > > > > > > @@ -232,8 +235,48 @@ Note: When the 'security.capability' xattr is > > > > remapped, the daemon has to do > > > > extra work to remove it during many operations, which the host kernel > > > > normally > > > > does itself. > > > > > > > > -xattr-mapping Examples > > > > ----------------------- > > > > +Security considerations > > > > +~~~~~~~~~~~~~~~~~~~~~~~ > > > > + > > > > +Operating systems typically partition the xattr namespace using > > > > +well defined name prefixes. Each partition may have different > > > > +access controls applied. For example, on Linux there are multiple > > > > +partitions > > > > + > > > > + * ``system.*`` - access varies depending on attribute & filesystem > > > > + * ``security.*`` - only processes with CAP_SYS_ADMIN > > > > + * ``trusted.*`` - only processes with CAP_SYS_ADMIN > > > > + * ``user.*`` - any process granted by file permissions / ownership > > > > + > > > > +While other OS such as FreeBSD have different name prefixes > > > > +and access control rules. > > > > + > > > > +When remapping attributes on the host, it is important to > > > > +ensure that the remapping does not allow a guest user to > > > > +evade the guest access control rules. > > > > + > > > > +Consider if ``trusted.*`` from the guest was remapped to > > > > +``user.virtiofs.trusted*`` in the host. An unprivileged > > > > +user in a Linux guest has the ability to write to xattrs > > > > +under ``user.*``. Thus the user can evade the access > > > > +control restriction on ``trusted.*`` by instead writing > > > > +to ``user.virtiofs.trusted.*``. > > > > + > > > > +As noted above, the partitions used and access controls > > > > +applied, will vary across guest OS, so it is not wise to > > > > +try to predict what the guest OS will use. > > > > + > > > > +The simplest way to avoid an insecure configuration is > > > > +to remap all xattrs at once, to a given fixed prefix. > > > > > > Remapping all xattrs seem to make sense. It probably will lead > > > to less confusion. Nested guests add another level of redirection. > > > > > > BTW, remapping xattr has limitation that it does not work on > > > symlinks. So "user.*" can't be set on symlink. And that means > > > selinux relabeling of symlinks fails with remapped xattrs. > > > > > > Not sure how to address this limitation. Host kernel imposes > > > this limit. (man xattr). > > > > Oh fun, I had not noticed this limitation before :-( > > > > Here are some ideas I had, none especially nice > > > > - Use 'trusted.*' namespace for remapping instead of 'user.' > > > > virtiofsd needs to have CAP_SYS_ADMIN though which is > > quite horrible if your goal is to confine its privileges > > in any meaningful way > > > > - Store a symlinks' xattr on the target of the symlink > > > > if the symlink has dev:inode 54:224, and points to > > a file 'foo', then on 'foo' create an xattr > > "user.virtiofs.link:54:224.<original xattrpath>" > > > > This gets quite horrendous when you have symlinks > > pointing to symlinks pointing to symlinks. Does > > not work too well if the 'st_dev' value is > > not stable across reboots either. > > > > - Don't use xattrs at all for remapping, instead use > > hidden files. > > > > eg for a file 'foo', if an xattr is set then create > > a file '.foo.xattr' in the same directory and store > > the xattrs in that file in some format. Need to hide > > this lookaside files from the guest of course. > > I kind of like this idea of creating a regular file, say > .user.virtiofs.foo.xattr. So any file prefixed by ".user.virtiofs" will > be hidden from guest. > > And probably same can be done for selinux labels for special files (device > nodes, pipes, sockets etc).
Except that splitting the attrs from the file makes it a nightmare to deal with renames and anything else that might be expected to happen atomically. Dave > Thanks > Vivek -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
