Opened 3 years ago

Closed 3 years ago

#18570 closed defect (fixed)

Fix memory handling in incoming cell queues

Reported by: andrea Owned by:
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.1-alpha
Severity: Normal Keywords: 027-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor: None

Description

Per e-mail by David Goulet:

Hi little-t tor team!

(Please add anyone in CC if I forgot anyone, my brain is kind of fried to I'm
sure I'm forgetting someone...)

(Also, this is closed CC list because could be security related if my analysis
is wrong thus not good idea for tor-dev@ right now)

(An idea, a little-t@torproject.org that could email every dev doing work on
little-t tor code actively would be nice instead of having an epic CC chain :)

I stumble upon this yesterday while working with Rob on the tor code. At first,
it seems like a _bad_ security issue but turns out we are OK however there
seems to be some collateral dammage.

We start our code spelunking in connection_or.c (please correct me if I'm wrong
as I go because this ain't a trivial subsystem so I might interpret things
wrong).

Function connection_or_process_cells_from_inbuf() basically takes the cell from
the inbuf of the given OR connection and either handle them or queue them. Two
types of cells here either a var_cell_t or cell_t. Please follow the cell_t in
that function which is in the else statement of this if:

    if (connection_fetch_var_cell_from_buf(conn, &var_cell)) {
        ...
    } else {
        HERE
    }

The issue we noticed is that "cell_t cell" is on the stack and a reference to
it is passed to channel_tls_handle_cell(). This function can end up in
channel_queue_cell(). Remember, at that point the cell pointer passed to that
function is on the stack.

  /* Do we need to queue it, or can we just call the handler right away? */
  if (!(chan->cell_handler)) need_to_queue = 1;
  if (! TOR_SIMPLEQ_EMPTY(&chan->incoming_queue))
    need_to_queue = 1

In other words:

    * if we do NOT have a cell_handler function for this channel, queue it.
    * if the incoming_queue of the channel is NOT empty, queue it.

It all makes sense so far. The issue _could_ have arised if we queued the cell
because we would ended up in cell_queue_entry_new_fixed() which stores the cell
pointer which is BAD!!! because stack pointer thus potentially leaking stack
bytes to the network and breaking lots of things in tor functionnalities :).

However, after testing, I realized that we _never_ end up in that code path so
why? Turns out that we never actually queue cell in the incoming_queue of the
channel. Because 1) cell_handler of the channel seems always set and 2) we only
insert a cell in the queue if the queue is not empty so how can we insert a
cell in there if we only add it if it's not empty? (bootstrap issue :)

Since I can't find any information on the reason for this code or "high level"
view of it either in the commit message or mailing list (maybe my search is
bad), here are some questions that I'm sure someone in CC can answer me :).

1) What's the intended behavior here of "incoming_queue"? I'm pretty sure (not
100%) that we are _NOT_ using it so what are we losing in theory?

2) If that feature makes sense to keep, fixing it here would require a bit more
of testing and check, at the very least _NOT_ using the stack pointer when
queueing :)

3) Should we rip it off from the code if we think we don't need it?

4) In any of those above cases, having a document/proposal/<whatever> to
explain how cell handling works (10k feet view is enough) would be very needed
here fearing too little of us know about it.

(FYI, same goes for var_cell_t I think, channel_queue_var_cell())

Thanks!
David

There's no security hazard here because, as channel_t is currently used by the upper layer, the incoming cell queue never fills, but this is definitely a bug and the correct solution is for the channel layer itself to copy cells when queueing them, and be responsible for freeing them after the cell handler returns in that case.

Child Tickets

Change History (8)

comment:1 Changed 3 years ago by andrea

Status: newneeds_review

Proposed fix in my channel_queue_cell_bug branch.

comment:2 Changed 3 years ago by nickm

Keywords: must-fix-before-028-rc 027-backport added

comment:3 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

comment:4 Changed 3 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.7.x-final
Resolution: fixed
Status: closedreopened

comment:5 Changed 3 years ago by nickm

Keywords: must-fix-before-028-rc removed
Status: reopenedneeds_review

comment:6 Changed 3 years ago by nickm

I made a new bug18570_027 branch to backport the fixes here onto 0.2.7, added a changes file, and merged it to master. Now it should get considered for backport.

comment:7 Changed 3 years ago by andrea

Looks fine; merging to maint-0,2.7.

comment:8 Changed 3 years ago by andrea

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.