[systemd-devel] [PATCH] selinux: fix missing SELinux unit access check

[HATAYAMA Daisuke]

From: HATAYAMA Daisuke <d.hatayama at jp.fujitsu.com>
Subject: Re: [systemd-devel] [PATCH] selinux: fix missing SELinux unit access check
Date: Wed, 10 Jun 2015 12:18:48 +0900 (JST)

> From: Lennart Poettering <lennart at poettering.net>
> Subject: Re: [systemd-devel] [PATCH] selinux: fix missing SELinux unit access check
> Date: Mon, 8 Jun 2015 12:37:14 +0200
> 
>> On Mon, 08.06.15 19:00, HATAYAMA Daisuke (d.hatayama at jp.fujitsu.com) wrote:
>> 
>>> Currently, SELinux unit access check is not performed if a given unit
>>> file has not been registered in a hash table. This is because function
>>> manager_get_unit() only tries to pick up a Unit object from a Unit
>>> hash table. Instead, we use function manager_load_unit() searching
>>> Unit file pathes for the given Unit file.
>> 
>> Were precisely is this relevant? I mean, we generally invoke
>> operations on units that are already loaded?
>> 
> 
> The SELinux unit access check is performed in terms of the following rule:
> 
> ~]# sesearch -A -s init_t | grep "systemd_unit_file_type" | grep service
>    allow init_t systemd_unit_file_type : service { start stop status reload kill load enable disable } ;
> 
> where systemd_unit_file_type is the attribute that is assigned to
> context types that unit files have, e.g. systemd_unit_file_t.
> 
> Because this SELinux unit access check uses context types that unit
> files have, we need to be able to refer to given unit files when we
> perform requested unit operations.
> 
> Here's how to reproduce this issue. Note that:
> 
> - the systemd used here is with the fixing patch, and
> 
> - test.service has not been loaded (even tried to get loaded)
>   throughout this boot to guarantee that test.service is not registered
>   in the unit hash table.
> 
> Then,
> 
> ~]# cat ./test.service
> [Unit]
> Description=test
> 
> [Service]
> Type=oneshot
> ExecStart=/usr/bin/true
> RemainAfterExit=yes
> 
> [Install]
> WantedBy=multi-user.target
> ~]# cat ./foo.sh
> #! /bin/sh
> 
> mv test.service /etc/systemd/system
> systemctl daemon-reload
> systemctl enable test.service
> ~]# ./foo.sh
> 
> Then, here is a gdb log:
> 
> break mac_selinux_unit_access_check_strv
> Breakpoint 1 at 0x7f5339807a28: file src/core/selinux-access.c, line 288.
> (gdb) continue
> Continuing.
> Detaching after fork from child process 2198.
> 
> Breakpoint 1, mac_selinux_unit_access_check_strv (units=0x7f533a1324f0, message=0x7f533a166a10, m=0x7f533a07c680, permission=0x7f533992bdb6 "enable", error=0x7ffe80abaec0) at src/core/selinux-access.c:288
> 288                                     sd_bus_error *error) {
> (gdb) l
> 283
> 284     int mac_selinux_unit_access_check_strv(char **units,
> 285                                     sd_bus_message *message,
> 286                                     Manager *m,
> 287                                     const char *permission,
> 288                                     sd_bus_error *error) {
> 289     #ifdef HAVE_SELINUX
> 290             char **i;
> 291             Unit *u;
> 292             int r;
> (gdb) n
> 294             STRV_FOREACH(i, units) {
> (gdb) n
> 295                     r = manager_load_unit(m, *i, NULL, error, &u);
> (gdb) s
> manager_load_unit (m=0x7f533a07c680, name=0x7f533a11ef30 "test.service", path=0x0, e=0x7ffe80abaec0, _ret=0x7ffe80abad80) at src/core/manager.c:1369
> 1369            assert(m);
> (gdb) n
> 1374            r = manager_load_unit_prepare(m, name, path, e, _ret);
> (gdb) n
> 1375            if (r != 0)
> (gdb) n
> 1378            manager_dispatch_load_queue(m);
> (gdb) p r
> $1 = 0
> 
> Here variable r has a return value of manager_load_unit_prepare() and
> its value is 0 here, meaning that now there's no unit file with the
> requested unit file name in the unit hash table.
> 
> (gdb) p *_ret
> $2 = (Unit *) 0x7f533a166ea0
> (gdb) p *_ret->load_state
> Attempt to take contents of a non-pointer value.
> (gdb) p (*_ret)->load_state
> $3 = UNIT_STUB
> 
> Actually, its load_state is still UNIT_STUB.
> 
> (gdb) n
> 1380            if (_ret)
> (gdb) p (*_ret)->load_state
> $4 = UNIT_LOADED
> 
> After dipatching the load queue by manager_dispatch_load_queue(), the
> given unit file has just been loaded and the load_state has been
> changed into UNIT_LOADED.
> 
> (gdb) finish
> Run till exit from #0  manager_load_unit (m=0x7f533a07c680, name=0x7f533a11ef30 "test.service", path=0x0, e=0x7ffe80abaec0, _ret=0x7ffe80abad80) at src/core/manager.c:1380
> 0x00007f5339807a6a in mac_selinux_unit_access_check_strv (units=0x7f533a1324f0, message=0x7f533a166a10, m=0x7f533a07c680, permission=0x7f533992bdb6 "enable", error=0x7ffe80abaec0) at src/core/selinux-access.c:295
> 295                     r = manager_load_unit(m, *i, NULL, error, &u);
> (gdb) continue
> 

Also, I used selinux-context branch of
https://github.com/keszybz/systemd.git in this test to avoid the issue
in https://bugzilla.redhat.com/show_bug.cgi?id=1224211.

--
Thanks.
HATAYAMA, Daisuke