[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:8 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.
 >

 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?

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