On Tue, Nov 19, 2013 at 08:54:41AM -0500, Daniel J Walsh wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 11/18/2013 05:45 PM, Michal Sekletar wrote: > > On Mon, Nov 18, 2013 at 04:19:20PM -0500, Daniel J Walsh wrote: On > > 11/16/2013 08:10 AM, Lennart Poettering wrote: > >>>> On Thu, 14.11.13 15:43, Daniel J Walsh (dwa...@redhat.com) wrote: > >>>> > >>>>> > >>>>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 > >>>>> > >>>>> On 11/14/2013 12:50 PM, Harald Hoyer wrote: > >>>>>> On 11/05/2013 11:12 PM, Daniel J Walsh wrote: > >>>>>>> On 11/05/2013 12:22 PM, Lennart Poettering wrote: > >>>>>> > >>>>>>> Ok lets add a check that checks for start on a service labeled > >>>>>>> with the remote process label, then we can add rules like > >>>>>> > >>>>>>> allow systemd_logind_t self:service start > >>>>>> > >>>>>>> Or we can make it simpler and have the local end check against > >>>>>>> the init_t process. > >>>>>> > >>>>>>> allow systemd_logind_t init_t:service start; > >>>>>> > >>>>>>> Which is probably a better solution, if we have no way of > >>>>>>> differentiating the services. > >>>>>> > >>>>>>> Machineid usually runs as init_t now. > >>>>>> > >>>>>>> systemd-run runs as the label of the process that executes it, > >>>>>>> Usually unconfined_t, and sysadm_t. > >>>>>> > >>>>>> > >>>>>> has any solution been found for this? > >>>>>> > >>>>>> seems like one is needed for > >>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1008864 > >>>>>> > >>>>> > >>>>> I guess the question I have is do you expect a patch from me? Or > >>>>> are you guys working on it? I would go with the checking based on > >>>>> process label. > >>>> > >>>> I am hoping for a patch for this! > >>>> > >>>> Thanks, > >>>> > >>>> Lennart > >>>> > > > > This patch adds a new call for SELINUX_SNAPSHOT_ACCESS_CHECK, because I > > believe this error will happen when a snapshot is created. Also now pass > > in "system" when doing a system check, if it is doing a service check and > > does not pass in a unit file we will default the target to the label that > > systemd is running with. > > > >> Hi, > > > >> Maybe I am missing something but isn't this about transient units in > >> general, i.e. what about StartTransient()? What is going to change in > >> this case after applying this patch? tclass will be "system" since in > >> SELINUX_ACCESS_CHECK you now pass "system" as path and you will set > >> tclass in else branch to "system" which is afaik same as before. > > > In the current code, passing a unit_file of NULL (StartTransients has a NULL > UnitFile) will indicate that it should do a system check. My patch is > intended to change this so a NULL UnitFile will indicate that systemd should > check the access between the calling process label and the current process > label for a "service" access. Where as the SELINUX_ACCESS_CHECK will now pass > a "system" flag to the function to make it do a system check.
Hi Dan, Agreed, I get the idea, but this means that SELINUX_SNAPSHOT_ACCESS_CHECK should be performed in StartTransient branch in dbus-manager.c too and macro should be probably renamed to something like SELINUX_RUNTIME_UNIT_ACCESS_CHECK. Hope that makes sense. Michal > >> On the side note, you forgot to define SELINUX_SNAPSHOT_ACCESS_CHECK as > >> do {} while (false) in case if we don't HAVE_SELINUX. > > > >> It might be the case that I completely misunderstood what's this about, > >> in such case ignore this email. > > > >> Michal > > > > Thanks added SELINUX_SNAPSHOT_ACCESS_CHECK as do {} while (false) in case if > we don't HAVE_SELINUX. > > Updated patch. > > <snip> > > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.15 (GNU/Linux) > Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ > > iEYEARECAAYFAlKLbaEACgkQrlYvE4MpobPMMACeNeyrC3OBhx99DZ+JzOtV1ykZ > PvMAoJfiYBoJRBFgh2+FyOV+tNTuojNU > =9I5G > -----END PGP SIGNATURE----- > From b372b48016f5c015b34db9f53b7a54a64a5a84e8 Mon Sep 17 00:00:00 2001 > From: Dan Walsh <dwa...@redhat.com> > Date: Mon, 18 Nov 2013 15:52:37 -0500 > Subject: [PATCH] Fix SELinux check for snapshot and transitent unit creation. > > SELinux does not have a path to check for a snapshot or transient > unit files service creation. Currently systemd does a bogus check. > > If we don't have a unit file for a snapshot or transient unit we i > should check if the remote process label, has the required access > for a service with the SELinux label that systemd is running with. > > Add SELINUX_SNAPSHOT_ACCESS_CHECK for non SELinux environments > --- > src/core/dbus-manager.c | 2 +- > src/core/selinux-access.c | 9 +++++---- > src/core/selinux-access.h | 13 +++++++++++++ > 3 files changed, 19 insertions(+), 5 deletions(-) > > diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c > index 747bcfc..a60a568 100644 > --- a/src/core/dbus-manager.c > +++ b/src/core/dbus-manager.c > @@ -1112,7 +1112,7 @@ static DBusHandlerResult > bus_manager_message_handler(DBusConnection *connection, > dbus_bool_t cleanup; > Snapshot *s; > > - SELINUX_ACCESS_CHECK(connection, message, "start"); > + SELINUX_SNAPSHOT_ACCESS_CHECK(connection, message, "start"); > > if (!dbus_message_get_args( > message, > diff --git a/src/core/selinux-access.c b/src/core/selinux-access.c > index c7e951c..af34b9e 100644 > --- a/src/core/selinux-access.c > +++ b/src/core/selinux-access.c > @@ -374,8 +374,9 @@ int selinux_access_check( > goto finish; > } > > - if (path) { > - tclass = "service"; > + > + tclass = "service"; > + if (path && strneq(path,"system", strlen("system"))) { > /* get the file context of the unit file */ > r = getfilecon(path, &fcon); > if (r < 0) { > @@ -384,9 +385,9 @@ int selinux_access_check( > log_error("Failed to get security context on %s: > %m",path); > goto finish; > } > - > } else { > - tclass = "system"; > + if (path) > + tclass = "system"; > r = getcon(&fcon); > if (r < 0) { > dbus_set_error(error, DBUS_ERROR_ACCESS_DENIED, > "Failed to get current context."); > diff --git a/src/core/selinux-access.h b/src/core/selinux-access.h > index 2d7ac64..296bada 100644 > --- a/src/core/selinux-access.h > +++ b/src/core/selinux-access.h > @@ -36,6 +36,18 @@ int selinux_access_check(DBusConnection *connection, > DBusMessage *message, const > DBusConnection *_c = (connection); \ > DBusMessage *_m = (message); \ > dbus_error_init(&_error); \ > + _r = selinux_access_check(_c, _m, "system", (permission), > &_error); \ > + if (_r < 0) \ > + return bus_send_error_reply(_c, _m, &_error, _r); \ > + } while (false) > + > +#define SELINUX_SNAPSHOT_ACCESS_CHECK(connection, message, permission) \ > + do { \ > + DBusError _error; \ > + int _r; \ > + DBusConnection *_c = (connection); \ > + DBusMessage *_m = (message); \ > + dbus_error_init(&_error); \ > _r = selinux_access_check(_c, _m, NULL, (permission), > &_error); \ > if (_r < 0) \ > return bus_send_error_reply(_c, _m, &_error, _r); \ > @@ -57,6 +69,7 @@ int selinux_access_check(DBusConnection *connection, > DBusMessage *message, const > #else > > #define SELINUX_ACCESS_CHECK(connection, message, permission) do { } while > (false) > +#define SELINUX_SNAPSHOT_ACCESS_CHECK(connection, message, permission) do { > } while (false) > #define SELINUX_UNIT_ACCESS_CHECK(unit, connection, message, permission) do > { } while (false) > > #endif > -- > 1.8.4.2 > _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel