Opened 7 years ago

Closed 7 months ago

#3940 closed defect (worksforme)

Allow MapAddress .exit even if AllowDotExit is 0

Reported by: sjmurdoch Owned by:
Priority: High Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version: Tor: 0.2.2.32
Severity: Normal Keywords: regression, tor-client, 032-backport, 031-backport, 029-backport, 033-must, review-group-33, review-group-34, 033-triage-20180320, 033-included-20180320
Cc: Actual Points:
Parent ID: Points:
Reviewer: isis Sponsor:

Description

If AllowDotExit is 0 (the default) MapAddress doesn't permit a .exit address to be set as the destination. It is desirable to keep AllowDotExit disabled for security reasons, but there shouldn't be any harm in always permitting MapAddress entries because the user must enter these manually. This feature is desirable because it allows the user to force exit enclaving.

Using Tor v0.2.2.32 (git-877e17749725ab88).

Child Tickets

Attachments (1)

regression.patch (1.2 KB) - added by fuzzyTew 8 months ago.
remnove check with false assumption

Download all attachments as: .zip

Change History (34)

comment:1 Changed 7 years ago by nickm

Milestone: Tor: 0.2.3.x-final

comment:2 Changed 7 years ago by arma

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.

comment:3 Changed 7 years ago by nickm

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.

comment:4 Changed 7 years ago by nickm

Priority: normalmajor

comment:5 Changed 7 years ago by nickm

Type: enhancementdefect

Closed #4812 as a duplicate of this. #3325 is possibly related here.

comment:6 Changed 7 years ago by nickm

Keywords: regression added

Marking as a regression; this used to work.

comment:7 Changed 7 years ago by nickm

Status: newneeds_review

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?

comment:8 in reply to:  7 Changed 7 years ago by arma

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.

comment:9 Changed 7 years ago by nickm

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.

comment:10 Changed 7 years ago by katmagic

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

comment:11 Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

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

comment:12 Changed 7 years ago by grarpamp

If SocksPort is the only place where literal strings of fqdn/ip.<fp>.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.

comment:13 in reply to:  12 Changed 7 years ago by rransom

Replying to grarpamp:

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

There's also DNSPort.

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

Status: needs_revisionneeds_review

Replying to grarpamp:

If SocksPort is the only place where literal strings of fqdn/ip.<fp>.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 3e4379c2e73bf4.

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)

comment:15 Changed 6 years ago by nickm

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?

comment:16 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

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

comment:17 Changed 6 years ago by arma

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

comment:18 Changed 6 years ago by nickm

Keywords: tor-client added

comment:19 Changed 6 years ago by nickm

Component: Tor ClientTor

Changed 8 months ago by fuzzyTew

Attachment: regression.patch added

remnove check with false assumption

comment:20 Changed 8 months ago by fuzzyTew

Resolution: fixed
Severity: Blocker
Status: closedreopened

This bug was reintroduced with 9d0fab9872807ef212fadb3feb299cf6309a185f 'Allow MapAddress and Automap to work together' on 2014-04-08. Combined with f02fd6c3af71141241137403d070d72310cbfd82 '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.

comment:21 Changed 8 months ago by fuzzyTew

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

comment:22 Changed 8 months ago by nickm

Keywords: 032-backport 031-backport 029-backport 033-must added
Milestone: Tor: 0.2.3.x-finalTor: 0.3.3.x-final
Severity: BlockerNormal
Status: reopenedneeds_review

comment:23 Changed 8 months ago by nickm

Keywords: review-group-33 added

comment:24 Changed 8 months ago by nickm

Reviewer: nickm

comment:25 Changed 8 months ago by isis

Reviewer: nickmnickm, isis

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.

comment:26 Changed 7 months ago by nickm

Keywords: review-group-34 added

comment:27 Changed 7 months ago by dgoulet

Reviewer: nickm, isisisis

comment:28 Changed 7 months ago by nickm

Status: needs_reviewneeds_information

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

comment:29 Changed 7 months ago by nickm

Keywords: 033-triage-20180320 added

Marking all tickets reached by current round of 033 triage.

comment:30 Changed 7 months ago by nickm

Keywords: 033-included-20180320 added

Mark 033-must tickets as triaged-in for 0.3.3

comment:31 Changed 7 months ago by arma

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?

comment:32 Changed 7 months ago by nickm

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.

comment:33 Changed 7 months ago by nickm

Resolution: worksforme
Status: needs_informationclosed

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

Note: See TracTickets for help on using tickets.