From: guido@trentalancia.com (Guido Trentalancia) Date: Mon, 06 Aug 2012 14:56:09 +0200 Subject: [refpolicy] [PATCH]: mcelog module initial rewrite Message-ID: <201208061256.q76Cu9v2028541@vivaldi20.register.it> To: refpolicy@oss.tresys.com List-Id: refpolicy.oss.tresys.com Hello Dominick. Thanks for your revision. A few notes follows. >On Sun, 2012-08-05 at 22:49 +0200, Guido Trentalancia wrote: >> Obsoltes (or is alternative to) the previous two mcelog patches. >> >> Initial rewrite of mcelog module: >> - version increment >> - fix and extend file contexts (private) >> - support daemon mode and init scripting (+ cron mode untested) >> - support triggers for all distributions, while leaving >> compatibility with their alternate location in Fedora (and >> current policy) >> - initial support for client mode (untested) >> >> Signed-off-by: Guido Trentalancia >> --- >> policy/modules/contrib/mcelog.fc | 15 +++++ >> policy/modules/contrib/mcelog.if | 100 ++++++++++++++++++++++++++++++++++ >> policy/modules/contrib/mcelog.te | 55 +++++++++++++++++- >> policy/modules/kernel/corecommands.fc | 6 -- >> 4 files changed, 167 insertions(+), 9 deletions(-) >> >> 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-05 23:36:37.355678527 +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) > >should probably be bin_t instead of mcelog_exec_t I'd like to confine the type of executable files that mcelog can reach, as that's safer... I have now added the missing can_exec() call, see below... Is there a specific reason for preferring bin_t ? >> + >> +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) > >should probably be bin_t instead of mcelog_exec_t See above. >> +') >> + >> +/etc/rc.d/init.d/mcelog -- gen_context(system_u:object_r:mcelog_initrc_exec_t,s0) > >/etc/rc.d/initd/mcelog (escape the periods) I think the mail application or the copy+paste did something wrong on that... However the entry for mcelog.pid did not bring the escape sequence, so I've fixed it. But I think it's not mandatory, as it also works fine without it and there are other similar entries in current policy (if you want to amend them): /var/run/lsassd.pid -- system_u:object_r:lsassd_var_run_t:s0 /var/run/lwregd.pid -- system_u:object_r:lwregd_var_run_t:s0 >> + >> /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-05 22:58:59.578345741 +0200 >> @@ -18,3 +18,103 @@ interface(`mcelog_domtrans',` >> domtrans_pattern($1, mcelog_exec_t, mcelog_t) >> ') >> >> +######################################## >> +## >> +## Read mcelog_etc_t files (usually >> +## in /etc/mcelog). >> +## >> +## >> +##

>> +## Allow the specified domain to read generic >> +## files in /etc/mcelog. These files are >> +## mcelog configuration files. >> +##

>> +## >> +## >> +## >> +## Domain allowed access. >> +## >> +## >> +interface(`mcelog_read_etc_files',` >> + gen_require(` >> + type mcelog_etc_t; >> + ') >> + > >files_search_etc($1) files_search_etc() should be redundant with read_lnk_files_pattern(). If we want to use files_search_etc(), then we should probably use a different equivalent pattern such as: files_search_etc($1) read_files_pattern($1, etc_file_type_t, etc_file_type_t) allow $1 etc_file_type_t:dir list_dir_perms; So, I have now moved to such pattern, which is probably also easier to read, but please let me know if you still believe it isn't right... >> + allow $1 mcelog_etc_t:dir list_dir_perms; >> + read_files_pattern($1, mcelog_etc_t, mcelog_etc_t) >> + read_lnk_files_pattern($1, mcelog_etc_t, mcelog_etc_t) >> +') >> + >> +######################################## >> +## >> +## Read from an mcelog unix stream socket. >> +## >> +## >> +## >> +## Domain allowed access. >> +## >> +## >> +# >> +interface(`mcelog_read_stream_sockets',` >> + gen_require(` >> + type mcelog_t, mcelog_var_run_t; >> + ') >> + >> + allow $1 mcelog_t:unix_stream_socket { read }; >> +') > >I dont think mcelog_read_stream_sockets is needed Client mode needs to { read connectto } the unix_stream_socket that the daemon eventually created using { write }. Even introducing the stream_connect_pattern that you suggest, it should still need read permissions on it... >> + >> +######################################## >> +## >> +## Read and write to an mcelog unix stream socket. >> +## >> +## >> +## >> +## Domain allowed access. >> +## >> +## >> +# >> +interface(`mcelog_rw_stream_sockets',` >> + gen_require(` >> + type mcelog_t, mcelog_var_run_t; >> + ') >> + >> + allow $1 mcelog_t:unix_stream_socket { read write }; >> +') > >Might above be a leaked file descriptor issue? It's an unused interface, removed. >> + >> +######################################## >> +## >> +## Write to an mcelog unix stream socket. >> +## >> +## >> +## >> +## Domain allowed access. >> +## >> +## >> +# >> +interface(`mcelog_stream_write',` >> + gen_require(` >> + type mcelog_t, mcelog_var_run_t; >> + ') >> + >> + files_search_pids($1) >> + allow $1 mcelog_t:sock_file write; >> +') > >The above isnt needed and wrong (mcelog_t is not a sock_file type), see >below I think you mean it should be mcelog_var_run_t:sock_file and wrong description... However, interfaces and their calls have now been redesigned mostly according to your feedback and that is now gone... >> +######################################## >> +## >> +## Connect to mcelog over an unix stream socket. >> +## >> +## >> +## >> +## Domain allowed access. >> +## >> +## >> +# >> +interface(`mcelog_stream_connect',` >> + gen_require(` >> + type mcelog_t, mcelog_var_run_t; >> + ') >> + >> + files_search_pids($1) >> + allow $1 mcelog_t:unix_stream_socket connectto; > >above is incorrect, use: stream_connect_pattern($1, mcelog_var_run_t, >mcelog_var_run_t, mcelog_t) Ok. >> +') >> 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 00:27:04.614197400 +0200 >> @@ -1,14 +1,37 @@ >> -policy_module(mcelog, 1.1.0) >> +policy_module(mcelog, 1.1.1) >> >> ######################################## >> # >> # Declarations >> # >> >> +## >> +##

