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

[HATAYAMA Daisuke]

From: Lennart Poettering <lennart at 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.hatayama at jp.fujitsu.com) wrote:
> 
>> From 398deee74edb06b54b8a74c25697cd6d977d8f2d Mon Sep 17 00:00:00 2001
>> From: HATAYAMA Daisuke <d.hatayama at 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 <lkundrak at 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