Opened 6 years ago

Closed 4 months ago

#4019 closed defect (fixed)

Tor warns about public SocksPort addresses twice on startup

Reported by: rransom Owned by: isis
Priority: Low Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-client, easy, review-group-18
Cc: Actual Points:
Parent ID: Points: 0.5
Reviewer: Sponsor:

Description

(moved from #4018)

Sep 14 04:11:29.518 [notice] Tor v0.2.3.4-alpha (git-5f4f727d58daa194). This is experimental software. Do not rely on it for strong anonymity. (Running on OpenBSD i386)
Sep 14 04:11:29.519 [notice] Read configuration file "/etc/tor/torrc".
Sep 14 04:11:29.519 [warn] You specified a public address for SocksPort. Other people on the Internet might find your computer and use it as an open proxy. Please don't allow this unless you have a good reason.
Sep 14 04:11:29.521 [warn] It's a little hard to tell, but you seem to have Libevent 1.4.0-beta header files, whereas you have linked against Libevent 1.4.14b-stable.  This will probably make Tor crash.
Sep 14 04:11:29.523 [notice] Initialized libevent version 1.4.14b-stable using method kqueue. Good.
Sep 14 04:11:29.524 [warn] You specified a public address for SocksPort. Other people on the Internet might find your computer and use it as an open proxy. Please don't allow this unless you have a good reason.
Sep 14 04:11:29.524 [notice] Opening Socks listener on 127.0.0.1:9050
Sep 14 04:11:29.524 [notice] Opening Socks listener on 192.168.7.1:9050
Sep 14 04:11:29.524 [notice] Opening Control listener on 127.0.0.1:9071

Tor is warning about a single SocksPort that it thinks is bound to a public address twice: once when it reads the config file, and a second time when it opens the listeners.

(192.168.7.1 isn't a public address; that bug is #4018.)

Child Tickets

Attachments (2)

0001-When-parsing-ports-only-print-warning-messages-when-.patch (2.9 KB) - added by tomfitzhenry 5 years ago.
0001-only-warn-about-non-local-ports-once.patch (8.7 KB) - added by joelanders 3 years ago.

Download all attachments as: .zip

Change History (39)

comment:1 Changed 5 years ago by tomfitzhenry

Owner: set to tomfitzhenry
Status: newassigned

comment:2 Changed 5 years ago by tomfitzhenry

Cc: trac-tor@… added
Status: assignedneeds_review

There are a few ways to fix this, including:

  1. When calling parse_ports, toggle CL_PORT_WARN_NONLOCAL on each type of port (SocksPort, DNSPort, etc.) based on whether validate_only is set. validate_only is set when validating the config file, but not when reading the config file to decide what to listen on.
  2. As above, but in another method in the call stack.
  3. Don't print warnings. Rather, pass them back up and decide whether to output based on whether we're validating the config file or opening listeners.
  4. Rewrite to only read the config once (scary! Not worth it for this minor issue.)

I went for 1, because it seemed like the most sensible place to put it in the call stack. 3 and 4 are non-trivial changes, that this small issue doesn't justify.

Feedback welcome!

comment:3 Changed 5 years ago by tomfitzhenry

Status: needs_reviewassigned

An important point is that this change has an unexpected side-effect.

Prior to this change: When validating the config file (in parse_port_config), warn_nonlocal_controller_ports() was called, which would remove unauthenticated non-local ControlPorts from 'ports'. parse_port_config() then returns to parse_port(). At the end of parse_port(), n_ports_out is set to the number of ports. This number would not include the unauthenticated non-local ControlPort.

After this change: When validating the config file (in parse_port_config), warn_nonlocal_controller_ports() would not be called. Thus n_ports_out /would/ include the unauthenticated non-local ControlPort.

I will see if there is a way to preserve old behaviour, and if not, whether n_ports_out's value changing will cause any issues (there are various corner cases related to value of FooListenAddress).

comment:4 Changed 5 years ago by nickm

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

Deferring this to 0.2.4; it's worth doing, but if I understand correctly it isn't hurting anything now.

comment:5 Changed 5 years ago by nickm

Keywords: tor-client added

comment:6 Changed 5 years ago by nickm

Component: Tor ClientTor

comment:7 Changed 5 years ago by nickm

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

comment:8 Changed 4 years ago by nickm

Keywords: easy added

comment:9 Changed 4 years ago by nickm

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

Moving nearly all needs_revision tickets into 0.2.6, as now untimely for 0.2.5.

Changed 3 years ago by joelanders

comment:10 Changed 3 years ago by joelanders

I plumbed validate_only two functions deeper so we could skip logging if it's set.
Not sure if it's worth the added ugliness?

comment:11 Changed 3 years ago by nickm

Status: needs_revisionneeds_review

Cool, a patch!

comment:12 Changed 3 years ago by nickm

This approach seems , but I don't think validate_only is really the right flag to use for this: Warning is a form of validation, after all. The convention for the validate_only flag elsewhere is that it applies to functions where validation happens in all cases, but applying the changes is optional.

I wonder, should this instead use a local static flag to track whether the warning has been given? Or maybe a flag on the port object or something...

comment:13 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

(Renaming validate_only would probably be fine too. But what does warn_nonlocal_controller_port actually _do_ if validate_only is not set? )

comment:14 Changed 3 years ago by tomfitzhenry

Cc: trac-tor@… removed

comment:15 Changed 3 years ago by nickm

Keywords: 026-triaged-1 026-deferrable added

comment:16 Changed 3 years ago by nickm

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

comment:17 Changed 3 years ago by nickm

Keywords: 027-triaged-1-out added

Marking triaged-out items from first round of 0.2.7 triage.

comment:18 Changed 3 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.???

Move *most* 0.2.7-triaged-1-out needs_revision items into 0.2.???. Keep a few based on my sense of the sensible.

comment:19 Changed 21 months ago by nickm

Points: small
Severity: Normal

comment:20 Changed 11 months ago by teor

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

Milestone renamed

comment:21 Changed 10 months 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:22 Changed 5 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:23 Changed 5 months ago by nickm

Keywords: 027-triaged-in added

comment:24 Changed 5 months ago by nickm

Keywords: 027-triaged-in removed

comment:25 Changed 5 months ago by nickm

Keywords: 027-triaged-1-out removed

comment:26 Changed 5 months ago by nickm

Keywords: 026-triaged-1 removed

comment:27 Changed 5 months ago by nickm

Keywords: 026-deferrable removed

comment:28 Changed 5 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.2.x-final
Owner: changed from tomfitzhenry to nickm
Points: small0.5
Priority: MediumLow
Status: needs_revisionaccepted

comment:29 Changed 5 months ago by nickm

Status: acceptedneeds_review

let's just clean this up and get it in; it's small and the bug is annoying.

comment:30 Changed 4 months ago by nickm

Keywords: review-group-18 added

comment:31 Changed 4 months ago by isis

Status: needs_reviewneeds_revision

If I understood nickm correctly, this needs a revision to use a local static variable to keep track of whether we've already warned the user. (Also because it's been a while and the patch no longer applies.)

comment:32 Changed 4 months ago by isis

Owner: changed from nickm to isis
Status: needs_revisionassigned

comment:33 Changed 4 months ago by isis

Status: assignedneeds_review

Okay, there's a patch in my bug4091 branch. While I was at it, I also changed the function signature of warn_nonlocal_client_ports() to make an argument const because it's only ever read from.

The patch is basically a one-line fix that says "only respect the value of the CL_PORT_WARN_NONLOCAL flag if parse_ports() wasn't called with validate_only==1".

At first, I tried doing a different thing with setting a bit-field on each port_cfg_t in warn_nonlocal_client_ports() (which is called by parse_port_config(), which is then called by parse_ports() in two different places), but since parse_port_config() sets up the smartlist it just loses the marker that we had already warned and still warns twice.

comment:34 Changed 4 months ago by nickm

Status: needs_reviewneeds_revision

LGTM but needs a changes file.

comment:35 Changed 4 months ago by nickm

(reviewers note: this is #4019, but the branch is bug4091. I also do this with bug numbers all the time.)

comment:36 in reply to:  35 Changed 4 months ago by isis

Status: needs_revisionmerge_ready

Replying to nickm:

(reviewers note: this is #4019, but the branch is bug4091. I also do this with bug numbers all the time.)


Oops! I renamed the branch to bug4019 to avoid future confusion, and I added an extra commit for the changes file.

comment:37 Changed 4 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged; thanks!

Note: See TracTickets for help on using tickets.