[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #5458 [Tor Client]: Clients should warn in the event of excessive circuit failures
#5458: Clients should warn in the event of excessive circuit failures
-----------------------------+----------------------------------------------
Reporter: mikeperry | Owner: mikeperry
Type: defect | Status: needs_review
Priority: major | Milestone: Tor: 0.2.2.x-final
Component: Tor Client | Version:
Keywords: MikePerry201205 | Parent: #5456
Points: 6 | Actualpoints:
-----------------------------+----------------------------------------------
Changes (by nickm):
* milestone: => Tor: 0.2.2.x-final
Comment:
Replying to [comment:10 rransom]:
> Oooh! A patch!
Indeed! (My workflow does a very bad job of showing me tickets that are
neither given a milestone nor put in needs_review. I'm throwing this into
0.2.2.x for now, but I think 0.2.3.x might be more appropriate.)
Quick notes on the code (actual stuff and cosmetic stuff all mixed
together):
* PATH_BIAS_* seem like they want to become configurable, or let via a
consensus param, or both.
* is_an_entry_guard() is misnamed: any function with a predicate name
like that should return a boolean. If we need a function that gets a
digest and returns an entry_guard_t *, it should have a name like
entry_guard_get_by_id_digest() or something.
* Also, there's no real reason to make it inline.
* I would be much more comfortable if callers of is_an_entry_guard() [or
whatever it gets renamed to] checked to make sure it returns non-null,
rather than asserting. Like, what if the node were to stop being an entry
guard for some reason after the circuit is launched?
* Obviously, the log_warns need to be turned into info or debug before
this goes into production.
* spurious newline added to or.h.
* Using u64 for the circuit count seems insanely high. If we have been
using an entry guard for long enough to get anything like UINT32_MAX
circuits, surely we don't want to be counting the older circuits any more.
I think that the "scale everything down" feature should really kick in
long before INT64_MAX. Like, it doesn't seem unreasonable to me for this
to kick in even at 500 or 1000 circuits and divide both values by 2 or 5
or 10. Maybe this should be a consensus parameter too; what do you think?
* The huge new blob in circuit_finish_handshake() should really get
extracted into a new function for readability.
* For the entry_guards_parse_state() string, wouldn't it be much easier
to implement this if we just had the two values stored in separate state
fields? That way we wouldn't need to have the state management functions
mess around with string manipulation.
* Failing that, "hop = (uint32_t)tor_parse_uint64" looks wrong, as does
"success = (build_time_t)...". Also, you can't use "%ld" to snprintf an
unsigned 64-bit int; look at U64_FORMAT and U64_PRINTF_ARG if you really
need to do that.
* For some of the stuff you're doing with base16_encode() now, you
should be using hex_str(), I think.
More general notes:
* I want other people's opinions here, but this doesn't feel like an
0.2.2.x change to me.
* A four-hop circuit IMO doesn't need special treatment. A cannibalized
circuit, though, will get counted as having succeeded twice, which isn't
necessarily what we want. This could throw off results if a large enough
fraction of circuits gets cannibalized. It
* Is it possible to have PATH_BIAS_MIN_CIRCS on a guard all in progress
at once? If so, there might be an annoying problem when starting up a new
guard.
* Removing stuff from the list is probably not a great idea, since it
wouldn't keep the node from getting re-added. Instead, you probably want
to flag the node as invalid or something.
More thought-requiring notes:
* We should do the math to see how successful the attack can be under
different parameter choices. (Under the current parameter choices, it
seems like you can do route capture pretty darn effectively so long as you
treat each new client honestly for a sufficiently large number of circuits
before doing the attack.)
* We should figure out what kind of false positive rate we expect, and
document that, and maybe even mention it in the warning. (A tiny but not
vanishing false positive rate, multiplied by a very large number of users
and a lot of time, means that we should expect some number of spurious
reports.)
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/5458#comment:11>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs