From 462568674a2465e8eb9cbb1cda65fa3e736c99fd Mon Sep 17 00:00:00 2001 From: Robert Hogan Date: Fri, 6 Aug 2010 20:29:15 +0100 Subject: [PATCH 1/3] Ensure controller RESOLVE commands respect __LeaveStreamsUnattached https://trac.torproject.org/projects/tor/ticket/1525 "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." Create a new function connection_ap_rewrite_and_attach_if_allowed() and call that when Tor needs to attach a stream to a circuit but needs to know if the controller permits it. No tests added. --- changes/bug1525 | 4 ++++ src/or/connection_edge.c | 37 ++++++++++++++++++++----------------- src/or/connection_edge.h | 3 +++ src/or/dnsserv.c | 22 ++++++++++++---------- 4 files changed, 39 insertions(+), 27 deletions(-) create mode 100644 changes/bug1525 diff --git a/changes/bug1525 b/changes/bug1525 new file mode 100644 index 0000000000..64f5b175d3 --- /dev/null +++ b/changes/bug1525 @@ -0,0 +1,4 @@ + o Minor bugfixes: + - Ensure DNS requests launched by "RESOLVE" commands from the controller + respect __LeaveStreamsUnattached. Bugfix on 0.2.2.14-alpha; + fixes bug 1525. diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 7522368c56..365a7972e3 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -1408,6 +1408,23 @@ consider_plaintext_ports(edge_connection_t *conn, uint16_t port) * different one? */ #define TRACKHOSTEXITS_RETRIES 5 +/** Call connection_ap_handshake_rewrite_and_attach() unless a controller + * asked us to leave streams unattached. + */ +int +connection_ap_rewrite_and_attach_if_allowed(edge_connection_t *conn, + origin_circuit_t *circ, + crypt_path_t *cpath) +{ + or_options_t *options = get_options(); + + if (options->LeaveStreamsUnattached) { + conn->_base.state = AP_CONN_STATE_CONTROLLER_WAIT; + return 0; + } + return connection_ap_handshake_rewrite_and_attach(conn, circ, cpath); +} + /** Connection conn just finished its socks handshake, or the * controller asked us to take care of it. If circ is defined, * then that's where we'll want to attach it. Otherwise we have to @@ -1908,11 +1925,7 @@ connection_ap_handshake_process_socks(edge_connection_t *conn) else control_event_stream_status(conn, STREAM_EVENT_NEW_RESOLVE, 0); - if (options->LeaveStreamsUnattached) { - conn->_base.state = AP_CONN_STATE_CONTROLLER_WAIT; - return 0; - } - return connection_ap_handshake_rewrite_and_attach(conn, NULL, NULL); + return connection_ap_rewrite_and_attach_if_allowed(conn, NULL, NULL); } /** connection_init_accepted_conn() found a new trans AP conn. @@ -1926,7 +1939,6 @@ int connection_ap_process_transparent(edge_connection_t *conn) { socks_request_t *socks; - or_options_t *options = get_options(); tor_assert(conn); tor_assert(conn->_base.type == CONN_TYPE_AP); @@ -1950,11 +1962,7 @@ connection_ap_process_transparent(edge_connection_t *conn) control_event_stream_status(conn, STREAM_EVENT_NEW, 0); - if (options->LeaveStreamsUnattached) { - conn->_base.state = AP_CONN_STATE_CONTROLLER_WAIT; - return 0; - } - return connection_ap_handshake_rewrite_and_attach(conn, NULL, NULL); + return connection_ap_rewrite_and_attach_if_allowed(conn, NULL, NULL); } /** connection_edge_process_inbuf() found a conn in state natd_wait. See if @@ -1975,7 +1983,6 @@ connection_ap_process_natd(edge_connection_t *conn) size_t tlen = 30; int err, port_ok; socks_request_t *socks; - or_options_t *options = get_options(); tor_assert(conn); tor_assert(conn->_base.type == CONN_TYPE_AP); @@ -2031,13 +2038,9 @@ connection_ap_process_natd(edge_connection_t *conn) control_event_stream_status(conn, STREAM_EVENT_NEW, 0); - if (options->LeaveStreamsUnattached) { - conn->_base.state = AP_CONN_STATE_CONTROLLER_WAIT; - return 0; - } conn->_base.state = AP_CONN_STATE_CIRCUIT_WAIT; - return connection_ap_handshake_rewrite_and_attach(conn, NULL, NULL); + return connection_ap_rewrite_and_attach_if_allowed(conn, NULL, NULL); } /** Iterate over the two bytes of stream_id until we get one that is not diff --git a/src/or/connection_edge.h b/src/or/connection_edge.h index c3d6098c5a..85444503b8 100644 --- a/src/or/connection_edge.h +++ b/src/or/connection_edge.h @@ -79,6 +79,9 @@ void client_dns_set_addressmap(const char *address, uint32_t val, const char *addressmap_register_virtual_address(int type, char *new_address); void addressmap_get_mappings(smartlist_t *sl, time_t min_expires, time_t max_expires, int want_expiry); +int connection_ap_rewrite_and_attach_if_allowed(edge_connection_t *conn, + origin_circuit_t *circ, + crypt_path_t *cpath); int connection_ap_handshake_rewrite_and_attach(edge_connection_t *conn, origin_circuit_t *circ, crypt_path_t *cpath); diff --git a/src/or/dnsserv.c b/src/or/dnsserv.c index e231b655f4..ad4f4122bc 100644 --- a/src/or/dnsserv.c +++ b/src/or/dnsserv.c @@ -141,16 +141,17 @@ evdns_server_callback(struct evdns_server_request *req, void *_data) control_event_stream_status(conn, STREAM_EVENT_NEW, 0); - /* Now, throw the connection over to get rewritten (which will answer it - * immediately if it's in the cache, or completely bogus, or automapped), - * and then attached to a circuit. */ + /* Now, unless a controller asked us to leave streams unattached, + * throw the connection over to get rewritten (which will + * answer it immediately if it's in the cache, or completely bogus, or + * automapped), and then attached to a circuit. */ log_info(LD_APP, "Passing request for %s to rewrite_and_attach.", escaped_safe_str_client(q->name)); q_name = tor_strdup(q->name); /* q could be freed in rewrite_and_attach */ - connection_ap_handshake_rewrite_and_attach(conn, NULL, NULL); + connection_ap_rewrite_and_attach_if_allowed(conn, NULL, NULL); /* Now, the connection is marked if it was bad. */ - log_info(LD_APP, "Passed request for %s to rewrite_and_attach.", + log_info(LD_APP, "Passed request for %s to rewrite_and_attach_if_allowed.", escaped_safe_str_client(q_name)); tor_free(q_name); } @@ -186,16 +187,17 @@ dnsserv_launch_request(const char *name, int reverse) return -1; } - /* Now, throw the connection over to get rewritten (which will answer it - * immediately if it's in the cache, or completely bogus, or automapped), - * and then attached to a circuit. */ + /* Now, unless a controller asked us to leave streams unattached, + * throw the connection over to get rewritten (which will + * answer it immediately if it's in the cache, or completely bogus, or + * automapped), and then attached to a circuit. */ log_info(LD_APP, "Passing request for %s to rewrite_and_attach.", escaped_safe_str_client(name)); q_name = tor_strdup(name); /* q could be freed in rewrite_and_attach */ - connection_ap_handshake_rewrite_and_attach(conn, NULL, NULL); + connection_ap_rewrite_and_attach_if_allowed(conn, NULL, NULL); /* Now, the connection is marked if it was bad. */ - log_info(LD_APP, "Passed request for %s to rewrite_and_attach.", + log_info(LD_APP, "Passed request for %s to rewrite_and_attach_if_allowed.", escaped_safe_str_client(q_name)); tor_free(q_name); return 0; From 162c8847de6f0d9f01c5ec1358e7345956990a74 Mon Sep 17 00:00:00 2001 From: Sebastian Hahn Date: Sun, 8 Aug 2010 15:20:24 +0200 Subject: [PATCH 2/3] Update some documentation for the bug 1525 fix --- changes/bug1525 | 2 +- src/or/connection_edge.c | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/changes/bug1525 b/changes/bug1525 index 64f5b175d3..04f2e3b793 100644 --- a/changes/bug1525 +++ b/changes/bug1525 @@ -1,4 +1,4 @@ o Minor bugfixes: - Ensure DNS requests launched by "RESOLVE" commands from the controller - respect __LeaveStreamsUnattached. Bugfix on 0.2.2.14-alpha; + respect __LeaveStreamsUnattached. Bugfix on 0.2.0.1-alpha; fixes bug 1525. diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 365a7972e3..a0f6e85c5c 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -1409,7 +1409,10 @@ consider_plaintext_ports(edge_connection_t *conn, uint16_t port) #define TRACKHOSTEXITS_RETRIES 5 /** Call connection_ap_handshake_rewrite_and_attach() unless a controller - * asked us to leave streams unattached. + * asked us to leave streams unattached. Return 0 in that case. + * + * See connection_ap_handshake_rewrite_and_attach()'s + * documentation for arguments and return value. */ int connection_ap_rewrite_and_attach_if_allowed(edge_connection_t *conn, From 22285aa964fd447f5537fa3e0a296a2b6ee405f9 Mon Sep 17 00:00:00 2001 From: Sebastian Hahn Date: Mon, 13 Sep 2010 07:10:15 +0200 Subject: [PATCH 3/3] Update the changelog for the bug1525 fix --- changes/bug1525 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/changes/bug1525 b/changes/bug1525 index 04f2e3b793..b0d9ac6f46 100644 --- a/changes/bug1525 +++ b/changes/bug1525 @@ -1,4 +1,5 @@ o Minor bugfixes: - Ensure DNS requests launched by "RESOLVE" commands from the controller - respect __LeaveStreamsUnattached. Bugfix on 0.2.0.1-alpha; - fixes bug 1525. + respect __LeaveStreamsUnattached. The same goes for requests launched + via DNSPort or transparent proxying. Bugfix on 0.2.0.1-alpha; fixes bug + 1525.