RFC: Adding a dyntrans in systemd pid1's forking

RFC: Adding a dyntrans in systemd pid1's forking

[Chris PeBenito]

I've recently become aware of systemd's credentials feature[1].  In a 
nutshell, the intent is to reduce privilege in units by systemd itself 
copying the credentials (crypto materials, passwords, though in practice 
it could be anything) and placing it in /run/credentials, with access 
managed by namespacing.  This is intended to eliminate the need for the 
daemon in the unit directly accessing the data.  My concern is the 
possibility of inadvertently leaking credentials or abuse.  i.e. putting in

LoadCredential=foobar:/etc/shadow

This illustrative, as systemd can't read shadow if it's confined, but 
you could replace shadow with a private key or any other highly 
confidential data systemd needs to access.  The common response to my 
concern is systemd units should be protected at high integrity; only 
root can modify them, etc.  However, I think we can do better to reduce 
the possibility of errors.

I've discussed this with one of the systemd maintainers, and I'm 
proposing this change:

1. pid1 forks (to pidN) to run the unit, as usual.
2. pidN does security_compute_create() using the current domain and the 
label of the unit to get a new domain.
3. pidN does setcon() to the new domain.
4. pidN runs the unit as per usual (including the credentials stuff)

Then we'd need to add something like this to the policy:

allow init_t httpd_initrc_t:process dyntransition;
type_transition init_t httpd_unit_t:process httpd_initrc_t;

I have not yet prototyped this, but based on my discussion with the 
systemd maintainers, this should be doable.  I believe the added benefit 
is we can decompose initrc_t's privilege and maybe even reduce init_t's 
privilege.

What are your thoughts?


[1] https://systemd.io/CREDENTIALS/

--
Chris

Re: RFC: Adding a dyntrans in systemd pid1's forking

[Chris PeBenito]

On 11/6/2024 10:21 AM, Chris PeBenito wrote:
> I've recently become aware of systemd's credentials feature[1].  In a 
> nutshell, the intent is to reduce privilege in units by systemd itself 
> copying the credentials (crypto materials, passwords, though in practice 
> it could be anything) and placing it in /run/credentials, with access 
> managed by namespacing.  This is intended to eliminate the need for the 
> daemon in the unit directly accessing the data.  My concern is the 
> possibility of inadvertently leaking credentials or abuse.  i.e. putting in
> 
> LoadCredential=foobar:/etc/shadow
> 
> This illustrative, as systemd can't read shadow if it's confined, but 
> you could replace shadow with a private key or any other highly 
> confidential data systemd needs to access.  The common response to my 
> concern is systemd units should be protected at high integrity; only 
> root can modify them, etc.  However, I think we can do better to reduce 
> the possibility of errors.
> 
> I've discussed this with one of the systemd maintainers, and I'm 
> proposing this change:
> 
> 1. pid1 forks (to pidN) to run the unit, as usual.
> 2. pidN does security_compute_create() using the current domain and the 
> label of the unit to get a new domain.
> 3. pidN does setcon() to the new domain.
> 4. pidN runs the unit as per usual (including the credentials stuff)
> 
> Then we'd need to add something like this to the policy:
> 
> allow init_t httpd_initrc_t:process dyntransition;
> type_transition init_t httpd_unit_t:process httpd_initrc_t;
> 
> I have not yet prototyped this, but based on my discussion with the 
> systemd maintainers, this should be doable.  I believe the added benefit 
> is we can decompose initrc_t's privilege and maybe even reduce init_t's 
> privilege.

Hearing no objections, I've done an initial implementation:

https://github.com/systemd/systemd/compare/main...pebenito:systemd:pidN-selinux-setcon

If there is no policy in place, it does not incur new denials.  One nice 
thing I found is that the unit name is available, so I used that in the 
security_compute_create_name_raw() call.  I tested by adding the 
following systemd-networkd.service drop-in:

[Service]
LoadCredential=shadow:/etc/shadow


I added the following to the policy:

type systemd_networkd_initrc_t;
domain_type(systemd_networkd_initrc_t)
role system_r types systemd_networkd_initrc_t;
allow init_t self:process setcurrent;
domain_dyntrans_type(init_t)
allow init_t systemd_networkd_initrc_t:process dyntransition;
type_transition init_t systemd_networkd_unit_t:process 
systemd_networkd_initrc_t;
domtrans_pattern(systemd_networkd_initrc_t, systemd_networkd_exec_t, 
systemd_networkd_t)


These changes resulted in this denial:

