Rate limit renegotiations
Currently tor allows any number of renegotiations because:
a) tor_tls_block_renegotiation() won't do it for rfc5746 renegotiations. b) The renegotiation callback which calls tor_tls_block_renegotiation() is only called on the first Application Data packet instead of when the renegotiation takes place. This is because the SSL_read() return value is not treated correctly.
Activity
changed milestone to %Tor: 0.2.3.x-final
The branch
bug4312in git://gitorious.org/mytor/mytor.git attempts to fix this for the non-bufferevents case.Please review the branch carefully.
Trac:
Status: new to needs_review-
Oh yeah.
Through my fix, I mentally assert that:
- ServerHello only appears after a ClientHello during a normal SSL handshake.
- We only send a single ServerHello during a normal SSL handshake.
-
wrt the bufferevents case, it seems like connection_or_handle_event_cb() handles renegotiation appropriately and no changes need to be made. It also uses tor_tls_read() and tor_tls_write() normally, so excess renegs. are blocked.
If I had to make a change in connection_or_handle_event_cb() I would add a
tor_assert(tls->got_renegotiate)in the body of:if (handshakes == 2) {But it's probably needlessly aggressive and I don't feel I know connection_or_handle_event_cb() well enough.
I tested the both cases (bufferevents and no-bufferevents) in a privnet. they seem to work.
So, this is tested to prevent multiple renegotiations when using either bufferevents or not?
Issues:
In the comment:
+ /* Call tor_tls_got_server_hello() for every SSL ServerHello we + send. */I think there's a mismatch between the comment and the code. We're about to call got_server_hello. But maybe we should instead be calling a function called got_client_hello or sending_server_hello?
Next: the tor_assert(tls->server_handshake_count == 2); makes me nervous. I don't have enough confidence there to make sure it can never be triggered. Can we change it to a BUG warning?
Next: in the last patch on that branch, should tor_tls_handshake() get a check here, just like read and write have?
Last: needs a changes file.
-
wrt #4312 (moved), it does not seem to work in the bufferevents case. I'm not sure how SSL is handled in USE_BUFFEREVENTS and I suspect it's handled by libevent because I don't see connection_or_handle_event_cb() getting called after the initial SSL handshake. so I suspect that libevent is internally calling SSL_read() or SSL_write(), is completing renegotiations and is not reporting back to tor. The renegotiated callback in the bufferevents case is called from connection_or_process_inbuf() when the first SSL data packet arrives.
-
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.
-
Replying to 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_tin there. I assume that calling a libevent event with thetor_tls_tattached and doing a linear search over all the connections to find thetor_tls_tbearer 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.)If a callback-based approach doesn't work, we might want to find a bufferevents libevent cb function that gets called frequently (and by 'frequently' I mean that it should be called on new SSL traffic, and not only on new SSL Application Data) and do a check there for excess renegotiations.
-
Replying to asn:
Replying to 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_tin there. I assume that calling a libevent event with thetor_tls_tattached and doing a linear search over all the connections to find thetor_tls_tbearer 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.
-
Replying to nickm:
Replying to asn:
Replying to 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_tin there. I assume that calling a libevent event with thetor_tls_tattached and doing a linear search over all the connections to find thetor_tls_tbearer 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? -
Replying to nickm:
Yup. "Zero microsections" is a time, after all. :)
Alternatively, we could do event_new() to allocate an event event_active() to force the event to become active immediately. We'd need to keep a pointer to the event around, though, so that we could event_free() it after it triggered.
OK. I'll go for it.
-
OK, I started something on my 'branch4312_dev' branch.
I have to leave now, so I'll leave it here in case you want to take it on.
It seems to work, but:
-
I'm not sure if asking for <event.h> to get event_base_once() in the non-bufferevents case is reasonable, or I should stick with the old approach in the non-bufferevents case.
-
I see a memleak in tor_tls_handshake() in the non-bufferevents case, which I haven't had the time to fix. I think it's caused by the way I close the connection in connection_or_close_connection_cb().
-
-
Check out branch
bug4312.I'm still somewhat worrying about the callback trigger timing. For example, if someone sends excess renegotiation requests and manages to mark the connection for close before the next event loop, after the callback is triggered we will have a double mark-for-close. Or, if the excess-reneg. callback is called after second_elapsed_callback() and close_closeable_connections() is called, then the callback pointer will be NULLed if the connection was marked-for-close.
(btw, seems like the memleak of comment:13 was not caused by this code; my branch parent was kinda old and it was caused by #4252 (moved).)
-
Replying to nickm:
Make that:
Oh, one last thing before I merge: have you verified that a correct v2 handshake (as you'd get from a client running Tor 0.2.2) still works with servers (with and without bufferevents) running this patch?
I have tested both cases with a client running maint-0.2.2. It worked.
I haven't done:
So, the usual way to avoid double-marks is just to check for a mark before you go on with the >rest of the code.
should we do it?
-
Replying to nickm:
I merged, and tried fixing a couple of other things too. Please have a look and make sure I didn't mess anything up?
Looks good!
-
Trac:
Parent: N/A to #4668 (moved) mentioned in issue #4594 (moved)
mentioned in issue #4662 (moved)
mentioned in issue #4668 (moved)
mentioned in issue #4672 (moved)
moved to tpo/core/tor#4312 (closed)
mentioned in issue tpo/core/tor#4594 (closed)
mentioned in issue tpo/core/tor#4668 (closed)
mentioned in issue tpo/core/tor#4672 (closed)