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

[Lennart Poettering]

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 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