Opened 3 months ago

Closed 3 months ago

#30775 closed defect (fixed)

Crash in close_or_reextend_intro_circ() (not released)

Reported by: asn Owned by:
Priority: Very High Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs bug 041-must stability 041-regression
Cc: asn, nickm, mikeperry Actual Points:
Parent ID: #30773 Points:
Reviewer: dgoulet Sponsor:

Description (last modified by asn)

There is a UAF in:

  if (!TO_CIRCUIT(intro_circ)->marked_for_close) {
    circuit_change_purpose(TO_CIRCUIT(intro_circ),
                           CIRCUIT_PURPOSE_C_INTRODUCE_ACKED);
    circuit_mark_for_close(TO_CIRCUIT(intro_circ), END_CIRC_REASON_FINISHED);
  }
  /* Close the related rendezvous circuit. */
  rend_circ = hs_circuitmap_get_rend_circ_client_side(
                                     intro_circ->hs_ident->rendezvous_cookie);

exact same bug class as #30773.

Child Tickets

Change History (7)

comment:1 Changed 3 months ago by asn

Description: modified (diff)

comment:2 Changed 3 months ago by asn

So it's obvious that this needs the same fix as #30771, but I don't think we should whack-a-mole this code pattern because it seems to be quite frequent and also there is nothing actually disallowing it.

I think we need some sort of other fix for this bug class. Here is some possible avenues since this is blocking the upcoming alpha release:

a) Change the approach of #29034 to not free hs_ident/rend_data upon repurpose, and just remove from circuitmap. Then hs_ident/rend_data will be freed upon final circuit free. The danger here might be that some part of the code might be using hs_ident/rend_data instead of the circuitmap (e.g. look at circuit_get_ready_rend_circ_by_rend_data() where we are saved by the purpose check). Me and David took a look at the code and we didnt find anything obvious but this doesn't mean too much.

b) Revert parts (or the entirety) of #29034 and #28780 (and/or #28634).

c) Continue whack-a-moling but this does not seem like a stress-free thing to do, since these bugs can get pretty nasty.

Feedback is very much appreciated.

comment:3 Changed 3 months ago by asn

Priority: MediumVery High

comment:4 Changed 3 months ago by nickm

Keywords: 041-regression added

comment:5 Changed 3 months ago by nickm

I think we should also consider

d) Audit all the code that calls circuit_mark_for_close() with a reason that implies that the circuit might stay alive.

comment:6 Changed 3 months ago by dgoulet

Cc: asn nickm mikeperry added
Reviewer: dgoulet
Status: newmerge_ready

Mike's branch has the appropriate fix: https://github.com/torproject/tor/pull/1076

I agree the fix is correct. Reverting #29034 and simply cleaning up the circuit from the HS map (which is what the mark for close is doing thus expected) and let the removal of the hs_ident/rend_data to the circuit free function.

ACK.

comment:7 Changed 3 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged.

If you think that c525135dac354892a45ad3d2f6de9450d393f09f should be backported, please make a separate commit for that?

Note: See TracTickets for help on using tickets.