[tor-bugs] #9249 [Tor]: GSOC seccomp stage 2

[Tor Bug Tracker & Wiki]

#9249: GSOC seccomp stage 2
--------------------------------------------+-------------------------------
 Reporter:  ctoader                         |          Owner:  nickm
     Type:  enhancement                     |         Status:  new  
 Priority:  normal                          |      Milestone:       
Component:  Tor                             |        Version:       
 Keywords:  tor-relay gsoc seccomp sandbox  |         Parent:  #5756
   Points:                                  |   Actualpoints:       
--------------------------------------------+-------------------------------

Comment(by ctoader):

 Just so I can keep here a complete list, there are also the memory leaks
 in sandbox_init_filter();

 > * LENGHT is mispelled. (Probably it should be ARRAY_LENGTH, and it
 should be in util.h).

 (FIXED)

 > * filter_dynamic should probably be static; nothing needs to use it
 outside of sandbox.c, right?

 (FIXED)

 > * Exposing sandbox_static_cfg_t, pfd_elem, and sandbox_filter_func_t in
 the header seems wrong: those are not part of the public sandbox
 interface, right? They only make sense for seccomp2.

 I added support for a sandbox_t struct which will have an array of
 function pointers associated with the filter as well as a list of
 parameters for the filters; this should be public as a configuration
 context for a sandbox; different definitions of the struct may exist based
 on the implementation so it's not necessarily seccomp specific..

 > * Some of the restrictions in sandbox.c don't make sense for security;
 we should at least document why we're doing them. For example, why
 restrict clock_gettime() to CLOCK_MONOTONIC? Why restrict time() to NULL?

 I agree that some restrictions may be dropped as they don't bring security
 value.. need to discuss which..

 > * Some of the restrictions in sandbox.c don't make sense given how the
 syscalls work. (e.g., surely we're sometimes calling epoll_ctl with
 EPOLL_CTL_DEL!)

 (FIXED)

 > * We probably want to restrict execve arguments in the same way we
 restrict open, openat, etc.

 (FIXED)

 > * It's confusing to me how filter_dynamic is a linked list of so many
 different things. Maybe once it's documented, it will make sense?

 it is a linked list of parameters, their index, type, and associated
 syscall; if prot is set to 0, then the pointer was not protected.. i kept
 changing it so it's not documented yet..

 > * It seems excessive to use a separate mmap() for every prot_strdup()
 call. Perhaps instead we should have a big mmap that fills up with
 strings, and which we mprotect when we're done. This could wait until
 later though.

 I don't really see the upside to that other than reducing the number of
 calls from N to 1; the downside however would be having to manage that
 memory similar to malloc/calloc but having only one allocated segment (or
 maybe have another block allocated when a limit is reached..?); do you
 consider it is worth the added code complexity?

 > * It's impossible for a static array to be NULL, and this code makes
 Coverity complain about deadcode issues.

 (FIXED)

 > * The structures and functions all need documentation.

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9249#comment:5>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online