Opened 7 years ago

Closed 6 years ago

#8879 closed defect (fixed)

Tor's socks5 handshake with username/password auth doesn't follow the protocol spec, and pidgin notices

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

With Tor 0.2.4.12-alpha and the #8117 patch, my pidgin will no longer handshake with my Tor.

May 15 01:15:13.260 [debug] conn_read_callback(): socket 6 wants to read.
May 15 01:15:13.260 [debug] connection_handle_listener_read(): Connection accepted on socket 14 (child of fd 6).
May 15 01:15:13.260 [debug] connection_add_impl(): new conn type Socks, socket 14, address 127.0.0.1, n_conns 6.
May 15 01:15:13.260 [debug] conn_read_callback(): socket 14 wants to read.
May 15 01:15:13.260 [debug] read_to_chunk(): New byte 0 is '5'
May 15 01:15:13.260 [debug] read_to_chunk(): New byte 1 is '3'
May 15 01:15:13.260 [debug] read_to_chunk(): New byte 2 is '0'
May 15 01:15:13.260 [debug] read_to_chunk(): New byte 3 is '3'
May 15 01:15:13.260 [debug] read_to_chunk(): New byte 4 is '2'
May 15 01:15:13.260 [debug] read_to_chunk(): Read 5 bytes. 5 on inbuf.
May 15 01:15:13.260 [debug] connection_ap_handshake_process_socks(): entered.
May 15 01:15:13.260 [debug] parse_socks(): socks5: accepted method 2 (username/password)
May 15 01:15:13.260 [debug] connection_write_to_buf_impl_(): Wrote byte 0: '5'
May 15 01:15:13.260 [debug] connection_write_to_buf_impl_(): Wrote byte 1: '2'
May 15 01:15:13.260 [debug] connection_ap_handshake_process_socks(): socks handshake not all here yet.
May 15 01:15:13.260 [debug] connection_ap_handshake_process_socks(): entered.
May 15 01:15:13.260 [debug] connection_ap_handshake_process_socks(): socks handshake not all here yet.
May 15 01:15:13.260 [debug] conn_write_callback(): socket 14 wants to write.
May 15 01:15:13.260 [debug] conn_read_callback(): socket 14 wants to read.
May 15 01:15:13.260 [debug] read_to_chunk(): New byte 0 is '1'
May 15 01:15:13.261 [debug] read_to_chunk(): New byte 1 is '0'
May 15 01:15:13.261 [debug] read_to_chunk(): New byte 2 is '0'
May 15 01:15:13.261 [debug] read_to_chunk(): Read 3 bytes. 3 on inbuf.
May 15 01:15:13.261 [debug] connection_ap_handshake_process_socks(): entered.
May 15 01:15:13.261 [debug] parse_socks(): socks5: Accepted username/password without checking.
May 15 01:15:13.261 [debug] connection_write_to_buf_impl_(): Wrote byte 0: '5'
May 15 01:15:13.261 [debug] connection_write_to_buf_impl_(): Wrote byte 1: '0'
May 15 01:15:13.261 [debug] connection_ap_handshake_process_socks(): socks handshake not all here yet.
May 15 01:15:13.261 [debug] connection_ap_handshake_process_socks(): entered.
May 15 01:15:13.261 [debug] connection_ap_handshake_process_socks(): socks handshake not all here yet.
May 15 01:15:13.261 [debug] conn_write_callback(): socket 14 wants to write.
May 15 01:15:13.261 [debug] conn_read_callback(): socket 14 wants to read.
May 15 01:15:13.261 [debug] read_to_chunk(): Encountered eof on fd 14
May 15 01:15:13.261 [debug] connection_ap_handshake_process_socks(): entered.
May 15 01:15:13.261 [debug] connection_ap_handshake_process_socks(): socks handshake not all here yet.
May 15 01:15:13.261 [info] connection_edge_reached_eof(): conn (fd 14) reached eof. Closing.

If I change my socksport line in my torrc to

SocksPort 9050 PreferSOCKSNoAuth

then it works again:

