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 kernel and system patches
Date:   Wed, 27 Jan 2021 15:05:31 +1100
Message-ID: <43567489.y7f9mAODUD@liv>
In-Reply-To: <ypjlwnw7lloh.fsf@defensec.nl>
References: <YAgAyh7eG5zePmGE@xev> <ypjlwnw7lloh.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 1:36:46 AM AEDT Dominick Grift wrote:
> > Index: refpolicy-2.20210120/policy/modules/kernel/corecommands.if
> > ===================================================================
> > --- refpolicy-2.20210120.orig/policy/modules/kernel/corecommands.if
> > +++ refpolicy-2.20210120/policy/modules/kernel/corecommands.if
> > @@ -662,6 +662,7 @@ interface(`corecmd_read_all_executables'
> > 
> >  	corecmd_search_bin($1)
> >  	read_files_pattern($1, exec_type, exec_type)
> > 
> > +	allow $1 exec_type:file map;
> 
> create a corecmd_map_read_all_executables() instead. This macro name is
> "read_all_executables" if you extend it with this rule then you
> effectively do several things:

OK, I'll do that in another patch.

> > +interface(`files_mounton_kernel_symbol_table',`
> > +	gen_require(`
> > +		type boot_t, system_map_t;
> > +	')
> > +
> > +	allow $1 boot_t:dir list_dir_perms;
> > +	allow $1 system_map_t:file mounton;
> 
> mount != listing boot_t dirs (i know its semi-related but you might want
> to mount on symbox table and not list boot_t and this will shut the door
> on that)
> 
> instead you should probably imply getattr here:
> 
>         allow $1 system_map_t:file { getattr mounton };
> 
> Would be even better to declare "mounton_file_perms" on a lower level
> and use that
> 
> define(`mounton_file_perms',`{ getattr mounton }')

OK, that will be in the next version.

> > +##	Mount on the selinuxfs filesystem.
> > +## </summary>
> > +## <param name="domain">
> > +##	<summary>
> > +##	Domain allowed access.
> > +##	</summary>
> > +## </param>
> > +#
> > +interface(`selinux_mounton_fs',`
> > +	gen_require(`
> > +		type security_t;
> > +	')
> > +
> > +	allow $1 security_t:dir mounton;
> 
> getattr should probably be implied here
> 
> a mounton_dir_perms would be even better:
> 
> define(`mounton_dir_perms',`{ getattr mounton }')

OK.

> > Index: refpolicy-2.20210120/policy/modules/kernel/terminal.te
> > ===================================================================
> > --- refpolicy-2.20210120.orig/policy/modules/kernel/terminal.te
> > +++ refpolicy-2.20210120/policy/modules/kernel/terminal.te
> > @@ -31,6 +31,9 @@ fs_associate_tmpfs(devpts_t)
> > 
> >  fs_xattr_type(devpts_t)
> >  fs_use_trans devpts gen_context(system_u:object_r:devpts_t,s0);
> > 
> > +# for systemd-nspawn
> > +allow console_device_t devpts_t:filesystem associate;
> 
> I am a fairly big user of systemd_nspawn and i have never ever
> encountered this. only pty devices should ever associate with devpts_t
> filesystems AFAIK

OK, I'll remove that and investigate other solutions.

> > +##	Allow a domain to be transitioned to from init_t with nnp_transition
> > +## </summary>
> > +## <param name="domain">
> > +##	<summary>
> > +##	Domain to transition
> > +##	</summary>
> > +## </param>
> > +#
> > +interface(`init_nnp_domain',`
> > +	gen_require(`
> > +		type init_t;
> > +	')
> > +
> > +	allow init_t $1:process2 nnp_transition;
> > +')
> 
> This is redundant. In systems with systemd (ifdef init_systemd) this access
> is already allowed.

OK, I'll remove it.

> > +######################################
> > +## <summary>
> > +##	restart systemd units, for /run/systemd/transient/*
> > +## </summary>
> > +## <param name="domain">
> > +##	<summary>
> > +##	Domain allowed access.
> > +##	</summary>
> > +## </param>
> > +#
> > +interface(`init_restart_units',`
> > +	gen_require(`
> > +		type init_var_run_t;
> > +	')
> > +
> > +	allow $1 init_var_run_t:service { start status stop };
> > +')
> 
> i would probably create a private type for "runtime units"
> but also in another patch you create another "restart_units" interface
> and that has different permissions (probably best to associate
> consistent permissions with interface names)
> 
> not where "restart_units" means something different somewhere else

I'll move this to another patch.

> > Index: refpolicy-2.20210120/policy/modules/system/init.te
> > ===================================================================
> > --- refpolicy-2.20210120.orig/policy/modules/system/init.te
> > +++ refpolicy-2.20210120/policy/modules/system/init.te
> > @@ -239,7 +239,8 @@ ifdef(`init_systemd',`
> > 
> >  	allow init_t self:unix_stream_socket { create_stream_socket_perms
> >  	connectto }; allow init_t self:netlink_audit_socket { nlmsg_relay
> >  	create_socket_perms }; allow init_t self:netlink_selinux_socket
> >  	create_socket_perms;
> > 
> > -	allow init_t self:system { status reboot halt reload };
> > +	# why does kernel 4.9 make it need start and stop while 4.19 does not?
> > +	allow init_t self:system { start stop status reboot halt reload
> > 
> >  };
> 
> I would remove the above change. might have been a bug in 4.9, no need
> to support bugs besides kernel 4.9 is old.

OK, I've removed that.

> > @@ -1002,6 +1003,7 @@ ifdef(`enabled_mls',`
> > 
> >  ifdef(`init_systemd',`
> >  
> >  	allow initrc_t init_t:system { start status reboot halt reload };
> > 
> > +	allow init_t initrc_t:process2 nnp_transition;
> 
> this is dedundant. Should already be allowed

OK.

> > Index: refpolicy-2.20210120/policy/modules/system/locallogin.te
> > ===================================================================
> > --- refpolicy-2.20210120.orig/policy/modules/system/locallogin.te
> > +++ refpolicy-2.20210120/policy/modules/system/locallogin.te
> > @@ -125,7 +125,8 @@ auth_manage_pam_runtime_files(local_logi
> > 
> >  auth_manage_pam_console_data(local_login_t)
> >  auth_domtrans_pam_console(local_login_t)
> > 
> > -init_dontaudit_use_fds(local_login_t)
> > +# if local_login_t can not inherit fd from init it takes ages to login
> > +init_use_fds(local_login_t)
> 
> Yes i think youre right but i think this applies to all processes forked
> by systemd. I believe that addressing rules associated with systemd
> forked processes should probably be addressed on a lower level instead
> 
> for example:
> 
> init_domain is obviously systemd forked in a systemd system (init_domain
> is allowed to use init fd via domtrans_pattern(init_t, $1, $2) in
> init_domain().
> 
> Howver local_login is not a direct fork of systemd (its not an
> init_daemon) and instead its a indirect forked process of systemd (it
> gets executed by a init domain but not by init itself)
> 
> I would create a type attribute "systemd_forked_type" and then associate
> the forked related rules to that and then use that
> 
> i think these (or somthing like it):
> 
> allow $1 systemd_forked_type:fd use;
> allow $1 systemd_forked_type:unix_stream_socket rw_socket_perms;
> 
> These these can be removed:

I'll move this to another patch and another discussion.

> https://github.com/SELinuxProject/refpolicy/blob/ea6002ddf9c09a307dccc4bf662
> ff7efa2395572/policy/modules/system/init.if#L186
> https://github.com/SELinuxProject/refpolicy/blob/master/policy/modules/syst
> em/init.if#L149 etc
> 
> otherwise you end up with very decentralized policy which is hard to
> maintain.

> > +######################################
> > +## <summary>
> > +##	Allow lvm_t to use a semaphore
> > +## </summary>
> > +## <param name="domain">
> > +##	<summary>
> > +##	Domain that created the semaphore
> > +##	</summary>
> > +## </param>
> > +#
> > +interface(`lvm_use_sem',`
> > +	gen_require(`
> > +		type lvm_t;
> > +	')
> > +
> > +	allow lvm_t $1:sem all_sem_perms;
> 
> Thats not allowed like this generally

OK, I'll do it differently.

> > @@ -99,6 +99,7 @@ fs_getattr_xattr_fs(kmod_t)
> > 
> >  fs_dontaudit_use_tmpfs_chr_dev(kmod_t)
> >  fs_search_tracefs(kmod_t)
> > 
> > +init_nnp_domain(kmod_t)
> 
> shouldnt be needed : kmod is a init_system_domain which is a
> init_domain, and systemd can already nnp transition to all init_domain
> if ifdef init_systemd is set

OK, I'll test that out.

> > +term_use_unallocated_ttys(systemd_passwd_agent_t)
> > 
> >  auth_use_nsswitch(systemd_passwd_agent_t)
> > 
> > @@ -1100,6 +1133,8 @@ logging_send_syslog_msg(systemd_pstore_t
> > 
> >  allow systemd_rfkill_t self:netlink_kobject_uevent_socket { bind create
> >  getattr read setopt };> 
> > +allow systemd_rfkill_t self:netlink_kobject_uevent_socket
> > client_stream_socket_perms;
> thats not a stream socket, do this instead:
> 
> - allow systemd_rfkill_t self:netlink_kobject_uevent_socket { bind create
> getattr read setopt }; + allow systemd_rfkill_t
> self:netlink_kobject_uevent_socket create_socket_perms;

OK.

> > @@ -1264,6 +1299,8 @@ allow systemd_tmpfiles_t systemd_journal
> > 
> >  allow systemd_tmpfiles_t systemd_tmpfiles_conf_t:dir list_dir_perms;
> >  allow systemd_tmpfiles_t systemd_tmpfiles_conf_type:file read_file_perms;
> > 
> > +allow systemd_tmpfiles_t systemd_nspawn_runtime_t:fifo_file unlink;
> 
> questionable

Why?

> > Index: refpolicy-2.20210120/policy/modules/system/unconfined.te
> > ===================================================================
> > --- refpolicy-2.20210120.orig/policy/modules/system/unconfined.te
> > +++ refpolicy-2.20210120/policy/modules/system/unconfined.te
> > @@ -83,6 +83,10 @@ optional_policy(`
> > 
> >  ')
> >  
> >  optional_policy(`
> > 
> > +	certbot_run(unconfined_t, unconfined_r)
> 
> unconfined should be unconfined.

certbot needs execmem, we generally don't want to give that to unconfined, so 
running certbot in a different domain seems better.

> >  optional_policy(`
> >  
> >  	lvm_run(unconfined_t, unconfined_r)
> > 
> > +	lvm_use_sem(unconfined_t)
> 
> that lvm_use_sem should probably just be part of lvm_run()
> 
> ie "allow $1 lvm_t:semd rw_sem_perms;"

OK, I'll do that.

> But in my personal view unconfined_t shouldnt run lvm with a domain
> transition in the first place (defeats the purpose of the unconfined domain)

I think the problem is the type transition rules.  Run lvm etc as unconfined_t 
and then lvm run from init etc won't be able to access it's temporary files 
etc.

> > Index: refpolicy-2.20210120/policy/modules/system/userdomain.if
> > ===================================================================
> > --- refpolicy-2.20210120.orig/policy/modules/system/userdomain.if
> > +++ refpolicy-2.20210120/policy/modules/system/userdomain.if
> > @@ -2167,6 +2167,8 @@ interface(`userdom_read_user_home_conten
> > 
> >  	')
> >  	
> >  	read_files_pattern($1, { user_home_dir_t user_home_t }, user_home_t)
> > 
> > +	allow $1 user_home_t:file map;
> 
> read != map
> and file != lnk_file
> 
> by generalizing interfaces you shut doors for fine grained access control

OK, I'll remove that.

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