mbox series

[v3,0/1] Relax restrictions on user.* xattr

Message ID 20210902152228.665959-1-vgoyal@redhat.com (mailing list archive)
Headers show
Series Relax restrictions on user.* xattr | expand

Message

Vivek Goyal Sept. 2, 2021, 3:22 p.m. UTC
Hi,

This is V3 of the patch. Previous versions were posted here.

v2:
https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
v1:
https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
+m/

Changes since v2
----------------
- Do not call inode_permission() for special files as file mode bits
  on these files represent permissions to read/write from/to device
  and not necessarily permission to read/write xattrs. In this case
  now user.* extended xattrs can be read/written on special files
  as long as caller is owner of file or has CAP_FOWNER.
 
- Fixed "man xattr". Will post a patch in same thread little later. (J.
  Bruce Fields)

- Fixed xfstest 062. Changed it to run only on older kernels where
  user extended xattrs are not allowed on symlinks/special files. Added
  a new replacement test 648 which does exactly what 062. Just that
  it is supposed to run on newer kernels where user extended xattrs
  are allowed on symlinks and special files. Will post patch in 
  same thread (Ted Ts'o).

Testing
-------
- Ran xfstest "./check -g auto" with and without patches and did not
  notice any new failures.

- Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
  filesystems and it works.
 
Description
===========

Right now we don't allow setting user.* xattrs on symlinks and special
files at all. Initially I thought that real reason behind this
restriction is quota limitations but from last conversation it seemed
that real reason is that permission bits on symlink and special files
are special and different from regular files and directories, hence
this restriction is in place. (I tested with xfs user quota enabled and
quota restrictions kicked in on symlink).

This version of patch allows reading/writing user.* xattr on symlink and
special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.

Who wants to set user.* xattr on symlink/special files
-----------------------------------------------------
I have primarily two users at this point of time.

- virtiofs daemon.

- fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
  fuse-overlay as well and he ran into similar issue. So fuse-overlay
  should benefit from this change as well.

Why virtiofsd wants to set user.* xattr on symlink/special files
----------------------------------------------------------------
In virtiofs, actual file server is virtiosd daemon running on host.
There we have a mode where xattrs can be remapped to something else.
For example security.selinux can be remapped to
user.virtiofsd.securit.selinux on the host.

This remapping is useful when SELinux is enabled in guest and virtiofs
as being used as rootfs. Guest and host SELinux policy might not match
and host policy might deny security.selinux xattr setting by guest
onto host. Or host might have SELinux disabled and in that case to
be able to set security.selinux xattr, virtiofsd will need to have
CAP_SYS_ADMIN (which we are trying to avoid). Being able to remap
guest security.selinux (or other xattrs) on host to something else
is also better from security point of view.

But when we try this, we noticed that SELinux relabeling in guest
is failing on some symlinks. When I debugged a little more, I
came to know that "user.*" xattrs are not allowed on symlinks
or special files.

So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
allow virtiofs to arbitrarily remap guests's xattrs to something
else on host and that solves this SELinux issue nicely and provides
two SELinux policies (host and guest) to co-exist nicely without
interfering with each other.

Thanks
Vivek

Vivek Goyal (1):
  xattr: Allow user.* xattr on symlink and special files

 fs/xattr.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

Comments

Casey Schaufler Sept. 2, 2021, 3:43 p.m. UTC | #1
On 9/2/2021 8:22 AM, Vivek Goyal wrote:
> Hi,
>
> This is V3 of the patch. Previous versions were posted here.
>
> v2:
> https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> v1:
> https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
> +m/
>
> Changes since v2
> ----------------
> - Do not call inode_permission() for special files as file mode bits
>   on these files represent permissions to read/write from/to device
>   and not necessarily permission to read/write xattrs. In this case
>   now user.* extended xattrs can be read/written on special files
>   as long as caller is owner of file or has CAP_FOWNER.
>  
> - Fixed "man xattr". Will post a patch in same thread little later. (J.
>   Bruce Fields)
>
> - Fixed xfstest 062. Changed it to run only on older kernels where
>   user extended xattrs are not allowed on symlinks/special files. Added
>   a new replacement test 648 which does exactly what 062. Just that
>   it is supposed to run on newer kernels where user extended xattrs
>   are allowed on symlinks and special files. Will post patch in 
>   same thread (Ted Ts'o).
>
> Testing
> -------
> - Ran xfstest "./check -g auto" with and without patches and did not
>   notice any new failures.
>
> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
>   filesystems and it works.
>  
> Description
> ===========
>
> Right now we don't allow setting user.* xattrs on symlinks and special
> files at all. Initially I thought that real reason behind this
> restriction is quota limitations but from last conversation it seemed
> that real reason is that permission bits on symlink and special files
> are special and different from regular files and directories, hence
> this restriction is in place. (I tested with xfs user quota enabled and
> quota restrictions kicked in on symlink).
>
> This version of patch allows reading/writing user.* xattr on symlink and
> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.

This part of your project makes perfect sense. There's no good
security reason that you shouldn't set user.* xattrs on symlinks
and/or special files.

However, your virtiofs use case is unreasonable.

> Who wants to set user.* xattr on symlink/special files
> -----------------------------------------------------
> I have primarily two users at this point of time.
>
> - virtiofs daemon.
>
> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
>   should benefit from this change as well.
>
> Why virtiofsd wants to set user.* xattr on symlink/special files
> ----------------------------------------------------------------
> In virtiofs, actual file server is virtiosd daemon running on host.
> There we have a mode where xattrs can be remapped to something else.
> For example security.selinux can be remapped to
> user.virtiofsd.securit.selinux on the host.

As I have stated before, this introduces a breach in security.
It allows an unprivileged process on the host to manipulate the
security state of the guest. This is horribly wrong. It is not
sufficient to claim that the breach requires misconfiguration
to exploit. Don't do this.

> This remapping is useful when SELinux is enabled in guest and virtiofs
> as being used as rootfs. Guest and host SELinux policy might not match
> and host policy might deny security.selinux xattr setting by guest
> onto host. Or host might have SELinux disabled and in that case to
> be able to set security.selinux xattr, virtiofsd will need to have
> CAP_SYS_ADMIN (which we are trying to avoid).

Adding this mapping to virtiofs provides the breach for any
LSM using xattrs.

>  Being able to remap
> guest security.selinux (or other xattrs) on host to something else
> is also better from security point of view.
>
> But when we try this, we noticed that SELinux relabeling in guest
> is failing on some symlinks. When I debugged a little more, I
> came to know that "user.*" xattrs are not allowed on symlinks
> or special files.
>
> So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
> allow virtiofs to arbitrarily remap guests's xattrs to something
> else on host and that solves this SELinux issue nicely and provides
> two SELinux policies (host and guest) to co-exist nicely without
> interfering with each other.

virtiofs could use security.* or system.* xattrs instead of user.*
xattrs. Don't use user.* xattrs.

>
> Thanks
> Vivek
>
> Vivek Goyal (1):
>   xattr: Allow user.* xattr on symlink and special files
>
>  fs/xattr.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
>
Vivek Goyal Sept. 2, 2021, 5:05 p.m. UTC | #2
On Thu, Sep 02, 2021 at 08:43:50AM -0700, Casey Schaufler wrote:
> On 9/2/2021 8:22 AM, Vivek Goyal wrote:
> > Hi,
> >
> > This is V3 of the patch. Previous versions were posted here.
> >
> > v2:
> > https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> > v1:
> > https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
> > +m/
> >
> > Changes since v2
> > ----------------
> > - Do not call inode_permission() for special files as file mode bits
> >   on these files represent permissions to read/write from/to device
> >   and not necessarily permission to read/write xattrs. In this case
> >   now user.* extended xattrs can be read/written on special files
> >   as long as caller is owner of file or has CAP_FOWNER.
> >  
> > - Fixed "man xattr". Will post a patch in same thread little later. (J.
> >   Bruce Fields)
> >
> > - Fixed xfstest 062. Changed it to run only on older kernels where
> >   user extended xattrs are not allowed on symlinks/special files. Added
> >   a new replacement test 648 which does exactly what 062. Just that
> >   it is supposed to run on newer kernels where user extended xattrs
> >   are allowed on symlinks and special files. Will post patch in 
> >   same thread (Ted Ts'o).
> >
> > Testing
> > -------
> > - Ran xfstest "./check -g auto" with and without patches and did not
> >   notice any new failures.
> >
> > - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
> >   filesystems and it works.
> >  
> > Description
> > ===========
> >
> > Right now we don't allow setting user.* xattrs on symlinks and special
> > files at all. Initially I thought that real reason behind this
> > restriction is quota limitations but from last conversation it seemed
> > that real reason is that permission bits on symlink and special files
> > are special and different from regular files and directories, hence
> > this restriction is in place. (I tested with xfs user quota enabled and
> > quota restrictions kicked in on symlink).
> >
> > This version of patch allows reading/writing user.* xattr on symlink and
> > special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
> 
> This part of your project makes perfect sense. There's no good
> security reason that you shouldn't set user.* xattrs on symlinks
> and/or special files.
> 
> However, your virtiofs use case is unreasonable.

Ok. So we can merge this patch irrespective of the fact whether virtiofs
should make use of this mechanism or not, right?

> 
> > Who wants to set user.* xattr on symlink/special files
> > -----------------------------------------------------
> > I have primarily two users at this point of time.
> >
> > - virtiofs daemon.
> >
> > - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
> >   fuse-overlay as well and he ran into similar issue. So fuse-overlay
> >   should benefit from this change as well.
> >
> > Why virtiofsd wants to set user.* xattr on symlink/special files
> > ----------------------------------------------------------------
> > In virtiofs, actual file server is virtiosd daemon running on host.
> > There we have a mode where xattrs can be remapped to something else.
> > For example security.selinux can be remapped to
> > user.virtiofsd.securit.selinux on the host.
> 
> As I have stated before, this introduces a breach in security.
> It allows an unprivileged process on the host to manipulate the
> security state of the guest. This is horribly wrong. It is not
> sufficient to claim that the breach requires misconfiguration
> to exploit. Don't do this.

So couple of things.

- Right now whole virtiofs model is relying on the fact that host
  unpriviliged users don't have access to shared directory. Otherwise
  guest process can simply drop a setuid root binary in shared directory
  and unpriviliged process can execute it and take over host system.

  So if virtiofs makes use of this mechanism, we are well with-in
  the existing constraints. If users don't follow the constraints,
  bad things can happen.

- I think Smalley provided a solution for your concern in other thread
  we discussed this issue.

  https://lore.kernel.org/selinux/CAEjxPJ4411vL3+Ab-J0yrRTmXoEf8pVR3x3CSRgPjfzwiUcDtw@mail.gmail.com/T/#mddea4cec7a68c3ee5e8826d650020361030209d6


  "So for example if the host policy says that only virtiofsd can set
attributes on those files, then the guest MAC labels along with all
the other attributes are protected against tampering by any other
process on the host."

 Apart from hiding the shared directory from unpriviliged processes,
 we could design selinux policy in such a way that only virtiofsd
 is allowed "setattr". That should make sure even in case of
 misconfiguration, unprivileged process is not able to change
 guest security xattrs stored in "user.virtiofs.security.selinux".

 I think that sounds like a very reasonable proposition.

> 
> > This remapping is useful when SELinux is enabled in guest and virtiofs
> > as being used as rootfs. Guest and host SELinux policy might not match
> > and host policy might deny security.selinux xattr setting by guest
> > onto host. Or host might have SELinux disabled and in that case to
> > be able to set security.selinux xattr, virtiofsd will need to have
> > CAP_SYS_ADMIN (which we are trying to avoid).
> 
> Adding this mapping to virtiofs provides the breach for any
> LSM using xattrs.

I think both the points above answer this as well.

> 
> >  Being able to remap
> > guest security.selinux (or other xattrs) on host to something else
> > is also better from security point of view.
> >
> > But when we try this, we noticed that SELinux relabeling in guest
> > is failing on some symlinks. When I debugged a little more, I
> > came to know that "user.*" xattrs are not allowed on symlinks
> > or special files.
> >
> > So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
> > allow virtiofs to arbitrarily remap guests's xattrs to something
> > else on host and that solves this SELinux issue nicely and provides
> > two SELinux policies (host and guest) to co-exist nicely without
> > interfering with each other.
> 
> virtiofs could use security.* or system.* xattrs instead of user.*
> xattrs. Don't use user.* xattrs.

So requirement is that every layer (host, guest and nested guest), will
have a separate security.selinux label and virtiofsd should be able
to retrieve/set any of the labels depending on access.

How do we achieve that with single security.selinux label per inode on host.

Vivek
Vivek Goyal Sept. 2, 2021, 5:42 p.m. UTC | #3
On Thu, Sep 02, 2021 at 01:05:01PM -0400, Vivek Goyal wrote:
> On Thu, Sep 02, 2021 at 08:43:50AM -0700, Casey Schaufler wrote:
> > On 9/2/2021 8:22 AM, Vivek Goyal wrote:
> > > Hi,
> > >
> > > This is V3 of the patch. Previous versions were posted here.
> > >
> > > v2:
> > > https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> > > v1:
> > > https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
> > > +m/
> > >
> > > Changes since v2
> > > ----------------
> > > - Do not call inode_permission() for special files as file mode bits
> > >   on these files represent permissions to read/write from/to device
> > >   and not necessarily permission to read/write xattrs. In this case
> > >   now user.* extended xattrs can be read/written on special files
> > >   as long as caller is owner of file or has CAP_FOWNER.
> > >  
> > > - Fixed "man xattr". Will post a patch in same thread little later. (J.
> > >   Bruce Fields)
> > >
> > > - Fixed xfstest 062. Changed it to run only on older kernels where
> > >   user extended xattrs are not allowed on symlinks/special files. Added
> > >   a new replacement test 648 which does exactly what 062. Just that
> > >   it is supposed to run on newer kernels where user extended xattrs
> > >   are allowed on symlinks and special files. Will post patch in 
> > >   same thread (Ted Ts'o).
> > >
> > > Testing
> > > -------
> > > - Ran xfstest "./check -g auto" with and without patches and did not
> > >   notice any new failures.
> > >
> > > - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
> > >   filesystems and it works.
> > >  
> > > Description
> > > ===========
> > >
> > > Right now we don't allow setting user.* xattrs on symlinks and special
> > > files at all. Initially I thought that real reason behind this
> > > restriction is quota limitations but from last conversation it seemed
> > > that real reason is that permission bits on symlink and special files
> > > are special and different from regular files and directories, hence
> > > this restriction is in place. (I tested with xfs user quota enabled and
> > > quota restrictions kicked in on symlink).
> > >
> > > This version of patch allows reading/writing user.* xattr on symlink and
> > > special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
> > 
> > This part of your project makes perfect sense. There's no good
> > security reason that you shouldn't set user.* xattrs on symlinks
> > and/or special files.
> > 
> > However, your virtiofs use case is unreasonable.
> 
> Ok. So we can merge this patch irrespective of the fact whether virtiofs
> should make use of this mechanism or not, right?
> 
> > 
> > > Who wants to set user.* xattr on symlink/special files
> > > -----------------------------------------------------
> > > I have primarily two users at this point of time.
> > >
> > > - virtiofs daemon.
> > >
> > > - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
> > >   fuse-overlay as well and he ran into similar issue. So fuse-overlay
> > >   should benefit from this change as well.
> > >
> > > Why virtiofsd wants to set user.* xattr on symlink/special files
> > > ----------------------------------------------------------------
> > > In virtiofs, actual file server is virtiosd daemon running on host.
> > > There we have a mode where xattrs can be remapped to something else.
> > > For example security.selinux can be remapped to
> > > user.virtiofsd.securit.selinux on the host.
> > 
> > As I have stated before, this introduces a breach in security.
> > It allows an unprivileged process on the host to manipulate the
> > security state of the guest. This is horribly wrong. It is not
> > sufficient to claim that the breach requires misconfiguration
> > to exploit. Don't do this.
> 
> So couple of things.
> 
> - Right now whole virtiofs model is relying on the fact that host
>   unpriviliged users don't have access to shared directory. Otherwise
>   guest process can simply drop a setuid root binary in shared directory
>   and unpriviliged process can execute it and take over host system.
> 
>   So if virtiofs makes use of this mechanism, we are well with-in
>   the existing constraints. If users don't follow the constraints,
>   bad things can happen.
> 
> - I think Smalley provided a solution for your concern in other thread
>   we discussed this issue.
> 
>   https://lore.kernel.org/selinux/CAEjxPJ4411vL3+Ab-J0yrRTmXoEf8pVR3x3CSRgPjfzwiUcDtw@mail.gmail.com/T/#mddea4cec7a68c3ee5e8826d650020361030209d6
> 
> 
>   "So for example if the host policy says that only virtiofsd can set
> attributes on those files, then the guest MAC labels along with all
> the other attributes are protected against tampering by any other
> process on the host."
> 
>  Apart from hiding the shared directory from unpriviliged processes,
>  we could design selinux policy in such a way that only virtiofsd
>  is allowed "setattr". That should make sure even in case of
>  misconfiguration, unprivileged process is not able to change
>  guest security xattrs stored in "user.virtiofs.security.selinux".
> 
>  I think that sounds like a very reasonable proposition.
> 
> > 
> > > This remapping is useful when SELinux is enabled in guest and virtiofs
> > > as being used as rootfs. Guest and host SELinux policy might not match
> > > and host policy might deny security.selinux xattr setting by guest
> > > onto host. Or host might have SELinux disabled and in that case to
> > > be able to set security.selinux xattr, virtiofsd will need to have
> > > CAP_SYS_ADMIN (which we are trying to avoid).
> > 
> > Adding this mapping to virtiofs provides the breach for any
> > LSM using xattrs.
> 
> I think both the points above answer this as well.
> 
> > 
> > >  Being able to remap
> > > guest security.selinux (or other xattrs) on host to something else
> > > is also better from security point of view.
> > >
> > > But when we try this, we noticed that SELinux relabeling in guest
> > > is failing on some symlinks. When I debugged a little more, I
> > > came to know that "user.*" xattrs are not allowed on symlinks
> > > or special files.
> > >
> > > So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
> > > allow virtiofs to arbitrarily remap guests's xattrs to something
> > > else on host and that solves this SELinux issue nicely and provides
> > > two SELinux policies (host and guest) to co-exist nicely without
> > > interfering with each other.
> > 
> > virtiofs could use security.* or system.* xattrs instead of user.*
> > xattrs. Don't use user.* xattrs.
> 
> So requirement is that every layer (host, guest and nested guest), will
> have a separate security.selinux label and virtiofsd should be able
> to retrieve/set any of the labels depending on access.
> 
> How do we achieve that with single security.selinux label per inode on host.

I guess we could think of using trusted.* but that requires CAP_SYS_ADMIN
in init_user_ns. And we wanted to retain capability to run virtiofsd
inside user namespace too. Also we wanted to give minimum required
capabilities to virtiofsd to reduce the risk what if virtiofsd gets
compromised. So amount of damage it can do to system is minimum.

So guest security.selinux xattr could potentially be mapped to.

trusted.virtiofs.security.selinux

nested guest selinux xattrs could be mapped to.

trusted.virtiofs.trusted.virtiofs.security.selinux

And given reading/setting trusted.* requires CAP_SYS_ADMIN, that means
unpriviliged processes can't change these security attributes of a
file.

And trade-off is that virtiofsd process needs to be given CAP_SYS_ADMIN.

Frankly speaking, we are more concerned about the security of host
system (as opposed to something changing in file for guest). So while
using "trusted.*" is still an option, I would think that not running
virtiofsd with CAP_SYS_ADMIN probably has its advantages too. On host
if we can just hide the shared dir from unpriviliged processes then
we get best of both the worlds. Unpriviliged processes can't change
anything on the shared file at the same time, possible damage by
virtiofsd is less if it gets compromised.

Thanks
Vivek
Andreas Gruenbacher Sept. 2, 2021, 5:52 p.m. UTC | #4
Hi,

On Thu, Sep 2, 2021 at 5:22 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> This is V3 of the patch. Previous versions were posted here.
>
> v2: https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> v1: https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.com/
>
> Changes since v2
> ----------------
> - Do not call inode_permission() for special files as file mode bits
>   on these files represent permissions to read/write from/to device
>   and not necessarily permission to read/write xattrs. In this case
>   now user.* extended xattrs can be read/written on special files
>   as long as caller is owner of file or has CAP_FOWNER.
>
> - Fixed "man xattr". Will post a patch in same thread little later. (J.
>   Bruce Fields)
>
> - Fixed xfstest 062. Changed it to run only on older kernels where
>   user extended xattrs are not allowed on symlinks/special files. Added
>   a new replacement test 648 which does exactly what 062. Just that
>   it is supposed to run on newer kernels where user extended xattrs
>   are allowed on symlinks and special files. Will post patch in
>   same thread (Ted Ts'o).
>
> Testing
> -------
> - Ran xfstest "./check -g auto" with and without patches and did not
>   notice any new failures.
>
> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
>   filesystems and it works.
>
> Description
> ===========
>
> Right now we don't allow setting user.* xattrs on symlinks and special
> files at all. Initially I thought that real reason behind this
> restriction is quota limitations but from last conversation it seemed
> that real reason is that permission bits on symlink and special files
> are special and different from regular files and directories, hence
> this restriction is in place. (I tested with xfs user quota enabled and
> quota restrictions kicked in on symlink).
>
> This version of patch allows reading/writing user.* xattr on symlink and
> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.

the idea behind user.* xattrs is that they behave similar to file
contents as far as permissions go. It follows from that that symlinks
and special files cannot have user.* xattrs. This has been the model
for many years now and applications may be expecting these semantics,
so we cannot simply change the behavior. So NACK from me.

> Who wants to set user.* xattr on symlink/special files
> -----------------------------------------------------
> I have primarily two users at this point of time.
>
> - virtiofs daemon.
>
> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
>   should benefit from this change as well.
>
> Why virtiofsd wants to set user.* xattr on symlink/special files
> ----------------------------------------------------------------
> In virtiofs, actual file server is virtiosd daemon running on host.
> There we have a mode where xattrs can be remapped to something else.
> For example security.selinux can be remapped to
> user.virtiofsd.securit.selinux on the host.
>
> This remapping is useful when SELinux is enabled in guest and virtiofs
> as being used as rootfs. Guest and host SELinux policy might not match
> and host policy might deny security.selinux xattr setting by guest
> onto host. Or host might have SELinux disabled and in that case to
> be able to set security.selinux xattr, virtiofsd will need to have
> CAP_SYS_ADMIN (which we are trying to avoid). Being able to remap
> guest security.selinux (or other xattrs) on host to something else
> is also better from security point of view.
>
> But when we try this, we noticed that SELinux relabeling in guest
> is failing on some symlinks. When I debugged a little more, I
> came to know that "user.*" xattrs are not allowed on symlinks
> or special files.
>
> So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
> allow virtiofs to arbitrarily remap guests's xattrs to something
> else on host and that solves this SELinux issue nicely and provides
> two SELinux policies (host and guest) to co-exist nicely without
> interfering with each other.

The fact that user.* xattrs don't work in this remapping scenario
should have told you that you're doing things wrong; the user.*
namespace seriously was never meant to be abused in this way.

You may be able to get away with using trusted.* xattrs which support
roughly the kind of daemon use I think you're talking about here, but
I'm not sure selinux will be happy with labels that aren't fully under
its own control. I really wonder why this wasn't obvious enough.

Thanks,
Andreas

> Thanks
> Vivek
>
> Vivek Goyal (1):
>   xattr: Allow user.* xattr on symlink and special files
>
>  fs/xattr.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
>
> --
> 2.31.1
>
Vivek Goyal Sept. 2, 2021, 6:48 p.m. UTC | #5
On Thu, Sep 02, 2021 at 07:52:41PM +0200, Andreas Gruenbacher wrote:
> Hi,
> 
> On Thu, Sep 2, 2021 at 5:22 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > This is V3 of the patch. Previous versions were posted here.
> >
> > v2: https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> > v1: https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.com/
> >
> > Changes since v2
> > ----------------
> > - Do not call inode_permission() for special files as file mode bits
> >   on these files represent permissions to read/write from/to device
> >   and not necessarily permission to read/write xattrs. In this case
> >   now user.* extended xattrs can be read/written on special files
> >   as long as caller is owner of file or has CAP_FOWNER.
> >
> > - Fixed "man xattr". Will post a patch in same thread little later. (J.
> >   Bruce Fields)
> >
> > - Fixed xfstest 062. Changed it to run only on older kernels where
> >   user extended xattrs are not allowed on symlinks/special files. Added
> >   a new replacement test 648 which does exactly what 062. Just that
> >   it is supposed to run on newer kernels where user extended xattrs
> >   are allowed on symlinks and special files. Will post patch in
> >   same thread (Ted Ts'o).
> >
> > Testing
> > -------
> > - Ran xfstest "./check -g auto" with and without patches and did not
> >   notice any new failures.
> >
> > - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
> >   filesystems and it works.
> >
> > Description
> > ===========
> >
> > Right now we don't allow setting user.* xattrs on symlinks and special
> > files at all. Initially I thought that real reason behind this
> > restriction is quota limitations but from last conversation it seemed
> > that real reason is that permission bits on symlink and special files
> > are special and different from regular files and directories, hence
> > this restriction is in place. (I tested with xfs user quota enabled and
> > quota restrictions kicked in on symlink).
> >
> > This version of patch allows reading/writing user.* xattr on symlink and
> > special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
> 
> the idea behind user.* xattrs is that they behave similar to file
> contents as far as permissions go. It follows from that that symlinks
> and special files cannot have user.* xattrs. This has been the model
> for many years now and applications may be expecting these semantics,
> so we cannot simply change the behavior. So NACK from me.

Directories with sticky bit break this general rule and don't follow
permission bit model.

man xattr says.

*****************************************************************
and access to user extended  attributes  is  re‐
       stricted  to  the  owner and to users with appropriate capabilities for
       directories with the sticky bit set
******************************************************************

So why not allow similar exceptions for symlinks and device files.

I can understand the concern about behavior change suddenly and
applications being surprised. If that's the only concern we could
think of making user opt-in for this new behavior based on a kernel
CONFIG, kernel command line or something else.


> 
> > Who wants to set user.* xattr on symlink/special files
> > -----------------------------------------------------
> > I have primarily two users at this point of time.
> >
> > - virtiofs daemon.
> >
> > - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
> >   fuse-overlay as well and he ran into similar issue. So fuse-overlay
> >   should benefit from this change as well.
> >
> > Why virtiofsd wants to set user.* xattr on symlink/special files
> > ----------------------------------------------------------------
> > In virtiofs, actual file server is virtiosd daemon running on host.
> > There we have a mode where xattrs can be remapped to something else.
> > For example security.selinux can be remapped to
> > user.virtiofsd.securit.selinux on the host.
> >
> > This remapping is useful when SELinux is enabled in guest and virtiofs
> > as being used as rootfs. Guest and host SELinux policy might not match
> > and host policy might deny security.selinux xattr setting by guest
> > onto host. Or host might have SELinux disabled and in that case to
> > be able to set security.selinux xattr, virtiofsd will need to have
> > CAP_SYS_ADMIN (which we are trying to avoid). Being able to remap
> > guest security.selinux (or other xattrs) on host to something else
> > is also better from security point of view.
> >
> > But when we try this, we noticed that SELinux relabeling in guest
> > is failing on some symlinks. When I debugged a little more, I
> > came to know that "user.*" xattrs are not allowed on symlinks
> > or special files.
> >
> > So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
> > allow virtiofs to arbitrarily remap guests's xattrs to something
> > else on host and that solves this SELinux issue nicely and provides
> > two SELinux policies (host and guest) to co-exist nicely without
> > interfering with each other.
> 
> The fact that user.* xattrs don't work in this remapping scenario
> should have told you that you're doing things wrong; the user.*
> namespace seriously was never meant to be abused in this way.

Guest's security label is not be parsed by host kernel. Host kernel
will have its own security label and will take decisions based on
that. In that aspect making use of "user.*" xattr seemed to make
lot of sense and we were wondering why user.* xattr is limited to
regualr files and directories only and can we change that behavior.

> 
> You may be able to get away with using trusted.* xattrs which support
> roughly the kind of daemon use I think you're talking about here, but
> I'm not sure selinux will be happy with labels that aren't fully under
> its own control. I really wonder why this wasn't obvious enough.

I guess trusted.* will do same thing. But it requires CAP_SYS_ADMIN
in init_user_ns. And that rules out running virtiofsd unpriviliged
or inside a user namespace. Also it reduces the risk posted by
virtiofsd on host filesystem due to CAP_SYS_ADMIN. That's why we
were trying to steer clear of trusted.* xattr space.

Also, trusted.* xattr space does not work with NFS.

$ setfattr -n "trusted.virtiofs" -v "foo" test.txt
setfattr: test.txt: Operation not supported

We want to be able run virtiofsd over NFS mounted dir too.

So its not that we did not consider trusted.* xattrs. We ran
into above issues.

Thanks
Vivek
Casey Schaufler Sept. 2, 2021, 6:55 p.m. UTC | #6
On 9/2/2021 10:42 AM, Vivek Goyal wrote:
> On Thu, Sep 02, 2021 at 01:05:01PM -0400, Vivek Goyal wrote:
>> On Thu, Sep 02, 2021 at 08:43:50AM -0700, Casey Schaufler wrote:
>>> On 9/2/2021 8:22 AM, Vivek Goyal wrote:
>>>> Hi,
>>>>
>>>> This is V3 of the patch. Previous versions were posted here.
>>>>
>>>> v2:
>>>> https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
>>>> v1:
>>>> https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
>>>> +m/
>>>>
>>>> Changes since v2
>>>> ----------------
>>>> - Do not call inode_permission() for special files as file mode bits
>>>>   on these files represent permissions to read/write from/to device
>>>>   and not necessarily permission to read/write xattrs. In this case
>>>>   now user.* extended xattrs can be read/written on special files
>>>>   as long as caller is owner of file or has CAP_FOWNER.
>>>>  
>>>> - Fixed "man xattr". Will post a patch in same thread little later. (J.
>>>>   Bruce Fields)
>>>>
>>>> - Fixed xfstest 062. Changed it to run only on older kernels where
>>>>   user extended xattrs are not allowed on symlinks/special files. Added
>>>>   a new replacement test 648 which does exactly what 062. Just that
>>>>   it is supposed to run on newer kernels where user extended xattrs
>>>>   are allowed on symlinks and special files. Will post patch in 
>>>>   same thread (Ted Ts'o).
>>>>
>>>> Testing
>>>> -------
>>>> - Ran xfstest "./check -g auto" with and without patches and did not
>>>>   notice any new failures.
>>>>
>>>> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
>>>>   filesystems and it works.
>>>>  
>>>> Description
>>>> ===========
>>>>
>>>> Right now we don't allow setting user.* xattrs on symlinks and special
>>>> files at all. Initially I thought that real reason behind this
>>>> restriction is quota limitations but from last conversation it seemed
>>>> that real reason is that permission bits on symlink and special files
>>>> are special and different from regular files and directories, hence
>>>> this restriction is in place. (I tested with xfs user quota enabled and
>>>> quota restrictions kicked in on symlink).
>>>>
>>>> This version of patch allows reading/writing user.* xattr on symlink and
>>>> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
>>> This part of your project makes perfect sense. There's no good
>>> security reason that you shouldn't set user.* xattrs on symlinks
>>> and/or special files.
>>>
>>> However, your virtiofs use case is unreasonable.
>> Ok. So we can merge this patch irrespective of the fact whether virtiofs
>> should make use of this mechanism or not, right?

I don't see a security objection. I did see that Andreas Gruenbacher
<agruenba@redhat.com> has objections to the behavior.


>>>> Who wants to set user.* xattr on symlink/special files
>>>> -----------------------------------------------------
>>>> I have primarily two users at this point of time.
>>>>
>>>> - virtiofs daemon.
>>>>
>>>> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
>>>>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
>>>>   should benefit from this change as well.
>>>>
>>>> Why virtiofsd wants to set user.* xattr on symlink/special files
>>>> ----------------------------------------------------------------
>>>> In virtiofs, actual file server is virtiosd daemon running on host.
>>>> There we have a mode where xattrs can be remapped to something else.
>>>> For example security.selinux can be remapped to
>>>> user.virtiofsd.securit.selinux on the host.
>>> As I have stated before, this introduces a breach in security.
>>> It allows an unprivileged process on the host to manipulate the
>>> security state of the guest. This is horribly wrong. It is not
>>> sufficient to claim that the breach requires misconfiguration
>>> to exploit. Don't do this.
>> So couple of things.
>>
>> - Right now whole virtiofs model is relying on the fact that host
>>   unpriviliged users don't have access to shared directory. Otherwise
>>   guest process can simply drop a setuid root binary in shared directory
>>   and unpriviliged process can execute it and take over host system.
>>
>>   So if virtiofs makes use of this mechanism, we are well with-in
>>   the existing constraints. If users don't follow the constraints,
>>   bad things can happen.
>>
>> - I think Smalley provided a solution for your concern in other thread
>>   we discussed this issue.
>>
>>   https://lore.kernel.org/selinux/CAEjxPJ4411vL3+Ab-J0yrRTmXoEf8pVR3x3CSRgPjfzwiUcDtw@mail.gmail.com/T/#mddea4cec7a68c3ee5e8826d650020361030209d6
>>
>>
>>   "So for example if the host policy says that only virtiofsd can set
>> attributes on those files, then the guest MAC labels along with all
>> the other attributes are protected against tampering by any other
>> process on the host."

You can't count on SELinux policy to address the issue on a
system running Smack. Or any other user of system.* xattrs,
be they in the kernel or user space. You can't even count on
SELinux policy to be correct. virtiofs has to present a "safe"
situation regardless of how security.* xattrs are used and
regardless of which, if any, LSMs are configured. You can't
do that with user.* attributes.


>>
>>  Apart from hiding the shared directory from unpriviliged processes,
>>  we could design selinux policy in such a way that only virtiofsd
>>  is allowed "setattr". That should make sure even in case of
>>  misconfiguration, unprivileged process is not able to change
>>  guest security xattrs stored in "user.virtiofs.security.selinux".
>>
>>  I think that sounds like a very reasonable proposition.
>>
>>>> This remapping is useful when SELinux is enabled in guest and virtiofs
>>>> as being used as rootfs. Guest and host SELinux policy might not match
>>>> and host policy might deny security.selinux xattr setting by guest
>>>> onto host. Or host might have SELinux disabled and in that case to
>>>> be able to set security.selinux xattr, virtiofsd will need to have
>>>> CAP_SYS_ADMIN (which we are trying to avoid).
>>> Adding this mapping to virtiofs provides the breach for any
>>> LSM using xattrs.
>> I think both the points above answer this as well.
>>
>>>>  Being able to remap
>>>> guest security.selinux (or other xattrs) on host to something else
>>>> is also better from security point of view.
>>>>
>>>> But when we try this, we noticed that SELinux relabeling in guest
>>>> is failing on some symlinks. When I debugged a little more, I
>>>> came to know that "user.*" xattrs are not allowed on symlinks
>>>> or special files.
>>>>
>>>> So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
>>>> allow virtiofs to arbitrarily remap guests's xattrs to something
>>>> else on host and that solves this SELinux issue nicely and provides
>>>> two SELinux policies (host and guest) to co-exist nicely without
>>>> interfering with each other.
>>> virtiofs could use security.* or system.* xattrs instead of user.*
>>> xattrs. Don't use user.* xattrs.
>> So requirement is that every layer (host, guest and nested guest), will
>> have a separate security.selinux label and virtiofsd should be able
>> to retrieve/set any of the labels depending on access.
>>
>> How do we achieve that with single security.selinux label per inode on host.
> I guess we could think of using trusted.* but that requires CAP_SYS_ADMIN
> in init_user_ns. And we wanted to retain capability to run virtiofsd
> inside user namespace too. Also we wanted to give minimum required
> capabilities to virtiofsd to reduce the risk what if virtiofsd gets
> compromised. So amount of damage it can do to system is minimum.
>
> So guest security.selinux xattr could potentially be mapped to.
>
> trusted.virtiofs.security.selinux
>
> nested guest selinux xattrs could be mapped to.
>
> trusted.virtiofs.trusted.virtiofs.security.selinux
>
> And given reading/setting trusted.* requires CAP_SYS_ADMIN, that means
> unpriviliged processes can't change these security attributes of a
> file.
>
> And trade-off is that virtiofsd process needs to be given CAP_SYS_ADMIN.
>
> Frankly speaking, we are more concerned about the security of host
> system (as opposed to something changing in file for guest). So while
> using "trusted.*" is still an option, I would think that not running
> virtiofsd with CAP_SYS_ADMIN probably has its advantages too. On host
> if we can just hide the shared dir from unpriviliged processes then
> we get best of both the worlds. Unpriviliged processes can't change
> anything on the shared file at the same time, possible damage by
> virtiofsd is less if it gets compromised.
>
> Thanks
> Vivek
>
Casey Schaufler Sept. 2, 2021, 7:19 p.m. UTC | #7
On 9/2/2021 11:48 AM, Vivek Goyal wrote:
> On Thu, Sep 02, 2021 at 07:52:41PM +0200, Andreas Gruenbacher wrote:
>> Hi,
>>
>> On Thu, Sep 2, 2021 at 5:22 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>>> This is V3 of the patch. Previous versions were posted here.
>>>
>>> v2: https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
>>> v1: https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.com/
>>>
>>> Changes since v2
>>> ----------------
>>> - Do not call inode_permission() for special files as file mode bits
>>>   on these files represent permissions to read/write from/to device
>>>   and not necessarily permission to read/write xattrs. In this case
>>>   now user.* extended xattrs can be read/written on special files
>>>   as long as caller is owner of file or has CAP_FOWNER.
>>>
>>> - Fixed "man xattr". Will post a patch in same thread little later. (J.
>>>   Bruce Fields)
>>>
>>> - Fixed xfstest 062. Changed it to run only on older kernels where
>>>   user extended xattrs are not allowed on symlinks/special files. Added
>>>   a new replacement test 648 which does exactly what 062. Just that
>>>   it is supposed to run on newer kernels where user extended xattrs
>>>   are allowed on symlinks and special files. Will post patch in
>>>   same thread (Ted Ts'o).
>>>
>>> Testing
>>> -------
>>> - Ran xfstest "./check -g auto" with and without patches and did not
>>>   notice any new failures.
>>>
>>> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
>>>   filesystems and it works.
>>>
>>> Description
>>> ===========
>>>
>>> Right now we don't allow setting user.* xattrs on symlinks and special
>>> files at all. Initially I thought that real reason behind this
>>> restriction is quota limitations but from last conversation it seemed
>>> that real reason is that permission bits on symlink and special files
>>> are special and different from regular files and directories, hence
>>> this restriction is in place. (I tested with xfs user quota enabled and
>>> quota restrictions kicked in on symlink).
>>>
>>> This version of patch allows reading/writing user.* xattr on symlink and
>>> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
>> the idea behind user.* xattrs is that they behave similar to file
>> contents as far as permissions go. It follows from that that symlinks
>> and special files cannot have user.* xattrs. This has been the model
>> for many years now and applications may be expecting these semantics,
>> so we cannot simply change the behavior. So NACK from me.
> Directories with sticky bit break this general rule and don't follow
> permission bit model.

The sticky bit is a hack. It was introduced to stave off proposed
implementations of Access Control Lists, which it did successfully
for quite some time.

> man xattr says.
>
> *****************************************************************
> and access to user extended  attributes  is  re‐
>        stricted  to  the  owner and to users with appropriate capabilities for
>        directories with the sticky bit set
> ******************************************************************
>
> So why not allow similar exceptions for symlinks and device files.

Limiting exceptions is usually a good thing. If every system mechanism
devolves into a heap of special cases it becomes very difficult to
describe your system semantics or the system security model. 

> I can understand the concern about behavior change suddenly and
> applications being surprised. If that's the only concern we could
> think of making user opt-in for this new behavior based on a kernel
> CONFIG, kernel command line or something else.

That doesn't work in the world of distros. But you knew that.


>>> Who wants to set user.* xattr on symlink/special files
>>> -----------------------------------------------------
>>> I have primarily two users at this point of time.
>>>
>>> - virtiofs daemon.
>>>
>>> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
>>>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
>>>   should benefit from this change as well.
>>>
>>> Why virtiofsd wants to set user.* xattr on symlink/special files
>>> ----------------------------------------------------------------
>>> In virtiofs, actual file server is virtiosd daemon running on host.
>>> There we have a mode where xattrs can be remapped to something else.
>>> For example security.selinux can be remapped to
>>> user.virtiofsd.securit.selinux on the host.
>>>
>>> This remapping is useful when SELinux is enabled in guest and virtiofs
>>> as being used as rootfs. Guest and host SELinux policy might not match
>>> and host policy might deny security.selinux xattr setting by guest
>>> onto host. Or host might have SELinux disabled and in that case to
>>> be able to set security.selinux xattr, virtiofsd will need to have
>>> CAP_SYS_ADMIN (which we are trying to avoid). Being able to remap
>>> guest security.selinux (or other xattrs) on host to something else
>>> is also better from security point of view.
>>>
>>> But when we try this, we noticed that SELinux relabeling in guest
>>> is failing on some symlinks. When I debugged a little more, I
>>> came to know that "user.*" xattrs are not allowed on symlinks
>>> or special files.
>>>
>>> So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
>>> allow virtiofs to arbitrarily remap guests's xattrs to something
>>> else on host and that solves this SELinux issue nicely and provides
>>> two SELinux policies (host and guest) to co-exist nicely without
>>> interfering with each other.
>> The fact that user.* xattrs don't work in this remapping scenario
>> should have told you that you're doing things wrong; the user.*
>> namespace seriously was never meant to be abused in this way.
> Guest's security label is not be parsed by host kernel. Host kernel
> will have its own security label and will take decisions based on
> that. In that aspect making use of "user.*" xattr seemed to make
> lot of sense

It doesn't make sense. For files, directories or anything. It's
freaking hazardous.

>  and we were wondering why user.* xattr is limited to
> regualr files and directories only and can we change that behavior.
>
>> You may be able to get away with using trusted.* xattrs which support
>> roughly the kind of daemon use I think you're talking about here, but
>> I'm not sure selinux will be happy with labels that aren't fully under
>> its own control. I really wonder why this wasn't obvious enough.
> I guess trusted.* will do same thing. But it requires CAP_SYS_ADMIN
> in init_user_ns.

Right. That's because you're doing dangerous things.

>  And that rules out running virtiofsd unpriviliged

Right. That's because you're doing dangerous things.

> or inside a user namespace. Also it reduces the risk posted by
> virtiofsd on host filesystem due to CAP_SYS_ADMIN. That's why we
> were trying to steer clear of trusted.* xattr space.

Yeah, I get it. What's wrong with admitting that what you're
trying to do is dangerous, and that you have to be careful?

> Also, trusted.* xattr space does not work with NFS.

So, fix that?

>
> $ setfattr -n "trusted.virtiofs" -v "foo" test.txt
> setfattr: test.txt: Operation not supported
>
> We want to be able run virtiofsd over NFS mounted dir too.
>
> So its not that we did not consider trusted.* xattrs. We ran
> into above issues.
>
> Thanks
> Vivek
>
Vivek Goyal Sept. 2, 2021, 8:06 p.m. UTC | #8
On Thu, Sep 02, 2021 at 11:55:11AM -0700, Casey Schaufler wrote:
> On 9/2/2021 10:42 AM, Vivek Goyal wrote:
> > On Thu, Sep 02, 2021 at 01:05:01PM -0400, Vivek Goyal wrote:
> >> On Thu, Sep 02, 2021 at 08:43:50AM -0700, Casey Schaufler wrote:
> >>> On 9/2/2021 8:22 AM, Vivek Goyal wrote:
> >>>> Hi,
> >>>>
> >>>> This is V3 of the patch. Previous versions were posted here.
> >>>>
> >>>> v2:
> >>>> https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> >>>> v1:
> >>>> https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
> >>>> +m/
> >>>>
> >>>> Changes since v2
> >>>> ----------------
> >>>> - Do not call inode_permission() for special files as file mode bits
> >>>>   on these files represent permissions to read/write from/to device
> >>>>   and not necessarily permission to read/write xattrs. In this case
> >>>>   now user.* extended xattrs can be read/written on special files
> >>>>   as long as caller is owner of file or has CAP_FOWNER.
> >>>>  
> >>>> - Fixed "man xattr". Will post a patch in same thread little later. (J.
> >>>>   Bruce Fields)
> >>>>
> >>>> - Fixed xfstest 062. Changed it to run only on older kernels where
> >>>>   user extended xattrs are not allowed on symlinks/special files. Added
> >>>>   a new replacement test 648 which does exactly what 062. Just that
> >>>>   it is supposed to run on newer kernels where user extended xattrs
> >>>>   are allowed on symlinks and special files. Will post patch in 
> >>>>   same thread (Ted Ts'o).
> >>>>
> >>>> Testing
> >>>> -------
> >>>> - Ran xfstest "./check -g auto" with and without patches and did not
> >>>>   notice any new failures.
> >>>>
> >>>> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
> >>>>   filesystems and it works.
> >>>>  
> >>>> Description
> >>>> ===========
> >>>>
> >>>> Right now we don't allow setting user.* xattrs on symlinks and special
> >>>> files at all. Initially I thought that real reason behind this
> >>>> restriction is quota limitations but from last conversation it seemed
> >>>> that real reason is that permission bits on symlink and special files
> >>>> are special and different from regular files and directories, hence
> >>>> this restriction is in place. (I tested with xfs user quota enabled and
> >>>> quota restrictions kicked in on symlink).
> >>>>
> >>>> This version of patch allows reading/writing user.* xattr on symlink and
> >>>> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
> >>> This part of your project makes perfect sense. There's no good
> >>> security reason that you shouldn't set user.* xattrs on symlinks
> >>> and/or special files.
> >>>
> >>> However, your virtiofs use case is unreasonable.
> >> Ok. So we can merge this patch irrespective of the fact whether virtiofs
> >> should make use of this mechanism or not, right?
> 
> I don't see a security objection. I did see that Andreas Gruenbacher
> <agruenba@redhat.com> has objections to the behavior.
> 
> 
> >>>> Who wants to set user.* xattr on symlink/special files
> >>>> -----------------------------------------------------
> >>>> I have primarily two users at this point of time.
> >>>>
> >>>> - virtiofs daemon.
> >>>>
> >>>> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
> >>>>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
> >>>>   should benefit from this change as well.
> >>>>
> >>>> Why virtiofsd wants to set user.* xattr on symlink/special files
> >>>> ----------------------------------------------------------------
> >>>> In virtiofs, actual file server is virtiosd daemon running on host.
> >>>> There we have a mode where xattrs can be remapped to something else.
> >>>> For example security.selinux can be remapped to
> >>>> user.virtiofsd.securit.selinux on the host.
> >>> As I have stated before, this introduces a breach in security.
> >>> It allows an unprivileged process on the host to manipulate the
> >>> security state of the guest. This is horribly wrong. It is not
> >>> sufficient to claim that the breach requires misconfiguration
> >>> to exploit. Don't do this.
> >> So couple of things.
> >>
> >> - Right now whole virtiofs model is relying on the fact that host
> >>   unpriviliged users don't have access to shared directory. Otherwise
> >>   guest process can simply drop a setuid root binary in shared directory
> >>   and unpriviliged process can execute it and take over host system.
> >>
> >>   So if virtiofs makes use of this mechanism, we are well with-in
> >>   the existing constraints. If users don't follow the constraints,
> >>   bad things can happen.
> >>
> >> - I think Smalley provided a solution for your concern in other thread
> >>   we discussed this issue.
> >>
> >>   https://lore.kernel.org/selinux/CAEjxPJ4411vL3+Ab-J0yrRTmXoEf8pVR3x3CSRgPjfzwiUcDtw@mail.gmail.com/T/#mddea4cec7a68c3ee5e8826d650020361030209d6
> >>
> >>
> >>   "So for example if the host policy says that only virtiofsd can set
> >> attributes on those files, then the guest MAC labels along with all
> >> the other attributes are protected against tampering by any other
> >> process on the host."
> 
> You can't count on SELinux policy to address the issue on a
> system running Smack.
> Or any other user of system.* xattrs,
> be they in the kernel or user space. You can't even count on
> SELinux policy to be correct. virtiofs has to present a "safe"
> situation regardless of how security.* xattrs are used and
> regardless of which, if any, LSMs are configured. You can't
> do that with user.* attributes.

Lets take a step back. Your primary concern with using user.* xattrs
by virtiofsd is that it can be modified by unprivileged users on host.
And our solution to that problem is hide shared directory from
unprivileged users.

In addition to that, LSMs on host can block setting "user.*" xattrs by
virtiofsd domain only for additional protection. If LSMs are not configured,
then hiding the directory is the solution.

So why that's not a solution and only relying on CAP_SYS_ADMIN is the
solution. I don't understand that part.

Also if directory is not hidden, unprivileged users can change file
data and other metadata. Why that's not a concern and why there is
so much of focus only security xattr. If you were to block modification
of file then you will have rely on LSMs. And if LSMs are not configured,
then we will rely on shared directory not being visible.

Can you please help me understand why hiding shared directory from
unprivileged users is not a solution (With both LSMs configured or
not configured on host). That's a requirement for virtiofs anyway. 
And if we agree on that, then I don't see why using "user.*" xattrs
for storing guest sercurity attributes is a problem.

Thanks
Vivek
Casey Schaufler Sept. 2, 2021, 10:34 p.m. UTC | #9
On 9/2/2021 1:06 PM, Vivek Goyal wrote:
> On Thu, Sep 02, 2021 at 11:55:11AM -0700, Casey Schaufler wrote:
>> On 9/2/2021 10:42 AM, Vivek Goyal wrote:
>>> On Thu, Sep 02, 2021 at 01:05:01PM -0400, Vivek Goyal wrote:
>>>> On Thu, Sep 02, 2021 at 08:43:50AM -0700, Casey Schaufler wrote:
>>>>> On 9/2/2021 8:22 AM, Vivek Goyal wrote:
>>>>>> Hi,
>>>>>>
>>>>>> This is V3 of the patch. Previous versions were posted here.
>>>>>>
>>>>>> v2:
>>>>>> https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
>>>>>> v1:
>>>>>> https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
>>>>>> +m/
>>>>>>
>>>>>> Changes since v2
>>>>>> ----------------
>>>>>> - Do not call inode_permission() for special files as file mode bits
>>>>>>   on these files represent permissions to read/write from/to device
>>>>>>   and not necessarily permission to read/write xattrs. In this case
>>>>>>   now user.* extended xattrs can be read/written on special files
>>>>>>   as long as caller is owner of file or has CAP_FOWNER.
>>>>>>  
>>>>>> - Fixed "man xattr". Will post a patch in same thread little later. (J.
>>>>>>   Bruce Fields)
>>>>>>
>>>>>> - Fixed xfstest 062. Changed it to run only on older kernels where
>>>>>>   user extended xattrs are not allowed on symlinks/special files. Added
>>>>>>   a new replacement test 648 which does exactly what 062. Just that
>>>>>>   it is supposed to run on newer kernels where user extended xattrs
>>>>>>   are allowed on symlinks and special files. Will post patch in 
>>>>>>   same thread (Ted Ts'o).
>>>>>>
>>>>>> Testing
>>>>>> -------
>>>>>> - Ran xfstest "./check -g auto" with and without patches and did not
>>>>>>   notice any new failures.
>>>>>>
>>>>>> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
>>>>>>   filesystems and it works.
>>>>>>  
>>>>>> Description
>>>>>> ===========
>>>>>>
>>>>>> Right now we don't allow setting user.* xattrs on symlinks and special
>>>>>> files at all. Initially I thought that real reason behind this
>>>>>> restriction is quota limitations but from last conversation it seemed
>>>>>> that real reason is that permission bits on symlink and special files
>>>>>> are special and different from regular files and directories, hence
>>>>>> this restriction is in place. (I tested with xfs user quota enabled and
>>>>>> quota restrictions kicked in on symlink).
>>>>>>
>>>>>> This version of patch allows reading/writing user.* xattr on symlink and
>>>>>> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
>>>>> This part of your project makes perfect sense. There's no good
>>>>> security reason that you shouldn't set user.* xattrs on symlinks
>>>>> and/or special files.
>>>>>
>>>>> However, your virtiofs use case is unreasonable.
>>>> Ok. So we can merge this patch irrespective of the fact whether virtiofs
>>>> should make use of this mechanism or not, right?
>> I don't see a security objection. I did see that Andreas Gruenbacher
>> <agruenba@redhat.com> has objections to the behavior.
>>
>>
>>>>>> Who wants to set user.* xattr on symlink/special files
>>>>>> -----------------------------------------------------
>>>>>> I have primarily two users at this point of time.
>>>>>>
>>>>>> - virtiofs daemon.
>>>>>>
>>>>>> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
>>>>>>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
>>>>>>   should benefit from this change as well.
>>>>>>
>>>>>> Why virtiofsd wants to set user.* xattr on symlink/special files
>>>>>> ----------------------------------------------------------------
>>>>>> In virtiofs, actual file server is virtiosd daemon running on host.
>>>>>> There we have a mode where xattrs can be remapped to something else.
>>>>>> For example security.selinux can be remapped to
>>>>>> user.virtiofsd.securit.selinux on the host.
>>>>> As I have stated before, this introduces a breach in security.
>>>>> It allows an unprivileged process on the host to manipulate the
>>>>> security state of the guest. This is horribly wrong. It is not
>>>>> sufficient to claim that the breach requires misconfiguration
>>>>> to exploit. Don't do this.
>>>> So couple of things.
>>>>
>>>> - Right now whole virtiofs model is relying on the fact that host
>>>>   unpriviliged users don't have access to shared directory. Otherwise
>>>>   guest process can simply drop a setuid root binary in shared directory
>>>>   and unpriviliged process can execute it and take over host system.
>>>>
>>>>   So if virtiofs makes use of this mechanism, we are well with-in
>>>>   the existing constraints. If users don't follow the constraints,
>>>>   bad things can happen.
>>>>
>>>> - I think Smalley provided a solution for your concern in other thread
>>>>   we discussed this issue.
>>>>
>>>>   https://lore.kernel.org/selinux/CAEjxPJ4411vL3+Ab-J0yrRTmXoEf8pVR3x3CSRgPjfzwiUcDtw@mail.gmail.com/T/#mddea4cec7a68c3ee5e8826d650020361030209d6
>>>>
>>>>
>>>>   "So for example if the host policy says that only virtiofsd can set
>>>> attributes on those files, then the guest MAC labels along with all
>>>> the other attributes are protected against tampering by any other
>>>> process on the host."
>> You can't count on SELinux policy to address the issue on a
>> system running Smack.
>> Or any other user of system.* xattrs,
>> be they in the kernel or user space. You can't even count on
>> SELinux policy to be correct. virtiofs has to present a "safe"
>> situation regardless of how security.* xattrs are used and
>> regardless of which, if any, LSMs are configured. You can't
>> do that with user.* attributes.
> Lets take a step back. Your primary concern with using user.* xattrs
> by virtiofsd is that it can be modified by unprivileged users on host.
> And our solution to that problem is hide shared directory from
> unprivileged users.

You really don't see how fragile that is, do you? How a single
errant call to rename(), chmod() or chown() on the host can expose
the entire guest to exploitation. That's not even getting into
the bag of mount() tricks.


> In addition to that, LSMs on host can block setting "user.*" xattrs by
> virtiofsd domain only for additional protection.

Try thinking outside the SELinux box briefly, if you possibly can.
An LSM that implements just Bell & LaPadula isn't going to have a
"virtiofs domain". Neither is a Smack "3 domain" system. Smack doesn't
distinguish writing user xattrs from writing other file attributes
in policy. Your argument requires a fine grained policy a'la SELinux.
And an application specific SELinux policy at that.

>  If LSMs are not configured,
> then hiding the directory is the solution.

It's not a solution at all. It's wishful thinking that
some admin is going to do absolutely everything right, will
never make a mistake and will never, ever, read the mount(2)
man page.

> So why that's not a solution and only relying on CAP_SYS_ADMIN is the
> solution. I don't understand that part.

It comes back to your design, which is fundamentally flawed. You
can't store system security information in an attribute that can
be manipulated by untrusted entities. That's why we have system.*
xattrs. You want to have an attribute on the host that maps to a
security attribute on the guest. The host has to protect the attribute
on the guest with mechanisms of comparable strength as the guest's
mechanisms. Otherwise you can't trust the guest with host data.

It's a real shame that CAP_SYS_ADMIN is so scary. The capability
mechanism as implemented today won't scale to the hundreds of individual
capabilities it would need to break CAP_SYS_ADMIN up. Maybe someday.
I'm not convinced that there isn't a way to accomplish what you're
trying to do without privilege, but this isn't it, and I don't know
what is. Sorry.

> Also if directory is not hidden, unprivileged users can change file
> data and other metadata.

I assumed that you've taken that into account. Are you saying that
isn't going to be done correctly either?

>  Why that's not a concern and why there is
> so much of focus only security xattr.

As with an NFS mount, the assumption is that UID 567 (or its magically
mapped equivalent) has the same access rights on both the server/host
and client/guest. I'm not worried about the mode bits because they are
presented consistently on both machines. If, on the other hand, an
attribute used to determine access is security.esprit on the guest and
user.security.esprit on the host, the unprivileged user on the host
can defeat the privilege requirements on the guest. That's why.

>  If you were to block modification
> of file then you will have rely on LSMs.

No. We're talking about the semantics of the xattr namespaces.
LSMs can further constrain access to xattrs, but the basic rules
of access to the user.* and security.* attributes are different
in any case. This is by design.

>  And if LSMs are not configured,
> then we will rely on shared directory not being visible.

LSMs are not the problem. LSMs use security.* xattrs, which is why
they come up in the discussion.

> Can you please help me understand why hiding shared directory from
> unprivileged users is not a solution

Maybe you can describe the mechanism you use to "hide" a shared directory
on the host. If the filesystem is mounted on the host it seems unlikely
that you can provide a convincing argument for sufficient protection.

>  (With both LSMs configured or
> not configured on host). That's a requirement for virtiofs anyway. 
> And if we agree on that, then I don't see why using "user.*" xattrs
> for storing guest sercurity attributes is a problem.
>
> Thanks
> Vivek
>
Vivek Goyal Sept. 3, 2021, 3:26 p.m. UTC | #10
On Thu, Sep 02, 2021 at 03:34:17PM -0700, Casey Schaufler wrote:
> On 9/2/2021 1:06 PM, Vivek Goyal wrote:
> > On Thu, Sep 02, 2021 at 11:55:11AM -0700, Casey Schaufler wrote:
> >> On 9/2/2021 10:42 AM, Vivek Goyal wrote:
> >>> On Thu, Sep 02, 2021 at 01:05:01PM -0400, Vivek Goyal wrote:
> >>>> On Thu, Sep 02, 2021 at 08:43:50AM -0700, Casey Schaufler wrote:
> >>>>> On 9/2/2021 8:22 AM, Vivek Goyal wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> This is V3 of the patch. Previous versions were posted here.
> >>>>>>
> >>>>>> v2:
> >>>>>> https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> >>>>>> v1:
> >>>>>> https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
> >>>>>> +m/
> >>>>>>
> >>>>>> Changes since v2
> >>>>>> ----------------
> >>>>>> - Do not call inode_permission() for special files as file mode bits
> >>>>>>   on these files represent permissions to read/write from/to device
> >>>>>>   and not necessarily permission to read/write xattrs. In this case
> >>>>>>   now user.* extended xattrs can be read/written on special files
> >>>>>>   as long as caller is owner of file or has CAP_FOWNER.
> >>>>>>  
> >>>>>> - Fixed "man xattr". Will post a patch in same thread little later. (J.
> >>>>>>   Bruce Fields)
> >>>>>>
> >>>>>> - Fixed xfstest 062. Changed it to run only on older kernels where
> >>>>>>   user extended xattrs are not allowed on symlinks/special files. Added
> >>>>>>   a new replacement test 648 which does exactly what 062. Just that
> >>>>>>   it is supposed to run on newer kernels where user extended xattrs
> >>>>>>   are allowed on symlinks and special files. Will post patch in 
> >>>>>>   same thread (Ted Ts'o).
> >>>>>>
> >>>>>> Testing
> >>>>>> -------
> >>>>>> - Ran xfstest "./check -g auto" with and without patches and did not
> >>>>>>   notice any new failures.
> >>>>>>
> >>>>>> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
> >>>>>>   filesystems and it works.
> >>>>>>  
> >>>>>> Description
> >>>>>> ===========
> >>>>>>
> >>>>>> Right now we don't allow setting user.* xattrs on symlinks and special
> >>>>>> files at all. Initially I thought that real reason behind this
> >>>>>> restriction is quota limitations but from last conversation it seemed
> >>>>>> that real reason is that permission bits on symlink and special files
> >>>>>> are special and different from regular files and directories, hence
> >>>>>> this restriction is in place. (I tested with xfs user quota enabled and
> >>>>>> quota restrictions kicked in on symlink).
> >>>>>>
> >>>>>> This version of patch allows reading/writing user.* xattr on symlink and
> >>>>>> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
> >>>>> This part of your project makes perfect sense. There's no good
> >>>>> security reason that you shouldn't set user.* xattrs on symlinks
> >>>>> and/or special files.
> >>>>>
> >>>>> However, your virtiofs use case is unreasonable.
> >>>> Ok. So we can merge this patch irrespective of the fact whether virtiofs
> >>>> should make use of this mechanism or not, right?
> >> I don't see a security objection. I did see that Andreas Gruenbacher
> >> <agruenba@redhat.com> has objections to the behavior.
> >>
> >>
> >>>>>> Who wants to set user.* xattr on symlink/special files
> >>>>>> -----------------------------------------------------
> >>>>>> I have primarily two users at this point of time.
> >>>>>>
> >>>>>> - virtiofs daemon.
> >>>>>>
> >>>>>> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
> >>>>>>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
> >>>>>>   should benefit from this change as well.
> >>>>>>
> >>>>>> Why virtiofsd wants to set user.* xattr on symlink/special files
> >>>>>> ----------------------------------------------------------------
> >>>>>> In virtiofs, actual file server is virtiosd daemon running on host.
> >>>>>> There we have a mode where xattrs can be remapped to something else.
> >>>>>> For example security.selinux can be remapped to
> >>>>>> user.virtiofsd.securit.selinux on the host.
> >>>>> As I have stated before, this introduces a breach in security.
> >>>>> It allows an unprivileged process on the host to manipulate the
> >>>>> security state of the guest. This is horribly wrong. It is not
> >>>>> sufficient to claim that the breach requires misconfiguration
> >>>>> to exploit. Don't do this.
> >>>> So couple of things.
> >>>>
> >>>> - Right now whole virtiofs model is relying on the fact that host
> >>>>   unpriviliged users don't have access to shared directory. Otherwise
> >>>>   guest process can simply drop a setuid root binary in shared directory
> >>>>   and unpriviliged process can execute it and take over host system.
> >>>>
> >>>>   So if virtiofs makes use of this mechanism, we are well with-in
> >>>>   the existing constraints. If users don't follow the constraints,
> >>>>   bad things can happen.
> >>>>
> >>>> - I think Smalley provided a solution for your concern in other thread
> >>>>   we discussed this issue.
> >>>>
> >>>>   https://lore.kernel.org/selinux/CAEjxPJ4411vL3+Ab-J0yrRTmXoEf8pVR3x3CSRgPjfzwiUcDtw@mail.gmail.com/T/#mddea4cec7a68c3ee5e8826d650020361030209d6
> >>>>
> >>>>
> >>>>   "So for example if the host policy says that only virtiofsd can set
> >>>> attributes on those files, then the guest MAC labels along with all
> >>>> the other attributes are protected against tampering by any other
> >>>> process on the host."
> >> You can't count on SELinux policy to address the issue on a
> >> system running Smack.
> >> Or any other user of system.* xattrs,
> >> be they in the kernel or user space. You can't even count on
> >> SELinux policy to be correct. virtiofs has to present a "safe"
> >> situation regardless of how security.* xattrs are used and
> >> regardless of which, if any, LSMs are configured. You can't
> >> do that with user.* attributes.
> > Lets take a step back. Your primary concern with using user.* xattrs
> > by virtiofsd is that it can be modified by unprivileged users on host.
> > And our solution to that problem is hide shared directory from
> > unprivileged users.
> 
> You really don't see how fragile that is, do you?

Yes, I am not sure why we are so opposed to the idea of using 
user.* xattrs for storing the guest security.selinux xattrs by virtiofsd.
And I am trying to understand that. And this discussion should help.

With virtiofsd, we want to keep all shared directory trees in a
parent directory which has read/write/search permissions only for root
so that no unpriviliged process can get to files in shared directory
and do any of the operations.

For example, /var/lib/containers/storage setup by podman has
following permissions.

ll -d /var/lib/containers/storage/
drwx------. 10 root root 4096 Jun 18  2020 /var/lib/containers/storage/

Now I should be able to create /var/lib/containers/storage/shared1
directory and ask virtiofsd to export "share1" to guest. Unprivileged
process on host can not open any of the files in shared1 dir, hence
should not be able to modify any data/metadata associated with the
file.

If this assumption is correct, then I should be able to use "user.*"
xattrs without having to worry about unprivileged processes modifying
security labels of guest/nested-guest.

> How a single
> errant call to rename(), chmod() or chown() on the host can expose
> the entire guest to exploitation. That's not even getting into
> the bag of mount() tricks.

I am relying on unpriviliged processes not having permissions to
read/search in shared directory. And I guess I have to. Even if
I use "trusted.*" xattrs, what about file data. Unpriviliged
processes can modify file data and that's going to be equally
problematic, isn't it? So why are we so focussed only on
security label part of it.

You have mentioned that file data is not necessarily
that big a problem because UID 1000 on host and UID 1000 in guest
should be treated same. But I am not sure we can do that. In kata
container model, guest images are untrusted. So they can simply cook
up UID 1000 or UID 0 or any other UID. Now there can be use
cases where we are ready to trust guest and treat UID 1000
on host and guest in same way. But primary model I am focussing
on is that guest shared directory remains isolated from other
users on host.

> 
> 
> > In addition to that, LSMs on host can block setting "user.*" xattrs by
> > virtiofsd domain only for additional protection.
> 
> Try thinking outside the SELinux box briefly, if you possibly can.
> An LSM that implements just Bell & LaPadula isn't going to have a
> "virtiofs domain". Neither is a Smack "3 domain" system. Smack doesn't
> distinguish writing user xattrs from writing other file attributes
> in policy. Your argument requires a fine grained policy a'la SELinux.
> And an application specific SELinux policy at that.

Ok, so does we have to have capability for every LSM to block write
to user xattr. I mean in above example, virtiofsd is relying on DAC so that
unprivileged processes can't modify user xattr labels. If we were to
use "trusted.*" xattr then we are relying on CAP_SYS_ADMIN.

IOW, it will be nice if one or more LSMs can provide mechanism fine
grained enough to block write to user xattr by unwanted application
and that provides extra level of security. But should it be mandatory?

> 
> >  If LSMs are not configured,
> > then hiding the directory is the solution.
> 
> It's not a solution at all. It's wishful thinking that
> some admin is going to do absolutely everything right, will
> never make a mistake and will never, ever, read the mount(2)
> man page.

I agree that its easy to make mistakes. But making mistakes is already
disastrous. So lets say and admin makes mistake and unprivileged
processes can open/read/write files in shared directory on host.
Assume I am using "trusted" xattrs to store guest's security labels.

Now file's data can be modified on host in unwated way and be very
problematic.

Guest can drop a setuid root binary in shared directory and unprivileged
process on host can run it and take over the system.

Isn't that even bigger problem. So to me making sure shared directory
is not reachable by unprivileged processes is absolute must requirement
for virtiofsd (when running as root). If we break that assumption,
we already have much bigger problems.

> 
> > So why that's not a solution and only relying on CAP_SYS_ADMIN is the
> > solution. I don't understand that part.
> 
> It comes back to your design, which is fundamentally flawed. You
> can't store system security information in an attribute that can
> be manipulated by untrusted entities.

You are assuming untrusted entities can have access to the shared dir. But
assumption in this model is, shared directory is not reachable by
unprivileged entities. If we break that requirement, there are much
bigger issues to deal with then just security attributes. 

> That's why we have system.*
> xattrs. You want to have an attribute on the host that maps to a
> security attribute on the guest. The host has to protect the attribute
> on the guest with mechanisms of comparable strength as the guest's
> mechanisms. Otherwise you can't trust the guest with host data.

Ok, I understand your desire that security xattrs as seen by guest kernel
should be protected by something stronger than simply user xattr.

> 
> It's a real shame that CAP_SYS_ADMIN is so scary. The capability
> mechanism as implemented today won't scale to the hundreds of individual
> capabilities it would need to break CAP_SYS_ADMIN up. Maybe someday.
> I'm not convinced that there isn't a way to accomplish what you're
> trying to do without privilege, but this isn't it, and I don't know
> what is. Sorry.
> 
> > Also if directory is not hidden, unprivileged users can change file
> > data and other metadata.
> 
> I assumed that you've taken that into account. Are you saying that
> isn't going to be done correctly either?

I am relying on shared directory not accessible to unprivileged processes.
If that assumption is broken, I guess,  all bets are off. Until and unless
one designs SELinux (or other LSM) policy in such a way so that
only virtiofsd can read/write to these shared directories.

I think Dan Walsh has got SELinux policy written for atleast kata
containers case.

So I guess we will rely on MAC to block unwanted access if users
make a mistake? Is that the idea? I guess that's why you want
a stronger mechansim to store guest security xattrs on host. If
users make a mistake then we have a fallback path?

> 
> >  Why that's not a concern and why there is
> > so much of focus only security xattr.
> 
> As with an NFS mount, the assumption is that UID 567 (or its magically
> mapped equivalent) has the same access rights on both the server/host
> and client/guest. I'm not worried about the mode bits because they are
> presented consistently on both machines. If, on the other hand, an
> attribute used to determine access is security.esprit on the guest and
> user.security.esprit on the host, the unprivileged user on the host
> can defeat the privilege requirements on the guest. That's why.

Hmm..., so if a user id 1000 inside guest can't modify security
xattrs then same user on host should be allowed to bypass that
by modifying user xattr. I agree with that.

Just that I am relying on shared directory not being accessible
to uid 1000 on host and you don't want to rely on that because
users can easily make mistake. And that's why want to stronger
form of mechanism to store security xattrs.

> 
> >  If you were to block modification
> > of file then you will have rely on LSMs.
> 
> No. We're talking about the semantics of the xattr namespaces.
> LSMs can further constrain access to xattrs, but the basic rules
> of access to the user.* and security.* attributes are different
> in any case. This is by design.
> 
> >  And if LSMs are not configured,
> > then we will rely on shared directory not being visible.
> 
> LSMs are not the problem. LSMs use security.* xattrs, which is why
> they come up in the discussion.
> 
> > Can you please help me understand why hiding shared directory from
> > unprivileged users is not a solution
> 
> Maybe you can describe the mechanism you use to "hide" a shared directory
> on the host. If the filesystem is mounted on the host it seems unlikely
> that you can provide a convincing argument for sufficient protection.

I am relying on changing direcotry permissions to allow read/write/search
permission to root only.

Thanks
Vivek
Casey Schaufler Sept. 3, 2021, 6:49 p.m. UTC | #11
On 9/3/2021 8:26 AM, Vivek Goyal wrote:
> On Thu, Sep 02, 2021 at 03:34:17PM -0700, Casey Schaufler wrote:
>> On 9/2/2021 1:06 PM, Vivek Goyal wrote:
>>> On Thu, Sep 02, 2021 at 11:55:11AM -0700, Casey Schaufler wrote:
>>>> On 9/2/2021 10:42 AM, Vivek Goyal wrote:
>>>>> On Thu, Sep 02, 2021 at 01:05:01PM -0400, Vivek Goyal wrote:
>>>>>> On Thu, Sep 02, 2021 at 08:43:50AM -0700, Casey Schaufler wrote:
>>>>>>> On 9/2/2021 8:22 AM, Vivek Goyal wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> This is V3 of the patch. Previous versions were posted here.
>>>>>>>>
>>>>>>>> v2:
>>>>>>>> https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
>>>>>>>> v1:
>>>>>>>> https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
>>>>>>>> +m/
>>>>>>>>
>>>>>>>> Changes since v2
>>>>>>>> ----------------
>>>>>>>> - Do not call inode_permission() for special files as file mode bits
>>>>>>>>   on these files represent permissions to read/write from/to device
>>>>>>>>   and not necessarily permission to read/write xattrs. In this case
>>>>>>>>   now user.* extended xattrs can be read/written on special files
>>>>>>>>   as long as caller is owner of file or has CAP_FOWNER.
>>>>>>>>  
>>>>>>>> - Fixed "man xattr". Will post a patch in same thread little later. (J.
>>>>>>>>   Bruce Fields)
>>>>>>>>
>>>>>>>> - Fixed xfstest 062. Changed it to run only on older kernels where
>>>>>>>>   user extended xattrs are not allowed on symlinks/special files. Added
>>>>>>>>   a new replacement test 648 which does exactly what 062. Just that
>>>>>>>>   it is supposed to run on newer kernels where user extended xattrs
>>>>>>>>   are allowed on symlinks and special files. Will post patch in 
>>>>>>>>   same thread (Ted Ts'o).
>>>>>>>>
>>>>>>>> Testing
>>>>>>>> -------
>>>>>>>> - Ran xfstest "./check -g auto" with and without patches and did not
>>>>>>>>   notice any new failures.
>>>>>>>>
>>>>>>>> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
>>>>>>>>   filesystems and it works.
>>>>>>>>  
>>>>>>>> Description
>>>>>>>> ===========
>>>>>>>>
>>>>>>>> Right now we don't allow setting user.* xattrs on symlinks and special
>>>>>>>> files at all. Initially I thought that real reason behind this
>>>>>>>> restriction is quota limitations but from last conversation it seemed
>>>>>>>> that real reason is that permission bits on symlink and special files
>>>>>>>> are special and different from regular files and directories, hence
>>>>>>>> this restriction is in place. (I tested with xfs user quota enabled and
>>>>>>>> quota restrictions kicked in on symlink).
>>>>>>>>
>>>>>>>> This version of patch allows reading/writing user.* xattr on symlink and
>>>>>>>> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
>>>>>>> This part of your project makes perfect sense. There's no good
>>>>>>> security reason that you shouldn't set user.* xattrs on symlinks
>>>>>>> and/or special files.
>>>>>>>
>>>>>>> However, your virtiofs use case is unreasonable.
>>>>>> Ok. So we can merge this patch irrespective of the fact whether virtiofs
>>>>>> should make use of this mechanism or not, right?
>>>> I don't see a security objection. I did see that Andreas Gruenbacher
>>>> <agruenba@redhat.com> has objections to the behavior.
>>>>
>>>>
>>>>>>>> Who wants to set user.* xattr on symlink/special files
>>>>>>>> -----------------------------------------------------
>>>>>>>> I have primarily two users at this point of time.
>>>>>>>>
>>>>>>>> - virtiofs daemon.
>>>>>>>>
>>>>>>>> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
>>>>>>>>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
>>>>>>>>   should benefit from this change as well.
>>>>>>>>
>>>>>>>> Why virtiofsd wants to set user.* xattr on symlink/special files
>>>>>>>> ----------------------------------------------------------------
>>>>>>>> In virtiofs, actual file server is virtiosd daemon running on host.
>>>>>>>> There we have a mode where xattrs can be remapped to something else.
>>>>>>>> For example security.selinux can be remapped to
>>>>>>>> user.virtiofsd.securit.selinux on the host.
>>>>>>> As I have stated before, this introduces a breach in security.
>>>>>>> It allows an unprivileged process on the host to manipulate the
>>>>>>> security state of the guest. This is horribly wrong. It is not
>>>>>>> sufficient to claim that the breach requires misconfiguration
>>>>>>> to exploit. Don't do this.
>>>>>> So couple of things.
>>>>>>
>>>>>> - Right now whole virtiofs model is relying on the fact that host
>>>>>>   unpriviliged users don't have access to shared directory. Otherwise
>>>>>>   guest process can simply drop a setuid root binary in shared directory
>>>>>>   and unpriviliged process can execute it and take over host system.
>>>>>>
>>>>>>   So if virtiofs makes use of this mechanism, we are well with-in
>>>>>>   the existing constraints. If users don't follow the constraints,
>>>>>>   bad things can happen.
>>>>>>
>>>>>> - I think Smalley provided a solution for your concern in other thread
>>>>>>   we discussed this issue.
>>>>>>
>>>>>>   https://lore.kernel.org/selinux/CAEjxPJ4411vL3+Ab-J0yrRTmXoEf8pVR3x3CSRgPjfzwiUcDtw@mail.gmail.com/T/#mddea4cec7a68c3ee5e8826d650020361030209d6
>>>>>>
>>>>>>
>>>>>>   "So for example if the host policy says that only virtiofsd can set
>>>>>> attributes on those files, then the guest MAC labels along with all
>>>>>> the other attributes are protected against tampering by any other
>>>>>> process on the host."
>>>> You can't count on SELinux policy to address the issue on a
>>>> system running Smack.
>>>> Or any other user of system.* xattrs,
>>>> be they in the kernel or user space. You can't even count on
>>>> SELinux policy to be correct. virtiofs has to present a "safe"
>>>> situation regardless of how security.* xattrs are used and
>>>> regardless of which, if any, LSMs are configured. You can't
>>>> do that with user.* attributes.
>>> Lets take a step back. Your primary concern with using user.* xattrs
>>> by virtiofsd is that it can be modified by unprivileged users on host.
>>> And our solution to that problem is hide shared directory from
>>> unprivileged users.
>> You really don't see how fragile that is, do you?
> Yes, I am not sure why we are so opposed to the idea of using 
> user.* xattrs for storing the guest security.selinux xattrs by virtiofsd.
> And I am trying to understand that. And this discussion should help.
>
> With virtiofsd, we want to keep all shared directory trees in a
> parent directory which has read/write/search permissions only for root
> so that no unpriviliged process can get to files in shared directory
> and do any of the operations.
>
> For example, /var/lib/containers/storage setup by podman has
> following permissions.
>
> ll -d /var/lib/containers/storage/
> drwx------. 10 root root 4096 Jun 18  2020 /var/lib/containers/storage/
>
> Now I should be able to create /var/lib/containers/storage/shared1
> directory and ask virtiofsd to export "share1" to guest. Unprivileged
> process on host can not open any of the files in shared1 dir, hence
> should not be able to modify any data/metadata associated with the
> file.
>
> If this assumption is correct, then I should be able to use "user.*"
> xattrs without having to worry about unprivileged processes modifying
> security labels of guest/nested-guest.

The only attributes you can really count on to protect an object
are the attributes on the object itself. Path based protections are
not reliable.


>> How a single
>> errant call to rename(), chmod() or chown() on the host can expose
>> the entire guest to exploitation. That's not even getting into
>> the bag of mount() tricks.
> I am relying on unpriviliged processes not having permissions to
> read/search in shared directory. And I guess I have to. Even if
> I use "trusted.*" xattrs, what about file data. Unpriviliged
> processes can modify file data and that's going to be equally
> problematic, isn't it? So why are we so focussed only on
> security label part of it.

We are concentrating on your proposed clever mapping trick.
We are doing so because it won't work, and when it blows up
into a full security bruhaha someone is going to try putting
the blame on the xattr mechanism.

>
> You have mentioned that file data is not necessarily
> that big a problem because UID 1000 on host and UID 1000 in guest
> should be treated same. But I am not sure we can do that. In kata
> container model, guest images are untrusted. So they can simply cook
> up UID 1000 or UID 0 or any other UID. Now there can be use
> cases where we are ready to trust guest and treat UID 1000
> on host and guest in same way. But primary model I am focussing
> on is that guest shared directory remains isolated from other
> users on host.

I don't have the time just now to examine the rest of virtiofs.
I am quite afraid that you are making dangerous assumptions on
a number of things. It sure doesn't sound like you've thought
through all of the implications of sharing between the host and
guest.

>>> In addition to that, LSMs on host can block setting "user.*" xattrs by
>>> virtiofsd domain only for additional protection.
>> Try thinking outside the SELinux box briefly, if you possibly can.
>> An LSM that implements just Bell & LaPadula isn't going to have a
>> "virtiofs domain". Neither is a Smack "3 domain" system. Smack doesn't
>> distinguish writing user xattrs from writing other file attributes
>> in policy. Your argument requires a fine grained policy a'la SELinux.
>> And an application specific SELinux policy at that.
> Ok, so does we have to have capability for every LSM to block write
> to user xattr. I mean in above example, virtiofsd is relying on DAC so that
> unprivileged processes can't modify user xattr labels. If we were to
> use "trusted.*" xattr then we are relying on CAP_SYS_ADMIN.
>
> IOW, it will be nice if one or more LSMs can provide mechanism fine
> grained enough to block write to user xattr by unwanted application
> and that provides extra level of security. But should it be mandatory?

No. The behavior of user xattrs is just fine the way it is.
It works as designed. 

>
>>>  If LSMs are not configured,
>>> then hiding the directory is the solution.
>> It's not a solution at all. It's wishful thinking that
>> some admin is going to do absolutely everything right, will
>> never make a mistake and will never, ever, read the mount(2)
>> man page.
> I agree that its easy to make mistakes. But making mistakes is already
> disastrous. So lets say and admin makes mistake and unprivileged
> processes can open/read/write files in shared directory on host.
> Assume I am using "trusted" xattrs to store guest's security labels.
>
> Now file's data can be modified on host in unwated way and be very
> problematic.
>
> Guest can drop a setuid root binary in shared directory and unprivileged
> process on host can run it and take over the system.
>
> Isn't that even bigger problem. So to me making sure shared directory
> is not reachable by unprivileged processes is absolute must requirement
> for virtiofsd (when running as root). If we break that assumption,
> we already have much bigger problems.

I can't help but think you probably do already have much bigger problems.

>>> So why that's not a solution and only relying on CAP_SYS_ADMIN is the
>>> solution. I don't understand that part.
>> It comes back to your design, which is fundamentally flawed. You
>> can't store system security information in an attribute that can
>> be manipulated by untrusted entities.
> You are assuming untrusted entities can have access to the shared dir. But
> assumption in this model is, shared directory is not reachable by
> unprivileged entities. If we break that requirement, there are much
> bigger issues to deal with then just security attributes.

That's a bad assumption. You have all sorts of issues.

>
>> That's why we have system.*
>> xattrs. You want to have an attribute on the host that maps to a
>> security attribute on the guest. The host has to protect the attribute
>> on the guest with mechanisms of comparable strength as the guest's
>> mechanisms. Otherwise you can't trust the guest with host data.
> Ok, I understand your desire that security xattrs as seen by guest kernel
> should be protected by something stronger than simply user xattr.
>
>> It's a real shame that CAP_SYS_ADMIN is so scary. The capability
>> mechanism as implemented today won't scale to the hundreds of individual
>> capabilities it would need to break CAP_SYS_ADMIN up. Maybe someday.
>> I'm not convinced that there isn't a way to accomplish what you're
>> trying to do without privilege, but this isn't it, and I don't know
>> what is. Sorry.
>>
>>> Also if directory is not hidden, unprivileged users can change file
>>> data and other metadata.
>> I assumed that you've taken that into account. Are you saying that
>> isn't going to be done correctly either?
> I am relying on shared directory not accessible to unprivileged processes.
> If that assumption is broken, I guess,  all bets are off. Until and unless
> one designs SELinux (or other LSM) policy in such a way so that
> only virtiofsd can read/write to these shared directories.
>
> I think Dan Walsh has got SELinux policy written for atleast kata
> containers case.
>
> So I guess we will rely on MAC to block unwanted access if users
> make a mistake? Is that the idea? I guess that's why you want
> a stronger mechansim to store guest security xattrs on host. If
> users make a mistake then we have a fallback path?
>
>>>  Why that's not a concern and why there is
>>> so much of focus only security xattr.
>> As with an NFS mount, the assumption is that UID 567 (or its magically
>> mapped equivalent) has the same access rights on both the server/host
>> and client/guest. I'm not worried about the mode bits because they are
>> presented consistently on both machines. If, on the other hand, an
>> attribute used to determine access is security.esprit on the guest and
>> user.security.esprit on the host, the unprivileged user on the host
>> can defeat the privilege requirements on the guest. That's why.
> Hmm..., so if a user id 1000 inside guest can't modify security
> xattrs then same user on host should be allowed to bypass that
> by modifying user xattr. I agree with that.
>
> Just that I am relying on shared directory not being accessible
> to uid 1000 on host and you don't want to rely on that because
> users can easily make mistake. And that's why want to stronger
> form of mechanism to store security xattrs.
>
>>>  If you were to block modification
>>> of file then you will have rely on LSMs.
>> No. We're talking about the semantics of the xattr namespaces.
>> LSMs can further constrain access to xattrs, but the basic rules
>> of access to the user.* and security.* attributes are different
>> in any case. This is by design.
>>
>>>  And if LSMs are not configured,
>>> then we will rely on shared directory not being visible.
>> LSMs are not the problem. LSMs use security.* xattrs, which is why
>> they come up in the discussion.
>>
>>> Can you please help me understand why hiding shared directory from
>>> unprivileged users is not a solution
>> Maybe you can describe the mechanism you use to "hide" a shared directory
>> on the host. If the filesystem is mounted on the host it seems unlikely
>> that you can provide a convincing argument for sufficient protection.
> I am relying on changing direcotry permissions to allow read/write/search
> permission to root only.

Then you have a real mess of problems coming your way. Sorry.

>
> Thanks
> Vivek
>
Sergio Lopez Pascual Sept. 6, 2021, 7:45 a.m. UTC | #12
On Thu, Sep 02, 2021 at 03:34:17PM -0700, Casey Schaufler wrote:
> On 9/2/2021 1:06 PM, Vivek Goyal wrote:
> > On Thu, Sep 02, 2021 at 11:55:11AM -0700, Casey Schaufler wrote:
> >> On 9/2/2021 10:42 AM, Vivek Goyal wrote:
> >>> On Thu, Sep 02, 2021 at 01:05:01PM -0400, Vivek Goyal wrote:
> >>>> On Thu, Sep 02, 2021 at 08:43:50AM -0700, Casey Schaufler wrote:
> >>>>> On 9/2/2021 8:22 AM, Vivek Goyal wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> This is V3 of the patch. Previous versions were posted here.
> >>>>>>
> >>>>>> v2:
> >>>>>> https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> >>>>>> v1:
> >>>>>> https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.co
> >>>>>> +m/
> >>>>>>
> >>>>>> Changes since v2
> >>>>>> ----------------
> >>>>>> - Do not call inode_permission() for special files as file mode bits
> >>>>>>   on these files represent permissions to read/write from/to device
> >>>>>>   and not necessarily permission to read/write xattrs. In this case
> >>>>>>   now user.* extended xattrs can be read/written on special files
> >>>>>>   as long as caller is owner of file or has CAP_FOWNER.
> >>>>>>  
> >>>>>> - Fixed "man xattr". Will post a patch in same thread little later. (J.
> >>>>>>   Bruce Fields)
> >>>>>>
> >>>>>> - Fixed xfstest 062. Changed it to run only on older kernels where
> >>>>>>   user extended xattrs are not allowed on symlinks/special files. Added
> >>>>>>   a new replacement test 648 which does exactly what 062. Just that
> >>>>>>   it is supposed to run on newer kernels where user extended xattrs
> >>>>>>   are allowed on symlinks and special files. Will post patch in 
> >>>>>>   same thread (Ted Ts'o).
> >>>>>>
> >>>>>> Testing
> >>>>>> -------
> >>>>>> - Ran xfstest "./check -g auto" with and without patches and did not
> >>>>>>   notice any new failures.
> >>>>>>
> >>>>>> - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
> >>>>>>   filesystems and it works.
> >>>>>>  
> >>>>>> Description
> >>>>>> ===========
> >>>>>>
> >>>>>> Right now we don't allow setting user.* xattrs on symlinks and special
> >>>>>> files at all. Initially I thought that real reason behind this
> >>>>>> restriction is quota limitations but from last conversation it seemed
> >>>>>> that real reason is that permission bits on symlink and special files
> >>>>>> are special and different from regular files and directories, hence
> >>>>>> this restriction is in place. (I tested with xfs user quota enabled and
> >>>>>> quota restrictions kicked in on symlink).
> >>>>>>
> >>>>>> This version of patch allows reading/writing user.* xattr on symlink and
> >>>>>> special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
> >>>>> This part of your project makes perfect sense. There's no good
> >>>>> security reason that you shouldn't set user.* xattrs on symlinks
> >>>>> and/or special files.
> >>>>>
> >>>>> However, your virtiofs use case is unreasonable.
> >>>> Ok. So we can merge this patch irrespective of the fact whether virtiofs
> >>>> should make use of this mechanism or not, right?
> >> I don't see a security objection. I did see that Andreas Gruenbacher
> >> <agruenba@redhat.com> has objections to the behavior.
> >>
> >>
> >>>>>> Who wants to set user.* xattr on symlink/special files
> >>>>>> -----------------------------------------------------
> >>>>>> I have primarily two users at this point of time.
> >>>>>>
> >>>>>> - virtiofs daemon.
> >>>>>>
> >>>>>> - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
> >>>>>>   fuse-overlay as well and he ran into similar issue. So fuse-overlay
> >>>>>>   should benefit from this change as well.
> >>>>>>
> >>>>>> Why virtiofsd wants to set user.* xattr on symlink/special files
> >>>>>> ----------------------------------------------------------------
> >>>>>> In virtiofs, actual file server is virtiosd daemon running on host.
> >>>>>> There we have a mode where xattrs can be remapped to something else.
> >>>>>> For example security.selinux can be remapped to
> >>>>>> user.virtiofsd.securit.selinux on the host.
> >>>>> As I have stated before, this introduces a breach in security.
> >>>>> It allows an unprivileged process on the host to manipulate the
> >>>>> security state of the guest. This is horribly wrong. It is not
> >>>>> sufficient to claim that the breach requires misconfiguration
> >>>>> to exploit. Don't do this.
> >>>> So couple of things.
> >>>>
> >>>> - Right now whole virtiofs model is relying on the fact that host
> >>>>   unpriviliged users don't have access to shared directory. Otherwise
> >>>>   guest process can simply drop a setuid root binary in shared directory
> >>>>   and unpriviliged process can execute it and take over host system.
> >>>>
> >>>>   So if virtiofs makes use of this mechanism, we are well with-in
> >>>>   the existing constraints. If users don't follow the constraints,
> >>>>   bad things can happen.
> >>>>
> >>>> - I think Smalley provided a solution for your concern in other thread
> >>>>   we discussed this issue.
> >>>>
> >>>>   https://lore.kernel.org/selinux/CAEjxPJ4411vL3+Ab-J0yrRTmXoEf8pVR3x3CSRgPjfzwiUcDtw@mail.gmail.com/T/#mddea4cec7a68c3ee5e8826d650020361030209d6
> >>>>
> >>>>
> >>>>   "So for example if the host policy says that only virtiofsd can set
> >>>> attributes on those files, then the guest MAC labels along with all
> >>>> the other attributes are protected against tampering by any other
> >>>> process on the host."
> >> You can't count on SELinux policy to address the issue on a
> >> system running Smack.
> >> Or any other user of system.* xattrs,
> >> be they in the kernel or user space. You can't even count on
> >> SELinux policy to be correct. virtiofs has to present a "safe"
> >> situation regardless of how security.* xattrs are used and
> >> regardless of which, if any, LSMs are configured. You can't
> >> do that with user.* attributes.
> > Lets take a step back. Your primary concern with using user.* xattrs
> > by virtiofsd is that it can be modified by unprivileged users on host.
> > And our solution to that problem is hide shared directory from
> > unprivileged users.
> 
> You really don't see how fragile that is, do you? How a single
> errant call to rename(), chmod() or chown() on the host can expose
> the entire guest to exploitation. That's not even getting into
> the bag of mount() tricks.
> 
> 
> > In addition to that, LSMs on host can block setting "user.*" xattrs by
> > virtiofsd domain only for additional protection.
> 
> Try thinking outside the SELinux box briefly, if you possibly can.
> An LSM that implements just Bell & LaPadula isn't going to have a
> "virtiofs domain". Neither is a Smack "3 domain" system. Smack doesn't
> distinguish writing user xattrs from writing other file attributes
> in policy. Your argument requires a fine grained policy a'la SELinux.
> And an application specific SELinux policy at that.
> 
> >  If LSMs are not configured,
> > then hiding the directory is the solution.
> 
> It's not a solution at all. It's wishful thinking that
> some admin is going to do absolutely everything right, will
> never make a mistake and will never, ever, read the mount(2)
> man page.

It's not worse than hoping that every admin is going to set up the
right permissions on a file backing the disk image of a Virtual
Machine.

And I'm no simply justifying one bad thing with another thing that's
even worse, I'm just trying to realign our expectations with the
current threat model of Virtualization in Linux. The host is protected
against the guest, but the guest is *not* implicitly protected against
the host. Everything depends on the host's admin good faith and even
better security practices.

This is the very reason why Virtualization-based Confidential
Computing technologies such as AMD SEV and Intel TDX are attracting so
much attention lately, as they aim to close the gap and also protect
guest against the host (as a necessary step to build trust on its
contents and behavior). But this is outside virtio-fs's scope.

Sergio.
Dr. David Alan Gilbert Sept. 6, 2021, 2:39 p.m. UTC | #13
* Andreas Gruenbacher (agruenba@redhat.com) wrote:
> Hi,
> 
> On Thu, Sep 2, 2021 at 5:22 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > This is V3 of the patch. Previous versions were posted here.
> >
> > v2: https://lore.kernel.org/linux-fsdevel/20210708175738.360757-1-vgoyal@redhat.com/
> > v1: https://lore.kernel.org/linux-fsdevel/20210625191229.1752531-1-vgoyal@redhat.com/
> >
> > Changes since v2
> > ----------------
> > - Do not call inode_permission() for special files as file mode bits
> >   on these files represent permissions to read/write from/to device
> >   and not necessarily permission to read/write xattrs. In this case
> >   now user.* extended xattrs can be read/written on special files
> >   as long as caller is owner of file or has CAP_FOWNER.
> >
> > - Fixed "man xattr". Will post a patch in same thread little later. (J.
> >   Bruce Fields)
> >
> > - Fixed xfstest 062. Changed it to run only on older kernels where
> >   user extended xattrs are not allowed on symlinks/special files. Added
> >   a new replacement test 648 which does exactly what 062. Just that
> >   it is supposed to run on newer kernels where user extended xattrs
> >   are allowed on symlinks and special files. Will post patch in
> >   same thread (Ted Ts'o).
> >
> > Testing
> > -------
> > - Ran xfstest "./check -g auto" with and without patches and did not
> >   notice any new failures.
> >
> > - Tested setting "user.*" xattr with ext4/xfs/btrfs/overlay/nfs
> >   filesystems and it works.
> >
> > Description
> > ===========
> >
> > Right now we don't allow setting user.* xattrs on symlinks and special
> > files at all. Initially I thought that real reason behind this
> > restriction is quota limitations but from last conversation it seemed
> > that real reason is that permission bits on symlink and special files
> > are special and different from regular files and directories, hence
> > this restriction is in place. (I tested with xfs user quota enabled and
> > quota restrictions kicked in on symlink).
> >
> > This version of patch allows reading/writing user.* xattr on symlink and
> > special files if caller is owner or priviliged (has CAP_FOWNER) w.r.t inode.
> 
> the idea behind user.* xattrs is that they behave similar to file
> contents as far as permissions go. It follows from that that symlinks
> and special files cannot have user.* xattrs. This has been the model
> for many years now and applications may be expecting these semantics,
> so we cannot simply change the behavior. So NACK from me.
> 
> > Who wants to set user.* xattr on symlink/special files
> > -----------------------------------------------------
> > I have primarily two users at this point of time.
> >
> > - virtiofs daemon.
> >
> > - fuse-overlay. Giuseppe, seems to set user.* xattr attrs on unpriviliged
> >   fuse-overlay as well and he ran into similar issue. So fuse-overlay
> >   should benefit from this change as well.
> >
> > Why virtiofsd wants to set user.* xattr on symlink/special files
> > ----------------------------------------------------------------
> > In virtiofs, actual file server is virtiosd daemon running on host.
> > There we have a mode where xattrs can be remapped to something else.
> > For example security.selinux can be remapped to
> > user.virtiofsd.securit.selinux on the host.
> >
> > This remapping is useful when SELinux is enabled in guest and virtiofs
> > as being used as rootfs. Guest and host SELinux policy might not match
> > and host policy might deny security.selinux xattr setting by guest
> > onto host. Or host might have SELinux disabled and in that case to
> > be able to set security.selinux xattr, virtiofsd will need to have
> > CAP_SYS_ADMIN (which we are trying to avoid). Being able to remap
> > guest security.selinux (or other xattrs) on host to something else
> > is also better from security point of view.
> >
> > But when we try this, we noticed that SELinux relabeling in guest
> > is failing on some symlinks. When I debugged a little more, I
> > came to know that "user.*" xattrs are not allowed on symlinks
> > or special files.
> >
> > So if we allow owner (or CAP_FOWNER) to set user.* xattr, it will
> > allow virtiofs to arbitrarily remap guests's xattrs to something
> > else on host and that solves this SELinux issue nicely and provides
> > two SELinux policies (host and guest) to co-exist nicely without
> > interfering with each other.
> 
> The fact that user.* xattrs don't work in this remapping scenario
> should have told you that you're doing things wrong; the user.*
> namespace seriously was never meant to be abused in this way.
> 
> You may be able to get away with using trusted.* xattrs which support
> roughly the kind of daemon use I think you're talking about here, but
> I'm not sure selinux will be happy with labels that aren't fully under
> its own control. I really wonder why this wasn't obvious enough.

It was; however in our use case it wasn't an issue in general, because
the selinux instance that was setting the labels was inside an untrusted
guest, as such it's labels on the host are themselves untrusted, and
hence user. made some sense to the host - until we found out the
restrictons on user. the hard way.

The mapping code we have doesn't explicitly set user. - it's an
arbitrary remapper that can map to anything you like, trusted. whatever,
but user. feels (to us) like it's right for an untrusted guest.

IMHO the real problem here is that the user/trusted/system/security
'namespaces' are arbitrary hacks rather than a proper namespacing
mechanism that allows you to create new (nested) namespaces and associate
permissions with each one.

Each one carries with it some arbitrary baggage (trusted not working on
NFS, user. having the special rules on symlinks etc).

Then every fs or application that trips over these arbitrary limits adds
some hack to work around them in a different way to every other fs or
app that's doing the same thing; (see 9p, overlayfs, fuse-overlayfs,
crosvm etc etc all that do some level of renaming)

What we really need is a namespace where you can do anything you like,
but it's then limited by the security modules, so that I could allow
user.virtiofsd.guest1 to be able to set labels on symlinks for example.

Dave

> Thanks,
> Andreas
> 
> > Thanks
> > Vivek
> >
> > Vivek Goyal (1):
> >   xattr: Allow user.* xattr on symlink and special files
> >
> >  fs/xattr.c | 23 ++++++++++++++++++-----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > --
> > 2.31.1
> >
>
Dr. David Alan Gilbert Sept. 6, 2021, 2:55 p.m. UTC | #14
* Casey Schaufler (casey@schaufler-ca.com) wrote:
> On 9/2/2021 1:06 PM, Vivek Goyal wrote:

> >  If LSMs are not configured,
> > then hiding the directory is the solution.
> 
> It's not a solution at all. It's wishful thinking that
> some admin is going to do absolutely everything right, will
> never make a mistake and will never, ever, read the mount(2)
> man page.

That is why we run our virtiofsd with a sandbox setup and seccomp; and
frankly anything we can or could turn on we would.

> > So why that's not a solution and only relying on CAP_SYS_ADMIN is the
> > solution. I don't understand that part.
> 
> It comes back to your design, which is fundamentally flawed. You
> can't store system security information in an attribute that can
> be manipulated by untrusted entities. That's why we have system.*
> xattrs. You want to have an attribute on the host that maps to a
> security attribute on the guest. The host has to protect the attribute
> on the guest with mechanisms of comparable strength as the guest's
> mechanisms.

Can you just explain this line to me a bit more: 
> Otherwise you can't trust the guest with host data.

Note we're not trying to trust the guest with the host data here;
we're trying to allow the guest to store the data on the host, while
trusting the host.

> 
> It's a real shame that CAP_SYS_ADMIN is so scary. The capability
> mechanism as implemented today won't scale to the hundreds of individual
> capabilities it would need to break CAP_SYS_ADMIN up. Maybe someday.
> I'm not convinced that there isn't a way to accomplish what you're
> trying to do without privilege, but this isn't it, and I don't know
> what is. Sorry.
> 
> > Also if directory is not hidden, unprivileged users can change file
> > data and other metadata.
> 
> I assumed that you've taken that into account. Are you saying that
> isn't going to be done correctly either?
> 
> >  Why that's not a concern and why there is
> > so much of focus only security xattr.
> 
> As with an NFS mount, the assumption is that UID 567 (or its magically
> mapped equivalent) has the same access rights on both the server/host
> and client/guest. I'm not worried about the mode bits because they are
> presented consistently on both machines. If, on the other hand, an
> attribute used to determine access is security.esprit on the guest and
> user.security.esprit on the host, the unprivileged user on the host
> can defeat the privilege requirements on the guest. That's why.

We're OK with that; remember that the host can do wth it likes to the
guest anyway - it can just go in and poke at the guests RAM if it wants
to do something evil to the guest.
We wouldn't suggest using a scheme like this once you have
encrypted/protected guest RAM for example (SEV/TDX etc)

> >  If you were to block modification
> > of file then you will have rely on LSMs.
> 
> No. We're talking about the semantics of the xattr namespaces.
> LSMs can further constrain access to xattrs, but the basic rules
> of access to the user.* and security.* attributes are different
> in any case. This is by design.

I'm happy if you can suggest somewhere else to store the guests xattr
data other than in one of the hosts xattr's - the challenge is doing
that in a non-racy way, and making sure that the xattr's never get
associated with the wrong file as seen by a guest.

> >  And if LSMs are not configured,
> > then we will rely on shared directory not being visible.
> 
> LSMs are not the problem. LSMs use security.* xattrs, which is why
> they come up in the discussion.
> 
> > Can you please help me understand why hiding shared directory from
> > unprivileged users is not a solution
> 
> Maybe you can describe the mechanism you use to "hide" a shared directory
> on the host. If the filesystem is mounted on the host it seems unlikely
> that you can provide a convincing argument for sufficient protection.

Why? What can a guests fs mounted on the host, under one of the
directories that's already typically used for container fs's do - it's
already what fileservers, and existing container systems do.

Dave



> >  (With both LSMs configured or
> > not configured on host). That's a requirement for virtiofs anyway. 
> > And if we agree on that, then I don't see why using "user.*" xattrs
> > for storing guest sercurity attributes is a problem.
> >
> > Thanks
> > Vivek
> >
>
Miklos Szeredi Sept. 6, 2021, 2:56 p.m. UTC | #15
On Mon, 6 Sept 2021 at 16:39, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:

> IMHO the real problem here is that the user/trusted/system/security
> 'namespaces' are arbitrary hacks rather than a proper namespacing
> mechanism that allows you to create new (nested) namespaces and associate
> permissions with each one.

Indeed.

This is what Eric Biederman suggested at some point for supporting
trusted xattrs within a user namespace:

| For trusted xattrs I think it makes sense in principle.   The namespace
| would probably become something like "trusted<ns-root-uid>.".

Theory sounds simple enough.  Anyone interested in looking at the details?

Thanks,
Miklos
Vivek Goyal Sept. 7, 2021, 9:40 p.m. UTC | #16
On Mon, Sep 06, 2021 at 04:56:44PM +0200, Miklos Szeredi wrote:
> On Mon, 6 Sept 2021 at 16:39, Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> 
> > IMHO the real problem here is that the user/trusted/system/security
> > 'namespaces' are arbitrary hacks rather than a proper namespacing
> > mechanism that allows you to create new (nested) namespaces and associate
> > permissions with each one.
> 
> Indeed.
> 
> This is what Eric Biederman suggested at some point for supporting
> trusted xattrs within a user namespace:
> 
> | For trusted xattrs I think it makes sense in principle.   The namespace
> | would probably become something like "trusted<ns-root-uid>.".
> 
> Theory sounds simple enough.  Anyone interested in looking at the details?

So this namespaced trusted.* xattr domain will basically avoid the need
to have CAP_SYS_ADMIN in init_user_ns, IIUC.  I guess this is better
than giving CAP_SYS_ADMIN in init_user_ns.

Vivek
Miklos Szeredi Sept. 8, 2021, 7:37 a.m. UTC | #17
On Tue, 7 Sept 2021 at 23:40, Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Mon, Sep 06, 2021 at 04:56:44PM +0200, Miklos Szeredi wrote:
> > On Mon, 6 Sept 2021 at 16:39, Dr. David Alan Gilbert
> > <dgilbert@redhat.com> wrote:
> >
> > > IMHO the real problem here is that the user/trusted/system/security
> > > 'namespaces' are arbitrary hacks rather than a proper namespacing
> > > mechanism that allows you to create new (nested) namespaces and associate
> > > permissions with each one.
> >
> > Indeed.
> >
> > This is what Eric Biederman suggested at some point for supporting
> > trusted xattrs within a user namespace:
> >
> > | For trusted xattrs I think it makes sense in principle.   The namespace
> > | would probably become something like "trusted<ns-root-uid>.".
> >
> > Theory sounds simple enough.  Anyone interested in looking at the details?
>
> So this namespaced trusted.* xattr domain will basically avoid the need
> to have CAP_SYS_ADMIN in init_user_ns, IIUC.  I guess this is better
> than giving CAP_SYS_ADMIN in init_user_ns.

That's the objective, yes.  I think the trick is getting filesystems
to store yet another xattr type.

Thanks,
Miklos
Eric W. Biederman Sept. 8, 2021, 2:20 p.m. UTC | #18
Miklos Szeredi <miklos@szeredi.hu> writes:

> On Tue, 7 Sept 2021 at 23:40, Vivek Goyal <vgoyal@redhat.com> wrote:
>>
>> On Mon, Sep 06, 2021 at 04:56:44PM +0200, Miklos Szeredi wrote:
>> > On Mon, 6 Sept 2021 at 16:39, Dr. David Alan Gilbert
>> > <dgilbert@redhat.com> wrote:
>> >
>> > > IMHO the real problem here is that the user/trusted/system/security
>> > > 'namespaces' are arbitrary hacks rather than a proper namespacing
>> > > mechanism that allows you to create new (nested) namespaces and associate
>> > > permissions with each one.
>> >
>> > Indeed.
>> >
>> > This is what Eric Biederman suggested at some point for supporting
>> > trusted xattrs within a user namespace:
>> >
>> > | For trusted xattrs I think it makes sense in principle.   The namespace
>> > | would probably become something like "trusted<ns-root-uid>.".
>> >
>> > Theory sounds simple enough.  Anyone interested in looking at the details?
>>
>> So this namespaced trusted.* xattr domain will basically avoid the need
>> to have CAP_SYS_ADMIN in init_user_ns, IIUC.  I guess this is better
>> than giving CAP_SYS_ADMIN in init_user_ns.
>
> That's the objective, yes.  I think the trick is getting filesystems
> to store yet another xattr type.

Using the uid of the root user of a user namespace is probably the best
idea we have so far for identifying a user namespace in persistent
on-disk meta-data.  We ran into a little trouble using that idea
for file capabilities.

The key problem was there are corner cases where some nested user
namespaces have the same root user id as their parent namespaces.  This
has the potential to allow privilege escalation if the creator of the
user namespace does not have sufficient capabilities.

The solution we adopted can be seen in db2e718a4798 ("capabilities:
require CAP_SETFCAP to map uid 0").

That solution is basically not allowing the creation of user namespaces
that could have problems.  I think use trusted xattrs this way the code
would need to treat CAP_SYS_ADMIN the same way it currently treats
CAP_SETFCAP.

Eric
Casey Schaufler Sept. 13, 2021, 7:05 p.m. UTC | #19
On 9/6/2021 7:55 AM, Dr. David Alan Gilbert wrote:
> * Casey Schaufler (casey@schaufler-ca.com) wrote:
>> On 9/2/2021 1:06 PM, Vivek Goyal wrote:
>>>  If LSMs are not configured,
>>> then hiding the directory is the solution.
>> It's not a solution at all. It's wishful thinking that
>> some admin is going to do absolutely everything right, will
>> never make a mistake and will never, ever, read the mount(2)
>> man page.
> That is why we run our virtiofsd with a sandbox setup and seccomp; and
> frankly anything we can or could turn on we would.

That doesn't address my concern at all. Being able to create an
environment in which a feature can be used safely does not make
the feature safe.


> So why that's not a solution and only relying on CAP_SYS_ADMIN is the
> solution. I don't understand that part.

Sure you do. If you didn't, you wouldn't be so concerned about
requiring CAP_SYS_ADMIN. You're trying hard to avoid taking the
level of responsibility that running with privilege requires.
To do that, you're introducing a massive security hole, a backdoor
into the file system security attributes.

>> It comes back to your design, which is fundamentally flawed. You
>> can't store system security information in an attribute that can
>> be manipulated by untrusted entities. That's why we have system.*
>> xattrs. You want to have an attribute on the host that maps to a
>> security attribute on the guest. The host has to protect the attribute
>> on the guest with mechanisms of comparable strength as the guest's
>> mechanisms.
> Can you just explain this line to me a bit more: 
>> Otherwise you can't trust the guest with host data.
> Note we're not trying to trust the guest with the host data here;
> we're trying to allow the guest to store the data on the host, while
> trusting the host.

But you can't trust the host! You're allowing unprivileged processes
on the host to modify security state of the guest.

>> It's a real shame that CAP_SYS_ADMIN is so scary. The capability
>> mechanism as implemented today won't scale to the hundreds of individual
>> capabilities it would need to break CAP_SYS_ADMIN up. Maybe someday.
>> I'm not convinced that there isn't a way to accomplish what you're
>> trying to do without privilege, but this isn't it, and I don't know
>> what is. Sorry.
>>
>>> Also if directory is not hidden, unprivileged users can change file
>>> data and other metadata.
>> I assumed that you've taken that into account. Are you saying that
>> isn't going to be done correctly either?
>>
>>>  Why that's not a concern and why there is
>>> so much of focus only security xattr.
>> As with an NFS mount, the assumption is that UID 567 (or its magically
>> mapped equivalent) has the same access rights on both the server/host
>> and client/guest. I'm not worried about the mode bits because they are
>> presented consistently on both machines. If, on the other hand, an
>> attribute used to determine access is security.esprit on the guest and
>> user.security.esprit on the host, the unprivileged user on the host
>> can defeat the privilege requirements on the guest. That's why.
> We're OK with that;

I understand that. I  am  not  OK  with  that.

>  remember that the host can do wth it likes to the
> guest anyway

We're not talking about "the host", we're talking about an
unprivileged user on the host.

>  - it can just go in and poke at the guests RAM if it wants
> to do something evil to the guest.
> We wouldn't suggest using a scheme like this once you have
> encrypted/protected guest RAM for example (SEV/TDX etc)
>
>>>  If you were to block modification
>>> of file then you will have rely on LSMs.
>> No. We're talking about the semantics of the xattr namespaces.
>> LSMs can further constrain access to xattrs, but the basic rules
>> of access to the user.* and security.* attributes are different
>> in any case. This is by design.
> I'm happy if you can suggest somewhere else to store the guests xattr
> data other than in one of the hosts xattr's - the challenge is doing
> that in a non-racy way, and making sure that the xattr's never get
> associated with the wrong file as seen by a guest.

I'm sorry, but I've got a bunch of other stuff on my plate.
I've already suggested implementing xattr namespaces a'la user
namespaces, but I understand that is beyond the scope of your
current needs, and has its own set of dragons.

>>>  And if LSMs are not configured,
>>> then we will rely on shared directory not being visible.
>> LSMs are not the problem. LSMs use security.* xattrs, which is why
>> they come up in the discussion.
>>
>>> Can you please help me understand why hiding shared directory from
>>> unprivileged users is not a solution
>> Maybe you can describe the mechanism you use to "hide" a shared directory
>> on the host. If the filesystem is mounted on the host it seems unlikely
>> that you can provide a convincing argument for sufficient protection.
> Why?

Because 99-44/100% of admins out there aren't as skilled at "hiding"
data as you are. Many (I almost said "most". I'm still not sure which.)
of them don't even know how to use mode bits correctly.

>  What can a guests fs mounted on the host, under one of the
> directories that's already typically used for container fs's do - it's
> already what fileservers, and existing container systems do.
>
> Dave
>
>
>
>>>  (With both LSMs configured or
>>> not configured on host). That's a requirement for virtiofs anyway. 
>>> And if we agree on that, then I don't see why using "user.*" xattrs
>>> for storing guest sercurity attributes is a problem.
>>>
>>> Thanks
>>> Vivek
>>>
Vivek Goyal Sept. 14, 2021, 12:51 p.m. UTC | #20
On Mon, Sep 13, 2021 at 12:05:07PM -0700, Casey Schaufler wrote:
> On 9/6/2021 7:55 AM, Dr. David Alan Gilbert wrote:
> > * Casey Schaufler (casey@schaufler-ca.com) wrote:
> >> On 9/2/2021 1:06 PM, Vivek Goyal wrote:
> >>>  If LSMs are not configured,
> >>> then hiding the directory is the solution.
> >> It's not a solution at all. It's wishful thinking that
> >> some admin is going to do absolutely everything right, will
> >> never make a mistake and will never, ever, read the mount(2)
> >> man page.
> > That is why we run our virtiofsd with a sandbox setup and seccomp; and
> > frankly anything we can or could turn on we would.
> 
> That doesn't address my concern at all. Being able to create an
> environment in which a feature can be used safely does not make
> the feature safe.

That's the requirement of virtiofs shared directory. It is a shared
directory, potentially being used by untrusted guest. So it should
not be accessible to unprivileged entities on host.

Same is the requirement for regular containers and that's why
podman (and possibly other container managers), make top level
storage directory only readable and searchable by root, so that
unpriveleged entities on host can not access container root filesystem
data.

I think similar requirements are there for idmapped mounts. One
will shift container images into user namespaces. And that means,
through idmapped mounts one can write files which will actually
show up as owned by root in original directory. And that original
directory should not be accessible to unprivileged users otherwise
user found an easy way to drop root owned files on system and
exectute these.

So more and more use cases are there which are relying on directory
not being accessible to unprivileged users.

I understand it is not going to be easy to get the configurations
right all the time. But orchestration tools can be helpful. container
managers can make sure kata container rootfs are not accessible.
libvirt can probably warn if shared directory is accessible to
unprivileged users. And all that will help in getting configuration
right.

> 
> 
> > So why that's not a solution and only relying on CAP_SYS_ADMIN is the
> > solution. I don't understand that part.
> 
> Sure you do. If you didn't, you wouldn't be so concerned about
> requiring CAP_SYS_ADMIN. You're trying hard to avoid taking the
> level of responsibility that running with privilege requires.

Running with minimal capabilities is always desired. So we want
to do away with CAP_SYS_ADMIN. And this reduces our risk in
case virtiofsd gets compromised.

I thought we had similar reasons that we did not want setuid
root binaries and wanted to give them limited set of capabilites
depending on what they are doing.

> To do that, you're introducing a massive security hole, a backdoor
> into the file system security attributes.

Shared directory is not accessible to unprivileged entities. If
configuration is not right, then it is user's problem and we
need to fix that.

> 
> >> It comes back to your design, which is fundamentally flawed. You
> >> can't store system security information in an attribute that can
> >> be manipulated by untrusted entities. That's why we have system.*
> >> xattrs. You want to have an attribute on the host that maps to a
> >> security attribute on the guest. The host has to protect the attribute
> >> on the guest with mechanisms of comparable strength as the guest's
> >> mechanisms.
> > Can you just explain this line to me a bit more: 
> >> Otherwise you can't trust the guest with host data.
> > Note we're not trying to trust the guest with the host data here;
> > we're trying to allow the guest to store the data on the host, while
> > trusting the host.
> 
> But you can't trust the host! You're allowing unprivileged processes
> on the host to modify security state of the guest.

No we are not. Shared directory should not be accessible to
unpriviliged entities on host.

> 
> >> It's a real shame that CAP_SYS_ADMIN is so scary. The capability
> >> mechanism as implemented today won't scale to the hundreds of individual
> >> capabilities it would need to break CAP_SYS_ADMIN up. Maybe someday.
> >> I'm not convinced that there isn't a way to accomplish what you're
> >> trying to do without privilege, but this isn't it, and I don't know
> >> what is. Sorry.
> >>
> >>> Also if directory is not hidden, unprivileged users can change file
> >>> data and other metadata.
> >> I assumed that you've taken that into account. Are you saying that
> >> isn't going to be done correctly either?
> >>
> >>>  Why that's not a concern and why there is
> >>> so much of focus only security xattr.
> >> As with an NFS mount, the assumption is that UID 567 (or its magically
> >> mapped equivalent) has the same access rights on both the server/host
> >> and client/guest. I'm not worried about the mode bits because they are
> >> presented consistently on both machines. If, on the other hand, an
> >> attribute used to determine access is security.esprit on the guest and
> >> user.security.esprit on the host, the unprivileged user on the host
> >> can defeat the privilege requirements on the guest. That's why.
> > We're OK with that;
> 
> I understand that. I  am  not  OK  with  that.

Unprivileged entities can't modify "user.viritiofs.security.selinux"
as shared directory is not accessible to them.

Sorry, I have to repeat this so many times because you are 
completely ignoring this requirement saying users will get it
wrong. Sure if they do get it wrong, they need to fix it.  But
that does not mean we start giving CAP_SYS_ADMIN to daemon. 

If shared direcotry is accessible to unprivileged entities, game
is already over (setuid root binaries). Being able to modify
security attributes of a file seems like such a minor concern
in comparison.

> 
> >  remember that the host can do wth it likes to the
> > guest anyway
> 
> We're not talking about "the host", we're talking about an
> unprivileged user on the host.
> 
> >  - it can just go in and poke at the guests RAM if it wants
> > to do something evil to the guest.
> > We wouldn't suggest using a scheme like this once you have
> > encrypted/protected guest RAM for example (SEV/TDX etc)
> >
> >>>  If you were to block modification
> >>> of file then you will have rely on LSMs.
> >> No. We're talking about the semantics of the xattr namespaces.
> >> LSMs can further constrain access to xattrs, but the basic rules
> >> of access to the user.* and security.* attributes are different
> >> in any case. This is by design.
> > I'm happy if you can suggest somewhere else to store the guests xattr
> > data other than in one of the hosts xattr's - the challenge is doing
> > that in a non-racy way, and making sure that the xattr's never get
> > associated with the wrong file as seen by a guest.
> 
> I'm sorry, but I've got a bunch of other stuff on my plate.
> I've already suggested implementing xattr namespaces a'la user
> namespaces, but I understand that is beyond the scope of your
> current needs, and has its own set of dragons.
> 
> >>>  And if LSMs are not configured,
> >>> then we will rely on shared directory not being visible.
> >> LSMs are not the problem. LSMs use security.* xattrs, which is why
> >> they come up in the discussion.
> >>
> >>> Can you please help me understand why hiding shared directory from
> >>> unprivileged users is not a solution
> >> Maybe you can describe the mechanism you use to "hide" a shared directory
> >> on the host. If the filesystem is mounted on the host it seems unlikely
> >> that you can provide a convincing argument for sufficient protection.
> > Why?
> 
> Because 99-44/100% of admins out there aren't as skilled at "hiding"
> data as you are. Many (I almost said "most". I'm still not sure which.)
> of them don't even know how to use mode bits correctly.

We need to rely on orchestration tools to this by default.
podman already does that for containers. We probably need to
add something to libvirt too and warn users.

Vivek
Casey Schaufler Sept. 14, 2021, 1:56 p.m. UTC | #21
On 9/14/2021 5:51 AM, Vivek Goyal wrote:
> Sorry, I have to repeat this so many times because you are 
> completely ignoring this requirement saying users will get it
> wrong. 

And I keep repeating myself because you are still wrong.
Configuration won't fix the problem. Tools won't fix the
problem. Your scheme is broken. Don't do it.
Bruce Fields Sept. 14, 2021, 1:59 p.m. UTC | #22
On Tue, Sep 14, 2021 at 8:52 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> Same is the requirement for regular containers and that's why
> podman (and possibly other container managers), make top level
> storage directory only readable and searchable by root, so that
> unpriveleged entities on host can not access container root filesystem
> data.

Note--if that directory is on NFS, making it readable and searchable
by root is very weak protection, since it's often possible for an
attacker to guess filehandles and access objects without the need for
directory lookups.

--b.
Vivek Goyal Sept. 14, 2021, 2:32 p.m. UTC | #23
On Tue, Sep 14, 2021 at 09:59:19AM -0400, Bruce Fields wrote:
> On Tue, Sep 14, 2021 at 8:52 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> > Same is the requirement for regular containers and that's why
> > podman (and possibly other container managers), make top level
> > storage directory only readable and searchable by root, so that
> > unpriveleged entities on host can not access container root filesystem
> > data.
> 
> Note--if that directory is on NFS, making it readable and searchable
> by root is very weak protection, since it's often possible for an
> attacker to guess filehandles and access objects without the need for
> directory lookups.

open_by_handle_at() requires CAP_DAC_READ_SEARCH. And if you have
CAP_DAC_READ_SEARCH, you don't need to even guess file handles. You
should be able to read/search through all directories, IIUC.

So how does one make sure that shared directory on host is not
accessible to unprivileged entities. If making directory accessible
to root only is weaker security, what are the options for stronger
security.

Vivek
Bruce Fields Sept. 14, 2021, 3:01 p.m. UTC | #24
On Tue, Sep 14, 2021 at 10:32 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> open_by_handle_at() requires CAP_DAC_READ_SEARCH.

Or some sort of access to the network.  If you can send rpc requests
to the nfs server that appear to be from someone with access to the
export, you can guess filehandles that allow access to objects under
that directory.  You'll need access to particular objects, but you
won't need read or lookup access to the directory.

You can prevent that if you set things up right, but these
filehandle-issues are poorly understood, and people often forget to
take them into account.

--b.

> And if you have
> CAP_DAC_READ_SEARCH, you don't need to even guess file handles. You
> should be able to read/search through all directories, IIUC.
>
> So how does one make sure that shared directory on host is not
> accessible to unprivileged entities. If making directory accessible
> to root only is weaker security, what are the options for stronger
> security.
Dr. Greg Sept. 15, 2021, 4:33 p.m. UTC | #25
On Tue, Sep 14, 2021 at 10:32:13AM -0400, Vivek Goyal wrote:

Good morning, I hope the day is going well for everyone.

> On Tue, Sep 14, 2021 at 09:59:19AM -0400, Bruce Fields wrote:
> > On Tue, Sep 14, 2021 at 8:52 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > Same is the requirement for regular containers and that's why
> > > podman (and possibly other container managers), make top level
> > > storage directory only readable and searchable by root, so that
> > > unpriveleged entities on host can not access container root filesystem
> > > data.
> > 
> > Note--if that directory is on NFS, making it readable and searchable
> > by root is very weak protection, since it's often possible for an
> > attacker to guess filehandles and access objects without the need for
> > directory lookups.

> open_by_handle_at() requires CAP_DAC_READ_SEARCH. And if you have
> CAP_DAC_READ_SEARCH, you don't need to even guess file handles. You
> should be able to read/search through all directories, IIUC.
>
> So how does one make sure that shared directory on host is not
> accessible to unprivileged entities. If making directory accessible
> to root only is weaker security, what are the options for stronger
> security.

I've been watching this thread, with some interest, given what we have
been working on with respect to providing a new security framework
that merges IMA and LSM and external security co-processor technology.

Some observations based on those experiences and this thread.

Casey is an expert on MAC and capability based security systems,
unfortunately for our industry, particularly bog standard system
administrators, a rarefied set of skills.  It may be helpful to
consider his concerns and position on the issues involved in the
framework of the number of systems that have, and blog posts that
recommend, setting 'selinux=0' on the kernel command-line.

I believe the best summary of his position on this issue, is the
notion that placing security labels, even in transitive form in user
accessible attributes, subordinates the security of the guest system,
regardless of the MAC policy it implements, to the DAC based policy on
the host system.

Given that, there are no legitimate security guarantees that are
inferrable based on the guest MAC policy.

A legitimate pundit, could and probably should question, in the face
of container filesystems and virtual machine images, whether any type
of inferrable security guarantees are possible, but that is a question
and argument for another day.

I didn't see any mention of EVM brought up in these discussions, which
may provide some options to improve the security integrity state of
the guest.

The 800 pound gorilla in the corner in all of this, is that inferrable
security guarantees in guests require a certifiable chain of trust
from the creator of the object to the kernel context that is making
the security gating decisions on the object.  A hard to implement and
prove concept in bare metal trusted systems, let alone the myriad of
edge cases lurking in namespaced and virtual environments.

Which, in a nod to the other corner of the ring, may simply mean, with
our current state of deployable technology, you pay your money and
take your chances in these virtual environments.  Which would in turn
support the notion of a minimum security, ie. DAC, based effort.

> Vivek

Have a good remainder of the week.

Dr. Greg

As always,
Dr. Greg Wettstein, Ph.D, Worker      Autonomously self-defensive
Enjellic Systems Development, LLC     IOT platforms and edge devices.
4206 N. 19th Ave.
Fargo, ND  58102
PH: 701-281-1686                      EMAIL: dg@enjellic.com
------------------------------------------------------------------------------
"This place is so screwed up.  It's just like the Titanic, only
 we don't even have a band playing.
                                -- Terrance George Wieland
                                   Resurrection.