From: dominick.grift@gmail.com (Dominick Grift)
Date: Wed, 08 Aug 2012 13:46:54 +0200
Subject: [refpolicy] [PATCH] Initial BIRD Internet Routing Daemon policy
In-Reply-To: <1344426166.2306.31.camel@d30.localdomain>
References: <1344415924-27382-1-git-send-email-dominick.grift@gmail.com>
	<5022443F.2040601@trentalancia.com>
	<1344426166.2306.31.camel@d30.localdomain>
Message-ID: <1344426414.2306.33.camel@d30.localdomain>
To: refpolicy@oss.tresys.com
List-Id: refpolicy.oss.tresys.com



On Wed, 2012-08-08 at 13:42 +0200, Dominick Grift wrote:
> 
> On Wed, 2012-08-08 at 12:49 +0200, Guido Trentalancia wrote:
> > Hello Dominick.
> > 
> > A very quick and possibly incomplete review, if it helps...
> > 
> > On 08/08/2012 10:52, Dominick Grift wrote:
> > >
> > > Signed-off-by: Dominick Grift <dominick.grift@gmail.com>
> > > diff --git a/bird.fc b/bird.fc
> > > new file mode 100644
> > > index 0000000..7b63b8e
> > > --- /dev/null
> > > +++ b/bird.fc
> > > @@ -0,0 +1,11 @@
> > > +/etc/bird\.conf	--	gen_context(system_u:object_r:bird_etc_t,s0)
> > > +
> > > +/etc/default/bird	--	gen_context(system_u:object_r:bird_etc_t,s0)
> > > +
> > > +/etc/rc\.d/init\.d/bird	--	gen_context(system_u:object_r:bird_initrc_exec_t,s0)
> > 
> > You might want to support init script locations for other distributions 
> > here, as in the oident module that you proposed to modify yesterday (I 
> > am going to modify the mcelog too for this purpose).
> > 
> > Debian (but also Gentoo and many others) are currently using /etc/init\.d.
> > 
> > The rest is unlikely to change, if it does, it's their business to 
> > modify the contexts, I think.
> 
> 
> You have a good point and i have been thinking abou this issue
> obviously. I decided to go this way because existing init daemons also
> only have the /etc/rc.d/init.d and not the /etc/init.d.
> 
> Maybe a better solution is to just add:
> 
> /etc/init.d /etc/rc.d/init.d
> 
> to file_contexts.subs_dist
> 
> > > +/usr/sbin/bird	--	gen_context(system_u:object_r:bird_exec_t,s0)
> > > +
> > > +/var/log/bird\.log.*	--	gen_context(system_u:object_r:bird_log_t,s0)
> > > +
> > > +/var/run/bird\.ctl	-s	gen_context(system_u:object_r:bird_var_run_t,s0)
> > > diff --git a/bird.if b/bird.if
> > > new file mode 100644
> > > index 0000000..fae3f36
> > > --- /dev/null
> > > +++ b/bird.if
> > > @@ -0,0 +1,42 @@
> > > +## <summary>BIRD Internet Routing Daemon.</summary>
> > > +
> > > +########################################
> > > +## <summary>
> > > +##	All of the rules required to administrate
> > > +##	an bird environment.
> > 
> > I think "Rules to administrate a BIRD environment" would read a bit 
> > better, but clearly it's not critical.
> 
> good point. However for the sake of uniformity ill just keep it like
> this. (this is the common summary for this interface)

s/uniformity/consistency

