[systemd-devel] [PATCH] selinux: fix selinux check for transient units
Michal Sekletar
msekleta at redhat.com
Mon Nov 18 14:45:35 PST 2013
On Mon, Nov 18, 2013 at 04:19:20PM -0500, Daniel J Walsh wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> 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.
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
>
> How does this patch look?
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.15 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlKKhFgACgkQrlYvE4MpobNd1ACbBrwtl5T/CEhCttI9QZ3IZbes
> k8UAoODr9J6D0CoyZfpdpvILU7QpxOD2
> =0zYK
> -----END PGP SIGNATURE-----
> From d8e5784f64a68580f136b99cd5e3fd173586312f 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 creation.
>
> SELinux does not have a path to check for a snapshot servic creation.
> This ends up giving us a bogus check.
>
> On snapshot creation we should check if the remote process type, has the ability to start a service with the type that systemd is running with.
> ---
> src/core/dbus-manager.c | 2 +-
> src/core/selinux-access.c | 9 +++++----
> src/core/selinux-access.h | 12 ++++++++++++
> 3 files changed, 18 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..53bb9b0 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); \
> --
> 1.8.4.2
>
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
More information about the systemd-devel
mailing list