[refpolicy] [PATCH v2]: mcelog module initial rewrite

* [refpolicy] [PATCH v2]: mcelog module initial rewrite
@ 2012-08-06 15:19 Guido Trentalancia
  2012-08-06 15:30 ` Dominick Grift
  0 siblings, 1 reply; 21+ messages in thread
From: Guido Trentalancia @ 2012-08-06 15:19 UTC (permalink / raw)
  To: refpolicy

Hello Dominick.

>On Mon, 2012-08-06 at 14:45 +0200, Guido Trentalancia wrote:

[cut]

>> diff -pruN refpolicy-04062012/policy/modules/contrib/mcelog.fc refpolicy-04062012-mcelog-support/policy/modules/contrib/mcelog.fc
>> --- refpolicy-04062012/policy/modules/contrib/mcelog.fc	2011-09-09 18:29:23.578610955 +0200
>> +++ refpolicy-04062012-mcelog-support/policy/modules/contrib/mcelog.fc	2012-08-06 13:34:45.568049105 +0200
>> @@ -1 +1,16 @@
>> +/etc/mcelog(/.*)?	gen_context(system_u:object_r:mcelog_etc_t,s0)
>> +/etc/mcelog/.*-error-trigger	--	gen_context(system_u:object_r:mcelog_exec_t,s0)
>> +/etc/mcelog/.*.local		--	gen_context(system_u:object_r:mcelog_exec_t,s0)
>> +
>
>I dont have a good argument for using bin_t but i dont have a good
>argument for using mcelog_exec_t either , therefore i suggest keeping it
>bin_t

If we keep bin_t, then we need to use corecmd_exec_bin() or whatever that is called, which means it can execute any script and in particular any binary.

So, the good reason is restricting the type of files that mcelog can execute. In my opinion policy should always been designed that way, when the application needs to execute internal (or user-defined) scripts or binaries (as opposed to system-wide executables in /bin, /sbin, /usr/bin or /usr/sbin).

Unless you give me a good reason, I won't change that.

>> +ifdef(`distro_redhat',`
>> +/etc/mcelog/triggers	-d	gen_context(system_u:object_r:mcelog_etc_t,s0)
>> +/etc/mcelog/triggers(/.*)?	gen_context(system_u:object_r:mcelog_exec_t,s0)
>> +')
>> +
>> +/etc/rc.d/init.d/mcelog	--	gen_context(system_u:object_r:mcelog_initrc_exec_t,s0)
>> +
>
>Weird, i still dont see the periods escaped above.

Unfortunately, at the moment I do not have another mail client at hand, so I suppose, it needs manual editing.

