[RFC/PATCH: 0/7] Add seccomp filtering support to Upstart

[James Hunt]

On 01/03/13 13:20, David Gaarenstroom wrote:
>> = Specific Comments =
>>
>> * init/job_process.c: job_process_spawn():
>>   - No check on return code of install_seccomp_filter(): this gives the
>>     behaviour that if the job specifies 'seccomp-filter' but the seccomp filter
>>     failed to install, the job is still allowed to run. I'm not sure
>>     this is desirable?
> 
> I'm not sure what would be "desirable" here? If it fails simply
> because seccomp is not enabled (user runs a customized kernel) I'm
> inclined to pretend nothing happened... (install_seccomp_filter will
> produce a waring).
If seccomp is not available that seems reasonable, but if seccomp *is*
installed, it probably wouldn't be.

> 
> 
>>   - I can understand why the seccomp filter it not applied until just
>>     before the child exec, but why is the parsing of the rules deferred to
>>     this point (such that each job that uses the seccomp-filter stanza has
>>     its rules parsed at startup)? Can't we parse the rules fully in
>>     parse_job() and then simply apply the already-parsed-and-verified rules
>>     prior to exec()?
> 
> We could, but at the moment I'm using (almost) the same code for
> Upstart as for a pet project of mine. I could split it up later, but I
> would like to focus on other remarks/issues first...
> 
> 
>> * init/seccomp_filter.c:
>>   - macros: Need comment headers.
>>   - ACT_TYPE:
>>     - Should use CamelCase for consistency with existing codebase.
>>     - Need description of elements.
>>   - actions: I'd prefer a macro to calculate 'len' to avoid
>>     nasty runtime surprises if someone forgets to change len for a new
>>     entry (or just drop the len element and use strcmp()).
>>   - ACT_CMP:
>>     - Needs a "Returns" comment.
>>     - Should be lower-case as it's a function.
>>     - sock_filter:
>>       - Can you add some details around those magic numbers?
>>       - Maybe 'filter' would be a clearer name than just 'f'.
> 
> I'll fix these.
> 
> 
>>   - __NR_arch_prctl: this symbol is not defined on 32-bit systems.
> 
> That's odd, I have tested that quite a while ago... I will fix this
> and test on x86 to make sure. (I cannot test on AArch64, but I will
> try to test on ARM (Nexus 7) as well.)
> 
> 
>>   - install_seccomp_filter:
>>     - @nnp is always set so can this param be dropped?
> 
> Because I am planning to add a separate NoNewPriviledges stanza later.
> (I'll probaly move the two prctl's from install_seccomp_filter to
> job_process.c)
> 
> 
>>     - As above: I think the parsing should be performed in parse_job.c,
>>       not here if possible. Note that by so doing, if the user specifies
>>       incorrect seccomp-filter syntax, the job will fail to parse and
>>       register. This is a good thing if it is syntactically incorrect.
> 
> Agreed and will split up parsing and installing later...
> 
>> = General comments =
>>
>> * Some of the formatting is not consistent with the standard. See
>>   'HACKING' for editor+indent rules.
> 
> AFAIK seccomp_filter.[ch] is consistent, but for the other files I've
> tried to blend in... ;-)
> parse_job.c itself is far from consistent BTW
> 
> 
>> * Tests: All new features need to provide tests, so you'll need to
>>   create a init/tests/test_seccomp_filter.c and also consider adding some system
>> tests to the
>> increasingly-erroneously-named-as-it-does-lots-more-than-the-name-suggests
>> test_initctl.c :-)
> 
> I've already been working on test_parse_job.c and test_job_process.c,
> these test-cases  should cover almost everything (assuming touch, true
> and echo are present).
That's a reasonable assumption to make.

> I'm thinking of adding a small test application as well that calls one
> syscall and then exits with errno, so I can test that part too. I
> think that covers everything I can think of without having to run as
> root...
> 
>> * The new files need the correct Copyright header.
>> * nih_warn() should make use of gettext's _() for translations.
>> * init/man/init.5: Need to document 'seccomp-filter' stanza.
> 
> Will look into this.
> 
>> * What happens when a job specifies 'seccomp-filter' is run in user
>>   mode ('init --user')?
> 
> It will run fine, however, since NoNewPriviledges is set (mandatory as
> non-root) you might not be able to run setuid'd programs properly
> (e.g. sudo, ping)
> 
>> * Stateful re-exec: Any change to the internal data structures
>>   requires updates to the serialisation and deserialisation code.
>>   Currently, that looks to be quite easy since you've only added a
>>   string to JobClass. So, you'd need to update job_class_serialise() and
>>   add a call to state_set_json_string_var_from_obj() for seccomp_filter,
>>   and upate job_class_deserialise() to add a call to
>>   state_get_json_string_var_strict(). However, if you can change the
>>   code to parse the seccomp filters at parse_job() time, I suspect you
>>   might end up changing the parameters to install_seccomp_filter() so
>>   the advice above might not be correct. The stateful re-exec code
>>   is pretty easy to update as all the existing data types used are
>>   catered for with macros (see init/*.c:*_serialise(), init/*.c:*_deserialise()
>>   and init/state.[ch]). Note though that you'll also need to add a new
>>   upgrade test as the JSON used to represent internal state on stateful
>>   re-exec will change with the advent of JobClass->seccomp_filter (see
>>   test_upgrade() in init/tests/test_state.c).
> 
> Will look into this as well...
> 
> 
> Thanks for all the remarks. I was going to send new patches including
> test-cases and fixing some errors, but I'll fix most of these items
> first. I've also added support for SECCOMP_RET_INFO if defined in
> <linux/seccomp.h> in that case "info" is recognized as a policy.
> SECCOMP_RET_INFO will be added in future kernels and will print the
> syscall number in the kernel log (and allow it), which is very handy
> to find what syscalls are being used by a program. I intend to add a
> modifier to make it the default policy (for unlisted syscalls).
> 
> Could you maybe explain what happens if a job is respawned or
> restarted, will it parse the job again or will it re-use the existing
> job class data?
If a job is respawned, or restarted using 'initctl restart', it will retain its
original configuration (as documented in initctl(8). To force a reload of the
config, either stop the job and start it again, or run 'initctl
reload-configuration' (but note this won't affect existing instances).

> 
> 
> Kind regards,
> David Gaarenstroom
> 

Kind regards,

James.
--
James Hunt
____________________________________
http://upstart.ubuntu.com/cookbook
http://upstart.ubuntu.com/cookbook/upstart_cookbook.pdf