[tor-bugs] #24902 [Core Tor/Tor]: Denial of Service mitigation subsystem

[Tor Bug Tracker & Wiki]

#24902: Denial of Service mitigation subsystem
-------------------------------------------------+-------------------------
 Reporter:  dgoulet                              |          Owner:  dgoulet
     Type:  enhancement                          |         Status:
                                                 |  needs_review
 Priority:  Very High                            |      Milestone:  Tor:
                                                 |  0.3.3.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  ddos, tor-relay, review-group-30,    |  Actual Points:
  029-backport, 031-backport, 032-backport,      |
  review-group-31                                |
Parent ID:                                       |         Points:
 Reviewer:  arma                                 |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by dgoulet):

 * reviewer:   => arma


Comment:

 First, I had to do a fixup unrelated to the review: `9d2699cad0`

 Replying to [comment:43 arma]:
 > Should we use END_CIRC_REASON_RESOURCELIMIT instead?

 Good point. Fixup commit `2d9b285f98`.

 > But it looks like the call to dos_should_refuse_single_hop_client()
 doesn't care whether public_server_mode().

 Agree. Fixup commit: `ab7b9581f3`

 > get_circuit_rate_per_second() still isn't doing what I think you wanted.

 Yah so thanks to Hello71 on IRC, I realized that there was an issue indeed
 and all your "Edit:" were indeed discussed and proposed by Hello71 :).

 I kind of think dropping the "tenths" would be good because, as you said,
 at best we can fill the bucket every second (approx_time()). So everything
 is rounded off to the second anyway. And a considerable time gap between
 CREATE cell will make a small difference which probably won't matter at
 that point.

 Thus, going to a number of circuit per second instead of "tenths" seems
 the improvement to do.

 What about this commit? `2106a5eaa8`

 > (a) should be "Number of allocated circuits remaining for this address",
 i.e. it's not a rate, it's a size.

 Fixup commit: `c7099765b4`

 > (b) What's this about "plus a bit of random"? :)

 Don't know what you are talking about, I can't find this string in the
 code :S ...

 > In the future, I plan to advocate for merging dos_cc_new_create_cell()
 and dos_cc_get_defense_type() into a single function, which notes the
 existence of the new create cell and also tells us whether to apply a
 defense. And I plan to advocate for a second cc defense, which returns
 DOS_CC_DEFENSE_REFUSE_CELL simply when stats->cc_stats.circuit_bucket ==
 0, without any marking or checking of stats->concurrent_count. I think I
 will want to instrument a real relay to see how often it would trigger
 that new defense, though, and I am happy to delay my future plans so we
 can get this patch out the door.

 Agree on both! Once we get this merged, lets open tickets for that!

 Final fixup on the man page (thanks to Hello71!): `1b8835fccd`

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/24902#comment:48>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online