Opened 21 months ago

Closed 21 months ago

Last modified 16 months ago

#17772 closed defect (fixed)

We don't consider the Guard flag when picking a directory guard

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version:
Severity: Major Keywords: regression, 024-backport, 025-backport, 026-backport, 2016-bug-retrospective
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Back in Tor 0.2.3, when we wanted a new guard, we ended up calling

    node = choose_good_entry_server(CIRCUIT_PURPOSE_C_GENERAL, NULL);

which called

  choice = router_choose_random_node(excluded, options->ExcludeNodes, flags);

with flags that included CRN_NEED_GUARD, meaning that inside router_choose_random_node(), when it called

  router_add_running_nodes_to_smartlist(sl, allow_invalid,
                                        need_uptime, need_capacity,
                                        need_guard, need_desc);

we would check

    if (node_is_unreliable(node, need_uptime, need_capacity, need_guard))
      continue;

In sum, we would avoid considering relays that didn't have the Guard flag, when picking a new guard.

However, starting with Tor 0.2.4, the logic inside add_an_entry_guard() is now:

  } else if (!for_directory) {
    node = choose_good_entry_server(CIRCUIT_PURPOSE_C_GENERAL, NULL);
    if (!node)
      return NULL;
  } else {
    const routerstatus_t *rs;
    rs = router_pick_directory_server(MICRODESC_DIRINFO|V3_DIRINFO,
                                      PDS_FOR_GUARD);
    [...]
  }

You would think router_pick_directory_server() would look at PDS_FOR_GUARD and choose to avoid relays that don't have the Guard flag. Alas, the only use of for_guard in this function is:

    if (for_guard && node->using_as_guard)
      continue; /* Don't make the same node a guard twice. */

I think we need to add another clause into this "if it's unsuitable, continue" list inside router_pick_directory_server_impl().

Right now this bug bites us because the first thing a bootstrapping Tor client does is fetch dir info, so that's when it notices it doesn't have enough guards, so the first three guards it picks are picked via router_pick_directory_server(), and then later when we want to use a guard for a normal circuit, we already find that we have workable guards.

In particular, the bug went in with commit 0c4210f (which looks like it went into 0.2.4.8-alpha).

Now, a separate issue here is that Nick and I were both under the impression that a client will avoid using one of its guards if it loses the Guard flag. That appears to not be happening here either. I'll leave that for a separate bug. :)

Thanks to Mohsen Imani for discovering and reporting!

Child Tickets

Attachments (1)

0001-Ensure-node-is-a-guard-candidate-when-picking-a-dire.patch (1.1 KB) - added by arlolra 21 months ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 21 months ago by arma

In terms of impact, we are still sticking with our guards for the intended periods (as far as I can tell), so from a security perspective this isn't the end of the world. Effectively it's like all the relays have the Guard flag.

From a performance perspective, we're probably doing our load balancing suboptimally, since we're shifting load onto relays that don't have the Guard flag, and also shifting load away from relays that do have the Guard flag.

comment:2 in reply to:  description Changed 21 months ago by arma

Replying to arma:

Now, a separate issue here is that Nick and I were both under the impression that a client will avoid using one of its guards if it loses the Guard flag. That appears to not be happening here either. I'll leave that for a separate bug. :)

Opened as #17773. My initial thoughts are that actually the behavior is ok as-is.

comment:3 Changed 21 months ago by teor

Could this be contributing to exits being overloaded?

comment:4 Changed 21 months ago by arlolra

Status: newneeds_review

comment:5 Changed 21 months ago by teor

The attached patch is a one-line change that looks like it fixes this issue.
We should test to be sure.
(And consider how far to backport it.)

comment:6 Changed 21 months ago by arma

Patch looks good to me.

Here's a go at a changes entry:

  o Major bugfixes (guard selection):
    - Actually look at the Guard flag when selecting a new directory
      guard. When we implemented the directory guard design, we
      accidentally started treating all relays as if they have the Guard
      flag during guard selection, leading to weaker anonymity and worse
      performance. Fixes bug 17222; bugfix on 0.2.4.8-alpha. Discovered
      by Mohsen Imani.

I think we should backport it starting from 0.2.4.x, and then only put out new stable releases in cases where we know that a package maintainer actually wants to move to it (e.g. hopefully Tor 0.2.6, definitely 0.2.7, but probably not 0.2.4 or 0.2.5).

comment:7 Changed 21 months ago by asn

Please check branch bug17772 in my repo.

I tested that arlo's patch works and I also included roger's changes file.
I slightly tweaked arlo's patch to do the check in a separate line as well.

Cheers to everyone!

Last edited 21 months ago by asn (previous) (diff)

comment:8 Changed 21 months ago by nickm

Keywords: regression 024-backport 025-backport 026-backport added

comment:9 Changed 21 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks fine, rebased and squashed onto maint-0.2.4 as "bug17772_024". Merging to 0.2.4 and forwards.

comment:10 Changed 16 months ago by nickm

Keywords: 2016-bug-retrospective added

Marking these tickets (based on severity and hand-review) for inclusion in 2016 bug retrospective

comment:11 Changed 16 months ago by nickm

Mark more tickets for bug retrospective based on hand-review of changelogs from 0.2.5 onwards.

Note: See TracTickets for help on using tickets.