Relay has Exit flag but short policy says reject *?
https://atlas.torproject.org/#details/65C35C03571307D7546D6978605A6B11B473F6EE
its short exit policy is reject :
but check out its actual exit policy
and it has the Exit flag
This seems like a contradiction, yes?
Activity
changed milestone to %Tor: unspecified
Arlo points me to https://github.com/arlolra/check/issues/21#issuecomment-29141247 which is where he encountered this relay producing false positives for Torcheck.
Well, the Exit flag and the short policy are both computed based on the original exit policy. So the authorities are sure doing something inconsistent.
Changing how short policy generation works would require a new consensus method, methinks. Changing the it works when we vote for the exit flag wouldn't.
-
Seems like the simple approach is to find the place in dirserv.c that assigns the exit flag, and find the place in policy_summarize() that decides that such relays summarize as reject :, and not give the Exit flag if the relay summarizes as reject :.
Maybe this is a fine introductory Tor ticket for TWN readers?
Replying to arma:
Maybe this is a fine introductory Tor ticket for TWN readers?
How about the following paragraph for TWN? (Feel free to tweak!)
Tor relays define an exit policy in the format "reject 0.0.0.0/8:*" that says which addresses and ports are accepted or rejected for outgoing connections. The directory authorities summarize this exit policy into a list of rejected or accepted ports, like "reject 1-65535", and they assign the "Exit" flag if two ports out of 80, 443, 6667 are permitted for "most" addresses. Apparently, there are edge cases when the summary is "reject 1-65535" but the relay still gets the "Exit" flag, which seems inconsistent. An easy fix would be to not assign the "Exit" flag in this specific case. Roger sketched out the relevant functions to look at in the ticket (#12264). Ideally, this fix comes with a short analysis what the edge cases are and with a specification update. This is probably a one-line patch, the difficulty is just in finding out which line that is.
policy_summarize()should return a NULLchar *if no exits are allowed, yet in the 1-65535 port case it returns the string "reject 1-65535"I think changing the line 1402 on policies.c should be enough to fix this, from
result = tor_strdup("reject 1-65535");to
result = NULL;Pertinent patch is attached
Trac:
Username: epilys
Cc: N/A to el13635@mail.ntua.gr
Status: new to needs_review-
Trac:
Username: epilys -
Actually, in this case the C documentation was wrong: according to the spec,
As an exception to this rule an allow-all policy is represented as "accept 1-65535" instead of "reject " and a reject-all policy is similarly given as "reject 1-65535".So, I'm going to fix the comment in the C file (commit 0fc2d0edce72) and put this ticket back into "new" for a fix of the Exit flag issue.
Nick, I worry this patch changes the definition of "Exit" to be more complex (adding more edge cases, not decreasing them), without changing the documentation.
The directory specification defines the "Exit" flag as: 0
1614 "Exit" if the router is more useful for building 1615 general-purpose exit circuits than for relay circuits.This patch substantially changes the specification of the Exit flag from:
1845 "Exit" -- A router is called an 'Exit' iff it allows exits to at 1846 least two of the ports 80, 443, and 6667 and allows exits to at 1847 least one /8 address space.To:
A router is called an 'Exit' iff it allows exits to at least two of the ports 80, 443, and 6667 and:
- where the accept summary is shorter: there is at least one port that allows exits to the entire IPv4 or IPv6 address space; or,
- where the reject summary is shorter: there is at least one port for which exits are not rejected to any non-private addresses (alternately, it is not the case that: every port is rejected to at least one non-private address); and allows exits to at least one /8 address space. (The /8 is not required in the "accept" case, because "entire address space" implies "at least one /8".) (I think - it's hard to tell exactly how policy_summarize() and exit_policy_is_general_exit() interact.)
This change in Exit flag definition is caused by the patch depending on not only exit_policy_is_general_exit(), but the exact implementation of policy_summarize(), including the length comparison of accept and reject summaries, and the implementation of policy_summary_add_item():
Add a single exit policy item to our summary: If it is an accept ignore it unless it is for all IP addresses ("*"), i.e. it's prefixlen/maskbits is 0, else call policy_summary_accept(). If it's a reject ignore it if it is about one of the private networks, else call policy_summary_reject().This leaves a large number of corner cases where:
- accept policies do not allow exiting to the entire address space; or
- reject policies block all ports to parts of the address space (or, more precisely, block each port to at least one address); or
- minor changes to policies flip the summary from accept to reject, causing instability;
- and potentially other cases I haven't picked up on yet.
In short, these cases are where policy_is_reject_star() is not the same as policy_summarize() == "reject 1-65535".
I'd feel much more comfortable with some unit tests for these cases, to make sure we don't take away too many Exit flags with this change.
In particular, I'd like to compare the results of:
- !exit_policy_is_general_exit()
- policy_summarize() == "reject 1-65535"
- policy_is_reject_star() Ideally, we could do this for the entire consensus, and see how many Exits would be affected.
And, of course, there's an awful lot of doco that would need to change with it, both in code comments and the dirspec.
Sorry to rain on your patch.
#11624 (moved) talks about this issue happening maliciously. Should we try to resolve both at once?