Opened 18 months ago

Last modified 5 weeks ago

#18100 merge_ready defect

src/or/connection_edge.c typo

Reported by: jirib Owned by:
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: 0.2.9.9
Severity: Normal Keywords: 029-backport, 030-backport
Cc: teor Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -1798,7 +1798,7 @@ destination_from_socket(entry_connection_t *conn, socks_request_t *req)

socklen_t orig_dst_len = sizeof(orig_dst);
tor_addr_t addr;


-#ifdef TRANS_TRPOXY
+#ifdef TRANS_TPROXY

if (options->TransProxyType_parsed == TPT_TPROXY) {

if (getsockname(ENTRY_TO_CONN(conn)->s, (struct sockaddr*)&orig_dst,

&orig_dst_len) < 0) {

Child Tickets

Attachments (1)

0001-trans_tproxy.patch (557 bytes) - added by d4fq0fQAgoJ 3 months ago.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 18 months ago by jirib

Found by sthen@ at OpenBSD ports' list - hxxp://marc.info/?l=openbsd-ports&m=145321419324485&w=2

comment:2 Changed 18 months ago by teor

  • Keywords 027-backport 026-backport added
  • Milestone set to Tor: 0.2.8.x-final
  • Status changed from new to needs_review
  • Version set to Tor: 0.2.6.3-alpha

This appears to be due to a8835170d710cae0ac1e8698cd5b750077025548 on 4 Feb 2015, released in 0.2.6.3-alpha.

Also in the same commit, connection_ap_get_original_destination doesn't have any code to call destination_from_socket when TRANS_TPROXY is defined. This is most likely a bug, too.

Please see my branch bug18100-v2 on https://github.com/teor2345/tor.git

It's based on maint-0.2.6 and split it into several commits, in order of importance:

  • Make connection_ap_get_original_destination & destination_from_socket work with TRANS_TPROXY
  • Make destination_from_socket only call the TRANS_PF code under TPT_PF_DIVERT
  • Clean up error handling and logging code

There's still some duplicate getsockname code in destination_from_socket, split off into #18105 (it's tough to get rid of, and not worth backporting).

comment:3 follow-up: Changed 17 months ago by nickm

Can somebody test this patch on an OpenBSD box with TPROXY?

comment:4 Changed 17 months ago by nickm

Ping? Is anybody able to test this?

comment:5 in reply to: ↑ 3 Changed 17 months ago by teor

Replying to nickm:

Can somebody test this patch on an OpenBSD box with TPROXY?

Tor's source says that TPROXY is a Linux-specific feature. Did we get that wrong?

    } else if (!strcasecmp(options->TransProxyType, "tproxy")) {
#if !defined(__linux__)
      REJECT("TPROXY is a Linux-specific feature.");
#else
      options->TransProxyType_parsed = TPT_TPROXY;
#endif

comment:6 Changed 17 months ago by teor

  • Status changed from needs_review to needs_information

comment:7 Changed 14 months ago by nickm

sorry, whoops. Were you able to test this with Linux?

comment:8 Changed 14 months ago by nickm

  • Keywords TorCoreTeam201605 added

sorry, whoops. Were you able to test this with Linux?

comment:9 Changed 13 months ago by nickm

  • Keywords 029-proposed added; 027-backport 026-backport removed
  • Milestone changed from Tor: 0.2.8.x-final to Tor: 0.2.???

comment:10 Changed 13 months ago by nickm

  • Keywords TorCoreTeam201605 removed

Remove "TorCoreTeam201605" keyword. The time machine is broken.

comment:11 Changed 12 months ago by nickm

  • Keywords 029-proposed removed
  • Milestone changed from Tor: 0.2.??? to Tor: 0.2.9.x-final

comment:12 Changed 11 months ago by isabela

  • Keywords isaremoved added
  • Milestone changed from Tor: 0.2.9.x-final to Tor: 0.2.???

comment:13 Changed 11 months ago by nickm

  • Keywords nickwants029 lorax added

comment:14 Changed 11 months ago by nickm

This will need somebody to actually test it, or it can't go in.

comment:15 Changed 8 months ago by teor

  • Milestone changed from Tor: 0.2.??? to Tor: 0.3.???

Milestone renamed

comment:16 Changed 6 months ago by nickm

  • Keywords tor-03-unspecified-201612 added
  • Milestone changed from Tor: 0.3.??? to Tor: unspecified

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

comment:17 Changed 4 months ago by nickm

  • Status changed from needs_information to needs_review

Changed 3 months ago by d4fq0fQAgoJ

comment:18 Changed 3 months ago by d4fq0fQAgoJ

  • Status changed from needs_review to needs_revision
  • Version changed from Tor: 0.2.6.3-alpha to Tor: 0.2.9.9

See attachment patch 0001-trans_tproxy.patch.

Revised patch for tor 0.2.9.9. Tested patch and TPROXY functionality on Linux.

Please revise.

comment:19 Changed 3 months ago by cypherpunks

could someone (nickm?) explain/discuss the effect/significance of this bug?

it appears that a block of code was ifdef'd with a typo'd name, so it was never running, and now it will run (when TRANS_TPROXY is set).

what does this code do?

(how) did transparent proxying support work without this code (with this typo)?

thanks!

comment:20 follow-up: Changed 3 months ago by d4fq0fQAgoJ

System: Linux 4.10.5-1-ARCH x86_64, tor 0.2.9.9

It appears to me that TPROXYing only works for me when connection tracking is active. When I apply the patch TPROXYing works for me no matter if conntection tracking is active or not.

Steps to reproduce:

Make sure no conntrack modules are loaded.
# lsmod | grep conntrack
should print nothing.

Flush your firewall rules:
# iptables -t raw -F
# iptables -t mangle -F
# iptables -t nat -F
# iptables -t filter -F

You also want to make sure the default policy is set to ACCEPT for your chains:
# iptables -t filter -P INPUT ACCEPT
# iptables -t filter -P OUTPUT ACCEPT
and so on...

Setup TPROXYing with iptables:
# iptables -t mangle -A PREROUTING -m socket --transparent -j ACCEPT
# iptables -t mangle -A PREROUTING -p tcp --syn -d 127.192.0.0/10 -j TPROXY --on-port 9052

Start Tor with the following config:

SOCKSPort 0
TransPort 9052 IsolateClientProtocol IsolateDestAddr
TransProxyType TPROXY
DNSPort 9053
AutomapHostsOnResolve 1
Log notice stdout
DataDirectory /tmp/tor
User tor

# tor -f ./<TORRC>
[notice] Opening DNS listener on 127.0.0.1:9053
[notice] Opening Transparent pf/netfilter listener on 127.0.0.1:9052

Verify traffic to 127.192.0.0/10 is routed properly to localhost (should always be):
# ip route get to 127.192.0.0/10 from 127.0.0.1

local 127.192.0.0 from 127.0.0.1 dev lo uid 0
cache <local>

Ask Tor to automap an onion for us:
# drill -p 9053 duskgytldkxiuqc6.onion @127.0.0.1

Make sure no conntrack modules are loaded (or more precisely: connection tracking is not active for the path to 127.192.0.0/10), otherwise curl will succeed! Use the IP address reported by drill.
# curl 127.192.A.B
Tor will report:
[warn] getsockopt() failed: No such file or directory
[warn] Fetching original destination failed. Closing.

Call curl again but this time with loaded conntrack modules.
# modprobe nf_conntrack_ipv4
# curl 127.192.A.B
It should succeed now.

Looking at the code it seems that getsockopt() is called. The patch activates a code path where getsockname() is called instead which seems to make it work even if no connection tracking is active. Maybe the author of that code can shed more light into that?

Last edited 3 months ago by d4fq0fQAgoJ (previous) (diff)

comment:21 Changed 3 months ago by nickm

  • Milestone changed from Tor: unspecified to Tor: 0.3.1.x-final
  • Status changed from needs_revision to needs_review

Thanks for analyzing and testing, d4fq0fQAgoJ.

comment:22 Changed 7 weeks ago by dgoulet

  • Status changed from needs_review to merge_ready

lgtm;

comment:23 Changed 7 weeks ago by nickm

  • Cc teor added

teor -- can you comment on d4fq0fQAgoJ's questions above before I merge?

comment:24 in reply to: ↑ 20 Changed 7 weeks ago by teor

Replying to d4fq0fQAgoJ:

System: Linux 4.10.5-1-ARCH x86_64, tor 0.2.9.9

It appears to me that TPROXYing only works for me when connection tracking is active. When I apply the patch TPROXYing works for me no matter if conntection tracking is active or not.

...
Looking at the code it seems that getsockopt() is called. The patch activates a code path where getsockname() is called instead which seems to make it work even if no connection tracking is active. Maybe the author of that code can shed more light into that?

I think the behaviour you describe is what we want.

I re-wrote the code to make it behave consistently and improve how it was structured back in early 2016.

I don't know enough about transparent proxying to say anything else with any confidence.

comment:25 Changed 7 weeks ago by nickm

Hm. Okay, I will need some confirmation about what to actually merge and what has actually been tested. Teor's branch? The small patch from d4fq0fQAgoJ above? Both? Neither?

comment:26 Changed 7 weeks ago by d4fq0fQAgoJ

I've been using the patched version (0001-trans_tproxy.patch) with a TPROXY iptables setup since I commented here and so far it's been working as expected for me.

The only real documentation about the TPROXY feature I found is from the kernel documentation (Documentation/networking/tproxy.txt). Unfortunately it does not say anything about getsockname() or getsockopt(SO_ORIGINAL_DST).

It seems that the TPROXY kernel feature enables transparent proxy capabilities without the need to DNAT (what else would be it's purpose then?). The above experiment backs this up because TPROXYing works without conntrack kernel modules loaded (conntracking is required for NAT). This only works with the above patch applied which utilizes getsockname() instead of getsockopt(SO_ORIGINAL_DST). Therefore it seems that getsockname() is the correct way.

comment:27 Changed 6 weeks ago by nickm

  • Keywords 029-backport 030-backport added; isaremoved nickwants029 lorax tor-03-unspecified-201612 removed
  • Milestone changed from Tor: 0.3.1.x-final to Tor: 0.3.0.x-final
  • Status changed from merge_ready to needs_review

Okay, sounds good. I've put your patch in a new branch bug18100_029 in my public repository, along with a commit message and a changes file. I'm merging it into master now. If nothing goes wrong (and I don't expect it will) we can backport to 0.2.9 and 0.3.0. Thanks!

comment:28 Changed 5 weeks ago by nickm

  • Status changed from needs_review to merge_ready

comment:29 Changed 5 weeks ago by nickm

  • Keywords changed from 029-backport 030-backport to 029-backport, 030-backport
Note: See TracTickets for help on using tickets.