Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#5644 closed defect (fixed)

rend_service_introduce() asserts circuit->rend_data before checking for proto violation

Reported by: asn Owned by:
Priority: High Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: #5643 Points:
Reviewer: Sponsor:

Description

...
#ifndef NON_ANONYMOUS_MODE_ENABLED
  tor_assert(!(circuit->build_state->onehop_tunnel));
#endif
  tor_assert(circuit->rend_data);

  base32_encode(serviceid, REND_SERVICE_ID_LEN_BASE32+1,
                circuit->rend_data->rend_pk_digest, REND_SERVICE_ID_LEN);
  log_info(LD_REND, "Received INTRODUCE2 cell for service %s on circ %d.",
           escaped(serviceid), circuit->_base.n_circ_id);

  if (circuit->_base.purpose != CIRCUIT_PURPOSE_S_INTRO) {
    log_warn(LD_PROTOCOL,
             "Got an INTRODUCE2 over a non-introduction circuit %d.",
             circuit->_base.n_circ_id);
    return -1;
  }

A bad exit might be able to exploit this by sending a RELAY_COMMAND_INTRODUCE2 cell to a client (through a CIRCUIT_PURPOSE_C_GENERAL circuit) and triggering tor_assert(circuit->rend_data);.

Child Tickets

Change History (8)

comment:1 Changed 8 years ago by asn

Component: - Select a componentTor Client
Milestone: Tor: 0.2.3.x-final

comment:2 Changed 8 years ago by asn

Status: newneeds_review

Please see bug5644 in https://git.gitorious.org/mytor/mytor.git.

I took the most minimal approach and simply dragged the protocol violation check to the beginning of the function. It might work.

I was also tempted to move some stuff below the length check, but I guess it was unneeded.

PS: I couldn't compact the commit message more.

comment:3 Changed 8 years ago by arma

If this is what it looks like it is, it should go into 0.2.2 as well, yes?

comment:4 in reply to:  3 Changed 8 years ago by asn

Milestone: Tor: 0.2.3.x-finalTor: 0.2.2.x-final

Replying to arma:

If this is what it looks like it is, it should go into 0.2.2 as well, yes?

True.

comment:5 Changed 8 years ago by nickm

Priority: normalmajor
Resolution: fixed
Status: needs_reviewclosed

Cherry-picked to 0.2.2 and merged forward. Please review both commits; I checked carefully, but might have messed something up.

comment:6 Changed 7 years ago by asn

The result looks good:
https://gitweb.torproject.org/tor.git/blob/HEAD:/src/or/rendservice.c#l1066
https://gitweb.torproject.org/tor.git/blob/refs/heads/maint-0.2.2:/src/or/rendservice.c#l911

I tried to trigger the bug on a patched master and I got:

[warn] Got an INTRODUCE2 over a non-introduction circuit 56994.

comment:7 Changed 7 years ago by nickm

Keywords: tor-client added

comment:8 Changed 7 years ago by nickm

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