From: russell@coker.com.au (Russell Coker)
Date: Tue, 21 Feb 2017 17:31:23 +1100
Subject: [refpolicy] [PATCH] yet another draft of systemd patch 1
In-Reply-To: <5c3f1957-70da-b98f-ed1e-ae877c25f97a@ieee.org>
References: <20170220053525.kiug5zsw3wpzwfxl@athena.coker.com.au>
	<5c3f1957-70da-b98f-ed1e-ae877c25f97a@ieee.org>
Message-ID: <201702211731.23687.russell@coker.com.au>
To: refpolicy@oss.tresys.com
List-Id: refpolicy.oss.tresys.com

On Tue, 21 Feb 2017 02:07:42 AM Chris PeBenito wrote:
===================================================================
> > --- refpolicy-2.20170220.orig/policy/modules/kernel/kernel.te
> > +++ refpolicy-2.20170220/policy/modules/kernel/kernel.te
> > @@ -361,6 +361,8 @@ optional_policy(`
> > 
> >  optional_policy(`
> >  
> >  	init_sigchld(kernel_t)
> > 
> > +	init_dyntrans(kernel_t)
> > +	domain_dyntrans_type(kernel_t)
> > 
> >  ')
> 
> I think these are redundant, otherwise systemd wouldn't work at all.

Well it wasn't working when I first tried it.  ;)

But I've tested and found that it works without them now.  Maybe it was 
mislabelled when I wrote that policy.

===================================================================
> > --- refpolicy-2.20170220.orig/policy/modules/system/init.if
> > +++ refpolicy-2.20170220/policy/modules/system/init.if
> > @@ -127,7 +127,11 @@ interface(`init_domain',`
> > 
> >  	role system_r types $1;
> > 
> > -	domtrans_pattern(init_t, $2, $1)
> > +	ifdef(`init_systemd', `
> > +		domtrans_pattern(init_t, $2, $1)
> > +		allow init_t $1:unix_stream_socket create_stream_socket_perms;
> > +		allow $1 init_t:unix_dgram_socket sendto;
> > +	')
> 
> This would break sysvinit.

I'll put that into my "hacks" patch and review it again later.

> > @@ -468,15 +488,35 @@ interface(`init_ranged_system_domain',`
> > 
> >  		ifdef(`enable_mcs',`
> >  		
> >  			range_transition initrc_t $2:process $3;
> > 
> > +			range_transition init_t $2:process $3;
> > 
> >  		')
> >  		
> >  		ifdef(`enable_mls',`
> >  		
> >  			range_transition initrc_t $2:process $3;
> > 
> > +			range_transition init_t $2:process $3;
> > 
> >  			mls_rangetrans_target($1)
> >  		
> >  		')
> >  	
> >  	')
> >  
> >  ')
> 
> These above range_transitions don't look relevant to systemd as they're
> in the else portion of the init_systemd blocks.

OK I'll remove them.  I don't test the non-systemd case.

> > +######################################
> > +## <summary>
> > +##  Allow domain dyntransition to init_t domain.
> > +## </summary>
> > +## <param name="domain">
> > +##  <summary>
> > +##  Domain allowed to transition.
> > +##  </summary>
> > +## </param>
> > +#
> > +interface(`init_dyntrans',`
> > +	gen_require(`
> > +		type init_t;
> > +	')
> > +
> > +	dyntrans_pattern($1, init_t)
> > +')
> > +
> > 
> >  ########################################
> >  ## <summary>
> >  ##	Mark the file type as a daemon pid file, allowing initrc_t
> > 
> > @@ -675,6 +715,7 @@ interface(`init_stream_connect',`
> > 
> >  	stream_connect_pattern($1, init_var_run_t, init_var_run_t, init_t)
> >  	files_search_pids($1)
> > 
> > +	allow $1 init_t:unix_stream_socket getattr;
> 
> I'm reluctant to overload this interface.  Are you sure this applies to
> all processes that connect to init_t?em

I'm not sure about all.  But most processes that connect use systemd code and 
work in the same way.  As most do it and getattr isn't a dangerous access I 
think it's the reasonable thing to do.

> >  ')
> >  
> >  ########################################
> > 
> > @@ -1195,19 +1236,25 @@ interface(`init_telinit',`
> > 
> >  		type initctl_t;
> >  	
> >  	')
> > 
> > +	corecmd_exec_bin($1)
> > +
> > 
> >  	dev_list_all_dev_nodes($1)
> >  	allow $1 initctl_t:fifo_file rw_fifo_file_perms;
> >  	
> >  	init_exec($1)
> > 
> > -	tunable_policy(`init_upstart',`
> > +	ifdef(`init_systemd',`
> > 
> >  		gen_require(`
> >  		
> >  			type init_t;
> >  		
> >  		')
> > 
> > +		ps_process_pattern($1, init_t)
> > +		allow $1 init_t:process signal;
> > 
> >  		# upstart uses a datagram socket instead of initctl pipe
> >  		allow $1 self:unix_dgram_socket create_socket_perms;
> >  		allow $1 init_t:unix_dgram_socket sendto;
> > 
> > +		#576913
> > +		allow $1 init_t:unix_stream_socket connectto;
> > 
> >  	')
> >  
> >  ')
> 
> I think making this block unconditional is probably called for because
> of the tunable/ifdef "conflict" (should be enabled if systemd or upstart
> but can't create a single expression for that).  Even though sysvinit
> doesn't use them, the perms aren't that bad.

OK

> > @@ -1315,18 +1362,21 @@ interface(`init_spec_domtrans_script',`
> > 
> >  #
> >  interface(`init_domtrans_script',`
> >  
> >  	gen_require(`
> > 
> > -		type initrc_t, initrc_exec_t;
> > +		type initrc_t;
> > +		attribute init_script_file_type;
> > +		attribute initrc_transition_domain;
> > 
> >  	')
> > 
> > +	typeattribute $1 initrc_transition_domain;
> > 
> >  	files_list_etc($1)
> > 
> > -	domtrans_pattern($1, initrc_exec_t, initrc_t)
> > +	domtrans_pattern($1, init_script_file_type, initrc_t)
> > 
> >  	ifdef(`enable_mcs',`
> > 
> > -		range_transition $1 initrc_exec_t:process s0;
> > +		range_transition $1 init_script_file_type:process s0;
> > 
> >  	')
> >  	
> >  	ifdef(`enable_mls',`
> > 
> > -		range_transition $1 initrc_exec_t:process s0 - mls_systemhigh;
> > +		range_transition $1 init_script_file_type:process s0 - 
mls_systemhigh;
> > 
> >  	')
> >  
> >  ')
> 
> I'd prefer to split this out to a init_spec_domtrans_labeled_scripts(),
> so there is differentiation between the *_initrc_exec_t and initrc_exec_t.

I've created a new init_domtrans_labelled_script().

> > @@ -1402,9 +1452,14 @@ interface(`init_manage_script_service',`
> > 
> >  interface(`init_labeled_script_domtrans',`
> >  
> >  	gen_require(`
> >  	
> >  		type initrc_t;
> > 
> > +		attribute initrc_transition_domain;
> > 
> >  	')
> > 
> > +	typeattribute $1 initrc_transition_domain;
> > +	# service script searches all filesystems via mountpoint
> > +	fs_search_all($1)
> 
> Can you elaborate on this?  There has to be a way to limit it to
> something reasonable.

I'll try removing it and see if I can come up with something more restrictive.  
But those scripts do lots of wild stuff.  :(

> >  	domtrans_pattern($1, $2, initrc_t)
> > 
> > +	allow $1 $2:file ioctl;
> 
> This looks like a rule that should be in the caller's policy.

OK, I'll remove that and investigate other options.

> >  	files_search_etc($1)
> >  
> >  ')
> > 
> > @@ -1536,9 +1591,10 @@ interface(`init_run_daemon',`
> > 
> >  interface(`init_startstop_all_script_services',`
> >  
> >  	gen_require(`
> >  	
> >  		attribute init_script_file_type;
> > 
> > +		class service { start status stop reload };
> > 
> >  	')
> > 
> > -	allow $1 init_script_file_type:service { start status stop };
> > +	allow $1 init_script_file_type:service { start status stop reload };
> > 
> >  ')
> 
> I'd prefer to split this into a separate interface.

There's no reason not to have separate interfaces for all the options, but 
it's easier to have a single rule to do all of them as that will be the most 
common requirement.

> > +#######################################
> > +## <summary>
> > +##  Allow the specified domain to write to
> > +##  init sock file.
> > +## </summary>
> > +## <param name="domain">
> > +##  <summary>
> > +##  Domain allowed access.
> > +##  </summary>
> > +## </param>
> > +#
> > +interface(`init_write_pid_socket',`
> > +    gen_require(`
> > +        type init_var_run_t;
> > +    ')
> > +
> > +    allow $1 init_var_run_t:sock_file write;
> 
> Is this unreleated to init_stream_connect()?  I would think this is a
> process trying to do a unix socket tonnect to init.

It looks like it.  Currently I have lots of domains having 
init_stream_connect_script() explicitely in their policy and also 
init_write_pid_socket(daemon).

Should we have a single interface for both accesses and allow daemon to do it?

> > +')
> > +
> > +########################################
> > +## <summary>
> > +##	Read init unnamed pipes.
> > +## </summary>
> > +## <param name="domain">
> > +##	<summary>
> > +##	Domain allowed access.
> > +##	</summary>
> > +## </param>
> > +#
> > +interface(`init_read_pipes',`
> 
> init_read_pid_pipes()

OK.

> > +########################################
> > +## <summary>
> > +##      Rename and unlink init_var_run_t files
> > +## </summary>
> > +## <param name="domain">
> > +##      <summary>
> > +##      domain
> > +##      </summary>
> > +## </param>
> > +#
> > +interface(`rename_unlink_init_var_run',`
> 
> init_delete_pid_files().  Also please move after the init_create_pid_dirs()
> 

OK.

> > +	gen_require(`
> > +		type init_var_run_t;
> > +	')
> > +
> > +	allow $1 init_var_run_t:file { rename getattr unlink };
> 
> Please use a delete_files_pattern

OK.

> > +')
> > Index: refpolicy-2.20170220/policy/modules/system/init.te
> > 
===================================================================
> > --- refpolicy-2.20170220.orig/policy/modules/system/init.te
> > +++ refpolicy-2.20170220/policy/modules/system/init.te
> > @@ -16,13 +16,29 @@ gen_require(`
> > 
> >  ## </desc>
> >  gen_tunable(init_upstart, false)
> > 
> > +## <desc>
> > +## <p>
> > +## Allow all daemons the ability to read/write terminals
> > +## </p>
> > +## </desc>
> > +gen_tunable(allow_daemons_use_tty, false)
> > +
> > +## <desc>
> > +## <p>
> > +## Allow all daemons to write corefiles to /
> > +## </p>
> > +## </desc>
> > +gen_tunable(allow_daemons_dump_core, false)
> 
> I'd prefer to have new tunables to be prefixed with the module name, so
> init_daemons_dump_core, etc.

OK

> >  # slapd needs to read cert files from its initscript
> > 
> > -miscfiles_read_generic_certs(initrc_t)
> > +miscfiles_manage_generic_cert_files(initrc_t)
> > +
> > +optional_policy(`
> > +	init_get_system_status(initrc_t)
> > +')
> 
> Making this optional should have no effect as all the types are in the
> same module.

ok

> The below should have a new section header for "Rules applied to all
> daemons." and also moving the initrc_t stuff up with the other initrc_t
> rules.

OK.

> However, I'm also very concerned about how many rules are a being
> blanketed onto all daemons.  It seems extremely excessive.

Well there's not much change really and the biggest change is one that 
defaults to off.

> > +optional_policy(`
> > +	unconfined_dontaudit_rw_pipes(daemon)
> > +	unconfined_dontaudit_rw_stream(daemon)
> > +	userdom_dontaudit_read_user_tmp_files(daemon)
> > +	userdom_dontaudit_write_user_tmp_files(daemon)
> > +')
> 
> This looks like it should be split up into separate optionals

ok

> There is already an init_systemd block for these to be put in.
> 
> > +ifdef(`init_systemd',`
> > +	allow init_t self:system { status reboot halt reload };
> > +
> > +	allow init_t self:unix_dgram_socket { create_socket_perms sendto };

OK.

> > +ifdef(`init_systemd', `
> > +	dev_read_kmsg(syslogd_t)
> > +	dev_write_kmsg(syslogd_t)
> > +	allow syslogd_t self:capability sys_ptrace;
> 
> These should be moved down into the existing init_systemd

ok

> >  ########################################
> >  #
> >  # Auditctl local policy
> > 
> > @@ -230,6 +250,9 @@ optional_policy(`
> > 
> >  	udev_read_db(auditd_t)
> >  
> >  ')
> > 
> > +# for systemd but can not be conditional
> > +filetrans_pattern(syslogd_t, var_run_t, syslogd_tmp_t, dir, "log")
> 
> Neeeds to use interfaces and move down with the syslogd_t->syslogd_tmp_t
> rules.

ok

> > +##      Append to all log files.
> > +## </summary>
> > +## <param name="domain">
> > +##      <summary>
> > +##      Domain allowed access.
> > +##      </summary>
> > +## </param>
> > +#
> > +interface(`logging_inherit_append_all_logs',`
> 
> logging_append_all_inherited_logs()

ok

> > +
> > +########################################
> > +## <summary>
> > +##      Read and write a lvm unnamed pipe.
> > +## </summary>
> > +## <param name="domain">
> > +##      <summary>
> > +##      Domain allowed access.
> > +##      </summary>
> > +## </param>
> > +#
> > +interface(`lvm_rw_pipes',`
> > +        gen_require(`
> > +                type lvm_var_run_t;
> > +        ')
> > +
> > +        allow $1 lvm_var_run_t:fifo_file rw_inherited_fifo_file_perms;
> > +')
> 
> lvm_rw_inherited_runtime_pipes()

ok
 
> > +interface(`files_manage_root_files',`
> > +	gen_require(`
> > +		type root_t;
> > +	')
> > +
> > +	manage_files_pattern($1, root_t, root_t)
> > +')
> 
> I'm wondering if it makes sense to have a new type, so root_t can stay
> for / only.

I've deleted it from my policy.

The kernel has had the ability to support dumping core in other directories 
for a long time.  So you could create a mode 1733 directory somewhere to store 
core files.  Also systemd now manages them.

I was dubious about this policy all along and have now decided that it's 
generally a bad idea.  It just makes audit2allow prompt users to allowing this 
whenever a daemon tries to do something inappropriate.

> > --- refpolicy-2.20170220.orig/policy/modules/system/systemd.if
> > +++ refpolicy-2.20170220/policy/modules/system/systemd.if
> > @@ -35,7 +35,8 @@ interface(`systemd_read_logind_pids',`
> > 
> >  	')
> >  	
> >  	files_search_pids($1)
> > 
> > -	read_files_pattern($1, systemd_logind_var_run_t,
> > systemd_logind_var_run_t) +	allow $1 systemd_logind_var_run_t:dir
> > list_dir_perms;
> > +	allow $1 systemd_logind_var_run_t:file read_file_perms;
> 
> This second rule is redundant.

Not when you remove the read_files_pattern line.

> > +#######################################
> > +## <summary>
> > +##  Allow systemd_tmpfiles_t to manage filesystem objects
> > +## </summary>
> > +## <param name="type">
> > +## <summary>
> > +##  type of object to manage
> > +## </summary>
> > +## </param>
> > +## <param name="class">
> > +## <summary>
> > +##  object class to manage
> > +## </summary>
> > +## </param>
> > +#
> > +interface(`systemd_tmpfiles_manage_object',`
> 
> systemd_tmpfilesd_managed()

ok

> > +##      allow systemd_passwd_agent to inherit fds
> > +## </summary>
> > +## <param name="domain">
> > +##      <summary>
> > +##      Domain that owns the fds
> > +##      </summary>
> > +## </param>
> > +#
> > +interface(`systemd_passwd_agent_inherits_fd',`
> 
> systemd_use_passwd_agent_fds

ok

> > +########################################
> > +## <summary>
> > +##      Transition to systemd named content
> > +##	need a better name for this
> > +## </summary>
> > +## <param name="domain">
> > +##      <summary>
> > +##	Domain allowed access.
> > +##      </summary>
> > +## </param>
> > +#
> > +interface(`systemd_filetrans_named_content',`
> 
> I'm struggling on the naming for this too, though I don't think
> named_content fits, but something like systemd_passd_pid_dirs or
> systemd_passwd_runtime_dirs

I've changed it to the latter.

> > +########################################
> > +## <summary>
> > +##      manage systemd unit dirs and the files in them
> > +## </summary>
> > +## <param name="domain">
> > +##      <summary>
> > +##      Domain allowed access.
> > +##      </summary>
> > +## </param>
> > +#
> > +interface(`systemd_manage_unit_dirs_files',`
> 
> systemd_manage_all_units

ok

> > +########################################
> > +## <summary>
> > +##      Do not audit attempts to read and write
> > +##      unconfined domain stream.
> > +## </summary>
> > +## <param name="domain">
> > +##      <summary>
> > +##      Domain to not audit.
> > +##      </summary>
> > +## </param>
> > +#
> > +interface(`unconfined_dontaudit_rw_stream',`
> 
> unconfined_dontaudit_rw_stream_sockets()

ok

I'll send you a new patch shortly.

-- 
My Main Blog         http://etbe.coker.com.au/
My Documents Blog    http://doc.coker.com.au/