Treat 'ExitRelay 0' as a reject-all policy

added component::archived/stem owner::dmr priority::medium resolution::fixed reviewer::atagar severity::normal status::closed type::defect labels

atagar, do you think Tor should prepend "reject :" to the ExitPolicy when ExitRelay is 0? I thought we did that, but if we don't, that's probably a bug in Tor.

Is stem looking at a 'getconf exitpolicy'?

It seems like it should instead be doing a 'getinfo exitpolicy', which should tell it what Tor is actually using as its exit policy.

Trac:
Username: franklin
Cc: N/A to franklin

Replying to arma:

Is stem looking at a 'getconf exitpolicy'?

It seems like it should instead be doing a 'getinfo exitpolicy', which should tell it what Tor is actually using as its exit policy.

I've looked into this a bit so far; stem is definitely using GETCONF ExitPolicy: https://gitweb.torproject.org/stem.git/tree/stem/control.py?id=72700087b94f2889b5b364738a1178c324862ba5#n1292

        for policy_line in self.get_conf('ExitPolicy', multiple = True):
          policy += policy_line.split(',')

On a local test relay (standard Debian9 tor stable) with torrc...

# configuration for local relay, for exploratory testing
DataDirectory <redacted>
SocksPort 1112
ORPort 1113
ControlPort 1111
HashedControlPassword <redacted>
ExitRelay 0
PublishServerDescriptor 0
AssumeReachable 1
DownloadExtraInfo 1
Log notice stdout
Log notice file <redacted>/tor_log

... str(controller.get_exit_policy()) returns:

'reject 0.0.0.0/8:*, reject 169.254.0.0/16:*, reject 127.0.0.0/8:*, reject 192.168.0.0/16:*, reject 10.0.0.0/8:*, reject 172.16.0.0/12:*, reject 107.5.239.102:*, reject *:25, reject *:119, reject *:135-139, reject *:445, reject *:563, reject *:1214, reject *:4661-4666, reject *:6346-6429, reject *:6699, reject *:6881-6999, accept *:*'

On a standard Debian9 tor client (i.e. no relaying), str(controller.get_exit_policy()) returns the same.

Obviously these are both incorrect.

I'm looking into switching get_exit_policy() to use controller.get_info('exit-policy/full') instead. I think that will cover a lot of the more-complicated get_conf / get_info logic (e.g. get_info('exit-policy/default')) that currently exists in get_exit_policy(), but I'm checking fairly conservatively.

Trac:
Owner: atagar to dmr
Status: new to assigned

I've pushed a few commits for the issue here: git@github.com:dmr-x/stem.git branch: 25423-get-exit-policy revrange: 2018f3fd2642f43aeab8e1e0d2142a6a4905e651..0f797ee5de4a7e25d307203d65a044ca1b9e9534

web link: https://github.com/dmr-x/stem/tree/25423-get-exit-policy

These commits should address the issue almost entirely.

However, in prepping these to submit, I saw that atagar recently addressed #25739 (closed). The changes are very similar (cool!) but overlap imperfectly (if they were identical, I'd be a bit freaked out!). I rebased my changes to //right before// the commits for #25739 (closed), but I'd need to spend some time to merge the changes together. (I can do that - it'll just likely not be for a week or so.)

I'd like to provide some info in this Trac ticket. Namely: I'd like to go over the changes I pushed, as well as some rationale and what I think might remain with the issue after changes are merged.

For reference, I tested against Tor version 0.3.2.10 (git-0edaa32732ec8930).

==== 1. Failure for GETINFO exit-policy/full to return info. I noticed in my testing that GETINFO exit-policy/full does not always return info. In that case, I've always seen this:

>>> GETINFO exit-policy/full
551 router_get_my_routerinfo returned NULL

I've noticed two circumstances in which the exit-policy is unqueryable:

tor is configured as a relay, and it //has not yet learned its address// (i.e. GETINFO address also fails)
tor is configured as a client

The former is transient and doesn't typically last long, but //could// for instance last a while if the relay doesn't have network access. The latter is persistent, i.e. the client-only configuration ALWAYS returns this:

>>> GETINFO exit-policy/full
551 router_get_my_routerinfo returned NULL

Controller.get_exit_policy() has the default parameter. Without specifying a default, get_exit_policy() will raise an exception in these cases.

So Controller.get_exit_policy(None) is handy to avoid an exception, as I've seen nyx and other parts of stem do. But since get_exit_policy() previously would ~always return something, and is now changing to a form that is less stable - and for clients always fails - consumers of this updated API have to be weary. As mentioned, I checked nyx's code - luckily it always uses default=None, so that shouldn't cause any problems.

