Russell Coker <russell@xxxxxxxxxxxx> 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@xxxxxxxxxxx
Key fingerprint = FCD2 3660 5D6B 9D27 7FC6 E0FF DA7E 521F 10F6 4098
https://sks-keyservers.net/pks/lookup?op=get&search=0xDA7E521F10F64098
Dominick Grift
