Re: [tor-bugs] #9168 [Tor]: GSOC seccomp stage 1

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#9168: GSOC seccomp stage 1
--------------------------------------------+-------------------------------
 Reporter:  ctoader                         |          Owner:  nickm             
     Type:  enhancement                     |         Status:  needs_revision    
 Priority:  normal                          |      Milestone:  Tor: 0.2.5.x-final
Component:  Tor                             |        Version:                    
 Keywords:  tor-relay gsoc seccomp sandbox  |         Parent:                    
   Points:                                  |   Actualpoints:                    
--------------------------------------------+-------------------------------
Changes (by nickm):

  * keywords:  => tor-relay gsoc seccomp sandbox
  * status:  needs_review => needs_revision
  * milestone:  => Tor: 0.2.5.x-final


Comment:

 Initial comments:

   * Please use the same file header header style as the rest of Tor.
   * Make sure that the code passes "make check-spaces" with no warnings.
   * By convention, stuff in src/common shouldn't include or use stuff in
 src/or.  So instead of calling get_options() there, have main.c do "if
 (get_options()->Sandbox) tor_global_sandbox();"
   * Why do the periodic events need special sandboxing?
   * In C, if you need to declare a function with no arguments, you have to
 say "int foo(void)".  Unlike Java or C++, if you say "int foo()",  you're
 declaring a function with an unspecified number of arguments, not a no-
 argument function.
   * We should strip out the unused , #if 0 parts before merging.
   * You don't need to check the return value of get_options(); it is not
 allowed to fail.
   * You don't need to use do { } while(0) and break statements to achieve
 single-point-of-return.  We don't believe in single-point-of-return.  Just
 use "return" in the middle of the function if that is clearest.  If
 there's significant cleanup, you can also use the "goto end" pattern.
   * By convention, we document every function; see doc/HACKING for more
 information.
   * Function documentation should explain what the function does in enough
 detail that somebody else could implement the function correctly using
 only the documentation.
   * Why is filters.h a separate header?  Why is it a header at all?
   * I don't think it's allowed to call fprintf from a signal callback.
 Also, fprintf() isn't likely to work at runtime for most Tor users.
   * It looks like this patch would break compilation on all non-Linux
 hosts.
   * Somebody (me?) needs to write the configure.in magic here.
   * Is "log and ignore" the right approach to disallowed syscalls? "Log
 and exit" might also be a good choice.
   * What happened to the magic "socketcall" syscall number?  We needed it
 for test_filter -- do we not need it for general_filter?  It seems that
 maybe we don't, so long as we're calling seccomp_rule_add ... but the code
 here seems to be calling seccomp_rule_add_exact (why?).
   * We should check the return value of tor_global_sandbox, and probably
 exit with an error if it fails.
   * We should probably warn if the user turns on Sandbox, but we were
 built without sandbox support.

 After that's done and we merge this, I'd say the next steps should be
 trying to make the sandbox more restrictive if we can: making the filters
 apply to specific files, only turning on execve when we must, and so on.
 But that's another ticket. :)

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9168#comment:2>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs