Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#4592 closed defect (fixed)

tor_tls_server_info_callback(): SSL3_ST_SW_SRVR_HELLO_B missed.

Reported by: troll_un Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version: Tor: 0.2.2.34
Severity: Keywords: tor-relay
Cc: stars@… Actual Points:
Parent ID: #4668 Points:
Reviewer: Sponsor:

Description

		case SSL3_ST_SW_SRVR_HELLO_A:
		case SSL3_ST_SW_SRVR_HELLO_B:
			ret=ssl3_send_server_hello(s);
			if (ret <= 0) goto end;
#ifndef OPENSSL_NO_TLSEXT
			if (s->hit)
				{
				if (s->tlsext_ticket_expected)
					s->state=SSL3_ST_SW_SESSION_TICKET_A;
				else
					s->state=SSL3_ST_SW_CHANGE_A;
				}
#else
			if (s->hit)
					s->state=SSL3_ST_SW_CHANGE_A;
#endif
			else
				s->state=SSL3_ST_SW_CERT_A;
			s->init_num=0;
			break;
			if ((cb != NULL) && (s->state != state))
				{
				new_state=s->state;
				s->state=state;
				cb(s,SSL_CB_ACCEPT_LOOP,1);
				s->state=new_state;
				}

As non blocking io ssl3_send_server_hello() can return -1 (can't to fit all bytes of record in wire), and no CallBack of tor's tor_tls_server_info_callback(). Later SSL_accept() calling callback with s->state == SSL3_ST_SW_SRVR_HELLO_B.

If relay can't to fit server's hello in wire once, it never can finish v2 link hanshake.

Child Tickets

Change History (14)

comment:1 Changed 8 years ago by nickm

See also #4594; I don't believe we can do both of these. The changes suggested in #4594 seem smarter to me.

comment:2 Changed 8 years ago by Sebastian

< frosty_un> oh puppets we are wrong, it's not about blocking tor protocol, it's about downgrade from v2 to v1 by enemy. not sure what is worsiest thing.
< frosty_un> we are talkking about expoiting #4592.
< frosty_un> HMAcs easy bypassing with tcp window, nice isn't?
< frosty_un>  viva la "This check is redundant". no rollback attack, only blocking tor protocol: server thinks it's a v2 while client using conn like v1.

comment:3 Changed 8 years ago by asn

Parent ID: #4668

comment:4 Changed 8 years ago by asn

This ticket applies to current code, but I was naughty and set its parent to #4668.

As far as I can tell, it's not exploitable, and it hasn't disrupted v2 handshake in real-life yet, otherwise we would have probably gotten bug reports with:

        log_warn(LD_BUG, "For some reason, wasV2Handshake didn't"
                 " get set. Fixing that.");

A patch that might fix this bug or it might break it even further could be:

if (!(((type == SSL_CB_ACCEPT_LOOP) && (ssl->state == SSL3_ST_SW_SRVR_HELLO_A)) ||
     ((type == SSL_CB_ACCEPT_EXIT) && (ssl->state == SSL3_ST_SW_SRVR_HELLO_B))))
   return;   

instead of the current:

  if (type != SSL_CB_ACCEPT_LOOP)
    return;
  if (ssl->state != SSL3_ST_SW_SRVR_HELLO_A)
    return;

in tor_tls_server_info_callback(). I thought of this fix carelessly, and I'm not a good thinker, so we will probably need to re-think it when we actually fix this issue.

We might want to fix this during 0.2.3.x or during 0.2.4.x along with the #4668 stuff.
In any case, the fix will need to be adjusted to any #4594 fixes.

comment:5 Changed 8 years ago by asn

wanoskarnet pointed out that suggested fix of comment:4 is wrong, since we would also be considering fatal SSL errors in SSL3_ST_SW_SRVR_HELLO_B.

A new, simpler and better fix could be:

if (type != SSL_CB_ACCEPT_LOOP)
  return;
if ((ssl->state != SSL3_ST_SW_SRVR_HELLO_A) || (ssl->state != SSL3_ST_SW_SRVR_HELLO_B))
  return;

comment:6 Changed 8 years ago by asn

Oops. This is more like it:

if (type != SSL_CB_ACCEPT_LOOP)
  return;
if ((ssl->state != SSL3_ST_SW_SRVR_HELLO_A) && (ssl->state != SSL3_ST_SW_SRVR_HELLO_B))
  return;

comment:7 in reply to:  4 Changed 8 years ago by arma

Replying to asn:

As far as I can tell, it's not exploitable, and it hasn't disrupted v2 handshake in real-life yet, otherwise we would have probably gotten bug reports with:

        log_warn(LD_BUG, "For some reason, wasV2Handshake didn't"
                 " get set. Fixing that.");

Ok, here's your bug report saying that it happens in the wild: #4873

comment:8 Changed 8 years ago by stars

Cc: stars@… added

comment:9 Changed 8 years ago by nickm

Sounds like we should apply one of these. Looking at the openssl code, my only worry here is that we might double-count handshakes, by triggering both for the SW_SRVR_HELLO_A and SW_SRVR_HELLO_B step of the same handshake. Is that a possibility do you think?

comment:10 in reply to:  9 Changed 7 years ago by nickm

Status: newneeds_review

Replying to nickm:

Sounds like we should apply one of these. Looking at the openssl code, my only worry here is that we might double-count handshakes, by triggering both for the SW_SRVR_HELLO_A and SW_SRVR_HELLO_B step of the same handshake. Is that a possibility do you think?

Ah, the "if (ret <= 0) goto end;" is the crucial bit. We shouldn't get an info_cb until one of the SW_SRVR_HELLO_* actually has a successful ssl3_send_server_hello, which will cause it to transition out. So asn's second patch above ought to be safe. Turning it into a branch as "bug4592".

My inner coward says to merge this into 0.2.3.x, though it would apply to 0.2.2.x too.

comment:11 Changed 7 years ago by nickm

Is this security-relevant? If so, I can argue for 0.2.2.x. Otherwise I'll just merge to 0.2.3.x.

comment:12 Changed 7 years ago by nickm

Milestone: Tor: 0.2.2.x-finalTor: 0.2.3.x-final
Resolution: fixed
Status: needs_reviewclosed

Merging to 0.2.3.x. I can't come up with a reason this has to go in 0.2.2.x

comment:13 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:14 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.