>>  /usr/sbin/mcelog	--	gen_context(system_u:object_r:mcelog_exec_t,s0)
>> +
>> +/var/log/mcelog		--	gen_context(system_u:object_r:mcelog_log_t,s0)
>> +/var/run/mcelog.pid	--	gen_context(system_u:object_r:mcelog_var_run_t,s0)
>> +/var/run/mcelog-client	-s	gen_context(system_u:object_r:mcelog_var_run_t,s0)
>> diff -pruN refpolicy-04062012/policy/modules/contrib/mcelog.if refpolicy-04062012-mcelog-support/policy/modules/contrib/mcelog.if
>> --- refpolicy-04062012/policy/modules/contrib/mcelog.if	2011-09-09 18:29:23.578610955 +0200
>> +++ refpolicy-04062012-mcelog-support/policy/modules/contrib/mcelog.if	2012-08-06 15:37:21.714522005 +0200
>> @@ -18,3 +18,78 @@ interface(`mcelog_domtrans',`
>>  	domtrans_pattern($1, mcelog_exec_t, mcelog_t)
>>  ')
>>  
>> +########################################
>> +## <summary>
>> +##	Read the mcelog configuration files.
>
>I would probably use "Read mcelog configuration files."

Ok. I will create a third version (v3) with further changes as necessary.

>> +## </summary>
>> +## <param name="domain">
>> +##	<summary>
>> +##	Domain allowed access.
>> +##	</summary>
>> +## </param>
>> +## <rolecap/>
>> +#
>> +interface(`mcelog_read_config',`
>
>I would probably call the interface "mcelog_read_config_files" or
>"mcelog_read_etc_files"

Yes, reasonable.

>> +	gen_require(`
>> +		type mcelog_etc_t;
>> +	')
>> +
>> +	files_search_etc($1)
>> +	read_files_pattern($1, mcelog_etc_t, mcelog_etc_t)
>> +	allow $1 mcelog_etc_t:dir list_dir_perms;
>> +')
>> +
>> +########################################
>> +## <summary>
>> +##	Create an mcelog unix stream socket.
>> +## </summary>
>> +## <param name="domain">
>> +##	<summary>
>> +##	Domain allowed access.
>> +##	</summary>
>> +## </param>
>> +#
>> +interface(`mcelog_stream_socket_create',`
>> +	gen_require(`
>> +		type mcelog_t;
>> +	')
>> +
>> +	allow $1 mcelog_t:unix_stream_socket create_socket_perms;
>> +')
>
>Not needed.

You suggested (initial review):

allow mcelog_t self:unix_stream_socket create_socket_perms;

I have just turned that into an interface...

So, I don't get the point now.

>> +########################################
>> +## <summary>
>> +##	Read from an mcelog unix stream socket.
>> +## </summary>
>> +## <param name="domain">
>> +##	<summary>
>> +##	Domain allowed access.
>> +##	</summary>
>> +## </param>
>> +#
>> +interface(`mcelog_stream_socket_read',`
>> +	gen_require(`
>> +		type mcelog_t, mcelog_var_run_t;
>> +	')
>> +
>> +	allow $1 mcelog_var_run_t:unix_stream_socket { read };
>> +')
>
>not needed.

I think it gets audited as denied otherwise (possibly the client mode). If time allows, I'll double-check.

>> +########################################
>> +## <summary>
>> +##	Connect to mcelog over an unix stream socket.
>> +## </summary>
>> +## <param name="domain">
>> +##	<summary>
>> +##	Domain allowed access.
>> +##	</summary>
>> +## </param>
>> +#
>> +interface(`mcelog_stream_socket_connect',`
>> +	gen_require(`
>> +		type mcelog_t, mcelog_var_run_t;
>> +	')
>> +
>> +	files_search_pids($1)
>> +	stream_connect_pattern($1, mcelog_var_run_t, mcelog_var_run_t, mcelog_t);
>> +')
>
>Not needed.

In client-mode it needs to connect to the socket.

Has the feature been removed in subsequent versions ? I can't find it anymore on kernel.org...

>> diff -pruN refpolicy-04062012/policy/modules/contrib/mcelog.te refpolicy-04062012-mcelog-support/policy/modules/contrib/mcelog.te
>> --- refpolicy-04062012/policy/modules/contrib/mcelog.te	2011-09-09 18:29:23.578610955 +0200
>> +++ refpolicy-04062012-mcelog-support/policy/modules/contrib/mcelog.te	2012-08-06 16:01:45.087746478 +0200
>> @@ -1,4 +1,4 @@
>> -policy_module(mcelog, 1.1.0)
>> +policy_module(mcelog, 1.1.1)
>>  
>>  ########################################
>>  #
>> @@ -7,8 +7,20 @@ policy_module(mcelog, 1.1.0)
>>  
>>  type mcelog_t;
>>  type mcelog_exec_t;
>> -application_domain(mcelog_t, mcelog_exec_t)
>> -cron_system_entry(mcelog_t, mcelog_exec_t)
>> +corecmd_executable_file(mcelog_exec_t);
>
>I still dont think corecmd_executable_file is needed. Not sure though.

The triggers are scripts. Their execution is triggered by CPU error events.

If such scripts are labelled as bin_t, all is needed is corecmd_exec_bin() or whatever it is called.

Otherwise, if they are labelled differently for increased security as explained above, it should need both corecmd_executable_file() and can_exec() on the private executable type.

>> +init_daemon_domain(mcelog_t, mcelog_exec_t)
>> +
>> +type mcelog_initrc_exec_t;
>> +init_script_file(mcelog_initrc_exec_t)
>> +
>> +type mcelog_etc_t;
>> +files_config_file(mcelog_etc_t)
>> +
>> +type mcelog_log_t;
>> +logging_log_file(mcelog_log_t)
>> +
>> +type mcelog_var_run_t;
>> +files_pid_file(mcelog_var_run_t)
>>  
>>  ########################################
>>  #
>> @@ -17,16 +29,48 @@ cron_system_entry(mcelog_t, mcelog_exec_
>>  
>>  allow mcelog_t self:capability sys_admin;
>
>add:
>
>allow mcelog_t self:unix_stream_socket create_stream_socket_perms;
>
>>  
>> +can_exec(mcelog_t, mcelog_exec_t)
>> +
>
>If we keep using bin_t then this isnt needed

To be honest I would rather prefer not using bin_t. Perhaps, it needs to be able to transition from the private exec type ?

>>  kernel_read_system_state(mcelog_t)
>>  
>>  dev_read_raw_memory(mcelog_t)
>>  dev_read_kmsg(mcelog_t)
>>  
>> +dev_rw_sysfs(mcelog_t)
>> +
>> +# optional support for running it as a cron job
>> +optional_policy(`
>> +	cron_system_entry(mcelog_t, mcelog_exec_t)
>> +')
>> +
>> +manage_files_pattern(mcelog_t, mcelog_var_run_t, mcelog_var_run_t)
>> +manage_sock_files_pattern(mcelog_t, mcelog_var_run_t, mcelog_var_run_t)
>> +files_pid_filetrans(mcelog_t, mcelog_var_run_t, { file sock_file })
>> +
>> +# needed in daemon mode
>>  files_read_etc_files(mcelog_t)
>>  
>> -# for /dev/mem access
>> -mls_file_read_all_levels(mcelog_t)
>> +locallogin_use_fds(mcelog_t)
>> +
>> +# create/append a logfile in a private log directory
>> +create_files_pattern(mcelog_t, mcelog_log_t, mcelog_log_t)
>> +append_files_pattern(mcelog_t, mcelog_log_t, mcelog_log_t)
>> +setattr_files_pattern(mcelog_t, mcelog_log_t, mcelog_log_t)
>> +logging_log_filetrans(mcelog_t, mcelog_log_t, file)
>>  
>> +# use syslog functionality (optional, configurable)
>>  logging_send_syslog_msg(mcelog_t)
>>  
>> +# to read the standard configuration file
>> +mcelog_read_config(mcelog_t)
>
>use read_files_pattern(mcelog_t, mcelog_etc_t, mcelog_etc_t)
>
>Interfaces are not for internal use

As far as I know other modules are using interfaces internally. I will double-check and if neccessary remove them.

>> +
>> +mcelog_stream_socket_create(mcelog_t)
>> +mcelog_stream_socket_read(mcelog_t)
>> +mcelog_stream_socket_connect(mcelog_t)
>> +
>
>none of the above are needed
>
>>  miscfiles_read_localization(mcelog_t)
>> +
>> +# for /dev/mem access
>> +mls_file_read_all_levels(mcelog_t)
>> +
>> +term_use_all_ttys(mcelog_t)

term_use_all_ttys() is needed for interactive use. Do you know anything more restrictive than that ?

>> diff -pruN refpolicy-04062012/policy/modules/kernel/corecommands.fc refpolicy-04062012-mcelog-support/policy/modules/kernel/corecommands.fc
>> --- refpolicy-04062012/policy/modules/kernel/corecommands.fc	2012-08-05 04:52:17.194005067 +0200
>> +++ refpolicy-04062012-mcelog-support/policy/modules/kernel/corecommands.fc	2012-08-05 17:49:05.594838788 +0200
>> @@ -72,12 +72,6 @@ ifdef(`distro_redhat',`
>>  /etc/kde/shutdown(/.*)?			gen_context(system_u:object_r:bin_t,s0)
>>  
>>  /etc/mail/make			--	gen_context(system_u:object_r:bin_t,s0)
>> -/etc/mcelog/.*-error-trigger	--	gen_context(system_u:object_r:bin_t,s0)
>> -/etc/mcelog/.*.local		--	gen_context(system_u:object_r:bin_t,s0)
>> -
>> -ifdef(`distro_redhat',`
>> -/etc/mcelog/triggers(/.*)?		gen_context(system_u:object_r:bin_t,s0)
>> -')
>>  
>>  /etc/mgetty+sendfax/new_fax	--	gen_context(system_u:object_r:bin_t,s0)

Regards,

Guido 

^ permalink raw reply	[flat|nested] 21+ messages in thread