SELinux rules overlap

[elmarco]

Hi,

There is overlap between the rules provided by swtpm & selinux-policy, which makes managment unnecessarily complicated.

The swtpm.if interface present in both isn't much of a problem, afaict: selinux will check interfaces are already defined. But which one will come first?

Sometime it's more complicated, for example:
fedora-selinux/selinux-policy@6c412b2

introduced allow svirt_t virtqemud_t:fifo_file read; when swtpm did allow svirt_t virtqemud_t:fifo_file write; already.

My understanding is that selinux-policy provides libvirt policies, and swtpm shouldn't have them.

[stefanberger]

There are rules that may only be due to swtpm. We have these rules here now:

swtpm/src/selinux/swtpm_svirt.te

Lines 23 to 33 in 350b6a5

	allow svirt_t swtpm_exec_t:file entrypoint;
	# Due to session mode and usage of dir /run/user/*/libvirt/qemu/run/swtpm
	allow svirt_t user_tmp_t:sock_file { create setattr unlink };
	allow svirt_t virtd_t:dir search;
	allow svirt_t virtd_t:fifo_file write;
	allow svirt_t virtqemud_t:fifo_file write;
	allow svirt_t virt_var_run_t:dir { write add_name remove_name };
	allow svirt_t virt_var_run_t:file { create write setattr unlink };
	allow svirt_t virt_var_run_t:sock_file { create write setattr unlink };

What I find in the file you are referring to above are the following rules:

allow svirt_t self:process ptrace;
allow svirt_t self:netlink_rdma_socket create_socket_perms;
# it was a part of auth_use_nsswitch
allow svirt_t self:netlink_route_socket r_netlink_socket_perms;
allow svirt_t virtlogd_t:fifo_file write;
allow svirt_t virtlogd_t:unix_stream_socket connectto;

allow svirt_t virtqemud_t:tun_socket attach_queue;
allow svirt_t virtqemud_t:fifo_file read;
allow svirt_t virtqemud_var_run_t:file write;

There's not much overlap from what I can see but I would say that swtpm's SELinux rules are an addition to these rules. Maybe the swtpm project should not maintain an SELinux policy anymore but all rules should be in the upstream policy and rules clearly marked showing which ones are being added due to swtpm.

[zpytela]

In the swtpm module, there should only be rules allowing permissions to swtpm_t, or in some cases to IPC from swtpm_t.

Everything else should go to other modules or selinux-policy. Unfortunately, we cannot just copy the rules to selinux-policy without understanding why, that would make the policy pointless. Can you point us to the original problems or bug reports? One of the examples can be allowing a transition from virtqemud_t to swtpm_t instead of allowing permissions to the original domain. Additionally, interfaces should be used when types from other modules are needed.

[stefanberger]

In the swtpm module, there should only be rules allowing permissions to swtpm_t, or in some cases to IPC from swtpm_t.

Everything else should go to other modules or selinux-policy. Unfortunately, we cannot just copy the rules to selinux-policy without understanding why, that would make the policy pointless.

If it comes to 'trusting the rules' I have to say that I do not have documentation for the rules to justify each one of them. The easiest way to recreate the rules would be to uninstall the swtpm selinux package and (hopefully) it will lead to denials that after exercising all the necessary interactions with libvirt and virt-install (in system and user modes) will lead to the same rules again.

I also have some notes on this page about which directories to remove so that all rules are captured because of the creation of a directory that only occurs if swtpm had not been installed before:

• https://github.com/stefanberger/swtpm/wiki/SELinux-Policy-Development

Can you point us to the original problems or bug reports? One of the examples can be allowing a transition from virtqemud_t to swtpm_t instead of allowing permissions to the original domain. Additionally, interfaces should be used when types from other modules are needed.

Here's a list of the recent SELinux related Buzillas I can come up with:

• https://bugzilla.redhat.com/show_bug.cgi?id=2277041
• https://bugzilla.redhat.com/show_bug.cgi?id=2271074
• https://bugzilla.redhat.com/show_bug.cgi?id=2283878
• https://bugzilla.redhat.com/show_bug.cgi?id=2278889
• https://bugzilla.redhat.com/show_bug.cgi?id=2278123

Some of the rules depend on the configuration of libvirt and may not be easy to reproduce. In fact I initially had problems reproducing any of the issues that people saw after installation of F40 while I had done an upgrade from F39 to get to the same SELinux policy version.

