tor_tls_state_changed_callback(): detects of ClientHello is too late

It's a git master 58d1aa44 with legacy/trac#4312 (moved) 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)

Trac:
Username: troll_un

changed milestone to %Tor: unspecified in legacy/trac

added component::core tor/tor in Legacy / Trac correctness in Legacy / Trac milestone::Tor: unspecified in Legacy / Trac parent::4668 in Legacy / Trac priority::high in Legacy / Trac reporter::troll_un in Legacy / Trac resolution::wontfix in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac tls in Legacy / Trac tor-relay in Legacy / Trac type::defect in Legacy / Trac version::tor 0.2.3.8-alpha in Legacy / Trac labels

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

Also see legacy/trac#4587 (moved) : the right check isn't counting clienthellos, but counting clienthellos when we were not previously handshaking.

Trac:
Priority: normal to major

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.

Trac:
Username: troll_un

Trac:

OpenSSL FSM

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 legacy/trac#4587 (moved).

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

Trac:
Parent: N/A to legacy/trac#4668 (moved)

Trac:
Keywords: N/A deleted, tls correctness added

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.

Trac:
Milestone: Tor: 0.2.3.x-final to Tor: 0.2.4.x-final

Trac:
Keywords: tls correctness deleted, tls correctness tor-relay added

Trac:
Component: Tor Relay to Tor

Trac:
Milestone: Tor: 0.2.4.x-final to Tor: 0.2.5.x-final

Trac:
Milestone: Tor: 0.2.5.x-final to Tor: 0.2.???

Milestone renamed

Trac:
Milestone: Tor: 0.2.??? to Tor: 0.3.???

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

Trac:
Milestone: Tor: 0.3.??? to Tor: unspecified
Keywords: tls correctness tor-relay deleted, tls, tor-relay, correctness, tor-03-unspecified-201612 added

Remove an old triaging keyword.

Trac:
Keywords: tor-03-unspecified-201612 deleted, N/A added

From legacy/trac#4587 (moved):

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

Trac:
Severity: N/A to Normal
Sponsor: N/A to N/A
Status: new to closed
Reviewer: N/A to N/A
Resolution: N/A to wontfix

closed

moved from legacy/trac#4594 (moved)

added Bug label and removed 1 deleted label

added Relay label and removed 1 deleted label