Opened 3 years ago

Last modified 5 months ago

#21967 new defect

obfs4proxy not killed when unused

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-pt, 031-deferred-20170425, 034-triage-20180328, 034-removed-20180328, tbb-needs, user-feedback, blog
Cc: mcs, gk, arlolra Actual Points:
Parent ID: Points: 1.5
Reviewer: Sponsor:

Description

Seems like Tor does not successfuly kill obfs4proxy when it's no longer used. It's unclear why. Maybe obfs4proxy never receives the SIGTERM, or never notices that the stdin is closed?

Experiment:

  • Startup Tor Browser using default obfs4 bridges.
  • Check that obfs4proxy process has spawned (ps -fax | grep obfs)
  • Reconfigure Tor Browser to not use bridges at all.
  • Check that obfs4proxy process is still there...

Child Tickets

Change History (17)

comment:1 Changed 3 years ago by mcs

Cc: mcs added

comment:2 Changed 3 years ago by gk

Cc: gk added

comment:3 Changed 3 years ago by arma

(Thanks asn! We first noticed this bug in #21314, but it's good to get it its own dedicated ticket here.)

comment:4 Changed 3 years ago by yawning

Seems like Tor does not successfuly kill obfs4proxy when it's no longer used. It's unclear why.
Maybe obfs4proxy never receives the SIGTERM, or never notices that the stdin is closed?

It's "tor never issues the SIGTERM, or close stdin".

As far as I can tell, the Bridge directives are always parsed, regardless of the UseBridges setting, so if a Bridge entry exists for a transport at all (regardless of it if is used), the PT will always exist.

Removing Bridges with RESETCONF doesn't seem to affect bridge_list at the time when pts would be torn down either.

All of this is crap is half broken on the tor side basically, and it has nothing to do with the process teardown code.

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

Replying to yawning:

Removing Bridges with RESETCONF doesn't seem to affect bridge_list at the time when pts would be torn down either.

Check out this code in config.c:

  if (options->Bridges) {
    mark_bridge_list();
    for (cl = options->Bridges; cl; cl = cl->next) {
      bridge_line_t *bridge_line = parse_bridge_line(cl->value);
      if (!bridge_line) {
        log_warn(LD_BUG,
                 "Previously validated Bridge line could not be added!");
        return -1;
      }
      bridge_add_from_config(bridge_line);
    }
    sweep_bridge_list();
  }

If we moved the mark and sweep to outside the if, then we would clear out bridge_list in the case where options->Bridges is 0. That seems wise.

Or if we prefered, we could start this stanza with

  if (!options->Bridges) {
    clear_bridge_list();
  } else {
    ...

comment:6 in reply to:  5 Changed 3 years ago by arma

Replying to arma:

If we moved the mark and sweep to outside the if, then we would clear out bridge_list in the case where options->Bridges is 0. That seems wise.

Compare to how it's done for Transports:

  mark_transport_list();
  pt_prepare_proxy_list_for_config_read();
  if (!options->DisableNetwork) {
    if (options->ClientTransportPlugin) {
      [...]
    }
  }
  sweep_transport_list();
  sweep_proxy_list();
Last edited 3 years ago by arma (previous) (diff)

comment:7 Changed 3 years ago by arma

Ok, that turns out to have been the easy part of the fix.

The fix above does indeed sweep unconfigured bridges from the bridge_list.

But the managed proxy still stays running.

That's because the transports and managed proxies get marked:

  mark_transport_list();
  pt_prepare_proxy_list_for_config_read();

but then they (both the transport and the managed proxy) get rescued in pt_kickstart_proxy:

    if (mp->was_around_before_config_read) {
      /* If this managed proxy was around even before we read the
         config this time, it means that it was already enabled before
         and is not useless and should be kept. If it's marked for
         removal, unmark it and make sure that we check whether it
         needs to be restarted. */
      if (mp->marked_for_removal) {
        mp->marked_for_removal = 0;
        check_if_restarts_needed = 1;
      }

      /* For each new transport, check if the managed proxy used to
         support it before the SIGHUP. If that was the case, make sure
         it doesn't get removed because we might reuse it. */
      SMARTLIST_FOREACH_BEGIN(with_transport_list, const char *, transport) {
        old_transport = transport_get_by_name(transport);
        if (old_transport)
          old_transport->marked_for_removal = 0;
      } SMARTLIST_FOREACH_END(transport);
    }

And with comments like that...are we sure this ever worked? It sure seems to be intentional about keeping them around.

comment:8 Changed 3 years ago by arma

Wugh. There's some sort of subtlety in pt_configure_remaining_proxies() where it restarts managed proxies sometimes, and it might implicitly drop transports it doesn't know it needs, or something. See the call to sweep_transport_list() in proxy_prepare_for_restart(). But by then it's too late because pt_kickstart_proxy set mp->marked_for_removal to 0 and nothing ever marks it again.

(Whoever decided that transport_t should have a marked_for_removal flag, and managed_proxy_t should have an identically named marked_for_removal flag, is not a nice person.)

comment:9 Changed 2 years ago by nickm

Keywords: 031-deferred-20170425 added
Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final

Triage: batch-defer unowned items of priority Medium or lower to 0.3.2.

comment:10 Changed 2 years ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.4.x-final

comment:11 Changed 20 months ago by arlolra

Cc: arlolra added

comment:12 Changed 20 months ago by gk

Keywords: tbb-wants added

comment:13 Changed 19 months ago by nickm

Keywords: 034-triage-20180328 added

comment:14 Changed 19 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:15 Changed 19 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

comment:16 Changed 15 months ago by teor

Keywords: tbb-needs added; tbb-wants removed

Prefer the more common tbb-needs to tbb-wants.
There doesn't appear to be any difference in how much TBB needs based on the flag.

comment:17 Changed 5 months ago by gaba

Keywords: user-feedback blog added

Some comments on this bug here: https://blog.torproject.org/comment/280961#comment-280961

Note: See TracTickets for help on using tickets.