Opened 10 years ago

Closed 10 years ago

Last modified 8 years ago

#1525 closed defect (fixed)

RESOLVE control port command code path is incorrect

Reported by: mikeperry Owned by: mwenge
Priority: Low Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: mwenge Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


The codepath taken by the control port "RESOLVE" command to create a synthetic SOCKS resolve request isn't the same as the path taken by a real SOCKS request from 'tor-resolve'.

This prevents controllers who set LeaveStreamsUnattached=1 from being able to attach RESOLVE streams to circuits of their choosing.

Child Tickets

Change History (14)

comment:1 Changed 10 years ago by mwenge

Owner: set to mwenge
Status: newassigned

comment:3 Changed 10 years ago by mwenge

Status: assignedneeds_review

comment:4 Changed 10 years ago by Sebastian

The actual code changes look good, thanks!

Here's some documentation comments:

You should document the return value and arguments of connection_ap_rewrite_and_attach_if_allowed() (or point to connection_ap_handshake_rewrite_and_attach()'s documentation).

Also generally, I think the documentation should be "unless a controller asked us to leave streams unattached" instead of "if the controller has asked us to take care of attaching streams to circuits".

Another small issue is with the changes file. We like to end the text there with "Bugfix on <tor-release>; fixes bug <bugnumber>." so that people reading the changelog instead of the commit log have that information readily available.

comment:5 Changed 10 years ago by mwenge

Cc: mwenge added

comment:6 Changed 10 years ago by Sebastian

I see you updated your branch again. The "bugfix on" line is supposed to list the version that introduced the bug, though. Afaict that would be

I've pushed a branch bug1525 to my repo so you don't have to update your branch yet again :)

comment:7 Changed 10 years ago by Sebastian

Milestone: Tor: 0.2.2.x-final

I'm adding 0.2.2.x milestone here because this is a real bug that is affecting controllers

comment:8 Changed 10 years ago by nickm

The approach looks good; I'd say we should merge, but can we test more first? I'd just like to know that we've tried most of the connection cases with a controller running and without, and the controller got the notifications, and the connections were made successfully.

comment:9 Changed 10 years ago by nickm

When we merge this (if not sooner) we should update the changes file to note that this fixes not only the RESOLVE case but also the dnsport case and the tranparent-proxy cases.

comment:10 Changed 10 years ago by Sebastian

updated the changelog

comment:11 Changed 10 years ago by arma

The patch looks plausible to me. If we find somebody who says they tested it, green light from me.

comment:12 Changed 10 years ago by nickm

Sebastian and mwenge both tested it. I assert it's good to go.

comment:13 Changed 10 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged. Thanks again!

comment:14 Changed 8 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.