Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#5541 closed defect (fixed)

NULL ptr deref. in connection_edge_process_relay_cell()

Reported by: asn Owned by:
Priority: Very Low Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In connection_edge_process_relay_cell(), if conn is NULL (because !rh.stream_id in relay_lookup_conn()), and the cell command is RELAY_COMMAND_DATA; if it gets inside:

      if (( layer_hint && --layer_hint->deliver_window < 0) ||
          (!layer_hint && --circ->deliver_window < 0)) {

it can cause a NULL pointer dereference in connection_edge_end(), since the check for (!conn) happens after that if statement.

I suspect that this can be triggered if you spam an OR to reduce its deliver_window to 0, and then send a RELAY_COMMAND_DATA cell with no stream_id.

Child Tickets

Change History (6)

comment:1 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.2.x-final
Priority: normalmajor
Status: newneeds_review

My first thought was to move the "if (!conn)" check up higher, but that's no good: it would mean that deliver_window stuff wouldn't get decremented for unrecognized connections.

I'm not completely sure that the the connection_edge_end() call there is actually necessary: we're about to tear down the circuit, after all; it should make the connection get ended one way or another. It looks like we introduced that connection_edge_end() back in 4a66865d, as a way to make sure that a good error got sent rather than a generic one.

But we can figure that out later. I think the right fix for now is to make the connection_edge_end() there conditional on whether conn is set. See branch "bug5541" on my public repo. It's against 0.2.2.

comment:2 Changed 8 years ago by asn

23:34 < lodger> just comment for lulz. are you seriously? bug5541? did you found way "spam an OR to reduce its `deliver_window` to 0" while calling circuit_consider_sending_sendme() for every relay_data? it's some quantum tor magic.

comment:3 Changed 8 years ago by nickm

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

Seems untriggerable then. Still let's do a patch in 0.2.3.x in case it becomes triggerable due to future code changes or something.

I've updated the branch as "bug5541_v2", to have a more correct commit message and changes file. It's still based on 0.2.2, but I'm going to suggest it only for 0.2.3.x and later.

comment:4 Changed 8 years ago by nickm

Priority: majortrivial
Resolution: fixed
Status: needs_reviewclosed

Still looks fine to me. Downgrading to "trivial", since it's untriggerable for now, but merging it since it makes the code more robust against future changes.

comment:5 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:6 Changed 7 years ago by nickm

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