Opened 7 months ago

Last modified 3 months ago

#30466 needs_information defect

hs: Do not allow more than one control cell on a circuit

Reported by: dgoulet Owned by: neel
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-dos, tor-hs, tor-relay
Cc: neel Actual Points:
Parent ID: Points: 0.1
Reviewer: dgoulet Sponsor: Sponsor27-can

Description

This is the list of HS control cell that is they are all for establishing a circuit or/and "connection" between HS entities (IP, RP, Service, client):

RELAY_COMMAND_ESTABLISH_INTRO:
RELAY_COMMAND_ESTABLISH_RENDEZVOUS:
RELAY_COMMAND_INTRODUCE1:
RELAY_COMMAND_INTRODUCE2:
RELAY_COMMAND_INTRODUCE_ACK:
RELAY_COMMAND_INTRO_ESTABLISHED:
RELAY_COMMAND_RENDEZVOUS1:
RELAY_COMMAND_RENDEZVOUS2:
RELAY_COMMAND_RENDEZVOUS_ESTABLISHED:

It appears that anyone can send an arbitrary amount of those cells on the same circuit. Even to the point that tor allows a rendezvous circuit to become an intro circuit.

The only special one is INTRODUCE2 which is by-design are sent a lot on the same circuit.

The only cell currently limited to 1 cell is INTRODUCE1 since we do not allow multiple introductions on the same client circuit for DoS reasons.

But the rest should only be seen *once* on a circuit. Lets restrict them and if we see more, then we close the circuit due to a protocol error. This would limit side-channels.

Child Tickets

Change History (7)

comment:1 Changed 7 months ago by neel

Cc: neel added
Owner: set to neel
Status: newassigned

comment:2 Changed 6 months ago by dgoulet

Parent ID: #29999

Doesn't have to do much with DoS after all since the INTRODUCE2 problem we'll be taken care with #15516.

Actually, there is a world where we might want to do this but there is also another world where we could leverage this for instance with prop305 where the service could send multiple ESTABLISH_INTRO cell over the lifetime of the intro circuits to change the parameters.

comment:3 Changed 4 months ago by neel

I have a branch here: https://github.com/neelchauhan/tor/commits/bug30466

It's incomplete (meaning not ready for review), I need to write tests.

comment:4 Changed 4 months ago by neel

Status: assignedneeds_review

PR: https://github.com/torproject/tor/pull/1259

Setting as needs review.

Tests were written where possible/feasible. If more tests are needed feel free to put as needs revision and I can add the extra tests.

comment:5 Changed 4 months ago by dgoulet

Reviewer: dgoulet

comment:6 Changed 4 months ago by dgoulet

Status: needs_reviewneeds_information

This approach is sensible and code seems accurate. But we should put this one on hold for now.

The reason is that there might be some cells, for instance ESTABLISH_INTRO, that we will want to be able to send multiple times during the lifetime of the circuit. (Relevant to #30924).

These control cells could be useful for DoS mitigation systems in order to be able to tell different HS entity on how to behave coming from the service.

comment:7 Changed 3 months ago by neel

I have a PR with the same code sans the ESTABLISH_INTRO part: https://github.com/torproject/tor/pull/1280

I am not setting this as needs review as I don't know if you need this or not (because of #30924). If you do, feel free to set it as needs_review.

Note: See TracTickets for help on using tickets.