Opened 7 months ago

Closed 6 months ago

#29665 closed defect (fixed)

hs: circuit_expire_old_circuits_serverside() should check for RP circuits

Reported by: dgoulet Owned by:
Priority: High Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version: Tor: 0.2.1.26
Severity: Normal Keywords: tor-hs, tor-relay, 029-backport, 034-backport, 035-backport, 040-backport, consider-backport-after-0404
Cc: Actual Points: 0.1
Parent ID: Points: 0.1
Reviewer: asn Sponsor:

Description

Thanks to a single onion service operator on IRC in the #tor channel (slingamn), armadev and I were able to identify this issue.

Any clients connecting to a single onion service and then being idle for 60 seconds would get disconnected as in the rendezvous circuit closed.

It turns out that the rendezvous point closes the service circuit through this function circuit_expire_old_circuits_serversid() if is idle for more than 60 seconds (only for single onion service). The faulty condition is:

    if (or_circ->p_chan && channel_is_client(or_circ->p_chan) &&
        !circ->n_chan &&
        !or_circ->n_streams && !or_circ->resolving_streams &&
        channel_when_last_xmit(or_circ->p_chan) <= cutoff) {

The RP is the end of the service circuit (or_circ), all data is spliced to the client circuit which makes it that n_streams and n_chan are NULL and thus validating the condition.

Also possible to hit this if channel_is_client() is a false positive for the exit point.

The fix here is to check if the circuit being looked at is a rendezvous point and ignore it if so. The or_circ->rend_splice should be non-NULL if so.

This needs to be backported and affects v2 and v3 hidden service.

Child Tickets

Change History (10)

comment:1 Changed 7 months ago by arma

Status: newneeds_review

My bug29665_029 branch fixes this bug.

We touched that code between 0.2.9 and 0.3.4, so there's a conflict merging that patch forward to 0.3.4. So I made a bug29665_034 branch in case that's useful.

It looks like that 034 branch merges cleanly into 035 and 040.

comment:2 Changed 7 months ago by asn

Reviewer: asn

comment:3 Changed 7 months ago by asn

Status: needs_reviewmerge_ready

Good find. Fix LGTM! I pushed a squash commit with a comment and also made PRs for it.

For some reason appveyor on the 029 PR refuses to run even tho I reuploaded the branch:
029 -> https://github.com/torproject/tor/pull/792
034 -> https://github.com/torproject/tor/pull/791

comment:4 Changed 7 months ago by nickm

LGTM; I can merge into 0.4.0 and forward whenever. But would you like to squash the branch first?

comment:5 in reply to:  4 Changed 7 months ago by asn

Replying to nickm:

LGTM; I can merge into 0.4.0 and forward whenever. But would you like to squash the branch first?

OK. I squashed and force-pushed on the PRs above. You can use those branches for merging.

comment:6 Changed 7 months ago by nickm

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

Merged PR 791 to 0.4.0 and forward. Marking for possible backport.

comment:7 in reply to:  3 Changed 7 months ago by teor

Replying to asn:

For some reason appveyor on the 029 PR refuses to run even tho I reuploaded the branch:
029 -> https://github.com/torproject/tor/pull/792

There is no Appveyor config on 0.2.9, so Appveyor does not run.

comment:8 Changed 7 months ago by teor

Actual Points: 0.1
Keywords: consider-backport-after-0404-alpha added

Backport evaluation:

This is a one-line code change that adds a single condition to a circuit expiry check on relays.

We should test this change in 0.4.0.3-alpha, and then backport after 0.4.0.4-alpha is released.

comment:9 Changed 6 months ago by teor

Keywords: consider-backport-after-0404 added; consider-backport-after-0404-alpha removed

Drop the -alpha from backport tags

comment:10 Changed 6 months ago by teor

Milestone: Tor: 0.3.5.x-finalTor: 0.3.4.x-final
Resolution: fixed
Status: merge_readyclosed
Version: Tor: 0.2.1.26

Merged to PR 792 0.2.9, merged PR 791 to 0.3.4 then "ours" merge from 0.2.9 to avoid conflicts, then merged forward.

Merged #23790, #29665, #29017, #27199, #29144, #13221, #28698.

Hi asn, if you're doing multiple branches, please merge forward, then edit?
It makes the backport merge easier.

Note: See TracTickets for help on using tickets.