Opened 8 years ago

Closed 2 years ago

#4587 closed defect (worksforme)

Bugs in tor_tls_got_client_hello()

Reported by: Sebastian Owned by: nickm
Priority: High Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tls, handshake, tor-client
Cc: asn, stars@… Actual Points:
Parent ID: #4668 Points:
Reviewer: Sponsor:

Description

irc backlog, because troll refuses to use trac again...

< frosty_un> seems like not so much openssl guru in the devs that why you going to wrong ways. it's a last of my try (non guru too, however).
< frosty_un> "log_warn(LD_BUG, "Got a renegotiation request but we don't"" no, no, no. it's remotely trigerable thing, it's can't be BUG.
< frosty_un> read the openssl code.
< frosty_un> ssl3_check_client_hello() just during reading client certs.
< frosty_un> two hello in the row triggers such warn.
< frosty_un> ok, thats your way. i did a try.
< frosty_un> "Looks good!"
< troll> it's even more fun bug than warns. fun. excess_renegotiations_callback is NULL before handshake complete.
< troll> NULL(NULL) whatt a nice func.

Child Tickets

Attachments (1)

clienthellotest.py (787 bytes) - added by asn 8 years ago.
Stupid script that sends two ClientHellos.

Download all attachments as: .zip

Change History (33)

comment:1 Changed 8 years ago by nickm

Cc: asn added

Fix for the NULL(NULL) case in 617617e21a2d3.

wrt the rest of this, I no longer see any reason to delay setting these callbacks till after the first renegotiation, now that we're counting hellos rather than triggering on every one. I'll upload a simple patch; it will need testing. asn, will you have a chance?

comment:2 Changed 8 years ago by nickm

Status: newneeds_review

Please review branch bug4587 in my public repo, and test it (especially server-side, doing 0.2.2 handshakes)

comment:3 Changed 8 years ago by rransom

06:20 < troll> re #4587. you missed a point. second hello doesnt mean reneg only it's mean hello,hello,hel for first non complete handshake. no 
               reneg even, no .

comment:4 Changed 8 years ago by rransom

06:26 < troll> tls->server_handshake_count == 2 doesn;t mean reneg. it's mean only second hello that tor sees. it's a wrong way to detect re 
               negatiations.
06:27 < rransom_> Is there a right way?

06:29 < troll> yes. just to count hello after TLS_DONE
06:29 < troll> if you wan to disable that hello too, it's another story.
06:30 < troll> I guess infinite helloes not a goot too, even for handshake.

comment:5 Changed 8 years ago by stars

Cc: stars@… added

Hi,

Today i get a bug report in my log: tor_tls_got_client_hello(): Bug: Got a renegotiation request but we don't have a renegotiation callback set!

I running : Tor version : "0.2.3.8-alpha-dev (git-58d1aa44023e8b45)"
Libevent git version: commit f3b89dec9eac2cf4000c8dc9467abdbf27121674
I running Kubuntu Lucid 10.02.4 LTS 64 bits bufferevents enabled
vidalia 0.31-git
OpenSSL 1.0.0e 6 Sep 2011

BTW: i running a relay "SwissTorHelp"

Best regards

SwissTorExit

comment:6 Changed 8 years ago by asn

FML. Fuck SGC as well, for ruining my 'One ClientHello per handshake' mental assert.

As a fix, going by troll's suggestion, should we add another flag to tor_tls_t saying "Next ClientHello is a new handshake request."?
It will be toggled ON by default, and it will get toggled OFF when we increase the server_handshake_count at tor_tls_got_client_hello(). It will be toggled ON again when we get to SSL_ST_OK. The server_handshake_count will only be increased if the flag is ON.

If the SSL_ST_OK state only occurs at the end of an SSL handshake, we will only consider the first ClientHello as a handshake request, and count handshakes (and renegotiations) correctly. I have not checked OpenSSL's code to see if SSL_ST_OK appears only in the end of the SSL handshake.

What do the OpenSSL gurus think?

comment:7 in reply to:  6 ; Changed 8 years ago by nickm_mobile

Replying to asn:

FML. Fuck SGC as well, for ruining my 'One ClientHello per handshake' mental assert.

As a fix, going by troll's suggestion, should we add another flag to tor_tls_t saying "Next ClientHello is a new handshake request."?
It will be toggled ON by default, and it will get toggled OFF when we increase the server_handshake_count at tor_tls_got_client_hello(). It will be toggled ON again when we get to SSL_ST_OK. The server_handshake_count will only be increased if the flag is ON.

