Opened 4 years ago

Last modified 2 years ago

#14828 needs_revision defect

Multiple hidden services can share a pk_digest/service_id.

Reported by: yawning Owned by: twim
Priority: Very Low Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Minor Keywords: easy, tor-hs, safety
Cc: yawning Actual Points:
Parent ID: Points: 0.1
Reviewer: Sponsor:

Description

This may be a duplicate, it's past my bed time, so I don't have time to check.

The current rendservice code's duplication check doesn't enforce uniqueness of pk_digest and service_id. It probably should do so for both things, since I can't think of a reason why this would ever be well defined, or desirable behavior.

The trivial fix would be to add a pair of checks to rendservice.c:rend_service_load_keys(s), that log on LD_CONFIG, and return an error if a collision is detected.

Child Tickets

Attachments (1)

14828_force_unique_pk_digest_and_service_id.patch (1.9 KB) - added by juce 4 years ago.
Seems like this would work.. don't know why it was in the wrong place?

Download all attachments as: .zip

Change History (32)

comment:1 Changed 4 years ago by yawning

Keywords: tor-hs added

comment:2 Changed 4 years ago by nickm

Milestone: Tor: 0.2.7.x-final

comment:3 Changed 4 years ago by nickm

Status: newassigned

comment:4 Changed 4 years ago by nickm

Keywords: 027-triaged-1-in added

Marking more tickets as triaged-in for 0.2.7

comment:5 Changed 4 years ago by isabela

Keywords: SponsorR added
Points: small
Priority: normaltrivial
Version: Tor: unspecifiedTor: 0.2.7

comment:6 Changed 4 years ago by nickm

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

comment:7 Changed 4 years ago by nickm

Keywords: SponsorR removed
Sponsor: SponsorR

Bulk-replace SponsorR keyword with SponsorR sponsor field in Tor component.

comment:8 Changed 4 years ago by dgoulet

Keywords: 027-triaged-1-in removed

comment:9 Changed 4 years ago by dgoulet

Keywords: hidden-service config removed

Changed 4 years ago by juce

Seems like this would work.. don't know why it was in the wrong place?

comment:10 Changed 4 years ago by juce

Severity: Blocker
Status: assignedneeds_review

comment:11 Changed 4 years ago by yawning

Severity: BlockerMinor

comment:12 Changed 3 years ago by teor

This looks like a simple code movement patch to fix this issue, but I'd like yawning to weigh in before we merge, because he's more familiar with ephemeral hidden services.

comment:13 Changed 3 years ago by nickm

Cc: yawning added

comment:14 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision

This patch moves the check to rend_service_load_keys() which makes sense but when adding an ephemeral onion we do not call it ending up with a case where we could have a collision.

  /* Enforcing pk/id uniqueness should be done by rend_service_load_keys(), but
   * it's not, see #14828. */

I think what we need here is a function that checks for the service key if it already exists and then called before adding the ephemeral service and also at load keys.

comment:15 Changed 3 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.???

It is impossible that we will fix all 261 currently open 028 tickets before 028 releases. Time to move some out. This is my first pass through the "needs_revision" and "needs_information" tickets, looking for things to move to ???.

Note that in most cases, if these tickets get the requested revisions done in time for the 0.2.8 merge window, they could get considered for review and merge in 0.2.8.

comment:16 Changed 3 years ago by dgoulet

Sponsor: SponsorRSponsorR-can

Move those from SponsorR to SponsorR-can.

comment:17 Changed 3 years ago by twim

Status: needs_revisionneeds_review

I've implemented a function that checks if there is a duplicate service. Please see the code at bug14828 branch of https://gitlab.com/nogoegst/tor. It also fixes unreachable code similar to one in #20400.

comment:18 Changed 3 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.3.0.x-final

comment:19 Changed 3 years ago by nickm

Keywords: review-group-11 added

comment:20 Changed 3 years ago by nickm

Owner: set to twim
Status: needs_reviewassigned

setting owner

comment:21 Changed 3 years ago by nickm

Status: assignedneeds_review

comment:22 Changed 3 years ago by dgoulet

Points: small0.1
Status: needs_reviewneeds_revision

That can't work (and I confirmed it with a simple test). That patch does: load the keys for each service then check for a duplicate key in all the service we have but yet our service is already in the list so you'll get a positive match everytime against yourself :).

comment:23 in reply to:  22 Changed 3 years ago by twim

Replying to dgoulet:

That can't work (and I confirmed it with a simple test). That patch does: load the keys for each service then check for a duplicate key in all the service we have but yet our service is already in the list so you'll get a positive match everytime against yourself :).

Yeah, thanks. This is because the logic appears to be kinda broken here. :\
There should be a separate temp list for services which keys we want to load. And if loading fails, there should be no invalid services in global rend_service_list. As for now, rend_service_list contains also broken services.
Also there is a problem if we going to call (there is no such call now) rend_service_load_all_keys() sometime after there are ephemeral services there: s->directory == NULL for them...
I think this upper logic has to be fixed. Thoughts?

comment:24 Changed 3 years ago by nickm

Keywords: review-group-12 added; review-group-11 removed

comment:25 Changed 2 years ago by nickm

Keywords: review-group-12 removed

comment:26 Changed 2 years ago by teor

Now that #20559 has been merged, I suggest we move this check to rend_add_service().

In fact, I suggest we move all the rend_service_add_ephemeral() checks to rend_add_service(), and then remove any duplicate checks.

A good patch for this will rename rend_service_add_ephemeral_status_t to rend_add_service_status_t, rename the reason codes to RAS_*, and then add reason codes for the other kinds of failures in rend_add_service().

comment:27 Changed 2 years ago by teor

(And it will add a unit test, like test_single_onion_poisoning(), but designed to exercise as many of these failure modes as possible.)

comment:28 Changed 2 years ago by dgoulet

Keywords: triage-out-030-201612 added
Milestone: Tor: 0.3.0.x-finalTor: unspecified

Triaged out on December 2016 from 030 to unspecified

comment:29 Changed 2 years ago by nickm

Keywords: triage-out-030-201612 removed

comment:30 Changed 2 years ago by dgoulet

Sponsor: SponsorR-can

comment:31 Changed 2 years ago by nickm

Keywords: safety added
Note: See TracTickets for help on using tickets.