[tor-bugs] #5458 [Tor Client]: Clients should warn in the event of excessive circuit failures

[Tor Bug Tracker & Wiki]

#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