[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: Thu, 18 Jun 2015 13:30:43 +0200

> On Thu, 18.06.15 18:29, HATAYAMA Daisuke (d.hatayama at 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