From: guido@trentalancia.com (Guido Trentalancia)
Date: Mon, 06 Aug 2012 17:38:37 +0200
Subject: [refpolicy] [PATCH]: mcelog module initial rewrite
Message-ID: <201208061538.q76FcbuT031917@vivaldi41.register.it>
To: refpolicy@oss.tresys.com
List-Id: refpolicy.oss.tresys.com

Hello Dominick.

>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

I trust it doesn't work, but at the moment, I have no other mail client.

>> >
>> >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. 

I think create_socket_perms does not allow "connectto", which is what the client needs, but I'll double-check again.

>> 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

I'll double-check that too.

>> 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.

Those redundancies (application_domain and role system_r) might keep getting propagated until they are removed everywhere.

>> >
>> >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.

I'll remove them, but it will be less easier to read.

>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.

Do you believe it won't work that way ?

Regards,

Guido