Re-add SELinux checks for unit install operations #20387

cgzones · 2021-08-05T14:17:34Z

The checks (permission verbs) in question are enable for the operations enable, reenable, link and unmask and disable for the operations disable and mask; those SELinux permissions exist in the the reference and fedora SELinux policy.
These checks were dropped with v225 (see #1044) due to incomplete and missing infrastructure in the unit handling code.

In addition the operations preset and revert are checked with the (also already existing) SELinux permission reload.
(In the future I'd like to separate them into a new permission modify? together with calls to the standard D-Bus interfaces at org.freedesktop.DBus.Properties.Set.)

Job actions JOB_RELOAD_OR_START and JOB_VERIFY_ACTIVE are now checked with the permission start instead of reload.

The D-Bus filter now falls back to an instance check in case no unit can be decoded (e.g. the job has finished or the unit does not exist).

Reduced proposal of #10023
Closes: #1050

/cc @SELinuxProject @fedora-selinux

bluca

not a selinux expert, so just some minor comments

src/core/selinux-access.c

src/core/selinux-access.h

src/core/dbus-manager.c

lgtm-com · 2021-08-06T14:39:27Z

This pull request introduces 6 alerts when merging ee8aee9 into 3c79a56 - view on LGTM.com

new alerts:

6 for FIXME comment

ghost · 2021-08-08T16:00:36Z

I run-tested this. Works as expected. Please merge.

keszybz · 2021-08-09T08:19:53Z

In addition the operations preset and revert are checked with the (also already existing) SELinux permission reload.

I'd expect preset to use enable permission verb too: it's very similar to enable command.

ghost · 2021-08-09T08:30:11Z

In addition the operations preset and revert are checked with the (also already existing) SELinux permission reload.

I'd expect preset to use enable permission verb too: it's very similar to enable command.

I tend to agree, but probably best to use a new patch set to add new stuff that might affect compatibility?

ghost · 2021-08-09T08:38:42Z

Ignore this comment. I am not sure how this shows up in the logs.

keszybz

Thank you for working on this. It looks reasonable, modulo some minor style issues and the pending decision what permission name to use. The only problem I see is with the path lookup logic. I think it should be integrated with the other path lookup logic.

src/core/selinux-access.c

src/core/dbus-manager.c

keszybz · 2021-08-09T08:33:16Z

src/test/test-install.c

@@ -52,12 +52,12 @@ int main(int argc, char* argv[]) {

        log_info("/*** enable **/");

-        r = unit_file_enable(UNIT_FILE_SYSTEM, 0, NULL, (char**) files, &changes, &n_changes);
+        r = unit_file_enable(UNIT_FILE_SYSTEM, 0, NULL, (char**) files, &changes, &n_changes, NULL, NULL);


Wouldn't it be useful to install the callback in a few cases and verify that it is called correctly, at least that it's called at all for the each of the verb types? I think it's fairly easy to forget the check for some verb, or maybe call it twice, and a unittest would make this less likely.

src/core/dbus-callbackdata.h

keszybz · 2021-08-09T08:45:19Z

src/shared/install.c

+        assert_return(unit_name_is_valid(name, UNIT_NAME_ANY), -EINVAL);
+        assert(ret_path);
+
+        r = lookup_paths_init(&paths, scope, 0, root_dir);


I can't say that I like this at all. lookup_paths_init() is already a fairly heavy operation. And filesystem lookup with directory scanning even more so. IIUC, we would get this overhead e.g. for all masked units, without any form of caching. Also, this duplicates the search logic, which is very finnicky, and it's likely that it does things differently than the existing search logic. So I don't think we can/should do this.

If it's necessary to look up paths for masked units, then instead please change the code that does the creation of the unit paths hashmaps to also return the paths for masked units somehow.

So you prefer the previous implementation by keeping track of real unit files behind masked ones at load time, like 7672981
(more recent rebased version: d1e8da4)?

why not return some fixed label if a unit is masked?

I do not know what you mean but in case you're talking about SELinux labels here then please be aware that there is no such thing as a "fixed label". One should not hard code identifiers as they are supposed to be configurable by security policy.

well, if there's no object to read a label off then we can't provide a label. so either it's a fixed label or we just don't do selinux here. you choose.

but the idea of going behind the mask to find a label just defeats the point of masking, that's a no go.

Ive been thinking about these checks, and then it occurred to me that it is probably very easy to circumvent these checks anyway, as these operations essentially amount to just managing symlinks in /etc/systemd/system. Maybe I am overlooking something.

src/core/selinux-access.c

ghost · 2021-08-09T09:31:33Z

Ignore this comment. I am not sure how this shows up in the logs.

If you have a preset of disable "unit" and if you would use the enable check for this then effectively disabling a unit would show up as enabling a unit. That then means that if someone has permission to enable a unit, then that entity can bypass the disable check by disabling the unit via preset. Am I wrong?

Please lets just table "preset" operation and try to address the regression first.

src/core/dbus-manager.c

poettering

as before, i am concerned about the effects on selinux policy this has. we need some form of ack from the policy maintainers if you rename verbs, since they need to update their policy accordingly. it would probably make sense to split some of the earlier changes out of this PR, i.e. the ones not immediately affecting policy, and that can thus be merged safely without an ack from them

src/core/manager.c

poettering · 2021-08-09T10:47:06Z

src/core/dbus-callbackdata.h

+        sd_bus_error *error;
+        const char *func;
+
+        const char *selinux_permission;
 } MacUnitCallbackUserdata;


why not put this in src/core/selinux-access.h? no need for a header file of its own, no?

The idea was to have a generic SELinux independent structure.

poettering · 2021-08-09T10:51:06Z

src/shared/install.c

+        assert_return(unit_name_is_valid(name, UNIT_NAME_ANY), -EINVAL);
+        assert(ret_path);
+
+        r = lookup_paths_init(&paths, scope, 0, root_dir);


why not return some fixed label if a unit is masked?

src/shared/install.c

poettering · 2021-08-09T10:55:33Z

src/core/dbus.c

@@ -235,42 +234,37 @@ static int mac_selinux_filter(sd_bus_message *message, void *userdata, sd_bus_er

        path = sd_bus_message_get_path(message);

-        if (object_path_startswith("/org/freedesktop/systemd1", path)) {


uh, we generally assume the objects leading towards the manager won't result in access issues when talked to, this should stay

Isn't

object_path_startswith("/org/freedesktop/systemd1", path)

and

streq_ptr(path, "/org/freedesktop/systemd1")

basically the same?

poettering · 2021-08-09T10:56:31Z

src/core/dbus.c

                _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL;
                pid_t pid;

                r = sd_bus_query_sender_creds(message, SD_BUS_CREDS_PID, &creds);
                if (r < 0)
-                        return 0;


why this change?

Are there legitimately use cases where the connection does not contain credentials? I tested several ways but not encountered one (although I am not a D-Bus expert).

src/core/dbus.c

poettering · 2021-08-09T10:59:03Z

src/core/job.c

                return "reload";
+        }


this will affect selinux policy. unless you manage to get some form of ack from the selinux policy maintainer (in fedora) i am not convinced we should make the change. They must adapt their policy to anything like this after all.

Or at least we need to boot a VM with selinux enabled and check that there's no new denials… I think the policy is actually fairly permissive in this regard, i.e. objects that have permission for 'start' also have it 'reload' and vice versa.

Exactly. Oh and by the way Packit/Copr makes it really easy to try this out!

I can confirm that running Fedora Rawhide and systemd with this patches does not generate any new avc denial.

reload perm is always connected with start perm in current Fedora Rawhide policy:

# sesearch -A -t systemd_unit_file_type -p reload | grep -v " start " | wc -l 0

ghost · 2021-08-09T11:10:32Z

as before, i am concerned about the effects on selinux policy this has. we need some form of ack from the policy maintainers if you rename verbs, since they need to update their policy accordingly. it would probably make sense to split some of the earlier changes out of this PR, i.e. the ones not immediately affecting policy, and that can thus be merged safely without an ack from them

AFAIK the current revision of this pull request already takes this into account with: ee8aee9

I have run-tested things with a policy that predates the revert. There are no new operations targeted and there are no new permissions added. There is one instance where an operation changed permission but that should not cause any compatiblity issues.

poettering · 2021-08-09T12:03:22Z

I have run-tested things with a policy that predates the revert. There are no new operations targeted and there are no new permissions added. There is one instance where an operation changed permission but that should not cause any compatiblity issues.

The verbs are changed there? That will affect policy, no?

ghost · 2021-08-09T12:22:00Z

I have run-tested things with a policy that predates the revert. There are no new operations targeted and there are no new permissions added. There is one instance where an operation changed permission but that should not cause any compatiblity issues.

The verbs are changed there? That will affect policy, no?

I agree that we should just drop that change and yes the verb has changed but the operations applicable are currently rarely used at all and if they are used then they are probably not used without access to the other verb as well. effectively that means that this change most likely has little impact if any at all. Also keep in mind that in the half of a decade that this functionality was reverted things evolved (or rather stagnated) regardless and re-adding this might cause some disruption anyway. Also I think when I asked the author of this on IRC earlier about this issue he mentioned that support for this functionality was added late in the first place which makes it that much less likely that the functionality was leveraged in the first place.

But again, I agree that it is easier to just exclude it from this PR.

poettering · 2021-08-09T14:28:23Z

from where i stand i can't really determine if anyone relies on these verbs being something or something else. I neither know selinux well enough nor how it's used IRL, but I am pretty sure that the policy people should comment on this, and ack it, because they are the ones that even they don't rely on it now will be the likeliestto come in touch with it one day.

I am sorry that it's hard getting anything out of those maintainers, judging by the previous attempts, but i am really not in the position to know enough to go wild with this and not require their input.

ghost · 2021-08-09T14:38:42Z

from where i stand i can't really determine if anyone relies on these verbs being something or something else. I neither know selinux well enough nor how it's used IRL, but I am pretty sure that the policy people should comment on this, and ack it, because they are the ones that even they don't rely on it now will be the likeliestto come in touch with it one day.

I am sorry that it's hard getting anything out of those maintainers, judging by the previous attempts, but i am really not in the position to know enough to go wild with this and not require their input.

Nah, you're right. SELinux is supposed to be flexible and customizable. Regardless of whether Fedora or whatever distribution leverages this there might be end-users relying on this. SELinux is a framework it is not some fixed policy.

ghost · 2021-08-09T14:50:18Z

It should not be a big deal to remove that questionable code from this pull request. Just remove the questionable stuff and then please just merge it.

poettering · 2021-08-16T09:15:09Z

So I read @bachradsusi's comments as a (weak) blessing that we can go ahead with this. Hence, please fix the code issues I pointed out above, then we can look into merging this. thanks

poettering · 2021-09-29T15:43:49Z

ping

ghost · 2021-10-23T13:42:24Z

Tested. Functionality wise looks good. Thanks.

poettering · 2022-07-11T12:48:31Z

In light of this coment: #10023 (comment) from an upstream selinux person i'd prefer if we implement the suggested approach here properly too.

Specifically:

the UnitFileInstallInfo structure should have a label filled in which we read whenever we fill the structure, i.e. when we read the file of disk.

This also means the obstructed stuff should go, simply because the new rule would be that file contents we operate with and inode metadata of what we read should always remain in sync. i.e. we should do fgetfilecon_raw() on the fd we also read() from, and nothing else.

And that means that masked files should use some generic label, and not tried to look "behind the mask".

cgzones · 2022-07-12T15:46:04Z

This also means the obstructed stuff should go, simply because the new rule would be that file contents we operate with and inode metadata of what we read should always remain in sync. i.e. we should do fgetfilecon_raw() on the fd we also read() from, and nothing else.

And that means that masked files should use some generic label, and not tried to look "behind the mask".

I still like to have a distinct unit label for unmasking (which requires the bookkeeping of the obstructed path), otherwise unmask is an all-or-nothing operation. I'll try to rebase once #23986 has been decided.

poettering · 2022-07-12T16:24:17Z

I still like to have a distinct unit label for unmasking (which requires the bookkeeping of the obstructed path), otherwise unmask is an all-or-nothing operation. I'll try to rebase once #23986 has been decided.

Masking is supposed to be big big hammer. Not to be used in clean workflows really, hence it's totally OK if the label is hard to deal with, because it is supposed to be unwieldy and drastic in its effect.

Or to say this differently: I think if you want to obstruct systemd to deal with some object stored on disk I am very sure it should be done comprhensively and thus also the label of it become obstructed and hence a generic default used instead.

Make it possible to retrieve the MAC target context for an operation on a masked unit beforehand the execution. E.g. if unit foo is installed at `/lib/systemd/system/foo.service` with file context `foo_unit_t`, but masked via a symbolic link `/etc/systemd/system/foo.service` -> `/dev/null`, this bookkeeping enables to check the unmask operation on the desired context `foo_unit_t`.

shared: Add a MAC callback call to unit_file_* functions to check unit operations, like enable or disable. The MAC callback is supplied with the unit name as first argument. The second argument is a blob used by the back-end MAC specific check. If no check should be performed, pass NULL pointers. core: Pass NULL, a generic callback will be implemented in the next commit. systemctl: Adopt to the new callback by passing NULL. No callback check is needed when called on the client side, cause regular kernel MAC checks (like write or unlink) on the unit file will be performed.

…Linux Register it for D-Bus exposed methods. Do not perform checks for reset/reset and add-dependency in this patch set, only re-add previously existing checks. Those operations should probably get a separate *new* SELinux permission verb, which should be done coordinated in another patch-set.

Do not allow access if D-Bus credentials are unavailable. Perform system check if job or unit not found.

JOB_RELOAD_OR_START might collapse into JOB_START and JOB_VERIFY_ACTIVE is similar to JOB_START. Use the more heavy and appropriate permission "start" instead of "reload".

Add permission "modify", used by the generic D-Bus filter, for adding dependencies and reverting units. Add permission "preset" use for presetting units. SELinux has the concept of "deny_unknown", which configured by the policy decides whether unknown permissions cause access decisions to be denied or allowed. The standard policies of Debian and Fedora use the setting of "allow", thus new permissions would be allowed. Thereby there should not be access regressions, but policies still should adopt them to avoid too broad access privilege.

poettering · 2023-06-14T15:23:49Z

could you rebase this, please?

bluca requested changes Aug 5, 2021

View reviewed changes

bluca added the selinux label Aug 5, 2021

cgzones force-pushed the selinux_unit_ops branch from 3d1a370 to b5e770a Compare August 6, 2021 13:37

github-actions bot added the systemctl label Aug 6, 2021

keszybz reviewed Aug 9, 2021

View reviewed changes

poettering reviewed Aug 9, 2021

View reviewed changes

src/core/dbus-manager.c Show resolved Hide resolved

poettering requested changes Aug 9, 2021

View reviewed changes

poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Aug 9, 2021

poettering added the needs-rebase label Sep 29, 2021

cgzones force-pushed the selinux_unit_ops branch 4 times, most recently from 36d52e8 to 88b79e4 Compare October 22, 2021 19:54

yuwata removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks needs-rebase labels Oct 23, 2021

bluca added the please-review label May 21, 2022

cgzones force-pushed the selinux_unit_ops branch from 9a68fea to 6c061e0 Compare May 27, 2022 12:37

mrc0mmand added the needs-rebase label Jun 15, 2022

cgzones force-pushed the selinux_unit_ops branch from 6c061e0 to f64df07 Compare July 11, 2022 12:19

bluca removed the needs-rebase label Jul 11, 2022

poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review labels Jul 11, 2022

poettering added the needs-rebase label Aug 3, 2022

cgzones force-pushed the selinux_unit_ops branch from f64df07 to 475a893 Compare August 5, 2022 18:20

cgzones marked this pull request as draft August 5, 2022 18:21

bluca removed the needs-rebase label Aug 5, 2022

cgzones force-pushed the selinux_unit_ops branch 2 times, most recently from 98f135e to a5dfe1d Compare August 6, 2022 18:49

cgzones force-pushed the selinux_unit_ops branch from a5dfe1d to b627133 Compare August 22, 2022 21:12

cgzones force-pushed the selinux_unit_ops branch from b627133 to a84c3dc Compare November 4, 2022 15:26

bluca removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Nov 4, 2022

cgzones added 2 commits November 4, 2022 17:06

cgzones force-pushed the selinux_unit_ops branch from a84c3dc to ac6d871 Compare November 4, 2022 16:10

cgzones force-pushed the selinux_unit_ops branch from ac6d871 to dae0838 Compare November 4, 2022 17:24

cgzones added 3 commits November 4, 2022 20:39

core: avoid bypasses in D-BUS SELinux filter

29869ee

Do not allow access if D-Bus credentials are unavailable. Perform system check if job or unit not found.

core: tweak job_type_to_access_method SELinux permissions

62b725c

JOB_RELOAD_OR_START might collapse into JOB_START and JOB_VERIFY_ACTIVE is similar to JOB_START. Use the more heavy and appropriate permission "start" instead of "reload".

cgzones force-pushed the selinux_unit_ops branch from dae0838 to dcbf1ba Compare November 4, 2022 19:40

poettering added the needs-rebase label Jun 14, 2023

		@@ -235,42 +234,37 @@ static int mac_selinux_filter(sd_bus_message message, void userdata, sd_bus_er

		path = sd_bus_message_get_path(message);

		if (object_path_startswith("/org/freedesktop/systemd1", path)) {

Re-add SELinux checks for unit install operations #20387

Are you sure you want to change the base?

Re-add SELinux checks for unit install operations #20387

Conversation

cgzones commented Aug 5, 2021

bluca left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Aug 6, 2021

ghost commented Aug 8, 2021

keszybz commented Aug 9, 2021

ghost commented Aug 9, 2021

ghost commented Aug 9, 2021 • edited by ghost

keszybz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 9, 2021 • edited by ghost

poettering left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bachradsusi Aug 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 9, 2021 • edited by ghost

poettering commented Aug 9, 2021

ghost commented Aug 9, 2021

poettering commented Aug 9, 2021

ghost commented Aug 9, 2021

ghost commented Aug 9, 2021

poettering commented Aug 16, 2021

poettering commented Sep 29, 2021

ghost commented Oct 23, 2021

poettering commented Jul 11, 2022

cgzones commented Jul 12, 2022

poettering commented Jul 12, 2022

poettering commented Jun 14, 2023

ghost commented Aug 9, 2021 •

edited by ghost

ghost commented Aug 9, 2021 •

edited by ghost

bachradsusi Aug 12, 2021 •

edited

ghost commented Aug 9, 2021 •

edited by ghost