> > > +## </summary>
> > > +## <param name="domain">
> > > +##	<summary>
> > > +##	Domain allowed access.
> > > +##	</summary>
> > > +## </param>
> > > +## <param name="role">
> > > +##	<summary>
> > > +##	Role allowed access..
> > > +##	</summary>
> > > +## </param>
> > > +## <rolecap/>
> > > +#
> > > +interface(`bird_admin',`
> > > +	gen_require(`
> > > +		type bird_t, bird_etc_t, bird_log_t;
> > > +		type bird_var_run_t, bird_initrc_exec_t;
> > > +	')
> > > +
> > > +	allow $1 bird_t:process { ptrace signal_perms };
> > > +	ps_process_pattern($1, bird_t)
> > > +
> > > +	init_labeled_script_domtrans($1, bird_initrc_exec_t)
> > > +	domain_system_change_exemption($1)
> > > +	role_transition $2 bird_initrc_exec_t system_r;
> > > +	allow $2 system_r;
> > > +
> > > +	files_list_etc($1)
> > > +	admin_pattern($1, bird_etc_t)
> > > +
> > > +	logging_list_logs($1)
> > > +	admin_pattern($1, bird_log_t)
> > > +
> > > +	files_list_pids($1)
> > > +	admin_pattern($1, bird_var_run_t)
> > > +')
> > 
> > Not checked (see below).
> > 
> > > diff --git a/bird.te b/bird.te
> > > new file mode 100644
> > > index 0000000..9afd52b
> > > --- /dev/null
> > > +++ b/bird.te
> > > @@ -0,0 +1,57 @@
> > > +policy_module(bird, 1.0.0)
> > > +
> > > +########################################
> > > +#
> > > +# Declarations
> > > +#
> > > +
> > > +type bird_t;
> > > +type bird_exec_t;
> > > +init_daemon_domain(bird_t, bird_exec_t)
> > > +
> > > +type bird_initrc_exec_t;
> > > +init_script_file(bird_initrc_exec_t)
> > > +
> > > +type bird_etc_t;
> > > +files_config_file(bird_etc_t)
> > > +
> > > +type bird_log_t;
> > > +logging_log_file(bird_log_t)
> > > +
> > > +type bird_var_run_t;
> > > +files_pid_file(bird_var_run_t)
> > > +
> > > +########################################
> > > +#
> > > +# Local policy
> > > +#
> > > +
> > > +allow bird_t self:capability { net_admin net_bind_service };
> > > +allow bird_t self:netlink_route_socket create_netlink_socket_perms;
> > > +allow bird_t self:tcp_socket create_stream_socket_perms;
> > > +
> > > +allow bird_t bird_etc_t:file read_file_perms;
> > 
> > Use of patterns for reading, writing or managing standard files is more 
> > common throughout the whole reference policy. It's not critical but 
> > think about it, it might improve readability and coherence across the 
> > work as a whole...
> 
> This is better in my view. The patterns provide permissions that are not
> needed here. i.e. read_files_pattern(bird_t, bird_etc_t , bird_etc_t)
> allow bird_t to search bird_etc_t dirs but there aren't any.
> 
> > > +allow bird_t bird_log_t:file { create_file_perms append_file_perms setattr_file_perms };
> > 
> > I think create+append+setattr grants more permissions than the 
> > manage_file_perms (or preferably corrispondent pattern), however I have 
> > not checked.
> > 
> 
> No, This combination make it possible to not allow bird_T to "write"
> bird_log_t files which is a real benefit over manage_file_perms.
> 
> > See above for using patterns versus individual or grouped permissions.
> > 
> > > +logging_log_filetrans(bird_t, bird_log_t, file)
> > > +
> > > +allow bird_t bird_var_run_t:sock_file manage_sock_file_perms;
> > 
> > See above for using patterns versus individual or grouped permissions.
> > 
> > > +files_pid_filetrans(bird_t, bird_var_run_t, sock_file)
> > > +
> > > +corenet_all_recvfrom_unlabeled(bird_t)
> > 
> > Unlabelled receive could probably made tunable as not everybody might 
> > like that.
> 
> unlabeled is a default in systems. The boolean would be have to be on by
> default. On other systems this rules would harm since the packets would
> be labeled.


would *not* harm

> and for the sake of uniformity i will leave it like this (other existing
> modules also dont have this conditional)


s/uniformity/consistency

> > > +corenet_all_recvfrom_netlabel(bird_t)
> > > +corenet_tcp_sendrecv_generic_if(bird_t)
> > > +corenet_tcp_bind_generic_node(bird_t)
> > > +corenet_tcp_sendrecv_generic_node(bird_t)
> > > +corenet_tcp_sendrecv_bgp_port(bird_t)
> > > +corenet_sendrecv_bgp_client_packets(bird_t)
> > > +corenet_tcp_connect_bgp_port(bird_t)
> > > +corenet_sendrecv_bgp_server_packets(bird_t)
> > > +corenet_tcp_bind_bgp_port(bird_t)
> > > +
> > > +# /etc/iproute2/rt_realms
> > > +files_read_etc_files(bird_t)
> > > +
> > > +logging_send_syslog_msg(bird_t)
> > > +
> > > +miscfiles_read_localization(bird_t)
> > 
> > You have not used the bird_admin() interface, since it's not a simple 
> > and generic interface (such as for managing files), you should really 
> > remove it in my opinion.
> 
> True and in general it is a good argument to not create unused
> interfaces. However this is a exception to that rules. We want end-users
> to be able to easily create a birdadm_r role. This interface facilitates
> that. If we would create a role for each init daemon by default we would
> end up with too many roles.
> 
> So the middle road is to not create the role by default but to provide
> the interface so that endusers can easily create it if they so desire.
> 
> > The most important reason is that permissions needed by BIRD might 
> > change with subsequent releases of the daemon by the time somebody will 
> > actually use the bird_admin() interface: if they shrink, it's a security 
> > matter (interface is loose) and you don't be doing much of a favour to 
> > anyone.
> 
> This argument is not valid in light of my comment above.
> 
> > Regards,
> > 
> > Guido
> 
>