Opened 8 years ago

Closed 7 years ago

#5178 closed defect (fixed)

Adaptive circuit-build timeout code does not match spec

Reported by: rransom Owned by: mikeperry
Priority: High Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client tor-spec
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

01:37 < rransom_> wanoskarnet: See section 2.4.4 of path-spec.txt in https://gitweb.torproject.org/torspec.git/tree . Does that match the behaviour in the code? Is that behaviour good?
01:43 < wanoskarnet> numbers are different. idea is fine except definition of networkk connectivity loss. and no double the timeout can happen because it reset number of counts and could count abandoned circuit if counted of success circ.
01:44 < wanoskarnet> "was already 60 or higher" "or higher" actually means changed initial values only. but it's ok to check for such case.
01:46 < wanoskarnet> in oter work: code do reset timeout to init values but can't double timeout.
01:55 < wanoskarnet> circuit_build_times_network_check_live() non the same as "2.4.4". cbt->liveness.nonlive_timeouts incremeants just after "we've received no cells or TLS handshakes since those circuits began." no matter what number of timeouts.
01:57 < wanoskarnet> "if (cbt->liveness.nonlive_timeouts > 0)" should be 3?

Child Tickets

Change History (10)

comment:1 Changed 8 years ago by rransom

02:01 < wanoskarnet> no "We then temporarily set the timeout to 60 seconds" if network loss.
02:10 < wanoskarnet> "If 80% of our recent circuits are timing out after the first hop" is not enough to count fails after 1 hop only.
02:13 < wanoskarnet> btw, tor can't recover if evil bridge drops extend cells. it can't mark it as down because 1 hop is fine.

comment:2 Changed 8 years ago by rransom

03:03 < wanoskarnet> cbtmaxtimeouts -- Max: 10000, Effect: When this many timeouts happen in the last 'cbtrecentcount'. cbtrecentcount -- Max: 1000. 10000 timeouts for the member of array of 1000? circuit_build_times_network_check_changed(). 1000 < 10000 forever, jfyi.

comment:3 Changed 8 years ago by mikeperry

Owner: set to mikeperry
Status: newassigned

comment:4 Changed 7 years ago by nickm

Keywords: spec added
Milestone: Tor: 0.2.2.x-finalTor: 0.2.3.x-final

comment:5 Changed 7 years ago by nickm

Keywords: tor-client added

comment:6 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:7 Changed 7 years ago by nickm

Keywords: tor-spec added; spec removed

Bulk-replacing "spec" and "torspec" keywords with "tor-spec".

comment:8 Changed 7 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

comment:9 Changed 7 years ago by mikeperry

Status: assignedneeds_review

Ok, I fixed the spec in my torspec remote branch mikeperry/bug5178.

I am not sure what the attack vector is for the evil bridge is from comment 1.

I also did not alter the constants from comment 1 in the tor source. As I see it, this provides a way for us to prevent clearing timeout data from the consensus, but does not introduce a bug. File a new bug for that (and attach a patch) if you're still concerned.

comment:10 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged; thanks!

Note: See TracTickets for help on using tickets.