>> +## Enable support for mcelog in client mode. >> +##

>> +## >> +gen_tunable(mcelog_client, false) >> + > >No need to make the above conditional It won't be much effective, as users of a generic unconfigured sudo could still access the log file or syslog instead. Removed. >> type mcelog_t; >> type mcelog_exec_t; >> +corecmd_executable_file(mcelog_exec_t); > >no ");" >can probably remove coremd_executable_file(mcelog_exec_t); altogether I have also added can_exec() now. Without them, it won't be able to execute the triggers, given they have been labelled mcelog_exec_t, rather than generic bin_t (see above). >> +init_daemon_domain(mcelog_t, mcelog_exec_t) >> + >> application_domain(mcelog_t, mcelog_exec_t) >no need for this (already enclosed in init_daemon_domain()) Removed. >> cron_system_entry(mcelog_t, mcelog_exec_t) > >Above is optional, wrap in optional policy and move to policy and out of >declarations Done. >> +role system_r types mcelog_t; > >above is redundant , already allowed in init_daemon_domain() Removed. As far as I remember, many other modules still keep it after init_daemon_domain(). If confirmed, we might want to do some maintenance cleaning ? >> + >> +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) >> >> ######################################## >> # >> @@ -22,11 +45,37 @@ kernel_read_system_state(mcelog_t) >> dev_read_raw_memory(mcelog_t) >> dev_read_kmsg(mcelog_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) >> >> +# append to a logfile in a generic var_log_t directory >> +manage_dirs_pattern(mcelog_t, mcelog_log_t, mcelog_log_t) >> +manage_files_pattern(mcelog_t, mcelog_log_t, mcelog_log_t) >> +logging_log_filetrans(mcelog_t, mcelog_log_t, file) > >use: >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) The comment is also wrong (should be create/append). >> + >> +# use syslog functionality (optional, configurable) >> logging_send_syslog_msg(mcelog_t) >> >> +# to read the standard configuration file >> +mcelog_read_etc_files(mcelog_t) >duplicate > > >> +mcelog_stream_write(mcelog_t) > >no needed Since the tunable client_mode has been removed, this is no longer needed as a separate call and therefore included in the modified mcelog_stream_socket_connect() interface. >> +# needed for client mode >> +tunable_policy(`mcelog_client',` >> + mcelog_read_stream_sockets(mcelog_t) >> + mcelog_stream_connect(mcelog_t) >> +') > >not needed, add: > >allow mcelog_t self:unix_stream_socket create_socket_perms; I have created an interface, as we started using them, if you don't mind... >> miscfiles_read_localization(mcelog_t) >> + >> +# for /dev/mem access >> +mls_file_read_all_levels(mcelog_t) >> + >> +term_use_all_ttys(mcelog_t) >> 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) If you leave them there, they would conflict as duplicates with the new private definitions ! >> -ifdef(`distro_redhat',` >> -/etc/mcelog/triggers(/.*)? gen_context(system_u:object_r:bin_t,s0) >> -') See above (duplication conflict with new private definitions). >> /etc/mgetty+sendfax/new_fax -- gen_context(system_u:object_r:bin_t,s0) It also appears to need dev_rw_sysfs(), so I have added it, although I do not know exactly whether this is strictly needed or not... Consider events are rare under normal circumstances (such as normal temperature), so in a certain sense, I am still testing it. A new version has just been posted, but please let me know if you think there is more to be fixed or have any other ideas. Regards, Guido