Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#19163 closed defect (implemented)

Make sure clients almost always use ntor

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: rsos, tor-hs, TorCoreTeam201608, review-group-7
Cc: Actual Points: 6
Parent ID: Points: 2.0
Reviewer: nickm Sponsor:

Description (last modified by teor)

Update:
All clients should use ntor for almost everything
The only exceptions are during the hidden service protocol. Client to intro and hidden service to rendezvous should still be able to use TAP.


isis asks in #1744:

 // XXXprop#188 Why do we not care if it's ntor if it's only one hop?

I think it's because one-hop circuits were originally used only for directory fetches, which are authenticated by signature (and not private).

But with RSOS, maybe we should require all one-hop paths to have ntor. I need to talk to a cryptographer about this.

See the populate_cpath function for details.

Child Tickets

Change History (24)

comment:1 Changed 3 years ago by isis

If we're worried that one-hop paths could be used, e.g. by the RSOS to upload its descriptor to the HSDir, then using the TAP handshake for said upload would reduce the guarantees of authenticity of the HSDir to RSA-1024. So yes, since it's trivial to do, there's no performance decrease, and nearly non-existent anonymity decrease to exclude TAP, we should exclude TAP for RSOSes.

comment:2 Changed 3 years ago by teor

Keywords: tor-hs TorCoreTeam201607 added
Parent ID: #17178

comment:3 Changed 3 years ago by teor

tor-spec.txt says "[The ntor handshake was added in Tor 0.2.4.8-alpha.]"
We no longer recommend versions before 0.2.4.26 or 0.2.5.11.
So let's simplify this patch by making sure every circuit, even single-hop circuits, has at least one relay that supports ntor.

That's the easy part.
And it's a nice defence against protocol downgrade attacks.

