[systemd-devel] [PATCH 2/2 v3] socket: introduce SELinuxContextFromNet option

[Lennart Poettering]

On Thu, 28.08.14 12:56, Michal Sekletar (msekleta at redhat.com) wrote:

> On Tue, Aug 26, 2014 at 08:54:01PM +0200, Lennart Poettering wrote:
> > On Mon, 25.08.14 10:02, Michal Sekletar (msekleta at redhat.com) wrote:
> > 
> > > +int label_get_our_label(char **label) {
> > > +        int r = 0;
> > > +        char *l = NULL;
> > > +
> > > +#ifdef HAVE_SELINUX
> > > +        r = getcon(&l);
> > > +        if (r < 0)
> > > +                return r;
> > > +
> > > +        *label = l;
> > > +#endif
> > > +
> > > +        return r;
> > > +}
> > 
> > Hmm, we shouldn't return success if selinux support is turned off, and
> > we don't write anything to *label... This really should return -ENOTSUP
> > or so, i figure....
> 
> Are you suggesting that we should change other functions in label.c as well?
> AFAICT all non-void functions from that module implement same pattern
> now.

No, I think the calls that can safely be NOPs if selinux support is
missing should just become NOPs and return 0, pretending success...

But lavel_get_out_label() above is not one of those: if we return 0 when
selinux is not available, then the calling code might assume success and
use the returned string which is then non-initialized. Hence, in this
case we should tell the caller that what he asked for didn't work, anhd
return ENOTSIP or so....

> > > +#include "util.h"
> > > +
> > > +#ifdef HAVE_SELINUX
> > > +#include <selinux/selinux.h>
> > > +#include <selinux/context.h>
> > > +#endif
> > > +
> > >  int label_init(const char *prefix);
> > >  void label_finish(void);
> > >  
> > > @@ -39,6 +46,8 @@ void label_context_clear(void);
> > >  void label_free(const char *label);
> > >  
> > >  int label_get_create_label_from_exe(const char *exe, char **label);
> > > +int label_get_our_label(char **label);
> > > +int label_get_child_mls_label(int socket_fd, const char *exec, char **label);
> > >  
> > >  int label_mkdir(const char *path, mode_t mode);
> > >  
> > > @@ -49,3 +58,11 @@ int label_apply(const char *path, const char *label);
> > >  int label_write_one_line_file_atomic(const char *fn, const char *line);
> > >  int label_write_env_file(const char *fname, char **l);
> > >  int label_fopen_temporary(const char *path, FILE **_f, char **_temp_path);
> > > +
> > > +#ifdef HAVE_SELINUX
> > > +DEFINE_TRIVIAL_CLEANUP_FUNC(security_context_t, freecon);
> > > +DEFINE_TRIVIAL_CLEANUP_FUNC(context_t, context_free);
> > > +
> > > +#define _cleanup_security_context_free_ _cleanup_(freeconp)
> > > +#define _cleanup_context_free_ _cleanup_(context_freep)
> > > +#endif
> > 
> > Hmm, wouldn't it suffice to have the latter four lines simply in
> > label.c, not in the header files? I'd prefer if we didn't have to
> > include the selinux headers from the header file...
> 
> Quick check revealed that we are using for example security_context_t type in
> following modules other than label.c: 
> 
> src/core/selinux-access.c
> src/core/selinux-setup.c
> src/journal/journald-server.c
> src/journal/journald-stream.c /* now cleanup macros can't be of any use here */
> src/nspawn/nspawn.c /* here as well */
> 
> Honestly I don't really care either way. Given that use cases in above modules
> for new macros are really minimal (1-2 per module) we can just put those four
> lines to C file make use of macros there and leave the rest as is.

Hmm, maybe introduce selinux-util.h where we can place these macros,
which then can be used in the modules that actually include the selinux
headers...

What I want to avoid here is that the selinux headers are pulled into
every single .c module indirectly... I want that the compiler will still
generate errors if somebody calls an selinux API function if somebody
forgot to include the header files...

Thanks,

Lennart

-- 
Lennart Poettering, Red Hat