Opened 5 months ago

Closed 4 weeks ago

#22461 closed defect (fixed)

Tor emits inaccurate safesocks warning event whenever you visit a naked IP address

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version: Tor: 0.2.6.2-alpha
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Start your Tor client, then connect to the control port and ask for setevents STATUS_CLIENT.

Then torify wget 128.31.0.34

And on the control port you'll get

650 STATUS_CLIENT WARN DANGEROUS_SOCKS PROTOCOL=SOCKS5 ADDRESS=128.31.0.34:80

That warn event happens if you use the current socks5 variant, but you give it a fqdn that happens to be an IP address:

          if (string_is_valid_ipv4_address(req->address) ||
              string_is_valid_ipv6_address(req->address)) {
            log_unsafe_socks_warning(5,req->address,req->port,safe_socks);

This bug went in to Tor 0.2.6.2-alpha during commit 2862b769.

Bug noticed because of #10165.

Child Tickets

Change History (15)

comment:1 in reply to:  description Changed 5 months ago by arma

Replying to arma:

This bug went in to Tor 0.2.6.2-alpha during commit 2862b769.

For archeology purposes, this commit was for #13315.

comment:2 Changed 5 months ago by arma

To be clear, the problem to warn about is when an application is using a fundamentally unsafe variant of socks -- e.g. socks4 rather than socks4a, or socks5-with-ip-address rather than socks5-with-fqdn.

If the application is correctly using socks5-with-fqdn, yet the address that gets asked for is a string that happens to be an ipv4 address, I think everything is going according to plan.

(I acknowledge that there could be edge cases where the user has set up some complex machinery to do resolves some other unsafe way, and then pass them to an application that does the correct sort of variant of socks. But I don't think it's our place for Tor to warn in that case.)

comment:3 Changed 5 months ago by catalyst

Milestone: Tor: unspecified

#10165 is related. So neither "localhost" nor 127.0.0.1 should work in Tor Browser because of possible leakage? (Also in the comments to #10165 that led to this ticket, why is an (alleged) online banking site trying to open those connections to 127.0.0.1?)

Is the goal here that a user-provided numeric IP address won't generate warnings? (on the assumption the user knows what they're doing?)

comment:4 in reply to:  3 Changed 5 months ago by arma

Replying to catalyst:

#10165 is related. So neither "localhost" nor 127.0.0.1 should work in Tor Browser because of possible leakage?

Yes, but I think that's separate from this ticket.

(Tor Browser removes the "no proxy for" values for localhost and 127.0.0.1, since allowing anything in the "wide open proxy bypass here" settings seemed poor. So if you visit 127.0.0.1 in Tor Browser, it dutifully passes the address to Tor like any other address, and then Tor says

Jun 02 16:16:15.000 [warn] Rejecting SOCKS request for anonymous connection to private address [scrubbed].

assuming you've left ClientRejectInternalAddresses set to 1. All of that is normal I think.)

(Also in the comments to #10165 that led to this ticket, why is an (alleged) online banking site trying to open those connections to 127.0.0.1?)

That is a fine and good question. I had originally thought that the 127.0.0.1 was the address of the application connection, i.e. "I received a tcp connection to the socksport, and the tcp info for the connection says it was from localhost and this high-numbered port". But looking more at the code, I think you're right that indeed something made that person's Tor make a big pile of connections to 127.0.0.1. I think so long as the person hadn't messed with their torrc even more, though, the ClientRejectInternalAddresses check would make Tor fail those requests.

Is the goal here that a user-provided numeric IP address won't generate warnings? (on the assumption the user knows what they're doing?)

Yes, the goal of this ticket is that if you type in http://128.31.0.34/ in your Tor Browser, it should not tell you that you're using an unsafe variant of the socks protocol. Because you're not -- you're using a safe variant of the socks protocol to ask for a legitimate web address.

comment:5 Changed 5 months ago by arma

(I think ordinary Tor Browser users would not see this controller level warning event, since there is no easy way in the interface to expose the controller events to ordinary users. Same with the Tor warning log messages. But that UX side is also a separate issue. :)

comment:6 Changed 5 months ago by rl1987

Status: newneeds_review
Version: Tor: 0.2.6.2-alpha

comment:7 in reply to:  6 Changed 5 months ago by arma

Replying to rl1987:

Quick patch for this bug: https://github.com/rl1987/tor/commit/9e2f78092395d1250f08a21815ab1145409530eb

I think we need to go farther. The patch above still refuses a naked IP address when it's presented using a safe socks version, and that's still a bug. I think we want this:

diff --git a/src/or/buffers.c b/src/or/buffers.c
index 3692ed4..399b591 100644
--- a/src/or/buffers.c
+++ b/src/or/buffers.c
@@ -1684,15 +1684,7 @@ parse_socks(const char *data, size_t datalen, socks_requ
est_t *req,
           req->port = ntohs(get_uint16(data+5+len));
           *drain_out = 5+len+2;
 
-          if (string_is_valid_ipv4_address(req->address) ||
-              string_is_valid_ipv6_address(req->address)) {
-            log_unsafe_socks_warning(5,req->address,req->port,safe_socks);
-
-            if (safe_socks) {
-              socks_request_set_socks5_error(req, SOCKS5_NOT_ALLOWED);
-              return -1;
-            }
-          } else if (!string_is_valid_hostname(req->address)) {
+          if (!string_is_valid_hostname(req->address)) {
             socks_request_set_socks5_error(req, SOCKS5_GENERAL_ERROR);
 
             log_warn(LD_PROTOCOL,

plus fixing the unit tests.

As a bonus (in a different commit hopefully), we could go down to the socks4 parsing section, where we do

      if (!tor_strisprint(req->address) || strchr(req->address,'\"')) {

and replace that with a call to string_is_valid_hostname() so we match up with the socks5 behavior.

comment:8 Changed 5 months ago by rl1987

Updated the patch to refrain from rejecting SOCKS5 requests that contain IP strings.

comment:9 Changed 4 months ago by arma

Status: needs_reviewmerge_ready

Looks great!

comment:10 Changed 4 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.2.x-final

comment:11 Changed 4 months ago by nickm

looks good!

Merging, with some editing to the changes file to try to clarify that this applies to IP-as-hostnames only.

There was a whitespace error in test_socks.c; next time, you can detect these yourself by running the larger testsuite with "make check".

comment:12 Changed 4 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

comment:13 Changed 4 months ago by cypherpunks

Using the example from the ticket description, I'm still able to reproduce the issue on the current master branch. Found this while trying to debug #22572.

comment:14 Changed 4 months ago by nickm

Resolution: fixed
Status: closedreopened

comment:15 Changed 4 weeks ago by dgoulet

Resolution: fixed
Status: reopenedclosed

Ok so after getting clarifications from arma, the bottom line is that tor should NOT warn if we have a SOCKS5 hostname + IP address that is ATYP 0x03 with an IPv4/6 address. Application should always use tor that way if they did get an IP address from the user in the first place and no DNS resolution happened.

Tor is doing that correctly, only warning for IPv4/v6 atyp + IP string.

Torsocks is the one not doing that correctly (#23667).

Note: See TracTickets for help on using tickets.