[elmarco]

@zpytela on fc40, if I remove swtpm_libvirt and swtpm_svirt modules and run the test from https://github.com/stefanberger/swtpm/wiki/SELinux-Policy-Development I get the following policy:

require {
	type admin_home_t;
	type urandom_device_t;
	type device_t;
	type swtpm_t;
	type virtqemud_t;
	type svirt_image_t;
	type svirt_t;
	type qemu_var_run_t;
	type virt_log_t;
	type virt_var_lib_t;
	type swtpm_exec_t;
	type var_log_t;
	class file { create entrypoint execute map read relabelfrom relabelto setattr unlink write };
	class process signull;
	class dir { relabelfrom relabelto remove_name };
	class filesystem unmount;
	class chr_file setattr;
	class sock_file relabelfrom;
	class fifo_file { relabelfrom write };
}

#============= svirt_t ==============
allow svirt_t swtpm_exec_t:file { entrypoint execute read };

#!!!! This avc can be allowed using the boolean 'domain_can_mmap_files'
allow svirt_t swtpm_exec_t:file map;
allow svirt_t virtqemud_t:fifo_file write;

#============= virtqemud_t ==============
allow virtqemud_t admin_home_t:file { relabelfrom relabelto write };
allow virtqemud_t device_t:filesystem unmount;
allow virtqemud_t qemu_var_run_t:sock_file relabelfrom;
allow virtqemud_t self:fifo_file relabelfrom;
allow virtqemud_t svirt_image_t:chr_file setattr;
allow virtqemud_t swtpm_t:process signull;
allow virtqemud_t urandom_device_t:chr_file setattr;
allow virtqemud_t var_log_t:file { create relabelfrom relabelto setattr write };
allow virtqemud_t virt_log_t:dir remove_name;
allow virtqemud_t virt_log_t:file unlink;
allow virtqemud_t virt_var_lib_t:dir { relabelfrom relabelto };
allow virtqemud_t virt_var_lib_t:file { relabelfrom relabelto };
auth_log_filetrans_login_records(virtqemud_t)
files_manage_var_lib_files(virtqemud_t)

Do you want me to look at patching selinux-policy? thanks

[zpytela]

Please note F40 is far behind the current dvelopment which happens in rawhide. Can you run the test for a rawhide system? In the meantime, I will look at the test.

[elmarco]

@zpytela on rawhide:

require {
	type admin_home_t;
	type virt_log_t;
	type virtqemud_t;
	type swtpm_t;
	type swtpm_exec_t;
	type qemu_var_run_t;
	type svirt_t;
	type virt_var_lib_t;
	class process signull;
	class dir { relabelfrom relabelto };
	class file { create entrypoint execute map read relabelfrom relabelto write };
	class sock_file relabelfrom;
	class fifo_file { relabelfrom write };
}

#============= svirt_t ==============
allow svirt_t swtpm_exec_t:file { entrypoint execute read };

#!!!! This avc can be allowed using the boolean 'domain_can_mmap_files'
allow svirt_t swtpm_exec_t:file map;
allow svirt_t virtqemud_t:fifo_file write;

#============= swtpm_t ==============
virt_read_log(swtpm_t)

#============= virtqemud_t ==============
allow virtqemud_t admin_home_t:file { create relabelfrom relabelto write };
allow virtqemud_t qemu_var_run_t:sock_file relabelfrom;
allow virtqemud_t self:fifo_file relabelfrom;
allow virtqemud_t swtpm_t:process signull;
allow virtqemud_t virt_log_t:file { relabelfrom relabelto };
allow virtqemud_t virt_var_lib_t:dir { relabelfrom relabelto };
allow virtqemud_t virt_var_lib_t:file { relabelfrom relabelto };
auth_filetrans_admin_home_content(virtqemud_t)
files_manage_var_lib_files(virtqemud_t)
files_rw_var_lib_dirs(virtqemud_t)

[stefanberger]

It looks like the intention is to merge swtpm-related SELinux rules into the upstream reference policy. I suppose the upstream SELinux reference policy will carry them forward and update them?

[stefanberger]

How do we want to proceed with this?

[stefanberger]

Closing this issue. If we want to proceed with this in some way, we can reopen.

[elmarco]

well, I think we should clean this.. if @zpytela could guide us on how to proceed, I can work on it.