2014/1/24 Lennart Poettering <lenn...@poettering.net>: > On Thu, 23.01.14 01:34, Ronny Chevalier (chevalier.ro...@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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel