Opened 4 years ago

Closed 2 years ago

Last modified 15 months ago

#9819 closed defect (fixed)

Circuits through entry guards aren't distinguished on whether they originated locally

Reported by: andrea Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.17-rc
Severity: Keywords: tor-relay, 026-triaged-1, nickm-patch, 2016-bug-retrospective
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

Change History (10)

comment:1 Changed 3 years ago by nickm

  • Milestone changed from Tor: 0.2.5.x-final to Tor: 0.2.6.x-final

comment:2 Changed 3 years ago by nickm

  • Keywords tor-relay 026-triaged-1 added

comment:3 Changed 2 years ago by nickm

  • Status changed from new to needs_review

Fix in branch bug9819; please review?

comment:4 Changed 2 years ago by nickm

  • Keywords nickm-patch added

Add the nickm-patch keyword to a bunch of needs_review tickets.

comment:5 Changed 2 years ago by dgoulet

comment * Close_origin_circs is 1 if we should close all the origin circuits through

Should be "close_origin_circuits is 1 if we should close all..."

Apart from that, lgtm, it implements the proposed fix properly in my view.

comment:6 Changed 2 years ago by nickm

  • Resolution set to fixed
  • Status changed from needs_review to closed

Fixed and merged; thanks!

comment:7 Changed 2 years ago by arma

Woah! Good bug.

comment:8 Changed 15 months ago by nickm

  • Keywords 2016-bug-retrospective added

Mark bugs for 2016 bug retrospective based on hand-examination of changelogs for 0.2.5 onwards.

comment:9 Changed 15 months ago by nickm

Mark bugs for 2016 bug retrospective based on hand-examination of changelogs for 0.2.5 onwards.

comment:10 Changed 15 months ago by nickm

Mark bugs for 2016 bug retrospective based on hand-examination of changelogs for 0.2.5 onwards.

Note: See TracTickets for help on using tickets.