Opened 4 weeks ago

Closed 4 weeks ago

#23610 closed defect (fixed)

handle_establish_intro() can mark circuits for close twice

Reported by: teor Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: 0.3.2.1-alpha
Severity: Normal Keywords: prop224, tor-hs, tor-bug-warning, 030-backport, 031-backport
Cc: Actual Points:
Parent ID: Points: 1
Reviewer: Sponsor:

Description

Some of the failure cases in handle_verified_establish_intro_cell() mark the circuit for close. So if handle_establish_intro() tries to mark it for close again, it will trigger a bug warning.

I think this is the only case in the v3 introduce protocol. We should check for cases like this in the v3 rend protocol as well.

  /* This cell is legit. Take the appropriate actions. */
  cell_ok = handle_verified_establish_intro_cell(circ, parsed_cell);
  if (cell_ok < 0) {
    goto err;
  }

  log_warn(LD_GENERAL, "Established prop224 intro point on circuit %" PRIu32,
           circ->p_circ_id);

  /* We are done! */
  retval = 0;
  goto done;

 err:
  circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_TORPROTOCOL);

Child Tickets

Change History (8)

comment:1 in reply to:  description Changed 4 weeks ago by teor

Replying to teor:

We should check for cases like this in the v3 rend protocol as well.

The v2 and v3 rend protocols are the same, and there are no additional instances of this bug in the rend protocol.

comment:2 Changed 4 weeks ago by dgoulet

Owner: set to dgoulet
Status: newaccepted

Indeed, hs_intro_send_intro_established_cell() might mark the circuit for close if relay_send_command_from_edge() errors.

comment:3 Changed 4 weeks ago by dgoulet

Status: acceptedneeds_review

See branch: bug23610_032_01

comment:4 Changed 4 weeks ago by nickm

Resolution: fixed
Status: needs_reviewclosed

looks fine; merging

comment:5 Changed 4 weeks ago by dgoulet

Keywords: 030-backport 031-backport added
Resolution: fixed
Status: closedreopened

I made a small mistake, this applies to tor-0.3.0.1-alpha which means this could qualify for an 031 and 030 backport if we don't want to trigger BUG() (rare) on those versions.

So the changes file should just be modified with that. SORRY!!!!

comment:6 Changed 4 weeks ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.1.x-final
Resolution: fixed
Status: reopenedclosed

cherry-picked to 0.3.1, changes file fixed, merged forward.

comment:7 Changed 4 weeks ago by teor

Milestone: Tor: 0.3.1.x-finalTor: 0.3.0.x-final
Resolution: fixed
Status: closedreopened

Hang on, this needed to go back to 0.3.0 as well as 0.3.1.

comment:8 Changed 4 weeks ago by nickm

Resolution: fixed
Status: reopenedclosed

backported

Note: See TracTickets for help on using tickets.