[tor-bugs] #6475 [Tor Client]: circuit_send_next_onion_skin(): Bug: Unexpectedly high circuit_successes

[Tor Bug Tracker & Wiki]

#6475: circuit_send_next_onion_skin(): Bug: Unexpectedly high circuit_successes
-----------------------------+----------------------------------------------
 Reporter:  grarpamp         |          Owner:  mikeperry         
     Type:  defect           |         Status:  assigned          
 Priority:  major            |      Milestone:  Tor: 0.2.3.x-final
Component:  Tor Client       |        Version:  Tor: 0.2.3.19-rc  
 Keywords:  MikePerry201208  |         Parent:                    
   Points:                   |   Actualpoints:                    
-----------------------------+----------------------------------------------

Comment(by mikeperry):

 Ok. Here's some initial analysis:

 1. There may be a few codepaths that allow circuit_send_next_onion_skin()
 to get called multiple times for the same circuit with a NULL
 onion_next_hop_in_cpath() value (which is the signal we use to determine
 the circuit is done). In particular the control port can do this for sure.
 rend_client_reextend_intro_circuit() and rend_client_receive_rendezvous()
 seem also able to do it. Possibly .exit notation, too.

 2. *However*, there is *no* codepath in circuit_send_next_onion_skin()
 that allows us to count a successful circuit without also setting
 circ->has_opened = 1. There are also no codepaths that *unset*
 circ->has_opened. Thus, double-counting on the success side should be
 impossible, even in the above cases for point 1.

 3. Therefore, this is an issue with the first_hop counting logic. Somehow
 circuits are completing a first hop without getting counted by
 entry_guard_inc_first_hop_count().

 4. There appear to be no codepaths that allow us to finish a hop in a
 circuit without going through circuit_finish_handshake(), though this is a
 bit trickier to determine for sure.

 All of this together seems to point to an issue with the checks in
 circuit_finish_handshake() itself.

 As a kludge and to protect against future bugs, nickm suggested creating a
 new field of origin_circuit_t that we set inside
 entry_guard_inc_first_hop_count() and then check for in
 circuit_send_next_onion_skin(), so that we are absolutely sure we only
 count successes for circuits whose first hops we counted. We could also
 update this field to a third value when we count the success, to avoid
 potentially insane cases where we *again* try to count a new first hop for
 a circuit after counting the success for it.

 All of these unexpected cases should have logging for them.

 Also, this might mean that whatever hidden service circumstances that are
 triggering this bug won't be protected by the Path Bias defense until we
 figure out what the hell codepaths are involved. Hopefully the log lines +
 a repro script will help us find them.

 I'll get to work on a patch for this as soon as I can.

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