Nov 13 15:10:54 azurelinux-vm audit[605]: AVC avc:  denied  { read } for 
  pid=605 comm="(sd-mkdcreds)" name="shadow" dev="sda2" ino=18058 
scontext=system_u:system_r:systemd_networkd_initrc_t:s0 
tcontext=system_u:object_r:shadow_t:s0 tclass=file permissive=1


The remaining policy for systemd_networkd_initrc_t would look like 
(denials summarized by audit2allow):

allow systemd_networkd_initrc_t autofs_t:dir getattr;
allow systemd_networkd_initrc_t autofs_t:filesystem unmount;
allow systemd_networkd_initrc_t bin_t:dir { getattr search };
allow systemd_networkd_initrc_t bin_t:file { execute execute_no_trans 
getattr map open read };
allow systemd_networkd_initrc_t boot_t:dir search;
allow systemd_networkd_initrc_t cgroup_t:dir { getattr search };
allow systemd_networkd_initrc_t cgroup_t:file { getattr mounton };
allow systemd_networkd_initrc_t cgroup_t:filesystem { getattr remount };
allow systemd_networkd_initrc_t device_t:dir mounton;
allow systemd_networkd_initrc_t devlog_t:sock_file write;
allow systemd_networkd_initrc_t dosfs_t:filesystem remount;
allow systemd_networkd_initrc_t fs_t:filesystem { remount unmount };
allow systemd_networkd_initrc_t home_root_t:dir { getattr mounton };
allow systemd_networkd_initrc_t init_runtime_t:dir { add_name create 
getattr mounton remove_name rmdir search write };
allow systemd_networkd_initrc_t init_t:dir search;
allow systemd_networkd_initrc_t init_t:fd use;
allow systemd_networkd_initrc_t init_t:file { getattr ioctl open read };
allow systemd_networkd_initrc_t init_t:unix_stream_socket getattr;
allow systemd_networkd_initrc_t kernel_t:unix_dgram_socket sendto;
allow systemd_networkd_initrc_t kmsg_device_t:chr_file { getattr mounton };
allow systemd_networkd_initrc_t modules_object_t:dir { getattr mounton };
allow systemd_networkd_initrc_t proc_kmsg_t:file { getattr mounton };
allow systemd_networkd_initrc_t proc_t:file { getattr open read };
allow systemd_networkd_initrc_t proc_t:filesystem { mount remount unmount };
allow systemd_networkd_initrc_t root_t:dir mounton;
allow systemd_networkd_initrc_t self:capability { dac_read_search fowner 
net_admin setgid setpcap setuid sys_resource };
allow systemd_networkd_initrc_t self:key { search setattr write };
allow systemd_networkd_initrc_t self:netlink_route_socket { bind create 
getattr getopt nlmsg_read read setopt write };
allow systemd_networkd_initrc_t self:process { getcap setcap setfscreate 
setrlimit };
allow systemd_networkd_initrc_t self:unix_dgram_socket { connect create 
getopt setopt };
allow systemd_networkd_initrc_t shell_exec_t:file getattr;
allow systemd_networkd_initrc_t sysctl_fs_t:dir { getattr mounton search };
allow systemd_networkd_initrc_t sysctl_kernel_t:dir search;
allow systemd_networkd_initrc_t sysctl_kernel_t:file { getattr ioctl 
open read };
allow systemd_networkd_initrc_t syslogd_runtime_t:dir search;
allow systemd_networkd_initrc_t systemd_networkd_runtime_t:dir { getattr 
mounton open read search watch };
allow systemd_networkd_initrc_t systemd_networkd_runtime_t:file { 
getattr open read };
allow systemd_networkd_initrc_t systemd_networkd_t:process2 nnp_transition;
### other than mounton this tmpfs dir/file access is for creating the 
/run/credentials content
allow systemd_networkd_initrc_t tmpfs_t:dir { add_name create getattr 
mounton open read remove_name search setattr write };
contents:
allow systemd_networkd_initrc_t tmpfs_t:file { create getattr open read 
rename setattr write };
allow systemd_networkd_initrc_t tmpfs_t:filesystem { mount remount 
unmount };
allow systemd_networkd_initrc_t unlabeled_t:dir mounton;
allow systemd_networkd_initrc_t user_home_dir_t:dir { getattr mounton };
allow systemd_networkd_initrc_t user_runtime_root_t:dir { getattr mounton };

This seems like a very promising way to break up initrc_t, limit 
privileges, and prevent administrator errors.  What do you think?


--
Chris

Re: RFC: Adding a dyntrans in systemd pid1's forking

