Circuits through entry guards aren't distinguished on whether they originated locally
When a node is both a relay and a client, and another node extends a circuit through it to one of its entry guards, that circuit isn't distinguished from a circuit which originated locally. If entry_guard_register_connect_status() decides we should retry a different entry guard, the circuit will be killed in channel_do_open_actions(). This potentially could leak information about entry guards in circumstances which appear to be hard to exploit.
IRC log:
07:42 < athena> skruffy: what makes you believe 'channel_do_open_actions() can
leak of used guards if relay used as client' ?
07:43 < skruffy> if entry_guard_register_connect_status() failed it will close circuits
07:49 < athena> sooo - in other words, you think if an attacker can build a
circuit through a node N which is both a relay and a client to an
another node E, and then arrange for
entry_guard_register_connect_status() to fail, the attacker can
test a hypothesis about whether E is an entry guard for N?
07:52 < skruffy> something like that
07:53 < athena> okay; how would the attacker do that?
07:54 < athena> you may have something there; it seems fishy that killing pending
circuits on an entry guard we don't want to use doesn't
distinguish between locally originating circuits and circuits from
another relay - but i think if so it probably was there pre-
channels and i'm not convinced it's exploitable yet
07:59 < skruffy> yes it's pre-chans.
08:02 < athena> hmm, it looks like the only circumstance in which
entry_guard_register_connect_status() can fail is if this is a
first connection to a new entry guard *and* an old entry guard
which was offline has just come back
08:03 < athena> it doesn't seem that practical to exploit - for an attacker to try
to selectively manipulate connectivity to the old entry guards to
induce those conditions requires already knowing them
08:04 < nickm> It couldn't hurt to treat this as a bug that we should fix sooner
or later, though.
08:04 < athena> i think agree conceptually that locally originating circuits and
relay circuits should be distinguished and failing
entry_guard_register_connect_status() should only kill the local
ones, though
Proposed fix:
Modify circuit_n_chan_done() to take another possible parameter that notifies or_circuits of success but tells all other pending circuits to give up, and pass this from channel_do_open_actions() in case of entry_guard_register_connect_status() failing.