Re: [systemd-devel] [PATCH] selinux: fix selinux check for transient units
-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. 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)
Re: [systemd-devel] [PATCH] selinux: fix selinux check for transient units
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
Re: [systemd-devel] [PATCH] selinux: fix selinux check for transient units
Ú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 (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 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 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); +
Re: [systemd-devel] [PATCH] selinux: fix selinux check for transient units
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 (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 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 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
Re: [systemd-devel] [PATCH] selinux: fix selinux check for transient units
-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 (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. 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 dwa...@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;
Re: [systemd-devel] [PATCH] selinux: fix selinux check for transient units
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 (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. 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 dwa...@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 +++
Re: [systemd-devel] [PATCH] selinux: fix selinux check for transient units
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 -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] selinux: fix selinux check for transient units
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 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 -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.15 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJShQ19AAoJEANOs3ABTfJwqLQQAMCwlsVui5GxrAIgm1pnRYy0 1vBZRJombQ93cYr5RbXcQzo9raGwD9C/TOA6PXJNyIKoxCh5unCMGdUB1pwR6y57 o1Uql1V1wVXPKqlsDsRdiKi14Qdz6e2CBqNQ1Gn1wx1JPvKPVy52iIMWjnFUCTzM FzKoX+CJlbR77tOgn24WxhP+Zll4QFqBAAwgptKBCyZf8lyHgsgTSgS7KDKAr/Jr v9BumGS9210fEHSQBJdkoaoYnMZOHPpaDxSDjZ76AqBw0MOksQsiamCRr20gbWnQ a8wvguQzTXhjKLeM0rX9x5hwrCI2Q4YL+VMsr5I1GPfR5GBIldmPhe8V4SYrJQY4 zDa+pHc1ubsuv/b4c9mHS/4Wl6IY8Nz6AVIAjvM0wR6cKJ+Ip09bEEeyIFWk7oRa RxNnFLwnUW4yweGCq4HlVv8r+SLpXIFkW9HkG1tr1UswB/jEC13wXW8WLfemGhuk XjMus/oMdQkd79wPvlfKF9JT4xTtx0u613kDEc/A6uEEoR4dwIeuLFf1Lss+zydg okLbsc8pLEFzXj1JKMut3DMEuhZss+WhLslGFEPKPpsAbHMEf42gJjuvMFb9aRTW V1qNW9adpMbqYC4MEzEV9SD4rwfDk1RQPW8iNEfm4XINcF1X2HK7+DGvCgVVeK8d tSM5P0ZhmXwimbrU63EH =IedG -END PGP SIGNATURE- ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] selinux: fix selinux check for transient units
-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. -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.15 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlKFNdUACgkQrlYvE4MpobNuXACg1eKUvMGKMv5zuwKHDvj44K+F L6gAn3sQtD0QvGUUmJWRGRSolZTdOqN0 =pYrx -END PGP SIGNATURE- ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] selinux: fix selinux check for transient units
On Thu, 31.10.13 15:51, Vaclav Pavlin (vpav...@redhat.com) wrote: From: Václav Pavlín vpav...@redhat.com Sorry, I don't understand what this patch is doing. Please explain in a commit message! --- src/core/selinux-access.c | 59 ++- src/core/selinux-access.h | 15 +--- 2 files changed, 70 insertions(+), 4 deletions(-) diff --git a/src/core/selinux-access.c b/src/core/selinux-access.c index 0a3ee18..5908a79 100644 --- a/src/core/selinux-access.c +++ b/src/core/selinux-access.c @@ -333,6 +333,50 @@ static int get_calling_context( return 0; } +static int load_context( +const char *path, +const char *name, +char **context) { + +int r = 0; +char l[LINE_MAX]; +char *p; +FILE *fd; + +fd = fopen(path, r); Please always use re rather than r. Not that it would matter much here, but Please always do. +if (!fd) +return -EEXIST; + +if (!fgets(l, sizeof(l), fd)) { Hmm, isn't this a job for read_one_line_file()? +if (feof(fd)) { +r = -EINVAL; +goto finish; +} + +log_error(Failed to read context file '%s': %m, path); +r = -errno; +goto finish; +} + +p = strtok (l, =); strtok() is evil, as it keeps internal state, please don't use it. Also one space too much... +if (p streq(p, name)) { +p = strtok (NULL, =); +if (!p) { +r = -EINVAL; +goto finish; +} + +*context = strdup(p); Missing OOM check. +} else +r = -EINVAL; + +finish: +if (fd) +fclose(fd); Please use _cleanup_close_ for cases like this. +} else if (class == CLASS_TRANSIENT) { +const char *context_path = selinux_systemd_contexts_path(); Where is selinux_systemd_contexts_path() define? I cant find it in this patch and neither in the sources? What am I missing? +if (_u-source_path || _u-fragment_path) \ +_r = selinux_access_check(_c, _m, CLASS_SERVICE, _u-source_path ?: _u-fragment_path, (permission), _error); \ +else\ +_r = selinux_access_check(_c, _m, CLASS_TRANSIENT, _u-id, (permission), _error); \ You can recognize transient units via the transient bool on the Unit structure. Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] selinux: fix selinux check for transient units
On Mon, Nov 4, 2013 at 5:06 PM, Lennart Poettering lenn...@poettering.net wrote: Sorry, I don't understand what this patch is doing. Please explain in a commit message! The file format should also be documented in the code itself, if not done by selinx, then we need to add the link to the doc. --- src/core/selinux-access.c | 59 ++- src/core/selinux-access.h | 15 +--- 2 files changed, 70 insertions(+), 4 deletions(-) diff --git a/src/core/selinux-access.c b/src/core/selinux-access.c index 0a3ee18..5908a79 100644 --- a/src/core/selinux-access.c +++ b/src/core/selinux-access.c @@ -333,6 +333,50 @@ static int get_calling_context( return 0; } +static int load_context( +const char *path, +const char *name, +char **context) { + +int r = 0; +char l[LINE_MAX]; +char *p; +FILE *fd; + +fd = fopen(path, r); Please always use re rather than r. Not that it would matter much here, but Please always do. +if (!fd) +return -EEXIST; + +if (!fgets(l, sizeof(l), fd)) { Hmm, isn't this a job for read_one_line_file()? +if (feof(fd)) { +r = -EINVAL; +goto finish; +} + +log_error(Failed to read context file '%s': %m, path); +r = -errno; +goto finish; +} + +p = strtok (l, =); strtok() is evil, as it keeps internal state, please don't use it. Also one space too much... +if (p streq(p, name)) { +p = strtok (NULL, =); +if (!p) { +r = -EINVAL; +goto finish; +} + +*context = strdup(p); Missing OOM check. +} else +r = -EINVAL; + +finish: +if (fd) +fclose(fd); Please use _cleanup_close_ for cases like this. It should probably just use: parse_env_file() it does all that. +} else if (class == CLASS_TRANSIENT) { +const char *context_path = selinux_systemd_contexts_path(); Where is selinux_systemd_contexts_path() define? I cant find it in this patch and neither in the sources? What am I missing? http://people.fedoraproject.org/~dwalsh/SELinux/Patches/0008-Add-selinux_systemd_contexts_path.patch +if (_u-source_path || _u-fragment_path) \ +_r = selinux_access_check(_c, _m, CLASS_SERVICE, _u-source_path ?: _u-fragment_path, (permission), _error); \ +else\ +_r = selinux_access_check(_c, _m, CLASS_TRANSIENT, _u-id, (permission), _error); \ You can recognize transient units via the transient bool on the Unit structure. Kay ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] selinux: fix selinux check for transient units
On Mon, 04.11.13 17:06, Lennart Poettering (lenn...@poettering.net) wrote: On Thu, 31.10.13 15:51, Vaclav Pavlin (vpav...@redhat.com) wrote: From: Václav Pavlín vpav...@redhat.com Sorry, I don't understand what this patch is doing. Please explain in a commit message! Hmm, so, here's another idea. The transient units are created by a client process. We could easily determine the label of that client process. Wouldn't it a better approach to calculate the label of the transient units somehow from the client process' label? This way wouldn't need any additional systemd-specific infrastructure in libselinux. Dan, could that work? Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] selinux: fix selinux check for transient units
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 11/04/2013 02:05 PM, Lennart Poettering wrote: On Mon, 04.11.13 17:06, Lennart Poettering (lenn...@poettering.net) wrote: On Thu, 31.10.13 15:51, Vaclav Pavlin (vpav...@redhat.com) wrote: From: Václav Pavlín vpav...@redhat.com Sorry, I don't understand what this patch is doing. Please explain in a commit message! Hmm, so, here's another idea. The transient units are created by a client process. We could easily determine the label of that client process. Wouldn't it a better approach to calculate the label of the transient units somehow from the client process' label? This way wouldn't need any additional systemd-specific infrastructure in libselinux. Dan, could that work? Lennart I suppose it would. The only label we have the the clients is the process label. What process types create these runtime objects and what do they request to do with them? Currently systemd asks for permissions on system class and service class, where class system { ipc_info syslog_read syslog_mod syslog_console module_request halt reboot status undefined enable disable reload } class service { start stop status reload kill load enable disable } Do we have to add a rule like allow sysadm_t networkmanager_t:service start; Were networkmanager_t is a process type? -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.15 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlJ3/gsACgkQrlYvE4MpobPWbQCfWElx/pR6cOjQKM1Ad0cE/eU1 cAcAoJ1k49KbB143/NJH/DEfl0aRLhnn =eao5 -END PGP SIGNATURE- ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel