Opened 6 years ago

Last modified 22 months ago

#8163 new defect

It is no longer deterministic which Sybils we omit

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.2.4.10-alpha
Severity: Normal Keywords: tor-dirauth sybil voting needs-spec
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

It seems that each dir auth is voting for its favorite two relays, in the case of Sybils. The result is that none of them get listed in the consensus (as opposed to the "two of them" that our design says do).

I think the issue is in compare_routerinfo_by_ip_and_bw_().

Maybe it's here:

  node_first = node_get_by_id(first->cache_info.identity_digest);
  node_second = node_get_by_id(second->cache_info.identity_digest);
  first_is_running = node_first && node_first->is_running;
  second_is_running = node_second && node_second->is_running;

The "is_running" part here is suspicious -- if we cleared flags from some of them last time through the loop, does that change the order of picking them this time through the loop?

(Also, the comments for the function don't mention comparing identity digests as the last resort. They probably should.)

Child Tickets

Change History (21)

comment:1 Changed 6 years ago by nickm

When we have too many Tor nodes at the same IP -- should we even list any of them?

comment:2 Changed 6 years ago by arma

If somebody runs a Tor relay on my shared apartment building IP address, I can destroy its history right now by running a few alleged relays of my own. That's not so good.

comment:3 Changed 6 years ago by arma

Also, if I publish two relay descriptors claiming to have your IP address, even though I have nothing to do with your IP address, is that enough to make all three of us Sybils?

comment:4 Changed 6 years ago by arma

<bobnomnom> 8163 bug probably not about comparing but auth votes differently .9 and .10
<bobnomnom> need to compare all votes

comment:5 Changed 6 years ago by arma

boboper thinks this bug might be a false alarm

comment:6 in reply to:  5 ; Changed 6 years ago by arma

Replying to arma:

boboper thinks this bug might be a false alarm

But it still seems to me that there's something to it: if we're clearing the Running flag on some of the Sybils each time we vote, and also we're setting it to 1 whenever we finish a reachability test, and the reachability tests are spread out, that function isn't going to return the same way every vote, right?

comment:7 in reply to:  6 Changed 6 years ago by arma

Replying to arma:

if we're clearing the Running flag on some of the Sybils each time we vote, and also we're setting it to 1 whenever we finish a reachability test

If this bug turns out to be an issue, one workaround might be to look at node->last_reachable rather than node->is_running.

comment:8 Changed 6 years ago by nickm

If we compare node->last_reachable after node->is_running, we should pretty much just ignore all the other fields (advertised bw, identity), since we'll almost never get a tie on last_reachable.

comment:9 in reply to:  8 Changed 6 years ago by arma

Replying to nickm:

If we compare node->last_reachable after node->is_running, we should pretty much just ignore all the other fields (advertised bw, identity), since we'll almost never get a tie on last_reachable.

I think that would be extra bad, because it would make each dir auth inconsistent about which sybils it picks, and worse, each dir auth would be differently inconsistent. We want something where the globally emergent behavior is consistent.

comment:10 Changed 6 years ago by nickm

So, do we in fact have an idea of how to fix this? We could try to grandfather in whichever nodes are currently in the consensus directory, but as soon as nodes drop out once, we'll be out of luck. We could go with whichever node has been around the longest -- the one with the highest total_weighted_time or highest weighted_uptime in its rephist. Any other thoughts?

comment:11 Changed 6 years ago by nickm

We could add a new thing that we track in or_history_t; maybe 'how many consensuses has this been in'?

comment:12 Changed 6 years ago by nickm

(But unless there's a clear solution, I suggest deferring to 0.2.5.)

comment:13 Changed 6 years ago by nickm

Keywords: 024-deferrable added

comment:14 Changed 6 years ago by nickm

Keywords: 024-deferrable removed
Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

I'm deferring this to 0.2.5; authorities tend to track alpha codebases anyway, and we don't currently know a great answer for this.

comment:15 Changed 6 years ago by nickm

Over at #8710, Roger says:

If your adversary can run a relay on your IP address, that's bad news as you say.

In that case let's just break ties based on fingerprint, and assume that the business about adversaries running relays on your IP is out-of-scope?

comment:16 Changed 5 years ago by nickm

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

comment:17 Changed 2 years ago by teor

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

Milestone renamed

comment:18 Changed 2 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:19 Changed 22 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:20 Changed 22 months ago by dgoulet

Keywords: tor-dirauth added; tor-auth removed

Turns out that tor-auth is for directory authority so make it clearer with tor-dirauth

comment:21 Changed 22 months ago by nickm

Keywords: sybil voting needs-spec added
Severity: Normal
Note: See TracTickets for help on using tickets.