Opened 6 years ago

Closed 2 years ago

#11264 closed defect (duplicate)

Relay has Exit flag but short policy says reject *?

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: nickm-patch, needs-proposal, tor-dirauth
Cc: el13635@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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?

Child Tickets

Attachments (1)

fix-11264.patch (784 bytes) - added by epilys 5 years ago.
switch result string to NULL

Download all attachments as: .zip

Change History (34)

comment:1 Changed 6 years ago by arma

(Set to 0.2.5 so it will get properly triaged. Is there a component for "I haven't decided what milestone this should be", which won't get lost in a pile of tickets that nobody looks at?)

comment:2 Changed 6 years ago by arma

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.

comment:3 Changed 6 years ago by nickm

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.

comment:4 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

Putting this in 0.2.6 since it's authority-side.

comment:5 Changed 5 years ago by arma

Keywords: tor-auth easy added

comment:6 Changed 5 years ago by arma

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?

comment:7 in reply to:  6 Changed 5 years ago by karsten

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.

comment:8 Changed 5 years ago by epilys

Cc: el13635@… added
Status: newneeds_review

policy_summarize() should return a NULL char * 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

Changed 5 years ago by epilys

Attachment: fix-11264.patch added

switch result string to NULL

comment:9 Changed 5 years ago by nickm

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.

comment:10 Changed 5 years ago by nickm

Status: needs_reviewnew

comment:11 Changed 5 years ago by nickm

Keywords: 026-triaged-1 added; easy removed
Status: newneeds_review

See branch "bug11264" in my public repository for a fix for the Exit flag issue. Needs review.

comment:12 Changed 5 years ago by nickm

Keywords: nickm-patch added

Apply a nickm-patch keyword to tickets in needs_review in 0.2.6 where I wrote the patch, so I know which ones I can('t) review myself.

comment:13 Changed 5 years ago by teor

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.

[0]: https://gitweb.torproject.org/torspec.git/blob/HEAD:/dir-spec.txt

comment:14 Changed 5 years ago by teor

#11624 talks about this issue happening maliciously. Should we try to resolve both at once?

comment:15 Changed 5 years ago by teor

TL;DR: perhaps we should fix policy_summarize() rather than the Exit flag (but I don't think we can do that without another consensus method, can we?)

comment:16 Changed 5 years ago by nickm

I think you're probably right; let's come up with a description of exactly what we'd like policy_summarize() and Exit to do, write a short proposal describing the exact behavior, and then implement it in a new consensus method.

comment:17 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

comment:18 Changed 5 years ago by nickm

Keywords: needs-proposal added

comment:19 Changed 5 years ago by teor

In the meantime, where can we (more clearly) document the difference between policy_summarize() and Exit? It's obviously causing some confusion.

What do you think of this draft?

  • policy_summarize() lists exit ports that are allowed to the whole internet; or exit ports that are blocked to one or more internet addresses (whichever list is shorter).
  • Exit is applied to routers that allow exits to at least two of the HTTP, HTTPS, and IRC ports; and allow exits to at least 1/256 of the IPv4 internet.

To check:

  • does policy_summarize() work the same for IPv4 and IPv6?
  • how does "Exit" work for IPv6?

Next Steps:

  • Put my white hat on and determine a set of pathological cases
  • Create test cases for these cases (?)
  • Check the current consensus for cases with: Exit & Reject 1-65535; No Exit & Accept 1-65535 (?)

comment:20 Changed 5 years ago by teor

Milestone: Tor: 0.2.6.x-finalTor: 0.2.7.x-final

The core of this issue appears to be that the Exit flag code is optimistic (just needs a /8 and 2 ports), but the microdescriptor exit policy summary code is pessimistic (needs the entire internet).

We need a proposal to fix the microdescriptor exit policy summary code (and a new consensus method), but the Exit flag could be fixed to be more pessimistic straight away, as it is assigned by authorities.

Perhaps it is easiest to just make one depend on the other?
(A quick fix would be to summarise the exit policy, then use that as input to the Exit flag determination. This would also require a documentation fix.)

comment:21 Changed 5 years ago by teor

Keywords: tor-auth, 026-triaged-1, nickm-patch needs-proposaltor-auth 026-triaged-1 nickm-patch needs-proposal

comment:22 Changed 5 years ago by teor

See #13839 for the converse issue, where the policy is accept * but no Exit flag is assigned.

comment:23 Changed 5 years ago by nickm

Keywords: 027-triaged-1-out added

Marking triaged-out items from first round of 0.2.7 triage.

comment:24 Changed 5 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.???

Move *most* 0.2.7-triaged-1-out needs_revision items into 0.2.???. Keep a few based on my sense of the sensible.

comment:25 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:26 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:27 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:28 Changed 2 years ago by nickm

Keywords: 027-triaged-in added

comment:29 Changed 2 years ago by nickm

Keywords: 027-triaged-in removed

comment:30 Changed 2 years ago by nickm

Keywords: 027-triaged-1-out removed

comment:31 Changed 2 years ago by nickm

Keywords: 026-triaged-1 removed

comment:32 Changed 2 years ago by dgoulet

Keywords: tor-dirauth added; tor-auth removed

Turns out that tor-auth is for directory authority so make it clearer with tor-dirauth

comment:33 Changed 2 years ago by nickm

Resolution: duplicate
Severity: Normal
Status: needs_revisionclosed

There is a modern treatment of this as #21413.

Note: See TracTickets for help on using tickets.