Opened 5 years ago

Closed 5 years ago

#12585 closed enhancement (fixed)

Implement new option SocksSocket

Reported by: ioerror Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: 026-triaged-1
Cc: dgoulet, adrelanos@…, andrea, intrigeri, mcs, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Hi,

I've implemented a new way for client applications to speak to Tor. I wanted to lock down applications like web browsers to ensure that they cannot even make AF_INET or AF_INET6 sockets. There is one problem: all those clients need AF_INET to talk to Tor! This patch fixes this issue - if a client is able to make an AF_UNIX socket and it talks to a Tor that supports AF_UNIX, it will be able to use SOCKS to connect to the internet.

I plan to write a patch to torsocks to implement this as a generic client. Later, I suspect we can add support to other applications very easily and then we can lock down those applications or even entire unix uids from being able to make AF_INET/AF_INET6 sockets.

This helps us with AppArmor like issues - AppArmor doesn't have the ability to permit traffic to 127.0.0.1:9050 and to deny it for other addresses. With this implementation, we can simply deny all AF_INET and the application can still communicate with Tor as long as it has AF_UNIX permissions.

This also helps us with iptables issues - there are no generally open TCP/IP sockets for anyone who is able to connect to (for example) 127.0.0.1:9050 - we can control who can read and write to the SocksSocket with unix uid/gid controls.

I've spent about two days testing (on Tails 1.0.1) these patches and loading it with the following configuration file:

Socks5Proxy 127.0.0.1:9050
WarnUnsafeSocks 0
SocksPort 0
Log debug stderr
SocksSocket /tmp/testing/SocksSocket
SocksSocket /tmp/testing/SocksSocket1
SocksSocket /tmp/testing/SocksSocket2
SocksSocket /tmp/testing/SocksSocket3
AvoidDiskWrites 1

I've been running it in valgrind like so:

valgrind --log-file=/tmp/SocksSocket-valgrind-005-with-three-SocksSockets.log -v --leak-check=full --track-origins=yes ./src/or/tor -f torrc.test 

As I haven't yet implemented the torsocks client side of this, I've been using socat like so:

socat -v UNIX-CONNECT:/tmp/testing/SocksSocket TCP-LISTEN:6667,fork,RETRY,reuseaddr,end-close;

Finally, I use curl like so to fetch a web page through this totally convoluted mess of AF_*:

curl --socks5-hostname 127.0.0.1:6667 https://check.torproject.org;

Valgrind reports the following:

