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

changed milestone to %Tor: 0.3.2.x-final

added component::core tor/tor milestone::Tor: 0.3.2.x-final priority::medium resolution::fixed severity::normal status::closed type::defect version::tor 0.2.6.2-alpha labels

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 (moved).

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.)

#10165 (moved) 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 (moved) 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?)

Trac:
Milestone: N/A to Tor: unspecified

Replying to catalyst:

#10165 (moved) 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 (moved) 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.

(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. :)

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

Trac:
Status: new to needs_review
Version: N/A to Tor: 0.2.6.2-alpha

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.

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

Looks great!

Trac:
Status: needs_review to merge_ready

Trac:
Milestone: Tor: unspecified to Tor: 0.3.2.x-final

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".

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

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 (moved).

Trac:
Status: closed to reopened
Resolution: fixed to N/A

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 (moved)).

Trac:
Status: reopened to closed
Resolution: N/A to fixed

closed

mentioned in issue #22572 (moved)

mentioned in issue #23667 (moved)

mentioned in issue #25036 (moved)

moved to tpo/core/tor#22461 (closed)

mentioned in issue tpo/core/torsocks#23667 (closed)

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

Child items ...

Activity