Opened 7 years ago

Closed 5 years ago

#10267 closed defect (fixed)

[PATCH] Fixed transparent proxy destination lookup on FreeBSD

Reported by: yurivict Owned by:
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: freebsd 026-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

FreeBSD has two mutually exclusive firewalls: ipfw(8) and pf(8). Currenlty only pf(8) is supported for transparent proxy lookups of destination addresses.

This patch adds support for ipfw(8). It first checks if /dev/pf exists. If it does, it assumes pf(8) is in use. Otherwise it assumes ipfw(8) is used.

ipfw(8) is actually native on FreeBSD and much more popular than pf(8).

Patch is against tor-0.2.3.25_1

Child Tickets

Attachments (1)

patch-zz-yuri.patch (2.1 KB) - added by yurivict 7 years ago.

Download all attachments as: .zip

Change History (20)

Changed 7 years ago by yurivict

Attachment: patch-zz-yuri.patch added

comment:1 Changed 7 years ago by nickm

Milestone: Tor: 0.2.5.x-final
Status: newneeds_review

Marking for inclusion in 0.2.5; 0.2.3 and 0.2.4 are currently in bugfixes-only mode.

Can we do any checks to verify that ipfw is actually in use? Otherwise, getsockname() will succeed, but will give a (presumably local) address.

The code could be simplified by the use of tor_addr_from_sockaddr().

In get_pf_socket(), do we really want to keep trying to open /dev/pf every time we get an incoming socket? That seems pretty wasteful. Maybe we should remember that /dev/pf wasn't there.

comment:2 Changed 7 years ago by yurivict1

I agree with your suggestions about tor_addr_from_sockaddr() and keeping the flag "/dev/pf exists"

On FreeBSD ipfw is the default, and to the minimal degree, is always used for the default allow-all rule. It can't be turned off completely, and also no additional rules can be added when pf is used. pf is a special-case, replacement firewall functionality. So opening /dev/pf is probably the best way to check what is the current firewall type in use.

You are right, this leaves the possibility for somebody to just connect to that address without firewall forwarding and then getsockaddr would produce the (meaningless) local address. This would be the error condition. Tor should not be trying to recursively connect to its own TransPort.

Another possibility is to allow the user to set the firewall type in config file, for example like this:
TransFirewallType ipfw
But this may be an overkill for this.

comment:3 Changed 7 years ago by nickm

Well, we already need a special option for selecting TPROXY on linux (#10582), so making a general purpose one wouldn't be too bad. I'm changing the TPROXY configuration option to be a bit more general so that we don't need one option per firewall type.

comment:5 Changed 6 years ago by yurivict271

It looks good. The only thing is that I think ipfw should be made default, and pf should be turned on by the option on FreeBSD.

My reasoning is that ipfw is the native firewall implementation in FreeBSD, and pf is ported from OpenBSD.

See here, in FreeBSD handbook: https://www.freebsd.org/doc/handbook/firewalls-pf.html

comment:6 Changed 6 years ago by andrea

Keywords: 025-triaged added

comment:7 Changed 6 years ago by nickm

This will conflict with #10896 if we merge both; I'll work on a combined branch.

Changing to ipfw by default would be problematic because it would break all existing pf configurations. What we *could* do is have Tor 0.2.5 or 0.2.6 warn the users that the default will change in a later version, and then later change to ipfw by default.

comment:8 Changed 6 years ago by nickm

Also please let me know if I should attribute this to some name other than "yurivict" in the ChangeLog.

comment:9 Changed 6 years ago by nickm

See branch 10267_plus_10896_rebased in my public repository (https://gitweb.torproject.org/nickm/tor.git) for both branches, plus some small tweaks. I can confirm that the code looks okay to me so far and it doesn't break compilation on OSX or Linux; can somebody else confirm that ipfw still works with this branch, and that it doesn't break regular pf users?

yurivict, can you confirm that I didn't mangle your code badly or put anything inaccurate in the changes file?

comment:10 Changed 6 years ago by nickm

Keywords: andrea-review-0254 added

Drop owners from needs_review tickets in tor 0.2.5

comment:11 Changed 6 years ago by andrea

See comment on #10267; recommend merging the nickm/10267_plus_10896_rebased branch for 0.2.5.4.

comment:12 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Rebased again and merged! Thanks, everybody.

comment:13 Changed 5 years ago by yurivict271

How do tickets relate to tor releases?

This was closed 11 months ago, yet today 0.2.6.3-rc is released and the patch still isn't there.
get_pf_socket function in 0.2.6.3 is obviously old, and just opens /dev/pf on FreeBSD.

Milestone is set to Tor: 0.2.5.x-final. What am I missing?

Last edited 5 years ago by yurivict271 (previous) (diff)

comment:14 Changed 5 years ago by nickm

The branch for this ticket got merged as 973661394abc393e3dfd5b82de86659ecadc72a4. The commits merged as part of it were:

  • Only expose clean_backtrace() if we'll implement it (211b8cc)
  • ipfw TransPort support on FreeBSD (10267) (3e4680f)
  • tor_addr_from_sockaddr() is applicable in ipfw code, so use it. (08ef8c0)
  • Educate tor on OpenBSD's use of divert-to rules with the pf firewall. (f680d0f)
  • Whitespace, doc fixes (db8259c)
  • Fix OSX compilation. (c00c45f)
  • Call pf-divert openbsd-specific, not no-linux (89e520e)
  • Changes file for 10896 (f9719b0)
  • Merge branch '10267_plus_10896_rebased_twice' (9736613)

comment:15 Changed 5 years ago by yurivict271

Resolution: implemented
Status: closedreopened
Type: enhancementdefect

My original patch for this ticket was lost, and this ticket was erroneously closed.
tor git head still doesn't have it.

On FreeBSD /dev/pf is the second firewall, the native firewall is ipfw, and it should be supported. Missing /dev/pf is *not* an error.

This particular section fails on FreeBSD in src/or/connection_edge.c:

#ifdef OPENBSD
  /* only works on OpenBSD */
  pf = tor_open_cloexec("/dev/pf", O_RDONLY, 0);
#else
  /* works on NetBSD and FreeBSD */
  pf = tor_open_cloexec("/dev/pf", O_RDWR, 0);
#endif

  if (pf < 0) {
    log_warn(LD_NET, "open(\"/dev/pf\") failed: %s", strerror(errno));
    return -1;
  }
Last edited 5 years ago by yurivict271 (previous) (diff)

comment:16 Changed 5 years ago by yurivict271

Priority: normalmajor
Status: reopenednew

I submitted this 1.5 years ago. Target milestone was 0.2.5.X, FYI now tor is @ 0.2.6.6 and this patch is still not in.

Therefore, setting priority=high. This needs to be resolved.

comment:17 Changed 5 years ago by nickm

Keywords: freebsd 026-backport added; 025-triaged andrea-review-0254 removed
Milestone: Tor: 0.2.5.x-finalTor: 0.2.7.x-final
Priority: majornormal

An updated patch was merged; see above for discussion why.

Does "TransProxyType ipfw" not work for you? That should cause ipfw, rather than pf, to get used.

comment:18 Changed 5 years ago by yurivict271

I see, you modified it the way that the tor app still breaks with the same message, and old patch still applies, but "TransProxyType ipfw" makes it work. You should have mentioned this option.

Thanks, closing now.

comment:19 Changed 5 years ago by yurivict271

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.