Opened 2 years ago

Closed 2 years ago

#23361 closed defect (fixed)

prop224: client can pick super old rendezvous points

Reported by: asn Owned by:
Priority: High Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: prop224, tor-hs
Cc: Actual Points:
Parent ID: Points:
Reviewer: asn Sponsor: SponsorR-can

Description

We just discovered that we dont enforce protover rules when prop224 clients pick rendezvous points, so we end up picking rps on 0.2.8 which make our circuits hang.

Child Tickets

Change History (19)

comment:1 Changed 2 years ago by dgoulet

Reviewer: asn
Status: newneeds_review

See branch: bug23361_032_01

The change it not that trivial but I hope simple enough.

comment:2 Changed 2 years ago by asn

Status: needs_reviewneeds_revision

Patch does not handle cannibalized circs correctly. Cannibalizable circs to non-v3-supporting exits should not be used for v3 rend.

Last edited 2 years ago by asn (previous) (diff)

comment:3 Changed 2 years ago by arma

You might also find it useful to put a warn message in, if you're about to send a v3 rend request to a relay that doesn't support a v3 rend request. In case there are other edge cases (or new ones show up in the future).

comment:4 Changed 2 years ago by teor

This also needs a spec change. (Or maybe I missed the relevant part in the spec.)

comment:5 Changed 2 years ago by asn

Status: needs_revisionneeds_review

Fixup branch pushed at bug23361_032_01 in my repo!

Contains two fixup commits: One fixes the hsv3 circuit detection, and the second regulates cannibalization for hsv3 rp circuits.

Let me know if you have any questions. Thanks!

comment:6 Changed 2 years ago by dgoulet

Status: needs_reviewneeds_revision
  • uncanibalizable --> I think missing a n.
  • Not sure about this log info: log_info(LD_GENERAL, "Getting v3 rp circuit!");

Rest lgtm;

comment:7 Changed 2 years ago by asn

Status: needs_revisionneeds_review

Doc fixes done, and force pushed branch.

comment:8 Changed 2 years ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;

comment:9 Changed 2 years ago by nickm

Code looks okay.

This still needs the spec change that teor mentioned above, right?

comment:10 Changed 2 years ago by nickm

Status: merge_readyneeds_revision

Also, this seems to violate proposal 224 section 4.3, which says that we _can_ use older rendezvous points. Why did we decide not to do that?

comment:11 in reply to:  10 Changed 2 years ago by dgoulet

Replying to nickm:

Also, this seems to violate proposal 224 section 4.3, which says that we _can_ use older rendezvous points. Why did we decide not to do that?

That is a spec issue that needs to be updated. Back at the Montreal hidden service meeting, we realized that we needed legacy rendezvous point to relay an HS cell that had more bytes than the 20 bytes rendezvous cookie and that patch got in 0.2.9 (commit be0e1e9e2f6). So, HS client can *not* use RPs below that version which is HSRend=2.

*HOWEVER*, we should most certainly pad all RENDEZVOUS cells in the legacy HS system so v2 and v3 cells look "alike".

comment:12 Changed 2 years ago by asn

Status: needs_revisionneeds_review

Spec patch can be found in bug23361 in my repo. Putting this back in needs_review.

I also made #23420 for the padding feature that David mentioned in comment:11.

comment:13 Changed 2 years ago by asn

Status: needs_reviewmerge_ready

comment:14 Changed 2 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Okay; I merged the spec branch, but I get a pile of warnings when I try to merge it. It looks like there was a runaway comment.

I'm fine with just fixing the comment and merging, but if it's been unbuildable, that means that nobody has has tried testing the code, right?

comment:15 Changed 2 years ago by nickm

Resolution: fixed
Status: closedreopened

comment:16 Changed 2 years ago by nickm

Status: reopenedneeds_revision

comment:17 Changed 2 years ago by asn

Status: needs_revisionmerge_ready

Ouch. Seems like I left a bad comment in there.

Pushed a fixup that addresses it.

Let me know if you want me to rebase the whole branch on master.

comment:18 Changed 2 years ago by asn

OK I tested this branch again to make sure that nothing broke in the meanwhile.
Branch seems to work well.

To test, I used a client with this branch, connected to an hsv3 service and made sure that the rendezvous point used supported the hsv3 protocol and data made it to the service. I did this test about 20 times to make sure that we only pick legit RPs.

comment:19 Changed 2 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

I'm always afraid whenever there's a chance that what we're testing has diverged from what we're running--so, thanks for double-checking that we actually have been testing this code.

Squashed and merging to 0.3.2.

Note: See TracTickets for help on using tickets.