May 15 01:19:03.307 [debug] conn_read_callback(): socket 6 wants to read.
May 15 01:19:03.307 [debug] connection_handle_listener_read(): Connection accepted on socket 11 (child of fd 6).
May 15 01:19:03.307 [debug] connection_add_impl(): new conn type Socks, socket 11, address 127.0.0.1, n_conns 5.
May 15 01:19:03.307 [debug] conn_read_callback(): socket 11 wants to read.
May 15 01:19:03.307 [debug] read_to_chunk(): New byte 0 is '5'
May 15 01:19:03.307 [debug] read_to_chunk(): New byte 1 is '3'
May 15 01:19:03.307 [debug] read_to_chunk(): New byte 2 is '0'
May 15 01:19:03.307 [debug] read_to_chunk(): New byte 3 is '3'
May 15 01:19:03.307 [debug] read_to_chunk(): New byte 4 is '2'
May 15 01:19:03.307 [debug] read_to_chunk(): Read 5 bytes. 5 on inbuf.
May 15 01:19:03.307 [debug] connection_ap_handshake_process_socks(): entered.
May 15 01:19:03.307 [debug] parse_socks(): socks5: accepted method 0 (no authentication)
May 15 01:19:03.307 [debug] connection_write_to_buf_impl_(): Wrote byte 0: '5'
May 15 01:19:03.307 [debug] connection_write_to_buf_impl_(): Wrote byte 1: '0'
May 15 01:19:03.307 [debug] connection_ap_handshake_process_socks(): socks handshake not all here yet.
May 15 01:19:03.307 [debug] connection_ap_handshake_process_socks(): entered.
May 15 01:19:03.307 [debug] connection_ap_handshake_process_socks(): socks handshake not all here yet.
May 15 01:19:03.307 [debug] conn_write_callback(): socket 11 wants to write.
May 15 01:19:03.311 [debug] conn_read_callback(): socket 11 wants to read.
May 15 01:19:03.311 [debug] read_to_chunk(): New byte 0 is '5'
May 15 01:19:03.311 [debug] read_to_chunk(): New byte 1 is '1'
May 15 01:19:03.311 [debug] read_to_chunk(): New byte 2 is '0'
May 15 01:19:03.311 [debug] read_to_chunk(): New byte 3 is '3'
May 15 01:19:03.311 [debug] read_to_chunk(): New byte 4 is '22'
May 15 01:19:03.311 [debug] read_to_chunk(): New byte 5 is '97'
[...]
May 15 01:19:03.311 [debug] read_to_chunk(): Read 29 bytes. 29 on inbuf.
May 15 01:19:03.311 [debug] connection_ap_handshake_process_socks(): entered.
May 15 01:19:03.312 [debug] parse_socks(): socks5: checking request
May 15 01:19:03.312 [debug] parse_socks(): socks5: fqdn address type

Child Tickets

Change History (16)

comment:1 Changed 7 years ago by arma

For posterity, here's the patch that provided the extra debugging lines above:

