[systemd-devel] [PATCH] selinux: fix selinux check for transient units
Michal Sekletar
msekleta at redhat.com
Tue Nov 19 06:16:47 PST 2013
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 (dwalsh at 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 <dwalsh at 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
>
More information about the systemd-devel
mailing list