Opened 9 years ago

Closed 9 years ago

Last modified 7 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:

Description

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 9 years ago by mwenge

Owner: set to mwenge
Status: newassigned

comment:3 Changed 9 years ago by mwenge

Status: assignedneeds_review

comment:4 Changed 9 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 9 years ago by mwenge

Cc: mwenge added

comment:6 Changed 9 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 0.2.0.1-alpha.

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

comment:7 Changed 9 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 9 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 9 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 9 years ago by Sebastian

updated the changelog

comment:11 Changed 9 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 9 years ago by nickm

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

comment:13 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged. Thanks again!

comment:14 Changed 7 years ago by nickm

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