Re: [PATCH] add lldpd policy

[Dominick Grift <dac.override@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 11929 bytes --]

On Sat, Jun 15, 2019 at 12:08:16PM -0400, Chris PeBenito wrote:
> On 6/10/19 10:20 AM, Alexander Miroshnichenko wrote:
> > New policy for lldpd ( http://vincentbernat.github.io/lldpd ).
> > 
> > Signed-off-by: Alexander Miroshnichenko <alex@millerson.name>
> > ---
> >   policy/modules/roles/sysadm.te   |   4 +
> >   policy/modules/services/lldpd.fc |   9 ++
> >   policy/modules/services/lldpd.if | 206 +++++++++++++++++++++++++++++++
> >   policy/modules/services/lldpd.te |  80 ++++++++++++
> >   4 files changed, 299 insertions(+)
> >   create mode 100644 policy/modules/services/lldpd.fc
> >   create mode 100644 policy/modules/services/lldpd.if
> >   create mode 100644 policy/modules/services/lldpd.te
> > 
> > diff --git a/policy/modules/roles/sysadm.te b/policy/modules/roles/sysadm.te
> > index 8f891c83865f..ea4e06a29e30 100644
> > --- a/policy/modules/roles/sysadm.te
> > +++ b/policy/modules/roles/sysadm.te
> > @@ -595,6 +595,10 @@ optional_policy(`
> >   	lldpad_admin(sysadm_t, sysadm_r)
> >   ')
> > +optional_policy(`
> > +        lldp_admin(sysadm_t, sysadm_r)
> 
> A whitespace problem here (spaces instead of tab).
> 
> > +')
> > +
> >   optional_policy(`
> >   	lockdev_role(sysadm_r, sysadm_t)
> >   ')
> > diff --git a/policy/modules/services/lldpd.fc b/policy/modules/services/lldpd.fc
> > new file mode 100644
> > index 000000000000..997a80a3baf9
> > --- /dev/null
> > +++ b/policy/modules/services/lldpd.fc
> > @@ -0,0 +1,9 @@
> > +/etc/lldpd.conf		--	gen_context(system_u:object_r:lldpd_etc_t,s0)
> > +/etc/lldpd.d(/.*)?		gen_context(system_u:object_r:lldpd_etc_t,s0)
> > +
> > +/usr/sbin/lldpd		--	gen_context(system_u:object_r:lldpd_exec_t,s0)
> > +/usr/sbin/lldpcli         --      gen_context(system_u:object_r:lldp_cli_exec_t,s0)
> > +
> > +/run/lldpd		-d      gen_context(system_u:object_r:lldpd_var_run_t,s0)
> > +/run/lldpd(/.*)?		gen_context(system_u:object_r:lldpd_var_run_t,s0)
> > +/run/lldpd.pid		--	gen_context(system_u:object_r:lldpd_var_run_t,s0)
> > diff --git a/policy/modules/services/lldpd.if b/policy/modules/services/lldpd.if
> > new file mode 100644
> > index 000000000000..f7030b1ead19
> > --- /dev/null
> > +++ b/policy/modules/services/lldpd.if
> > @@ -0,0 +1,206 @@
> > +
> > +## <summary>policy for lldpd</summary>
> > +
> > +########################################
> > +## <summary>
> > +##	Execute lldpd_exec_t in the lldpd domain.
> > +## </summary>
> > +## <param name="domain">
> > +## <summary>
> > +##	Domain allowed to transition.
> > +## </summary>
> > +## </param>
> > +#
> > +interface(`lldpd_domtrans',`
> > +	gen_require(`
> > +		type lldpd_t, lldpd_exec_t;
> > +	')
> > +
> > +	corecmd_search_bin($1)
> > +	domtrans_pattern($1, lldpd_exec_t, lldpd_t)
> > +')
> > +
> > +########################################
> > +## <summary>
> > +##      Execute a domain transition to run lldpcli.
> > +## </summary>
> > +## <param name="domain">
> > +##      <summary>
> > +##      Domain allowed to transition.
> > +##      </summary>
> > +## </param>
> > +#
> > +interface(`lldp_cli_domtrans',`
> 
> Interface name should be lldp_domtrans_cli
> 
> > +        gen_require(`
> > +                type lldp_cli_t, lldp_cli_exec_t;
> > +        ')
> > +
> > +        corecmd_search_bin($1)
> > +        can_exec($1, lldp_cli_exec_t)
> 
> This can_exec should not be in a domtrans interface, as it provides
> execute_no_trans, which isn't necessary for domtrans.
> 
> > +        domtrans_pattern($1, lldp_cli_exec_t, lldp_cli_t)
> > +')
> > +
> > +########################################
> > +## <summary>
> > +##      Execute lldpcli in the lldp_cli domain,
> > +##      and allow the specified role
> > +##      the lldp_cli domain.
> > +## </summary>
> > +## <param name="domain">
> > +##      <summary>
> > +##      Domain allowed to transition.
> > +##      </summary>
> > +## </param>
> > +## <param name="role">
> > +##      <summary>
> > +##      Role allowed access.
> > +##      </summary>
> > +## </param>
> > +#
> > +interface(`lldp_cli_run',`
> 
> lldp_run_cli
> 
> > +        gen_require(`
> > +                type lldp_cli_t;
> > +        ')
> > +
> > +        lldp_cli_domtrans($1)
> > +        role $2 types lldp_cli_t;
> > +')
> > +
> > +######################################
> > +## <summary>
> > +##	Execute lldpd in the caller domain.
> > +## </summary>
> > +## <param name="domain">
> > +##	<summary>
> > +##	Domain allowed access.
> > +##	</summary>
> > +## </param>
> > +#
> > +interface(`lldpd_exec',`
> > +	gen_require(`
> > +		type lldpd_exec_t;
> > +	')
> > +
> > +	corecmd_search_bin($1)
> > +	can_exec($1, lldpd_exec_t)
> > +')
> > +
> > +########################################
> > +## <summary>
> > +##	Search lldpd conf directories.
> > +## </summary>
> > +## <param name="domain">
> > +##	<summary>
> > +##	Domain allowed access.
> > +##	</summary>
> > +## </param>
> > +#
> > +interface(`lldpd_search_conf',`
> > +	gen_require(`
> > +		type lldpd_etc_t;
> > +	')
> > +
> > +	allow $1 lldpd_etc_t:dir search_dir_perms;
> > +	files_search_etc($1)
> > +')
> > +
> > +########################################
> > +## <summary>
> > +##	Read lldpd conf files.
> > +## </summary>
> > +## <param name="domain">
> > +##	<summary>
> > +##	Domain allowed access.
> > +##	</summary>
> > +## </param>
> > +#
> > +interface(`lldpd_read_conf_files',`
> > +	gen_require(`
> > +		type lldpd_etc_t;
> > +	')
> > +
> > +	allow $1 lldpd_etc_t:dir list_dir_perms;
> > +	read_files_pattern($1, lldpd_etc_t, lldpd_etc_t)
> > +	files_search_etc($1)
> > +')
> > +
> > +########################################
> > +## <summary>
> > +##	Manage lldpd conf files.
> > +## </summary>
> > +## <param name="domain">
> > +##	<summary>
> > +##	Domain allowed access.
> > +##	</summary>
> > +## </param>
> > +#
> > +interface(`lldpd_manage_conf_files',`
> > +	gen_require(`
> > +		type lldpd_etc_t;
> > +	')
> > +
> > +	manage_files_pattern($1, lldpd_etc_t, lldpd_etc_t)
> > +	files_search_etc($1)
> > +')
> > +
> > +########################################
> > +## <summary>
> > +##	Create, read, write, and delete
> > +##      lldpd PID files.
> > +## </summary>
> > +## <param name="domain">
> > +##	<summary>
> > +##	Domain allowed access.
> > +##	</summary>
> > +## </param>
> > +#
> > +interface(`lldpd_manage_pid_files',`
> > +	gen_require(`
> > +		type lldpd_var_run_t;
> > +	')
> > +
> > +	files_search_pids($1)
> > +	manage_files_pattern($1, lldpd_var_run_t, lldpd_var_run_t)
> > +	manage_dirs_pattern($1, lldpd_var_run_t, lldpd_var_run_t)
> > +')
> > +
> > +
> > +########################################
> > +## <summary>
> > +##	All of the rules required to administrate
> > +##	an lldpd environment
> > +## </summary>
> > +## <param name="domain">
> > +##	<summary>
> > +##	Domain allowed access.
> > +##	</summary>
> > +## </param>
> > +## <param name="role">
> > +##	<summary>
> > +##	Role allowed access.
> > +##	</summary>
> > +## </param>
> > +## <rolecap/>
> > +#
> > +interface(`lldp_admin',`
> > +	gen_require(`
> > +		type lldpd_t;
> > +		type lldpd_etc_t;
> > +		type lldpd_var_run_t;
> > +	')
> > +
> > +	allow $1 lldpd_t:process { signal_perms };
> > +	ps_process_pattern($1, lldpd_t)
> > +
> > +    tunable_policy(`allow_ptrace',`
> > +        allow $1 lldpd_t:process ptrace;
> > +    ')
> 
> allow_ptrace is not a global tunable and cannot be used here.  Also there
> are whitespace problems.
> 
> > +	files_search_etc($1)
> > +	admin_pattern($1, lldpd_etc_t)
> > +
> > +	files_search_pids($1)
> > +	admin_pattern($1, lldpd_var_run_t)
> > +
> > +	lldp_cli_run($1, $2)
> > +')
> > diff --git a/policy/modules/services/lldpd.te b/policy/modules/services/lldpd.te
> > new file mode 100644
> > index 000000000000..9a0f68dc4b7b
> > --- /dev/null
> > +++ b/policy/modules/services/lldpd.te
> > @@ -0,0 +1,80 @@
> > +policy_module(lldpd, 1.0.0)
> > +
> > +########################################
> > +#
> > +# Declarations
> > +#
> > +
> > +type lldpd_t;
> > +type lldpd_exec_t;
> > +init_daemon_domain(lldpd_t, lldpd_exec_t)
> > +
> > +type lldp_cli_t;
> > +type lldp_cli_exec_t;
> > +init_system_domain(lldp_cli_t, lldp_cli_exec_t)
> > +application_domain(lldp_cli_t, lldp_cli_exec_t)
> > +
> > +type lldpd_etc_t;
> 
> Please rename to lldpd_conf_t, as I'd like to try to get away from encoding
> paths into type names.
> 
> > +files_config_file(lldpd_etc_t)
> > +
> > +type lldpd_var_run_t;
> 
> Same thing here, lldpd_runtime_t.
> 
> 
> > +files_pid_file(lldpd_var_run_t)
> > +init_daemon_pid_file(lldpd_var_run_t, dir, "lldpd")
> > +typealias lldpd_var_run_t alias lldp_sock_t;
> 
> Not really a necessary alias.  I'd prefer to keep aliases for backwards
> compatibility situations.
> 
> 
> > +
> > +########################################
> > +#
> > +# lldpd local policy
> > +#
> > +allow lldpd_t self:capability { chown dac_override fowner fsetid kill net_admin net_raw setgid setuid sys_chroot };
> > +allow lldpd_t self:process { fork signal_perms };
> > +allow lldpd_t self:fifo_file rw_fifo_file_perms;
> > +allow lldpd_t self:unix_stream_socket { accept listen };
> 
> These perms should probably be create_stream_socket_perms.

the other permissions are already provided with logging_send_syslog_msg() so would be reduntant

> 
> 
> > +allow lldpd_t lldp_sock_t:sock_file { create_sock_file_perms delete_sock_file_perms setattr };
> 
> This is not necessary, as there is a sock_file rule below.
> 
> 
> > +allow lldpd_t self:packet_socket create_socket_perms;
> > +
> > +lldp_cli_domtrans(lldpd_t)
> 
> The daemon runs the cli tool?
> 
> 
> 
> > +kernel_read_net_sysctls(lldpd_t)
> > +
> > +lldpd_read_conf_files(lldpd_t)
> > +
> > +lldpd_manage_pid_files(lldpd_t)
> 
> Since there are other rules that explicitly operate on lldpd_var_run_t, it
> would be clearer to do the same for files instead of calling its own
> interface.
> 
> > +manage_sock_files_pattern(lldpd_t, lldpd_var_run_t, lldpd_var_run_t)
> > +manage_lnk_files_pattern(lldpd_t, lldpd_var_run_t, lldpd_var_run_t)
> > +files_pid_filetrans(lldpd_t, lldpd_var_run_t, {file dir sock_file})
> > +
> > +domain_use_interactive_fds(lldpd_t)
> 
> This does not seem likely since it is a daemon, not an interactive process.
> 
> 
> > +files_read_etc_files(lldpd_t)
> > +
> > +logging_send_syslog_msg(lldpd_t)
> > +
> > +miscfiles_read_localization(lldpd_t)
> > +
> > +sysnet_dns_name_resolve(lldpd_t)
> > +
> > +########################################
> > +#
> > +# lldp_cli local policy
> > +#
> > +allow lldp_cli_t self:capability dac_override;
> > +allow lldp_cli_t self:unix_dgram_socket { connect create };
> > +allow lldp_cli_t self:unix_stream_socket { connect create read write };
> > +allow lldp_cli_t self:process signal;
> > +
> > +allow lldp_cli_t lldpd_t:unix_stream_socket connectto;
> > +allow lldp_cli_t lldpd_var_run_t:sock_file { read write };
> 
> Please use stream_connect_pattern()
> 
> 
> > +
> > +lldpd_read_conf_files(lldp_cli_t)
> > +
> > +logging_send_syslog_msg(lldp_cli_t)
> > +
> > +files_dontaudit_read_etc_files(lldp_cli_t)
> > +
> > +miscfiles_read_localization(lldp_cli_t)
> > +
> > +domain_use_interactive_fds(lldp_cli_t)
> 
> This line is in the wrong place.
> 
> > +userdom_use_user_ptys(lldp_cli_t)
> 
> > +init_dontaudit_use_script_ptys(lldp_cli_t)
> 
> This should not be necessary, as this is allowed via init_system_domain().
> 
> 
> 
> -- 
> Chris PeBenito

-- 
Key fingerprint = 5F4D 3CDB D3F8 3652 FBD8 02D5 3B6C 5F1D 2C7B 6B02
https://sks-keyservers.net/pks/lookup?op=get&search=0x3B6C5F1D2C7B6B02
Dominick Grift

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]