Allow MapAddress .exit even if AllowDotExit is 0

changed milestone to %Tor: 0.3.3.x-final in legacy/trac

Trac:
Milestone: N/A to Tor: 0.2.3.x-final

Agree. This one was on my queue of bugs to report as well.

Depending on the fix, we might want to fix it in 0.2.2 as well -- it worked in 0.2.1, and we broke it for 0.2.2. We can sort that out once we have a fix though.

There's code that's supposed to make it work... look at how we set remapped_to_exit and use it in connection_ap_handshake_rewrite_and_attach().

If I were designing this from scratch, I'd add some other syntax, like (for example) example.com::servername.exit or example.com!!servername.exit or something, so that we could block all invalid domain names at the socks/dns layer, thereby insuring that client applications never give us a name of this form, but we could still get there via MapAddress and friends.

Trac:
Priority: normal to major

Closed legacy/trac#4812 (moved) as a duplicate of this. legacy/trac#3325 (moved) is possibly related here.

Trac:
Type: enhancement to defect

Marking as a regression; this used to work.

Trac:
Keywords: N/A deleted, regression added

Testing a little with a MapAddress to a .exit, I see that we hit this clause:


      if (started_without_chosen_exit &&
          !strcasecmpend(socks->address, ".exit") &&
          map_expires < TIME_MAX)
        remapped_to_exit = 1;

Why isn't remapped_to_exit getting set? Because map_expires == TIME_MAX. Why was this map_expires==TIME_MAX check here? Because remapped_to_exit was originally used to let us know that we were in a TrackExitHost situation, where we needed to set the chosen_exit_retries field.

Probably fix in branch bug3940_022 in my public repository.

Should there be an additional option that says "not even in MapAddress"? Maybe.

Should there be more documentation that says that AllowDotExit 0 doesn't apply to TrackHostExits and MapAddress? Yes, I think so.

Other issues?

Trac:
Status: new to needs_review

Replying to nickm:

Probably fix in branch bug3940_022 in my public repository.

Looks fine I think. Bonus points if somebody has tested it. :)

Should there be an additional option that says "not even in MapAddress"? Maybe.

I'd say no. It's a pretty esoteric use. "Don't set a torrc option you didn't want to set" is probably better advice than adding more code to Tor. The goal of AllowDotExit was to protect us against remote hosts that can make us ask our socks port for new destinations. There's no analog to that with MapAddress.

Should there be more documentation that says that AllowDotExit 0 doesn't apply to TrackHostExits and MapAddress? Yes, I think so.

Yes.

I just added the manpage clarification.

Additionally, I made the requirements for .exit with MapAddress 0 more explicit: the last address mapping in the rewrite chain MUST be a TORRC, a TRACKEXIT, or a CONTROLLER. I don't think it was previously possible to reach that case without having the address come from one of those sources, but let's make the rule explicit.

Needs another review.

The first request I made to the mapped .exit address through the bug3940_022 (43f84c1) successfully connected, but the second failed, with Tor logging:

Mar 31 00:00:00.000 [warn] The ".exit" notation is disabled in Tor due to security risks. Set AllowDotExit in your torrc to enable it.
Mar 31 00:00:00.000 [warn] Invalid onion hostname [scrubbed]; rejecting

My torrc was:

SocksPort 19050
AllowDotExit 0
MapAddress blah.com check.torproject.org.politkovskaja2.exit
DataDirectory /tmp/tmp.3HrXkyBcNc

Trac:
Username: katmagic