If the SSL_ST_OK state only occurs at the end of an SSL handshake, we will only consider the first ClientHello as a handshake request, and count handshakes (and renegotiations) correctly. I have not checked OpenSSL's code to see if SSL_ST_OK appears only in the end of the SSL handshake.

What do the OpenSSL gurus think?

First, I'd like to know if the fix I suggested (branch bug 4587 in my repo) works to address the issue stars is reporting above.

That done, I want to see if the TLS rfc actually allows multiple clienthellos for any purpose othef than rengeotiations. If not, we should call >2 clienthellos forbidden anyways, and just edit the log message to be less dire. IM(basically unconsidered) opinion.

comment:8 in reply to:  7 ; Changed 8 years ago by asn

Replying to nickm_mobile:

Replying to asn:

FML. Fuck SGC as well, for ruining my 'One ClientHello per handshake' mental assert.

As a fix, going by troll's suggestion, should we add another flag to tor_tls_t saying "Next ClientHello is a new handshake request."?
It will be toggled ON by default, and it will get toggled OFF when we increase the server_handshake_count at tor_tls_got_client_hello(). It will be toggled ON again when we get to SSL_ST_OK. The server_handshake_count will only be increased if the flag is ON.

If the SSL_ST_OK state only occurs at the end of an SSL handshake, we will only consider the first ClientHello as a handshake request, and count handshakes (and renegotiations) correctly. I have not checked OpenSSL's code to see if SSL_ST_OK appears only in the end of the SSL handshake.

What do the OpenSSL gurus think?

First, I'd like to know if the fix I suggested (branch bug 4587 in my repo) works to address the issue stars is reporting above.

Without yet testing it, I believe that your fix would fix stars' issue. As in that the callback will be set from the very start and it will never trigger any logging.

I will test it shortly.

That done, I want to see if the TLS rfc actually allows multiple clienthellos for any purpose othef than rengeotiations. If not, we should call >2 clienthellos forbidden anyways, and just edit the log message to be less dire. IM(basically unconsidered) opinion.

Check out this openssl-dev thread:
http://www.mail-archive.com/openssl-dev@openssl.org/msg03590.html
tl;dr the RFC is kinda vague, but extensions like SGC require multiple ClientHellos, and that's why OpenSSL supports them. Of course, we don't care at all about SGC.

