[tor-dev] Torsocks 2.0 RC code review

[Nick Mathewson]

On Tue, Nov 26, 2013 at 5:17 PM, David Goulet <dgoulet at ev0ke.net> wrote:
> Hi everyone,
>
> The torsocks 2.0-rc3 code is getting quite stable in my opinion. There
> are still some issues but nothing feature critical.
>
>         https://github.com/dgoulet/torsocks
>
> I would really love to have help with code review so it can get accepted
> as a replacement in the near future. Some of you already gave feedbacks
> so thanks but now it needs the "seal of approval" from the community :).

Hi, David!   Here's what I found on initial review for a couple of hours.

(Want me to review your Tor-related project for a while?  I'd be glad
to do a review exchange; just do a useful nontrivial review for an
important pending Tor patch I've written.  That's what David did on
#9682.  Thanks!)

I can take a closer look later if you'd like, but I think this might
keep things busy for a while.


- Need to consider use of assert; is or is it not safe to build
  torsocks with -DNDEBUG?  There seem to be cases, such as
  tsocks_mutex_unlock, where you're assuming that assert() isn't
  removed.  If you want to make sure -assert() always happens, you
  need to check for whether NDEBUG has been defined, and give an
  #error.  Alternatively, define a tsocks_assert() that can't be
  defined out.

- Your definition of IN_LOOPBACK is only going to work with
  little-endian hosts.  It's only used in one place
  (utils_is_ipv4_local) which is itself only used in one place
  (tsocks_connect).  Why not instead write a general portable check
  for whether a sockaddr is local?

     static int
     tsocks_sockaddr_is_localhost(const struct sockaddr *sa)
     {
             if (sa->sa_family == AF_INET) {
                     const struct sockaddr_in *sin = (const struct
sockaddr_in*)sa;
                     return (ntohl(sin->sin_addr.s_addr) & 0xff000000)
== 0x7f000000;
             } else if (sa->sa_family == AF_INET6) {
                    const struct sockaddr_in *sin6 = (const struct
sockaddr_in6*)sa;
                    static const uint8_t addr[] = {0,0,0,0,0,0,0,0,
                                                   0,0,0,0,0,0,0,1};
                    return ! memcmp(sin6->sin6_addr.s6_addr8, addr, 16);
             } else {
                    return 0; /* Or 1, depending on what you want. */
             }
     }
     /* that's totally untested; caveat haxxor */

- For that matter, checking for 127.0.0.0/8 is not sufficient; you
  should also check for ::1.  Also, you should probably also check
  for connect() attempts to 0.0.0.0 and ::.  I don't recall if there
  are larger parts of their networks you need to check for.

- You're really not supposed to be declaring identifiers that start
  with __ or _ unless you're the operating system.

- I see in connect.c that it handles IPv4 and IPv6 and defers to
  libc for other address families. Is it really the case that other
  address families can't leave the local host?  There are rather a
  lot of AF_* types in my sys/socket.h.

  - Oh. socket.c refuses to make AF_INET6 sockets.  Why?  Tor 0.2.4
    is supposed to be able to handle them just fine.

- In connect.c, it makes me worried that you're unconditionally
  casting the sockaddr to sockaddr_in.  Also, you should really have
  it be a const sockaddr_in.

- Throughout the code, it appears you're faily good about checking
  return values from zmalloc, but only so-so at checking return
  values from strdup.  I see an unchecked strdup() in connect.c;
  there are probably some more.

- It might be a good idea to document somewhere -- possibly when
  each mutex is declared -- how (if at all) the mutexes are allowed
  to nest.  FWICT the current answer is "not at all", but it might
  not be so easy for the next person to figure out.

- I wonder if anything can be done to unify Tor's "virtual address
  mapping" support and torsock's "onion mapping" support.

- I didn't see anything obvious in the socks code to make sure that
  names are 255 characters or less.  If that's handled elsewhere, it
  should at least have a comment in the socks code.

- It seems as though even with nonblocking sockets this code will
  block during connect, after it's sent Tor a connect() and it's
  waiting for a reply.  Is that so?

- I don't see how this works with nonblocking sockets.  It looks
  like send_data and recv_data just busy-loop until the socket is
  readable/writeable?  That's going to make the whole thread block
  until Tor answers the socks reply, if I'm reading this right.  Did
  the old Torsocks do this too?  When I look at the old torsocks, I
  see what appears to be select() and poll() code...

     - Perhaps Tor should have a "answer SOCKS requests right away,
       regardless of whether the stream succeeded" option?  I wonder
       what that would break.

- The socket.c code is going to break Linux sockets with flags like
  SOCK_NONBLOCK and SOCK_CLOEXEC set on them.

- Why is socketpair() intercepted?  What's the danger in creating a
  socketpair()?  That shouldn't use Tor at all; it's all local.

- Shouldn't listen() and/or accept() and accept4() get intercepted?
  I don't see that happening.

- It might be cool to find a networking library with unit tests, and
  make sure that those unit tests run correctly under torsocks -- or
  rather, that they don't fail except when you would expect them to
  fail.

- The "hints" argument to getaddrinfo is allowed to be NULL.

- Are you using tsocks_libc_getaddrinfo() as a fancy way to do IP
  resolution if the IP happens to pass inet_pton?  That seems pretty
  unsafe unless you use the AI_NUMERICHOST flag.

- I seem to recall that there are some low-level resolver interfaces
  in gnu libc that possibly need to be intercepted.

- It looks like getpeername() isn't actually implemented? If you're
  trying to have it return "no address", leaving the input unchanged
  is not going to work.

- I see that you're looking at getenv() stuff.  It's important not
  to look at environment variables when you're running a setuid or
  setgid program.  Some of the getenvs() in torsocks check is_suid,
  but some don't.

- I think tsocks_tor_resolve() and tsocks_tor_resolve_ptr() leak the
  file descriptor if there's an error after opening the socket and
  before the close.

- Are you trying to support Windows or something? If not, using a
  hashtable for file descriptors is overkill.  Just use an array,
  indexed by file descriptor.  (Unix file descriptors are allocated
  starting with low numbers.)  It's only necessary to use a
  hashtable for sockets if you're on Windows....

  ....but if you're on windows, you can't use int for sockets, since
  win64 has 64-bit SOCKETs.  (All IIRC)

- strtok, as used in in utils_tokenize_ignore_comments, isn't
  threadsafe.  Maybe best to avoid it, or at least use strtok_r
  where available?

- utils_tokenize_ignore_comments is being dangerous.  It uses spaces
  to count how many arguments their are, but then it tokenizes based
  on spaces *and* tabs.  So if I say "a b\tc", and size==2, then the
  first loop (which counts the entries) will think it's seeing "a",
  "b\tc", but the second loop will tokenize "a", "b", "c", storing
  the "c" element off the end of the array.

- Can you document your plans for connection_get_ref and
  connection_put_ref?  From what I can tell, nothing ever calls
  connection_get_ref, and connection_put_ref is only called on
  close.  Do you really need it?

- zmalloc's definition should really wrap the 'x' argument in
  parentheses.

- In socks5.c and elsewhere: when working with binary data, it's a
  good idea to use unsigned char for buffers.

- It's a good idea to have a target or something to test your unit
test coverage to see what's tested and what isn't.

peace,
-- 
Nick