[tor-bugs] #4670 [Tor Relay]: More bugs on renegotiation limiting code.

Tor Bug Tracker & Wiki torproject-admin at torproject.org
Thu Dec 8 04:08:03 UTC 2011

#4670: More bugs on renegotiation limiting code.
 Reporter:  asn        |          Owner:                    
     Type:  defect     |         Status:  new               
 Priority:  normal     |      Milestone:  Tor: 0.2.4.x-final
Component:  Tor Relay  |        Version:                    
 Keywords:             |         Parent:  #4668             
   Points:             |   Actualpoints:                    

Comment(by asn):

 <wanoskarnet> re bug4626_try*, you don't want to call callback unless ssl
 error is want_read or want_write.
 <wanoskarnet> maybe thats not a good too. some pending bytes can make bug
 such complicate as it is.
 <wanoskarnet> not because callback but returned value to
 <wanoskarnet> after callbck.
 <asn> you mean that returning TOR_TLS_WANTREAD or TOR_TLS_WANTWRITE to
 connection_read_to_buf() after calling the renegotiation callback, might
 cause a bug?
 <wanoskarnet> want_write not a such bad. it's return 0. nut want_read is
 more different.
 <wanoskarnet> but there are version cell and conn could start writing
 during reneg. so it calls ssl_write with real bytes during reneg.
 <asn> Hm, this should happen on the client side.
 <asn> But what do you think it's the problem with this?
 <wanoskarnet> that changes ssl->state
 <asn> When you call ssl_write during reneg., shouldn't you get back a
 WANT_WRITE error?
 <wanoskarnet> debug logs could help here.
 <wanoskarnet> can't get why connection_read_to_buf() process later if
 want_read happened.
 <wanoskarnet> yes SSL_write during handhsake or reneg it's a way to real
 bugs. ssl3_write_bytes(): it's can't to ccontinue handhsake if state non
 of _ACCEPT or _CONNECT. Then it mess with crypto (non inited yet) stuff.
 <wanoskarnet> every server's state during hadshake is xxx|SSL_ST_ACCEPT,
 so SSL_write() can't be so bad as I thought.

 Above discussion is about the concerns of comment:14:ticket:4626 and about
 calling SSL_write() during an SSL handshake.

 <wanoskarnet> the logic of 4fd79f9def289965 is broken. you can't to call
 connection_or_check_valid_tls_handshake() just after wantread from
 ssl_read(), that doesn't means that certs recved yet.
 <asn> got_renegotiate && WANTREAD, does it for most cases
 <asn> but in certain high-traffic scenarios, indeed, the certs might not
 be there.
 <wanoskarnet> no for most cases there are no certs at all. server just
 send hello and keys and returns -1 as non blockin io (no yet answer with
 cert and finish by client).
 <wanoskarnet> callback that sets got_renegotiate happens during first
 ssl_read just after client hello.
 <wanoskarnet> the fix is "if (tls->got_renegotiate && tls->ssl->state ==
 <wanoskarnet> instead of "if (tls->got_renegotiate)"
 <wanoskarnet> plus bug4626_try2 would be nice (645d31f4)
 <asn> if (tls->got_renegotiate && tls->ssl->state == SSL_ST_OK && (err ==
 <asn> for extra assurance?
 <wanoskarnet> for SSL_ST_OK only TOR_TLS_WANTREAD must be valid.
 <wanoskarnet> unless "Guess we're wrong about how SSL_read works"
 <asn> yes, that's what I'm afraid.

 The above is what lead to the branch `bug4626_callback_conditions_theory`
 (commit `b6c18f81f71`).

 <wanoskarnet> ugh. ssl3_read_bytes(). seems like if SSL_ST_OK after
 calling handshake then it going to get record. if any one record there
 then it returns number of bytes not a error. most times it's impossible
 because non-blocking io, but non impossible. SSL_read() can return non
 error if reneg for some extremal cases.
 <wanoskarnet> it's ever worse if record is client hello again.
 <wanoskarnet> wrong fix.
 <wanoskarnet> I guess we need to use SSL_CB_HANDSHAKE_DONE ssl's callback
 for calling reneg callback.
 <wanoskarnet> and leave tor_tls_read() to do the read work.
 <wanoskarnet> better to fix logic for "tls->got_renegotiate = 1", so it
 happened only after callback with type == SSL_CB_HANDSHAKE_DONE
 <wanoskarnet> the fix https://pastee.org/y4nu9
 <wanoskarnet> calling of negotiated_callback() if fatal link error is not
 so good idea too, so need to fix it.
 <wanoskarnet>  ugh. wrong fix. second client hello during handshake breaks
 <wanoskarnet> something wrong with basis of reneg detect code I think
 <wanoskarnet> bug4626_callback_conditions_theory is wrong about how
 SSL_read() works. all of reneg limit stuff is wrong, but this is worse as
 idea for limit of reneg and better as idea to correctly call of reneg
 <wanoskarnet> reneg limit stufff must be rewriten with another logic.
 <wanoskarnet> asn's b6c18f81f71 works just because rfc's client sends
 application data only after server's 'finished'. That doesn't mean evil
 client can't send application (hello again) just after it sent client's
 'finished'. Then it could trigger bug warn with remind about not a smart
 coders and SSL_read.

 wanoskarnet explains how SSL_read() can return a positive integer *and* do
 a renegotiation, which would break the tor_tls_read() of `reapply_4312`.

 --- tortls.c.original
 +++ tortls.c
 @@ -1311,14 +1311,7 @@
    if (tls->server_handshake_count < 3)

 -  if (tls->server_handshake_count == 2) {
 -    if (!tls->negotiated_callback) {
 -      log_warn(LD_BUG, "Got a renegotiation request but we don't"
 -               " have a renegotiation callback set!");
 -    }
 -    tls->got_renegotiate = 1;
 -  } else if (tls->server_handshake_count > 2) {
 +  if (tls->server_handshake_count > 2) {
      /* 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
 @@ -1383,6 +1376,9 @@

 +  if (type == SSL_CB_HANDSHAKE_DONE)
 +    if (tls->server_handshake_count == 2)
 +      tls->got_renegotiate = 1;

 @@ -1641,12 +1637,7 @@
    tor_assert(tls->state == TOR_TLS_ST_OPEN);
    r = SSL_read(tls->ssl, cp, (int)len);
 -  if (r > 0) /* return the number of characters read */
 -    return r;

 -  /* If we got here, SSL_read() did not go as expected. */
 -  err = tor_tls_get_error(tls, r, CATCH_ZERO, "reading", LOG_DEBUG,

    if (tls->got_renegotiate) {
 @@ -1661,10 +1652,16 @@
        tls->negotiated_callback(tls, tls->callback_arg);
      tls->got_renegotiate = 0;

 -    return r;

 +  if (r > 0) /* return the number of characters read */
 +    return r;
 +  /* If we got here, SSL_read() did not go as expected. */
 +  err = tor_tls_get_error(tls, r, CATCH_ZERO, "reading", LOG_DEBUG,
    if (err == _TOR_TLS_ZERORETURN || err == TOR_TLS_CLOSE) {
      log_debug(LD_NET,"read returned r=%d; TLS is closed",r);
      tls->state = TOR_TLS_ST_CLOSED;

 The above is what https://pastee.org/y4nu9 (referenced in the previous
 log) contained before it expired.

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

More information about the tor-bugs mailing list