diff --git a/src/or/buffers.c b/src/or/buffers.c
index 47fa31d..1ef46ad 100644
--- a/src/or/buffers.c
+++ b/src/or/buffers.c
@@ -645,6 +645,11 @@ read_to_chunk(buf_t *buf, chunk_t *chunk, tor_socket_t fd,
 size_t at_most,
     *reached_eof = 1;
     return 0;
   } else { /* actually got bytes. */
+    {
+      int i;
+      for (i = chunk->datalen; i < chunk->datalen + read_result; i++)
+        log_debug(LD_NET,"New byte %d is '%u'", i, *(chunk->data + i));
+    }
     buf->datalen += read_result;
     chunk->datalen += read_result;
     log_debug(LD_NET,"Read %ld bytes. %d on inbuf.", (long)read_result,
diff --git a/src/or/connection.c b/src/or/connection.c
index 87fa799..8b50e20 100644
--- a/src/or/connection.c
+++ b/src/or/connection.c
@@ -3714,6 +3714,11 @@ connection_write_to_buf_impl_(const char *string, size_t len,
                                                  dir_conn->zlib_state,
                                                  string, len, done));
   } else {
+    {
+      int i;
+      for (i = 0; i < len; i++)
+        log_debug(LD_NET,"Wrote byte %d: '%u'", i, string[i]);
+    }
     CONN_LOG_PROTECT(conn, r = write_to_buf(string, len, conn->outbuf));
   }
   if (r < 0) {

comment:2 Changed 7 years ago by arma

The plot thickens!

Pidgin's debug window tells me

(01:26:14) util: requesting to fetch a URL
(01:26:14) dnsquery: Performing DNS lookup for 127.0.0.1
(01:26:14) dnsquery: IP resolved for 127.0.0.1
(01:26:14) proxy: Attempting connection to 127.0.0.1
(01:26:14) proxy: Connecting to api.screenname.aol.com:443 via 127.0.0.1:9050 using SOCKS5
(01:26:14) socks5 proxy: Connection in progress
(01:26:14) socks5 proxy: Connected.
(01:26:14) socks5 proxy: Able to read.
(01:26:14) socks5 proxy: Got auth response.
(01:26:14) proxy: Connection attempt failed: Received invalid data on connection with server

comment:3 Changed 7 years ago by arma

Here's the code in pidgin's libpurple:

        if ((connect_data->read_buffer[0] != 0x01) || (connect_data->read_buffe
r[1] != 0x00)) {
                purple_proxy_connect_data_disconnect(connect_data,
                                _("Received invalid data on connection with server"));
                return;
        }

And sure enough, if I change Tor's response like this:

@@ -1750,7 +1755,7 @@ parse_socks(const char *data, size_t datalen, socks_reque
         return 0;
       }
       req->replylen = 2; /* 2 bytes of response */
-      req->reply[0] = 5;
+      req->reply[0] = 1; /* WRONG! should be 5. but appeases pidgin. */
       req->reply[1] = 0; /* authentication successful */
       log_debug(LD_APP,
                "socks5: Accepted username/password without checking.");

then pidgin's handshake with Tor works.

comment:4 Changed 7 years ago by arma

Actually! I think this might be Tor's fault, yes?

Here's what the wikipedia socks page says:

For username/password authentication the client's authentication request is

    field 1: version number, 1 byte (must be 0x01)
    field 2: username length, 1 byte
    field 3: username
    field 4: password length, 1 byte
    field 5: password

Server response for username/password authentication:

    field 1: version, 1 byte
    field 2: status code, 1 byte.
        0x00 = success
        any other value = failure, connection must be closed

We get "version number, 1 byte (must be 0x01)" on the request from pidgin, and then we use version 5 in our response. But don't we want to use version 1 in our response? It's not the socks version, it's the auth format version.

comment:5 Changed 7 years ago by arma

Summary: pidgin fails to do socks5 handshake with username/password authTor's socks5 handshake with username/password auth doesn't follow the protocol spec, and pidgin notices

comment:6 Changed 7 years ago by arma

Status: newneeds_review

http://tools.ietf.org/html/rfc1929 confirms "The VER field contains the current version of the subnegotiation, which is X'01'."

See my bug8879 branch for a fix.

(Backporting is probably inappropriate so long as the #8117 fix doesn't get backported. Then again, this fix is independent of that bug wrt applications that offer *only* username+password auth, and do not offer no-auth. Still, 0.2.3 has lasted this long without a fix, and it will last longer.)

comment:7 Changed 7 years ago by arma

(sysrqb confirms for me that torsocks does not care whether this byte is a 1 or a 5, so it should work either way)

comment:8 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.3.x-final

Looks correct for me; merging the branch into 0.2.4 and throwing the ticket into the 0.2.3 milestone so we don't forget about it.

comment:9 Changed 7 years ago by nickm

Had to add another commit to make the unit tests pass again. It's now in branch "bug8879" in my public repository.

comment:10 in reply to:  9 Changed 7 years ago by arma

Status: needs_reviewnew

Replying to nickm:

Had to add another commit to make the unit tests pass again.

Ha. And I thought I was so hot for actually doing a development thing again. Thanks!

comment:11 Changed 7 years ago by nickm

Status: newneeds_review

not to worry -- I forget stuff and break the unit tests waaaay too often.

comment:12 Changed 7 years ago by arma

What needs review here? You already merged your fix, I believe.

comment:13 Changed 7 years ago by arma

(and the fix looked fine)

comment:14 Changed 7 years ago by nickm

It's now in the 0.2.3 milestone pending review. I lean towards "don't merge" I guess.

comment:15 Changed 6 years ago by arma

I also lean towards don't merge.

comment:16 Changed 6 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final
Resolution: fixed
Status: needs_reviewclosed

Resolving as fixed-in-0.2.4 then.

Note: See TracTickets for help on using tickets.