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

2015-06-18 Thread Lennart Poettering 
On Thu, 18.06.15 18:29, HATAYAMA Daisuke (d.hatay...@jp.fujitsu.com) wrote:

   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_error != 0)
  +return u-load_error;
   r = mac_selinux_unit_access_check(u, message, 
  permission, error);
   if (r  0)
   return r;
  
  I commented on the issue now in github, could you please follow up
  there?
  
  https://github.com/systemd/systemd/pull/145
  
 
 Thanks for your reviewing. However, unfortunately, I cannot use github
 for some reason for now. So, please let me discuss in the mailing
 list. I'm sorry for the inconvenience.
 
  I don't think we should check this, it doesn't matter if we managed
  to successfully load the unit or not, we should still allow access
  either way.
 
 Sorry, I don't understand well. Do you mean removing this SELinux unit
 access check? If so, it means that current logic since the following
 commit, is changed, right?

No, I meant tjust the u-load_error != 0 check, and the one line
following it. The SELinux check following that should stay.

I guess this related to the question you raised in the other thread:
Unit objects may or may not have a unit file on disk attached, and
that's completely OK. We should do the access check on the label of
the unit file if we have one, and otherwise fall back to some more
generic selinux access check.

Specifically that means here that u-load_error is nothing we need to
explicitly check, as mac_selinux_unit_access_check() already has a
generic fallback if the unit doesn't have a file associated. 

Long story short: just remove the two lines I pointed out, and leave
the mac_selinux_unit_access_check() in place and all should be good.

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 v2] selinux: fix missing SELinux unit access check

2015-06-18 Thread HATAYAMA Daisuke 
From: Lennart Poettering lenn...@poettering.net
Subject: Re: [systemd-devel] [PATCH v2] selinux: fix missing SELinux unit 
access check
Date: Thu, 18 Jun 2015 13:30:43 +0200

 On Thu, 18.06.15 18:29, HATAYAMA Daisuke (d.hatay...@jp.fujitsu.com) wrote:
 
   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_error != 0)
  +return u-load_error;
   r = mac_selinux_unit_access_check(u, message, 
  permission, error);
   if (r  0)
   return r;
  
  I commented on the issue now in github, could you please follow up
  there?
  
  https://github.com/systemd/systemd/pull/145
  
 
 Thanks for your reviewing. However, unfortunately, I cannot use github
 for some reason for now. So, please let me discuss in the mailing
 list. I'm sorry for the inconvenience.
 
  I don't think we should check this, it doesn't matter if we managed
  to successfully load the unit or not, we should still allow access
  either way.
 
 Sorry, I don't understand well. Do you mean removing this SELinux unit
 access check? If so, it means that current logic since the following
 commit, is changed, right?
 
 No, I meant tjust the u-load_error != 0 check, and the one line
 following it. The SELinux check following that should stay.
 

I see.

 I guess this related to the question you raised in the other thread:
 Unit objects may or may not have a unit file on disk attached, and
 that's completely OK. We should do the access check on the label of
 the unit file if we have one, and otherwise fall back to some more
 generic selinux access check.
 
 Specifically that means here that u-load_error is nothing we need to
 explicitly check, as mac_selinux_unit_access_check() already has a
 generic fallback if the unit doesn't have a file associated. 
 
 Long story short: just remove the two lines I pointed out, and leave
 the mac_selinux_unit_access_check() in place and all should be good.
 

I see. I'll post v3 patch soon.

Also, I'll include in the v3 patch set, a patch related to the
question I raised in
http://lists.freedesktop.org/archives/systemd-devel/2015-June/033104.html.

--
Thanks.
HATAYAMA, Daisuke

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2015-06-18 Thread HATAYAMA Daisuke 
From: Lennart Poettering lenn...@poettering.net
Subject: Re: [systemd-devel] [PATCH v2] selinux: fix missing SELinux unit 
access check
Date: Wed, 17 Jun 2015 18:25:32 +0200

 On Wed, 10.06.15 14:40, HATAYAMA Daisuke (d.hatay...@jp.fujitsu.com) wrote:
 
 From 398deee74edb06b54b8a74c25697cd6d977d8f2d Mon Sep 17 00:00:00 2001
 From: HATAYAMA Daisuke d.hatay...@jp.fujitsu.com
 Date: Wed, 10 Jun 2015 14:10:31 +0900
 Subject: [PATCH] selinux: fix missing SELinux unit access check
 
 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.
 ---
  src/core/selinux-access.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)
 
 v2:
 - checking an error status by u-load_error to cover UNIT_ERROR case.
 
 diff --git a/src/core/selinux-access.c b/src/core/selinux-access.c
 index decd42f..ac52906 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_error != 0)
 +return u-load_error;
  r = mac_selinux_unit_access_check(u, message, 
 permission, error);
  if (r  0)
  return r;
 
 I commented on the issue now in github, could you please follow up
 there?
 
 https://github.com/systemd/systemd/pull/145
 

