Opened 3 years ago

Closed 3 years ago

#17027 closed defect (fixed)

policies_parse_exit_policy_internal should block all IPv4 and IPv6 local addresses

Reported by: teor Owned by:
Priority: High Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: TorCoreTeam201512, security, 027-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Currently it just handles a single IPv4 address, allowing IPv6 exits to be connected to on their IPv6 address, or multihomed IPv4 exits to be connected to on their other IPv4 addresses.

This is a potential security issue, as it allows connections to local ports on an exit.

Child Tickets

Change History (39)

comment:1 Changed 3 years ago by nickm

Keywords: 026-backport added
Milestone: Tor: 0.2.7.x-final

comment:2 Changed 3 years ago by teor

Keywords: 026-backport removed
Milestone: 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.

comment:3 Changed 3 years ago by teor

Keywords: 026-backport added
Milestone: Tor: 0.2.7.x-final

Didn't mean to revert those.

comment:4 Changed 3 years ago by teor

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.)

comment:5 Changed 3 years ago by teor

Version: Tor: 0.2.7.2-alphaTor: unspecified

Further notes:

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

This fix will automatically benefit from changes that find more interfaces/addresses, perhaps #12377 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).

comment:6 Changed 3 years ago by teor

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

comment:7 Changed 3 years ago by teor

The unit tests pass on OS X and Linux

comment:8 Changed 3 years ago by teor

Status: newneeds_review

This branch may require manual changes to merge correctly with #16069: this branch adds arguments to functions that #16069 adds calls to.

comment:9 Changed 3 years ago by teor

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.)

comment:10 Changed 3 years ago by teor

Added two fixups that update the documentation and torrcs

comment:11 Changed 3 years ago by nickm

Looks good!

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

comment:12 in reply to:  11 Changed 3 years ago by teor

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 #12497. 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.

comment:13 Changed 3 years ago by teor

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

comment:14 Changed 3 years ago by teor

Resolution: fixed
Status: needs_reviewclosed

Merged

comment:15 Changed 3 years ago by teor

Resolution: fixed
Status: closedreopened

Reopened for possible backport to 026

comment:16 Changed 3 years ago by nickm

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

comment:17 Changed 3 years ago by nickm

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

comment:18 Changed 3 years ago by teor

Status: reopenedneeds_revision

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.

comment:19 Changed 3 years ago by teor

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

comment:20 Changed 3 years ago by teor

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 #17183 easier.

comment:21 Changed 3 years ago by teor

Keywords: TorCoreTeam201512 027-backport added; TorCoreTeam201509 removed
Severity: Normal

comment:22 Changed 3 years ago by teor

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.

comment:23 Changed 3 years ago by teor

Keywords: TorCoreTeam201511 added; TorCoreTeam201512 removed
Status: needs_revisionneeds_review

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.)

comment:24 Changed 3 years ago by teor

The 0.2.6 backport is in #17610.

comment:25 Changed 3 years ago by 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?

comment:26 in reply to:  25 Changed 3 years ago by teor

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.)

comment:27 Changed 3 years ago by 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?

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

comment:28 in reply to:  27 Changed 3 years ago by teor

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.

comment:29 Changed 3 years ago by teor

Keywords: 026-backport removed

This was merged into master in #17183.

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 #17610 for a limited reimplementation of this patch based on 0.2.6 functionality.)

comment:30 Changed 3 years ago by teor

This branch needed a fixup from #17183, 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.

comment:31 Changed 3 years ago by teor

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

comment:32 Changed 3 years ago by teor

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

comment:33 Changed 3 years ago by teor

Keywords: TorCoreTeam201512 added; TorCoreTeam201511 removed

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

comment:34 Changed 3 years ago by teor

Status: needs_reviewneeds_information

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:

  • 6913bdfcc568 - 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 (#17183) 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

6913bdfcc568 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 6913bdfcc568 and 22f82361ab4a so that they don't cause merge headaches for nickm?

comment:35 Changed 3 years ago by teor

Status: needs_informationneeds_revision

I don't think we need to backport refactoring.

But we might need to fix this code to handle non-printable addresses better, see #17762.

comment:36 Changed 3 years ago by teor

Status: needs_revisionneeds_review

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

comment:37 Changed 3 years ago by nickm

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).

comment:38 Changed 3 years ago by teor

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.

comment:39 Changed 3 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final
Resolution: fixed
Status: needs_reviewclosed

Okay, marking these tickets as "no backport." Please reopen if you disagree.

Note: See TracTickets for help on using tickets.