==15187== Memcheck, a memory error detector
==15187== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al.
==15187== Using Valgrind-3.6.0.SVN-Debian and LibVEX; rerun with -h for copyright info
==15187== Command: ./src/or/tor -f torrc.test
==15187== Parent PID: 29356
==15187== 
--15187-- 
--15187-- Valgrind options:
--15187--    --suppressions=/usr/lib/valgrind/debian-libc6-dbg.supp
--15187--    --log-file=/tmp/SocksSocket-valgrind-005-with-three-SocksSockets.log
--15187--    -v
--15187--    --leak-check=full
--15187--    --track-origins=yes
--15187-- Contents of /proc/version:
--15187--   Linux version 3.14-1-amd64 (debian-kernel@lists.debian.org) (gcc version 4.8.3 (Debian 4.8.3-2) ) #1 SMP Debian 3.14.5-1 (2014-06-05)
--15187-- Arch and hwcaps: X86, x86-sse1-sse2
--15187-- Page sizes: currently 4096, max supported 4096
--15187-- Valgrind library directory: /usr/lib/valgrind
--15187-- Reading syms from /home/amnesia/Persistent/src/tor/src/or/tor (0x108000)
--15187-- Reading syms from /lib/ld-2.11.3.so (0x4400000)
--15187--   Considering /lib/ld-2.11.3.so ..
--15187--   .. CRC mismatch (computed 19231304 wanted 2b6c260a)
--15187--   Considering /usr/lib/debug/lib/ld-2.11.3.so ..
--15187--   .. CRC is valid
--15187-- Reading syms from /usr/lib/valgrind/memcheck-x86-linux (0x38000000)
--15187--    object doesn't have a dynamic symbol table
--15187-- Reading suppressions file: /usr/lib/valgrind/debian-libc6-dbg.supp
--15187-- Reading suppressions file: /usr/lib/valgrind/default.supp
--15187-- REDIR: 0x4416490 (index) redirected to 0x3803eda3 (vgPlain_x86_linux_REDIR_FOR_index)
--15187-- Reading syms from /usr/lib/valgrind/vgpreload_core-x86-linux.so (0xabcb000)
--15187-- Reading syms from /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so (0xabd1000)
==15187== WARNING: new redirection conflicts with existing -- ignoring it
--15187--     new: 0x04416490 (index               ) R-> 0x0abd4cb0 index
--15187-- REDIR: 0x4416670 (strlen) redirected to 0xabd50f0 (strlen)
--15187-- Reading syms from /usr/lib/libz.so.1.2.3.4 (0xccc3000)
--15187--   Considering /usr/lib/libz.so.1.2.3.4 ..
--15187--   .. CRC mismatch (computed 7be92cfa wanted 329326cb)
--15187--    object doesn't have a symbol table
--15187-- Reading syms from /lib/libm-2.11.3.so (0xccdf000)
--15187--   Considering /lib/libm-2.11.3.so ..
--15187--   .. CRC mismatch (computed 0116a1b2 wanted cca4fc2f)
--15187--   Considering /usr/lib/debug/lib/libm-2.11.3.so ..
--15187--   .. CRC is valid
--15187-- Reading syms from /usr/lib/libevent-1.4.so.2.1.3 (0xcd09000)
--15187--    object doesn't have a symbol table
--15187-- Reading syms from /usr/lib/i686/cmov/libssl.so.0.9.8 (0xcd20000)
--15187--   Considering /usr/lib/i686/cmov/libssl.so.0.9.8 ..
--15187--   .. CRC mismatch (computed 7cd446f3 wanted 6aaecd6b)
--15187--    object doesn't have a symbol table
--15187-- Reading syms from /usr/lib/i686/cmov/libcrypto.so.0.9.8 (0xcd70000)
--15187--   Considering /usr/lib/i686/cmov/libcrypto.so.0.9.8 ..
--15187--   .. CRC mismatch (computed a803f391 wanted 934b1db6)
--15187--    object doesn't have a symbol table
--15187-- Reading syms from /lib/librt-2.11.3.so (0xcecd000)
--15187--   Considering /lib/librt-2.11.3.so ..
--15187--   .. CRC mismatch (computed 11db8d18 wanted 4837ea6c)
--15187--   Considering /usr/lib/debug/lib/librt-2.11.3.so ..
--15187--   .. CRC is valid
--15187-- Reading syms from /lib/libdl-2.11.3.so (0xceda000)
--15187--   Considering /lib/libdl-2.11.3.so ..
--15187--   .. CRC mismatch (computed 3740dd8b wanted 09c06eb3)
--15187--   Considering /usr/lib/debug/lib/libdl-2.11.3.so ..
--15187--   .. CRC is valid
--15187-- Reading syms from /lib/libc-2.11.3.so (0xcede000)
--15187--   Considering /lib/libc-2.11.3.so ..
--15187--   .. CRC mismatch (computed 4ef5e22d wanted 481f3942)
--15187--   Considering /usr/lib/debug/lib/libc-2.11.3.so ..
--15187--   .. CRC is valid
--15187-- Reading syms from /lib/libpthread-2.11.3.so (0xd027000)
--15187--   Considering /lib/libpthread-2.11.3.so ..
--15187--   .. CRC mismatch (computed d08a9725 wanted 0065618d)
--15187--   Considering /usr/lib/debug/lib/libpthread-2.11.3.so ..
--15187--   .. CRC is valid
--15187-- Reading syms from /lib/libnsl-2.11.3.so (0xd040000)
--15187--   Considering /lib/libnsl-2.11.3.so ..
--15187--   .. CRC mismatch (computed 65a29afd wanted f8853f76)
--15187--   Considering /usr/lib/debug/lib/libnsl-2.11.3.so ..
--15187--   .. CRC is valid
--15187-- Reading syms from /lib/libresolv-2.11.3.so (0xd05b000)
--15187--   Considering /lib/libresolv-2.11.3.so ..
--15187--   .. CRC mismatch (computed 66a703f9 wanted 6378a0ac)
--15187--   Considering /usr/lib/debug/lib/libresolv-2.11.3.so ..
--15187--   .. CRC is valid
--15187-- REDIR: 0xcf50950 (index) redirected to 0xabd4c20 (index)
--15187-- REDIR: 0xcf52750 (memchr) redirected to 0xabd5830 (memchr)
--15187-- REDIR: 0xcf513f0 (rindex) redirected to 0xabd4b60 (rindex)
--15187-- REDIR: 0xcf51040 (strlen) redirected to 0xabd50b0 (strlen)
--15187-- REDIR: 0xcf4d7c0 (malloc) redirected to 0xabd3ecb (malloc)
--15187-- REDIR: 0xcf52ed0 (memcpy) redirected to 0xabd5870 (memcpy)
--15187-- REDIR: 0xcf55830 (strchrnul) redirected to 0xabd6590 (strchrnul)
--15187-- REDIR: 0xcf4d6e0 (free) redirected to 0xabd3ae5 (free)
--15187-- REDIR: 0xcf52a20 (mempcpy) redirected to 0xabd6600 (mempcpy)
--15187-- REDIR: 0xcf4ced0 (calloc) redirected to 0xabd31af (calloc)
--15187-- Reading syms from /lib/libgcc_s.so.1 (0xd483000)
--15187--   Considering /lib/libgcc_s.so.1 ..
--15187--   .. CRC mismatch (computed 5efc9915 wanted ece5a7a0)
--15187--    object doesn't have a symbol table
--15187-- REDIR: 0xcf4e760 (realloc) redirected to 0xabd3f7a (realloc)
--15187-- REDIR: 0xcf51230 (strncmp) redirected to 0xabd55d0 (strncmp)
--15187-- REDIR: 0xcf52bd0 (stpcpy) redirected to 0xabd6120 (stpcpy)
--15187-- REDIR: 0xcf51310 (strncpy) redirected to 0xabd52f0 (strncpy)
--15187-- REDIR: 0xcf50ac0 (strcmp) redirected to 0xabd56b0 (strcmp)
--15187-- REDIR: 0xcf529c0 (memset) redirected to 0xabd64a0 (memset)
--15187-- REDIR: 0xcf50b40 (strcpy) redirected to 0xabd5130 (strcpy)
--15187-- REDIR: 0xcf55760 (rawmemchr) redirected to 0xabd65c0 (rawmemchr)
--15187-- REDIR: 0xcf52910 (memmove) redirected to 0xabd6510 (memmove)
--15187-- REDIR: 0xcfbc620 (__memcpy_chk) redirected to 0xabd69b0 (__memcpy_chk)
==15187== Conditional jump or move depends on uninitialised value(s)
==15187==    at 0x1E8C04: connection_ap_expire_beginning (connection_edge.c:600)
==15187==    by 0x13669D: second_elapsed_callback (main.c:1501)
==15187==    by 0x25E572: periodic_timer_cb (compat_libevent.c:538)
==15187==    by 0xCD0EEE3: event_base_loop (in /usr/lib/libevent-1.4.so.2.1.3)
==15187==    by 0x1318E0: do_main_loop (main.c:2028)
==15187==    by 0x133BDC: tor_main (main.c:2998)
==15187==    by 0x12F7D2: main (tor_main.c:30)
==15187==  Uninitialised value was created by a stack allocation
==15187==    at 0x1DE763: connection_handle_listener_read (connection.c:1454)
==15187== 
--15187-- Discarding syms at 0xd485350-0xd49d738 in /lib/libgcc_s.so.1 due to munmap()
==15187== 
==15187== HEAP SUMMARY:
==15187==     in use at exit: 3,565 bytes in 29 blocks
==15187==   total heap usage: 353,781 allocs, 353,752 frees, 85,358,749 bytes allocated
==15187== 
==15187== Searching for pointers to 29 not-freed blocks
==15187== Checked 276,744 bytes
==15187== 
==15187== LEAK SUMMARY:
==15187==    definitely lost: 0 bytes in 0 blocks
==15187==    indirectly lost: 0 bytes in 0 blocks
==15187==      possibly lost: 0 bytes in 0 blocks
==15187==    still reachable: 3,565 bytes in 29 blocks
==15187==         suppressed: 0 bytes in 0 blocks
==15187== Reachable blocks (those to which a pointer was found) are not shown.
==15187== To see them, rerun with: --leak-check=full --show-reachable=yes
==15187== 
==15187== ERROR SUMMARY: 660 errors from 1 contexts (suppressed: 37 from 12)
==15187== 
==15187== 660 errors in context 1 of 1:
==15187== Conditional jump or move depends on uninitialised value(s)
==15187==    at 0x1E8C04: connection_ap_expire_beginning (connection_edge.c:600)
==15187==    by 0x13669D: second_elapsed_callback (main.c:1501)
==15187==    by 0x25E572: periodic_timer_cb (compat_libevent.c:538)
==15187==    by 0xCD0EEE3: event_base_loop (in /usr/lib/libevent-1.4.so.2.1.3)
==15187==    by 0x1318E0: do_main_loop (main.c:2028)
==15187==    by 0x133BDC: tor_main (main.c:2998)
==15187==    by 0x12F7D2: main (tor_main.c:30)
==15187==  Uninitialised value was created by a stack allocation
==15187==    at 0x1DE763: connection_handle_listener_read (connection.c:1454)
==15187== 
--15187-- 
--15187-- used_suppression:     37 dl-hack3-cond-1
==15187== 
==15187== ERROR SUMMARY: 660 errors from 1 contexts (suppressed: 37 from 12)

