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

changed milestone to %Tor: 0.2.6.x-final

added 025-triaged asn-review component::core tor/tor milestone::Tor: 0.2.6.x-final nickm-patch owner::nickm parent::14192 priority::medium resolution::fixed status::closed tor-client type::enhancement version::tor unspecified labels

Trac:
Description: 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 prested 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!

to

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!

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.)

Trac:
Keywords: N/A deleted, tor-client added
Milestone: N/A to Tor: unspecified

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

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.

Trac:
Milestone: Tor: unspecified to Tor: 0.2.4.x-final
Priority: minor to normal

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.

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;
                       });

Trac:
screen_log_of_openssl_sclient_to_MapAddress_hidden_service

Screen Log of Tor debug output.

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.

Trac:
7555.patch

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

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

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.

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)

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?

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.

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

Trac:
Milestone: Tor: 0.2.4.x-final to Tor: 0.2.5.x-final

Trac:
Keywords: tor-client deleted, tor-client 025-triaged added

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

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

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

Trac:
Status: new to assigned
Owner: N/A to 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.

Trac:
Milestone: Tor: 0.2.5.x-final to Tor: 0.2.6.x-final
Type: defect to enhancement

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

Trac:
Parent: N/A to #14192 (moved)

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

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.

Trac:
Status: assigned to needs_review

Trac:
Keywords: tor-client 025-triaged deleted, tor-client 025-triaged nickm-patch added

Trac:
Keywords: tor-client 025-triaged nickm-patch deleted, tor-client, asn-review, 025-triaged, nickm-patch added

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.

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

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 (moved)). 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.

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 (moved)). 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.

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 (moved)

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

This is now #14265 (moved)

Okay, I've added about 2^7^ lines of new comments, and fixed #12459 (moved). Thoughts?

Replying to nickm:

Okay, I've added about 2^7^ lines of new comments, and fixed #12459 (moved). 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.

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.

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

The other bug I found was #14280 (moved).

closed

mentioned in issue #14193 (moved)

mentioned in issue #14259 (moved)

moved to tpo/core/tor#7555 (closed)

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

Child items 0

Activity