Opened 3 years ago

Last modified 2 years ago

#18213 accepted defect

The parameter WarnUnsafeSocks does not work as specified in the documentation, no warning is logged in the log file

Reported by: propropus Owned by: arma
Priority: Low Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Major Keywords: WarnUnsafeSocks, Log, tor-client needs-decision,
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by arma)

The parameter WarnUnsafeSocks does not work as specified in the documentation, no warning is logged in the log file when a connection is done to an ip address.

If WarnUnsafeSocks 1 (default) is set there is no warning in the log file.
If you look at the code for log_unsafe_socks_warning, the only case where an error is logged is when safe_socks is true. safe_socks is true only when SafeSocks parameter is set, but not when WarnUnsafeSocks is set.

The code should be

if (safe_socks || options->WarnUnsafeSocks) {

instead of

if (safe_socks) {

Child Tickets

Change History (21)

comment:1 Changed 3 years ago by propropus

if (safe_socks || options->WarnUnsafeSocks) {

instead of

if (safe_socks) {

comment:2 Changed 3 years ago by nickm

Keywords: 027-backport added
Milestone: Tor: 0.2.8.x-final

comment:3 Changed 3 years ago by nickm

Status: newneeds_review

comment:4 Changed 3 years ago by cypherpunks

Seems like propropus is correct.

Commit a7334f5122046b55, "Use log_fn_ratelim in a few places", Nick Mathewson, 2012-12-26 (!), did the following to this part of the code:

@@ -1505,22 +1505,19 @@ log_unsafe_socks_warning(int socks_protocol, const char *address,
   static ratelim_t socks_ratelim = RATELIM_INIT(SOCKS_WARN_INTERVAL);
 
   const or_options_t *options = get_options();
-  char *m = NULL;
   if (! options->WarnUnsafeSocks)
     return;
-  if (safe_socks || (m = rate_limit_log(&socks_ratelim, approx_time()))) {
-    log_warn(LD_APP,
+  if (safe_socks) {
+    log_fn_ratelim(&socks_ratelim, LOG_WARN, LD_APP,
              "Your application (using socks%d to port %d) is giving "
              "Tor only an IP address. Applications that do DNS resolves "
              "themselves may leak information. Consider using Socks4A "
              "(e.g. via privoxy or socat) instead. For more information, "
              "please see https://wiki.torproject.org/TheOnionRouter/"
-             "TorFAQ#SOCKSAndDNS.%s%s",
+             "TorFAQ#SOCKSAndDNS.%s",
              socks_protocol,
              (int)port,
-             safe_socks ? " Rejecting." : "",
-             m ? m : "");
-    tor_free(m);
+             safe_socks ? " Rejecting." : "");
   }
   control_event_client_status(LOG_WARN,
                               "DANGEROUS_SOCKS PROTOCOL=SOCKS%d ADDRESS=%s:%d",

A sloppy refactoring that changed the semantics.

Seems like the condition should be removed completely.

comment:5 Changed 3 years ago by special

I don't know why this was in needs_review. I couldn't find an actual patch. propropus' solution would leave the function documentation incorrect.

See my bug18213 branch. I dislike the idea of silently rejecting connections when SafeSocks 1 and WarnUnsafeSocks 0, so I downgraded it to a notice in that case.

https://gitweb.torproject.org/user/special/tor.git/commit/?h=bug18213&id=1033043df081d251fa4b227b8046dc9b34aac2d8

comment:6 Changed 3 years ago by nickm

the patch looks safe to me, and looks like it does what it says it does. I am too distracted right now to confirm whether that's a good idea. Somebody else check please? :)

comment:7 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_information

We should srubbed() the port number here in the log.

Also, I'm worried of a side effect on the user here.

This adds a warning that could scare a user (maybe that's the whole point). I, for instance, often use IPs for my torsocks SSH and with this patch I'm getting quite a bit of warning in my notice log (even though it's rate limited) but still it's on purpose...

Over days of using it, my notice log will be filled with those warnings which could 1) scare/confuse me and 2) add lots of noise and potentially miss other useful log statement.

Are we worried about that? I am a bit to be honest... maybe it's just me.

comment:8 Changed 3 years ago by arma

I've closed #18833 as a duplicate of this ticket.

comment:9 in reply to:  4 ; Changed 3 years ago by arma

Replying to cypherpunks:

Seems like the condition should be removed completely.

I'm not yet convinced. The original goal was to warn every single time we had an unsafe connection, when SafeSocks is on. That way you don't end up in a situation where you do a thing, check your log, see no complaints, and decide that the thing was ok to do.

There's a bigger issue here though. We have a log_unsafe_socks_warning parameter in the parent function, which is set by the TestSocks config option, and that parameter isn't making its way to this log_unsafe_socks_warning() function. Yuck.

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

Replying to dgoulet:

We should srubbed() the port number here in the log.

Why? We want this log to be useful. Hm. There's definitely a conflict here. The man page offers

           This way logs can still be useful, but they don’t leave behind
           personally identifying information about what sites a user might
           have visited.

Are destination ports PII? "It depends."

Also, I'm worried of a side effect on the user here.

This adds a warning that could scare a user (maybe that's the whole point). I, for instance, often use IPs for my torsocks SSH and with this patch I'm getting quite a bit of warning in my notice log (even though it's rate limited) but still it's on purpose...

Then you should either see the warnings and decide they're ok, or turn off this warning feature in Tor because you know you're using an unsafe socks variant and you've decided it's ok?

Are we worried about that? I am a bit to be honest... maybe it's just me.

The even more complicating issue here is that back when I made all these config options, users actually knew what logs were and they looked at them. Now only the expert power users know that there *are* logs. Actually, no, now only the expert power users *have* logs at all. So we might want to do a bigger rethink here.

comment:11 Changed 3 years ago by arma

Description: modified (diff)

comment:12 in reply to:  9 Changed 3 years ago by arma

Replying to arma:

The original goal was to warn every single time we had an unsafe connection, when SafeSocks is on. That way you don't end up in a situation where you do a thing, check your log, see no complaints, and decide that the thing was ok to do.

Ok, let me try that one again. If SafeSocks is on, then Tor will actually reject the unsafe connections. So your Tor will mysteriously not be working. That's when a user (at least a power user) might go to her logs to see what went wrong. We want a reject message for that connection to be there in the logs for her.

Here's the commit message from da3a6e7 when we made that choice:

    We also keep warning every time if safesocks is
    specified, because then the user presumably wants to hear about every
    blocked instance.

So it is ok if we want to change our intended behavior, but we should first know what the original intended behavior was. :)

comment:13 in reply to:  9 Changed 3 years ago by cypherpunks

Replying to arma:

Replying to cypherpunks:

Seems like the condition should be removed completely.

I'm not yet convinced.

Ok, but how is not removing the condition going to help then? If you remove the condition then you will get the warning, unless of course you're over the rate-limiting threshold.

To fully regain the semantics before Nick's patch, it is necessary to disable the rate-limiting, now part of the logging function itself, in the case of safe_socks being true. Depending on the implementation, a similar condition might be necessary. Is this what you are trying to get to?

Or am I misremembering the code that badly?

(I don't yet understand your second paragraph.)

comment:14 Changed 3 years ago by nickm

Keywords: TorCoreTeam201605 added

(We need to decide whether this is an 0.2.8 blocker.)

comment:15 Changed 3 years ago by arma

Keywords: TorCoreTeam201606 added; 027-backport TorCoreTeam201605 removed
Milestone: Tor: 0.2.8.x-finalTor: 0.2.???
Owner: set to arma
Status: needs_informationaccepted

I'll take this one on -- also I am removing it from 0.2.8 since it doesn't need to block the release.

comment:16 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:17 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:18 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:19 Changed 2 years ago by nickm

Keywords: TorCoreTeam201606 removed

comment:20 Changed 2 years ago by nickm

Keywords: tor-client needs-decision added
Priority: HighLow

comment:21 Changed 2 years ago by cypherpunks

The WarnUnsafeSocks option mentioned in the ticket summary was deprecated in #19820 and removed in #22060.

Note: See TracTickets for help on using tickets.