[tor-dev] Torsocks 2.0 RC code review

[David Goulet]

On 30 Nov (19:56:11), Nick Mathewson wrote:
> 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 Nick,

Again, sorry for the delayed response! I'm back on the keybord! :)

> 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!)

Please, keep pointing me stuff you would like reviewed, don't hesitate
to send them to me on IRC/email.

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

Sure! What I can do is to reply back to this email (again) with the
commit IDs of the fixed issues if you like.

> 
> - 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.

Hmmm, so the way I used assert() in this code base is to detect code
flow issues meaning that if the assert is triggered on the mutex pointer
in tsocks_mutex_unlock() that tells me that there is a broken code path.

The assert() breaks on code path that are *never* suppose to happen so
it helps me to find bad code flow during development and testing. I
never use assert() on input I don't control internally like user data
for instance. If I did, it's a bad mistake that should be fixed.

With your experience, I'll be glad to reconsider that if it does not
make sense to you or/and some use cases may fail.

> 
> - 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?

Right, the Linux one (linux/in.h) is in little endian where the one I
provide in the compat layer (common/compat.h) is network byte order even
though the comment says host byte order (bad comment).

> 
>      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 */

I agree! Much more useful and supporting both v4 and v6. I would use
however the macros provided by Linux like so (and define them if not
present in the compat layer):

0xff000000 == IN_CLASSA_NET
0x7f000000 == IN_LOOPBACKNET

static const uint8_t addr[] = {0,0,0,0,0,0,0,0, 0,0,0,0,0,0,0,1};
-->
static const uint8_t addr[] = IN6ADDR_LOOPBACK_INIT;

GNU libc provides also stuff like "IN6_IS_ADDR_LOOPBACK(a)" but I'm fine
with the above portable way. For constant values, I'm just not to keen
to hardcode them so I prefer using defines for that because I might
reuse them somewhere else in the code.

> 
> - 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.

Right, the above code will fix that with a patch making connect check
for local address for both inet and inet6.

For the 0.0.0.0/::., since connect() sends back an EINVAL, that makes
sense to avoid a round trip to Tor.

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

You are right, my mistake. Just for completion, here is the reference.

https://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html

I'll make sure to find an alternative for that.

> 
> - 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.

Yes, this is controlled in the socket() call, every socket that Tor is
not able to handle will fail with EINVAL. For INET6, at the time I wrote
this, Tor didn't support v6 so I guess I can change it! :). Is the SOCKS
protocol for v6 DNS resolution has been added as well?

> 
> - 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.

Since at that point (line 72) where the cast occurs, only INET socket
can be handle. This must be fixed when allowing socket() to create v6.

True for the const!

> 
> - 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.

That's indeed a missed one! Good catch :). I found a couple 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.

Absolutely, I usually do that only if there is nesting happening but I
can surely add a comment to indicate that there is none.

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

I haven't seen the code of that feature but I was told by Isis that it
exists and that should be considered for unification. I agree.

> 
> - 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.

If by name you mean DNS name, I use that in common/default.h

/*
 * RFC 1035 specifies a maxium of 255 possibe for domain name.
 * (https://www.ietf.org/rfc/rfc1035.txt).
 */
#define DEFAULT_DOMAIN_NAME_SIZE    255

> 
> - 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?

Yes indeed for now it actually blocks. That's something I want to
improve having a more detailed state on a per connection basis so we do
don't block when connecting to the Tor daemon.

> 
> - 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.

Yup every I/O operation is non blocking *except* the connect() part
where we do actually wait for established a connection to Tor. The old
torsocks code did respect the non blocking socket thus this needs to be
added to this code base where we handle a non blocking socket even when
connecting to the Tor daemon.

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

Sheit true that! Good catch!

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

Local DNS resolution. If you pass a SOCK_STREAM, it just routes you to
the libc call but for instance SOCK_DGRAM, we have to intercept that to
deny UDP 53 on localhost for instance.

Linux only supports AF_UNIX and AF_LOCAL so this shouldn't be a problem.

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

Unless we are doing some accounting, I don't see the need. These are for
inbound connections and it's a bit difficult to force that to go through
Tor.

> 
> - 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.

Absolutely.

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

True!

> 
> - 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 only use that function to know if the passed node is a network address
(v4/v6) or a DNS name. A return code of 0 from inet_pton() indicates an
*invalid* network addr. thus it's passed to Tor for resolution assuming
it might be a DNS name.

Else, if it's a valid network address, it's simply pass to getaddrinfo()
directly of which I understand it should NOT do domain name resolution
on it's own.

Am I mistaken here?

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

Any chance you can point me to their names?

> 
> - 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.

Hrm, it is? src/lib/getpeername.c

> 
> - 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.

Right, half of them don't check the is_suid flag. Will fix that!

> 
> - 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.

True!

> 
> - 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)

So yes that would be nice actually! However, it's so different I'm not
sure how both could use the same code for now.

However, I've checked in the kernel Documentation and there is actually
no mention of any guarantee that Linux will return the lowest fd value.
To be honest, I don't see the issue here of using an hash table since I
don't need to handle the resize of the FD array, just simpler and this
hashtable comes from the Tor code thus I guess pretty tested :).

If you have an issue with ht, I'm all ears and it's easily modifiable
also to use an array.

> 
> - 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?

True!

> 
> - 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.

True!

> 
> - 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?

You are right, it's never used for now. I think I had it just for the
sake of completion (put/get).

The refcount of the connection is set to 1 on creation but I guess it
could use get_ref instead. It's not that problematic though since the
object is not globally visible before an insert() occurs.

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

True!

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

Right, I'll add the unsigned to the socks5 ABI and in the code as well.

> 
> - 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.

Yah, there should be way more unit test and have a coverage plan. I do
plan to do that, just a time factor at this point :).

.---

Huge thanks Nick! This is an amazing code review, immensely appreciated!
I'll make sure to fix everything that can be done now.

Cheers!
David

> 
> peace,
> -- 
> Nick
> _______________________________________________
> tor-dev mailing list
> tor-dev at lists.torproject.org
> https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 603 bytes
Desc: Digital signature
URL: <http://lists.torproject.org/pipermail/tor-dev/attachments/20140119/8b751667/attachment-0001.sig>