[systemd-devel] [PATCH] selinux: fix selinux check for transient units

Tue Nov 19 20:49:36 PST 2013

And I obviously attached wrong file...this is the right one, sorry

St 20. listopad 2013, 05:47:36 CET, Václav Pavlín napsal:
>
>
>
> Út 19. listopad 2013, 15:16:47 CET, Michal Sekletar napsal:
>>
>>
>> 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
>
>
> Hi,
>
> I tried to improve Dan's patch, so I added an empty call if selinux is
> not supported, renamed the function so it doesn't imply it's only for
> snapshots and used it in StartTransient and RemoveSnapshot as well. I
> wanted to test it, but I am not sure if the policy is already updated.
>
> Vaclav
>>
>>
>>>
>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>> 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
>>>
>>
>>
>>
>> _______________________________________________
>> systemd-devel mailing list
>> systemd-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
>
>
>
> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-SELinux-check-for-snapshot-creation.patch
Type: text/x-patch
Size: 5244 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20131120/20988791/attachment.bin>