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

Tue Nov 19 20:47:36 PST 2013

Ú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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-SELinux-check-for-snapshot-creation.patch
Type: text/x-patch
Size: 3755 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20131120/e56c7cc0/attachment-0001.bin>