Opened 5 years ago

Closed 5 years ago

#13213 closed defect (implemented)

Tor should tell its pluggable transports when DisableNetwork gets set/unset

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

Description

Orbot uses Tor's DisableNetwork feature to turn off use of the network when it wants to save battery. In fact, some people might even use it as a security feature to avoid touching the network at certain times.

But Yawning tells me that when Tor sets DisableNetwork to 1, nobody tells the pluggable transports to go quiet. Or turn things back on when DisableNetwork goes back to 0.

Child Tickets

Change History (9)

comment:1 Changed 5 years ago by yawning

Cc: yawning added

comment:2 Changed 5 years ago by dcf

Isn't it sufficient for tor just to terminate all its existing SOCKS connections when DisableNetwork gets set? Does it do that already? Having the SOCKS connection terminated will cause a PT to close the corresponding outgoing network connection.

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

Replying to dcf:

Having the SOCKS connection terminated will cause a PT to close the corresponding outgoing network connection.

All of them? Is that specified in the pt protocol? I bet Brandon's Dust protocol won't want to obey this constraint.

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

Replying to arma:

Replying to dcf:

Having the SOCKS connection terminated will cause a PT to close the corresponding outgoing network connection.

All of them? Is that specified in the pt protocol? I bet Brandon's Dust protocol won't want to obey this constraint.

It is not specified that that they do. I might argue that if they keep using the network, it's for a good reason. Like maybe a PT's purpose is to generate cover traffic so an observer can't see when your tor is idle. Flash proxy will break its existing WebSocket connections, but won't close its external listeners, for example.

The confusion is because we're punning on different interpretations of "DisableNetwork". One is "drop the firewall and let nothing get out": in that case, tor should just kill its client PTs. The other is "make tor close its connections, knowing that the (PT, socket, OS) abstractions on which they are built may continue to operate as usual": in that case, tor should just close its PT connections.

If the use case for DisableNetwork is saving battery on a mobile, then I think you want to kill the PT processes. Same is the use case is to be a poor man's firewall.

Speaking as an implementer, I would probably rather have my program killed and restarted.

The same issue exists with SocksProxy. Tor can't enforce the proxy not to keep using the network after it has disconnected. Same with external-mode proxies: there's nothing you can do to them besides terminate your connections.

comment:5 in reply to:  4 Changed 5 years ago by yawning

Replying to dcf:

It is not specified that that they do. I might argue that if they keep using the network, it's for a good reason. Like maybe a PT's purpose is to generate cover traffic so an observer can't see when your tor is idle. Flash proxy will break its existing WebSocket connections, but won't close its external listeners, for example.

The confusion is because we're punning on different interpretations of "DisableNetwork". One is "drop the firewall and let nothing get out": in that case, tor should just kill its client PTs. The other is "make tor close its connections, knowing that the (PT, socket, OS) abstractions on which they are built may continue to operate as usual": in that case, tor should just close its PT connections.

For my 2 JPY, ideally we would provide both options, with a new config entry for the latter definition of "DisableNetwork". I'm in the camp that the first definition follows the principle of least surprise.

If the use case for DisableNetwork is saving battery on a mobile, then I think you want to kill the PT processes. Same is the use case is to be a poor man's firewall.

Speaking as an implementer, I would probably rather have my program killed and restarted.

This is what I am planning to do when I go and address this problem, since most of the bits are in place for this already (to handle shutting down/starting up pts on config change).

The same issue exists with SocksProxy. Tor can't enforce the proxy not to keep using the network after it has disconnected. Same with external-mode proxies: there's nothing you can do to them besides terminate your connections.

Indeed.

comment:6 Changed 5 years ago by yawning

Status: newneeds_review

So this was a lot easier than I thought it would be, since the way tor handles pt configuration is to mark all running instances for termination, and selectively re-enable them based on the new config.

https://github.com/yawning/tor/tree/bug13213

With my branch, when DisableNetwork is set, PTs will not be launched, and any existing client/server plugins will be terminated. It additionally does not affect the validation of said config directives. Tested with kill -HUP, vi, and ps.

comment:7 Changed 5 years ago by asn

Logic looks good to me, but I didn't test it.

Maybe you want to use net_is_disabled() instead of options->DisableNetwork, like it's done for pt_configure_remaining_proxies() in the next code block?

comment:8 Changed 5 years ago by yawning

Hmm, I think what's done here is correct, at least for the issue at hand, and using net_is_disabled() will break things.

The difference between what I'm doing and using the call is that the call checks hibernation status, but without additional code to relaunch pts that happen to get torn down when coming out of hibernation (and to tear down pts when entering hibernation), it's not safe to look at hibernation state here.

What the code below does is delay doing the pt config process (under the assumption that you can just
Now, when entering hibernation, PTs *SHOULD* be torn down, and *SHOULD* be relaunched when exiting, but that's requires more invasive code changes, and (IMO) is an orthogonal issue.

comment:9 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

looks plausible to me. Merging!

Note: See TracTickets for help on using tickets.