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:   Dominick Grift <dominick.grift@defensec.nl>
To:     Russell Coker <russell@coker.com.au>
Cc:     selinux-refpolicy@vger.kernel.org
Subject: Re: [PATCH] misc kernel and system patches
References: <YAgAyh7eG5zePmGE@xev> <ypjlwnw7lloh.fsf@defensec.nl>
        <43567489.y7f9mAODUD@liv>
Date:   Wed, 27 Jan 2021 07:03:06 +0100
In-Reply-To: <43567489.y7f9mAODUD@liv> (Russell Coker's message of "Wed, 27
        Jan 2021 15:05:31 +1100")
Message-ID: <ypjllfcekjc5.fsf@defensec.nl>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Russell Coker <russell@coker.com.au> writes:

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

Not sure yet. other than that is looks incomplete and that i am
wondering why one would be bothering with this.

Can you tell me a bit more about this event?

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

Those day's are long gone. Nowaday's even `grep` does execmem.

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

why would lvm run for init have any busyness with temporary files? Seems
unlikely to me and nowaday's we have a lot more flexibility with
type-trans rules. But yes, its a bit late in the game now to change
this. It breaks the model though IMHO.

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

-- 
gpg --locate-keys dominick.grift@defensec.nl
Key fingerprint = FCD2 3660 5D6B 9D27 7FC6  E0FF DA7E 521F 10F6 4098
https://sks-keyservers.net/pks/lookup?op=get&search=0xDA7E521F10F64098
Dominick Grift