On 30 Nov (19:56:11), Nick Mathewson wrote:
On Tue, Nov 26, 2013 at 5:17 PM, David Goulet dgoulet@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/torsocksI 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 structsockaddr_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@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev