Bug 1022762

Why is 

+ optional_policy(`
+	init_enable_services(kdumpgui_t)
+	init_disable_services(kdumpgui_t)
+	init_reload_services(kdumpgui_t)
+')

needed? It should be covered by

kdump_systemctl(kdumpgui_t)

I was trying to deal with the following denials (where the target appears to be init_t:system, not kdump_unit_file_t:service). Maybe something else is wrong and there's a more clever way around it I am not aware of?

type=USER_AVC msg=audit(1382570178.393:449): pid=1 uid=0 auid=4294967295 ses=4294967295  subj=system_u:system_r:init_t:s0 msg='avc:  denied  { enable } for auid=-1 uid=0 gid=0 scontext=system_u:system_r:kdumpgui_t:s0-s0:c0.c1023 tcontext=system_u:system_r:init_t:s0 tclass=system  exe="/usr/lib/systemd/systemd" sauid=0 hostname=? addr=? terminal=?'

        Was caused by:
                Missing type enforcement (TE) allow rule.

                You can use audit2allow to generate a loadable module to allow this access.

type=USER_AVC msg=audit(1382570178.394:450): pid=1 uid=0 auid=4294967295 ses=4294967295  subj=system_u:system_r:init_t:s0 msg='avc:  denied  { reload } for auid=-1 uid=0 gid=0 scontext=system_u:system_r:kdumpgui_t:s0-s0:c0.c1023 tcontext=system_u:system_r:init_t:s0 tclass=system  exe="/usr/lib/systemd/systemd" sauid=0 hostname=? addr=? terminal=?'

        Was caused by:
                Missing type enforcement (TE) allow rule.

                You can use audit2allow to generate a loadable module to allow this access.

Ping?

Yes, you are right. We are also getting this issue and we are working on systemd+SELinux patch. 

Basically we don't want to have

+ optional_policy(`
+	init_enable_services(kdumpgui_t)
+	init_disable_services(kdumpgui_t)
+	init_reload_services(kdumpgui_t)
+')

We have

SELinux access check scon=system_u:system_r:kdumpgui_t:s0-s0:c0.c1023 tcon=system_u:system_r:init_t:s0 tclass=system perm=enable path=(null) cmdline=systemctl cmdline=systemctl enable kdump.service: -13

which means there is no "path" and the kdump unit file is not checked.

Created attachment 829782 [details]
Proposed fix

Attaching proposed fix.
Please have a look.

'systemctl disable' for non-existent units is currently a noop. Proposed patch would change that, and I don't think that's a good idea. Maybe something like that instead:

+                STRV_FOREACH(i, l) {
+                        Unit *u;
+
+                        u = manager_get_unit(m, *i);
+                        if (u)
+                               SELINUX_UNIT_ACCESS_CHECK(u, connection, message, streq(member, "MaskUnitFiles") ? "disable" : "enable");
+                }

Similarly for the other check — if the file cannot be found, it is nicer to return ENOENT than EPERM.
Can you rework the patch to do that? Once we have that, we can port it to current git.

There's also the issue that this code shouldn't do anything if selinux is not compiled in, so the loop should be under some conditional include. But the patch doesn't apply to current git, so it would have to be reworked anyway.

Created attachment 834057 [details]
[PATCH v2] selinux: Check access vector for enable and disable perm for  each unit file

(In reply to Zbigniew Jędrzejewski-Szmek from comment #10)

Thank you for your comments, Zbyszek. I've attempted to address all of your concerns, please have a look at updated patch.

Thank you!
Lubo

Thank you for the patch.

Committed as http://cgit.freedesktop.org/systemd/systemd/commit/?id=4f7385f.

This commit was in systemd-209, but I forgot to close this issue.

Zbigniew I think that patch we have upstream is not correct. It is not going to work for unit files which are newly created and admin wants to enable them right away, i.e. in order to enable unit file, there must be loaded unit. I'd argue that those two are orthogonal.

@Michal: I'm wondering if you or anyone else plans to look into a proper solution? I could have a look over the weekend, but I'd prefer if we didn't do a duplicate work.

@Lubomir: I'm working on something else currently, it's all yours :)

@Michal: but wouldn't manager_get_unit() always load the unit?

I don't think it loads anything, because it does only return hashmap_get(m->units, name).

I plan to look into this, but currently I am busy with some other work. Lubo please let me know if you will look into it or not. Thanks!

FWIW, re-posting my thoughts about a proper fix (originally posted under RHEL clone of the same bug).

------

Problem is that module src/core/install.c where check against the file should be done is part of systemd but also baked into systemctl binary as well, and code gets executed when --root argument is given to systemctl, so it operates on the filesystem on its own and doesn't call D-Bus API of systemd. When D-Bus API is used, same code (src/core/install.c) looks for the file and enables it, however it is executed in PID 1 and scon to check against should be determined from D-Bus request.

------

So from the above paragraph, I think it is clear that we have to pass down the stack D-Bus API caller context or NULL and then acquire our own context, i.e. code is executed in systemctl, and then do the SELinux check.

(In reply to Michal Sekletar from comment #17)
> I don't think it loads anything, because it does only return
> hashmap_get(m->units, name).
Oops, you're right. But it should be enough to use
manager_load_unit() instead.

> I plan to look into this, but currently I am busy with some other work. Lubo
> please let me know if you will look into it or not. Thanks!
> 
> FWIW, re-posting my thoughts about a proper fix (originally posted under
> RHEL clone of the same bug).
> 
> ------
> 
> Problem is that module src/core/install.c where check against the file
> should be done is part of systemd but also baked into systemctl binary as
> well, and code gets executed when --root argument is given to systemctl, so
> it operates on the filesystem on its own and doesn't call D-Bus API of
> systemd. When D-Bus API is used, same code (src/core/install.c) looks for
> the file and enables it, however it is executed in PID 1 and scon to check
> against should be determined from D-Bus request.
> 
> ------
> 
> So from the above paragraph, I think it is clear that we have to pass down
> the stack D-Bus API caller context or NULL and then acquire our own context,
> i.e. code is executed in systemctl, and then do the SELinux check.
Hm, I'm not sure. systemctl --root does not allow privilege escalation so I don't think it has to check anything. Having the check only in the systemd path should be enough.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #18)
> Oops, you're right. But it should be enough to use
> manager_load_unit() instead.

Hmm...Thinking about reasons why it shouldn't work and only instance enablement comes to mind. But I may be mistaken and it would just work. Which would be great.