Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#4312 closed defect (implemented)

Rate limit renegotiations

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: nickm Actual Points:
Parent ID: #4668 Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

Change History (26)

comment:1 Changed 8 years ago by asn

Status: newneeds_review

The branch bug4312 in git://gitorious.org/mytor/mytor.git attempts to fix this for the non-bufferevents case.

Please review the branch carefully.

comment:2 Changed 8 years ago by asn

Oh yeah.

Through my fix, I mentally assert that:

comment:3 Changed 8 years ago by asn

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.

comment:4 Changed 8 years ago by nickm

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.

comment:5 Changed 8 years ago by asn

<asn> wrt #4312, it does *not* seem to work in the bufferevents case.
<asn> I'm not sure how SSL is handled in USE_BUFFEREVENTS
<asn> and I suspect it's handled by libevent
<asn> because I don't see connection_or_handle_event_cb() getting called after the

initial SSL handshake.

<asn> so I suspect that libevent is internally calling SSL_read() or SSL_write(), is completing renegotiations and is not reporting back to tor.
<asn> The renegotiated callback in the bufferevents case is called from

connection_or_process_inbuf() when the first SSL data packet arrives.

comment:6 in reply to:  4 Changed 8 years ago by asn

Pushed fixes for comment:4.

bufferevents case is still *not* working.

comment:7 Changed 8 years ago by 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.

comment:8 in reply to:  7 ; Changed 8 years ago by 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_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.)

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.

comment:9 in reply to:  8 ; Changed 8 years ago by 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_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.

comment:10 in reply to:  9 Changed 8 years ago by asn

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_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?

comment:11 Changed 8 years ago by 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.

comment:12 in reply to:  11 Changed 8 years ago by asn

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.

comment:13 Changed 8 years ago by asn

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().

comment:14 Changed 8 years ago by nickm

Hmm. <event.h> is kinda deprecated for direct use in Tor -- we try to do everything libevent-related via compat_libevent.h, so that we can get the new improved libevent2 headers if they're present, and fall back to the old boring libevent1 headers if not.

comment:15 Changed 8 years ago by asn

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.)

comment:16 Changed 8 years ago by nickm

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.

Once close_closeable_connections() is called on a connection, the bufferevent should be freed, and it shouldn't be doing any more SSL calls at all.

I'll check out the code more later on.

comment:17 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-final

comment:18 Changed 8 years ago by nickm

This looks pretty good to me! I'm going to re-read it one last time, merge it, rename a couple of things, and call it done.

comment:19 Changed 8 years ago by nickm

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 running this patch?

comment:20 in reply to:  19 ; Changed 8 years ago by 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?

comment:21 in reply to:  20 Changed 8 years ago by asn

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?

comment:22 Changed 8 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

I merged, and tried fixing a couple of other things too. Please have a look and make sure I didn't mess anything up?

comment:23 in reply to:  22 Changed 8 years ago by asn

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!

comment:24 Changed 8 years ago by asn

Parent ID: #4668

comment:25 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:26 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.