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:   Tue, 2 Jul 2019 17:58:23 +0200
From:   Dominick Grift <dac.override@gmail.com>
To:     Alexander Miroshnichenko <alex@millerson.name>
Cc:     selinux-refpolicy@vger.kernel.org
Subject: Re: [PATCH] Add knot module
Message-ID: <20190702155823.GA27193@brutus.lan>
Mail-Followup-To: Alexander Miroshnichenko <alex@millerson.name>,
        selinux-refpolicy@vger.kernel.org
References: <20190702125559.15631-1-alex@millerson.name>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="bg08WKrSYDhXBjb5"
Content-Disposition: inline
In-Reply-To: <20190702125559.15631-1-alex@millerson.name>
User-Agent: Every email client sucks, this one just sucks less.
Sender: selinux-refpolicy-owner@vger.kernel.org
Precedence: bulk


--bg08WKrSYDhXBjb5
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Jul 02, 2019 at 03:55:59PM +0300, Alexander Miroshnichenko wrote:
> Add a SELinux Reference Policy module for the
> Knot authoritative-only DNS server.

Some observations in line below

>=20
> Signed-off-by: Alexander Miroshnichenko <alex@millerson.name>
> ---
>  policy/modules/roles/sysadm.te  |   4 +
>  policy/modules/services/knot.fc |  11 +++
>  policy/modules/services/knot.if | 156 ++++++++++++++++++++++++++++++++
>  policy/modules/services/knot.te |  92 +++++++++++++++++++
>  4 files changed, 263 insertions(+)
>  create mode 100644 policy/modules/services/knot.fc
>  create mode 100644 policy/modules/services/knot.if
>  create mode 100644 policy/modules/services/knot.te
>=20
> diff --git a/policy/modules/roles/sysadm.te b/policy/modules/roles/sysadm=
=2Ete
> index 8f891c83865f..e3079ad65d17 100644
> --- a/policy/modules/roles/sysadm.te
> +++ b/policy/modules/roles/sysadm.te
> @@ -550,6 +550,10 @@ optional_policy(`
>  	keystone_admin(sysadm_t, sysadm_r)
>  ')
> =20
> +optional_policy(`
> +	knotc_role(sysadm_r, sysadm_t)
> +')
> +
>  optional_policy(`
>  	kismet_admin(sysadm_t, sysadm_r)
>  ')
> diff --git a/policy/modules/services/knot.fc b/policy/modules/services/kn=
ot.fc
> new file mode 100644
> index 000000000000..a809fbc72b14
> --- /dev/null
> +++ b/policy/modules/services/knot.fc
> @@ -0,0 +1,11 @@
> +/etc/knot(/.*)?		gen_context(system_u:object_r:knot_conf_t,s0)
> +
> +/usr/sbin/knotd		--	gen_context(system_u:object_r:knotd_exec_t,s0)
> +
> +/usr/sbin/knotc		--      gen_context(system_u:object_r:knotc_exec_t,s0)
> +
> +/var/lib/knot(/.*)?	gen_context(system_u:object_r:knot_var_lib_t,s0)
> +
> +/run/knot		-d	gen_context(system_u:object_r:knot_runtime_t,s0)

redundant, fc spec below covers this

> +
> +/run/knot(/.*)?		gen_context(system_u:object_r:knot_runtime_t,s0)
> diff --git a/policy/modules/services/knot.if b/policy/modules/services/kn=
ot.if
> new file mode 100644
> index 000000000000..71eec0c9c1e3
> --- /dev/null
> +++ b/policy/modules/services/knot.if
> @@ -0,0 +1,156 @@
> +
> +## <summary>policy for knotc</summary>

"high-performance authoritative-only DNS server."

> +
> +########################################
> +## <summary>
> +##	Execute knotd_exec_t in the knotd domain.
> +## </summary>
> +## <param name=3D"domain">
> +## <summary>
> +##	Domain allowed to transition.
> +## </summary>
> +## </param>
> +#
> +interface(`knotd_domtrans',`
> +	gen_require(`
> +		type knotd_t, knotd_exec_t;
> +	')
> +
> +	corecmd_search_bin($1)
> +	domtrans_pattern($1, knotd_exec_t, knotd_t)
> +')
> +
> +########################################
> +## <summary>
> +##      Manage knot runtime files.

Manage Knot runtime (its not just files)

> +## </summary>
> +## <param name=3D"domain">
> +##      <summary>
> +##      Domain allowed access.
> +##      </summary>
> +## </param>
> +#
> +interface(`knot_manage_runtime_files',`

knot_manage_runtime (its not just files)

> +	gen_require(`
> +		type knot_runtime_t;
> +		type var_run_t;

not allowed to reference external types. also its not used anyway

> +	')
> +
> +	manage_dirs_pattern($1, knot_runtime_t, knot_runtime_t)
> +	manage_files_pattern($1, knot_runtime_t, knot_runtime_t)
> +	manage_lnk_files_pattern($1, knot_runtime_t, knot_runtime_t)
> +	manage_sock_files_pattern($1, knot_runtime_t, knot_runtime_t)

Add files_search_pids($1) to allow callers to traverse /run (you cannot "ma=
nage knot runtime" if you cannot traverse /run)

> +	search_dirs_pattern($1, knot_runtime_t, knot_runtime_t)

redundant as the manage_dirs_pattern($1, knot_runtime_t, knot_runtime_t) ab=
ove already covers this

> +	files_pid_filetrans($1, knot_runtime_t, { file dir sock_file})

This does not belong in this interface. you would create a seperate knot_ru=
ntime_filetrans() instead. Also you can remove the "file" and "sock_file" h=
ere. Everything is inside /run/knot (as per the fc spec above)

> +')
> +
> +########################################
> +## <summary>
> +##      Knot /var/lib files mamange.

cannot parse "mamange" use "Knot manage var lib."

> +## </summary>
> +## <param name=3D"domain">
> +##      <summary>
> +##      Domain allowed access.
> +##      </summary>
> +## </param>
> +#
> +interface(`knot_manage_var_lib_files',`

knot_manage_var_lib (its not just files)

> +	gen_require(`
> +		type knot_var_lib_t;
> +	')
> +
> +	manage_dirs_pattern($1, knot_var_lib_t, knot_var_lib_t)
> +	manage_files_pattern($1, knot_var_lib_t, knot_var_lib_t)
> +	manage_lnk_files_pattern($1, knot_var_lib_t, knot_var_lib_t)

Add files_search_var_lib($1) because caller cannot "manage Knot var lib" if=
 it cannot traverse /var/lib

> +	allow $1 knot_var_lib_t:file map;

this (probably) does not belong here: you would create a knot_mmap_var_lib_=
files() instead, i suppose)

> +	files_var_lib_filetrans($1, knot_var_lib_t, { file dir })

This does not belong here. you would create a knot_var_lib_filetrans() inst=
ead.

> +')
> +
> +########################################
> +## <summary>
> +##      Knot /etc/knot files read.

knot_read_config_files (as read_files_pattern($1, knot_conf_t, knot_conf_t)=
 only allows for reading files)

> +## </summary>
> +## <param name=3D"domain">
> +##      <summary>
> +##      Domain allowed access.
> +##      </summary>
> +## </param>
> +#
> +interface(`knot_read_conf',`

knot_read_config_files (because this interface allows callers to only "read=
" knot_conf_t files (not dirs or anything else)

> +	gen_require(`
> +		type knot_conf_t;
> +		type initrc_t;

This does not belong here. not allowed to reference external types directly

> +	')
> +
> +	mmap_read_files_pattern($1, knot_conf_t, knot_conf_t)

this does not belong here, if you need map then create a knot_mmap_config_f=
ile()

> +	read_files_pattern(initrc_t, knot_conf_t, knot_conf_t)

Youre not allowed to reference initrc_t here. Use instead: read_files_patte=
rn($1, knot_conf_t, knot_conf_t)

Add files_search_etc($1) to allow callers to traverse /etc

> +')
> +
> +########################################
> +## <summary>
> +##      Manage knot temporary files.

Manage Knot tmp (this interface also allows caller to manage knot_tmp_t dir=
s, so "files" is not accurate)

> +## </summary>
> +## <param name=3D"domain">
> +##      <summary>
> +##      Domain allowed access.
> +##      </summary>
> +## </param>
> +#
> +interface(`knot_manage_tmpfs_files',`

knot_manage_tmp (this also allows managing dirs)

> +	gen_require(`
> +		type knot_tmp_t;
> +	')
> +
> +	files_tmp_filetrans($1, knot_tmp_t, { file dir })

You would create a knot_tmp_filetrans for the above, and "file" can be remo=
ved as everything is in the dir.

> +	allow $1 knot_tmp_t:file map;

This probably does not belong here you would create a knot_mmap_tmp_files()=
 instead.

> +	allow $1 knot_tmp_t:file manage_file_perms;
> +	allow $1 knot_tmp_t:dir manage_dir_perms;
> +')
> +
> +########################################
> +## <summary>
> +##      Execute knotc_exec_t in the knotc domain.

"Execute knotc in the knotc domain". Ones does not "execute" types

> +## </summary>
> +## <param name=3D"domain">
> +## <summary>
> +##      Domain allowed to transition.
> +## </summary>
> +## </param>
> +#
> +interface(`knotc_domtrans',`
> +	gen_require(`
> +		type knotc_t, knotc_exec_t;
> +	')
> +
> +	corecmd_search_bin($1)
> +	domtrans_pattern($1, knotc_exec_t, knotc_t)
> +')
> +
> +########################################
> +## <summary>
> +##      Role access for knotc
> +## </summary>
> +## <param name=3D"role">
> +##      <summary>
> +##      Role allowed access
> +##      </summary>
> +## </param>
> +## <param name=3D"domain">
> +##      <summary>
> +##      User domain for the role
> +##      </summary>
> +## </param>
> +#
> +interface(`knotc_role',`
> +	gen_require(`
> +		type knotc_t;
> +		attribute_role knotc_roles;
> +	')
> +
> +	roleattribute $1 knotc_roles;
> +
> +	knotc_domtrans($2)
> +
> +	ps_process_pattern($2, knotc_t)
> +	allow $2 knotc_t:process { signull signal sigkill };
> +')
> diff --git a/policy/modules/services/knot.te b/policy/modules/services/kn=
ot.te
> new file mode 100644
> index 000000000000..d96b7bf4ce98
> --- /dev/null
> +++ b/policy/modules/services/knot.te
> @@ -0,0 +1,92 @@
> +policy_module(knot, 1.0.0)
> +
> +########################################
> +#
> +# Declarations
> +#
> +
> +type knotd_t;
> +type knotd_exec_t;
> +init_daemon_domain(knotd_t, knotd_exec_t)
> +
> +type knotc_t;
> +type knotc_exec_t;
> +application_domain(knotc_t, knotc_exec_t)
> +init_daemon_domain(knotc_t, knotc_exec_t)

init_system_domain() as knotc is short-running not long-running

> +role knotc_roles types knotc_t;
> +
> +attribute_role knotc_roles;
> +roleattribute system_r knotc_roles;

redundant as init_system_domain() already authorizes system_r to knotc_t

> +
> +type knot_conf_t;
> +files_type(knot_conf_t)

files_config_file(knot_conf_t)

> +
> +type knot_runtime_t;
> +files_pid_file(knot_runtime_t)
> +
> +type knot_var_lib_t;
> +files_type(knot_var_lib_t)
> +
> +type knot_tmp_t;
> +files_tmp_file(knot_tmp_t)
> +
> +########################################
> +#
> +# knotd local policy
> +#
> +allow knotd_t self:capability { dac_read_search setgid setpcap setuid };

You might need dac_override here as indicated by setuid/setgid

> +allow knotd_t self:process { fork signal_perms getcap getsched setsched =
};

fork is probably redundant since all "domain" is allowed to fork

> +allow knotd_t self:tcp_socket create_stream_socket_perms;
> +allow knotd_t self:udp_socket create_stream_socket_perms;

udp is not connection based, use create_socket_perms for udp

> +allow knotd_t self:unix_stream_socket create_stream_socket_perms;
> +
> +corenet_tcp_bind_generic_node(knotd_t)
> +corenet_udp_bind_generic_node(knotd_t)
> +
> +corenet_sendrecv_dns_server_packets(knotd_t)
> +corenet_tcp_bind_dns_port(knotd_t)
> +corenet_udp_bind_dns_port(knotd_t)
> +# Slave replication
> +corenet_tcp_connect_dns_port(knotd_t)
> +
> +kernel_read_kernel_sysctls(knotd_t)
> +
> +knot_read_conf(knotd_t)
> +knot_manage_runtime_files(knotd_t)
> +knot_manage_tmpfs_files(knotd_t)
> +
> +# Read /etc/passwd
> +files_read_etc_files(knotd_t)
> +# Read /etc/{resolv.conf,hosts}
> +sysnet_read_config(knotd_t)

you probably want sysnet_dns_name_resolve() for slave replication (ie when =
you use dns names instead of ip addresses to connect to master

> +
> +fs_dontaudit_getattr_xattr_fs(knotd_t)
> +
> +fs_dontaudit_getattr_tmpfs(knotd_t)

I would probably just allow the above two. Use dontaudit rules conservative=
ly

> +
> +logging_send_syslog_msg(knotd_t)
> +
> +miscfiles_read_localization(knotd_t)
> +
> +########################################
> +#
> +# knotc local policy
> +#
> +
> +allow knotc_t self:capability { dac_override dac_read_search };
> +
> +stream_connect_pattern(knotc_t, knot_runtime_t, knot_runtime_t, knotd_t)
> +
> +knot_read_conf(knotc_t)
> +knot_manage_tmpfs_files(knotc_t)
> +knot_manage_var_lib_files(knotc_t)
> +
> +files_dontaudit_search_var_lib(knotc_t)

This can be removed when you fix knot_manage_var_lib() It did not make sens=
e. knotc can never get to /var/lib/knot if its not allowed to traverse /var=
/lib

> +
> +fs_dontaudit_getattr_tmpfs(knotc_t)

I would just allow this

> +
> +domain_use_interactive_fds(knotc_t)
> +
> +miscfiles_read_localization(knotc_t)
> +
> +userdom_use_user_ptys(knotc_t)
> --=20
> 2.21.0
>=20

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

--bg08WKrSYDhXBjb5
Content-Type: application/pgp-signature; name="signature.asc"

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

iQGzBAEBCAAdFiEEujmXliIBLFTc2Y4AJXSOVTf5R2kFAl0bfxsACgkQJXSOVTf5
R2ng0Av/S00KEuPVOQhk6wwU9qR0WRo+iMHYhazYju7vlnkssmDjNfo/t+YFBN6K
X0b1+9an/b/3bXXbxwxNpYwAg5CjvdwSZoEtoW6jCxaZlx1wNBP/AoQex3es8qvj
mPRSx6EVd3ovwV5r5abonpvdnlmSHqGh4WYJpy+on/kwFpe2gexBqHHWxV2/g+Pa
5kWdu9YVrFdqn78Pf3wYj1nFaOeq/Ip/on6kSSQLWIKbKTHt9XA8R6ooEA4LZ1YV
rplosYcAiwCTqyCAq8IO9Bk+r2mGO9LOIgc2RZaAbbbe1Ovj9jA+TMNwdT47JHpg
Oe1JSh5gW/LUAy7FqV60Qbcy1jqxbfdzsdG5CpxTpmNwq0DTCq4g5BKRrUlcUAWE
mVmgFv7dewFF6Ll3UP3r9oGXHs7QvGlefCIbklrfe8gYIjrOxoiYZz5tYujB4hNY
uSytj5B/BdDMaOHFl3VscoVv3GDRFFY8TcOgdYpjO1iVkJLvVjuolshfICRwA7S0
njSM5YtN
=9gRI
-----END PGP SIGNATURE-----

--bg08WKrSYDhXBjb5--