[tor-bugs] #4587 [Tor Client]: Bugs in tor_tls_got_client_hello()

[Tor Bug Tracker & Wiki]

#4587: Bugs in tor_tls_got_client_hello()
------------------------+---------------------------------------------------
 Reporter:  Sebastian   |          Owner:                    
     Type:  defect      |         Status:  needs_review      
 Priority:  normal      |      Milestone:  Tor: 0.2.3.x-final
Component:  Tor Client  |        Version:                    
 Keywords:              |         Parent:                    
   Points:              |   Actualpoints:                    
------------------------+---------------------------------------------------

Comment(by asn):

 Replying to [comment:7 nickm_mobile]:
 > Replying to [comment:6 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).

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/4587#comment:8>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online