[tor-bugs] #6799 [Tor]: Don't expire unused relay-to-relay TLS conns so quickly

[Tor Bug Tracker & Wiki]

#6799: Don't expire unused relay-to-relay TLS conns so quickly
-------------------------+-------------------------------------------------
     Reporter:  arma     |      Owner:
         Type:  defect   |     Status:  needs_review
     Priority:  major    |  Milestone:  Tor: 0.2.5.x-final
    Component:  Tor      |    Version:
   Resolution:           |   Keywords:  tor-relay anonymity-attack
Actual Points:           |  025-triaged 024-backport andrea-review-0255
       Points:           |  Parent ID:
-------------------------+-------------------------------------------------

Comment (by nickm):

 Replying to [comment:25 andrea]:
 > Code review for the ticket6799_024_v2 branch:
 >
 > 051d599b4adba70312e23148f5e208075b673bae:
 >  - I think this is the case, but just to double-check: is the only
 visible
 >    behavior which depends on or_conn->idle_timeout closing the
 connection?
 >    This defense depends on there not being any way for the attacker to
 learn
 >    the randomized timeout.

 I believe that's the case; the only things that look at it (according to
 git grep idle_timeout in ticket6799_024_v2) are the timeout check in
 main.c which closes the connection, and the initialization check in
 connection_or which checks whether it's already set before setting it
 again.

 > b9919b7bae75f831d31ae5d3d11bb0b721bb9aab:
 >  - I think this patch is correct, but is this another case where things
 might
 >    break if the clock jumps and we should use CLOCK_MONOTONIC if
 available?
 >    That may be a bug in the old code too.

 That's right.  We have a ticket in 0.2.6 to do CLOCK_MONOTONIC migration
 in #10168.  In 0.2.4, we don't have tor_gettimeofday_cached_monotonic, so
 this patch doesn't use it.

 I don't think that our failure to use monotonic time here actually makes
 stuff ''worse'' than it was without this patch?

 >  - We run run_connection_housekeeping() once a second, so this has the
 same
 >    granularity as the randomized timeouts and doesn't reduce the
 effective
 >    entropy, but that's a bit by coincidence.  Perhaps a comment to that
 >    effect somewhere?

 I'm not sure what that comment should say, which probably means that we
 need one. I'd be happy to apply any comment patch you have there if you
 write one?

 > fbc964b41b0bff4e55e90a1245dc65744abbebc2:
 >  - Isn't have_any_circuits = 0; redundant, since it's initialized to
 zero?

 yeah. I'll remove the initializer.

 I just added a new fixup patch for that in ticket6799_024_v2, and squashed
 it all in ticket6799_024_v2_squashed.  I'm going to review it all again,
 then merge.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/6799#comment:26>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online