Re: [systemd-devel] [PATCH v2] smack: introduce new SmackLabelAccess option
On Fri, Nov 21, 2014 at 03:16:01PM +0900, WaLyong Cho wrote:
In case of systemd has _ label and run as root, if a service file
has User= option and the command line file has a special SMACK label
then systemd will fail to access to given file. SMACK label is ignored
for root uid processes. But if a service has a User= then systemd
will call setresuid() in the child process. After then it no more root
uid. So it should have some of accessable label for the command. To
set the before the uid is changed, introduce new SmackLabelAccess=.
^ This provides the motivation. Please add a paragraph which
describes the functionality.
I'm still bothered by the name SmackLabelAccess.
To recap the discussion, SMACK64EXEC is set on a file and it is an
analogue of setuid bit, except that it changes the 'current' smack
label instead of the UID. The analogy goes pretty far, since the
value set with User= may be overriden by the setuid bit on the file,
and SmackLabelAccess= may be be overriden by SMACK64EXEC on the file.
We use User= to set the user, so the name SmackLabel= would be the
most appropriate. But like Lennart said, SmackLabel= is already used
to label the socket file, so we need something different. By process
of elimination you arrived as SmackLabelAccess=. I don't like the name
because Access is misleading, because it suggests that the label is
only used when starting the process. But the normal case is that
SMACK64EXEC is *not* set, so the process will run with this label
until the end.
Please just call it SmackProcessLabel.
---
man/systemd.exec.xml | 15 +++
src/core/dbus-execute.c | 19 +
src/core/execute.c| 11
src/core/execute.h| 3 +++
src/core/load-fragment-gperf.gperf.m4 | 7 +++--
src/core/load-fragment.c | 50
+++
src/core/load-fragment.h | 1 +
src/shared/exit-status.h | 1 +
src/shared/smack-util.c | 20 ++
src/shared/smack-util.h | 1 +
10 files changed, 126 insertions(+), 2 deletions(-)
diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml
index e9af4ab..5381946 100644
--- a/man/systemd.exec.xml
+++ b/man/systemd.exec.xml
@@ -1138,6 +1138,21 @@
/varlistentry
varlistentry
+
termvarnameSmackLabelAccess=/varname/term
+
+listitemparaSet the SMACK security
+label to access given executable file in
+varnameExecStart=/varname.
If my analysis above is correct this should be reworded - it's not
just for access.
This option only has effect with
+varnameUser=/varname.
Is this entirely true? If User= is not given, and SMACK64EXEC is not set,
then this will determine the label of the process iiuc.
Because, SMACK access checking is ignored
+for root uid processes. If
varnameSmackLabelAccess=/varname is
+specified with varnameUser=/varname,
forked child systemd process
+set its /proc/$PID/attr/current to specified
label in
+varnameSmackLabelAccess=/varname. This
directive is ignored if
+SMACK is disabled. If prefixed by
literal-/literal, all errors
+will be ignored./para/listitem
Please describe the functionality in higher level language,
what the option does. Don't go into the details of SMACK.
Please mention that empty rhs can be used to unset.
+/varlistentry
+
+varlistentry
termvarnameIgnoreSIGPIPE=/varname/term
listitemparaTakes a boolean
diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c
index 9276da4..0764a42 100644
--- a/src/core/dbus-execute.c
+++ b/src/core/dbus-execute.c
@@ -508,6 +508,24 @@ static int property_get_apparmor_profile(
return sd_bus_message_append(reply, (bs),
c-apparmor_profile_ignore, c-apparmor_profile);
}
+static int property_get_smack_label_access(
+sd_bus *bus,
+const char *path,
+const char *interface,
+const char *property,
+sd_bus_message *reply,
+void *userdata,
+sd_bus_error *error) {
+
+ExecContext *c = userdata;
+
+assert(bus);
+assert(reply);
+assert(c);
+
+return sd_bus_message_append(reply, (bs),
c-smack_label_access_ignore, c-smack_label_access);
+}
+
static int property_get_personality(
sd_bus *bus,
Re: [systemd-devel] [PATCH v2] smack: introduce new SmackLabelAccess option
On 11/24/2014 02:36 AM, Zbigniew Jędrzejewski-Szmek wrote: On Fri, Nov 21, 2014 at 03:16:01PM +0900, WaLyong Cho wrote: In case of systemd has _ label and run as root, if a service file has User= option and the command line file has a special SMACK label then systemd will fail to access to given file. SMACK label is ignored for root uid processes. But if a service has a User= then systemd will call setresuid() in the child process. After then it no more root uid. So it should have some of accessable label for the command. To set the before the uid is changed, introduce new SmackLabelAccess=. ^ This provides the motivation. Please add a paragraph which describes the functionality. I'm still bothered by the name SmackLabelAccess. To recap the discussion, SMACK64EXEC is set on a file and it is an analogue of setuid bit, except that it changes the 'current' smack label instead of the UID. The analogy goes pretty far, since the value set with User= may be overriden by the setuid bit on the file, and SmackLabelAccess= may be be overriden by SMACK64EXEC on the file. We use User= to set the user, so the name SmackLabel= would be the most appropriate. But like Lennart said, SmackLabel= is already used to label the socket file, so we need something different. By process of elimination you arrived as SmackLabelAccess=. I don't like the name because Access is misleading, because it suggests that the label is only used when starting the process. But the normal case is that SMACK64EXEC is *not* set, so the process will run with this label until the end. Please just call it SmackProcessLabel. I'm agree. I will change. --- man/systemd.exec.xml | 15 +++ src/core/dbus-execute.c | 19 + src/core/execute.c| 11 src/core/execute.h| 3 +++ src/core/load-fragment-gperf.gperf.m4 | 7 +++-- src/core/load-fragment.c | 50 +++ src/core/load-fragment.h | 1 + src/shared/exit-status.h | 1 + src/shared/smack-util.c | 20 ++ src/shared/smack-util.h | 1 + 10 files changed, 126 insertions(+), 2 deletions(-) diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml index e9af4ab..5381946 100644 --- a/man/systemd.exec.xml +++ b/man/systemd.exec.xml @@ -1138,6 +1138,21 @@ /varlistentry varlistentry + termvarnameSmackLabelAccess=/varname/term + +listitemparaSet the SMACK security +label to access given executable file in +varnameExecStart=/varname. If my analysis above is correct this should be reworded - it's not just for access. Yes, if the file has SMACK64EXEC then the label only be used for access checking. But if that file has no SMACK64EXEC then the label is also set as the subject of executed process. The label is used also for checking so I'd like to just add when if the file has no SMACK64EXEC then the executed process will have specified label. This option only has effect with +varnameUser=/varname. Is this entirely true? If User= is not given, and SMACK64EXEC is not set, then this will determine the label of the process iiuc. You are totally right. I just focused on accessing to execute. As you said, if the file has no SMACK64EXEC and SmackProcessLabel= is given then the executed process will has given label. I will change this. Because, SMACK access checking is ignored +for root uid processes. If varnameSmackLabelAccess=/varname is +specified with varnameUser=/varname, forked child systemd process +set its /proc/$PID/attr/current to specified label in +varnameSmackLabelAccess=/varname. This directive is ignored if +SMACK is disabled. If prefixed by literal-/literal, all errors +will be ignored./para/listitem Please describe the functionality in higher level language, what the option does. Don't go into the details of SMACK. OK, I will. Please mention that empty rhs can be used to unset. Oh, I didn't imagine this. But I can't find some other proper operation. So I think if rhs was empty then just do like the config was not given. Really thanks to read. WaLyong +/varlistentry + +varlistentry termvarnameIgnoreSIGPIPE=/varname/term listitemparaTakes a boolean diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index 9276da4..0764a42 100644 ---
[systemd-devel] [PATCH v2] smack: introduce new SmackLabelAccess option
In case of systemd has _ label and run as root, if a service file
has User= option and the command line file has a special SMACK label
then systemd will fail to access to given file. SMACK label is ignored
for root uid processes. But if a service has a User= then systemd
will call setresuid() in the child process. After then it no more root
uid. So it should have some of accessable label for the command. To
set the before the uid is changed, introduce new SmackLabelAccess=.
---
man/systemd.exec.xml | 15 +++
src/core/dbus-execute.c | 19 +
src/core/execute.c| 11
src/core/execute.h| 3 +++
src/core/load-fragment-gperf.gperf.m4 | 7 +++--
src/core/load-fragment.c | 50 +++
src/core/load-fragment.h | 1 +
src/shared/exit-status.h | 1 +
src/shared/smack-util.c | 20 ++
src/shared/smack-util.h | 1 +
10 files changed, 126 insertions(+), 2 deletions(-)
diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml
index e9af4ab..5381946 100644
--- a/man/systemd.exec.xml
+++ b/man/systemd.exec.xml
@@ -1138,6 +1138,21 @@
/varlistentry
varlistentry
+
termvarnameSmackLabelAccess=/varname/term
+
+listitemparaSet the SMACK security
+label to access given executable file in
+varnameExecStart=/varname. This option
only has effect with
+varnameUser=/varname. Because, SMACK
access checking is ignored
+for root uid processes. If
varnameSmackLabelAccess=/varname is
+specified with varnameUser=/varname,
forked child systemd process
+set its /proc/$PID/attr/current to specified
label in
+varnameSmackLabelAccess=/varname. This
directive is ignored if
+SMACK is disabled. If prefixed by
literal-/literal, all errors
+will be ignored./para/listitem
+/varlistentry
+
+varlistentry
termvarnameIgnoreSIGPIPE=/varname/term
listitemparaTakes a boolean
diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c
index 9276da4..0764a42 100644
--- a/src/core/dbus-execute.c
+++ b/src/core/dbus-execute.c
@@ -508,6 +508,24 @@ static int property_get_apparmor_profile(
return sd_bus_message_append(reply, (bs),
c-apparmor_profile_ignore, c-apparmor_profile);
}
+static int property_get_smack_label_access(
+sd_bus *bus,
+const char *path,
+const char *interface,
+const char *property,
+sd_bus_message *reply,
+void *userdata,
+sd_bus_error *error) {
+
+ExecContext *c = userdata;
+
+assert(bus);
+assert(reply);
+assert(c);
+
+return sd_bus_message_append(reply, (bs),
c-smack_label_access_ignore, c-smack_label_access);
+}
+
static int property_get_personality(
sd_bus *bus,
const char *path,
@@ -636,6 +654,7 @@ const sd_bus_vtable bus_exec_vtable[] = {
SD_BUS_PROPERTY(UtmpIdentifier, s, NULL, offsetof(ExecContext,
utmp_id), SD_BUS_VTABLE_PROPERTY_CONST),
SD_BUS_PROPERTY(SELinuxContext, (bs),
property_get_selinux_context, 0, SD_BUS_VTABLE_PROPERTY_CONST),
SD_BUS_PROPERTY(AppArmorProfile, (bs),
property_get_apparmor_profile, 0, SD_BUS_VTABLE_PROPERTY_CONST),
+SD_BUS_PROPERTY(SmackLabelAccess, (bs),
property_get_smack_label_access, 0, SD_BUS_VTABLE_PROPERTY_CONST),
SD_BUS_PROPERTY(IgnoreSIGPIPE, b, bus_property_get_bool,
offsetof(ExecContext, ignore_sigpipe), SD_BUS_VTABLE_PROPERTY_CONST),
SD_BUS_PROPERTY(NoNewPrivileges, b, bus_property_get_bool,
offsetof(ExecContext, no_new_privileges), SD_BUS_VTABLE_PROPERTY_CONST),
SD_BUS_PROPERTY(SystemCallFilter, (bas),
property_get_syscall_filter, 0, SD_BUS_VTABLE_PROPERTY_CONST),
diff --git a/src/core/execute.c b/src/core/execute.c
index 5cfd4a1..6dd4cdd 100644
--- a/src/core/execute.c
+++ b/src/core/execute.c
@@ -83,6 +83,7 @@
#include af-list.h
#include mkdir.h
#include apparmor-util.h
+#include smack-util.h
#include bus-kernel.h
#include label.h
@@ -1618,6 +1619,16 @@ static int exec_child(ExecCommand *command,
}
}
+#ifdef HAVE_SMACK
+if (context-smack_label_access) {
+err = mac_smack_apply_pid(0,
context-smack_label_access);
+if (err 0) {
+*error =