Nonetheless, we can't be sure who all is using stem - maybe we need to note this possibility in the changelog?

Or maybe we should change the functionality, so that consumers of controller.get_exit_policy() don't have this problem / have to wait? We could potentially do a get_conf('ORPort') and return None proactively if tor is running as a client (i.e. we don't have an ORPort configured).

==== 2. Potential tor bug: waiting for address in non-exit relays Of particular note: The behavior of returning NULL / waiting for knowing an address happens even in non-exit relays, i.e. even when the exit policy should be fully known as reject *:* beforehand by the config alone.

atagar: Perhaps we should file this as a bug in tor?

==== 3. Fixed cache-invalidation bugs Two commits I pushed are related to existing cache-invalidation bugs, but I ran into them when testing for cache invalidation related to the changes. The crux was: exitpolicy instead of exit_policy cache key, and exitpolicy compared with (non-lower()ed) ExitPolicy. Since there weren't any tests (unit or integ) for the cache invalidation here, I added regression tests.

I ran into a further bug related to cache invalidation and still have it to file, but it's largely orthogonal, and it's not as straightforward of a fix, so I didn't just go fix it.

==== 4. Multiple configuration changes could cause our cache to be invalid As alluded to above, I had to edit the cache invalidation anyway for this change.

All of these torrc options, if changed, could invalidate our cache: (code snippet)

CONFIG_OPTIONS_AFFECTING_EXIT_POLICY = (
  'ExitRelay',
  'ExitPolicy',
  'ExitPolicyRejectPrivate',
  'ExitPolicyRejectLocalInterfaces',
  'IPv6Exit',
)

See the corresponding commit.

==== 5. Additional event that can invalidate our cache If our relay's address changes, then our ExitPolicy may change (due to the default rule of rejecting its own address).

In looking at the [spec], it looks like this can be done by registering for a StatusEvent and handling status_type==StatusType.SERVER with action EXTERNAL_ADDRESS.

