[tor-bugs] #4312 [Tor Relay]: Rate limit renegotiations

[Tor Bug Tracker & Wiki]

#4312: Rate limit renegotiations
-----------------------+----------------------------------------------------
 Reporter:  asn        |          Owner:              
     Type:  defect     |         Status:  needs_review
 Priority:  normal     |      Milestone:              
Component:  Tor Relay  |        Version:              
 Keywords:             |         Parent:              
   Points:             |   Actualpoints:              
-----------------------+----------------------------------------------------

Comment(by asn):

 Replying to [comment:9 nickm]:
 > Replying to [comment:8 asn]:
 > > Replying to [comment:7 nickm]:
 > > > Looks ok.
 > > >
 > > > For the bufferevents case: The same callback-based approach works
 for detecting excessive renegotiation, but we run into problems in knowing
 what to do about it.  We want to run some "kill this connection" callback
 soon, but not immediately (because running that kind of thing from inside
 the SSL callback from inside the bufferevent code is a recipe for Lots Of
 The Wrong Kind Fun.)  Probably, we could just use event_base_once code to
 invoke a trampoline function to call the appropriate user-level callback
 to call  bufferevent_disable() and connection_mark_for_close() on the
 connection.  Does that make sense?  I think it would work.
 > >
 > > In the non-bufferevents case, the OpenSSL callbacks increment the
 ClientHello-seen count, but they are not killing the connection. The
 connection is killed after SSL_{read,write}() functions are done.
 > >
 > > The problem with setting a "kill the connection" callback from within
 the OpenSSL callbacks is that we only have access to the `tor_tls_t` in
 there. I assume that calling a libevent event with the `tor_tls_t`
 attached and doing a linear search over all the connections to find the
 `tor_tls_t` bearer is super stupid, and we must find something better. If
 you have something in mind, I indeed believe a callback-based approach is
 the cleanest (we can also do that in the non-bufferevents case as well,
 and stop catching excess renegotiations around the code.)
 >
 > Right.  I'd be thinking of something like adding a function pointer and
 an void* to the tor_tls_t such that if the renegotiation count is
 exceeded, the function pointer gets called with the void* and the
 tor_tls_t as its arguments.  (Like how the renegotiation callback works
 now) When we set up a TLS connection to use bufferevents, we'd pass a
 callback function took the or_connection_t as an argument, and did  that
 did bufferevent_disable() on its bufferevent and
 connection_mark_for_close() on the connection.
 >
 > At least, that's what I'm thinking here.

 And how do we know when to trigger the event that disables the bev and
 marks the connection? `event_base_once()` seems to accept a timer as an
 argument. Do you think it's "safe" triggering the event based on time?

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