Opened 8 years ago

Closed 7 years ago

#8203 closed defect (fixed)

Inconsistent stream events when resolving hostname

Reported by: Desoxy Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client controller dns 024-deferrable
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

There are three different ways to tell Tor to resolve a hostname:

  1. Using the SOCKS RESOLVE protocol (e.g. via tor-resolve)
  2. Setting DNSPort and using standard DNS requests
  3. Sending a RESOLVE request via the control protocol.

All three methods lead to different stream events being sent via the control protocol.

Method 1 leads to these events:

650 STREAM 95 NEWRESOLVE 0 heise.de:0 PURPOSE=DNS_REQUEST
650 STREAM 95 NEW 15 heise.de:0 SOURCE_ADDR=(Tor_internal):58776 PURPOSE=DNS_REQUEST
650 STREAM 95 SENTRESOLVE 15 heise.de:0
650 STREAM 95 SUCCEEDED 15 heise.de:0
650 STREAM 95 REMAP 15 193.99.144.80:0 SOURCE=EXIT
650 STREAM 95 CLOSED 15 193.99.144.80:0 REASON=DONE

Method 2:

650 STREAM 99 NEW 0 heise.de:0 SOURCE_ADDR=127.0.0.1:39482 PURPOSE=DNS_REQUEST
650 STREAM 99 NEW 17 heise.de:0 SOURCE_ADDR=(Tor_internal):39482 PURPOSE=DNS_REQUEST
650 STREAM 99 SENTRESOLVE 17 heise.de:0
650 STREAM 99 REMAP 17 193.99.144.80:0 SOURCE=EXIT
650 STREAM 99 CLOSED 17 193.99.144.80:0 REASON=DONE

Method 3:

650 STREAM 103 NEW 18 heise.de:0 SOURCE_ADDR=(Tor_internal):0 PURPOSE=DNS_REQUEST
650 STREAM 103 SENTRESOLVE 18 heise.de:0
650 STREAM 103 REMAP 18 193.99.144.80:0 SOURCE=EXIT
650 STREAM 103 CLOSED 18 193.99.144.80:0 REASON=DONE

In my opinion, the following stream events should be generated:

650 STREAM 1 NEWRESOLVE 0 heise.de:0 SOURCE_ADDR=127.0.0.1:12345 PURPOSE=DNS_REQUEST
650 STREAM 1 SENTRESOLVE 1 heise.de:0
650 STREAM 1 REMAP 1 193.99.144.80:0 SOURCE=EXIT
650 STREAM 1 CLOSED 1 193.99.144.80:0 REASON=DONE

If the request was generated by method 3, the SOURCE_ADDR should be omitted. The reason why there is a SOURCE_ADDR and a duplicate STREAM_EVENT_NEW is that this was added for ticket #646.

I have attached a patch that will lead to the desired output, but I can change it if another output is preferred.

Child Tickets

Attachments (3)

stream_event_new_resolve.patch (3.5 KB) - added by Desoxy 8 years ago.
stream_event_new_resolve_torspec.patch (830 bytes) - added by Desoxy 8 years ago.
stream_event_new_resolve-v2.patch (5.1 KB) - added by Desoxy 7 years ago.

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by Desoxy

Changed 8 years ago by Desoxy

comment:1 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-final
Status: newneeds_review

comment:2 Changed 7 years ago by nickm

Keywords: tor-client added

comment:3 Changed 7 years ago by nickm

Keywords: controller dns 024-deferrable added
Status: needs_reviewneeds_revision

I think that the change in connection_edge, where you make conn->address no longer get set to tor_internal, seems likely to be iffy. Perhaps it would be better to change it to

   if (!ap_conn->_base.address) {
      ap_conn->_base.address = tor_strdup("(Tor_internal)");
   }

or something. If it's interfering with the SENT_RESOLVE event, we could move the SENT_RESOLVE event earlier in the function.

As for the other stuff, I like the idea of making this consistent! Certainly, the change is compatible with the text and spirit of the design and specification. Have you tested the changes with Vidalia? I'd like to get these changes tested with Vidalia and other controllers if possible to make sure that they're not going to break any of the controllers' assumptions.

(Marking as "0.2.4" deferrable, since even though I like this, it wouldn't be a disaster if it went in 0.2.5.1-alpha instead.)

comment:4 Changed 7 years ago by atagar

Hi Desoxy, looks good to me! I like the STREAM events you're proposing. It _might_ be possible that a controller somewhere relies on new streams having a NEW StreamStatus, but I'm not aware of any (and would probably be a bug on their part).

I'm less clear on why you want to omit SOURCE_ADDR when issued via RESOLVE - that would require a small note to be added to the control-spec, but otherwise I'm fine with it.

comment:5 Changed 7 years ago by Desoxy

Status: needs_revisionneeds_review

Thank you both for reviewing my patch.

Regarding the "(Tor_internal)" address and the control spec: The control spec specified there would always be a SOURCE_ADDR (when extended events were turned on), but it specified that SOURCE_ADDR to be Address ":" Port, which means that returning "(Tor_internal):0" was actually violating that spec:

  Address = ip4-address / ip6-address / hostname   (XXXX Define these)

(...)

      "650" SP "STREAM" SP StreamID SP StreamStatus SP CircuitID SP Target
          [SP "REASON=" Reason [ SP "REMOTE_REASON=" Reason ]]
          [SP "SOURCE=" Source] [ SP "SOURCE_ADDR=" Address ":" Port ]
          [SP "PURPOSE=" Purpose]
          CRLF

(...)

       Port = an integer from 0 to 65535 inclusive

I have now changed the patch to simply use the control connection informationas SOURCE_ADDR for method 3. This means that a) there is always a valid SOURCE_ADDR and b) the control spec does not have to be changed.

Vidalia: Vidalia uses stream events to display the Tor network map. However, it does not particularly care about the StreamStatus, other than displaying it to the user. As far as I could see, there are no assumptions that a stream has to start with a StreamStatus of "NEW".

Other controllers: Given that there are 3 different StreamEvent sequences that can happen depending on how the resolve request was made, it is a fair assumption that any controller relying on new streams having a "NEW" StreamStatus would have not worked with all 3 of them anyway. I am very much in favor of doing it right this time instead of keeping a duplicate "NEW" StreamStatus just to keep some controller library happy. Also, let's not forget that the "NEW" StreamStatus would only be sent after the stream was already attached to a circuit, while for non-DNS-streams it would be sent before the stream was attached to a circuit.

Changed 7 years ago by Desoxy

comment:6 Changed 7 years ago by nickm

Looking good. The issue with the ("tor_internal") block there is that the address might not be set at that point in the code, I had thought. I'll investigate that, fix if needed, and merge.

comment:7 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Tweaked slightly; applied as 3f837d4826cce0e7917e79d73b81aefc3fefc6bd . Thanks!

Note: See TracTickets for help on using tickets.