Opened 3 years ago

Closed 3 years ago

Last modified 20 months ago

#15515 closed defect (fixed)

Don't allow multiple INTRODUCE1s on the same circuit

Reported by: asn Owned by:
Priority: High Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Keywords: 026-backport, 025-backport, 024-backport, 027-triaged-1-in, SponsorU, 2016-bug-retrospective
Cc: Actual Points:
Parent ID: #15463 Points: small
Reviewer: Sponsor: SponsorR

Description

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), 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.

Child Tickets

Change History (31)

comment:1 Changed 3 years ago by asn

Parent ID: #15463

comment:2 Changed 3 years ago by asn

Status: newneeds_review

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.

comment:3 Changed 3 years ago by 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.

comment:4 Changed 3 years ago by nickm

Keywords: 026-backport added

comment:5 Changed 3 years ago by nickm

Keywords: 025-backport added
Milestone: Tor: 0.2.7.x-finalTor: 0.2.6.x-final

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.

comment:6 Changed 3 years ago by yawning

Keywords: 024-backport added

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.

comment:7 in reply to:  6 Changed 3 years ago by dgoulet

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.

comment:8 Changed 3 years ago by 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.

comment:9 in reply to:  3 ; Changed 3 years ago by 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.

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

comment:10 in reply to:  9 Changed 3 years ago by yawning

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.

comment:11 in reply to:  8 Changed 3 years ago by yawning

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.

comment:12 in reply to:  9 Changed 3 years ago by dgoulet

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.

comment:13 in reply to:  8 Changed 3 years ago by dgoulet

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. :)

comment:14 Changed 3 years ago by 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?

comment:15 in reply to:  14 Changed 3 years ago by asn

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?

comment:16 Changed 3 years ago by cypherpunks

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

comment:17 Changed 3 years ago by cypherpunks

One ACK many NACKs policy, isn't?

comment:18 Changed 3 years ago by cypherpunks

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.

comment:19 Changed 3 years ago by asn

<armadev> dgoulet: what if the hsdesc has only two intro points, and there's a newer hsdesc with two and one of them overlaps. you try both, discard desc, get new desc, try the two new ones, there you are with a fine intro circ open already to one of the two new ones.

here is a possible edge case for clients that don't have b5525476, that could make us trigger this warning.

if the above is possible we should consider one of these two possible avenues:

  • reduce the logging severity of the warning message from this ticket to info, till 0.2.5 goes out of style.
  • backport b5525476 to older releases actually that won't work
  • ???
Last edited 3 years ago by asn (previous) (diff)

comment:20 Changed 3 years ago by nickm

Merged to 0.2.4 and forward. Any more things for me to apply here?

comment:21 Changed 3 years ago by cypherpunks

reduce the logging severity of the warning message from this ticket to info, till 0.2.5 goes out of style.

Upgrading can't to prevent legit client from sending second introduce1 cell on behalf of broken or evil HS. Client do not deduplicates intro points when parsing and storing them and not checking for anything for intro point when get NAK and re-extend intro circuit.

comment:22 Changed 3 years ago by cypherpunks

Upgrading can't to prevent legit client from sending second introduce1 cell on behalf of broken or evil HS.

I'll remove this sentence, it's wrong.
But let this to send as many NACKs as it can, anyway.

comment:23 Changed 3 years ago by cypherpunks

I'll revoke comments 14, 17, 21, 22, which is about ability to process introduce1 cells as long as not any ACK'ed yet. It's not fun to allow IP so easy to know one client want to visit HS1, HS2, .. HSn, HSn+1, even if it nothing knows about HS1..HSn. One intro circuit allows only one introduce1 cell is valid and legit limitation for intro circuits. Has no more questions to this ticket then.

comment:24 Changed 3 years ago by arma

Keywords: SponsorR added

comment:25 Changed 3 years ago by isabela

Keywords: 027-triaged-1-in SponsorU added
Points: small
Priority: normalmajor
Version: Tor: 0.2.7

comment:26 in reply to:  2 ; Changed 3 years ago by qwerty1

  unsigned int already_received_introduce1 : 1;

circ->already_received_introduce1 wants to be initialized to 0 - no INTRODUCE1 received on this circuit yet, otherwise the first INTRODUCE1 will be blocked too.

comment:27 in reply to:  26 ; Changed 3 years ago by asn

Replying to qwerty1:

  unsigned int already_received_introduce1 : 1;

circ->already_received_introduce1 wants to be initialized to 0 - no INTRODUCE1 received on this circuit yet, otherwise the first INTRODUCE1 will be blocked too.

In or_circuit_new() we do:

  circ = tor_malloc_zero(sizeof(or_circuit_t));

Since tor_malloc_zero() is used, this element should be inited to 0. Same goes for is_first_hop etc.

Or not?

comment:28 in reply to:  27 Changed 3 years ago by arma

Replying to asn:

Replying to qwerty1:
Since tor_malloc_zero() is used, this element should be inited to 0. Same goes for is_first_hop etc.

Right, I agree. Maybe qwerty1 is confusing the " : 1" syntax ("use one bit of space for this element") with "assign a default value of 1"?

comment:29 Changed 3 years ago by dgoulet

Resolution: fixed
Status: needs_reviewclosed

Fix is merged was merged to 0.2.4 and forward. Closing this.

comment:30 Changed 2 years ago by dgoulet

Keywords: SponsorR removed
Sponsor: SponsorR

comment:31 Changed 20 months ago by nickm

Keywords: 2016-bug-retrospective added

Mark more tickets for bug retrospective based on hand-review of changelogs from 0.2.5 onwards.

Note: See TracTickets for help on using tickets.