Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#6344 closed enhancement (implemented)

Implement proposal 204: ignore subdomains in hidden service addresses

Reported by: lunar Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: lunar, ale@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Attached is a patch that implements proposal 204. The present version address Nick's comments made on <https://lists.torproject.org/pipermail/tor-dev/2012-July/003723.html>.

Child Tickets

Attachments (1)

0001-Implement-proposal-204-ignore-subdomains-in-hidden-s.patch (3.7 KB) - added by lunar 7 years ago.
implement proposal 204; v2

Download all attachments as: .zip

Change History (12)

Changed 7 years ago by lunar

implement proposal 204; v2

comment:1 Changed 7 years ago by lunar

Status: newneeds_review

comment:2 Changed 7 years ago by nickm

Looks good! Remaining issues from my email:

  • Somebody should run this to make sure that it works. :)

Otherwise it looks fine to me.

comment:3 Changed 7 years ago by nickm

Oh! Actually, this may break IsolateDestAddr.

There's a security issue to think about here: Should we allow connections to two vhosts on the same hidden service to share a circuit? I think the answer is "not by default."

comment:4 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-final

comment:5 Changed 7 years ago by nickm

Keywords: tor-client added

comment:6 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:7 Changed 7 years ago by lunar

After applying the patch on top of ee4182612f, I have made some tests using the following protocol:

Tor started through:
src/or/tor RunAsDaemon 0 Log "info stdout" SafeLogging 0 SocksPort "127.0.0.1:9850 IsolateDestAddr"

Using curl to reach hidden services through:
{{{curl -I --socks5-hostname 127.0.0.1:9850 http://idnxcnkne4qt76tg.onion/}}

  1. http://idnxcnkne4qt76tg.onion/ → build circuit A
  2. http://idnxcnkne4qt76tg.onion/ → reuse circuit A
  3. http://first.idnxcnkne4qt76tg.onion/ → build circuit B
  4. http://first.idnxcnkne4qt76tg.onion/ → reuse circuit B
  5. http://idnxcnkne4qt76tg.onion/ → reuse circuit A
  6. http://second.idnxcnkne4qt76tg.onion/ → build circuit C

So it looks like it does not break IsolateDestAddr. I have been unable to make sure of that by looking at the code, though. :(

comment:8 Changed 7 years ago by nickm

Cool. I'll look at the code some more to figure out why it works. My guess now would be that the address getting changes is conn->address, and that edge_conn->original_dest_address (or whatever it's called: the one that circuit isolation looks at) is unchanged.

One last question if I may: when IsolateDestAddr *isn't* set, do requests like this share circuits? I wonder if for security maybe we should turn on IsolateDestAddr for all onion circuits no matter what.

comment:9 Changed 7 years ago by lunar

Same tests without using IsolateDestAddr:

  1. http://idnxcnkne4qt76tg.onion/ → build circuit A
  2. http://idnxcnkne4qt76tg.onion/ → reuse circuit A
  3. http://first.idnxcnkne4qt76tg.onion/ → reuse circuit A
  4. http://first.idnxcnkne4qt76tg.onion/ → reuse circuit A
  5. http://idnxcnkne4qt76tg.onion/ → reuse circuit A
  6. http://second.idnxcnkne4qt76tg.onion/ → reuse circuit A

comment:10 Changed 7 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Great; after discussion with Roger, I believe this is correct and mergeable.

comment:11 Changed 7 years ago by nickm

(and it's merged now too)

Note: See TracTickets for help on using tickets.