[systemd-devel] [PATCH] Add AppArmor profile switching

[Michael Scherer]

Le vendredi 14 février 2014 à 12:31 +0100, Lennart Poettering a écrit :
> On Fri, 14.02.14 12:21, Michael Scherer (misc at zarb.org) wrote:
> 
> > This permit to switch to a specific apparmor profile when starting a daemon. This
> > will result in a non operation if apparmor is disabled.
> > It also add a new build requirement on libapparmor for using this feature.
> > ---
> >  Makefile.am                           |  7 +++++++
> >  configure.ac                          | 13 +++++++++++++
> >  man/systemd.exec.xml                  | 13 +++++++++++++
> >  src/core/build.h                      |  8 +++++++-
> >  src/core/dbus-execute.c               |  1 +
> >  src/core/execute.c                    | 30 ++++++++++++++++++++++++++++++
> >  src/core/execute.h                    |  2 ++
> >  src/core/load-fragment-gperf.gperf.m4 |  3 ++-
> >  src/shared/exit-status.c              |  3 +++
> >  src/shared/exit-status.h              |  3 ++-
> >  10 files changed, 80 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 79c49e6..79d355c 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -776,6 +776,13 @@ libsystemd_shared_la_SOURCES += \
> >  	src/shared/seccomp-util.c
> >  endif
> >  
> > +libsystemd_shared_la_CFLAGS = \
> > +       $(AM_CFLAGS) \
> > +       $(APPARMOR_CFLAGS)
> > +
> > +libsystemd_shared_la_LIBADD = \
> > +       $(APPARMOR_LIBS)
> > +
> 
> Why is this in libsystemd-shared? This really should be added to the
> core la, not shared... Or am I missing something?

It did also look weird this morning when I reviewed the patch, but I
trusted more my past-self than my current-self, I will take a look and
send another patch if needed.

> >          SD_BUS_PROPERTY("SELinuxContext", "s", NULL, offsetof(ExecContext, selinux_context), SD_BUS_VTABLE_PROPERTY_CONST),
> > +        SD_BUS_PROPERTY("AppArmorProfile", "s", NULL,
> > offsetof(ExecContext, apparmor_profile),
> > SD_BUS_VTABLE_PROPERTY_CONST),
> 
> Hmm, so thinking about this, we should normalize both these options and
> turn the "s" signature into "(bs)", i.e. a structure made of a bool and
> the label, where the bool inidcates whether a non-existing label shall
> be ignored or not. We have the same split up when serializing exec
> commands, and we should do that here too...

So, you want a 2nd property SELinuxcontextIgnore/AppArmorProfileIgnore
that would be True when SELinuxContext/AppArmorProfile is prefixed by
'-', or also when SELinux/AppArmor is disabled ?

Also, SELinuxContext would be the context without the leading '-',
correct ?

> > +                        if (context->apparmor_profile && use_apparmor()) {
> > +                                char* c = context->apparmor_profile;
> > +                                bool ignore = false;
> > +                                if (c[0] == '-') {
> > +                                    c++;
> > +                                    ignore = true;
> 
> Indentation 8 chars please...

Will do

> > +                                }
> > +
> > +                                err = aa_change_onexec(context->apparmor_profile);
> > +                                if (err < 0 && !ignore) {
> > +                                        r = EXIT_APPARMOR;
> > +                                        goto fail_child;
> > +                                }
> > +                        }
> > +#endif
> >                  }
> >  
> > @@ -140,6 +140,8 @@ struct ExecContext {
> >  
> >          char *selinux_context;
> >  
> > +        char *apparmor_profile;
> > +
> 
> Similar as above, I'd like this to be stored normalized, i.e.:
> 
>   bool selinux_context_ignore;
>   char *selinux_context;
> 
>   bool apparmor_profile_ignore;
>   char *apparmor_profile;
> 
> Or similar...

So that would requires a custom change to load-fragment.c, so I guess
that's a separate task as we need to apply that to selinuxcontext.

So I will provides the change for SELinuxContext, then once accepted,
send a v3 of the apparmor patch with the code following the "style" of
SELinuxContext, is this ok ?

-- 
Michael Scherer