[Kenton Groombridge]

[-- Attachment #1: Type: text/plain, Size: 7908 bytes --]

On 24/11/13 11:07AM, Chris PeBenito wrote:
> On 11/6/2024 10:21 AM, Chris PeBenito wrote:
> > I've recently become aware of systemd's credentials feature[1].  In a
> > nutshell, the intent is to reduce privilege in units by systemd itself
> > copying the credentials (crypto materials, passwords, though in practice
> > it could be anything) and placing it in /run/credentials, with access
> > managed by namespacing.  This is intended to eliminate the need for the
> > daemon in the unit directly accessing the data.  My concern is the
> > possibility of inadvertently leaking credentials or abuse.  i.e. putting
> > in
> > 
> > LoadCredential=foobar:/etc/shadow
> > 
> > This illustrative, as systemd can't read shadow if it's confined, but
> > you could replace shadow with a private key or any other highly
> > confidential data systemd needs to access.  The common response to my
> > concern is systemd units should be protected at high integrity; only
> > root can modify them, etc.  However, I think we can do better to reduce
> > the possibility of errors.
> > 
> > I've discussed this with one of the systemd maintainers, and I'm
> > proposing this change:
> > 
> > 1. pid1 forks (to pidN) to run the unit, as usual.
> > 2. pidN does security_compute_create() using the current domain and the
> > label of the unit to get a new domain.
> > 3. pidN does setcon() to the new domain.
> > 4. pidN runs the unit as per usual (including the credentials stuff)
> > 
> > Then we'd need to add something like this to the policy:
> > 
> > allow init_t httpd_initrc_t:process dyntransition;
> > type_transition init_t httpd_unit_t:process httpd_initrc_t;
> > 
> > I have not yet prototyped this, but based on my discussion with the
> > systemd maintainers, this should be doable.  I believe the added benefit
> > is we can decompose initrc_t's privilege and maybe even reduce init_t's
> > privilege.
> 
> Hearing no objections, I've done an initial implementation:
> 
> https://github.com/systemd/systemd/compare/main...pebenito:systemd:pidN-selinux-setcon
> 
> If there is no policy in place, it does not incur new denials.  One nice
> thing I found is that the unit name is available, so I used that in the
> security_compute_create_name_raw() call.  I tested by adding the following
> systemd-networkd.service drop-in:
> 
> [Service]
> LoadCredential=shadow:/etc/shadow
> 
> 
> I added the following to the policy:
> 
> type systemd_networkd_initrc_t;
> domain_type(systemd_networkd_initrc_t)
> role system_r types systemd_networkd_initrc_t;
> allow init_t self:process setcurrent;
> domain_dyntrans_type(init_t)
> allow init_t systemd_networkd_initrc_t:process dyntransition;
> type_transition init_t systemd_networkd_unit_t:process
> systemd_networkd_initrc_t;
> domtrans_pattern(systemd_networkd_initrc_t, systemd_networkd_exec_t,
> systemd_networkd_t)
> 
> 
> These changes resulted in this denial:
> 
> Nov 13 15:10:54 azurelinux-vm audit[605]: AVC avc:  denied  { read } for
> pid=605 comm="(sd-mkdcreds)" name="shadow" dev="sda2" ino=18058
> scontext=system_u:system_r:systemd_networkd_initrc_t:s0
> tcontext=system_u:object_r:shadow_t:s0 tclass=file permissive=1
> 
> 
> The remaining policy for systemd_networkd_initrc_t would look like (denials
> summarized by audit2allow):
> 
> allow systemd_networkd_initrc_t autofs_t:dir getattr;
> allow systemd_networkd_initrc_t autofs_t:filesystem unmount;
> allow systemd_networkd_initrc_t bin_t:dir { getattr search };
> allow systemd_networkd_initrc_t bin_t:file { execute execute_no_trans
> getattr map open read };
> allow systemd_networkd_initrc_t boot_t:dir search;
> allow systemd_networkd_initrc_t cgroup_t:dir { getattr search };
> allow systemd_networkd_initrc_t cgroup_t:file { getattr mounton };
> allow systemd_networkd_initrc_t cgroup_t:filesystem { getattr remount };
> allow systemd_networkd_initrc_t device_t:dir mounton;
> allow systemd_networkd_initrc_t devlog_t:sock_file write;
> allow systemd_networkd_initrc_t dosfs_t:filesystem remount;
> allow systemd_networkd_initrc_t fs_t:filesystem { remount unmount };
> allow systemd_networkd_initrc_t home_root_t:dir { getattr mounton };
> allow systemd_networkd_initrc_t init_runtime_t:dir { add_name create getattr
> mounton remove_name rmdir search write };
> allow systemd_networkd_initrc_t init_t:dir search;
> allow systemd_networkd_initrc_t init_t:fd use;
> allow systemd_networkd_initrc_t init_t:file { getattr ioctl open read };
> allow systemd_networkd_initrc_t init_t:unix_stream_socket getattr;
> allow systemd_networkd_initrc_t kernel_t:unix_dgram_socket sendto;
> allow systemd_networkd_initrc_t kmsg_device_t:chr_file { getattr mounton };
> allow systemd_networkd_initrc_t modules_object_t:dir { getattr mounton };
> allow systemd_networkd_initrc_t proc_kmsg_t:file { getattr mounton };
> allow systemd_networkd_initrc_t proc_t:file { getattr open read };
> allow systemd_networkd_initrc_t proc_t:filesystem { mount remount unmount };
> allow systemd_networkd_initrc_t root_t:dir mounton;
> allow systemd_networkd_initrc_t self:capability { dac_read_search fowner
> net_admin setgid setpcap setuid sys_resource };
> allow systemd_networkd_initrc_t self:key { search setattr write };
> allow systemd_networkd_initrc_t self:netlink_route_socket { bind create
> getattr getopt nlmsg_read read setopt write };
> allow systemd_networkd_initrc_t self:process { getcap setcap setfscreate
> setrlimit };
> allow systemd_networkd_initrc_t self:unix_dgram_socket { connect create
> getopt setopt };
> allow systemd_networkd_initrc_t shell_exec_t:file getattr;
> allow systemd_networkd_initrc_t sysctl_fs_t:dir { getattr mounton search };
> allow systemd_networkd_initrc_t sysctl_kernel_t:dir search;
> allow systemd_networkd_initrc_t sysctl_kernel_t:file { getattr ioctl open
> read };
> allow systemd_networkd_initrc_t syslogd_runtime_t:dir search;
> allow systemd_networkd_initrc_t systemd_networkd_runtime_t:dir { getattr
> mounton open read search watch };
> allow systemd_networkd_initrc_t systemd_networkd_runtime_t:file { getattr
> open read };
> allow systemd_networkd_initrc_t systemd_networkd_t:process2 nnp_transition;
> ### other than mounton this tmpfs dir/file access is for creating the
> /run/credentials content
> allow systemd_networkd_initrc_t tmpfs_t:dir { add_name create getattr
> mounton open read remove_name search setattr write };
> contents:
> allow systemd_networkd_initrc_t tmpfs_t:file { create getattr open read
> rename setattr write };
> allow systemd_networkd_initrc_t tmpfs_t:filesystem { mount remount unmount
> };
> allow systemd_networkd_initrc_t unlabeled_t:dir mounton;
> allow systemd_networkd_initrc_t user_home_dir_t:dir { getattr mounton };
> allow systemd_networkd_initrc_t user_runtime_root_t:dir { getattr mounton };
> 
> This seems like a very promising way to break up initrc_t, limit privileges,
> and prevent administrator errors.  What do you think?
> 
> 
> --
> Chris
> 

Overall I like the direction this is going! I am curious, though, about
whether this will affect systemd units' ExecStartPre=, ExecStartPost=,
and similar directives.

One of the problems I have when writing policy for some systemd units is
these directives will run commands normally under init_t instead of the
resulting daemon domain. A unit may, as an example, want to remove a
file on ExecStartPre=, which will run as init_t and therefore will need
to be allowed to do so by policy if the command there is a simple
/bin/rm.

Will this be extended to have these commands run under the unit's
corresponding initrc_t domain if there is one? That would solve so much
headache when dealing with these types of units and also allow breaking
up init_t potentially further.

-Kenton Groombridge

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

Re: RFC: Adding a dyntrans in systemd pid1's forking

[Chris PeBenito]

On 11/13/2024 1:07 PM, Kenton Groombridge wrote:
> On 24/11/13 11:07AM, Chris PeBenito wrote:
>> On 11/6/2024 10:21 AM, Chris PeBenito wrote:
>>> I've recently become aware of systemd's credentials feature[1].  In a
>>> nutshell, the intent is to reduce privilege in units by systemd itself
>>> copying the credentials (crypto materials, passwords, though in practice
>>> it could be anything) and placing it in /run/credentials, with access
>>> managed by namespacing.  This is intended to eliminate the need for the
>>> daemon in the unit directly accessing the data.  My concern is the
>>> possibility of inadvertently leaking credentials or abuse.  i.e. putting
>>> in
>>>
>>> LoadCredential=foobar:/etc/shadow
>>>
>>> This illustrative, as systemd can't read shadow if it's confined, but
>>> you could replace shadow with a private key or any other highly
>>> confidential data systemd needs to access.  The common response to my
>>> concern is systemd units should be protected at high integrity; only
>>> root can modify them, etc.  However, I think we can do better to reduce
>>> the possibility of errors.
>>>
>>> I've discussed this with one of the systemd maintainers, and I'm
>>> proposing this change:
>>>
>>> 1. pid1 forks (to pidN) to run the unit, as usual.
>>> 2. pidN does security_compute_create() using the current domain and the
>>> label of the unit to get a new domain.
>>> 3. pidN does setcon() to the new domain.
>>> 4. pidN runs the unit as per usual (including the credentials stuff)
>>>
>>> Then we'd need to add something like this to the policy:
>>>
>>> allow init_t httpd_initrc_t:process dyntransition;
>>> type_transition init_t httpd_unit_t:process httpd_initrc_t;
>>>
>>> I have not yet prototyped this, but based on my discussion with the
>>> systemd maintainers, this should be doable.  I believe the added benefit
>>> is we can decompose initrc_t's privilege and maybe even reduce init_t's
>>> privilege.
>>
>> Hearing no objections, I've done an initial implementation:
>>
>> https://github.com/systemd/systemd/compare/main...pebenito:systemd:pidN-selinux-setcon
>>
>> If there is no policy in place, it does not incur new denials.  One nice
>> thing I found is that the unit name is available, so I used that in the
>> security_compute_create_name_raw() call.  I tested by adding the following
>> systemd-networkd.service drop-in:
>>
>> [Service]
>> LoadCredential=shadow:/etc/shadow
>>
>>
>> I added the following to the policy:
>>
>> type systemd_networkd_initrc_t;
>> domain_type(systemd_networkd_initrc_t)
>> role system_r types systemd_networkd_initrc_t;
>> allow init_t self:process setcurrent;
>> domain_dyntrans_type(init_t)
>> allow init_t systemd_networkd_initrc_t:process dyntransition;
>> type_transition init_t systemd_networkd_unit_t:process
>> systemd_networkd_initrc_t;
>> domtrans_pattern(systemd_networkd_initrc_t, systemd_networkd_exec_t,
>> systemd_networkd_t)
>>
>>
>> These changes resulted in this denial:
>>
>> Nov 13 15:10:54 azurelinux-vm audit[605]: AVC avc:  denied  { read } for
>> pid=605 comm="(sd-mkdcreds)" name="shadow" dev="sda2" ino=18058
>> scontext=system_u:system_r:systemd_networkd_initrc_t:s0
>> tcontext=system_u:object_r:shadow_t:s0 tclass=file permissive=1
>>
>>
>> The remaining policy for systemd_networkd_initrc_t would look like (denials
>> summarized by audit2allow):
>>
[... cut ...]
>> allow systemd_networkd_initrc_t bin_t:file { execute execute_no_trans getattr map open read };
[... cut ...]
>>
>> This seems like a very promising way to break up initrc_t, limit privileges,
>> and prevent administrator errors.  What do you think?
>>
> 
> Overall I like the direction this is going! I am curious, though, about
> whether this will affect systemd units' ExecStartPre=, ExecStartPost=,
> and similar directives.

It would affect Exec* directives in a unit and theoretically any other 
directives that generate a call to exec_invoke() in the systemd code.  I 
placed the setcon() immediately before

> One of the problems I have when writing policy for some systemd units is
> these directives will run commands normally under init_t instead of the
> resulting daemon domain. A unit may, as an example, want to remove a
> file on ExecStartPre=, which will run as init_t and therefore will need
> to be allowed to do so by policy if the command there is a simple
> /bin/rm.

This type of behavior is one of the secondary reasons I've been looking 
in to this.  In refpolicy, if init_t execs a bin_t file, it runs in 
initrc_t.  Then, because of unit-specific Exec* that run bin_t stuf, 
initrc_t gets bloated with permissions which could be (ab)used by other 
units.


> Will this be extended to have these commands run under the unit's
> corresponding initrc_t domain if there is one? That would solve so much
> headache when dealing with these types of units and also allow breaking
> up init_t potentially further.

This initial implementation does that; note the bin_t 
execute(_no_trans).   That's the unit running systemd-networkd-wait-online.


--
Chris