Opened 3 years ago

Closed 3 years 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 3 years ago.
Patch to disallow UseEntryGuards as 0 and UseBridges as 1
useentryguards_usebridges_fix.patch (3.6 KB) - added by neel 3 years ago.
Updated patch which includes regression test and ChangeLog updates
usebridges-1-useentryguards-0.patch (3.6 KB) - added by neel 3 years 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 3 years ago.
Updated patch (1/12/17) to disallow UseBridges 1 and UseEntryGuards 0

Download all attachments as: .zip

Change History (29)

comment:1 Changed 3 years ago by dgoulet

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

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

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

Changed 3 years ago by neel

Patch to disallow UseEntryGuards as 0 and UseBridges as 1

comment:4 Changed 3 years 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 3 years ago by nickm

Status: newneeds_review

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

Status: needs_reviewneeds_revision

comment:8 Changed 3 years ago by nickm

Keywords: review-group-13 added

comment:9 Changed 3 years ago by nickm

Reviewer: chelseakomlo

Changed 3 years ago by neel

Updated patch which includes regression test and ChangeLog updates

comment:10 Changed 3 years 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 3 years ago by nickm

Points: 1

comment:12 Changed 3 years ago by nickm

Status: needs_revisionneeds_review

comment:13 Changed 3 years 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 3 years ago by nickm

Status: needs_reviewneeds_revision

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

Changed 3 years ago by neel

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

comment:17 Changed 3 years 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 3 years 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 3 years ago by neel

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

comment:19 Changed 3 years 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 3 years ago by nickm

Status: needs_revisionneeds_review

comment:21 Changed 3 years ago by nickm

Owner: set to neel
Status: needs_reviewassigned

setting owner

comment:22 Changed 3 years ago by nickm

Status: assignedneeds_review

comment:23 Changed 3 years ago by chelseakomlo

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

comment:24 Changed 3 years ago by chelseakomlo

Status: needs_reviewmerge_ready

comment:25 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

lgtm too; merged!

Note: See TracTickets for help on using tickets.