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

[James Hunt]

Hi David,

On 21/02/13 22:23, David Gaarenstroom wrote:
> December last year I announced I wanted to add seccomp filter support
> to upstart, and since that some things got in the way but here is my
> current code. These patches still have a RFC status as I expect some
> remarks anyway, and at the moment I don't have test-cases readily
> available. Nevertheless I'd like to know if I'm on the right track.
Firstly, thank you very much for working on this! I have quite a few comments
which, along with preparation for an impending release explains the delay in
responding :-)

= Specific Comments =

* init/Makefile.am: I'm wondering if the code generation would be better
  handled by a script which the Makefile calls to make it a little less cryptic?
* 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 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()?
* 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.
  - __NR_arch_prctl: this symbol is not defind on 32-bit systems.
  - install_seccomp_filter:
    - @nnp is always set so can this param be dropped?
    - sock_filter:
      - Can you add some details around those magic numbers?
      - Maybe 'filter' would be a clearer name than just 'f'.
    - 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.

= General comments =

* Some of the formatting is not consistent with the standard. See
  'HACKING' for editor+indent rules.
* 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 :-)
* 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.
* What happens when a job specifies 'seccomp-filter' is run in user
  mode ('init --user')?
* 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).

Kind regards,

James.

> 
> These patches a a new dependency on "gperf" which is used to create
> lookup tables for errno's and syscalls. Other than that there are no
> new dependencies. Part 2 and 3 add the seccomp-filter creation code
> itself, which is based on the same code as my guardian "seccomp
> wrapper" at https://gitorious.org/guardian/guardian
> 
> As agreed earlier, the seccomp-filter syntax is defined as:
> seccomp filter
>     : "seccomp-filter" WS [ '~' ] seccomp_rules;
> 
> seccomp_rules
>     : seccomp_rule ( WS seccomp_rule )*;
> 
> seccomp_rule
>     : systemcall ( ':' policy )?;
> 
> policy
>     : "allow"
>     | "errno" ( '(' errno ')' )?
>     | "kill"
>     | "trace"
>     | "trap" ( '(' errno ')' )?
>     ;
> 
> WS : ' '|'\t'|'\n';
> 
> The default policy is "allow explicitly listed syscalls as default
> policy, and use the kill policy for anything not explicitly listed".
> That is, unless the set of rules is preceded with "~" which reverts
> this policy, just like Systemd does. (deny explicitly listed syscalls
> as default policy, allow anything not explicitly listed")
> 
> E.g.:
>   seccomp-filter write
> 
> ...for "echo hello world".
> or:
> 
>   seccomp-filter getrlimit:allow setrlimit:errno(EACCES)
> 
> ...for a fictional program that is allowed to call getrlimit, but
> calls to setrlimit are simply ignored and errno is set to EACCES.
> or:
> 
>   seccomp-filter ~setuid socket
> 
> ...to prevent the usage of setuid and socket
> 
> 
> Kind regards,
> David Gaarenstroom
> 

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