Opened 6 months ago

Closed 3 months ago

#21155 closed defect (implemented)

Similar to #14917: Client's choice of rend point can leak info about guard(s) of misconfigured hidden services with EntryNodes option

Reported by: Jaym Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, tor-guard
Cc: Actual Points:
Parent ID: Points: 0.1
Reviewer: Sponsor:

Description

Hello !

I discovered #14917 while configuring an onion service with the EntryNodes option set. I believe (after checking the tor-0.2.9.8 source code) that a similar problem arises when the EntryNodes option is set AND the operator configures entry nodes that are part of the same family or the same /16. (let's say that the operator configures the service with 2 of its own guard nodes running in the same cloud provider, thinking this move is wise). Then this happens:

  • When someone use a RDV point of the same family or the same /16 than the onion's guards, then as you said: "entry_list_is_constrained() is true, so populate_live_entry_guards() will happily return an empty list if your one choice is inappropriate, resulting in choose_random_entry_impl() returning NULL".

Is there a reason why we do not check family, /16 and user misconfiguration ? "EntryNodes fingerprint1, fingerprint1" works just fine for example. What do you think ?

Child Tickets

Change History (11)

comment:1 Changed 6 months ago by dgoulet

  • Component changed from - Select a component to Core Tor/Tor
  • Keywords tor-hs tor-guard added
  • Milestone set to Tor: 0.3.0.x-final
  • Version Tor: unspecified deleted

comment:2 Changed 6 months ago by asn

Hmm,

I'm not sure what's the right fix here. If we want to maintain the same "warn & exit" behavior, perhaps we could use nodelist_add_node_and_family() in the same fashion that is used by populate_live_entry_guards()? So that if all configured EntryNodes are basically in the same /16 or family we still refuse to startup?

I wonder if we ever hope to catch all edge cases here.

comment:3 Changed 6 months ago by dgoulet

First of all, I don't think an hidden service should ever have its Family set because also running as a relay is dangerous but hey seems we can't stop users from doing that. But the /16 is also a serious problem...

This is very tricky as it's easy for tor to check at startup if a family node is in EntryNodes. So that we can fix easily to prevent an operator doing a bad thing and refusing to start. Might not totally be that easy though if EntryNodes value requires a consensus to parse like a country for instance.

The same /16 check has to happen at runtime since client will ask to connect to a specific RP and if that RP happens to be in the /16 of your EntryNodes, circuit won't build and the attacker learns very valuable information.

I'm starting to think that at the very least, we should warn that setting up EntryNodes while being a hidden service can expose you more in some known or yet unknown ways....

comment:4 follow-up: Changed 6 months ago by Jaym

What about removing the /16 constraint when extending to the rendezvous ? And we take care of the family at startup.

comment:5 in reply to: ↑ 4 Changed 5 months ago by dgoulet

  • Milestone changed from Tor: 0.3.0.x-final to Tor: 0.3.1.x-final
  • Status changed from new to needs_information

Replying to Jaym:

What about removing the /16 constraint when extending to the rendezvous ? And we take care of the family at startup.

That could be something to think about yes! I currently don't see something bad with this idea. More people need to think about this!

I'm deferring this one to 031 for now as the freeze deadline is approaching very fast.

comment:6 Changed 3 months ago by dgoulet

  • Points set to 0.1
  • Status changed from needs_information to needs_review

I went with a potential controversial patch ;). Warning the operator if EntryNodes are pinned. We already NOT allow it if only one entry node is pinned (#14917). It seems there are too many issues with pinned node so at the very least warn the operator.

In the future, we might want a Wiki page explaining the dangers and pointing to that? What do we all think of this solution?

See branch: bug21155_031_01

comment:7 Changed 3 months ago by dgoulet

  • Owner set to dgoulet
  • Status changed from needs_review to accepted

comment:8 Changed 3 months ago by dgoulet

  • Status changed from accepted to needs_review

comment:9 Changed 3 months ago by nickm

  • Status changed from needs_review to needs_revision

looks okay, but needs a changes file.

comment:10 Changed 3 months ago by dgoulet

  • Status changed from needs_revision to needs_review

See branch: bug21155_031_02.

comment:11 Changed 3 months ago by nickm

  • Resolution set to implemented
  • Status changed from needs_review to closed

merging; thanks!

Note: See TracTickets for help on using tickets.