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 <[email protected]>
>> ---
>> 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)
>> ')
>>
>> +########################################
>> +## <summary>
>> +## Read mcelog_etc_t files (usually
>> +## in /etc/mcelog).
>> +## </summary>
>> +## <desc>
>> +## <p>
>> +## Allow the specified domain to read generic
>> +## files in /etc/mcelog. These files are
>> +## mcelog configuration files.
>> +## </p>
>> +## </desc>
>> +## <param name="domain">
>> +## <summary>
>> +## Domain allowed access.
>> +## </summary>
>> +## </param>
>> +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)
>> +')
>> +
>> +########################################
>> +## <summary>
>> +## Read from an mcelog unix stream socket.
>> +## </summary>
>> +## <param name="domain">
>> +## <summary>
>> +## Domain allowed access.
>> +## </summary>
>> +## </param>
>> +#
>> +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...
>> +
>> +########################################
>> +## <summary>
>> +## Read and write to an mcelog unix stream socket.
>> +## </summary>
>> +## <param name="domain">
>> +## <summary>
>> +## Domain allowed access.
>> +## </summary>
>> +## </param>
>> +#
>> +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.
>> +
>> +########################################
>> +## <summary>
>> +## Write to an mcelog unix stream socket.
>> +## </summary>
>> +## <param name="domain">
>> +## <summary>
>> +## Domain allowed access.
>> +## </summary>
>> +## </param>
>> +#
>> +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...
>> +########################################
>> +## <summary>
>> +## Connect to mcelog over an unix stream socket.
>> +## </summary>
>> +## <param name="domain">
>> +## <summary>
>> +## Domain allowed access.
>> +## </summary>
>> +## </param>
>> +#
>> +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
>> #
>>
>> +## <desc>
>> +## <p>
>> +## Enable support for mcelog in client mode.
>> +## </p>
>> +## </desc>
>> +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
On Mon, 2012-08-06 at 14:56 +0200, Guido Trentalancia wrote:
> >> 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 ?
Generally things like this add complexity with no real benefit. In this
case since the files are in /etc anyways that argument may not apply.
> >/etc/rc.d/initd/mcelog (escape the periods)
hmm yes seems like your mail app screw it up.
> 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.
it only seem tp apply on periods in directories. So the period in the
mcelog.pid file should work without escaping 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
For the /etc/rc\.d/init\.d/mcelog it is needed, alteast last time i
tried.
try it out, with and without escaping the periods and see if
matchpathcon catches /etc/rc.d/init.d/mcelog
> >
> >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...
I dont agree, i would just add files_search_etc($1) and get it over with
> Client mode needs to { read connectto } the unix_stream_socket that the daemon eventually created using { write }.
That should be allowed with allow mcelog_t self:unix_stream_socket
create_socket_perms; i believe as ive suggested below.
> Even introducing the stream_connect_pattern that you suggest, it should still need read permissions on it...
There is no need for a mcelog_stream_connect_pattern() interface in the
first place. Interfaces are not for internal use.
> >
> >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...
Yes, but even so, its still not needed. interfaces shouldnt be used
internally
>
> However, interfaces and their calls have now been redesigned mostly according to your feedback and that is now gone...
> 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).
I still thinks its not needed but i may be wrong init_daemon_domain()
makes mcelog_exec_t a application executable file automatically
> 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 ?
Yes some.
> >
> >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...
i do mind ;)
interfaces shouldnt be used internally.
mcelog_t already has full access to mcelog_var_run_t
you just need to add:
allow mcelog_t self:unix_stream_socket create_socket_perms; (or was it
create_stream_socket_perms? i am not sure, have a look)
>
> >> 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 !
i would probably leave it bin_t. i dont have a compelling argument for
it but i dont have a compelling argument for changing it to
mcelog_exec_t either.
On 08/06/12 09:32, Dominick Grift wrote:
> On Mon, 2012-08-06 at 14:56 +0200, Guido Trentalancia wrote:
>>>> 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 ?
>
> Generally things like this add complexity with no real benefit. In this
> case since the files are in /etc anyways that argument may not apply.
If the files aren't actually meant to be entrypoints for the domain, then bin_t is more appropriate. Or if they really for some reason needed to be isolated, they'd be something like mcelog_helper_exec_t. If someone is meant to modify those (if they're scripts) then the latter might make more sense.
--
Chris PeBenito
Tresys Technology, LLC
http://www.tresys.com | oss.tresys.com