From: dominick.grift@gmail.com (Dominick Grift) Date: Mon, 06 Aug 2012 15:32:44 +0200 Subject: [refpolicy] [PATCH]: mcelog module initial rewrite In-Reply-To: <201208061256.q76Cu9v2028541@vivaldi20.register.it> References: <201208061256.q76Cu9v2028541@vivaldi20.register.it> Message-ID: <1344259964.29329.31.camel@d30.localdomain> To: refpolicy@oss.tresys.com List-Id: refpolicy.oss.tresys.com 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.