Opened 5 years ago

Closed 3 years ago

#12450 closed defect (fixed)

Network down race condition might lead to primary guards getting skipped

Reported by: asn Owned by:
Priority: High Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-client, tor-guard
Cc: isis Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The behavior at:
https://gitweb.torproject.org/tor.git/blob/d064773595f1d0bf1b76dd6f7439bff653a3c8ce:/src/or/entrynodes.c#l776

tries to ensure that if our network is down, and connections to already existing guards fail, when the network is back up we will still try to connect to the guards on the top of our list.

It does so, by checking whether the guard we connected to is a newly added one. If it is so, it assumes that this guard was added because all our previous guards were found to be down, which might be a sign of the network being down. So if that's the case, the code walks our guard list and marks all the previous guards as to be retried.

This usually works fine, but consider the case where we have 60 guards in our guard list, and the network goes back up when we walk through the 50th guard. If that's the case, the code at https://gitweb.torproject.org/tor.git/blob/d064773595f1d0bf1b76dd6f7439bff653a3c8ce:/src/or/entrynodes.c#l776 doesn't get triggered because first_contact is not true, and we still stay connected to the 50th guard.

This sounds like a bug, since we should try to connect to our primary guards (the ones at the top of the list) even then.

Child Tickets

Change History (18)

comment:1 Changed 5 years ago by nickm

Keywords: 025-backport 024-backport added
Milestone: Tor: 0.2.5.x-final

asn, is there a reasonable fix for this that would fit into 0.2.5 or 0.2.4?

comment:2 Changed 5 years ago by asn

nick, I need to think more about reasonable fixes for this (that don't have unpredictable side effects)

FWIW, more information about this bug can be found in a related tor-dev post:
https://lists.torproject.org/pipermail/tor-dev/2014-June/007042.html

comment:3 Changed 5 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final
Priority: normalmajor

If we get a good fix written soon, it's a backport candidate for 0.2.5.

comment:4 Changed 5 years ago by anon

Two scenarios discussed in #tor-dev that induce this behavior, or lead to add more guards, are:

  • Delayed captive portal bypass where wifi network radio up leads actual connectivity past hotspot by many minutes.
  • Mobile network activity where continually sporadic upstream outages result in much churn and many guards.

comment:5 Changed 5 years ago by andrea

Keywords: 026-triaged-1 added

comment:6 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.7.x-final

comment:7 Changed 5 years ago by nickm

Status: newassigned

comment:8 Changed 5 years ago by nickm

Keywords: 027-triaged-1-out added

Marking triaged-out items from first round of 0.2.7 triage.

comment:9 Changed 5 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.???

Make all non-needs_review, non-needs_revision, 027-triaged-1-out items belong to 0.2.???

comment:10 Changed 4 years ago by isis

Cc: isis added
Severity: Normal

comment:11 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:12 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:13 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:14 Changed 3 years ago by nickm

Keywords: 027-triaged-in added

comment:15 Changed 3 years ago by nickm

Keywords: 027-triaged-in removed

comment:16 Changed 3 years ago by nickm

Keywords: 027-triaged-1-out removed

comment:17 Changed 3 years ago by nickm

Keywords: 026-triaged-1 removed

comment:18 Changed 3 years ago by nickm

Keywords: 025-backport 024-backport removed
Resolution: fixed
Status: assignedclosed

This was a big part of the point of proposal 271, implemented in 0.3.0.

Note: See TracTickets for help on using tickets.