Received-SPF: pass (google.com: domain of selinux-refpolicy-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Russell Coker <russell@coker.com.au>
To:     Dominick Grift <dominick.grift@defensec.nl>
Cc:     selinux-refpolicy@vger.kernel.org
Subject: Re: [PATCH] misc network patches
Date:   Wed, 27 Jan 2021 18:00:20 +1100
Message-ID: <1818414.Kogm6ZmNZq@liv>
In-Reply-To: <ypjla6t3n3n2.fsf@defensec.nl>
References: <YAf6/c8XKLACDF9P@xev> <ypjla6t3n3n2.fsf@defensec.nl>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Thursday, 21 January 2021 12:23:29 AM AEDT Dominick Grift wrote:
> > Index: refpolicy-2.20210120/policy/modules/roles/staff.te
> > ===================================================================
> > --- refpolicy-2.20210120.orig/policy/modules/roles/staff.te
> > +++ refpolicy-2.20210120/policy/modules/roles/staff.te
> > @@ -15,6 +15,10 @@ userdom_unpriv_user_template(staff)
> > 
> >  #
> >  corenet_ib_access_unlabeled_pkeys(staff_t)
> > 
> > +corenet_tcp_bind_all_unreserved_ports(staff_t)
> > +corenet_udp_bind_all_unreserved_ports(staff_t)
> > +corenet_tcp_bind_generic_node(staff_t)
> 
> staff_t is a "unpriv user" and so "userdom_unpriv_user_template()"
> applies to staff_t.

OK, I'll remove that.

> this template has two booleans: user_tcp_server and user_udp_server
> these booleans currently allow unpriv users to bind tcp and udp sockets
> to generic ports respectively.
> 
> This is old, inaccurate and outdated. Instead those booleans should
> probably be modernized:
> 
> 	tunable_policy(`user_tcp_server',`
> 		corenet_tcp_bind_generic_node($1_t)
> 		corenet_tcp_bind_all_unreserved_ports($1_t)
> 	')
> 
> 	tunable_policy(`user_udp_server',`
> 		corenet_udp_bind_generic_node($1_t)
> 		corenet_udp_bind_all_unreserved_ports($1_t)
> 	')

Unreserved ports means all ports >1023.  I don't think that's what we want.  
There are lots of daemons using ports >1023, many of which are IANA assigned.

> >  optional_policy(`
> >  
> >  	apache_role(staff_r, staff_t)
> >  
> >  ')
> > 
> > @@ -36,6 +40,10 @@ optional_policy(`
> > 
> >  ')
> >  
> >  optional_policy(`
> > 
> > +	netutils_domtrans_ping(staff_t)
> > +')
> > +
> 
> This is already (conditionlly) allowed in the userdom_unpriv_user_template()
> To make it work requires a boolean to be set to true (i believe)

OK, I'll remove that.

> > +optional_policy(`
> > 
> >  	postgresql_role(staff_r, staff_t)
> >  
> >  ')
> > 
> > @@ -65,6 +73,11 @@ optional_policy(`
> > 
> >  ')
> >  
> >  optional_policy(`
> > 
> > +	# for torbrowser-launcher
> > +	xdg_exec_data(staff_t)
> 
> What location is this exactly? ~/.local/bin?

~/.local/share/torbrowser/tbb/x86_64/tor-browser_en-US/Browser/
 
> I would associate a "bin_home_t" with ~/.local/bin and only allow that
> to be executed, rather than all of ~/.local
> 
> But regardless this, or similar rule should apply to
> userdom_unpriv_user_template() instead. Use the templates.

OK, I'll remove it for now.

> > +')
> > +
> > +optional_policy(`
> > 
> >  	xscreensaver_role(staff_r, staff_t)
> >  
> >  ')
> > 
> > Index: refpolicy-2.20210120/policy/modules/roles/unprivuser.te
> > ===================================================================
> > --- refpolicy-2.20210120.orig/policy/modules/roles/unprivuser.te
> > +++ refpolicy-2.20210120/policy/modules/roles/unprivuser.te
> > @@ -7,11 +7,23 @@ policy_module(unprivuser, 2.10.0)
> > 
> >  #
> >  # Declarations
> >  #
> > 
> > +## <desc>
> > +## <p>
> > +## Allow user to bind all unreserved ports
> > +## </p>
> > +## </desc>
> > +gen_tunable(user_bind_unreserved, false)
> 
> Modernize and use the existing user_tcp_server and user_udp_server
> booleans in userdom_unpriv_user_template() instead? See above comment.

OK.

> >  #role user_r;
> >  
> >  userdom_unpriv_user_template(user)
> > 
> > +tunable_policy(`user_bind_unreserved', `
> > +	corenet_tcp_bind_all_unreserved_ports(user_t)
> > +	corenet_udp_bind_all_unreserved_ports(user_t)
> > +	corenet_tcp_bind_generic_node(user_t)
> > +')
> 
> See above comment about user_tcp_server and user_udp_server booleans in
> userdom_unpriv_user_template()

OK.

> >  ')
> > 
> > @@ -25,6 +37,10 @@ optional_policy(`
> > 
> >  ')
> >  
> >  optional_policy(`
> > 
> > +	netutils_domtrans_ping(user_t)
> > +')
> > +
> 
> This is already allowed conditionally in userdom_unpriv_user_template()
> requires a boolean to be set to true to work. Adding this rule would
> break that existing model.

OK.

> > +########################################
> > +## <summary>
> > +##	Restart and get status of samba daemon
> > +## </summary>
> > +## <param name="domain">
> > +##	<summary>
> > +##	Domain allowed access.
> > +##	</summary>
> > +## </param>
> > +#
> > +interface(`samba_restart',`
> > +	gen_require(`
> > +		type samba_unit_t;
> > +	')
> > +
> > +	allow $1 samba_unit_t:file getattr;
> > +	allow $1 samba_unit_t:service { start stop status reload };
> 
> This interface seems a bit broad

OK.

-- 
My Main Blog         http://etbe.coker.com.au/
My Documents Blog    http://doc.coker.com.au/