Opened 5 years ago

Closed 5 years ago

#11304 closed defect (fixed)

connection_write_to_buf_impl_() might incorrectly call connection_mark_for_close() on an orconn

Reported by: andrea Owned by: andrea
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version: Tor: 0.2.5.3-alpha
Severity: Keywords: tor-relay, 025-triaged, nickm-review-0254
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The function connection_write_to_buf_impl_() in connection.c contains this code:

old_datalen = buf_datalen(conn->outbuf);
if (zlib) {
  dir_connection_t *dir_conn = TO_DIR_CONN(conn);
  int done = zlib < 0;
  CONN_LOG_PROTECT(conn, r = write_to_buf_zlib(conn->outbuf,
                                               dir_conn->zlib_state,
                                               string, len, done));
} else {
  CONN_LOG_PROTECT(conn, r = write_to_buf(string, len, conn->outbuf));
}
if (r < 0) {
  if (CONN_IS_EDGE(conn)) {
    /* if it failed, it means we have our package/delivery windows set
       wrong compared to our max outbuf size. close the whole circuit. */
    log_warn(LD_NET,
             "write_to_buf failed. Closing circuit (fd %d).", (int)conn->s);
    circuit_mark_for_close(circuit_get_by_edge_conn(TO_EDGE_CONN(conn)),
                           END_CIRC_REASON_INTERNAL);
  } else {
    log_warn(LD_NET,
             "write_to_buf failed. Closing connection (fd %d).",
             (int)conn->s);
             connection_mark_for_close(conn);
  }
  return;
}

If conn is an orconn and write_to_buf() or write_to_buf_zlib() ever fails, this call to connection_mark_for_close() will incorrectly close it out from under the channel layer. This should test for orconns and use connection_or_close_for_error() instead in that case. Created pursuant to connection_mark_for_close() audit task #7472.

Child Tickets

Change History (6)

comment:1 Changed 5 years ago by nickm

Keywords: tor-relay 025-triaged added

comment:2 Changed 5 years ago by nickm

Status: newassigned

comment:3 Changed 5 years ago by andrea

Proposed fix in my bug11304 branch; it doesn't appear to break anything, but since this is in an apparent can't-happen case I can't confirm that it fixes anything in testing.

comment:4 Changed 5 years ago by andrea

Status: assignedneeds_review

comment:5 Changed 5 years ago by nickm

Keywords: nickm-review-0254 added

comment:6 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks good; merged!

Note: See TracTickets for help on using tickets.