Ow. I suspect that this behavior has something to do with cacheing the IP from looking up check.torproject.org the first time. Time to investigate. :(

Trac:
Status: needs_review to needs_revision

If SocksPort is the only place where literal strings of fqdn/ip..exit names are presented by the user, that is the only place that should check AllowDotExit. Meaning: maybe a bug in manpage re physical IP addresses:

AllowDotExit 0|1 ... /TransPort/NATDPort ...

I disabled ADE for now, looks like what's in the works is fine too. Thx.

Replying to grarpamp:

If SocksPort is the only place where literal strings of fqdn/ip..exit names are presented by the user, that is the only place that should check AllowDotExit.

There's also DNSPort.

Replying to grarpamp:

If SocksPort is the only place where literal strings of fqdn/ip..exit names are presented by the user, that is the only place that should check AllowDotExit. Meaning: maybe a bug in manpage re physical IP addresses:

AllowDotExit 0|1 ... /TransPort/NATDPort ...

I disabled ADE for now, looks like what's in the works is fine too. Thx.

Hmmm. That's a actually a pretty reasonable idea for a fix, maybe. I can't recall why we did it the other way back in 3e4379c2.

Please consider, examine, review, and test branch "bug3940_redux" in my public repository. I think this time, maybe I got it right? (There are some more complexities discussed in the commit message that make the patch nontrivial)

Trac:
Status: needs_revision to needs_review

I've pushed some trivial documentation and whitespace fixes to that branch, and tested it a little. It seems okay to me. Can anybody else test and review?

Okay. I did more testing, and reviewed it yet again. I am going to say that this probably works, and merge it.

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

reminder: next time please mention the bug number in the changes file. thanks!

Trac:
Keywords: regression deleted, regression tor-client added

Trac:
Component: Tor Client to Tor

Trac:
Username: fuzzyTew

regression.patch

remnove check with false assumption

Trac:
Username: fuzzyTew

This bug was reintroduced with 9d0fab98 'Allow MapAddress and Automap to work together' on 2014-04-08. Combined with f02fd6c3 'Remove AllowDotExit' 2017-09-07, it has not been possible to use MapAddress .exit for a number of months now.

The attached patch fixes the issue again.

Trac:
Username: fuzzyTew
Reviewer: N/A to N/A
Severity: N/A to Blocker
Sponsor: N/A to N/A
Status: closed to reopened
Resolution: fixed to N/A

Note that although the comment on the code I removed states it is preventing .exit domains that have come from users, actually it is preventing automapped mapaddresses. User-provided .exit domains are blocked on line 1247 of src/or/connection_edge.c:

  /* Check for whether this is a .exit address.  By default, those are
   * disallowed when they're coming straight from the client, but you're
   * allowed to have them in MapAddress commands and so forth. */
  if (!strcmpend(socks->address, ".exit")) {
    log_warn(LD_APP, "The  \".exit\" notation is disabled in Tor due to "
             "security risks.");
    control_event_client_status(LOG_WARN, "SOCKS_BAD_HOSTNAME HOSTNAME=%s",
                                escaped(socks->address));
    out->end_reason = END_STREAM_REASON_TORPROTOCOL;
    out->should_close = 1;
    return;
  }

Trac:
Username: fuzzyTew

Trac:
Keywords: regression tor-client deleted, regression tor-client 032-backport 031-backport 029-backport 033-must added
Status: reopened to needs_review
Milestone: Tor: 0.2.3.x-final to Tor: 0.3.3.x-final
Severity: Blocker to Normal

Trac:
Keywords: regression tor-client 032-backport 031-backport 029-backport 033-must deleted, regression tor-client 032-backport 031-backport 029-backport 033-must review-group-33 added

Trac:
Reviewer: N/A to nickm

I tested this on a tor-0.3.1.4-alpha (the version just after when AllowDotExit was removed). I'm not able to reproduce the issue? It could very well be that I've not taken the right steps, since it's been so long since I've tried to manually use .exit notation. In the arm control panel, I did:

>>> MAPADDRESS google.com=google.com.9D6AE1BD4FDF39721CE908966E79E16F9BFCCF2F.exit
google.com=google.com.9d6ae1bd4fdf39721ce908966e79e16f9bfccf2f.exit
>>> GETINFO address-mappings/all
google.com.9d6ae1bd4fdf39721ce908966e79e16f9bfccf2f.exit 172.217.22.142.9d6ae1bd4fdf39721ce908966e79e16f9bfccf2f.exit "2018-03-01 01:57:29"

I then went to google.com, and my tor seems to have built a four-hop circuit, with 9d6ae1bd4fdf39721ce908966e79e16f9bfccf2f as the exit node to google.com.

Trac:
Reviewer: nickm to nickm, isis

Trac:
Keywords: regression tor-client 032-backport 031-backport 029-backport 033-must review-group-33 deleted, tor-client, review-group-34, 029-backport, regression, 031-backport, 033-must, 032-backport, review-group-33 added

Trac:
Reviewer: nickm, isis to isis

fuzzyTew -- could you give instructions for reproducing this bug?

Trac:
Status: needs_review to needs_information

Marking all tickets reached by current round of 033 triage.

Trac:
Keywords: N/A deleted, 033-triage-20180320 added

Mark 033-must tickets as triaged-in for 0.3.3

Trac:
Keywords: N/A deleted, 033-included-20180320 added

According to the regression.patch that they attached, the new bug happens when they're doing an automap. That is, they probably have DNSPort set, and something like AutomapHostsOnResolve set, and they're making queries against their DNSPort.

I wonder if we intended for that use case to work?

I wonder if we intended for that use case to work?

So, AutomapHostsOnResolve is supposed to work with DNSPort... and if you pass a .exit address to the DNSPort, it is supposed to counts as if you had specified the .exit address in your connect request. If I understand correctly, AllowDotExit is not supposed to allow that case.

Can't reproduce; no contact from reporter for ~2 months; and above analysis suggests this is not a bug. Please reopen if one of those changes. :)

Trac:
Resolution: N/A to worksforme
Status: needs_information to closed

closed

mentioned in issue legacy/trac#4812 (moved)

mentioned in issue legacy/trac#6138 (moved)

mentioned in issue legacy/trac#6211 (moved)

mentioned in issue legacy/trac#6244 (moved)

moved from legacy/trac#3940 (moved)

added Bug label and removed 1 deleted label

added Client Regression labels and removed 1 deleted label

Allow MapAddress .exit even if AllowDotExit is 0

Child items 0

Activity