Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#5604 closed defect (fixed)

If DisableNetwork is set, we open all our ports and then close them at each setconf

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

    /* Launch the listeners.  (We do this before we setuid, so we can bind to
     * ports under 1024.)  We don't want to rebind if we're hibernating. If
     * networking is disabled, this will close all but the control listeners,
     * but disable those. */
    if (!we_are_hibernating()) {
      if (retry_all_listeners(replaced_listeners, new_listeners) < 0) {
        *msg = tor_strdup("Failed to bind one of the listener ports.");
        goto rollback;
      }
    }
    if (options->DisableNetwork) {
      /* Aggressively close non-controller stuff, NOW */
      log_notice(LD_NET, "DisableNetwork is set. Tor will not make or accept "
                 "non-control network connections. Shutting down all existing "
                 "connections.");
      connection_mark_all_noncontrol_connections();
    }

It isn't sufficient to simply not call the first part when DisableNetwork is set, because of the case where we want to open new control ports. But it looks like we're just opening them all, even when in the next clause we'll close (most of) them again?

Child Tickets

Change History (8)

comment:1 Changed 8 years ago by nickm

I think the right fix is something like:

  • give retry_all_listeners a flag (or make it notice that DisableNetwork is set) to have it not enable any non-Control listeners. (It disables any listeners that it doesn't enable IIRC)
  • Leave the connection_mark_all_noncontrol_connections() call alone.

Does that sound ok?

comment:2 Changed 8 years ago by arma

Sounds good

comment:3 Changed 8 years ago by nickm

Status: newneeds_review

Please review branch "bug5604" in my public repo.

comment:4 Changed 8 years ago by arma

Summary: If DisableNetwork is set, we open all our ports and then close then at each setconfIf DisableNetwork is set, we open all our ports and then close them at each setconf

comment:5 Changed 7 years ago by ln5

FWIW, 074bf72 looks correct to me given that there are no "listener
connections" which are not of the type CONN_TYPE_CONTROL_LISTENER.

Since retry_listener_ports() doesn't really _close_ any ports, the new
formal parameter 'close_all_noncontrol' seems slightly misnamed. We
could make it 'uint8_t ports_filter' but that would make the function
harder to understand from its prototype alone.

At least it needs documentation. Maybe adding something like this?

If <b>close_all_noncontrol</b> is 1, don't consider any ports in
<b>ports</b> which are not control listener ports.

comment:6 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Added some documentation, renamed a parameter, and merged; thanks!

comment:7 Changed 7 years ago by nickm

Keywords: tor-client added

comment:8 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.