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

[HATAYAMA Daisuke]

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


>> Signed-off-by: HATAYAMA Daisuke <d.hatayama at jp.fujitsu.com>
> 
> We don't use S-o-b in systemd.
> 

Sorry, I'll not use the tag next. I decided to use it becuase I found
commits with the tag in git log (of course, I found the commits
without the tag, too).

--
Thanks.
HATAYAMA, Daisuke