[refpolicy] [PATCH v2] wm: update the window manager (wm) module and enable its role template

[guido@trentalancia.net (Guido Trentalancia)]

On Thu, 08/12/2016 at 22.13 +0100, Dominick Grift via refpolicy wrote:
> On 12/08/2016 09:53 PM, Guido Trentalancia via refpolicy wrote:
> > 
> > On Thu, 08/12/2016 at 18.51 +0100, Dominick Grift via refpolicy
> > wrote:
> > > 
> > > i
> > > On 12/08/2016 06:20 PM, Guido Trentalancia via refpolicy wrote:
> > > > 
> > > > 
> > > > Enable the window manager role (wm contrib module) and update
> > > > the module to work with gnome-shell.
> > > > 
> > > > This second version introduces better integration with common
> > > > desktop applications and requires the following recently posted
> > > > patch for the games module:
> > > > 
> > > > [PATCH 1/2] games: general update and improved pulseaudio
> > > > integration
> > > > http://oss.tresys.com/pipermail/refpolicy/2016-December/008650.
> > > > html
> > > > 
> > > > This patch might need some more testing.
> > > 
> > > Yes, i dont think this should be merged.
> > 
> > It needs to be tested with other window managers.
> > 
> > Have you found any actual limitation with the old gnome (gnome-
> > panel/metacity), kde or others ?
> > 
> 
> It is hard to explain but consider some of this stuff. gnome-shell
> connects to http ports, gnome-shell needs to be able to run programs
> on
> behalf of the calling user (ALT-f2), gnome-shell is a pulseaudio
> client,
> gnome-shell needs comms with: bluetooth, accountsd, geoclue, upower,
> logind: all things that a normal windows manager does not need.

gnome-shell does not need to connect to http ports and the Alt-F2 key
combination is not used to run programs.

Once again, the difference in terms of permissions compared to the
actual module is mostly general and harmless.

> Not to mention that gnome requires permissions that are specific to
> wayland compositors

The module is only tested with the X server and not wayland.

If and when support for wayland will be introduced, we'll decide
whether or not to fork new policy specifically for gnome-shell. As
already explained, at the moment, it definitely doesn't pay back.

