Opened 5 years ago

Closed 5 years ago

#15215 closed defect (not a bug)

Use destination address type in SOCKS5 reply rather than AP address

Reported by: sysrqb Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: dgoulet, yawning Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When replying, checking the address family of the local address only works under the assumption that the local socket type is the same as the destination socket. This was true, until we began supporting AF_UNIX socksports. The result is now that when tor replies to an application over a unix socket, it specifies that it connected to an IPv6 address, when it's possible it connected to an IPv4 address. It seems like a good idea that we base this on the address family of the destination address.

Child Tickets

Change History (5)

comment:1 Changed 5 years ago by sysrqb

Status: newneeds_review

This seems to work for me, is there a better way?

diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index 2a1a2f0..f18474f 100644
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -2568,9 +2568,10 @@ connection_ap_handshake_socks_reply(entry_connection_t *conn, char *reply,
     /* leave version, destport, destip zero */
     connection_write_to_buf(buf, SOCKS4_NETWORK_LEN, ENTRY_TO_CONN(conn));
   } else if (conn->socks_request->socks_version == 5) {
+    tor_addr_t addr;
     size_t buf_len;
     memset(buf,0,sizeof(buf));
-    if (tor_addr_family(&conn->edge_.base_.addr) == AF_INET) {
+    if (tor_addr_parse(&addr, conn->socks_request->address) == AF_INET) {
       buf[0] = 5; /* version 5 */
       buf[1] = (char)status;
       buf[2] = 0;

Also in my pub repo on branch bug15215.

comment:2 Changed 5 years ago by dgoulet

This one is important because with AF_UNIX or even hostname, a v6 payload is returned which is too many bytes for the application resulting in a possibly bad state for the app.

Ack!

comment:3 Changed 5 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.6.x-final

We can consider it for 0.2.6, I guess, but I still don't think I understand. Let me try to unpack my thoughts.

The old behavior (in 0.2.5 and earlier), was that if you connected to SocksPort over IPv4 you got "0.0.0.0" as your address, and otherwise you were connecting over IPv6 so you got "[::]" as your address.

In 0.2.6, with the addition of AF_UNIX sockports, the behavior is that if you connected to SocksPort over IPv4 you got "0.0.0.0" as your address, and otherwise you are connecting over IPv6 OR AF_UNIX so you get "[::]" as your address.

The proposed change is that if the target address is an IPv4 address, we say "0.0.0.0" regardless of how you connected to Tor, and otherwise we say "[::]" regardless of how you connected to Tor. Is that right?

(Are we looking at the address the user asked for or the address in the CONNECTED cell? I think maybe you told me the latter, but I can't see where that would be getting set. Also, what happens with hidden services here?)

comment:4 Changed 5 years ago by sysrqb

Cc: dgoulet yawning added

After re-rereading the socks5 spec, and discussing this with dgoulet and yawning, I realized that the new behavior is wrong. I have a better and not-wrong, this time (as I understand it) patch. The old behavior of looking at the ap connection address family type was correct, but it seems like there's an even easier way because connection_t has socket_family which is set when we open the connection listener and it's copied into new connections, so looking at this seems easier.

New branch bug15215_2

diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index 2a1a2f0..5571817 100644
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -2570,7 +2570,8 @@ connection_ap_handshake_socks_reply(entry_connection_t *conn, char *reply,
   } else if (conn->socks_request->socks_version == 5) {
     size_t buf_len;
     memset(buf,0,sizeof(buf));
-    if (tor_addr_family(&conn->edge_.base_.addr) == AF_INET) {
+    if (conn->edge_.base_.socket_family == AF_INET ||
+        conn->edge_.base_.socket_family == AF_UNIX) {
       buf[0] = 5; /* version 5 */
       buf[1] = (char)status;
       buf[2] = 0;

comment:5 Changed 5 years ago by sysrqb

Resolution: not a bug
Status: needs_reviewclosed

After further consideration and discussion with yawning it was decided that there's no reason to change this. I like the patch, but ipv4 isn't a better choice than ipv6, and this isn't a workaround for a confused client.

Note: See TracTickets for help on using tickets.