I think that other than that single conditional jump in connection_ap_expire_beginning, there aren't any serious valgrind issues that are related to my patch. Though I admit, I'm not entirely sure of why that valgrind issue is showing up and I'm starting to dig into it now.

I've based my patch on 48d7fceee5e6041ccdd4316f51de0d6b5e1818ed; I'm happy to rebase it if that is useful.

Feedback is appreciated!

Child Tickets

Attachments (5)

SocksSocket-007.patch (21.0 KB) - added by ioerror 5 years ago.
first public patch
SocksSocket-008.patch (21.1 KB) - added by ioerror 5 years ago.
fix valgrind warnings
socks-socket-http-client.py (1.1 KB) - added by cypherpunks 5 years ago.
http client which can speak socks over a unix socket (or any other twisted endpoint)
SocksSocket-valgrind-008-with-three-SocksSockets.log (23.0 KB) - added by ioerror 5 years ago.
SocksSocket-009.patch (21.0 KB) - added by ioerror 5 years ago.

Download all attachments as: .zip

Change History (59)

comment:1 Changed 5 years ago by ioerror

( I haven't added unit tests to this patch but I'm willing to implement them if anyone has suggestions for what they'd like to see tested. )

comment:2 Changed 5 years ago by dgoulet

Cc: dgoulet added

comment:3 Changed 5 years ago by dgoulet

The "is_socks_socket" is added to an entry_connection_t object which is then used in src/or/relay.c +1325 to bypass the check to the IPv4/IPV6 traffic OK variable.

However, it seems that SocksSocket is parsed as a SocksPort option which means that the entry connection created is set with the possible flag that can be passed to the SocksPort option like "IPv6Traffic" or "CacheDNS", etc...

I see that the man page entry you've added does not mention "flags" or "isolation flags" like SocksPort does but they are still set with the default value. Is this the intended behavior to have one single possible way to configure that Unix port and don't offer to the user the possibility to deny IPv6 traffic through it for instance like a normal SocksPort?

Changed 5 years ago by ioerror

Attachment: SocksSocket-007.patch added

first public patch

comment:4 in reply to:  3 Changed 5 years ago by ioerror

Replying to dgoulet:

The "is_socks_socket" is added to an entry_connection_t object which is then used in src/or/relay.c +1325 to bypass the check to the IPv4/IPV6 traffic OK variable.

I think this is fine - though perhaps I'm mistaken - is there a preference? Should I signal that one or the other is preferred?

However, it seems that SocksSocket is parsed as a SocksPort option which means that the entry connection created is set with the possible flag that can be passed to the SocksPort option like "IPv6Traffic" or "CacheDNS", etc...

True.

I see that the man page entry you've added does not mention "flags" or "isolation flags" like SocksPort does but they are still set with the default value. Is this the intended behavior to have one single possible way to configure that Unix port and don't offer to the user the possibility to deny IPv6 traffic through it for instance like a normal SocksPort?

Yes. That seems like a feature to add later - I just wanted a basic thing landed before I try to make it fancy. This is similar to how SOCKSPort was developed. :)

comment:5 Changed 5 years ago by rl1987

I have applied the proposed patch to current Tor master (48d7fceee5e6041ccdd4316f51de0d6b5e1818ed) and found that it works on Mac OS X 10.10 with following modifications to above torrc:

  1. SocksSocket lines pointing to data directory of my TBB.
  2. First line deleted.

check.tpo was successfully retrieved with above socat and curl snippets.

comment:6 Changed 5 years ago by ioerror

I think I've fixed the valgrind issue - I'm going to let this run for a few hours and then upload a minor fix to the patch, if it does indeed fix it.

Changed 5 years ago by ioerror

Attachment: SocksSocket-008.patch added

fix valgrind warnings

comment:7 Changed 5 years ago by ioerror

I've uploaded SocksSocket-008.patch to fix the valgrind issues. It looks clean now.