> > > Also gnome-shell is a wm but its not every wm needs the
> > > permissions
> > > that
> > > gnome-shell needs. We should either give gnome-shell its own
> > > domain
> > > or
> > > add the gnome-shell specific rules to a tunable.
> > > 
> > > For example: gnome-shell is a wayland compositor. but there are
> > > many
> > > more things that gnome shell needs that a "normal" window manager
> > > should
> > > never need.
> > 
> > The permissions that have been added are very general and
> > harmless.?
> 
> For now maybe because the policy is not comprehensive
> 
> > 
> > > 
> > > > 
> > > > ?policy/modules/contrib/pulseaudio.if |???19 +++++++++
> > > > ?policy/modules/contrib/wm.if?????????|???44
> > > > +++++++++++++++++++++
> > > > ?policy/modules/contrib/wm.te?????????|???73
> > > > +++++++++++++++++++++++++++++++++++
> > > > ?policy/modules/roles/staff.te????????|????1
> > > > ?policy/modules/roles/sysadm.te???????|????1
> > > > ?policy/modules/roles/unprivuser.te??|????1
> > > > ?6 files changed, 139 insertions(+)
> > > > 
> > > > diff -pruN refpolicy-git-07122016-
> > > > orig/policy/modules/contrib/pulseaudio.if refpolicy-git-
> > > > 07122016/policy/modules/contrib/pulseaudio.if
> > > > --- refpolicy-git-07122016-
> > > > orig/policy/modules/contrib/pulseaudio.if	2016-09-27
> > > > 16:28:51.964139648 +0200
> > > > +++ refpolicy-git-07122016/policy/modules/contrib/pulseaudio.if
> > > > 	
> > > > 2016-12-08 15:47:40.117740787 +0100
> > > > @@ -424,3 +424,22 @@ interface(`pulseaudio_rw_tmpfs_files',`
> > > > ?	fs_search_tmpfs($1)
> > > > ?	rw_files_pattern($1, pulseaudio_tmpfs_t,
> > > > pulseaudio_tmpfs_t)
> > > > ?')
> > > > +
> > > > +#######################################
> > > > +## <summary>
> > > > +##	Manage pulseaudio tmpfs files.
> > > > +## </summary>
> > > > +## <param name="domain">
> > > > +##	<summary>
> > > > +##	Domain allowed access.
> > > > +##	</summary>
> > > > +## </param>
> > > > +#
> > > > +interface(`pulseaudio_manage_tmpfs_files',`
> > > > +	gen_require(`
> > > > +		type pulseaudio_tmpfs_t;
> > > > +	')
> > > > +
> > > > +	fs_search_tmpfs($1)
> > > > +	manage_files_pattern($1, pulseaudio_tmpfs_t,
> > > > pulseaudio_tmpfs_t)
> > > > +')
> > > 
> > > does not make sense, and i would like to know what prompted you
> > > to
> > > add
> > > this (preferably backed with avc denials)
> > 
> > It's needed to unlink them. Probably stale files from previous
> > sessions.
> > 
> > > 
> > > wm should is a pulseaudio_client and pulseaudio_client can
> > > already rw
> > > and delete pulseaudio_tmpfs_files. It should not need more than
> > > that.
> > 
> > I decided to avoid using the pulseaudio_client attribute because it
> > implies other unneeded permissions, including corenet permissions
> > that
> > are quite undesirable.
> > 
> 
> Those permissions aren't unneeded. They should be conditional maybe
> but
> there are configurations where clients may want to connect to
> pulseaudio
> via the network.
> 
> > 
> > > 
> > > > 
> > > > diff -pruN refpolicy-git-07122016-
> > > > orig/policy/modules/contrib/wm.if?
> > > > refpolicy-git-07122016/policy/modules/contrib/wm.if
> > > > --- refpolicy-git-07122016-orig/policy/modules/contrib/wm.if	
> > > > 2016-08-14 21:28:11.597521187 +0200
> > > > +++ refpolicy-git-07122016/policy/modules/contrib/wm.if	
> > > > 2016
> > > > -12-08 15:47:40.118740804 +0100
> > > > @@ -47,6 +47,8 @@ template(`wm_role_template',`
> > > > ?	# Policy
> > > > ?	#
> > > > ?
> > > > +	allow $3 $1_wm_t:fd use;
> > > > +
> > > > ?	allow $1_wm_t $3:unix_stream_socket connectto;
> > > > ?	allow $3 $1_wm_t:unix_stream_socket connectto;
> > > > ?
> > > > @@ -72,6 +74,7 @@ template(`wm_role_template',`
> > > > ?	xserver_manage_core_devices($1_wm_t)
> > > > ?
> > > > ?	optional_policy(`
> > > > +		dbus_connect_spec_session_bus($1, $1_wm_t)
> > > > ?		dbus_spec_session_bus_client($1, $1_wm_t)
> > > > ?		dbus_system_bus_client($1_wm_t)
> > > > ?
> > > > @@ -81,11 +84,14 @@ template(`wm_role_template',`
> > > > ?	')
> > > > ?
> > > > ?	optional_policy(`
> > > > +		gnome_dbus_chat_gkeyringd($1, $1_wm_t)
> > > > ?		gnome_stream_connect_gkeyringd($1, $1_wm_t)
> > > > ?	')
> > > > ?
> > > > ?	optional_policy(`
> > > > ?		pulseaudio_run($1_wm_t, $2)
> > > > +		pulseaudio_manage_tmpfs_files($1_wm_t)
> > > > +		pulseaudio_use_fds($1_wm_t)
> > > 
> > > these shouldnt be needed. what prompted you to add these?
> > > (preferably
> > > show avc denials)
> > 
> > See above. Stale files need to be unlinked.
> 
> That is a bug here then:
> 
> https://github.com/TresysTechnology/refpolicy-contrib/blob/master/pul
> seaudio.te#L241
> 
> clients should then also be able to delete pulseaudio tmpfs files
> 
> > 
> > 
> > > 
> > > > 
> > > > ?	')
> > > > ?')
> > > > ?
> > > > @@ -134,3 +140,41 @@ interface(`wm_dbus_chat',`
> > > > ?	allow $2 $1_wm_t:dbus send_msg;
> > > > ?	allow $1_wm_t $2:dbus send_msg;
> > > > ?')
> > > > +
> > > > +########################################
> > > > +## <summary>
> > > > +##	Do not audit attempts to execute
> > > > +##	files in temporary directories.
> > > > +## </summary>
> > > > +## <param name="domain">
> > > > +##	<summary>
> > > > +##	Domain to not audit.
> > > > +##	</summary>
> > > > +## </param>
> > > > +#
> > > > +interface(`wm_dontaudit_exec_tmp_files',`
> > > > +	gen_require(`
> > > > +		type wm_tmp_t;
> > > > +	')
> > > > +
> > > > +	dontaudit $1 wm_tmp_t:file exec_file_perms;
> > > > +')
> > > > +
> > > > +########################################
> > > > +## <summary>
> > > > +##	Do not audit attempts to execute
> > > > +##	files in temporary filesystems.
> > > > +## </summary>
> > > > +## <param name="domain">
> > > > +##	<summary>
> > > > +##	Domain to not audit.
> > > > +##	</summary>
> > > > +## </param>
> > > > +#
> > > > +interface(`wm_dontaudit_exec_tmpfs_files',`
> > > > +	gen_require(`
> > > > +		type wm_tmpfs_t;
> > > > +	')
> > > > +
> > > > +	dontaudit $1 wm_tmpfs_t:file exec_file_perms;
> > > > +')
> > > > diff -pruN refpolicy-git-07122016-
> > > > orig/policy/modules/contrib/wm.te?
> > > > refpolicy-git-07122016/policy/modules/contrib/wm.te
> > > > --- refpolicy-git-07122016-orig/policy/modules/contrib/wm.te	
> > > > 2016-10-29 16:29:19.762328008 +0200
> > > > +++ refpolicy-git-07122016/policy/modules/contrib/wm.te	
> > > > 2016
> > > > -12-08 17:57:40.843768477 +0100
> > > > @@ -10,6 +10,14 @@ attribute wm_domain;
> > > > ?type wm_exec_t;
> > > > ?corecmd_executable_file(wm_exec_t)
> > > > ?
> > > > +type wm_tmp_t;
> > > > +typealias wm_tmp_t alias { user_wm_tmp_t staff_wm_tmp_t
> > > > sysadm_wm_tmp_t };
> > > > +userdom_user_tmp_file(wm_tmp_t)
> > > > +
> > > > +type wm_tmpfs_t;
> > > > +typealias wm_tmpfs_t alias { user_wm_tmpfs_t staff_wm_tmpfs_t
> > > > sysadm_wm_tmpfs_t };
> > > > +userdom_user_tmpfs_file(wm_tmpfs_t)
> > > 
> > > this is a pulseaudio_tmpfs_file()
> > 
> > I am not sure about that.
> > 
> > It works fine, so at the moment I can't see a reason to change it.
> > 
> > It is probably used to create libffi files and not pulseaudio files
> > (that I expect to be created by the pulseaudio process). See below.
> > 
> 
> All pulseaudio clients need to be able to read/write/unlink eachother
> tmpfs files.
> 
> > 
> > > 
> > > > 
> > > > +
> > > > ?########################################
> > > > ?#
> > > > ?# Common wm domain local policy
> > > > @@ -21,31 +29,60 @@ allow wm_domain self:netlink_kobject_uev
> > > > ?allow wm_domain self:shm create_shm_perms;
> > > > ?allow wm_domain self:unix_dgram_socket create_socket_perms;
> > > > ?
> > > > +manage_dirs_pattern(wm_domain, wm_tmp_t, wm_tmp_t)
> > > > +manage_files_pattern(wm_domain, wm_tmp_t, wm_tmp_t)
> > > > +manage_lnk_files_pattern(wm_domain, wm_tmp_t, wm_tmp_t)
> > > > +files_tmp_filetrans(wm_domain, wm_tmp_t, { dir file lnk_file
> > > > })
> > > > +
> > > > +manage_dirs_pattern(wm_domain, wm_tmpfs_t, wm_tmpfs_t)
> > > > +manage_files_pattern(wm_domain, wm_tmpfs_t, wm_tmpfs_t)
> > > > +manage_lnk_files_pattern(wm_domain, wm_tmpfs_t, wm_tmpfs_t)
> > > > +fs_tmpfs_filetrans(wm_domain, wm_tmpfs_t, { dir file lnk_file
> > > > })
> > > > +
> > > > +can_exec(wm_domain, wm_exec_t)
> > > > +
> > > > ?kernel_read_system_state(wm_domain)
> > > > ?
> > > > ?corecmd_getattr_all_executables(wm_domain)
> > > > ?
> > > > +dev_read_rand(wm_domain)
> > > > ?dev_read_sound(wm_domain)
> > > > ?dev_read_sysfs(wm_domain)
> > > > ?dev_read_urand(wm_domain)
> > > > +dev_rw_dri(wm_domain)
> > > > ?dev_rw_wireless(wm_domain)
> > > > ?dev_write_sound(wm_domain)
> > > > ?
> > > > +files_read_etc_runtime_files(wm_domain)
> > > > ?files_read_usr_files(wm_domain)
> > > > ?
> > > > ?fs_getattr_all_fs(wm_domain)
> > > > ?
> > > > +kernel_read_fs_sysctls(wm_domain)
> > > > +kernel_read_proc_symlinks(wm_domain)
> > > > +kernel_read_sysctl(wm_domain)
> > > > +
> > > > ?miscfiles_read_fonts(wm_domain)
> > > > +miscfiles_read_generic_certs(wm_domain)
> > > > ?miscfiles_read_localization(wm_domain)
> > > > ?
> > > > +udev_read_pid_files(wm_domain)
> > > > +
> > > > +# this is needed by gnome-shell
> > > > +userdom_exec_user_home_content_files(wm_domain)
> > > 
> > > What exactly is it executing here?
> > 
> > I think it is executing files generated by libffi.
> 
> and how do those files end up in $HOME, maybe because you're blocking
> access to tmp (XDG_RUNTIME_DIR)?
> 
> /run/user/UID is first choice

No, it doesn't work like that.

And the home directory is the safest option.

> > > > +
> > > > ?userdom_manage_user_tmp_sockets(wm_domain)
> > > > ?userdom_tmp_filetrans_user_tmp(wm_domain, sock_file)
> > > > ?userdom_user_runtime_filetrans_user_tmp(wm_domain, sock_file)
> > > > ?
> > > > ?userdom_manage_user_home_content_dirs(wm_domain)
> > > > ?userdom_manage_user_home_content_files(wm_domain)
> > > > +
> > > > ?userdom_user_home_dir_filetrans_user_home_content(wm_domain, {
> > > > dir
> > > > file })
> > > > ?
> > > > +wm_dontaudit_exec_tmp_files(wm_domain)
> > > > +wm_dontaudit_exec_tmpfs_files(wm_domain)
> > > 
> > > why dontaudit the above? gnome-sheM
> 
> 
> These files are probably initially created in $CDG_RUNTIME_DIR which
> is
> the best place. However if you block that then it falls back to
> ~/.cache
> (which is second best)
> 
> > 
> > 
> > > 
> > > > 
> > > > +
> > > > ?optional_policy(`
> > > > ?	accountsd_dbus_chat(wm_domain)
> > > > ?')
> > > > @@ -55,10 +92,42 @@ optional_policy(`
> > > > ?')		
> > > > ?
> > > > ?optional_policy(`
> > > > +	consolekit_dbus_chat(wm_domain)
> > > > +')
> > > > +
> > > > +optional_policy(`
> > > > ?	devicekit_dbus_chat_power(wm_domain)
> > > > ?')
> > > > ?
> > > > ?optional_policy(`
> > > > +	evolution_dbus_chat(wm_domain)
> > > > +	evolution_alarm_dbus_chat(wm_domain)
> > > > +	evolution_domtrans(wm_domain)
> > > > +')
> > > > +
> > > > +optional_policy(`
> > > > +	games_dbus_chat(wm_domain)
> > > > +	games_domtrans(wm_domain)
> > > > +')
> > > > +
> > > > +optional_policy(`
> > > > +	java_domtrans(wm_domain)
> > > > +')
> > > > +
> > > > +optional_policy(`
> > > > +	mono_domtrans(wm_domain)
> > > > +')
> > > > +
> > > > +optional_policy(`
> > > > +	mozilla_domtrans(wm_domain)
> > > > +	mozilla_dbus_chat(wm_domain)
> > > > +')
> > > > +
> > > > +optional_policy(`
> > > > +	mplayer_domtrans(wm_domain)
> > > > +')
> > > > +
> > > > +optional_policy(`
> > > > ?	networkmanager_dbus_chat(wm_domain)
> > > > ?')
> > > > ?
> > > > @@ -71,5 +140,9 @@ optional_policy(`
> > > > ?')
> > > > ?
> > > > ?optional_policy(`
> > > > +	telepathy_mission_control_dbus_chat(wm_domain)
> > > > +')
> > > > +
> > > > +optional_policy(`
> > > > ?	userhelper_exec_consolehelper(wm_domain)
> > > > ?')
> > > > diff -pruN refpolicy-git-07122016-
> > > > orig/policy/modules/roles/staff.te refpolicy-git-
> > > > 07122016/policy/modules/roles/staff.te
> > > > --- refpolicy-git-07122016-orig/policy/modules/roles/staff.te	
> > > > 2016-12-07 13:39:08.669449296 +0100
> > > > +++ refpolicy-git-07122016/policy/modules/roles/staff.te	
> > > > 201
> > > > 6-12-08 15:47:40.140741170 +0100
> > > > @@ -85,6 +85,7 @@ ifndef(`distro_redhat',`
> > > > ?
> > > > ?		optional_policy(`
> > > > ?			gnome_role_template(staff, staff_r,
> > > > staff_t)
> > > > +			wm_role_template(staff, staff_r,
> > > > staff_t)
> > > > ?		')
> > > > ?
> > > > ?		optional_policy(`
> > > > diff -pruN refpolicy-git-07122016-
> > > > orig/policy/modules/roles/sysadm.te refpolicy-git-
> > > > 07122016/policy/modules/roles/sysadm.te
> > > > --- refpolicy-git-07122016-orig/policy/modules/roles/sysadm.te	
> > > > 2016-12-07 13:39:08.669449296 +0100
> > > > +++ refpolicy-git-07122016/policy/modules/roles/sysadm.te	
> > > > 20
> > > > 16-12-08 15:47:40.141741187 +0100
> > > > @@ -1245,6 +1245,7 @@ ifndef(`distro_redhat',`
> > > > ?
> > > > ?		optional_policy(`
> > > > ?			gnome_role_template(sysadm, sysadm_r,
> > > > sysadm_t)
> > > > +			wm_role_template(sysadm, sysadm_r,
> > > > sysadm_t)
> > > > ?		')
> > > > ?	')
> > > > ?
> > > > diff -pruN refpolicy-git-07122016-
> > > > orig/policy/modules/roles/unprivuser.te refpolicy-git-
> > > > 07122016/policy/modules/roles/unprivuser.te
> > > > --- refpolicy-git-07122016-
> > > > orig/policy/modules/roles/unprivuser.te	
> > > > 2016-12-07 13:39:08.669449296 +0100
> > > > +++ refpolicy-git-07122016/policy/modules/roles/unprivuser.te	
> > > > 2016-12-08 15:47:40.141741187 +0100
> > > > @@ -54,6 +54,7 @@ ifndef(`distro_redhat',`
> > > > ?
> > > > ?		optional_policy(`
> > > > ?			gnome_role_template(user, user_r,
> > > > user_t)
> > > > +			wm_role_template(user, user_r, user_t)
> > > > ?		')
> > > > ?
> > > > ?		optional_policy(`

Guido