Opened 8 years ago

Closed 2 years ago

#4594 closed defect (wontfix)

tor_tls_state_changed_callback(): detects of ClientHello is too late

Reported by: troll_un Owned by:
Priority: High Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.2.3.8-alpha
Severity: Normal Keywords: tls, correctness, tor-relay
Cc: Actual Points:
Parent ID: #4668 Points:
Reviewer: Sponsor:

Description

It's a git master 58d1aa4 with #4312 fixes.

  if (type == SSL_CB_ACCEPT_LOOP &&
      ssl->state == SSL3_ST_SW_SRVR_HELLO_A) {

    /* Call tor_tls_got_client_hello() for every SSL ClientHello we
       receive. */

As OpenSSL's code says, such conditions happens not after ClientHello recved. It happens already when serverhello sent. It's too late for accurate counting cleinthello with limit renegs.

Server shouldn't say hello if doesn't want a new clienthello.

Correct states for such case is

SSL3_ST_SR_CLNT_HELLO_A
SSL3_ST_SR_CLNT_HELLO_B SSL3_ST_SR_CLNT_HELLO_C (reason is non blocking io)

Child Tickets

Attachments (1)

openssl.svg (112.5 KB) - added by asn 8 years ago.
OpenSSL FSM

Download all attachments as: .zip

Change History (15)

comment:1 Changed 8 years ago by nickm

Priority: normalmajor

See also #4592 : we can do only one of those. I think this one is smarter: want to detect handshakes as early as possible.

Also see #4587 : the right check isn't counting clienthellos, but counting clienthellos when we were not previously handshaking.

comment:2 Changed 8 years ago by troll_un

But early detect have no sense:

    /* We got more than one renegotiation requests. The Tor protocol
       needs just one renegotiation; more than that probably means
       They are trying to DoS us and we have to stop them. We can't
       close their connection from in here since it's an OpenSSL
       callback, so we set a libevent timer that triggers in the next
       event loop and closes the connection. */

    if (tor_run_in_libevent_loop(tls->excess_renegotiations_callback,
                                 tls->callback_arg) < 0) {
      log_warn(LD_GENERAL, "Didn't manage to set a renegotiation "
               "limiting callback.");
    }

Server anyway continues handshaking while looping ssl3_accept() and sends hellos and key stuff and anything that it can to do for nonblocking io.

Changed 8 years ago by asn

Attachment: openssl.svg added

OpenSSL FSM

comment:3 Changed 8 years ago by asn

In some offline time I made the above attached state diagram of an OpenSSL server (s3_srvr.c).

It might help us with this ticket and #4587.

Note that the aesthetics of it are not optimal and there might be some bugs too. Tomorrow I'll look some more into it.

(I made it with a small application called qfsm. Use at your own risk. I will make a git repository with my 'qfsm file' soon.)

comment:4 Changed 8 years ago by asn

Parent ID: #4668

comment:5 Changed 8 years ago by nickm

Keywords: tls correctness added

comment:6 Changed 7 years ago by nickm

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

IIUC, this isn't a correctness issue so much as it is an anti-renegotiation-DOS issue. It matters when we go back and try to merge in the reverted renegotiation-counting code again in 0.2.4.x. For 0.2.3.x, though, it doesn't help.

comment:7 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:8 Changed 7 years ago by nickm

Component: Tor RelayTor

comment:9 Changed 7 years ago by nickm

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

comment:10 Changed 6 years ago by nickm

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

comment:11 Changed 3 years ago by teor

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

Milestone renamed

comment:12 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:13 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:14 Changed 2 years ago by nickm

Resolution: wontfix
Severity: Normal
Status: newclosed

From #4587:

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.