Opened 7 months ago

Closed 4 months ago

#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 6 months ago.
Patch to disallow UseEntryGuards as 0 and UseBridges as 1
useentryguards_usebridges_fix.patch (3.6 KB) - added by neel 5 months ago.
Updated patch which includes regression test and ChangeLog updates
usebridges-1-useentryguards-0.patch (3.6 KB) - added by neel 5 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 4 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 7 months ago by dgoulet

  • Keywords config added
  • Milestone set to Tor: 0.3.0.x-final

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

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

Changed 6 months ago by neel

Patch to disallow UseEntryGuards as 0 and UseBridges as 1

comment:4 Changed 6 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 6 months ago by nickm

  • Status changed from new to needs_review

comment:6 Changed 6 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 6 months ago by chelseakomlo

  • Status changed from needs_review to needs_revision

comment:8 Changed 6 months ago by nickm

  • Keywords review-group-13 added

comment:9 Changed 6 months ago by nickm

  • Reviewer set to chelseakomlo

Changed 5 months ago by neel

Updated patch which includes regression test and ChangeLog updates

comment:10 Changed 5 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 5 months ago by nickm

  • Points set to 1

comment:12 Changed 5 months ago by nickm

  • Status changed from needs_revision to needs_review

comment:13 follow-up: Changed 5 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 5 months ago by nickm

  • Status changed from needs_review to needs_revision

comment:15 Changed 5 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 5 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 5 months ago by chelseakomlo (previous) (diff)

Changed 5 months ago by neel

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

comment:17 follow-up: Changed 5 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 5 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 4 months ago by neel

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

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

  • Status changed from needs_revision to needs_review

comment:21 Changed 4 months ago by nickm

  • Owner set to neel
  • Status changed from needs_review to assigned

setting owner

comment:22 Changed 4 months ago by nickm

  • Status changed from assigned to needs_review

comment:23 Changed 4 months ago by chelseakomlo

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

comment:24 Changed 4 months ago by chelseakomlo

  • Status changed from needs_review to merge_ready

comment:25 Changed 4 months ago by nickm

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

lgtm too; merged!

Note: See TracTickets for help on using tickets.