[tor-bugs] #18570 [Tor]: Fix memory handling in incoming cell queues

[Tor Bug Tracker & Wiki]

#18570: Fix memory handling in incoming cell queues
------------------------+--------------------------------
     Reporter:  andrea  |      Owner:
         Type:  defect  |     Status:  new
     Priority:  Medium  |  Milestone:  Tor: 0.2.8.x-final
    Component:  Tor     |    Version:  Tor: 0.2.8.1-alpha
     Severity:  Normal  |   Keywords:
Actual Points:          |  Parent ID:
       Points:          |   Reviewer:
      Sponsor:  None    |
------------------------+--------------------------------
 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 at 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.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18570>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online