Opened 2 years ago

Closed 5 months ago

#18100 closed defect (fixed)

src/or/connection_edge.c typo

Reported by: jirib Owned by:
Priority: Medium Milestone: Tor: 0.2.9.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 9 months ago.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 2 years ago by jirib

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

comment:2 Changed 2 years ago by teor

Keywords: 027-backport 026-backport added
Milestone: Tor: 0.2.8.x-final
Status: newneeds_review
Version: 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 Changed 2 years ago by nickm

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

comment:4 Changed 23 months ago by nickm

Ping? Is anybody able to test this?

comment:5 in reply to:  3 Changed 23 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 23 months ago by teor

Status: needs_reviewneeds_information

comment:7 Changed 20 months ago by nickm

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

comment:8 Changed 20 months ago by nickm

Keywords: TorCoreTeam201605 added

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

comment:9 Changed 19 months ago by nickm

Keywords: 029-proposed added; 027-backport 026-backport removed
Milestone: Tor: 0.2.8.x-finalTor: 0.2.???

comment:10 Changed 19 months ago by nickm

Keywords: TorCoreTeam201605 removed

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

comment:11 Changed 18 months ago by nickm

Keywords: 029-proposed removed
Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

comment:12 Changed 16 months ago by isabela

Keywords: isaremoved added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

comment:13 Changed 16 months ago by nickm

Keywords: nickwants029 lorax added

comment:14 Changed 16 months ago by nickm

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

comment:15 Changed 13 months ago by teor

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

Milestone renamed

comment:16 Changed 12 months 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:17 Changed 9 months ago by nickm

Status: needs_informationneeds_review

Changed 9 months ago by d4fq0fQAgoJ

Attachment: 0001-trans_tproxy.patch added

comment:18 Changed 9 months ago by d4fq0fQAgoJ

Status: needs_reviewneeds_revision
Version: Tor: 0.2.6.3-alphaTor: 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 9 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 Changed 9 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

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?

Version 0, edited 9 months ago by d4fq0fQAgoJ (next)

comment:21 Changed 9 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.1.x-final
Status: needs_revisionneeds_review

Thanks for analyzing and testing, d4fq0fQAgoJ.

comment:22 Changed 7 months ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;

comment:23 Changed 7 months 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 months 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 months 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 months 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 7 months ago by nickm

Keywords: 029-backport 030-backport added; isaremoved nickwants029 lorax tor-03-unspecified-201612 removed
Milestone: Tor: 0.3.1.x-finalTor: 0.3.0.x-final
Status: merge_readyneeds_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 7 months ago by nickm

Status: needs_reviewmerge_ready

comment:29 Changed 7 months ago by nickm

Keywords: 029-backport 030-backport029-backport, 030-backport

comment:30 Changed 5 months ago by nickm

Milestone: Tor: 0.3.0.x-finalTor: 0.2.9.x-final
Resolution: fixed
Status: merge_readyclosed

Merged to 0.2.9 and 0.3.0.

Note: See TracTickets for help on using tickets.