[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
connection_read_to_buf()
<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 ==
SSL_ST_OK)"
<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 ==
TOR_TLS_WANTREAD || err == TOR_TLS_WANTWRITE)) {
<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
it.
<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
callbacks.
<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)
++tls->server_handshake_count;
- 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 @@
tor_tls_got_client_hello(tls);
}
+ if (type == SSL_CB_HANDSHAKE_DONE)
+ if (tls->server_handshake_count == 2)
+ tls->got_renegotiate = 1;
#endif
}
@@ -1641,12 +1637,7 @@
tor_assert(tls->state == TOR_TLS_ST_OPEN);
tor_assert(len<INT_MAX);
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,
LD_NET);
#ifdef V2_HANDSHAKE_SERVER
if (tls->got_renegotiate) {
@@ -1661,10 +1652,16 @@
tls->negotiated_callback(tls, tls->callback_arg);
tls->got_renegotiate = 0;
- return r;
}
#endif
+ 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,
LD_NET);
+
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