[systemd-devel] [PATCH 2/2] syscallfilter: port to libseccomp

[Lennart Poettering]

On Thu, 23.01.14 01:34, Ronny Chevalier (chevalier.ronny at gmail.com) wrote:

> ---
> Hi,
> 
> This patch ports the syscall filter to libseccomp. It can be disable with
> --disable-seccomp and is enabled by default if libseccomp is present.
> 
> Maybe I should add a warning when parsing SyscallFilter in a .service
> if seccomp has been disabled ?

There's "config_warn_commpat()" for cases like this, but it is currently
only used for the optional sysv-hookup. I am pretty sure we should reuse
this scheme here...
> 
> Now the SyscallFilter property is a duplicate of the string in the .service
> file instead of a uint array.

>  
>  const sd_bus_vtable bus_exec_vtable[] = {
> @@ -420,7 +416,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("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", "au", property_get_syscall_filter, 0, SD_BUS_VTABLE_PROPERTY_CONST),
> +        SD_BUS_PROPERTY("SystemCallFilter", "s", property_get_syscall_filter, 0, SD_BUS_VTABLE_PROPERTY_CONST),
>          SD_BUS_VTABLE_END

Soooo, strictly speaking this is API breakage. However, given that the
old API was soo naive and pretty much unusable and arch specific, and I
hence doubt that people wrote code against this specific property I am
tempted to just let go this through...

> @@ -1936,49 +1928,61 @@ int config_parse_syscall_filter(const char *unit,
>                                  const char *rvalue,
>                                  void *data,
>                                  void *userdata) {
> +#ifdef HAVE_SECCOMP
> +#define SECCOMP_RULE_ADD(ctx, action, id, name) \
> +        do {\
> +                r = seccomp_rule_add(ctx, action, id, 0);\
> +                if (r < 0)\
> +                        log_syntax(unit, LOG_ERR, filename, line, -r,\
> +                                   "Failed to add syscall filter, ignoring: %s", name);\
> +        } while(0)

I'd prefer if this could become a normal function rather than a
macro... There's no benefit of this being a macro...

>  
>          ExecContext *c = data;
>          Unit *u = userdata;
>          bool invert = false;
> +        uint32_t action = SCMP_ACT_ALLOW;
>          char *w;
>          size_t l;
>          char *state;
> +        int r;
>  
>          assert(filename);
>          assert(lvalue);
>          assert(rvalue);
>          assert(u);
>  
> +

No spurious double whitespace lines, please....

>          if (isempty(rvalue)) {
>                  /* Empty assignment resets the list */
> -                free(c->syscall_filter);
> +                seccomp_release(c->syscall_filter);
>                  c->syscall_filter = NULL;
> +                free(c->syscall_filter_string = NULL);
                                                 ^^^^^^^
This certainly looks wrong?                

> +                c->syscall_filter_string = NULL;
>                  return 0;
>          }
> +        c->syscall_filter_string = strdup(rvalue);

I really don't like this I must say. Handing the unnormalized original
configuration string back to the clients via the bus API sounds wrong.

Doesn't libseccomp provide a way to enumerate the contents of the
defined filter again? I'd really prefer if we could find a way that
specifiying a filter of "read write" and of "write read" would actually
result in the same string exposed via the bus.

Lennart

-- 
Lennart Poettering, Red Hat