comment:8 Changed 5 years ago by ioerror

I've been grinding away and loading my Tor client for ~18 hours and the valgrind issues are gone. Tor is happy to serve data over several SocksSockets at the same time.

Changed 5 years ago by cypherpunks

Attachment: socks-socket-http-client.py added

http client which can speak socks over a unix socket (or any other twisted endpoint)

comment:9 Changed 5 years ago by proper

Cc: adrelanos@… added

comment:10 Changed 5 years ago by ioerror

I've been loading the network with this patch for around two days and I've had no issues. I've also had zero valgrind issues related to my patch. I've sent two other unrelated zlib related valgrind issues to nickm.

Has anyone used that python http client? I don't have twisted installed on my development machine, so I haven't yet tested it. I'm curious what other clients will work well out of the box with this AF_UNIX socket patch...

comment:11 Changed 5 years ago by ioerror

I've concluded my load testing and attached the valgrind output for ~72+ hours of testing:

https://trac.torproject.org/projects/tor/attachment/ticket/12585/SocksSocket-valgrind-008-with-three-SocksSockets.log

comment:12 Changed 5 years ago by anon

I have reviewed the patch and it looks good! There is a SocksSocketsGroupWriteable option to support use by users who are not the tor user.

The patch correctly handles non-applicable scenarios where IPV6 and other uses apply but not to AF_UNIX.

Green for merge on my account!

P.S. I am testing some performance behavior with larger send and receive buffers for Unix domain sockets which may have a significant impact on throughput. If the benefit is large I will submit as another patch. The nature of buffer SOCKOPTs implies more time for testing on platforms for compatibility. (E.g. need additional configure / ifdefs for particular BSD socket options.)

comment:13 Changed 5 years ago by andrea

Initial code review of ioerror's SOCKS-over-AF_UNIX patch:

  • In tor_addr_from_sockaddr(), you're calling a helper function tor_addr_make_af_unix(), and then returning -1. The existing convention for that function (which is not documented in its comment, but should be), is to return -1 in the error case of an unknown address family, and 0 in the success case. The line after tor_addr_make_af_unix(a); should be return 0, I think.
  • The tor_addr_make_af_unix() helper itself looks okay to me, but this function is only called in one place, from within address.c. It should be declared static at the top of address.c rather than in the global namespace in address.h.
  • The change to tor_addr_to_str() looks okay to me.
  • All the config.c changes look okay to me.
  • Changes to entry_connection_new() and connection_free_() look okay.
  • As far as I can tell, the new check_location_for_socks_unix_socket() function differs from the existing check_location_for_unix_socket() only in the text of the warning message emitted. These should be refactored to avoid duplicated logic.
  • In connection_listener_new():
    • Perhaps is_tcp should be renamed, since obviously an AF_UNIX socket is not TCP. Is the relevant property actually SOCK_STREAM (vs. SOCK_DGRAM in the CONN_TYPE_AP_DNS_LISTENER case)?
  • Why are you adding log_warn(LD_NET,"accept() fails after this...") when no actual error condition appears to be occurring in the AF_INET/ AF_INET6 path?
  • The logic in the "if (listensockaddr->sa_family == AF_UNIX && type != CONN_TYPE_CONTROL_LISTENER)" branch is very similar to the existing logic for the CONN_TYPE_CONTROL_LISTENER case. They should be combined to avoid duplicated code.
  • Changes check_sock_addr() and connection_handle_listener_read() look okay.
  • All changes to main.c, or.h and relay.c look okay to me.
  • Changes to connection_edge.c look okay.

I will now proceed to attempt to apply the patch against current master and test.

comment:14 Changed 5 years ago by andrea

This patch causes a unit test failure on test_addr.c line 468; the test suite depended on the old behavior of tor_addr_to_str() returning NULL for AF_UNSPEC rather than the string "AF_UNSPEC". I'm not sure this causes problems anywhere else, but recommend changing tor_addr_to_str() back to the original behavior for the AF_UNSPEC case.

comment:15 Changed 5 years ago by andrea

This does indeed produce a spurious "accept() fails after this" warning:

Jul 15 08:02:40.072 [notice] Tor v0.2.6.0-alpha-dev (git-48d7fceee5e6041c) running on Linux with Libevent 2.0.21-stable, OpenSSL 1.0.1h and Zlib 1.2.7.
Jul 15 08:02:40.072 [notice] Tor can't help you if you use it wrong! Learn how to be safe at https://www.torproject.org/download/download#warning
Jul 15 08:02:40.072 [notice] This version is not a stable Tor release. Expect more bugs than usual.
Jul 15 08:02:40.072 [notice] Read configuration file "/sechome/andrea/tor/test/bug12585/torrc.bug12585-client".
Jul 15 08:02:40.079 [notice] Opening Socks listener on 127.0.0.1:9052
Jul 15 08:02:40.079 [warn] accept() fails after this...
Jul 15 08:02:40.079 [notice] Opening SocksSocket Socks listener on /sechome/andrea/tor/test/bug12585/socks/socket

comment:16 Changed 5 years ago by andrea

Existing SOCKS-over-TCP client functionality seems unimpaired with this patch.

comment:17 Changed 5 years ago by andrea

SOCKS over AF_UNIX client functionality seems to work. Test procedure was "socat -d -d -d TCP4-LISTEN:9053,bind=localhost,fork UNIX-CONNECT:$socket_name" and curl -x socks5h://127.0.0.1:9053/.

comment:18 Changed 5 years ago by andrea

Status: newneeds_revision

Conclusion: this patch works and I think it's a good idea, but it's in need of some code cleanup and minor bugfixes. I'm setting this to needs_revision.

comment:19 Changed 5 years ago by andrea

Also, given the obvious defense-in-depth value of being able to drop AF_INET/AF_INET6 privileges in Tor Browser, I think once this is cleaned up it should go in 0.2.6, so I'm setting milestones and tagging this as triaged in.

