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

[Ronny Chevalier]

2014/1/24 Lennart Poettering <lennart at poettering.net>:
> 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...
Ok, this sounds great for this.

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

>
>>
>>          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?
Oops, I didn't see this one...

>
>> +                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.
Unfortunately no, this is why I strdup the string from the .service,
but yes I see why this is not really a good idea...

Maybe by adding each syscall, after being validated by the libseccomp
API, in an array and sorting them ? And if the first element is the ~
then it's a blacklist ?

>
> Lennart
>
> --
> Lennart Poettering, Red Hat