Opened 3 months ago

Last modified 5 weeks ago

#31548 merge_ready defect

hs-v3: Service can pick more than HiddenServiceNumIntroductionPoints intro points

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version: Tor: 0.3.2.1-alpha
Severity: Normal Keywords: consider-backport-after-0424, tor-hs, service, hs-v3, 035-backport, 040-backport, 041-backport, 042-should
Cc: Actual Points: 0.2
Parent ID: #29995 Points: 0.2
Reviewer: asn Sponsor: Sponsor27-must

Description

During my testing of #30200, I ended up with service descriptor with 4 intro points even though HiddenServiceNumIntroductionPoints is set to 3 (default).

Further investigation confirmed this by adding a log in the decode_intro_points() function which showed me 4 intro points.

I haven't found out why but one feature of HS is that we launch HiddenServiceNumIntroductionPoints + 2 intro circuits in parallel and the first one to finish are picked.

It appears that more than the defined value can finish at the same time and will be picked.

Child Tickets

Change History (9)

comment:1 Changed 3 months ago by dgoulet

Cc: asn removed
Keywords: 035-backport 040-backport 041-backport added
Points: 0.2
Reviewer: asn
Status: newneeds_review

I recommend backport up to 035. This is major bugfix in my opinion affecting HS reachability quite a bit. This fix also mitigates #31561 that makes a service possibly put in the descriptor intro points for which it has *no* circuits, never.

Following is based on maint-0.3.5:

PR: https://github.com/torproject/tor/pull/1274
Branch: ticket31548_035_01

comment:2 Changed 3 months ago by asn

Status: needs_reviewneeds_revision

Great find. LGTM. Just a tiny comment nitpick.

BTW, do we do this check in v2?

comment:3 Changed 2 months ago by nickm

Keywords: 042-should added

Mark some needs_revision tickets as 042-should

comment:4 Changed 2 months ago by dgoulet

Owner: set to dgoulet
Status: needs_revisionaccepted

comment:5 Changed 2 months ago by dgoulet

Status: acceptedneeds_revision

comment:6 Changed 7 weeks ago by dgoulet

Status: needs_revisionneeds_review

Done, removed the XXX in the fixup commit: afcca119989a5b078cbfee8641024997af133e2b

BTW, do we do this check in v2?

Nope, v2 is very different on how it picks intro points. I haven't analyze if it is also affected but I don't think so, I've never seen that behavior on v2.

comment:7 Changed 6 weeks ago by asn

Status: needs_reviewmerge_ready

LGTM!

comment:8 Changed 6 weeks ago by nickm

Milestone: Tor: 0.4.2.x-finalTor: 0.4.1.x-final

Okay. I've made a squashed branch ticket31548_035_01_squashed with PR at https://github.com/torproject/tor/pull/1395

Merged it to master, marking for backport.

comment:9 Changed 5 weeks ago by teor

Actual Points: 0.2
Keywords: consider-backport-after-0424 added
Version: Tor: 0.3.2.1-alpha
Note: See TracTickets for help on using tickets.