Don't allow multiple INTRODUCE1s on the same circuit

Currently, it seems like clients are able to send multiple INTRODUCE1 cells to the IP. The result is that many INTRODUCE2 cells reach the HS, which means that the HS will try to establish multiple rendezvous circuits.

This gives a better position to attackers who want to flood a HS with rendezvous circuits (like #15463 (moved)), since with a single circuit they can cause hundreds of rendezvous.

We should fix this in the IP-side, by closing the circuit after sending the INTRODUCE_ACK to the client. An alternate behavior, is to change the state of the circuit after INTRODUCE1 is received and close it if more such cells are received.

changed milestone to %Tor: 0.2.6.x-final

added 024-backport 025-backport 026-backport 027-triaged-1-in 2016-bug-retrospective SponsorU component::core tor/tor milestone::Tor: 0.2.6.x-final parent::15463 points::small priority::high resolution::fixed sponsor::R status::closed type::defect version::tor 0.2.7 labels

Trac:
Parent: N/A to #15463 (moved)

Hello,

please see branch bug15515_2 on my repo: https://gitweb.torproject.org/user/asn/tor.git/log/?h=bug15515_2

In the end, we decided to go with the "don't accept more than one introduction on a circuit" approach.

We originally planned to go with the "IP closes circuit after INTRODUCE_ACK gets sent", but we learned that there might be race conditions with the INTRODUCE_ACK cell and the DESTROY cell that could lead a client to close the circuit before receiving the ACK cell. So we ended up going with the other approach which is a bit messier.

Trac:
Status: new to needs_review

some more thoughts: 16:31 < qwerty1> maybe it would be better to return 0 instead of goto err 16:31 < qwerty1> if they get a nack they will know to try a new circuit 16:32 < asn> qwerty1: hm plausible. maybe we should also not waste cells for them.

Trac:
Keywords: N/A deleted, 026-backport added

I made a new branch called "bug15515_025" against maint-0.2.5, and cherry-picked this ticket there. Merged to 0.2.7. We should consider this for backport.

Trac:
Keywords: 026-backport deleted, 026-backport 025-backport added
Milestone: Tor: 0.2.7.x-final to Tor: 0.2.6.x-final

This needs a spec change to document the new behavior for implementations.

Given that a large fraction of the network is still running 0.2.4.x, IMO this should be backported to maint-0.2.4, otherwise there's a good chance that HSes will pick IPs that are running unpatched versions, and will still be easily DoSable.

Trac:
Keywords: 026-backport 025-backport deleted, 026-backport 025-backport 024-backport added

Replying to yawning:

This needs a spec change to document the new behavior for implementations.

Given that a large fraction of the network is still running 0.2.4.x, IMO this should be backported to maint-0.2.4, otherwise there's a good chance that HSes will pick IPs that are running unpatched versions, and will still be easily DoSable.

Agree on the backport to 0.2.4. ACK on my side.

I asked weasel about the efficacy of an 0.2.4 backport here: "jessie will release in april, and that might move a lot of people to 0.2.5.x within a relatively short period of time. so maybe wait for that?" "also, which versions of 0.2.4 are they running? 0.2.4.26? if not, they aren't using squeeze or wheezy anyway."

I'm just trying to sort out if, for relays not running the latest 0.2.4.x, a new 0.2.4.x is going to help them or us any.

Replying to asn:

some more thoughts: 16:31 < qwerty1> maybe it would be better to return 0 instead of goto err 16:31 < qwerty1> if they get a nack they will know to try a new circuit 16:32 < asn> qwerty1: hm plausible. maybe we should also not waste cells for them.

As discussed yesterday, I implemented this alternative behavior in a fixup commit on my branch bug15515_025 which is based on nick's rebase. Basically, it marks the circuit as closed and does not send a NAK, if we detect multiple INTRODUCE1 cells.

The fixup commit should probably also be applied on master. It should apply cleanly I think.

Replying to asn:

Replying to asn:

some more thoughts: 16:31 < qwerty1> maybe it would be better to return 0 instead of goto err 16:31 < qwerty1> if they get a nack they will know to try a new circuit 16:32 < asn> qwerty1: hm plausible. maybe we should also not waste cells for them.

As discussed yesterday, I implemented this alternative behavior in a fixup commit on my branch bug15515_025 which is based on nick's rebase. Basically, it marks the circuit as closed and does not send a NAK, if we detect multiple INTRODUCE1 cells.

The fixup commit should probably also be applied on master. It should apply cleanly I think.

Looks good to me.

Replying to arma:

I asked weasel about the efficacy of an 0.2.4 backport here: "jessie will release in april, and that might move a lot of people to 0.2.5.x within a relatively short period of time. so maybe wait for that?" "also, which versions of 0.2.4 are they running? 0.2.4.26? if not, they aren't using squeeze or wheezy anyway."

I'm just trying to sort out if, for relays not running the latest 0.2.4.x, a new 0.2.4.x is going to help them or us any.

Based off the current consensus:

      1 v Tor 0.2.4.18-rc
      4 v Tor 0.2.4.19
    190 v Tor 0.2.4.20
     69 v Tor 0.2.4.21
    132 v Tor 0.2.4.22
    902 v Tor 0.2.4.23
      1 v Tor 0.2.4.23-dev
    750 v Tor 0.2.4.24
     26 v Tor 0.2.4.25
    688 v Tor 0.2.4.26

~24.9% are on latest 0.2.4.x. I'm unsure of how many of the remainder will upgrade if we make a lot of noise about upgrading. Given some of the other changes pending that I also believe should go into maint-0.2.4, I'm still in favor of a backport here.

Replying to asn:

Replying to asn:

some more thoughts: 16:31 < qwerty1> maybe it would be better to return 0 instead of goto err 16:31 < qwerty1> if they get a nack they will know to try a new circuit 16:32 < asn> qwerty1: hm plausible. maybe we should also not waste cells for them.

As discussed yesterday, I implemented this alternative behavior in a fixup commit on my branch bug15515_025 which is based on nick's rebase. Basically, it marks the circuit as closed and does not send a NAK, if we detect multiple INTRODUCE1 cells.

The fixup commit should probably also be applied on master. It should apply cleanly I think.

lgtm. ACK.

Replying to arma:

I asked weasel about the efficacy of an 0.2.4 backport here: "jessie will release in april, and that might move a lot of people to 0.2.5.x within a relatively short period of time. so maybe wait for that?" "also, which versions of 0.2.4 are they running? 0.2.4.26? if not, they aren't using squeeze or wheezy anyway."

That's true but we are talking about fix on relay that runs mostly on servers running stable Debian. In my experience as a sysadmin a long time ago in a galaxy far far away, moving a server running a stable Debian to a new stable release is anything but short-term. :)

Why it counts already_received_introduce1 if NACK only will be sent? Look at code of client, if all IP expired after received NACK, it keeps circuit alive and going to refetch the service descriptor to reuse intro circuit again next time for data from new descriptor for a same hs or another?

Replying to cypherpunks:

Why it counts already_received_introduce1 if NACK only will be sent? Look at code of client, if all IP expired after received NACK, it keeps circuit alive and going to refetch the service descriptor to reuse intro circuit again next time for data from new descriptor for a same hs or another?

hey, please see branch from comment:9 . In that one we don't send a NAK, we just shut down the circuit. Does your comment still apply there?

s/counts/sets/ I want to get what going to do legit client with those alive CIRCUIT_PURPOSE_C_INTRODUCING.

One ACK many NACKs policy, isn't?

OK, found problem, was reading code of 0.2.5.x, new code closes circuit for described above. It's good spec allows to change code in the middle.