| Message ID | 20220202193935.268777-1-vgoyal@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | virtiofsd: Add support for file security context at file creation | expand |
* Vivek Goyal (vgoyal@redhat.com) wrote: > Hi, > > This is V5 of the patches. I posted V4 here. > > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html > > These will allow us to support SELinux with virtiofs. This will send > SELinux context at file creation to server and server can set it on > file. I think that's pretty close; I've got some minor comments I've replied to on the individual patches. I do worry that the number of different paths for each operation is now quite large so hard to test. I also wonder what happens on something other than SELinux. Dave > Changes since V4 > ---------------- > - Parse only known current size of fuse_init_in. This will make sure > that future extension does not break existing code upon header > update. (David Gilbert) > > - Changed order of one of the patch. It is first patch in series. This > will help fix the breakage before header update patch and code remains > git bisectable. (David Gilbert) > > - Changed %lx to %llx at one place. (David Gilbert). > > Thanks > Vivek > > Vivek Goyal (9): > virtiofsd: Fix breakage due to fuse_init_in size change > linux-headers: Update headers to v5.17-rc1 > virtiofsd: Parse extended "struct fuse_init_in" > virtiofsd: Extend size of fuse_conn_info->capable and ->want fields > virtiofsd, fuse_lowlevel.c: Add capability to parse security context > virtiofsd: Move core file creation code in separate function > virtiofsd: Create new file with fscreate set > virtiofsd: Create new file using O_TMPFILE and set security context > virtiofsd: Add an option to enable/disable security label > > docs/tools/virtiofsd.rst | 7 + > include/standard-headers/asm-x86/kvm_para.h | 1 + > include/standard-headers/drm/drm_fourcc.h | 11 + > include/standard-headers/linux/ethtool.h | 1 + > include/standard-headers/linux/fuse.h | 60 ++- > include/standard-headers/linux/pci_regs.h | 142 +++--- > include/standard-headers/linux/virtio_gpio.h | 72 +++ > include/standard-headers/linux/virtio_i2c.h | 47 ++ > include/standard-headers/linux/virtio_iommu.h | 8 +- > .../standard-headers/linux/virtio_pcidev.h | 65 +++ > include/standard-headers/linux/virtio_scmi.h | 24 + > linux-headers/asm-generic/unistd.h | 5 +- > linux-headers/asm-mips/unistd_n32.h | 2 + > linux-headers/asm-mips/unistd_n64.h | 2 + > linux-headers/asm-mips/unistd_o32.h | 2 + > linux-headers/asm-powerpc/unistd_32.h | 2 + > linux-headers/asm-powerpc/unistd_64.h | 2 + > linux-headers/asm-riscv/bitsperlong.h | 14 + > linux-headers/asm-riscv/mman.h | 1 + > linux-headers/asm-riscv/unistd.h | 44 ++ > linux-headers/asm-s390/unistd_32.h | 2 + > linux-headers/asm-s390/unistd_64.h | 2 + > linux-headers/asm-x86/kvm.h | 16 +- > linux-headers/asm-x86/unistd_32.h | 1 + > linux-headers/asm-x86/unistd_64.h | 1 + > linux-headers/asm-x86/unistd_x32.h | 1 + > linux-headers/linux/kvm.h | 17 + > tools/virtiofsd/fuse_common.h | 9 +- > tools/virtiofsd/fuse_i.h | 7 + > tools/virtiofsd/fuse_lowlevel.c | 162 +++++-- > tools/virtiofsd/helper.c | 1 + > tools/virtiofsd/passthrough_ll.c | 414 ++++++++++++++++-- > 32 files changed, 1013 insertions(+), 132 deletions(-) > create mode 100644 include/standard-headers/linux/virtio_gpio.h > create mode 100644 include/standard-headers/linux/virtio_i2c.h > create mode 100644 include/standard-headers/linux/virtio_pcidev.h > create mode 100644 include/standard-headers/linux/virtio_scmi.h > create mode 100644 linux-headers/asm-riscv/bitsperlong.h > create mode 100644 linux-headers/asm-riscv/mman.h > create mode 100644 linux-headers/asm-riscv/unistd.h > > -- > 2.34.1 >
On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote: > Hi, > > This is V5 of the patches. I posted V4 here. > > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html > > These will allow us to support SELinux with virtiofs. This will send > SELinux context at file creation to server and server can set it on > file. I've not entirely figured it out from the code, so easier for me to ask... How is the SELinux labelled stored on the host side ? It is stored directly in the security.* xattr namespace, or is is subject to xattr remapping that virtiofsd already supports. Storing directly means virtiofsd has to run in an essentially unconfined context, to let it do arbitrary changes on security.* xattrs without being blocked by SELinux) and has risk that guest initiated changes can open holes in the host confinement if the exported FS is generally visible to processes on the host. Using remapping lets virtiofsd be strictly isolated by SELinux policy on the host, and ensures that guest context changes can't open up holes in the host. Both are valid use cases, so I'd ultimately expect us to want to support both, but my preference for a "default" behaviour would be remapping. Regards, Daniel
On Mon, Feb 07, 2022 at 01:05:16PM +0000, Daniel P. Berrangé wrote: > On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote: > > Hi, > > > > This is V5 of the patches. I posted V4 here. > > > > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html > > > > These will allow us to support SELinux with virtiofs. This will send > > SELinux context at file creation to server and server can set it on > > file. > > I've not entirely figured it out from the code, so easier for me > to ask... > > How is the SELinux labelled stored on the host side ? It is stored > directly in the security.* xattr namespace, or is is subject to > xattr remapping that virtiofsd already supports. > > Storing directly means virtiofsd has to run in an essentially > unconfined context, to let it do arbitrary changes on security.* > xattrs without being blocked by SELinux) and has risk that guest > initiated changes can open holes in the host confinement if > the exported FS is generally visible to processes on the host. > > > Using remapping lets virtiofsd be strictly isolated by SELinux > policy on the host, and ensures that guest context changes > can't open up holes in the host. > > Both are valid use cases, so I'd ultimately expect us to want > to support both, but my preference for a "default" behaviour > would be remapping. I am expecting users to configure virtiofsd to remap "security.selinux" to "trusted.virtiofsd.security.selinux" and that will allow guest and host security selinux to co-exist and allow separate SELinux policies for guest and host. I agree that my preference for a default behavior is remapping as well. That makes most sense. One downside of mapping to trusted namespace is that it requires CAP_SYS_ADMIN for virtiofsd. Having said that, these patches don't enforce the remapping default. That has to come from the user because it also needs to be given CAP_SYS_ADMIN. So out of box default is no remapping and passthrough SELinux. Thanks Vivek > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
On Mon, Feb 07, 2022 at 08:24:08AM -0500, Vivek Goyal wrote: > On Mon, Feb 07, 2022 at 01:05:16PM +0000, Daniel P. Berrangé wrote: > > On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote: > > > Hi, > > > > > > This is V5 of the patches. I posted V4 here. > > > > > > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html > > > > > > These will allow us to support SELinux with virtiofs. This will send > > > SELinux context at file creation to server and server can set it on > > > file. > > > > I've not entirely figured it out from the code, so easier for me > > to ask... > > > > How is the SELinux labelled stored on the host side ? It is stored > > directly in the security.* xattr namespace, or is is subject to > > xattr remapping that virtiofsd already supports. > > > > Storing directly means virtiofsd has to run in an essentially > > unconfined context, to let it do arbitrary changes on security.* > > xattrs without being blocked by SELinux) and has risk that guest > > initiated changes can open holes in the host confinement if > > the exported FS is generally visible to processes on the host. > > > > > > Using remapping lets virtiofsd be strictly isolated by SELinux > > policy on the host, and ensures that guest context changes > > can't open up holes in the host. > > > > Both are valid use cases, so I'd ultimately expect us to want > > to support both, but my preference for a "default" behaviour > > would be remapping. > > I am expecting users to configure virtiofsd to remap "security.selinux" > to "trusted.virtiofsd.security.selinux" and that will allow guest > and host security selinux to co-exist and allow separate SELinux policies > for guest and host. > > I agree that my preference for a default behavior is remapping as well. > That makes most sense. > > One downside of mapping to trusted namespace is that it requires > CAP_SYS_ADMIN for virtiofsd. > > Having said that, these patches don't enforce the remapping default. That > has to come from the user because it also needs to be given CAP_SYS_ADMIN. > So out of box default is no remapping and passthrough SELinux. Ok, that all makes sense then. My only suggestion then is to put something more explicit in the man page docs to highlight the implications / interaction beteen the new command line options for labelling and the likely need for remapping security.* Regards, Daniel
On Mon, Feb 07, 2022 at 12:49:24PM +0000, Dr. David Alan Gilbert wrote: > * Vivek Goyal (vgoyal@redhat.com) wrote: > > Hi, > > > > This is V5 of the patches. I posted V4 here. > > > > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html > > > > These will allow us to support SELinux with virtiofs. This will send > > SELinux context at file creation to server and server can set it on > > file. > > I think that's pretty close; I've got some minor comments I've replied > to on the individual patches. > > I do worry that the number of different paths for each operation is now > quite large so hard to test. It is indeed many combinations to test. During development, I have made sure to test every path atleast once to make sure it works. > I also wonder what happens on something other than SELinux. As of now this pretty much works only for SELinux. Especially usage of fscreate knob is very specific to SELinux. In some cases, it will work with some other LSM other than SELinux as well. But lets not go there. If we want to support multiple security contexts at some point of time, fuse procotol changes have been written in such a way so that fuse can send mutiple security context and then we will have to modify code to be able to deal with that. In short, for now, this code is pretty much expectin one security context that too SELinux. This is very much in line with ceph and nfs. Vivek > > Dave > > > Changes since V4 > > ---------------- > > - Parse only known current size of fuse_init_in. This will make sure > > that future extension does not break existing code upon header > > update. (David Gilbert) > > > > - Changed order of one of the patch. It is first patch in series. This > > will help fix the breakage before header update patch and code remains > > git bisectable. (David Gilbert) > > > > - Changed %lx to %llx at one place. (David Gilbert). > > > > Thanks > > Vivek > > > > Vivek Goyal (9): > > virtiofsd: Fix breakage due to fuse_init_in size change > > linux-headers: Update headers to v5.17-rc1 > > virtiofsd: Parse extended "struct fuse_init_in" > > virtiofsd: Extend size of fuse_conn_info->capable and ->want fields > > virtiofsd, fuse_lowlevel.c: Add capability to parse security context > > virtiofsd: Move core file creation code in separate function > > virtiofsd: Create new file with fscreate set > > virtiofsd: Create new file using O_TMPFILE and set security context > > virtiofsd: Add an option to enable/disable security label > > > > docs/tools/virtiofsd.rst | 7 + > > include/standard-headers/asm-x86/kvm_para.h | 1 + > > include/standard-headers/drm/drm_fourcc.h | 11 + > > include/standard-headers/linux/ethtool.h | 1 + > > include/standard-headers/linux/fuse.h | 60 ++- > > include/standard-headers/linux/pci_regs.h | 142 +++--- > > include/standard-headers/linux/virtio_gpio.h | 72 +++ > > include/standard-headers/linux/virtio_i2c.h | 47 ++ > > include/standard-headers/linux/virtio_iommu.h | 8 +- > > .../standard-headers/linux/virtio_pcidev.h | 65 +++ > > include/standard-headers/linux/virtio_scmi.h | 24 + > > linux-headers/asm-generic/unistd.h | 5 +- > > linux-headers/asm-mips/unistd_n32.h | 2 + > > linux-headers/asm-mips/unistd_n64.h | 2 + > > linux-headers/asm-mips/unistd_o32.h | 2 + > > linux-headers/asm-powerpc/unistd_32.h | 2 + > > linux-headers/asm-powerpc/unistd_64.h | 2 + > > linux-headers/asm-riscv/bitsperlong.h | 14 + > > linux-headers/asm-riscv/mman.h | 1 + > > linux-headers/asm-riscv/unistd.h | 44 ++ > > linux-headers/asm-s390/unistd_32.h | 2 + > > linux-headers/asm-s390/unistd_64.h | 2 + > > linux-headers/asm-x86/kvm.h | 16 +- > > linux-headers/asm-x86/unistd_32.h | 1 + > > linux-headers/asm-x86/unistd_64.h | 1 + > > linux-headers/asm-x86/unistd_x32.h | 1 + > > linux-headers/linux/kvm.h | 17 + > > tools/virtiofsd/fuse_common.h | 9 +- > > tools/virtiofsd/fuse_i.h | 7 + > > tools/virtiofsd/fuse_lowlevel.c | 162 +++++-- > > tools/virtiofsd/helper.c | 1 + > > tools/virtiofsd/passthrough_ll.c | 414 ++++++++++++++++-- > > 32 files changed, 1013 insertions(+), 132 deletions(-) > > create mode 100644 include/standard-headers/linux/virtio_gpio.h > > create mode 100644 include/standard-headers/linux/virtio_i2c.h > > create mode 100644 include/standard-headers/linux/virtio_pcidev.h > > create mode 100644 include/standard-headers/linux/virtio_scmi.h > > create mode 100644 linux-headers/asm-riscv/bitsperlong.h > > create mode 100644 linux-headers/asm-riscv/mman.h > > create mode 100644 linux-headers/asm-riscv/unistd.h > > > > -- > > 2.34.1 > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
On Mon, Feb 07, 2022 at 01:30:16PM +0000, Daniel P. Berrangé wrote: > On Mon, Feb 07, 2022 at 08:24:08AM -0500, Vivek Goyal wrote: > > On Mon, Feb 07, 2022 at 01:05:16PM +0000, Daniel P. Berrangé wrote: > > > On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote: > > > > Hi, > > > > > > > > This is V5 of the patches. I posted V4 here. > > > > > > > > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html > > > > > > > > These will allow us to support SELinux with virtiofs. This will send > > > > SELinux context at file creation to server and server can set it on > > > > file. > > > > > > I've not entirely figured it out from the code, so easier for me > > > to ask... > > > > > > How is the SELinux labelled stored on the host side ? It is stored > > > directly in the security.* xattr namespace, or is is subject to > > > xattr remapping that virtiofsd already supports. > > > > > > Storing directly means virtiofsd has to run in an essentially > > > unconfined context, to let it do arbitrary changes on security.* > > > xattrs without being blocked by SELinux) and has risk that guest > > > initiated changes can open holes in the host confinement if > > > the exported FS is generally visible to processes on the host. > > > > > > > > > Using remapping lets virtiofsd be strictly isolated by SELinux > > > policy on the host, and ensures that guest context changes > > > can't open up holes in the host. > > > > > > Both are valid use cases, so I'd ultimately expect us to want > > > to support both, but my preference for a "default" behaviour > > > would be remapping. > > > > I am expecting users to configure virtiofsd to remap "security.selinux" > > to "trusted.virtiofsd.security.selinux" and that will allow guest > > and host security selinux to co-exist and allow separate SELinux policies > > for guest and host. > > > > I agree that my preference for a default behavior is remapping as well. > > That makes most sense. > > > > One downside of mapping to trusted namespace is that it requires > > CAP_SYS_ADMIN for virtiofsd. > > > > Having said that, these patches don't enforce the remapping default. That > > has to come from the user because it also needs to be given CAP_SYS_ADMIN. > > So out of box default is no remapping and passthrough SELinux. > > Ok, that all makes sense then. My only suggestion then is to put something > more explicit in the man page docs to highlight the implications / > interaction beteen the new command line options for labelling and the > likely need for remapping security.* Ok, will do. While describing this new command line option, will also mention the likely need of remapping and additional capability and security implication. Or may be I will create a small new section for SELinux in same file. Thanks Vivek
* Vivek Goyal (vgoyal@redhat.com) wrote: > On Mon, Feb 07, 2022 at 12:49:24PM +0000, Dr. David Alan Gilbert wrote: > > * Vivek Goyal (vgoyal@redhat.com) wrote: > > > Hi, > > > > > > This is V5 of the patches. I posted V4 here. > > > > > > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html > > > > > > These will allow us to support SELinux with virtiofs. This will send > > > SELinux context at file creation to server and server can set it on > > > file. > > > > I think that's pretty close; I've got some minor comments I've replied > > to on the individual patches. > > > > I do worry that the number of different paths for each operation is now > > quite large so hard to test. > > It is indeed many combinations to test. During development, I have made > sure to test every path atleast once to make sure it works. > > > I also wonder what happens on something other than SELinux. > > As of now this pretty much works only for SELinux. Especially usage of > fscreate knob is very specific to SELinux. > > In some cases, it will work with some other LSM other than SELinux > as well. But lets not go there. > > If we want to support multiple security contexts at some point of time, > fuse procotol changes have been written in such a way so that fuse > can send mutiple security context and then we will have to modify > code to be able to deal with that. > > In short, for now, this code is pretty much expectin one security > context that too SELinux. This is very much in line with ceph and > nfs. OK, as long as we have some feel for what we'd need to do if something hit that. Dave > Vivek > > > > Dave > > > > > Changes since V4 > > > ---------------- > > > - Parse only known current size of fuse_init_in. This will make sure > > > that future extension does not break existing code upon header > > > update. (David Gilbert) > > > > > > - Changed order of one of the patch. It is first patch in series. This > > > will help fix the breakage before header update patch and code remains > > > git bisectable. (David Gilbert) > > > > > > - Changed %lx to %llx at one place. (David Gilbert). > > > > > > Thanks > > > Vivek > > > > > > Vivek Goyal (9): > > > virtiofsd: Fix breakage due to fuse_init_in size change > > > linux-headers: Update headers to v5.17-rc1 > > > virtiofsd: Parse extended "struct fuse_init_in" > > > virtiofsd: Extend size of fuse_conn_info->capable and ->want fields > > > virtiofsd, fuse_lowlevel.c: Add capability to parse security context > > > virtiofsd: Move core file creation code in separate function > > > virtiofsd: Create new file with fscreate set > > > virtiofsd: Create new file using O_TMPFILE and set security context > > > virtiofsd: Add an option to enable/disable security label > > > > > > docs/tools/virtiofsd.rst | 7 + > > > include/standard-headers/asm-x86/kvm_para.h | 1 + > > > include/standard-headers/drm/drm_fourcc.h | 11 + > > > include/standard-headers/linux/ethtool.h | 1 + > > > include/standard-headers/linux/fuse.h | 60 ++- > > > include/standard-headers/linux/pci_regs.h | 142 +++--- > > > include/standard-headers/linux/virtio_gpio.h | 72 +++ > > > include/standard-headers/linux/virtio_i2c.h | 47 ++ > > > include/standard-headers/linux/virtio_iommu.h | 8 +- > > > .../standard-headers/linux/virtio_pcidev.h | 65 +++ > > > include/standard-headers/linux/virtio_scmi.h | 24 + > > > linux-headers/asm-generic/unistd.h | 5 +- > > > linux-headers/asm-mips/unistd_n32.h | 2 + > > > linux-headers/asm-mips/unistd_n64.h | 2 + > > > linux-headers/asm-mips/unistd_o32.h | 2 + > > > linux-headers/asm-powerpc/unistd_32.h | 2 + > > > linux-headers/asm-powerpc/unistd_64.h | 2 + > > > linux-headers/asm-riscv/bitsperlong.h | 14 + > > > linux-headers/asm-riscv/mman.h | 1 + > > > linux-headers/asm-riscv/unistd.h | 44 ++ > > > linux-headers/asm-s390/unistd_32.h | 2 + > > > linux-headers/asm-s390/unistd_64.h | 2 + > > > linux-headers/asm-x86/kvm.h | 16 +- > > > linux-headers/asm-x86/unistd_32.h | 1 + > > > linux-headers/asm-x86/unistd_64.h | 1 + > > > linux-headers/asm-x86/unistd_x32.h | 1 + > > > linux-headers/linux/kvm.h | 17 + > > > tools/virtiofsd/fuse_common.h | 9 +- > > > tools/virtiofsd/fuse_i.h | 7 + > > > tools/virtiofsd/fuse_lowlevel.c | 162 +++++-- > > > tools/virtiofsd/helper.c | 1 + > > > tools/virtiofsd/passthrough_ll.c | 414 ++++++++++++++++-- > > > 32 files changed, 1013 insertions(+), 132 deletions(-) > > > create mode 100644 include/standard-headers/linux/virtio_gpio.h > > > create mode 100644 include/standard-headers/linux/virtio_i2c.h > > > create mode 100644 include/standard-headers/linux/virtio_pcidev.h > > > create mode 100644 include/standard-headers/linux/virtio_scmi.h > > > create mode 100644 linux-headers/asm-riscv/bitsperlong.h > > > create mode 100644 linux-headers/asm-riscv/mman.h > > > create mode 100644 linux-headers/asm-riscv/unistd.h > > > > > > -- > > > 2.34.1 > > > > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > >
On Mon, Feb 07, 2022 at 01:05:16PM +0000, Daniel P. Berrangé wrote: > On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote: > > Hi, > > > > This is V5 of the patches. I posted V4 here. > > > > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html > > > > These will allow us to support SELinux with virtiofs. This will send > > SELinux context at file creation to server and server can set it on > > file. > > I've not entirely figured it out from the code, so easier for me > to ask... > > How is the SELinux labelled stored on the host side ? It is stored > directly in the security.* xattr namespace, [ CC Dan Walsh ] I just tried to test the mode where I don't do xattr remapping and try to set /proc/pid/attr/fscreate with the context I want to set. It will set security.selinux xattr on host. But write to /proc/pid/attr/fscreate fails if host does not recognize the label sent by guest. I am running virtiofsd with unconfined_t but it still fails because guest is trying to create a file with "test_filesystem_filetranscon_t" and host does not recognize this label. Seeing following in audit logs. type=SELINUX_ERR msg=audit(1644268262.666:8111): op=fscreate invalid_context="unconfined_u:object_r:test_filesystem_filetranscon_t:s0" So if we don't remap xattrs and host has SELinux enabled, then it probably work in very limited circumstances where host and guest policies don't conflict. I guess its like running fedora 34 guest on fedora 34 host. I suspect that this will see very limited use. Though I have put the code in for the sake of completeness. Thanks Vivek > or is is subject to > xattr remapping that virtiofsd already supports. > > Storing directly means virtiofsd has to run in an essentially > unconfined context, to let it do arbitrary changes on security.* > xattrs without being blocked by SELinux) and has risk that guest > initiated changes can open holes in the host confinement if > the exported FS is generally visible to processes on the host. > > > Using remapping lets virtiofsd be strictly isolated by SELinux > policy on the host, and ensures that guest context changes > can't open up holes in the host. > > Both are valid use cases, so I'd ultimately expect us to want > to support both, but my preference for a "default" behaviour > would be remapping. > > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
On 2/7/22 16:19, Vivek Goyal wrote: > On Mon, Feb 07, 2022 at 01:05:16PM +0000, Daniel P. Berrangé wrote: >> On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote: >>> Hi, >>> >>> This is V5 of the patches. I posted V4 here. >>> >>> https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html >>> >>> These will allow us to support SELinux with virtiofs. This will send >>> SELinux context at file creation to server and server can set it on >>> file. >> I've not entirely figured it out from the code, so easier for me >> to ask... >> >> How is the SELinux labelled stored on the host side ? It is stored >> directly in the security.* xattr namespace, > [ CC Dan Walsh ] > > I just tried to test the mode where I don't do xattr remapping and try > to set /proc/pid/attr/fscreate with the context I want to set. It will > set security.selinux xattr on host. > > But write to /proc/pid/attr/fscreate fails if host does not recognize > the label sent by guest. I am running virtiofsd with unconfined_t but > it still fails because guest is trying to create a file with > "test_filesystem_filetranscon_t" and host does not recognize this > label. Seeing following in audit logs. > > type=SELINUX_ERR msg=audit(1644268262.666:8111): op=fscreate invalid_context="unconfined_u:object_r:test_filesystem_filetranscon_t:s0" > > So if we don't remap xattrs and host has SELinux enabled, then it probably > work in very limited circumstances where host and guest policies don't > conflict. I guess its like running fedora 34 guest on fedora 34 host. > I suspect that this will see very limited use. Though I have put the > code in for the sake of completeness. > > Thanks > Vivek > >> or is is subject to >> xattr remapping that virtiofsd already supports. >> >> Storing directly means virtiofsd has to run in an essentially >> unconfined context, to let it do arbitrary changes on security.* >> xattrs without being blocked by SELinux) and has risk that guest >> initiated changes can open holes in the host confinement if >> the exported FS is generally visible to processes on the host. >> >> >> Using remapping lets virtiofsd be strictly isolated by SELinux >> policy on the host, and ensures that guest context changes >> can't open up holes in the host. >> >> Both are valid use cases, so I'd ultimately expect us to want >> to support both, but my preference for a "default" behaviour >> would be remapping. >> >> Regards, >> Daniel >> -- >> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| >> |: https://libvirt.org -o- https://fstop138.berrange.com :| >> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >> I had a bug today, where someone tried to run a --privileged container with RHEL9 which attempted to place a label onto a volume from the RHEL8 host and got a MAC_ADMIN avc and the label was denied. Even unconfined domains are not allowed to place non-understood labels on disk. The problem is when the processes on RHEL8 would look at the labeled file, they would just see it as unlabeled_t.
On Mon, Feb 07, 2022 at 04:19:38PM -0500, Vivek Goyal wrote: > On Mon, Feb 07, 2022 at 01:05:16PM +0000, Daniel P. Berrangé wrote: > > On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote: > > > Hi, > > > > > > This is V5 of the patches. I posted V4 here. > > > > > > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html > > > > > > These will allow us to support SELinux with virtiofs. This will send > > > SELinux context at file creation to server and server can set it on > > > file. > > > > I've not entirely figured it out from the code, so easier for me > > to ask... > > > > How is the SELinux labelled stored on the host side ? It is stored > > directly in the security.* xattr namespace, > > [ CC Dan Walsh ] > > I just tried to test the mode where I don't do xattr remapping and try > to set /proc/pid/attr/fscreate with the context I want to set. It will > set security.selinux xattr on host. > > But write to /proc/pid/attr/fscreate fails if host does not recognize > the label sent by guest. I am running virtiofsd with unconfined_t but > it still fails because guest is trying to create a file with > "test_filesystem_filetranscon_t" and host does not recognize this > label. Seeing following in audit logs. > > type=SELINUX_ERR msg=audit(1644268262.666:8111): op=fscreate invalid_context="unconfined_u:object_r:test_filesystem_filetranscon_t:s0" Yes, that's to be expected if the host policy doesn't know about the label that the guest is using. IOW, non-mapping case is only useful if you have a very good match between host + guest OS policy. This could be useful for an app like Kata because their guest is not a full OS, it is something special purpose and tightly controlled. > So if we don't remap xattrs and host has SELinux enabled, then it probably > work in very limited circumstances where host and guest policies don't > conflict. I guess its like running fedora 34 guest on fedora 34 host. > I suspect that this will see very limited use. Though I have put the > code in for the sake of completeness. For general purpose guest OS virtualization remapping is going to be effectuively mandatory. The non-mapped case only usable when you tightly control the guest OS packages from the host. Regards, Daniel
On Tue, Feb 8, 2022 at 11:44 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > On Mon, Feb 07, 2022 at 04:19:38PM -0500, Vivek Goyal wrote: > > On Mon, Feb 07, 2022 at 01:05:16PM +0000, Daniel P. Berrangé wrote: > > > On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote: > > > > Hi, > > > > > > > > This is V5 of the patches. I posted V4 here. > > > > > > > > > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html > > > > > > > > These will allow us to support SELinux with virtiofs. This will send > > > > SELinux context at file creation to server and server can set it on > > > > file. > > > > > > I've not entirely figured it out from the code, so easier for me > > > to ask... > > > > > > How is the SELinux labelled stored on the host side ? It is stored > > > directly in the security.* xattr namespace, > > > > [ CC Dan Walsh ] > > > > I just tried to test the mode where I don't do xattr remapping and try > > to set /proc/pid/attr/fscreate with the context I want to set. It will > > set security.selinux xattr on host. > > > > But write to /proc/pid/attr/fscreate fails if host does not recognize > > the label sent by guest. I am running virtiofsd with unconfined_t but > > it still fails because guest is trying to create a file with > > "test_filesystem_filetranscon_t" and host does not recognize this > > label. Seeing following in audit logs. > > > > type=SELINUX_ERR msg=audit(1644268262.666:8111): op=fscreate > invalid_context="unconfined_u:object_r:test_filesystem_filetranscon_t:s0" > > Yes, that's to be expected if the host policy doesn't know about the > label that the guest is using. > > IOW, non-mapping case is only useful if you have a very good match > between host + guest OS policy. This could be useful for an app > like Kata because their guest is not a full OS, it is something > special purpose and tightly controlled. > > > So if we don't remap xattrs and host has SELinux enabled, then it > probably > > work in very limited circumstances where host and guest policies don't > > conflict. I guess its like running fedora 34 guest on fedora 34 host. > > I suspect that this will see very limited use. Though I have put the > > code in for the sake of completeness. > > For general purpose guest OS virtualization remapping is going to be > effectuively mandatory. The non-mapped case only usable when you tightly > control the guest OS packages from the host. > If remap is recommended, why not make it mandatory or automatic?, for instance, '-o security_label' either requires '-o xattrmap=' or automatically makes the mapping with the 'trusted' prefix, while '-o security_label=nomap' doesn't, so you can choose whatever you want. (I'm not suggesting the 'nomap' name, I'm terrible choosing names)
On Wed, Feb 09, 2022 at 11:24:29AM +0100, German Maglione wrote: > On Tue, Feb 8, 2022 at 11:44 PM Daniel P. Berrangé <berrange@redhat.com> > wrote: > > > On Mon, Feb 07, 2022 at 04:19:38PM -0500, Vivek Goyal wrote: > > > On Mon, Feb 07, 2022 at 01:05:16PM +0000, Daniel P. Berrangé wrote: > > > > On Wed, Feb 02, 2022 at 02:39:26PM -0500, Vivek Goyal wrote: > > > > > Hi, > > > > > > > > > > This is V5 of the patches. I posted V4 here. > > > > > > > > > > > > https://listman.redhat.com/archives/virtio-fs/2022-January/msg00041.html > > > > > > > > > > These will allow us to support SELinux with virtiofs. This will send > > > > > SELinux context at file creation to server and server can set it on > > > > > file. > > > > > > > > I've not entirely figured it out from the code, so easier for me > > > > to ask... > > > > > > > > How is the SELinux labelled stored on the host side ? It is stored > > > > directly in the security.* xattr namespace, > > > > > > [ CC Dan Walsh ] > > > > > > I just tried to test the mode where I don't do xattr remapping and try > > > to set /proc/pid/attr/fscreate with the context I want to set. It will > > > set security.selinux xattr on host. > > > > > > But write to /proc/pid/attr/fscreate fails if host does not recognize > > > the label sent by guest. I am running virtiofsd with unconfined_t but > > > it still fails because guest is trying to create a file with > > > "test_filesystem_filetranscon_t" and host does not recognize this > > > label. Seeing following in audit logs. > > > > > > type=SELINUX_ERR msg=audit(1644268262.666:8111): op=fscreate > > invalid_context="unconfined_u:object_r:test_filesystem_filetranscon_t:s0" > > > > Yes, that's to be expected if the host policy doesn't know about the > > label that the guest is using. > > > > IOW, non-mapping case is only useful if you have a very good match > > between host + guest OS policy. This could be useful for an app > > like Kata because their guest is not a full OS, it is something > > special purpose and tightly controlled. > > > > > So if we don't remap xattrs and host has SELinux enabled, then it > > probably > > > work in very limited circumstances where host and guest policies don't > > > conflict. I guess its like running fedora 34 guest on fedora 34 host. > > > I suspect that this will see very limited use. Though I have put the > > > code in for the sake of completeness. > > > > For general purpose guest OS virtualization remapping is going to be > > effectuively mandatory. The non-mapped case only usable when you tightly > > control the guest OS packages from the host. > > > > > If remap is recommended, why not make it mandatory or automatic?, > for instance, '-o security_label' either requires '-o xattrmap=' or > automatically makes > the mapping with the 'trusted' prefix, while '-o security_label=nomap' > doesn't, so you > can choose whatever you want. It is a recommended settings but not a mandatory setting. So enforcing any kind of policy will work for some and not for others. We could refine it down the line depending on how it is used and where people find it useful. For now, primary focus is to get basic support patches in the tree. Thanks Vivek > > (I'm not suggesting the 'nomap' name, I'm terrible choosing names) > > -- > German