Changing address is a bit unlikely to happen, so I'll file that as a separate ticket. But it is an outstanding "bug", if you will. I didn't have time to get to addressing it yet. (Pun not //originally// intended, but now I kinda like it.)

==== 6. Further potential to change exit policy In reviewing the tor man page, I saw this torrc option:

ServerDNSTestAddresses hostname,hostname,...
   When we're detecting DNS hijacking, make sure that these valid addresses aren't getting redirected. If
   they are, then our DNS is completely useless, and we'll reset our exit policy to "reject *:*". This
   option only affects name lookups that your server does on behalf of clients. (Default:
   "www.google.com, www.mit.edu, www.yahoo.com, www.slashdot.org")

To note here is that tor may change our exit policy underneath us. While again this should be rare, I don't know if there's anything stem can listen to, in order to have a correct exit policy if this happens.

I looked at bit at the [spec] - perhaps tor would emit this event: SERVER_STATUS / DNS_USELESS action?

If it isn't... perhaps we need another event in the control spec for any time tor's exit policy changes? (That would also cover the address-changing case)

==== 7. Test function compare_exit_policy() is probably unnecessary (kept in) I cannot think of a configuration case where we shouldn't be able to predict whether the exit policy has the relay's address in it, or not. I left this in the test code (refactored into a function) because I was being conservative, but I wanted to call it out since it may now be unnecessary. It could be a remnant from the previous behavior that built the exit policy based on get_info('address', None) and other pieces of data, so if tor didn't know its address stem wouldn't add that into the exit policy.

atagar, do you think we can remove this?

==== 8. Probably unnecessary use of with self._msg_lock (removed) I spent a deal of time working through why the with self._msg_lock: line existed in get_exit_policy() and believe that it was because the old implementation sent multiple messages and that we'd ideally want them to represent as atomic of a state as possible, so we'd lock-out others from sending messages during that time.

In the new implementation, we only send a GETINFO message and block on its response. I don't think we need the line for the lock anymore, since that comes as part of msg().

I removed it in my commits, but atagar I see you still have it in the commits you pushed for #25739 (closed) - I think it's probably unnecessary but it would be great to get your opinion on this, too.

==== 9. Potentially deprecate ExitPolicy.get_config_policy() (no change made) In stem, Controller.get_exit_policy() was the only consumer of exit_policy.get_config_policy() (and now there isn't any). Perhaps we should deprecate the latter? nyx doesn't use it either. I'm not sure an external consumer of our API would need/want to use this.

atagar: I don't think the conflicting branch is "ready" to merge, per se, but I'm switching ticket status so you might be able to review the above. Thanks!

Trac:
Reviewer: N/A to atagar
Status: assigned to needs_review
Cc: franklin to franklin, dmr

I ran into a further bug related to cache invalidation and still have it to file, but it's largely orthogonal, and it's not as straightforward of a fix, so I didn't just go fix it. Filed; see #25821 (closed).

Replying to dmr:

==== 4. Multiple configuration changes could cause our cache to be invalid As alluded to above, I had to edit the cache invalidation anyway for this change.

All of these torrc options, if changed, could invalidate our cache: (code snippet) {{{ CONFIG_OPTIONS_AFFECTING_EXIT_POLICY = ( 'ExitRelay', 'ExitPolicy', 'ExitPolicyRejectPrivate', 'ExitPolicyRejectLocalInterfaces', 'IPv6Exit', ) }}}

See the corresponding commit.

The ExitPolicyRejectPrivate and ExitPolicyRejectLocalInterfaces also depend on:

Address
the addresses in any published or local *Port option
OutboundBindAddress*
possibly other options, which should be documented in the man page or the relevant function comments in tor. Maybe you'll have to read the code.

Trying to find all the options that can change an exit policy could be difficult. tor doesn't guarantee the options that affect the exit policy, and these options have changed in previous versions. (Fortunately, those versions are now obsolete.)

Why don't you just invalidate the entire config cache any time you see a CONF_CHANGED event?

Obviously, it's possible to do better but getting config is fast and it won't change very often in general.

Failure for GETINFO exit-policy/full to return info.

Crap. Great catch, dmr! This is a bug within tor. Mind filing a ticket for it?

In the meantime I wonder if we should resurrect our old exit policy fetching code as a fallback...

Fixed cache-invalidation bugs

Good catch! Fix pushed.

Multiple configuration changes could cause our cache to be invalid

Another nice catch. Think I agree with meejah here. Constructing another ExitPolicy instance is cheap so we can be broader with invalidation. Change pushed so we now invalidate it with any config change.

Additional event that can invalidate our cache

Good idea about listening for StatusEvent events. This could also let us drop our CACHE_ADDRESS_FOR constant.

Probably unnecessary use of with self._msg_lock (removed)

Oh! Good point. Removed.

Potentially deprecate ExitPolicy.get_config_policy() (no change made)

Good idea. Now that we're no longer attempting to make sense of the torrc this function will become less and less reliable over time. Function deprecated.

How does this look?

https://gitweb.torproject.org/stem.git/commit/?id=c305b7f

Fixed the 'ProtocolError when disconnected' issue you caught...

https://gitweb.torproject.org/stem.git/commit/?id=f61e913

... and filed #25842 (moved) for getting that fixed in tor. Think the last thing left for this ticket is invalidating our exit policy and address caches when our address changes.

Oh oops, I misread an earlier comment that we had a STATUS_SERVER to tell us when our address changed. Seems that's not the case. I'll probably file another ticket in a bit to request that. In the meantime I'll give this some more thought (maybe we'll just add a TTL, like what we do for cached addresses).

Oops about the oops. STATUS_SERVER does have what we want but it's EXTERNAL_ADDRESS rather than DNS_USELESS. Pushed a change to invalidate the cache when our address changes...

https://gitweb.torproject.org/stem.git/commit/?id=331406c

Feel free to file another ticket or reopen if I missed something!

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

[comment:6 Quoting dmr]:

==== 1. Failure for GETINFO exit-policy/full to return info. [...]

I've noticed two circumstances in which the exit-policy is unqueryable:

[...]

tor is configured as a client

The former is [...] The latter is persistent, i.e. the client-only configuration ALWAYS returns this: {{{

GETINFO exit-policy/full 551 router_get_my_routerinfo returned NULL }}}

Controller.get_exit_policy() has the default parameter. Without specifying a default, get_exit_policy() will raise an exception in these cases.

Tickets filed; handling these scenarios is covered in:

#25852 (moved) - tor and control spec
#25853 (closed) - stem behavior

Replying to atagar:

Oops about the oops. STATUS_SERVER does have what we want but it's EXTERNAL_ADDRESS rather than DNS_USELESS. Pushed a change to invalidate the cache when our address changes...

https://gitweb.torproject.org/stem.git/commit/?id=331406c

Feel free to file another ticket or reopen if I missed something!

Was discussing this ticket with arma a bit on IRC, and that reconfirmed the need to handle DNS_USELESS, too. Opened another ticket for this: #26129 (closed)

closed

mentioned in issue #25821 (closed)

mentioned in issue #25842 (moved)

mentioned in issue #25852 (moved)

mentioned in issue #25853 (closed)

mentioned in issue #26129 (closed)

mentioned in issue tpo/core/tor#25842 (closed)

mentioned in issue tpo/core/tor#25852 (closed)

Treat 'ExitRelay 0' as a reject-all policy

Child items ...

Activity