comment:20 Changed 5 years ago by andrea

Cc: andrea added
Keywords: 026-triaged-1 added
Milestone: Tor: 0.2.6.x-final
Version: Tor: unspecified

comment:21 in reply to:  13 Changed 5 years ago by ioerror

Replying to andrea:

Initial code review of ioerror's SOCKS-over-AF_UNIX patch:

Thank you for the review! I really appreciate it!

  • In tor_addr_from_sockaddr(), you're calling a helper function tor_addr_make_af_unix(), and then returning -1. The existing convention for that function (which is not documented in its comment, but should be), is to return -1 in the error case of an unknown address family, and 0 in the success case. The line after tor_addr_make_af_unix(a); should be return 0, I think.

Sure, I made it return 0;

  • The tor_addr_make_af_unix() helper itself looks okay to me, but this function is only called in one place, from within address.c. It should be declared static at the top of address.c rather than in the global namespace in address.h.


I changed void tor_addr_make_af_unix() to static tor_addr_make_af_unix() and removed the reference to it in address.h.

  • The change to tor_addr_to_str() looks okay to me.

OK.

  • All the config.c changes look okay to me.

OK.

  • Changes to entry_connection_new() and connection_free_() look okay.

OK.

  • As far as I can tell, the new check_location_for_socks_unix_socket() function differs from the existing check_location_for_unix_socket() only in the text of the warning message emitted. These should be refactored to avoid duplicated logic.

I figured that we will want to change the logic internally as well later - for example - this is where we might check the user/group restrictions. That was part of the grounding for it in later revisions - does it make sense to keep it around with this in mind?

  • In connection_listener_new():
    • Perhaps is_tcp should be renamed, since obviously an AF_UNIX socket is not TCP. Is the relevant property actually SOCK_STREAM (vs. SOCK_DGRAM in the CONN_TYPE_AP_DNS_LISTENER case)?

I think it is a TCP stream that we attach but I see that it isn't really TCP, of course.

  • Why are you adding log_warn(LD_NET,"accept() fails after this...") when no actual error condition appears to be occurring in the AF_INET/ AF_INET6 path?

I have removed that - it was a mistake to include it. Good catch!

  • The logic in the "if (listensockaddr->sa_family == AF_UNIX && type != CONN_TYPE_CONTROL_LISTENER)" branch is very similar to the existing logic for the CONN_TYPE_CONTROL_LISTENER case. They should be combined to avoid duplicated code.

I didn't combine them because I expect that this branch of code may change later and I wanted it to be treated totally separate.

  • Changes check_sock_addr() and connection_handle_listener_read() look okay.

OK.

  • All changes to main.c, or.h and relay.c look okay to me.

OK.

  • Changes to connection_edge.c look okay.

OK.

I will now proceed to attempt to apply the patch against current master and test.

OK.

comment:22 in reply to:  14 Changed 5 years ago by ioerror

Replying to andrea:

This patch causes a unit test failure on test_addr.c line 468; the test suite depended on the old behavior of tor_addr_to_str() returning NULL for AF_UNSPEC rather than the string "AF_UNSPEC". I'm not sure this causes problems anywhere else, but recommend changing tor_addr_to_str() back to the original behavior for the AF_UNSPEC case.

Done.

Changed 5 years ago by ioerror

Attachment: SocksSocket-009.patch added

comment:23 Changed 5 years ago by ioerror

I've updated the patch and uploaded SocksSocket-009.patch. It doesn't incorporate all the changes but if my reasoning is unconvincing, let me know and I'll update the patch again.

comment:24 Changed 5 years ago by ioerror

Status: needs_revisionneeds_review

comment:25 Changed 5 years ago by ioerror

Generally, regardless of the patch revision details...

How about backporting this to other versions of Tor - for example - to get it into Tor Browser? Would that be needed? Would that be crazy talk?

comment:26 Changed 5 years ago by intrigeri

Cc: intrigeri added

comment:27 Changed 5 years ago by cypherpunks

I like this patch; note that anyone in the group can delete the UNIX domain socket and thus prevent other users in the group from accessing the service. You could fix that by forcing the domain socket path to be in a directory which has 750 permissions. Then, group members cannot unlink.

Last edited 5 years ago by cypherpunks (previous) (diff)

comment:28 Changed 5 years ago by yawning

Status: needs_reviewneeds_revision

(I looked at SocksSocket-009.patch.)

