Received-SPF: pass (google.com: best guess record for domain of selinux-refpolicy-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 15 Jun 2019 19:58:59 +0200
From:   Dominick Grift <dac.override@gmail.com>
To:     Chris PeBenito <pebenito@ieee.org>
Cc:     Alexander Miroshnichenko <alex@millerson.name>,
        selinux-refpolicy@vger.kernel.org
Subject: Re: [PATCH] add lldpd policy
Message-ID: <20190615175859.GA2818@brutus.lan>
Mail-Followup-To: Chris PeBenito <pebenito@ieee.org>,
        Alexander Miroshnichenko <alex@millerson.name>,
        selinux-refpolicy@vger.kernel.org
References: <20190610142004.2719-1-alex@millerson.name>
 <749388e0-6da1-4b06-c62c-35302a5aba78@ieee.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="HlL+5n6rz5pIUxbD"
Content-Disposition: inline
In-Reply-To: <749388e0-6da1-4b06-c62c-35302a5aba78@ieee.org>
User-Agent: Every email client sucks, this one just sucks less.
Sender: selinux-refpolicy-owner@vger.kernel.org
Precedence: bulk


--HlL+5n6rz5pIUxbD
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 ).
> >=20
> > 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
> >=20
> > diff --git a/policy/modules/roles/sysadm.te b/policy/modules/roles/sysa=
dm.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)
>=20
> A whitespace problem here (spaces instead of tab).
>=20
> > +')
> > +
> >   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_c=
li_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=3D"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=3D"domain">
> > +##      <summary>
> > +##      Domain allowed to transition.
> > +##      </summary>
> > +## </param>
> > +#
> > +interface(`lldp_cli_domtrans',`
>=20
> Interface name should be lldp_domtrans_cli
>=20
> > +        gen_require(`
> > +                type lldp_cli_t, lldp_cli_exec_t;
> > +        ')
> > +
> > +        corecmd_search_bin($1)
> > +        can_exec($1, lldp_cli_exec_t)
>=20
> This can_exec should not be in a domtrans interface, as it provides
> execute_no_trans, which isn't necessary for domtrans.
>=20
> > +        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=3D"domain">
> > +##      <summary>
> > +##      Domain allowed to transition.
> > +##      </summary>
> > +## </param>
> > +## <param name=3D"role">
> > +##      <summary>
> > +##      Role allowed access.
> > +##      </summary>
> > +## </param>
> > +#
> > +interface(`lldp_cli_run',`
>=20
> lldp_run_cli
>=20
> > +        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=3D"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=3D"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=3D"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=3D"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=3D"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=3D"domain">
> > +##	<summary>
> > +##	Domain allowed access.
> > +##	</summary>
> > +## </param>
> > +## <param name=3D"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;
> > +    ')
>=20
> allow_ptrace is not a global tunable and cannot be used here.  Also there
> are whitespace problems.
>=20
> > +	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;
>=20
> Please rename to lldpd_conf_t, as I'd like to try to get away from encodi=
ng
> paths into type names.
>=20
> > +files_config_file(lldpd_etc_t)
> > +
> > +type lldpd_var_run_t;
>=20
> Same thing here, lldpd_runtime_t.
>=20
>=20
> > +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;
>=20
> Not really a necessary alias.  I'd prefer to keep aliases for backwards
> compatibility situations.
>=20
>=20
> > +
> > +########################################
> > +#
> > +# 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 };
>=20
> These perms should probably be create_stream_socket_perms.

the other permissions are already provided with logging_send_syslog_msg() s=
o would be reduntant

>=20
>=20
> > +allow lldpd_t lldp_sock_t:sock_file { create_sock_file_perms delete_so=
ck_file_perms setattr };
>=20
> This is not necessary, as there is a sock_file rule below.
>=20
>=20
> > +allow lldpd_t self:packet_socket create_socket_perms;
> > +
> > +lldp_cli_domtrans(lldpd_t)
>=20
> The daemon runs the cli tool?
>=20
>=20
>=20
> > +kernel_read_net_sysctls(lldpd_t)
> > +
> > +lldpd_read_conf_files(lldpd_t)
> > +
> > +lldpd_manage_pid_files(lldpd_t)
>=20
> 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.
>=20
> > +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)
>=20
> This does not seem likely since it is a daemon, not an interactive proces=
s.
>=20
>=20
> > +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 };
>=20
> Please use stream_connect_pattern()
>=20
>=20
> > +
> > +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)
>=20
> This line is in the wrong place.
>=20
> > +userdom_use_user_ptys(lldp_cli_t)
>=20
> > +init_dontaudit_use_script_ptys(lldp_cli_t)
>=20
> This should not be necessary, as this is allowed via init_system_domain().
>=20
>=20
>=20
> --=20
> Chris PeBenito

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

--HlL+5n6rz5pIUxbD
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQGzBAEBCAAdFiEEujmXliIBLFTc2Y4AJXSOVTf5R2kFAl0FMd8ACgkQJXSOVTf5
R2noDAv9FjLrp+3DP71gb2HUlRpwl30R9Hq/REcaelwG0B6RHZBcQn6TtNETqQup
9qfeAeXwORnemcnLfYIvXdP54x7mQjZ25YqWzIcmgbzKmEgOqlSELQdXkSfkIxKh
arc++/lmxChV1d76lPSmgpFJcXBOy5kchooxs2+gTW0ewvwV9a57/Y+k8L6hv4M+
o8Fo8aI1+Q2JPOCTphDL5lmyeN0I/yoRoKuQ1c+Xsb+X3E23lO+FqKbEiOXcR0/x
yGN0WKkwwJSxSmuiwVX1LgzUdV9WQAsbquhPzIr8qgCO4155PCST1cBeryrxrUbd
bysKdSvK00ursKSGs8SoGIhN3vy0AsBtNHOIiszwP2JOUvD1Q9IWlBhr5yBOcEfq
ExMNiOqnHwmdtLPFyx9sLEcAiBj4iA3nf+Rx0W0q5lEr9hdDPRP18/bdLLKG0rtC
ESE/8iJIQZCo/g0cbSIqRlKRSwtmbIQecEu9CgRfprB/kqEYm11C/nghpX5Dinq4
idnwx+pT
=4Tem
-----END PGP SIGNATURE-----

--HlL+5n6rz5pIUxbD--