#20502 closed defect (fixed)

Setting UseBridges=1 UseEntryGuards=0 means you bypass your bridges

Reported by: arma Owned by: neel
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: config, review-group-14
Cc: Actual Points:
Parent ID: Points: 1
Reviewer: chelseakomlo Sponsor:

Description

If you set UseBridges=1, it's because you wanted to use bridges.

But if you also happen to set UseEntryGuards to 0, then in choose_good_entry_server() we do

  if (state && options->UseEntryGuards &&
      (purpose != CIRCUIT_PURPOSE_TESTING || options->BridgeRelay)) {
    /* This request is for an entry server to use for a regular circuit,
     * and we use entry guard nodes.  Just return one of the guard nodes.  */
    return choose_random_entry(state);
  }

and we end up skipping that section because UseEntryGuards is 0. The result is that we make normal 3-hop circuits through normal relays.

I think the fix is that in config.c when we're validating the config, we need to not let the user proceed if UseBridges is 1 yet UseEntryGuards is 0.

Child Tickets

Attachments (4)

0001-Disallow-setting-UseEntryGuards-to-0-and-UseBridges-.patch (1010 bytes) - added by neel 12 months ago.
Patch to disallow UseEntryGuards as 0 and UseBridges as 1
useentryguards_usebridges_fix.patch (3.6 KB) - added by neel 11 months ago.
Updated patch which includes regression test and ChangeLog updates
usebridges-1-useentryguards-0.patch (3.6 KB) - added by neel 11 months ago.
Updated patch (1/8/17) to disallow UseBridges 1 and UseEntryGuards 0
0001-Disallow-setting-UseBridges-to-1-and-UseEntryGuards-.patch (2.9 KB) - added by neel 10 months ago.
Updated patch (1/12/17) to disallow UseBridges 1 and UseEntryGuards 0

Download all attachments as: .zip

Change History (29)

comment:1 Changed 13 months ago by dgoulet

Keywords: config added
Milestone: Tor: 0.3.0.x-final

comment:2 Changed 13 months ago by teor

I propose an alternate fix: set UseEntryGuards to 1 if UseBridges is 1, and warn the user.

We already have UseEntryGuards_opt or something, so this won't overwrite the config value if we write out the config.

comment:3 Changed 13 months ago by nickm

I'm fine with either idea. Implementor gets to pick. :)

Changed 12 months ago by neel

Patch to disallow UseEntryGuards as 0 and UseBridges as 1

comment:4 Changed 12 months ago by neel

I have made a patch to disallow UseEntryGuards as 0 and UseBridges as 1. Tell me what you think about this.

Thanks,
Neel Chauhan
===
https://www.neelc.org/

comment:5 Changed 12 months ago by nickm

Status: newneeds_review

comment:6 Changed 12 months ago by chelseakomlo

Looks good!

A couple recommended changes:

  1. The message could be a bit more about the intent of this validation. So rather "You cannot set UseBridges to 1 and UseEntryGuards to 0", it could be something like "Setting UseBridges requires also setting UseEntryGuards"
  2. Adding a unit test to validate this behavior
  3. Adding a changes file

comment:7 Changed 12 months ago by chelseakomlo

Status: needs_reviewneeds_revision

comment:8 Changed 12 months ago by nickm

Keywords: review-group-13 added

comment:9 Changed 12 months ago by nickm

Reviewer: chelseakomlo

Changed 11 months ago by neel

Updated patch which includes regression test and ChangeLog updates

comment:10 Changed 11 months ago by neel

I have a patch which (hopefully) fixes it. Keep in mind that I had to add "UseEntryGuards 1" to some parts of test_options_validate_use_bridges in order to prevent some errors.

comment:11 Changed 11 months ago by nickm

Points: 1

comment:12 Changed 11 months ago by nickm

Status: needs_revisionneeds_review

comment:13 Changed 11 months ago by nickm

When I apply this, I get a new unit test failure:

options/validate__reachable_addresses: [forking] 
  FAIL src/test/test_options.c:1876: assert(ret OP_EQ 0): -1 vs 0
  [validate__reachable_addresses FAILED]

Also, why is it necessary to add "UseEntryGuards 1" to the unit tests there? "UseEntryGuards 1" is already supposed to be the default. Are our tests getting the defaults wrong?

comment:14 Changed 11 months ago by nickm

Status: needs_reviewneeds_revision

comment:15 Changed 11 months ago by nickm

Keywords: review-group-14 added; review-group-13 removed

And that's all for review-group-13.

comment:16 in reply to:  13 Changed 11 months ago by chelseakomlo

Replying to nickm:

When I apply this, I get a new unit test failure:

options/validate__reachable_addresses: [forking] 
  FAIL src/test/test_options.c:1876: assert(ret OP_EQ 0): -1 vs 0
  [validate__reachable_addresses FAILED]

Yes, I get the same failure.

Also, why is it necessary to add "UseEntryGuards 1" to the unit tests there? "UseEntryGuards 1" is already supposed to be the default. Are our tests getting the defaults wrong?

It looks like UseEntryGuards is not set in TEST_OPTIONS_DEFAULT_VALUES.

Last edited 11 months ago by chelseakomlo (previous) (diff)

Changed 11 months ago by neel

Updated patch (1/8/17) to disallow UseBridges 1 and UseEntryGuards 0

comment:17 Changed 11 months ago by neel

I have an updated patch which I have just submitted. Please tell me what you think about this.

comment:18 in reply to:  17 Changed 11 months ago by cypherpunks

Replying to neel:

I have an updated patch which I have just submitted. Please tell me what you think about this.

Please read the coding standards document with regards to logging changes. Instead of modifying the ChangeLog file directly, changes are logged with separate files which are placed inside the changes directory.

Changed 10 months ago by neel

Updated patch (1/12/17) to disallow UseBridges 1 and UseEntryGuards 0

comment:19 Changed 10 months ago by neel

Under the "Updated patch (1/8/17) to disallow UseBridges 1 and UseEntryGuards 0" label, I have an updated patch which logs the change as a file changes/bug20502.

comment:20 Changed 10 months ago by nickm

Status: needs_revisionneeds_review

comment:21 Changed 10 months ago by nickm

Owner: set to neel
Status: needs_reviewassigned

setting owner

comment:22 Changed 10 months ago by nickm

Status: assignedneeds_review

comment:23 Changed 10 months ago by chelseakomlo

Thanks for this patch and making these updates, looks good to me & tests are passing!

comment:24 Changed 10 months ago by chelseakomlo

Status: needs_reviewmerge_ready

comment:25 Changed 10 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

lgtm too; merged!

Note: See TracTickets for help on using tickets.