Some quick thoughts after a casual review:

  • This patch will break the build in environments that do not define AF_UNIX. I'm not sure we actually care or build in such places (src/or/connections.c and src/or/cpuworker.c also will break the build), but a good fraction of the code has explicit guards.
  • In or/config.c:parse_ports() the parse_unix_socket_config() call should probably be next to where the normal SocksPort/SocksListenAddress is handled, instead of in a block where the control port is handled. (Minor)
  • In or/connection.c:
    • check_location_for_socks_unix_socket() is duplicating check_location_for_unix_socket() just to look at a different option and print "SocksSocket" instead of "control socket". This should probably be one routine, that takes the group writable option as an argument and an error message that works for both cases (or another argument). (Edit: Whoops, just saw the response)
    • check_location_for_socks_unix_socket() inherited not using path_is_relative() from check_location_for_unix_socket(). Both cases should probably be fixed for future portability.
    • connection_listener_new() has left overs from what looks like an attempt to handle the control port and SocksSocket with the same code. "if ( type == CONN_TYPE_CONTROL_LISTENER ) {") is harmless but should be removed. "if (options->SocksSocketsGroupWritable) {" is a bug (the control port socket will be made group writable based off SocksSocketGroupWritable").
    • connection_handle_listener_read() leaks memory, a tor_dup_addr() call needs to be moved into a branch.
          newconn->address = tor_dup_addr(&addr); /* malloc() here */
          /* <snip> */
          if (new_type == CONN_TYPE_AP && conn->socket_family == AF_UNIX) {
            newconn->port = 0;
            newconn->address = tor_strdup(conn->address); /* leak here */
      
  • Run make check-spaces and fix the whitespace issues.

It looks mostly ok, but there's a few minor bug fixes required (and whitespace cleanup), setting to needs_revision.

Last edited 5 years ago by yawning (previous) (diff)

comment:29 Changed 5 years ago by andrea

Testing the SocksSocket-009 revision:

  • This required extensive formatting fixes (see fixup in my ticket12585_v2 branch); please run make check-spaces and remember to keep lines under 80 characters and not use hard tabs next time.
  • The patch appears to work correctly in testing.

Will proceed with detailed code review now.

comment:30 Changed 5 years ago by andrea

Code review of second draft (SocksSocket-009.patch) of ioerror's
SOCKS-over-AF_UNIX patch:

  • The tor_addr_from_sock_addr() and tor_addr_make_af_unix() functions look okay now.
  • The change to tor_addr_to_str() looks okay to me.
  • All the config.c changes look okay to me.
  • Changes to entry_connection_new() and connection_free_() look okay.
  • I still don't like this duplicated code in check_location_for_socks_unix_socket() vs. check_location_for_unix_socket(); they should be combined and if we ever want the behavior to substantively differ we can split them then.
  • Since I don't see any changes to the connection_listener_new() part of the patch, all my previous comments, reproduced below, apply. In particular, the near-identical code blocks have to go.
    • Perhaps is_tcp should be renamed, since obviously an AF_UNIX socket is not TCP. Is the relevant property actually SOCK_STREAM (vs. SOCK_DGRAM in the CONN_TYPE_AP_DNS_LISTENER case)?
    • Why are you adding log_warn(LD_NET,"accept() fails after this...") when no actual error condition appears to be occurring in the AF_INET/AF_INET6 path?
    • The logic in the "if (listensockaddr->sa_family == AF_UNIX && type != CONN_TYPE_CONTROL_LISTENER)" branch is very similar to the existing logic for the CONN_TYPE_CONTROL_LISTENER case. They should be combined to avoid duplicated code.
  • Changes check_sock_addr() and connection_handle_listener_read() look okay.
  • All changes to main.c, or.h and relay.c look okay to me.
  • Changes to connection_edge.c look okay.

comment:31 Changed 5 years ago by nickm

Keywords: nickm-patch added

Add nickm-patch keyword to needs_revision tickets in 0.2.6 where (I think) I wrote the patch, so I know which ones are my responsibility to revise.

comment:32 Changed 5 years ago by nickm

Keywords: nickm-patch removed

comment:33 Changed 5 years ago by ioerror

I'm still working on this patch - I'd like Mike Perry to comment and tell me that he'd use this on platforms where we support it (OS X, GNU/Linux, *BSD). Pretty please Mike? This will help us sandbox the browser...

comment:34 in reply to:  33 Changed 5 years ago by mikeperry

Replying to ioerror:

I'm still working on this patch - I'd like Mike Perry to comment and tell me that he'd use this on platforms where we support it (OS X, GNU/Linux, *BSD). Pretty please Mike? This will help us sandbox the browser...

FWIW, I am on board for trying this in an alpha in Tor Browser if we can find a straight-forward deployment strategy. I think I would prefer to avoid an LD_PRELOAD approach long-term, but it can be used in the alpha series as a prototype, or perhaps even in a stable as an option that automatically gets turned on if you run a script to install an included apparmor profile that enforces it.

comment:35 Changed 5 years ago by mcs

Cc: mcs added

comment:36 Changed 5 years ago by gk

Cc: gk added

comment:37 Changed 5 years ago by andrea

Rebased against latest master, problems fixed and redundant code eliminated in my ticket12585_v3 branch. The previous version had a bug involving chmod()ing both control socket and SOCKS socket if either SocksSocketsGroupWritable or ControlSocketsGroupWritable were set, which I also fixed.

comment:38 Changed 5 years ago by andrea

Status: needs_revisionneeds_review

comment:39 in reply to:  37 Changed 5 years ago by ioerror

Replying to andrea:

Rebased against latest master, problems fixed and redundant code eliminated in my ticket12585_v3 branch. The previous version had a bug involving chmod()ing both control socket and SOCKS socket if either SocksSocketsGroupWritable or ControlSocketsGroupWritable were set, which I also fixed.

Do you want me to review your current version for a full circle?

comment:40 Changed 5 years ago by nickm

The more reviews, the better!

(And thanks for the patch and thanks for the revisions.)

comment:41 Changed 5 years ago by ioerror

OK, I've given it a first pass and my thought is: hooray - Andrea you're a really great programmer and thank you for the revision!

I totally agree with renaming is_tcp to is_stream ( https://gitweb.torproject.org/user/andrea/tor.git/commit/?h=ticket12585_v3&id=48633c07660216d3b852b609c44fa318d55908f0 ) - seems very reasonable. The refactor ( https://gitweb.torproject.org/user/andrea/tor.git/commit/?h=ticket12585_v3&id=2ca1c386b0d1c396fa8d8f4b5334349e24a2f9e8 ) for connection_listener_new() looks reasonable and I'd like a second set of eyes to confirm it. The extra guard on the chmod is a nice touch, thanks for fixing it. The commit ( https://gitweb.torproject.org/user/andrea/tor.git/commit/?h=ticket12585_v3&id=a3bcde3638d035303a9a5bf8373c1b841a1f5636 ) to switch logging to info from notice seems reasonable.

So - I'd consider this a sign-off and a request for nickm or others to also sign off.

comment:42 Changed 5 years ago by ioerror

An open question for me:

Do we want to enable SocksSocket by default?

I'd like to do this - it will encourage people to use it and then in the future torsocks can use SocksSocket as a default with _zero_ configuration.

comment:43 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

Throughout:

  • Looks nice! Much simpler now.
  • This is probably gonna break on windows: I don't think they have AF_UNIX, and at least address.c uses AF_UNIX unconditionally. I can clean it up if you want, or you can if you've got mingw cross-compilation stuff installed.
  • Is "SocksSocket" really the right name for this? I'm used to it, but I bet it would confuse users some.

address.c

  • Probably should document how to work with unix domain sockets; they don't actually fit in a tor_addr_t.

config.c / tor.1.txt

  • Can we support SocksSocket on-by-default? If so we'd need a way to turn it off. "SocksSocket 0"?

connection.c:

  • I wonder if we should try fchmod first, and then chmod. fchmod is a bit safer, right? (Not a new issue.)
  • If we chmod 0660 in the event of a group-writable socket, should we shmod 0600 in the event of a non-group-writable socket? (Not a new issue.)
  • The foosocketsgroupwriteable code in connection_listener_new seems to be nearly duplicated.
  • This comment is wrong:
        /* For now only control ports can be Unix domain sockets
          * and listeners at the same time */
    

connection_edge.c:

  • What's the point of the modifications of connection_ap_get_begincell_flags and connection_ap_handshake_rewrite_and_attach ? I think they might be wrong. The flags that they adjust are about whether the destination address is IPv4/IPv6/etc, not whether the socksport address is IPv4 or IPv6.

relay.c:

  • Same comment as comment above, wrt the change in connection_edge_process_relay_cell_not_open().

Otherwise, looks okay. Have we tested the latest version of this? :)

comment:44 in reply to:  43 ; Changed 5 years ago by ioerror

Replying to nickm:

Throughout:

  • Looks nice! Much simpler now.

Agreed - a lot of refactoring that makes it easier to read!

  • This is probably gonna break on windows: I don't think they have AF_UNIX, and at least address.c uses AF_UNIX unconditionally. I can clean it up if you want, or you can if you've got mingw cross-compilation stuff installed.

I think it would be great if anyone with Windows could make this work. It would remove lots of local firewall issues, I think. I think the proper way to implement it would be to use a named pipe ( http://msdn.microsoft.com/en-us/library/windows/desktop/aa365590%28v=vs.85%29.aspx ) and it would effectively be the same feature.

  • Is "SocksSocket" really the right name for this? I'm used to it, but I bet it would confuse users some.

I'm partial to the name but I'm open to bikesheds. :-) Other thoughts on names:

SocksPipe
SocksSocket
SocksUnixSocket
PipeProxy
PipeSocksTransport
SocksFile
SocksSandbox

Any other ideas welcome :)

address.c

  • Probably should document how to work with unix domain sockets; they don't actually fit in a tor_addr_t.

config.c / tor.1.txt

  • Can we support SocksSocket on-by-default? If so we'd need a way to turn it off. "SocksSocket 0"?

I think if we can enable SocksSocket by default, we'd want a way to turn it off, yes. Perhaps a totally new option? Perhaps we want to have a different ticket for enabling it by default where we add a SocksSocket kill switch?

connection.c:

  • I wonder if we should try fchmod first, and then chmod. fchmod is a bit safer, right? (Not a new issue.)

I'm not familiar with this - Andrea, any thoughts?

  • If we chmod 0660 in the event of a group-writable socket, should we shmod 0600 in the event of a non-group-writable socket? (Not a new issue.)

Same as above.

  • The foosocketsgroupwriteable code in connection_listener_new seems to be nearly duplicated.

Yes, I think that is true

  • This comment is wrong:
        /* For now only control ports can be Unix domain sockets
          * and listeners at the same time */
    

connection_edge.c:

  • What's the point of the modifications of connection_ap_get_begincell_flags and connection_ap_handshake_rewrite_and_attach ? I think they might be wrong. The flags that they adjust are about whether the destination address is IPv4/IPv6/etc, not whether the socksport address is IPv4 or IPv6.

Interesting. I think this is a mistake, then.

relay.c:

  • Same comment as comment above, wrt the change in connection_edge_process_relay_cell_not_open().

Perhaps a second bug? Andrea - what do you think?

Otherwise, looks okay. Have we tested the latest version of this? :)

