GitLab

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.