Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#18483 closed enhancement (fixed)

Clients should always tunnel connections, and never fall back to a DirPort

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: must-fix-before-028-rc, TorCoreTeam201605
Cc: Actual Points: 2 hours
Parent ID: #18809 Points: 1
Reviewer: nickm Sponsor: None

Description

Otherwise, client privacy might be compromised.

Child Tickets

TicketTypeStatusOwnerSummary
#18921defectclosedFix IPv6 bridge client directory address selection
#18929defectclosedFix selection of directories with non-preferred address families
#18962enhancementclosedMerge feature 18483 cleaup commits to master

Change History (18)

comment:1 Changed 3 years ago by nickm

Milestone: Tor: 0.2.9.x-finalTor: 0.2.8.x-final

I'd even take this in 0.2.8.

comment:2 Changed 3 years ago by dgoulet

Sponsor: None
Status: newneeds_information

Does this apply also to more cases? I see in should_use_directory_guards():

  if (options->DownloadExtraInfo || options->FetchDirInfoEarly ||
      options->FetchDirInfoExtraEarly || options->FetchUselessDescriptors)
    return 0;

At that point, we know it's a client, should it simply return 1 all the time instead so we always use a dir guard? (Also, am I looking at the right place? :)

comment:3 Changed 3 years ago by nickm

Directory guards isn't the same as tunneled connections. Directory guards means "Use a fixed node (a guard node!) for directory connections, just as with regular circuits." Tunneled connections mean to use a tor connection and BEGINDIR, instead of simply connecting to an HTTP dirport.

Or do I misunderstand the question?

comment:4 Changed 3 years ago by teor

Keywords: must-fix-before-028-rc added

Roger's fixes in #18809 might mean that we need this in 0.2.8 as well.

comment:5 Changed 3 years ago by teor

Status: needs_informationnew
Summary: Clients should always tunnel connections, even if FetchDirInfoExtraEarly is setClients should always tunnel connections, and never fall back to a DirPort

When a Tor client selects a directory mirror with an ORPort it can't reach, it uses the DirPort. Instead, clients should only select relays with ORPorts they can reach, and should never use DirPorts.

Clients (and onion services, and bridges(?)) should never use the following dir_indirection_t:

  • DIRIND_DIRECT_CONN
  • DIRIND_ANON_DIRPORT

We should modify the meaning of DIRIND_ONEHOP so tor only falls back when it is in public_server_mode():

Default: connect over a one-hop Tor circuit. Only fall back to direct connection if you are a non-bridge relay or authority.

Is this too big a change to make just before the 0.2.8 release?
I'm concerned because we need it to make #18809 simpler, and that's a fix on a feature that's in 0.2.8.

Last edited 3 years ago by teor (previous) (diff)

comment:6 Changed 3 years ago by teor

Actual Points: 2 hours
Parent ID: #18809
Points: small
Status: newneeds_review
Type: defectenhancement

Please see my branch feature18483 on https://github.com/teor2345/tor.git

comment:7 Changed 3 years ago by teor

Keywords: CoreTorTeam201605 added

comment:8 Changed 3 years ago by teor

I've added 3 more commits to this branch, to fix #18929.

comment:9 Changed 3 years ago by teor

Added further commits to this branch as part of #18929, including a unit test from isis.

comment:10 Changed 3 years ago by nickm

Keywords: TorCoreTeam201605 added

Calling all non-needs_information tickets for May.

comment:11 Changed 3 years ago by teor

Keywords: TorCoreTeam201605 removed

I split #18483, #18921, and #18929 into three separate branches for 0.2.8, and a branch for 0.2.9.
Please see feature18483-028 on https://github.com/teor2345/tor.git for this issue.
feature18483-028 depends on bug18921 and bug18929-v2. It needs to be merged to 0.2.8.
I split off a branch which contains cosmetic changes only into #18962.

Replying to nickm from #18921:

Initial Questions:

  • e726459eaf57ffe98c36730af58287229c9531d6: Are we really really sure that every possible case of the big complicated function directory_fetches_from_authorities() should be replaced with !directory_must_use_begindir() ?

Calling this NM1.

e726459eaf57ffe98c36730af58287229c9531d6 is Make clients always use begindir for directory requests.
After the split, it is 0de9faa402bebc98a58c9f65149e72e598926184 in feature18483-028. The code is unchanged from the original commit.