I haven't tested the latest version. I'd like to get the final patch ready for testing and then I'll gladly test it. Though I suspect it would be *really* useful if someone else *also* tested it and confirmed it worked. Especially on OS X!

In some kind of ideal world, I like the idea of shipping TBB with Firefox completely sandboxed from making TCP/IP connection on two of our three platforms. The third one being windows, of course. I guess that depends on discovering if NamedPipes will work or not.

comment:45 in reply to:  44 ; Changed 5 years ago by nickm

Replying to ioerror:

Replying to nickm:

Throughout:

  • Looks nice! Much simpler now.

Agreed - a lot of refactoring that makes it easier to read!

  • This is probably gonna break on windows: I don't think they have AF_UNIX, and at least address.c uses AF_UNIX unconditionally. I can clean it up if you want, or you can if you've got mingw cross-compilation stuff installed.

I think it would be great if anyone with Windows could make this work. It would remove lots of local firewall issues, I think. I think the proper way to implement it would be to use a named pipe ( http://msdn.microsoft.com/en-us/library/windows/desktop/aa365590%28v=vs.85%29.aspx ) and it would effectively be the same feature.

Not that simple, I'm afraid. Named pipes don't interoperate with sockets in the way we're using them. You'd need to do some low-level hacking on the async io layer. (Not impossible, but likely to involve complexity.)

In other words "Good idea, but a bit tricky. Somebody should open a new ticket so this one doesn't block on it." ;)

[...]

In some kind of ideal world, I like the idea of shipping TBB with Firefox completely sandboxed from making TCP/IP connection on two of our three platforms. The third one being windows, of course. I guess that depends on discovering if NamedPipes will work or not.

Same as above wrt "Good idea but a bit tricky. Somebody should open a new ticket so this one doesn't block on it." ;)

comment:46 in reply to:  43 ; Changed 5 years ago by nickm

Replying to nickm:

Throughout:

  • Looks nice! Much simpler now.
  • This is probably gonna break on windows: I don't think they have AF_UNIX, and at least address.c uses AF_UNIX unconditionally. I can clean it up if you want, or you can if you've got mingw cross-compilation stuff installed.