This has the following implications:

  • bridges must support ntor (we should warn if we connect to a bridge that doesn't support ntor)
  • guards must support ntor (we should only select guards with ntor)
  • directory guards must support ntor (we should only select directory guards with ntor)
  • we should make sure that directories we select from the consensus have ntor
  • we should make sure that fallbacks have ntor (in the fallback script)
    • this ensures directories we select from the hard-coded authority and fallback lists have ntor

comment:4 Changed 3 years ago by teor

And:

  • tor2web should choose rendezvous points that have ntor
  • single onion services should choose introduction points that have ntor

comment:5 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.2.9.x-final
Points: 0.51.0

nickm: I think 029 for all of it, if there's time.

And:

  • nickm: also authorities should reject all descriptors that include only a TAP key. (unless they do already)
  • never select a TAP-only router for any circuit
  • make sure every extend actually uses ntor (note that the hidden service protocol itself uses TAP, even if the circuits don't)

If we rebuild the fallback list for 0.2.9 in #18828, it will fix:

  • we should make sure that fallbacks have ntor (in the fallback script)

because all recommended tor versions have ntor.

The are edge cases where relays lie about their version, or lie about having an ntor key, or lie about supporting ntor, or never actually use the ntor key. But we'll catch those with the circuit/extend checks.

comment:6 Changed 3 years ago by teor

Actual Points: 3
Status: newneeds_revision

I've made the following changes:

authorities:

  • should reject all descriptors that include only a TAP key (unless they do already)
    • dirserv_get_status_impl already rejects versions older than 0.2.4.18-rc, and ntor was introduced in 0.2.4.8-alpha. So there should be no TAP-only routers in the consensus.
    • But we don't actually check if each descriptor has an ntor key in dirserv_router_get_status.

2171a79 rejects descriptors where onion_curve25519_pkey is NULL.
This comes before the version check. So the message for very old relays will change.

817ee49 is a fixup which also rejects descriptors where the ntor key is all zero (this is the actual check the client performs, so we should replicate it on the authorities)

relays:

  • bridges must support ntor
  • guards / directory guards must support ntor

329ff78 relays make sure their own descriptor has an ntor key

clients (and client code):

descriptor downloads:

6cc2d2c Clients no longer download descriptors for relays without ntor

relay selection:

  • we should only select guards with ntor
  • we should only select directory guards with ntor
  • we should only select directories with ntor
  • we should only select exits with ntor
  • hidden service clients should choose rendezvous points that have ntor
  • hidden services should choose introduction points that have ntor

e3a0b50 Clients avoid choosing nodes that can't do ntor

Nodes that have a version that's too old, or a descriptor without an ntor key, are not considered running. (Nodes without versions or descriptors are permitted.)

path selection:

  • never select a TAP-only router in any hop in any circuit

c341848 Make sure every hop in every path supports ntor
We check when selecting nodes for the path, and when creating extend_infos for the path.
We can't check in extend_info_new, because some extend_infos legitimately don't know the ntor key.

d83b655 Make sure every relay in a path supports ntor, not just one
Change the definition of "path supports ntor" to "all relays support ntor" (it was "at least one relay supports ntor")

circuit building:

  • make sure every extend actually uses ntor, to protect against downgrade attacks
    • stop and warn if we connect to a bridge that doesn't support ntor
      • this happens automatically as part of circuit-building
    • note that some single-hop connections (such as bootstrap connections) still use CREATE_FAST
    • hidden services can use ntor if the intro/rend points are in the consensus

44e1d19 Deprecate UseNTorHandshake, assume all hops use ntor
0ceb4b2 Clients require hidden service intro points to have an ntor key
1630e1d Hidden services require rend points to have an ntor key

This code is in my branch reject-tap-v2 on https://github.com/teor2345/tor.git, but it needs revision.

Currently, if intro or rend points aren't in the consensus the client or hidden service has, they can't be used (because there is no ntor onion key available for them).

Therefore:

  • intro and rend points should check if they are missing either the ntor or the TAP key, and if either are missing, look up both in the consensus
  • intro and rend points that aren't in the consensus should be available via TAP
Last edited 3 years ago by teor (previous) (diff)

comment:7 Changed 3 years ago by teor

I have a working version of this in my branch reject-tap-v2.
But I'd like to squash the last half of the commits before it gets merged.

comment:8 Changed 3 years ago by teor

Actual Points: 35
Status: needs_revisionneeds_review

Please see my branch reject-tap-v3-rebased on https://github.com/teor2345/tor.git
I am happy to take reviews through gitlab at https://gitlab.com/teor/tor/merge_requests/1/diffs

It makes the following changes:

  • Relays make sure their own descriptor has an ntor key.
  • Authorites no longer trust the version a relay claims (if any), instead, they check specifically for an ntor key.
  • Clients avoid downloading a descriptor if the relay version is too old to support ntor.
  • Client code never chooses nodes without ntor keys: they will not be selected during circuit-building, or as guards, or as directory mirrors, or as introduction or rendezvous points.
  • Circuit-building code assumes that all hops can use ntor, except for rare hidden service protocol cases.
  • Clients opportunistically upgrade to intro point ntor onion keys in relay descriptors. If they do not have a relay descriptor, they fall back to using the intro point TAP onion key in the hidden service descriptor.
  • Hidden services opportunistically upgrade to rend point ntor onion keys in relay descriptors. If they do not have a relay descriptor, they fall back to using the rend point TAP onion key in the INTRODUCE cell.

Other tickets:

There's a single onion service stub function in this code that will conflict with #17178, whichever is merged later will have to delete it, or get a compile error. (And it says so in the function comment.)

I split off #19649, because there's no ntor onion key link specifier.
This changes some code that's related to hidden service reachability (#17945, #19662, and #19663).

comment:9 Changed 3 years ago by teor

Let's try that again with reject-tap-v5 in https://gitlab.com/teor/tor/merge_requests/2

I moved the hidden service rend ntor opportunistic upgrade to #17178, because it will be easier to merge that way.

comment:10 Changed 3 years ago by teor

Description: modified (diff)
Summary: Maybe RSOS single-hop circuits should always have ntorMake sure clients almost always use ntor

comment:11 Changed 3 years ago by teor

I added some fixups to reject-tap-v5 on both github and gitlab:

d790fee fixup! Client & HS ignore UseNTorHandshake, all non-HS handshakes use ntor

  • Allow client intro and HS rend to use ntor, TAP, and CREATE_FAST, in that order

5bc3335 fixup! Client intro opportunistically upgrades to ntor

  • Allow client intro to use TAP when none of the intro points are in the consensus
  • Allow Tor2web client intro to use CREATE_FAST when making a direct connection, none of the intro points are in the consensus, and none have TAP keys in the HS descriptor

Gitlab is at:
https://gitlab.com/teor/tor/merge_requests/2

comment:12 Changed 3 years ago by nickm

Keywords: review-group-6 added

comment:13 Changed 3 years ago by nickm

Reviewer: nickm
Status: needs_reviewneeds_revision

Actual-review-points: .1

Added some comments on the gitlab page. Issues seem minor. I'm most concerned here about the testing of this, but if you've tested that it doesn't break hidden services, I believe it.

(Have you looked at the test coverage for the lines that changed?)

comment:14 Changed 3 years ago by nickm

Keywords: review-group-7 added; review-group-6 removed

All review-group-6 tickets have had at least one review; moving them to 7.

comment:15 Changed 3 years ago by teor

Keywords: TorCoreTeam201608 added; TorCoreTeam201607 removed
Points: 1.02.0
Status: needs_revisionneeds_review

I've made an update based on nickm's review, and pushed it to github and gitlab.
branch reject-tap-v5 on https://github.com/teor2345/tor.git
https://gitlab.com/teor/tor/merge_requests/2

I also folded the code from #19923 into this branch. I needed to add a stub for rend_service_allow_direct_connection to get it to compile.

Sorry for the swapping back and forth.

I still need to look at the test coverage of the changed lines, and do some more testing using mixed-version chutney networks, in particular:

  • basic-min (but with half/half old/new tor versions)
  • hs-min (but with half/half old/new tor versions)

comment:16 Changed 3 years ago by teor

Testing this is difficult!

I have merged two new chutney networks to chutney master, hs-min-mixed and basic-min-mixed. They are an even mix of old and new tor versions, and will likely fail if old and new versions do incompatible things. I've tested them with this patch and Tor 0.2.7.6.

I also want to write unit tests for the cases where:

  • the service selects an intro point that's not in the client consensus, and
  • the client selects a rendezvous point that's not in the service consensus

It is very hard to give a client and a relay different consensuses in chutney, so unit tests will have to do. Perhaps I should check code coverage first, so I know where to focus my efforts.

comment:17 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

Re-review:

  • I worry about the security of the opportunistic upgrade stuff. It has the potential to enable epistemic attacks.

Otherwise stuff looks good.

comment:18 in reply to:  17 Changed 3 years ago by teor

Replying to nickm:

Re-review:

  • I worry about the security of the opportunistic upgrade stuff. It has the potential to enable epistemic attacks.

Otherwise stuff looks good.

Yes, I have the same concerns - you could probe a client / hidden service to find out which relays it knows about. And it's hard to test.

Here are our options:

  • We could control opportunistic upgrades with a consensus parameter that we only switch on when 0.2.8 is no longer recommended. But this means the code won't be tested.
  • We could remove opportunistic upgrades entirely, and only kill off TAP when we kill off the old hidden service protocol.

And separately, for Single Onion Services / Tor2web:

  • We could always do opportunistic upgrades, because it doesn't matter if anyone knows what consensus a Single Onion Service or Tor2web client has, and it's more important to protect the single-hop link with ntor rather than using the vulnerable TAP protocol.
  • Or we could go with either of the above options.

What do you think?

comment:19 Changed 3 years ago by teor

Actual Points: 56
Status: needs_revisionneeds_review

nickm said on IRC to just get rid of the opportunistic upgrades.

Turns out that rend_client_get_random_intro_impl() already inadvertently upgrades to ntor in the following circumstances:

  • the HS descriptor doesn't contain a TAP onion key
  • the node can be found by nickname or fingerprint in the client's consensus

I've left that code as-is, but I can easily remove it if you'd like.
I think we should be consistent between client intro and service rend, and never upgrade from the consensus. (It certainly doesn't break modern clients or hidden services.)

Please see my branch reject-tap-v6 on https://github.com/teor2345/tor.git
Or on gitlab at https://gitlab.com/teor/tor/merge_requests/7

comment:20 Changed 3 years ago by nickm

I think we should consider _that_ ntor-upgrade a separate bug, and open another ticket for it.

comment:21 in reply to:  20 Changed 3 years ago by nickm

Replying to nickm:

I think we should consider _that_ ntor-upgrade a separate bug, and open another ticket for it.


Teor has opened this ticket as #20012 .

comment:22 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Okay, looks good now! Merging it. Please have a look at my merge commit (bbaa7d09a045130560a2f5da579671c5e02c9cd7) to make sure that my solution for the conflict in routerlist.c didn't reintroduce #19973 .

comment:23 Changed 3 years ago by nickm

Actual-review-points: .2

comment:24 in reply to:  22 Changed 3 years ago by teor

Replying to nickm:

Okay, looks good now! Merging it. Please have a look at my merge commit (bbaa7d09a045130560a2f5da579671c5e02c9cd7) to make sure that my solution for the conflict in routerlist.c didn't reintroduce #19973 .

The merge commit looks good, apart from the following comment line being removed:

-      * if we are making a direct connection */

I'm not too worried about that, because the next line is pretty readable even without the comment:

 +    if (direct_conn && check_reach &&
Note: See TracTickets for help on using tickets.