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

[Tor Bug Tracker & Wiki]

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

Comment (by ctoader):

 I added comments at the end of each item describing their state. I need a
 bit of feedback, wasn't able to fully complete 2 items. Please let me know
 as soon as you can what I should do. I have also fixed a few bugs, might
 be worth looking over the commit history.

 Looking forward to a response!

 Replying to [comment:8 nickm]:


 > Notes:
 > * Does MAX_PARAM_LEN really mean that we can't have a filename longer
 than 64 characters? That sounds poor. Those happen all the time in real
 life. -- DONE
 > * Using strncmp instead of strcmp (in various places in sandbox.c) seems
 likely to give a wrong result if one value is a prefix of the other.
 strcmp() should be safe instead. --DONE
 >
 > * Do we truly only use getaddrinfo in the code to look up our own
 address? --Probably not, but the calls are many and complicated, do you
 have a suggestion as how to approach this? So far I have added extra
 logging for whenever a sandbox_getaddrinfo() is not successful and also I
 have implemented a way to allow storing more than one result.
 >
 > * Use doxygen style for documenting structure elments too. -- DONE
 >
 > * I'd still be more comfortable if the data structure for pfd_elem
 weren't so closely tied to the sandbox implementation. At the very least,
 it shouldn't be exposed in the header if it's implementation-specific. --I
 have made everything more dynamic in order to allow for different
 implementations. It is basically a linked list of void* data, and the
 pointer is interpreted in different ways depending on the implementation.
 >
 > * Prefer TOR_SLIST or TOR_SINGLEQ to manually managing a linked list.
 (They're defined in src/ext/tor_queue.h) --I found it very difficult to
 use the macros in tor_queue.h; also they modify the data structures a bit,
 is this a requirement or is it possible for the code to be accepted as is
 from this point of view. Everything seems much clearer to me without the
 macros but I would guess that someone familiar with them would think
 otherwise. Please let me know..
 >
 > * All functions that take no arguments need to be declared as foo(void),
 not as foo().  ex: sandbox_cfg_new() in sandbox.h, sandbox_init_filter()
 in main.c -- DONE..
 >
 > * Use "int" for a boolean, not "char".  (There's almost never a reason
 to use char this way) -- DONE
 >
 > * We should be supporting /dev/urandom, not /dev/random --DONE
 >
 > * When we call openssl's RAND_poll  periodically, does that call still
 work? --DONE (the function that was failing before the fix was
 RSAPrivateKey_dup in crypto_pk_copy_full, and it seems to be working fine,
 ran the code with the debugger)
 >
 > * In the functions that take an variable of filenames, it seems fragile
 to have to include the count of the filenames.  What if instead we make
 them end with NULL.? -- DONE
 >
 > Also:
 >
 > > > *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?
 > >
 >
 > Well, it does mean that we use an entire memory page for every interned
 string.  That seems inefficient.
 >
 > You don't need malloc/calloc here, since these strings are never freed.
 Instead, you could probably use something closer to what we have in
 memarea.c.  (For example, you could make an alternative constructor for
 memarea_t that takes a function which it uses to get a new chunk.)  This
 isn't a change you need to make yourself if you don't want to, though; i'm
 happy to hack it in later. -- DONE (the implementation is a bit different:
 due to the fact that we know the full memory size of all strings before
 the filter is applied, so I made it that the memory is allocated all at
 once and protected right before applying the filter, so memarea_t was not
 needed..)

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