Thanks for your reviewing. However, unfortunately, I cannot use github
for some reason for now. So, please let me discuss in the mailing
list. I'm sorry for the inconvenience.

 I don't think we should check this, it doesn't matter if we managed
 to successfully load the unit or not, we should still allow access
 either way.

Sorry, I don't understand well. Do you mean removing this SELinux unit
access check? If so, it means that current logic since the following
commit, is changed, right?

$ git log -1 -p 4f7385fa496242f06aaf358b66b28d71348607b3
commit 4f7385fa496242f06aaf358b66b28d71348607b3
Author: Lubomir Rintel lkund...@v3.sk
Date:   Fri Dec 6 14:05:49 2013 +0100

selinux: Check access vector for enable/disable perm for each unit file

SELinux check will be done using the context of the unit file as as a
target instead of the default init_t context, allowing selinux control
on the level of individual units.

https://bugzilla.redhat.com/show_bug.cgi?id=1022762

--
Thanks.
HATAYAMA, Daisuke

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2015-06-17 Thread Lennart Poettering 
On Wed, 10.06.15 14:40, HATAYAMA Daisuke (d.hatay...@jp.fujitsu.com) wrote:

 From 398deee74edb06b54b8a74c25697cd6d977d8f2d Mon Sep 17 00:00:00 2001
 From: HATAYAMA Daisuke d.hatay...@jp.fujitsu.com
 Date: Wed, 10 Jun 2015 14:10:31 +0900
 Subject: [PATCH] selinux: fix missing SELinux unit access check
 
 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.
 ---
  src/core/selinux-access.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)
 
 v2:
 - checking an error status by u-load_error to cover UNIT_ERROR case.
 
 diff --git a/src/core/selinux-access.c b/src/core/selinux-access.c
 index decd42f..ac52906 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_error != 0)
 +return u-load_error;
  r = mac_selinux_unit_access_check(u, message, 
 permission, error);
  if (r  0)
  return r;

I commented on the issue now in github, could you please follow up
there?

https://github.com/systemd/systemd/pull/145

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 v2] selinux: fix missing SELinux unit access check

2015-06-10 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:20150610.144033.42163196.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 v2] selinux: fix missing SELinux unit access check

2015-06-10 Thread Harald Hoyer 
Am 10.06.2015 um 08:05 schrieb 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:20150610.144033.42163196.d.hatayama%40jp.fujitsu.com

imported as PR:
https://github.com/systemd/systemd/pull/145

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2015-06-09 Thread HATAYAMA Daisuke 
I found an oversight in the previous v1 patch, found at
http://lists.freedesktop.org/archives/systemd-devel/2015-June/032796.html,
that u-load_state == UNIT_ERROR in the error path of unit_load(),
that is:

int unit_load(Unit *u) {
int r;
...cut...
fail:
u-load_state = u-load_state == UNIT_STUB ? UNIT_NOT_FOUND : 
UNIT_ERROR;
u-load_error = r;
unit_add_to_dbus_queue(u);
unit_add_to_gc_queue(u);

log_unit_debug_errno(u, r, Failed to load configuration: %m);

return r;
}

To fix this, this v2 version looks at u-load_error to check error
status instead of u-load_state.

How to reproduce


Assume that test.service has not been loaded (even tried to get
loaded) in this boot. Then,

  ~]# cat ./foo.sh
  #! /bin/sh
  
  mv test.service /etc/systemd/system
  systemctl daemon-reload
  systemctl enable test.service
  ~]# ./foo.sh

Test


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

From 398deee74edb06b54b8a74c25697cd6d977d8f2d Mon Sep 17 00:00:00 2001
From: HATAYAMA Daisuke d.hatay...@jp.fujitsu.com
Date: Wed, 10 Jun 2015 14:10:31 +0900
Subject: [PATCH] selinux: fix missing SELinux unit access check

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.
---
 src/core/selinux-access.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

v2:
- checking an error status by u-load_error to cover UNIT_ERROR case.

diff --git a/src/core/selinux-access.c b/src/core/selinux-access.c
index decd42f..ac52906 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_error != 0)
+return u-load_error;
 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