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