Opened 5 years ago

Closed 3 years ago

#6480 closed defect (fixed)

double connection_free() in dns_resolve()

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay 022-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


If dns_resolve()'s call to dns_resolve_impl() returns -1, it ends up running


      if (!exitconn->_base.marked_for_close) {

But dns_cancel_pending_resolve() runs

  while (resolve->pending_connections) {
    if (!pendconn->_base.marked_for_close)

So we would end up calling connection_free() on it twice. But we don't in practice, since the first connection_free() scribbles 0xCC on it, which sets marked_for_close to true, so we don't free it the second time! Cue Nick's circus music.

Our friendly irc person says "fix not so easy btw, connection_free() still need to call if no it was attached to pending resolve list."

Child Tickets

Change History (10)

comment:1 Changed 5 years ago by arma

Status: newneeds_review

Our friendly irc person suggests this patch:

--- src/or/dns.c
+++ src/or/dns.mod.c
@@ -168,7 +168,8 @@
 static int configure_nameservers(int force);
 static int answer_is_wildcarded(const char *ip);
 static int dns_resolve_impl(edge_connection_t *exitconn, int is_resolve,
-                            or_circuit_t *oncirc, char **resolved_to_hostname);
+                            or_circuit_t *oncirc, char **resolved_to_hostname,
+                            int *pended_connection);
 static void _assert_cache_ok(void);
 #define assert_cache_ok() _assert_cache_ok()
@@ -597,9 +598,11 @@
   or_circuit_t *oncirc = TO_OR_CIRCUIT(exitconn->on_circuit);
   int is_resolve, r;
   char *hostname = NULL;
+  int pended_connection = 0;
   is_resolve = exitconn->_base.purpose == EXIT_PURPOSE_RESOLVE;
-  r = dns_resolve_impl(exitconn, is_resolve, oncirc, &hostname);
+  r = dns_resolve_impl(exitconn, is_resolve, oncirc, &hostname,
+                       &pended_connection);
   switch (r) {
     case 1:
@@ -639,7 +642,7 @@
-      if (!exitconn->_base.marked_for_close) {
+      if (!pended_connection && !exitconn->_base.marked_for_close) {
         // XXX ... and we just leak exitconn otherwise? -RD
         // If it's marked for close, it's on closeable_connection_lst in
@@ -670,7 +673,8 @@
 static int
 dns_resolve_impl(edge_connection_t *exitconn, int is_resolve,
-                 or_circuit_t *oncirc, char **hostname_out)
+                 or_circuit_t *oncirc, char **hostname_out,
+                 int *pended_connection)
   cached_resolve_t *resolve;
   cached_resolve_t search;
@@ -797,6 +801,7 @@
   pending_connection = tor_malloc_zero(sizeof(pending_connection_t));
   pending_connection->conn = exitconn;
   resolve->pending_connections = pending_connection;
+  *pended_connection = 1;
   /* Add this resolve to the cache and priority queue. */
   HT_INSERT(cache_map, &cache_root, resolve);

which looks pretty straightforward.

I assigned the ticket to 0.2.4 originally since it isn't occurring in practice. We might want to move that to 0.2.3 if we become confident of the diagnosis and fix.

comment:2 Changed 5 years ago by nickm

Adapted that patch, and added some more comments, in branch "bug6480" in my public repo. Personally I'd like to stick it in 0.2.3, but if you think it's risky, we can try it out in 0.2.4.x for a bit first.

comment:3 Changed 5 years ago by nickm

Based on a conversation on IRC, I now think that maybe this should be "major" and in 0.2.3 with a possible backport to 0.2.2. It *is* a remote freed-memory read. If somebody's running Tor under valgrind, or with a particularly aggressive/paranoid malloc implementation, this could turn into a remote crash for them.

comment:4 Changed 5 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.3.x-final

comment:5 Changed 5 years ago by arma

I think putting it in 0.2.3 is fine.

I don't think backporting to 0.2.2 will be worth our while. I want 0.2.2 to be solidly frozen except for super critical things.

comment:6 Changed 5 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.2.x-final

Okay. Merged "bug6480_squashed" to maint-0.2.3. Moving this ticket to 0.2.2, just in case.

comment:7 Changed 5 years ago by nickm

Keywords: tor-relay added

comment:8 Changed 5 years ago by nickm

Component: Tor RelayTor

comment:9 Changed 4 years ago by nickm

Keywords: 022-backport added

comment:10 Changed 3 years ago by nickm

Milestone: Tor: 0.2.2.x-finalTor: 0.2.3.x-final
Resolution: fixed
Status: needs_reviewclosed

We're dropping 0.2.2 servers from the Tor network. Nothing gets backported to 0.2.2 for server-side at this point.

Note: See TracTickets for help on using tickets.