[systemd-devel] [PATCH] socket: introduce SELinuxLabeledNet option

[Lennart Poettering]

On Tue, 05.08.14 13:46, Michal Sekletar (msekleta at redhat.com) wrote:

> This makes possible to spawn service instances triggered by socket with
> MLS/MCS SELinux labels which are created based on information provided by
> connected peer.
> 
> Implementation of label_get_socket_label derived from xinetd.
> ---
>  man/systemd.socket.xml                | 11 ++++++
>  src/core/execute.c                    | 23 +++++++++++-
>  src/core/execute.h                    |  1 +
>  src/core/load-fragment-gperf.gperf.m4 |  3 ++
>  src/core/socket.c                     | 18 +++++++--
>  src/core/socket.h                     |  2 +
>  src/shared/label.c                    | 69 +++++++++++++++++++++++++++++++++++
>  src/shared/label.h                    |  1 +
>  8 files changed, 123 insertions(+), 5 deletions(-)
> 
> diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
> index 09a7311..959f46f 100644
> --- a/man/systemd.socket.xml
> +++ b/man/systemd.socket.xml
> @@ -588,6 +588,17 @@
>                          </varlistentry>
>  
>                          <varlistentry>
> +                          <term><varname>SELinuxLabeledNet=</varname></term>
> +                          <listitem><para>Takes a boolean
> +                          value. Controls whether systemd attempts to figure out
> +                          SELinux label used for instantiated service from
> +                          information handed by peer over the
> +                          network. Configuration option has effect only
> +                          on sockets with <literal>Accept=</literal>
> +                          mode set to <literal>yes</literal>.</para></listitem>
> +                        </varlistentry>

Could you please indent the same way as the other settings?

>  
>  #ifdef HAVE_SECCOMP
>  #include "seccomp-util.h"
> @@ -1729,6 +1730,22 @@ int exec_spawn(ExecCommand *command,
>                                          goto fail_child;
>                                  }
>                          }
> +
> +                        if (context->selinux_labeled_net && use_selinux()) {
> +                                _cleanup_free_ char *label = NULL;
> +
> +                                err = label_get_socket_label(socket_fd, command->path, &label);
> +                                if (err < 0) {
> +                                        r = EXIT_SELINUX_CONTEXT;
> +                                        goto fail_child;
> +                                }
> +
> +                                err = setexeccon(label);
> +                                if (err < 0) {
> +                                        r = EXIT_SELINUX_CONTEXT;
> +                                        goto fail_child;
> +                                }
> +                        }

If both SELinuxContext= and SELinuxLabeledNet= are set we should
probably not execute one after the other, but only one of them.

Also, before applying selinux_labeled_net you really need to check for
socket_fd if it is >= 0...

I think it would be good to enclose processing of SELinuxContext= and
SELinuxLabeledNet= processing in single "if (use_selinux())" check, and then
use SELinuxLabeledNet= if it is set and if socket_fd >= 0, and otherwise
apply SELinuxContext= if it is set.

I have no understanding of the actual selinux logic here, but it looks
right, I hope this is blessed by dwalsh or someome else knowledgable in
these things?


> -
> +#ifdef HAVE_SELINUX
> +                        if (!know_label && s->selinux_labeled_net) {
> +                                r = getcon(&label);
> +                                if (r < 0)
> +                                        return r;
> +                                know_label = true;
> +                        }
> +#endif
>                          if (!know_label) {
>  

Can you explain this bit? Why would we label the listening socket with our own
label here?

>                                  r = socket_instantiate_service(s);
> @@ -1773,6 +1782,9 @@ static void socket_enter_running(Socket *s, int cfd) {
>                  cfd = -1;
>                  s->n_connections ++;
>  
> +                if (s->selinux_labeled_net)
> +                        service->exec_context.selinux_labeled_net = true;
> +

This I don't like. We shouldn#t make permanent changes here... I'd
prefer if we could pass this somehow else, so that the service isn't
changed permanently...

I must say I feel a bit uneasy about the naming of SELinuxContext= and
SELinuxLabeledNet=... One uses the term "context", the other one
"label". afaiu that's actually the same thing, no? If it is, can we use
the same terminology here? (which would mean sticking to "context" since
that's what we already are using...)

Lennart

-- 
Lennart Poettering, Red Hat