This function determines whether one-hop connections use a DirPort or begindir over ORPort.

Previously, our logic was "if connecting to an authority, don't use begindir".
(Perhaps this was to reduce load? If so, the fallback directories feature should reduce load on the authorities, more than enough to compensate for the increased load from begindir. Also, some relays will not use begindir now, when they used to before.)

Now, our logic is, "if you are a client, or hidden service, or bridge, use begindir".

Here are the cases from directory_fetches_from_authorities, and how they change:

  • FetchDirInfoEarly
    • changes clients to use begindir
    • previously, anything with FetchDirInfoEarly 1 used DirPort, which is bad for clients
    • now, clients with FetchDirInfoEarly will use begindir, which is what we want
    • now, relays with FetchDirInfoEarly will use DirPort, which is also what we want
  • BridgeRelay
    • stays the same
    • unchanged, bridge relays will continue to use begindir
  • server_mode() and don't know our address
    • stays the same
    • previously, relays without an IP address used DirPort to get an IP address
    • now, all relays will use DirPort (including relays without an IP address)
  • !dir_server_mode(options) && !refuseunknown
    • changes some relays to use DirPort
    • previously, non-directory non-exits used to use begindir
    • now, all relays will use DirPort (including non-directory non-exit relays)
    • now, all clients will use begindir
  • !server_mode(options) || !advertised_server_mode()
    • changes some relays to use DirPort
    • previously, non-servers or non-advertised servers used to use begindir
    • now, all public servers will use DirPort
    • now, all clients and bridges will use begindir
  • !router_get_my_routerinfo || (!supports_tunnelled_dir_requests && !refuseunknown)
    • changes some relays to use DirPort
    • previously, tors without descriptors (clients), and non-directory non-exits used to use begindir
    • now, all relays will use DirPort (including non-directory non-exit relays)
    • now, all clients will use begindir
  • default
    • stays the same
    • previously, directories and exits used to use DirPort
    • now, all relays will use DirPort (including directories and exits)

Edit: quote code for readability on trac

Last edited 3 years ago by teor (previous) (diff)

comment:12 Changed 3 years ago by bugzilla

Keywords: TorCoreTeam201605 added; CoreTorTeam201605 removed

comment:13 Changed 3 years ago by nickm

NOTE to nickm: I'll have to rebase this one last time since I went and squashed and merged #18921. That's on me, though.

0de9faa402bebc98a58c9f65149e72e598926184:

  • begindir_reason will never be set! You need to declare the reason argument in directory_command_should_use_begindir as "const char * *", not const char *.

Otherwise, this looks good.

comment:14 Changed 3 years ago by nickm

Reviewer: nickm

comment:15 Changed 3 years ago by nickm

Oh, one last thing: Did you forget to push feature18483-028? All I see is feature18483 and feature18483-cleanup.

comment:16 in reply to:  13 Changed 3 years ago by teor

Replying to nickm:

NOTE to nickm: I'll have to rebase this one last time since I went and squashed and merged #18921. That's on me, though.

I did the rebase into my branch feature18483-028-v2 because of changes in #18929:

Old commit / New commit / Name
0de9faa a5b5447 Make clients always use begindir for directory requests
15fe5e8 950eae4 Make clients only select directories with reachable ORPorts
aec84c5 8febffe Only choose directory DirPorts on relays

The following commits were deleted because this no longer depends on #18929:
8adbfab Merge branch 'bug18921' into feature18483-028
1100a70 Revert "Remove must_have_or from router_has_non_preferred_address_rs"
5c8fde2 Only fall back to nodes with valid ORPorts when using begindir

0de9faa402bebc98a58c9f65149e72e598926184:

  • begindir_reason will never be set! You need to declare the reason argument in directory_command_should_use_begindir as "const char * *", not const char *.

Oops! Pointer handling error.
Calling this NM1.
9f01dc4 fixup! Make clients always use begindir for directory requests

Otherwise, this looks good.

Let's get it merged, I am much happier with it now it's disentangled and #18929 has shrunk.
(And it merges fine on top of both maint-0.2.8 and the new #18929.)

See #18962 for a rebased feature18483-cleanup-v2, which can go in 0.2.9.

comment:17 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merging!

comment:18 Changed 3 years ago by isabela

Points: small1
Note: See TracTickets for help on using tickets.