policies_parse_exit_policy_internal should block all IPv4 and IPv6 local addresses

changed milestone to %Tor: 0.2.8.x-final in legacy/trac

added 027-backport in Legacy / Trac TorCoreTeam201512 in Legacy / Trac component::core tor/tor in Legacy / Trac milestone::Tor: 0.2.8.x-final in Legacy / Trac priority::high in Legacy / Trac resolution::fixed in Legacy / Trac security in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac type::defect in Legacy / Trac version::tor unspecified in Legacy / Trac labels

Trac:
Keywords: TorCoreTeam201509 security deleted, TorCoreTeam201509 security 026-backport added
Milestone: N/A to Tor: 0.2.7.x-final

Anything we do here will always have edge cases, like (multihomed) NAT, or NAT without an autodetected IP address.

That said, I think I can fix this by:

refactoring get_interface_address6 as a wrapper for a new get_interface_address6_list, which produces a smartlist of local addresses. (Similarly, create get_interface_address_list, a wrapper which produces a list of IPv4 addresses.)
blocking connections to all IPv4 addresses from get_interface_address_list, as long as rejectprivate and local_address are non-zero.
blocking connections to all IPv6 addresses from get_interface_address6_list, as long as rejectprivate and local_address and ipv6_exit are non-zero. (Otherwise, we're blocking all IPv6 addresses anyway.)
Creating or updating unit tests for get_interface_address6_list, get_interface_address_list, and policies_parse_exit_policy_internal.

Trac:
Milestone: Tor: 0.2.7.x-final to N/A
Keywords: TorCoreTeam201509 security 026-backport deleted, TorCoreTeam201509 security added

Didn't mean to revert those.

Trac:
Keywords: TorCoreTeam201509 security deleted, TorCoreTeam201509 security 026-backport added
Milestone: N/A to Tor: 0.2.7.x-final

We should also update the man page and torrcs with this change, as it changes the meaning of "private" to mean "all IPv4 and IPv6 addresses on all server interfaces", rather than "a single IPv4 address on one of the server's interfaces". (It's ambiguous in the man page at the moment.)

Further notes:

This is a patch on 42b8fb5a (11 Nov 2007), released in 0.2.0.11-alpha.

This fix will automatically benefit from changes that find more interfaces/addresses, perhaps legacy/trac#12377 (moved) will do this for some platforms.

We should log an info-level (notice?) message for each address blocked Internal addresses are blocked anyway by reject private *:*, so this patch doesn't need to block them.

This change will include all addresses in non-internal blocks in the publicly available exit policy, but these addresses are typically globally visible on the Internet anyway. I believe the security benefits outweigh the small risk of leaking public server addresses from unusual configurations (and operators can always set ExitPolicyRejectPrivate 0 and block only the private and server addresses they want to block).

Trac:
Version: Tor: 0.2.7.2-alpha to Tor: unspecified

See my branch bug17027-reject-private-all-interfaces-squashed on https://github.com/teor2345/tor.git

Commits:

Add get_interface_address[6]_list by refactoring get_interface_address6 (with unit tests)
ExitPolicyRejectInternal rejects a relay's published IPv6 address (if we're an IPv6 Exit), and publicly routable IPv4 & IPv6 interface addresses (with unit tests)
Log an info-level message for each local IP address blocked this way

The unit tests pass on OS X and Linux

This branch may require manual changes to merge correctly with legacy/trac#16069 (moved): this branch adds arguments to functions that legacy/trac#16069 (moved) adds calls to.

Trac:
Status: new to needs_review

Pushed a fixup commit that makes sure we don't reject addresses that are both the relay's published address, and the address of an interface. (These would have been caught by exit_policy_remove_redundancies, but it's best to avoid them in the first place.)

Added two fixups that update the documentation and torrcs

Looks good!

Needs a changes file.
I'm thinking this doesn't run us into trouble with bug legacy/trac#12497 (moved). Somebody should check my logic, though.
get_interface_address6_list() can't return NULL, but its callers all check whether it does.

Replying to nickm:

Looks good!

Needs a changes file.

It's there as changes/bug17027-reject-private-all-interfaces

I'm thinking this doesn't run us into trouble with bug legacy/trac#12497 (moved). Somebody should check my logic, though.

This doesn't change the definition of private:*, instead, it appends explicit IP-based reject items to the ExitPolicy when ExitPolicyRejectPrivate is 1. The existing code adds a reject for the configured public IPv4 address, this new code does it for the configured public IPv6 address (if any), and any other public IPv4 or IPv6 addresses found on any interfaces.

get_interface_address6_list() can't return NULL, but its callers all check whether it does.

Oops, fixed and squashed in bug17027-reject-private-all-interfaces-v2.

Please use my bug16069-bug17027 branch if you want to merge both legacy/trac#16069 (moved) and legacy/trac#17027 (moved). It resolves some merge conflicts and function argument changes, and includes the latest changes in both branches.

Merged

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

Reopened for possible backport to 026

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

Trac:
Milestone: Tor: 0.2.7.x-final to Tor: 0.2.6.x-final

IMO this is really potentially backportable to 0.2.6 as a security improvement.

Trac:
Sponsor: N/A to N/A

I agree, let's backport to 0.2.6, if the required function(s) or data structures exist to support each address discovery method. But can we make the following changes first?

In addition to blocking:

the configured or autodiscovered IPv4 address (Address or resolve_my_address()),
the configured IPv6 address (first IPv6 ORPort entry),
the publicly routable IPv4 or IPv6 address(es) of every interface on the server, if available.

We could also block the following configured addresses by looking at OutboundBindAddressIPv4_/OutboundBindAddressIPv6_ and get_configured_ports():

OutboundBindAddress
ControlPort / ControlListenAddress
SOCKSPort / SOCKSListenAddress
TransPort / TransListenAddress
NATDPort / NATDListenAddress
DNSPort / DNSListenAddress
ORPort / ORListenAddress (IPv4 entries or subsequent IPv6 entries)
DirPort / DirListenAddress

Ideally, we'd do this out of two smartlists of unique IPv4 and IPv6 addresses, to avoid rejecting the same address multiple times. Duplicates will be removed by exit_policy_remove_redundancies() in any case.

Trac:
Status: reopened to needs_revision

See https://lists.torproject.org/pipermail/tor-relays/2015-October/007928.html for a discussion that sparked the additional changes to block configured addresses.

We might like to split blocking the standard list of private addresses and (each method of) blocking local relay addresses into its own function. That would make legacy/trac#17183 (moved) easier.

Trac:
Severity: N/A to Normal
Keywords: TorCoreTeam201509 security 026-backport deleted, security, 026-backport, 027-backport, TorCoreTeam201512 added

I'm working on this branch rebased on 0.2.7.4-rc in bug17027-reject-private-027.

Forward-porting to 0.2.8 should simply require a merge.

To backport this to 0.2.6:

we'll also need (some of) the documentation from ipv6-exitpolicy-docs;
we might want to exclude the changers to the torrc files, to avoid the risk of overwriting operators' torrc files.

I'm sorry the commits here are a bit of a mess.

Please see my commit bug17027-reject-private-027-v4 on https://github.com/teor2345/tor.git

It's based on tor 0.2.7.4-rc, and appears to merge cleanly into 0.2.8 / master.

ExitPolicyRejectPrivate now blocks configured outbound bind addresses and port addresses (such as ORPort and DirPort).

I'll create another bug for the backport to 0.2.6. (It's in bug17027-reject-private-026-v4.)

Trac:
Status: needs_revision to needs_review
Keywords: TorCoreTeam201512 deleted, TorCoreTeam201511 added

The 0.2.6 backport is in legacy/trac#17610 (moved).

nickm
teor: other suggestion: instead of adding a bunch of tor_addr_t arguments to policies_parse_exit_policy() etc, why not have them take a const smartlist of "local" addresses?

Replying to teor:

{{{ nickm teor: other suggestion: instead of adding a bunch of tor_addr_t arguments to policies_parse_exit_policy() etc, why not have them take a const smartlist of "local" addresses? }}}

Please see my branch bug17027-reject-private-027-v5 which implements this suggested change. It also unifies the rejection list handling code, and does some extra error checks. The unit tests have also been updated.

(It turns out that when you get everything as a list, it all works very neatly.)

hmm. I just went to try to squash and merge this to master, but the fixup commits aren't in the standard form and I can't figure out how to squash them the way that you intended. The commits they refer to no longer exist with those identifiers. Can you give me a quick squasher's guide on this?

I think this is also viable for a 0.2.7 merge once it's seen more testing in master.

Replying to nickm:

hmm. I just went to try to squash and merge this to master, but the fixup commits aren't in the standard form and I can't figure out how to squash them the way that you intended. The commits they refer to no longer exist with those identifiers. Can you give me a quick squasher's guide on this?

Yes, you're right, the fixups are messy because I refactored the argument lists last based on changes in earlier commits.

Please see bug17027-reject-private-027-v6 based on maint-0.2.7.

I squashed the fixups, but left the refactoring and documentation separate. Please feel free to squash further.

I think this is also viable for a 0.2.7 merge once it's seen more testing in master.

I agree.

This was merged into master in legacy/trac#17183 (moved).

It needs more testing in master before a merge to 0.2.7. (It's not suitable for a merge to 0.2.6 as the related functions have changed too much since then, see legacy/trac#17610 (moved) for a limited reimplementation of this patch based on 0.2.6 functionality.)

Trac:
Keywords: 026-backport deleted, N/A added

This branch needed a fixup from legacy/trac#17183 (moved), so I cherry-picked it onto the end of bug17027-reject-private-027-v6.

policies_parse_exit_policy_reject_private Unit Tests

The unit tests for policies_parse_exit_policy_reject_private assumed too much about the interface addresses on the local system. This meant that tests failed on some machines.

I modified the unit tests to assume less, and used a mocked version of get_interface_address6_list to provide a range of addresses for testing.

Trac:
Milestone: Tor: 0.2.6.x-final to Tor: 0.2.7.x-final

(This code has been merged into master, and is being tested there before backport to 0.2.7.)

(Give this code another month's testing in master.)

Trac:
Keywords: TorCoreTeam201511 deleted, TorCoreTeam201512 added

This code doesn't have any known issues, but it's diverged from the code being tested in master.

bug17027-reject-private-027-v6 doesn't include the following commits:

From getinfo-private-exitpolicy-v4:

6913bdfc - Split out policy_dump_to_string to use it in getinfo_helper_policies.
22f82361ab4a - Create helper functions for adding ipv4h and tor_addr_t* to a smartlist. They would need to be split, as they include changes to getinfo_helper_policies (legacy/trac#17183 (moved)) as well as policies_parse_exit_policy* changes.

Once (parts of) those commits are applied, we need to cherry-pick from fix-policies-memory-v2:

d27f3ec8302e - Fix use-after-free of stack memory

6913bdfc and 22f82361ab4a aren't required, they're both refactoring changes. d27f3ec8302e isn't required: the issue it fixes was introduced in 22f82361ab4a.

Do we want to add the refactoring from master to 0.2.7? If so, how can I split the commits 6913bdfc and 22f82361ab4a so that they don't cause merge headaches for nickm?

Trac:
Status: needs_review to needs_information

I don't think we need to backport refactoring.

But we might need to fix this code to handle non-printable addresses better, see legacy/trac#17762 (moved).

Trac:
Status: needs_information to needs_revision

Please see my branch bug17027-reject-private-027-v7, which includes the fix in legacy/trac#17762 (moved). It rebases on the latest maint-0.2.7, and fixes a few spacing issues picked up by make check-spaces.

Trac:
Status: needs_revision to needs_review

My current vote here is "this is a feature; no backport." Are the remaining issues in 0.2.7 worse than I think? I'm trying to balance those issues against the risk that comes with any backport of significant size or complexity (and this looks like one to me).

No backport, the current code in 0.2.7 covers the common cases already. And 0.2.6 is getting pretty old. And exit relay operators can fix it themselves by rejecting their own address(es) in their exit policy.