Actually, whoops, winsock2.h does define an AF_UNIX, though it doesn't support it. So, no problem.

The only new Windows warning I see is:

src/or/connection.c: In function 'connection_listener_new':
src/or/connection.c:1069:23: warning: unused variable 'options' [-Wunused-variable]
   or_options_t const *options = get_options();
                       ^

comment:47 in reply to:  45 Changed 5 years ago by ioerror

Replying to nickm:

Replying to ioerror:

Replying to nickm:

Throughout:

  • Looks nice! Much simpler now.

Agreed - a lot of refactoring that makes it easier to read!

  • This is probably gonna break on windows: I don't think they have AF_UNIX, and at least address.c uses AF_UNIX unconditionally. I can clean it up if you want, or you can if you've got mingw cross-compilation stuff installed.

I think it would be great if anyone with Windows could make this work. It would remove lots of local firewall issues, I think. I think the proper way to implement it would be to use a named pipe ( http://msdn.microsoft.com/en-us/library/windows/desktop/aa365590%28v=vs.85%29.aspx ) and it would effectively be the same feature.

Not that simple, I'm afraid. Named pipes don't interoperate with sockets in the way we're using them. You'd need to do some low-level hacking on the async io layer. (Not impossible, but likely to involve complexity.)

Ah. That sounds sad.

In other words "Good idea, but a bit tricky. Somebody should open a new ticket so this one doesn't block on it." ;)

Understood. Ok - so that answers that question - so your point was just to make sure that Tor will still work on Windows then? I think you are the best person to help with this Windows stuff - though I'm happy to learn about the mingw stuff if it is documented somewhere...

[...]

In some kind of ideal world, I like the idea of shipping TBB with Firefox completely sandboxed from making TCP/IP connection on two of our three platforms. The third one being windows, of course. I guess that depends on discovering if NamedPipes will work or not.

Same as above wrt "Good idea but a bit tricky. Somebody should open a new ticket so this one doesn't block on it." ;)

Ok. I think this is a TBB discussion that we can have after this is merged. I've opened #14132 for torsocks - which in theory, we can use to test all of these crazy ideas. That is - it should be possible to sandbox Tor Browser with torsocks in a single line - without patching firefox, if we suceed with torsocks hacking plans.

comment:48 in reply to:  46 Changed 5 years ago by ioerror

Actually, whoops, winsock2.h does define an AF_UNIX, though it doesn't support it. So, no problem.

Is that safe? What happens if you try to *use* SocksSocket on Windows? That is - if we enable it by default?

The only new Windows warning I see is:

src/or/connection.c: In function 'connection_listener_new':
src/or/connection.c:1069:23: warning: unused variable 'options' [-Wunused-variable]
   or_options_t const *options = get_options();
                       ^

Did we really not use it?

comment:49 in reply to:  43 Changed 5 years ago by andrea

Hmm, sounds like I'm going to be revising this before #11485 then, since I wanted to branch that off this to avoid duplicating any AF_UNIX code.

Replying to nickm:

Throughout:

  • Looks nice! Much simpler now.
  • This is probably gonna break on windows: I don't think they have AF_UNIX, and at least address.c uses AF_UNIX unconditionally. I can clean it up if you want, or you can if you've got mingw cross-compilation stuff installed.

I haven't got any Windows anything set up; might be best if you do this.

  • Is "SocksSocket" really the right name for this? I'm used to it, but I bet it would confuse users some.

I can't say I like it all that much either but I'm drawing a blank thinking of anything else.

address.c

  • Probably should document how to work with unix domain sockets; they don't actually fit in a tor_addr_t.

Okay.

config.c / tor.1.txt

  • Can we support SocksSocket on-by-default? If so we'd need a way to turn it off. "SocksSocket 0"?

Okay.

connection.c:

  • I wonder if we should try fchmod first, and then chmod. fchmod is a bit safer, right? (Not a new issue.)
  • If we chmod 0660 in the event of a group-writable socket, should we shmod 0600 in the event of a non-group-writable socket? (Not a new issue.)

These both sound like good ideas.

  • The foosocketsgroupwriteable code in connection_listener_new seems to be nearly duplicated.

Hmm...

  • This comment is wrong:
        /* For now only control ports can be Unix domain sockets
          * and listeners at the same time */
    

connection_edge.c:

  • What's the point of the modifications of connection_ap_get_begincell_flags and connection_ap_handshake_rewrite_and_attach ? I think they might be wrong. The flags that they adjust are about whether the destination address is IPv4/IPv6/etc, not whether the socksport address is IPv4 or IPv6.

relay.c:

  • Same comment as comment above, wrt the change in connection_edge_process_relay_cell_not_open().

Otherwise, looks okay. Have we tested the latest version of this? :)

Yeah, tested last night using socat to forward incoming TCP connections to AF_UNIX and it worked great.

comment:50 Changed 5 years ago by andrea

Updated ticket12585_v3 branch includes:

  • Ability to pass lists of default ports into parse_unix_socket_config(), and SocksSocket/ControlSocket 0 syntax to disable defaults
  • Removal of redundant foosocketsgroupwriteable in connection_listener_new()
  • Removal of wrong comment from connection.c
  • Correct initialization of ipv4/ipv6_traffic bits in port_cfg_t in config.c when setting up AF_UNIX ports, and removal of hacky workarounds in connection_ap_get_begincell_flags()/connection_ap_handshake_rewrite_and_attach()/connection_ap_handshake_rewrite_and_attach().

comment:51 Changed 5 years ago by andrea

Also, we now explicitly chmod to 0600 when *GroupWritable isn't specified.

comment:52 Changed 5 years ago by andrea

I've filed #14186 covering the fchmod() issue.

comment:53 Changed 5 years ago by nickm

Status: needs_revisionneeds_review

comment:54 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

LGTM! Merged.

Note: See TracTickets for help on using tickets.