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

2015-06-09 Thread Dominick Grift 
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.

Thanks

-- 
02DFF788
4D30 903A 1CF3 B756 FB48  1514 3148 83A2 02DF F788
http://keys.gnupg.net/pks/lookup?op=vindexsearch=0x314883A202DFF788
Dominick Grift


pgpLns3mVGvdM.pgp
Description: PGP signature
___
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

2015-06-09 Thread Lennart Poettering 
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

2015-06-09 Thread HATAYAMA Daisuke 
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

2015-06-09 Thread HATAYAMA Daisuke 
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

2015-06-08 Thread systemd github import bot 
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

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

2015-06-08 Thread HATAYAMA Daisuke 
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.

Signed-off-by: HATAYAMA Daisuke d.hatay...@jp.fujitsu.com
---
 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

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

2015-06-08 Thread Lennart Poettering 
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