[systemd-devel] [PATCH v2] smack: introduce new SmackLabelAccess option
Zbigniew Jędrzejewski-Szmek
zbyszek at in.waw.pl
Sun Nov 23 09:36:28 PST 2014
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>
> + <term><varname>SmackLabelAccess=</varname></term>
> +
> + <listitem><para>Set the SMACK security
> + label to access given executable file in
> + <varname>ExecStart=</varname>.
If my analysis above is correct this should be reworded - it's not
just for access.
> This option only has effect with
> + <varname>User=</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 <varname>SmackLabelAccess=</varname> is
> + specified with <varname>User=</varname>, forked child systemd process
> + set its /proc/$PID/attr/current to specified label in
> + <varname>SmackLabelAccess=</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>
> <term><varname>IgnoreSIGPIPE=</varname></term>
>
> <listitem><para>Takes 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 = EXIT_SMACK_LABEL_ACCESS;
> + return err;
> + }
> + }
> +#endif
> +
> if (context->user) {
> err = enforce_user(context, uid);
> if (err < 0) {
> diff --git a/src/core/execute.h b/src/core/execute.h
> index b16a24d..6cc8ad0 100644
> --- a/src/core/execute.h
> +++ b/src/core/execute.h
> @@ -142,6 +142,9 @@ struct ExecContext {
> bool apparmor_profile_ignore;
> char *apparmor_profile;
>
> + bool smack_label_access_ignore;
> + char *smack_label_access;
> +
> char **read_write_dirs, **read_only_dirs, **inaccessible_dirs;
> unsigned long mount_flags;
>
> diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4
> index 1d2debe..84e79c0 100644
> --- a/src/core/load-fragment-gperf.gperf.m4
> +++ b/src/core/load-fragment-gperf.gperf.m4
> @@ -95,8 +95,11 @@ m4_ifdef(`HAVE_SELINUX',
> `$1.SELinuxContext, config_parse_exec_selinux_context, 0, offsetof($1, exec_context)',
> `$1.SELinuxContext, config_parse_warn_compat, 0, 0')
> m4_ifdef(`HAVE_APPARMOR',
> -`$1.AppArmorProfile, config_parse_exec_apparmor_profile,0, offsetof($1, exec_context)',
> -`$1.AppArmorProfile, config_parse_warn_compat, 0, 0')'
> +`$1.AppArmorProfile, config_parse_exec_apparmor_profile, 0, offsetof($1, exec_context)',
> +`$1.AppArmorProfile, config_parse_warn_compat, 0, 0')
> +m4_ifdef(`HAVE_SMACK',
> +`$1.SmackLabelAccess, config_parse_exec_smack_label_access, 0, offsetof($1, exec_context)',
> +`$1.SmackLabelAccess, config_parse_warn_compat, 0, 0')'
> )m4_dnl
> m4_define(`KILL_CONTEXT_CONFIG_ITEMS',
> `$1.SendSIGKILL, config_parse_bool, 0, offsetof($1, kill_context.send_sigkill)
> diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c
> index 2ee16bd..2b1e7f3 100644
> --- a/src/core/load-fragment.c
> +++ b/src/core/load-fragment.c
> @@ -1313,6 +1313,56 @@ int config_parse_exec_apparmor_profile(
> return 0;
> }
>
> +int config_parse_exec_smack_label_access(
> + const char *unit,
> + const char *filename,
> + unsigned line,
> + const char *section,
> + unsigned section_line,
> + const char *lvalue,
> + int ltype,
> + const char *rvalue,
> + void *data,
> + void *userdata) {
> +
> + ExecContext *c = data;
> + Unit *u = userdata;
> + bool ignore;
> + char *k;
> + int r;
> +
> + assert(filename);
> + assert(lvalue);
> + assert(rvalue);
> + assert(data);
> +
> + if (isempty(rvalue)) {
> + free(c->smack_label_access);
> + c->smack_label_access = NULL;
> + c->smack_label_access_ignore = false;
> + return 0;
> + }
> +
> + if (rvalue[0] == '-') {
> + ignore = true;
> + rvalue++;
> + } else
> + ignore = false;
> +
> + r = unit_name_printf(u, rvalue, &k);
> + if (r < 0) {
> + log_syntax(unit, LOG_ERR, filename, line, -r,
> + "Failed to resolve specifiers, ignoring: %s", strerror(-r));
> + return 0;
> + }
> +
> + free(c->smack_label_access);
> + c->smack_label_access = k;
> + c->smack_label_access_ignore = ignore;
> +
> + return 0;
> +}
> +
> int config_parse_timer(const char *unit,
> const char *filename,
> unsigned line,
> diff --git a/src/core/load-fragment.h b/src/core/load-fragment.h
> index 65100c9..c770843 100644
> --- a/src/core/load-fragment.h
> +++ b/src/core/load-fragment.h
> @@ -94,6 +94,7 @@ int config_parse_job_mode_isolate(const char *unit, const char *filename, unsign
> int config_parse_exec_selinux_context(const char *unit, const char *filename, unsigned line, const char *section, unsigned section_line, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata);
> int config_parse_personality(const char *unit, const char *filename, unsigned line, const char *section, unsigned section_line, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata);
> int config_parse_exec_apparmor_profile(const char *unit, const char *filename, unsigned line, const char *section, unsigned section_line, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata);
> +int config_parse_exec_smack_label_access(const char *unit, const char *filename, unsigned line, const char *section, unsigned section_line, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata);
> int config_parse_address_families(const char *unit, const char *filename, unsigned line, const char *section, unsigned section_line, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata);
> int config_parse_runtime_directory(const char *unit, const char *filename, unsigned line, const char *section, unsigned section_line, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata);
> int config_parse_set_status(const char *unit, const char *filename, unsigned line, const char *section, unsigned section_line, const char *lvalue, int ltype, const char *rvalue, void *data, void *userdata);
> diff --git a/src/shared/exit-status.h b/src/shared/exit-status.h
> index f719580..6cab7bd 100644
> --- a/src/shared/exit-status.h
> +++ b/src/shared/exit-status.h
> @@ -78,6 +78,7 @@ typedef enum ExitStatus {
> EXIT_MAKE_STARTER,
> EXIT_CHOWN,
> EXIT_BUS_ENDPOINT,
> + EXIT_SMACK_LABEL_ACCESS,
> } ExitStatus;
>
> typedef enum ExitStatusLevel {
> diff --git a/src/shared/smack-util.c b/src/shared/smack-util.c
> index a8dccd1..b6c9643 100644
> --- a/src/shared/smack-util.c
> +++ b/src/shared/smack-util.c
> @@ -25,6 +25,7 @@
>
> #include "util.h"
> #include "path-util.h"
> +#include "fileio.h"
> #include "smack-util.h"
>
> #define SMACK_FLOOR_LABEL "_"
> @@ -123,6 +124,25 @@ int mac_smack_apply_ip_in_fd(int fd, const char *label) {
> return r;
> }
>
> +int mac_smack_apply_pid(pid_t pid, const char *label) {
> + int r = 0;
> + const char *p;
> +
> + assert(label);
> +
> +#ifdef HAVE_SMACK
> + if (!mac_smack_use())
> + return 0;
> +
> + p = procfs_file_alloca(pid, "attr/current");
> + r = write_string_file(p, label);
> + if (r < 0)
> + return r;
> +#endif
> +
> + return r;
> +}
> +
> int mac_smack_fix(const char *path, bool ignore_enoent, bool ignore_erofs) {
> int r = 0;
>
> diff --git a/src/shared/smack-util.h b/src/shared/smack-util.h
> index 68778da..50f55b1 100644
> --- a/src/shared/smack-util.h
> +++ b/src/shared/smack-util.h
> @@ -31,5 +31,6 @@ int mac_smack_fix(const char *path, bool ignore_enoent, bool ignore_erofs);
>
> int mac_smack_apply(const char *path, const char *label);
> int mac_smack_apply_fd(int fd, const char *label);
> +int mac_smack_apply_pid(pid_t pid, const char *label);
> int mac_smack_apply_ip_in_fd(int fd, const char *label);
> int mac_smack_apply_ip_out_fd(int fd, const char *label);
> --
> 1.9.3
>
> _______________________________________________
> 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