[tor-bugs] #7555 [Tor]: MapAddress from FQDN to .onion fails because resolve requests for hidden services are not allowed.

[Tor Bug Tracker & Wiki]

#7555: MapAddress from FQDN to .onion fails because  resolve requests for hidden
services are not allowed.
-------------------------+-------------------------------------------------
     Reporter:  aagbsn   |      Owner:  nickm
         Type:           |     Status:  needs_review
  enhancement            |  Milestone:  Tor: 0.2.6.x-final
     Priority:  normal   |    Version:  Tor: unspecified
    Component:  Tor      |   Keywords:  tor-client, 025-triaged, nickm-
   Resolution:           |  patch, asn-review
Actual Points:           |  Parent ID:  #14192
       Points:           |
-------------------------+-------------------------------------------------

Comment (by nickm):

 Replying to [comment:31 asn]:
 > Oh gosh. This was a very hard patch to review and after many hours I
 still don't understand a good part of the code...

 Sorry about that. I told you this was one of the Doom functions, right?
 The ones that sit in the deeps of the Tor code, like some kind of creature
 out of HP Lovecraft, waiting to steal your sanity.  I hoped this patch
 series would make it a little cleaner, to be honest...

 > Some comments:
 >
 > * I'm fairly sure that the part where we split `rewrite_and_attach` to
 two functions is alright.
 >
 > * I found the commit that actually fixes this bug quite hard to
 understand because the surrounding code is very rough and without much
 docs. As I understand it, it does an initial rewrite so that if the
 address gets rewritten to an .onion address, then it can get automapped to
 a virtual IP address. Because before, the code was only rewriting to
 .onion without automapping the address, which caused it to fail during
 resolve.
 >
 >  That said, there is lots of hidden underlying logic in those functions
 that I don't get. For example, there is this `  } else if (!out->automap)
 {` block which was changed in that commit and I'm still completely
 oblivious on what it does :/

 Hm.  I can think of two actions here.  I could either add a bunch of
 comments, or ask for another review, or both.

 Probably it makes sense to do the comments, and then ask for another
 review.


 > * During review I found a possible memleak (#14259). What I found
 perplexing is that all this weird rewrite code is called even if the user
 has not set any rewrite or automap rules in the config file. Now that the
 code is splitted, could we call `connection_ap_handshake_rewrite()`
 '''only''' if there are rewriting rules that need to be applied? Or is
 normal DNS operation part of the rewrite logic, so it's not that easy?

 That's an interesting idea; it feels like a separate patch to me.  The
 client-side DNS cache logic is _also_ off-by-default, so it's not crazy
 for us to make the individual steps here all off-by-default.

 Two other things can cause address rewriting too, btw: TrackHostExits, and
 MAPADDRESS commands from the controller.

 > * I tested the branch using aagbsn's test case. I got the same torsocks
 error that aagbsn got in comment:9. I'm unsure on whether torsocks could
 be modified to trust Tor's virtual addresses. To actually test the branch,
 I did the terrible hack, where I set `VirtualAddrNetworkIPv4` to a public
 IP range. With that hack, torsocks tried to resolve that fake public IP,
 and tor gave it the proper answer as a result, which made it work. Which
 is good. However, I'm not sure what needs to happen on the torsocks-side
 to make this more useful for aagbsn's use case.
 >

 Sounds like there should be a torsocks ticket there if there isn't
 already.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/7555#comment:32>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online