Opened 3 years ago

Closed 3 years 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:


Hello !

I discovered #14917 while configuring an onion service with the EntryNodes option set. I believe (after checking the tor- 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 3 years ago by dgoulet

Component: - Select a componentCore Tor/Tor
Keywords: tor-hs tor-guard added
Milestone: Tor: 0.3.0.x-final
Version: Tor: unspecified

comment:2 Changed 3 years ago by asn


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 3 years 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 Changed 3 years 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 3 years ago by dgoulet

Milestone: Tor: 0.3.0.x-finalTor: 0.3.1.x-final
Status: newneeds_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 years ago by dgoulet

Points: 0.1
Status: needs_informationneeds_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 years ago by dgoulet

Owner: set to dgoulet
Status: needs_reviewaccepted

comment:8 Changed 3 years ago by dgoulet

Status: acceptedneeds_review

comment:9 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

looks okay, but needs a changes file.

comment:10 Changed 3 years ago by dgoulet

Status: needs_revisionneeds_review

See branch: bug21155_031_02.

comment:11 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

merging; thanks!

Note: See TracTickets for help on using tickets.