Re: [systemd-devel] [PATCH] selinux: fix missing SELinux unit access check
On Tue, 09.06.15 17:01, Dominick Grift (dac.overr...@gmail.com) wrote: Development has moved to github.com/systemd It is probably better to submit a Github Push Request there if you have not done so already. Well, we still accept patches by mails to the mailing list. However we prefer gihub, especially for larger patch series. Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] selinux: fix missing SELinux unit access check
From: HATAYAMA Daisuke d.hatay...@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 lenn...@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.hatay...@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 1369assert(m); (gdb) n 1374r = manager_load_unit_prepare(m, name, path, e, _ret); (gdb) n 1375if (r != 0) (gdb) n 1378manager_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 1380if (_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 0x7f5339807a6a 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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] selinux: fix missing SELinux unit access check
From: Lennart Poettering lenn...@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.hatay...@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 1369assert(m); (gdb) n 1374r = manager_load_unit_prepare(m, name, path, e, _ret); (gdb) n 1375if (r != 0) (gdb) n 1378manager_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 1380if (_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 0x7f5339807a6a 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.hatay...@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 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] selinux: fix missing SELinux unit access check
Patchset imported to github. To create a pull request, one of the main developers has to initiate one via: https://github.com/systemd/systemd/compare/master...systemd-mailing-devs:20150608.190012.161290626.d.hatayama%40jp.fujitsu.com -- Generated by https://github.com/haraldh/mail2git ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] selinux: fix missing SELinux unit access check
On Mon, 08.06.15 19:00, HATAYAMA Daisuke (d.hatay...@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? Signed-off-by: HATAYAMA Daisuke d.hatay...@jp.fujitsu.com We don't use S-o-b in systemd. --- src/core/selinux-access.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/core/selinux-access.c b/src/core/selinux-access.c index decd42f..36bdbcc 100644 --- a/src/core/selinux-access.c +++ b/src/core/selinux-access.c @@ -292,8 +292,12 @@ int mac_selinux_unit_access_check_strv(char **units, int r; STRV_FOREACH(i, units) { -u = manager_get_unit(m, *i); +r = manager_load_unit(m, *i, NULL, error, u); +if (r 0) +return r; if (u) { +if (u-load_state == UNIT_NOT_FOUND) +return -ENOENT; r = mac_selinux_unit_access_check(u, message, permission, error); if (r 0) return r; -- 2.1.0 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel