Re: [tor-bugs] #25423 [Core Tor/Stem]: Treat 'ExitRelay 0' as a reject-all policy

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#25423: Treat 'ExitRelay 0' as a reject-all policy
---------------------------+--------------------------
 Reporter:  atagar         |          Owner:  dmr
     Type:  defect         |         Status:  assigned
 Priority:  Medium         |      Milestone:
Component:  Core Tor/Stem  |        Version:
 Severity:  Normal         |     Resolution:
 Keywords:                 |  Actual Points:
Parent ID:                 |         Points:
 Reviewer:                 |        Sponsor:
---------------------------+--------------------------

Comment (by dmr):

 I've pushed a few commits for the issue here:
 `git@xxxxxxxxxx: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. 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, 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 [[https://gitweb.torproject.org/torspec.git/tree
 /control-
 spec.txt?id=d4a64fbf5aaba383638d9f3c70bd2951f8c5ad89#n2539|control 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 [[https://gitweb.torproject.org/torspec.git/tree
 /control-
 spec.txt?id=d4a64fbf5aaba383638d9f3c70bd2951f8c5ad89#n2626|control 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 - 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.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25423#comment:6>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs