Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#7555 closed enhancement (fixed)

MapAddress from FQDN to .onion fails because resolve requests for hidden services are not allowed.

Reported by: aagbsn Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: tor-client, 025-triaged, nickm-patch, asn-review
Cc: Actual Points:
Parent ID: #14192 Points:
Reviewer: Sponsor:

Description (last modified by aagbsn)

Example torrc:

MapAddress irc.oftc.net 37lnq2veifl4kar7.onion

(Why would I want to do that? So that the host my IRC client connects to matches the SSL certificate presented by the server)

Here's what a connection to a hidden service without a MapAddress looks like.

Nov 22 13:41:54.000 [debug] connection_ap_handshake_rewrite_and_attach(): Client asked for [scrubbed]:7000
Nov 22 13:41:54.000 [info] connection_ap_handshake_rewrite_and_attach(): Got a hidden service request for ID '[scrubbed]'
Nov 22 13:41:54.000 [info] connection_ap_handshake_rewrite_and_attach(): Unknown descriptor [scrubbed]. Fetching.
Nov 22 13:41:54.000 [debug] rend_client_refetch_v2_renddesc(): Fetching v2 rendezvous descriptor for service [scrubbed]

And here's what happens with the above MapAddress:

Nov 22 13:53:52.000 [debug] connection_ap_handshake_rewrite_and_attach(): Client asked for [scrubbed]:0
Nov 22 13:53:52.000 [info] addressmap_rewrite(): Addressmap: rewriting [scrubbed] to [scrubbed]
Nov 22 13:53:52.000 [warn] Resolve requests to hidden services not allowed. Failing.

So it looks like the socks client tries to resolve www.duckduckgo.com, the address gets rewritten to 3g2upl4pq6kufc4m.onion, and then the request fails because resolving .onion doesn't make sense. Where do resolve requests for .onion normally get handled? I think I'd probably want to catch this MapAddress case in addressmap_rewrite and then proceed as usual for hidden services.

Thanks for any pointers!

Child Tickets

TicketTypeStatusOwnerSummary
#14259defectclosedmemleak in connection_ap_handshake_rewrite_and_attach()

Attachments (2)

screen_log_of_openssl_sclient_to_MapAddress_hidden_service (155.9 KB) - added by aagbsn 6 years ago.
Screen Log of Tor debug output.
7555.patch (1.6 KB) - added by aagbsn 6 years ago.

Download all attachments as: .zip

Change History (39)

comment:1 Changed 6 years ago by aagbsn

Description: modified (diff)

comment:2 Changed 6 years ago by nickm

Keywords: tor-client added
Milestone: Tor: unspecified

Is this in fact doing a resolve, or a direct request? If it's doing a direct connection attempt, this is indeed a bug. If it's doing a resolve, I'm not sure what your desired behavior is here. What should the behavior be for trying to look up the IP for a .onion address? What IP address should it give in return?

Maybe "automaphostsonresolve" will do what you want here?

(If you have automaphostsonresolve turned on, but this isn't working, that might also be a bug.)

comment:3 Changed 6 years ago by aagbsn

Here's my full torrc:

SocksPort 9050
AutomapHostsOnResolve 1
MapAddress irc.oftc.net 37lnq2veifl4kar7.onion
Log DEBUG stdout

I tried both with and without AutomapHostsOnResolve with Tor versions 0.2.2.39 and 0.2.3.25

comment:4 in reply to:  2 Changed 6 years ago by aagbsn

Replying to nickm:

Is this in fact doing a resolve, or a direct request? If it's doing a direct connection attempt, this is indeed a bug.

The socks connection is to [scrubbed]:0, which suggests a resolve IIRC. The debug log indicates that it fails at:

connection_edge.c:1266 (0f9524dbd0590c62a31b7d783a2ecbea7dbdcd37):

   if (SOCKS_COMMAND_IS_RESOLVE(socks->command)) {
      /* if it's a resolve request, fail it right now, rather than
       * building all the circuits and then realizing it won't work. */
      log_warn(LD_APP,
               "Resolve requests to hidden services not allowed. Failing.");

If it's doing a resolve, I'm not sure what your desired behavior is here. What should the behavior be for trying to look up the IP for a .onion address? What IP address should it give in return?

Ideally whatever IP a request for a hidden service would normally return? I'm afraid I assumed that clients always did a lookup for xyz.onion and Tor made this work transparently.

Maybe "automaphostsonresolve" will do what you want here?

(If you have automaphostsonresolve turned on, but this isn't working, that might also be a bug.)

Yep, see my torrc above.

comment:5 Changed 6 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.4.x-final
Priority: minornormal

comment:6 Changed 6 years ago by nickm

So, this seems to be happening just because the block in connection_ap_handshake_rewrite_and_attach() that handles automapping takes place *before* the one that calls addressmap_rewrite.

Unfortunately, this is some pretty ugly code: we need to think more about whether it's safe to reorder them, and if not, what to do instead.

comment:7 Changed 6 years ago by aagbsn

diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index 9563ca6..60df13e 100644
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -1831,7 +1831,7 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
       !tor_inet_aton(socks->address, &addr_tmp) &&
       options->AutomapHostsOnResolve && options->AutomapHostsSuffixes) {
     SMARTLIST_FOREACH(options->AutomapHostsSuffixes, const char *, cp,
-                      if (!strcasecmpend(socks->address, cp)) {
+                      if (strcasecmpend(socks->address, cp)) {
                         automap = 1;
                         break;
                       });
Last edited 4 years ago by arma (previous) (diff)

Changed 6 years ago by aagbsn

Screen Log of Tor debug output.

comment:8 Changed 6 years ago by aagbsn

So, my client was able to connect to the hidden service. But torsocks whined heartily about being pointed at a local address:

$ torsocks openssl s_client -connect irc.oftc.net:6697
05:42:38 libtorsocks(13106): connect: Connection is to a local address (127.192.0.1), may be a TCP DNS request to a local DNS server so have to reject to be safe. Please report a bug to http://code.google.com/p/torsocks/issues/entry if this is preventing a program from working properly with torsocks.
connect: No such file or directory
connect:errno=2

So for quick testing I committed a small sin and added this line to my torrc.

VirtualAddrNetwork 8.192.0.0/10

The attached screenlog was run with this torrc.

Changed 6 years ago by aagbsn

Attachment: 7555.patch added

comment:9 Changed 6 years ago by aagbsn

So, with this patch applied, why does torsocks complain when AutomapHostsOnResolve is set, for any host?

$ torsocks nc google.com 80
05:53:37 libtorsocks(13692): connect: Connection is to a local address (127.192.0.1), may be a TCP DNS request to a local DNS server so have to reject to be safe. Please report a bug to http://code.google.com/p/torsocks/issues/entry if this is preventing a program from working properly with torsocks.

If AutomapHostsOnResolve is not set, attempts to connect to the MapAddress mapped host fail as before:

torsocks openssl s_client -connect irc.oftc.net:6697
gethostbyname failure
connect:errno=2
Nov 24 05:58:49.000 [warn] Resolve requests to hidden services not allowed. Failing.

Connections to other hosts (without a MapAddress line) succeed:

$ torsocks nc google.com 80
get /
HTTP/1.0 400 Bad Request

comment:10 Changed 6 years ago by rransom

strcasecmpend("google.com", ".onion") should return non-zero, because "le.com" is not equal to ".onion".

comment:11 Changed 6 years ago by nickm

rransom is right -- Removing the ! from strcasecmpend() is completely wrong. The strcmp() family return 0 on equality: your patch makes everything *except* the members of AutomapHostsSuffixes get mapped.

The torsocks issue would appear to be a torsocks bug where it doesn't like the 127.192/10 address family.

As for Tor, I still think that the problem is what I said before:

So, this seems to be happening just because the block in connection_ap_handshake_rewrite_and_attach() that handles automapping takes place *before* the one that calls addressmap_rewrite. Unfortunately, this is some pretty ugly code: we need to think more about whether it's safe to reorder them, and if not, what to do instead.

comment:12 in reply to:  11 Changed 6 years ago by nickm

Replying to nickm:

rransom is right -- Removing the ! from strcasecmpend() is completely wrong. The strcmp() family return 0 on equality: your patch makes everything *except* the members of AutomapHostsSuffixes get mapped.

(Actually, to be precise, it makes everything get automapped unless it matches *every* suffix in automaphostssuffixes)

comment:13 Changed 6 years ago by aagbsn

Ahh, I forgot C convention is to return 0 on success. :-(

What I don't yet understand: if AutomapHostsOnResolve is 1, shouldn't everything get automapped on resolve?

Is it the case that AutomapHostsOnResolve will only work for suffixes in options>AutomapHostsSuffix?

comment:14 in reply to:  13 Changed 6 years ago by rransom

Replying to aagbsn:

Ahh, I forgot C convention is to return 0 on success. :-(

There is no general C convention for return values, but memcmp and strcmp return a negative value if their first argument is less than the second argument, zero if the arguments are equal, and a positive value if their first argument is greater than the second argument. strcasecmpend is related to strcmp. Perhaps it should be replaced with strcaseeqend in Tor. (Tor implements a tor_memeq function which returns non-zero iff its arguments are equal; it additionally runs in data-independent time to avoid side-channel leaks.)

What I don't yet understand: if AutomapHostsOnResolve is 1, shouldn't everything get automapped on resolve?

Is it the case that AutomapHostsOnResolve will only work for suffixes in options>AutomapHostsSuffix?

That is what the man page says.

comment:15 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final
03:18 < athena> so... 0.2.4 or not?
03:18 < nickm> I want to say "not" ?
03:19 < nickm> It seems a non-regression, and even if it were, The potential 
               for destabilization in that ugly code is high
03:20 < athena> okay, 0.2.5 then i think

comment:16 Changed 5 years ago by nickm

Keywords: 025-triaged added

comment:17 Changed 5 years ago by nickm

Okay, I think that the fix here is to split the addressmap_rewrite call into two: one part, which considers explicit MapAddress directives, happens before we do automapping. The other part, which considers everything else besides MapAddress mappings, happens after. "This shouldn't be terribly hard," he said...

comment:18 Changed 5 years ago by nickm

I've checkpointed a totally untested patch as branch "bug7555".

comment:19 Changed 5 years ago by nickm

Owner: set to nickm
Status: newassigned

marking as assigned since I've started coding on these.

comment:20 Changed 5 years ago by nickm

Current plan on these is to investigate the bug, try to finish writing the patch on each, and then evaluate whether the patch is 0.2.5 or 0.2.6 material in terms of simplicity and importance.

comment:21 Changed 5 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final
Type: defectenhancement

comment:22 Changed 4 years ago by nickm

branch bug7555_v2 has an improved refactoring attempt. Needs testing and an actual fix for this bug.

comment:23 Changed 4 years ago by nickm

Parent ID: #14192

comment:24 Changed 4 years ago by nickm

Now bug7555_v2 has lots of tests. Time to write a test and a fix for the actual issue, at long last.

comment:25 Changed 4 years ago by nickm

This branch is now complete, and helped me find a bug in my old fix and a couple of other bugs in the rewrite code.

comment:26 Changed 4 years ago by nickm

Status: assignedneeds_review

comment:27 Changed 4 years ago by nickm

Keywords: nickm-patch added

comment:28 Changed 4 years ago by nickm

Keywords: asn-review added

comment:29 Changed 4 years ago by rl1987

  • The unit test in test_entryconn_rewrite_mapaddress_automap_onion2() doesn't seem to cover all subcases that might be of interest here. What is supposed to happed when AutomapHostsOnResolve is off? What kind of behaviour we expect when there is no MapAddress line(s) in torrc? When user decides to remove .onion from AutomapHostsSuffixes and/or add .net? The unit test checks for single lucky case, but does not attempt define and assert correct behaviour for other cases.
  • AllowDotExit has nothing to do with functionality being tested in aforementioned unit test.
  • // tt_int_op(rr.automap, OP_EQ, 1); <-- commented out lines are generally poor code hygiene. Besided, this time we do care whether or not it did automapping.

comment:30 Changed 4 years ago by nickm

I think I've fixed those issues. How's it look now?

comment:31 Changed 4 years ago by 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...

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 :/

  • 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?
  • 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.


comment:32 in reply to:  31 ; Changed 4 years ago by nickm

Replying to 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.

comment:33 in reply to:  32 Changed 4 years ago by nickm

Replying to nickm:

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.

This is now #14266

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

This is now #14265

comment:34 Changed 4 years ago by nickm

Okay, I've added about 27 lines of new comments, and fixed #12459. Thoughts?

comment:35 in reply to:  34 Changed 4 years ago by asn

Replying to nickm:

Okay, I've added about 27 lines of new comments, and fixed #12459. Thoughts?

Very helpful. Thanks for all the comments.

As a nitpick, you added a comment that says condigured instead of configured. And you also added a comment saying /* Is the 'if' necessary here? ???? */ which might deserve an XXX or something.

Also, just making sure about this diff:

-    if (0 != (rr.end_reason & END_STREAM_REASON_DONE))
+    if (END_STREAM_REASON_DONE != (rr.end_reason & END_STREAM_REASON_MASK))
       return 0;
     else
       return -1;
   }

Do we want to return 0 when the end_reason includes END_STREAM_REASON_DONE, or do we want to return -1 in that case? The func doc does not specify.

comment:36 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

fixed those, squashed, merged. Thanks!

(The merge commit, 1053af0b9c4127873034a935ce3382940696e693, had a few conflicts. It looks okay to me, but more eyes would be a good thing here.)

Also, I found a bug when I did the merge, and marked it with "XXXX bug." I'll see when we introduced it, and make a ticket for it.

comment:37 Changed 4 years ago by nickm

The other bug I found was #14280.

Note: See TracTickets for help on using tickets.