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

[Michal Sekletar]

On Wed, Aug 13, 2014 at 09:42:14PM +0200, Lennart Poettering wrote:
> 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?

Will do.

> 
> >  
> >  #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.

I think that it makes sense to set both and resulting label will be combination
of both. Note that from SELinux label we acquire from network we are using only
security level part.

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

Agreed.

> 
> 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?

I will change it in a way such that resulting label will be combination of
SELinuxContext and whatever security level is handed to us from network. In most
cases we will combine label from target executable and security label from
network tough.

> 
> 
> > -
> > +#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?

This is because of MLS SELinux policy implementation details. If we relabel to
the context acquired from the target binary then it if not possible to connect to
the socket because SELinux denies a packet receive on the socket.

https://github.com/selinux-policy/selinux-policy/blob/rawhide-base/policy/mls#L361

> 
> >                                  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...)

Consistency arguments seems reasonable. I will rename then.

Thanks for review and sorry for premature push.

> 
> Lennart

Michal
> 
> -- 
> Lennart Poettering, Red Hat