One problem I can see with the current code plus the bug4587 branch, is that if someone sends 2 ClientHellos (without doing renegotiation) and somehow finishes the SSL handshake, we will still run the renegotiation callback. I'm kinda deep in #4549 so I haven't evaluated if this is really realistic. Another thing is that I have no easy ways to send an arbitrary amount of ClientHellos to a server (s_client(1SSL) won't do such magic).

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

Replying to asn:

Replying to nickm_mobile:

Replying to asn:

FML. Fuck SGC as well, for ruining my 'One ClientHello per handshake' mental assert.

As a fix, going by troll's suggestion, should we add another flag to tor_tls_t saying "Next ClientHello is a new handshake request."?
It will be toggled ON by default, and it will get toggled OFF when we increase the server_handshake_count at tor_tls_got_client_hello(). It will be toggled ON again when we get to SSL_ST_OK. The server_handshake_count will only be increased if the flag is ON.

If the SSL_ST_OK state only occurs at the end of an SSL handshake, we will only consider the first ClientHello as a handshake request, and count handshakes (and renegotiations) correctly. I have not checked OpenSSL's code to see if SSL_ST_OK appears only in the end of the SSL handshake.

What do the OpenSSL gurus think?

First, I'd like to know if the fix I suggested (branch bug 4587 in my repo) works to address the issue stars is reporting above.

Without yet testing it, I believe that your fix would fix stars' issue. As in that the callback will be set from the very start and it will never trigger any logging.

I will test it shortly.

I didn't manage to reproduce stars' problem with BFMI (by DoSing a relay with renegotiations). It can surely be reproduced by sending two ClientHellos on a single handshake to the relay, but I don't know how to test this. There are probably more code paths that lead there (because of the way we assign the callback), but I haven't had time to find them.
Assigning the callback as early as you do in bug4587 seems safe, since we haven't even run connection_start_reading() yet. But really, this code is tricky, and saying "seems safe" without testing, is an invitation for more silly bugs (and even more troll trolling).

Finally, I think we should find a way to make sure that server_handshake_count is indeed the number of SSL handshakes that have been initiated (comment:6), and not just the number of ClientHellos we have received (or not even that: #4594). After that, we can boot clients that send us more than one ClientHello per handshake, or do whatever. I suspect that if we let server_handshake_count be as arbitrary as it currently is, we are gonna run into more problems soon (comment:8). What do you believe?

comment:10 Changed 8 years ago by nickm

So, I think the right answer is to test the heck out of the pending patches, then merge them. Most important is to test with Tor clients.

Supporting stuff like SGC is interesting, but IMO not required at first: what we need most is to avoid crashes and bugs where valid handshakes don't finish; then to get the details all right.

I would indeed prefer that our handshake count in fact be a count of the number of times the client has tried to initiate a handshake.

comment:11 Changed 8 years ago by nickm

We'll also want a look at #4594 for getting the counts right.

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

Hm, looking at #4594. Do you think we can get the correct count of handshakes by only counting them in SSL3_ST_SR_CLNT_HELLO_A?

comment:13 Changed 8 years ago by asn

0452f34e of your bug4587 does not rate-limit renegotiations.

tor_tls_finish_handshake() does SSL_set_info_callback(tls->ssl, NULL); which doesn't get us into the tor_tls_state_changed_callback().
Before 0452f34e we reset the callback in tor_tls_set_renegotiate_callbacks(), but now we call tor_tls_set_renegotiate_callbacks() in the very beginning and the callback is reset too soon.

comment:14 Changed 8 years ago by asn

OK, I've been doing some tests with master plus 0452f34e plus 103d5ef3 plus:

diff --git a/src/common/tortls.c b/src/common/tortls.c
index b4d81de..8507069 100644
--- a/src/common/tortls.c
+++ b/src/common/tortls.c
@@ -1580,7 +1580,6 @@ tor_tls_set_renegotiate_callbacks(tor_tls_t *tls,
   tls->excess_renegotiations_callback = cb2;
   tls->callback_arg = arg;
   tls->got_renegotiate = 0;
-  SSL_set_info_callback(tls->ssl, tor_tls_state_changed_callback);
 }
 
 /** If this version of openssl requires it, turn on renegotiation on
@@ -1784,7 +1783,6 @@ tor_tls_finish_handshake(tor_tls_t *tls)
 {
   int r = TOR_TLS_DONE;
   if (tls->isServer) {
-    SSL_set_info_callback(tls->ssl, NULL);
     SSL_set_verify(tls->ssl, SSL_VERIFY_PEER, always_accept_verify_cb);
     /* There doesn't seem to be a clear OpenSSL API to clear mode flags. */
     tls->ssl->mode &= ~SSL_MODE_NO_AUTO_CHAIN;

Preliminary testing shows it to work. Bufferevent/non\ bufferevent version, successfully rate-limits renegotiations, does not crash with the DoS tool [0], and completes v1/v2/v3 handshake.

I haven't found a way to send two ClientHellos, but I was thinking of trying an MS IE version that supports SGC and pointing it to a relay. We still haven't fixed the fact that server_handshake_count is incorrect (but see comment:12 for an idea that a quick browsing of the OpenSSL code seems to support, but I haven't had the time to test it or look at the ssl3_accept() thoroughly.)

Please test more before merging anything, and don't be afraid of pulling the whole code out of master if you don't have enough time.

[0]:
I'm stress testing by doing:
thc-ssl-dosit() { while :; do (while :; do echo R; done) | openssl s_client -msg -connect 127.0.0.1:6666 2>/dev/null; done }
and then calling thc-ssl-dosit, as suggested in http://www.thc.org/thc-ssl-dos/. You can also try the tool itself.

comment:15 in reply to:  12 ; Changed 8 years ago by troll_un

Replying to asn:

Hm, looking at #4594. Do you think we can get the correct count of handshakes by only counting them in SSL3_ST_SR_CLNT_HELLO_A?

No you can't distinguish second hello during handshake and hello for reneg req. ssl3_get_client_hello() could return negative as reason non blocking io, so state during next call of ssl3_accept() can be as well as SSL3_ST_SR_CLNT_HELLO_B or SSL3_ST_SR_CLNT_HELLO_C. And SSL3_ST_SR_CLNT_HELLO_C is state that used if second hello during hanshake happened.

You need to wait for _A or _B or _C, and count (all) hellos. For distinguish reneg req only you need another logic (not depends of state during callbacks).

comment:16 in reply to:  15 Changed 8 years ago by troll_un

Replying to troll_un:

Replying to asn:

Hm, looking at #4594. Do you think we can get the correct count of handshakes by only counting them in SSL3_ST_SR_CLNT_HELLO_A?

For distinguish reneg req only you need another logic (not depends of state during callbacks).

Actually, You could detect parasitic hello instead of try to detect correct count of handshakes (in other words all that not a parasitic hello will be reneg request).

You need to remember the ssl->state for each callback and then to compare previous state and current state. Working condition could be looks like:

 if ((prev == SSL3_ST_SR_CERT_A || prev == SSL3_ST_SR_CERT_B) &&
     curr == SSL3_ST_SR_CLNT_HELLO_C) { /* parasitic hello detected */

comment:17 Changed 8 years ago by nickm

asn: "0452f34e plus 103d5ef3 " don't seem to refer to any commit I can see.

I'm merging my 4587 fix plus your fixes in the diff above, to make stars's issue better. (Yes, I know this is still not right, but this should make it less b0rken than before. I hope I can get to looking at all the proposed fixes on Thursday or Friday, when my brain gets out of "merge features" mode into "fix all the bugs!" mode.)

comment:18 Changed 8 years ago by nickm

Keywords: tls handshake added
Priority: normalmajor

comment:19 Changed 8 years ago by nickm

Owner: set to nickm
Status: needs_reviewassigned

Changed 8 years ago by asn

Attachment: clienthellotest.py added

Stupid script that sends two ClientHellos.

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

Replying to nickm:

asn: "0452f34e plus 103d5ef3 " don't seem to refer to any commit I can see.

I meant commit 0452f34e and commit 103d5ef3 (That wasn't an actual mathematical addition!).
103d5ef3 is "Make pending libevent actions cancelable" and 0452f34e is "Set renegotiation callbacks immediately on tls inititation".

I attached a stupid little python script which sends two ClientHello records, triggering the bug of this ticket. It doesn't seem to do anything in the current master, but I'll do more tests to make sure.

My next goal is to find a correct way to count renegotiations (also, check out attachment:openssl.svg:ticket:4594).

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

Replying to asn:

Replying to nickm:

asn: "0452f34e plus 103d5ef3 " don't seem to refer to any commit I can see.

I meant commit 0452f34e and commit 103d5ef3 (That wasn't an actual mathematical addition!).
103d5ef3 is "Make pending libevent actions cancelable" and 0452f34e is "Set renegotiation callbacks immediately on tls inititation".

The above hashes would be correct in an alternate universe where I understood git. In this universe, the hashes above were pointing to my local tree. In any case, you merged "Make pending libevent actions cancelable" and "Set renegotiation callbacks immediately on tls inititation" which is what I was thinking as well.

comment:22 Changed 8 years ago by asn

I forgot to mention that if you send a ClientHello when the server expects a Certificate (SSL3_ST_SR_CERT_A or SSL3_ST_SR_CERT_B), the server will go back to SSL3_ST_SR_CLNT_HELLO_C, restart the handshake, send a ServerHello, a Certificate and a ServerKeyExchange. So you don't need to issue a renegotiation to load the server through a single TCP connection.

This means that if we are serious about SSL DoS vectors, we must discard any clients doing the above. Furthermore, implementing this, immediately introduces a bridge fingerprint, which this time cannot simply be removed by disabling renegotiation/v2.

(You can PoC this using my clienthellotest.py)

comment:23 Changed 8 years ago by asn

Parent ID: #4668

comment:24 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

comment:25 Changed 7 years ago by nickm

Keywords: tor-client added

comment:26 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:27 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

comment:28 Changed 6 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.???

comment:29 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:30 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:31 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:32 Changed 2 years ago by nickm

Resolution: worksforme
Severity: Normal
Status: assignedclosed

Okay, this ticket is a mess, but it appears that we merged fixes for many of the above issues. In the long term, the multi-clienthello vector is probably just better off getting fixed through dropping v2 handshakes and migrating to TLS 1.3

Note: See TracTickets for help on using tickets.