[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #7144 [Tor]: Implement Bridge Guards and other anti-enumeration defenses



#7144: Implement Bridge Guards and other anti-enumeration defenses
-------------------------------------------------+-------------------------
 Reporter:  karsten                              |          Owner:  isis
     Type:  project                              |         Status:
 Priority:  High                                 |  needs_revision
Component:  Tor                                  |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.8.x-final
 Keywords:  SponsorZ, tor-bridge,                |        Version:
  027-triaged-1-out, TorCoreTeam201509,          |     Resolution:
  028-triage, 028-triaged                        |  Actual Points:
Parent ID:                                       |         Points:  medium
  Sponsor:                                       |
-------------------------------------------------+-------------------------

Comment (by nickm):

 Replying to [comment:27 isis]:
 > Replying to [comment:21 nickm]:
 > > Okay, I came here to drink coffee and review code, and my doctor tells
 me I shouldn't drink so much coffee.  I'll look at the smaller ones first.
 > >
 >
 > Thanks, Nick! I owe you a beverage at the next meeting in Valencia. :)

 Thanks, Isis!  I do enjoy me some beverage!  Almost as much as I enjoy me
 code.

 In any case where I don't respond below, either your code looks fine, or
 no change is needed, or a change can wait till later.

  [...]
 > > 6daf9165951d Make logic for choosing create cell type be agnostic to
 circuit type.
 > >    * hmm. I know this isn't new, but the
 `!cpath->extend_info->onion_key` check looks poor to me, since it will
 fail once no-TAP relays are a reality.  Probably doesn't need to get fixed
 on this branch though.
 >
 > What if I were to move it below the `if (server_mode(options))` check?
 That way, (at least in the future) we'll never accidentally use
 CREATE_FAST cells between ORs, where one has no `onion_key`.  Separate
 ticket?

 Sounds okay.  Separate ticket, I think.


 Replying to [comment:28 isis]:
 > Replying to [comment:22 nickm]:
 > > Okay, now to review "e81acaf5f33e Implement Bridge Guards (prop188).".
 > >
 > > The big files are command.c and loose.c.  I'll review the others
 first.
 > >
 > > circuitmux.c:
 > >    * I think that the changes here should make use of a
 CIRCUIT_HAS_CPATH macro rather than doing `origin || loose`; this logic is
 likely to be important elsewhere.
 > >
 >
 > Did you mean this check?
 > {{{
 >        if ((circ->magic == OR_CIRCUIT_MAGIC) ||
 >            (circ->magic == LOOSE_OR_CIRCUIT_MAGIC))
 > }}}
 >
 > That check is for discovering if the circuit has a `p_circ_id` and maybe
 having a `p_chan`.  Should I make a `CIRCUIT_HAS_PCHAN` macro? Also, I
 agree that `CIRCUIT_HAS_CPATH` sounds useful, but I don't understand where
 in circuitmux.c it should be applied.

 CIRCUITH_HAS_PCHAN would be great.

 >
 > >    * p_chan_relay_cell is a little confusing. How do we know that they
 won't send *two* relay early cells, and what do we do if they do?  Also
 consider prop#249 ("Large create cells")
 >
 > My reading of the spec and testing caused me to believe that only one
 relay_early should/would be sent, and that the OR should simply drop
 additional cells while the channel is not yet fully constructed:
 > {{{
 > 5.5. Routing relay cells
 >
 >    When an OR receives a RELAY or RELAY_EARLY cell, it checks the cell's
 >    circID and determines whether it has a corresponding circuit along
 that
 >    connection.  If not, the OR drops the cell.
 >
 >    Otherwise, if the OR is not at the OP edge of the circuit (that is,
 >    either an 'exit node' or a non-edge node), it de/encrypts the payload
 >    with the stream cipher, as follows:
 >         'Forward' relay cell (same direction as CREATE):
 >             Use Kf as key; decrypt.
 >         'Back' relay cell (opposite direction from CREATE):
 >             Use Kb as key; encrypt.
 >    Note that in counter mode, decrypt and encrypt are the same
 operation.
 >
 >    The OR then decides whether it recognizes the relay cell, by
 >    inspecting the payload as described in section 6.1 below.  If the OR
 >    recognizes the cell, it processes the contents of the relay cell.
 >    Otherwise, it passes the decrypted relay cell along the circuit if
 >    the circuit continues.  If the OR at the end of the circuit
 >    encounters an unrecognized relay cell, an error has occurred: the OR
 >    sends a DESTROY cell to tear down the circuit.
 >
 >    When a relay cell arrives at an OP, the OP decrypts the payload
 >    with the stream cipher as follows:
 >          OP receives data cell:
 >             For I=N...1,
 >                 Decrypt with Kb_I.  If the payload is recognized (see
 >                 section 6..1), then stop and process the payload.
 > }}}
 >
 > Without taking prop!#249 into account, my logic was that we should store
 one relay_early and drop the rest of them:
 >
 > {{{
 >   if (PREDICT_UNLIKELY(!loose_circ->has_opened)) {
 >     /* Store the first relay_early cell for later, after our loose
 circuit is
 >      * fully constructed. */
 >     tor_assert(cell);
 >
 >     if (cell_direction == CELL_DIRECTION_OUT) {
 >       log_debug(LD_CIRC,
 >                 "Received %s command on loose circuit %d, but we haven't
 "
 >                 "finished constructing the extra hops in this circuit! "
 >                 "Saving for later.",
 >                 cell_command_to_string(cell->command),
 >                 LOOSE_TO_CIRCUIT(loose_circ)->global_circuitlist_idx);
 >
 >       if (!loose_circ->p_chan_relay_cell) {
 >         ++num_seen;
 >         loose_circ->p_chan_relay_cell = tor_malloc_zero(sizeof(cell_t));
 >         *loose_circ->p_chan_relay_cell = *cell;
 >         return 0;
 >       } else {
 >         /* OPs should not be sending additional relay cells until the
 first
 >          * has been answered, so if we already have one stored, then the
 OP is
 >          * misbehaving.  Mark this circuit for close. */
 >         log_warn(LD_CIRC,
 >                  "Received another relay cell on loose circuit %d, which
 "
 >                  "is not done constructing.  Closing this circuit.",
 >                  LOOSE_TO_CIRCUIT(loose_circ)->global_circuitlist_idx);
 >         return -END_CIRC_REASON_TORPROTOCOL;
 >       }
 >     } else {
 >       /* If the first relay cell we saw on this circuit was incoming,
 then
 >        * something really weird is happening.  Drop it the cell. */
 >       log_warn(LD_OR,
 >                "Received incoming relay cell with command %s on loose
 circuit "
 >                "%d, but we haven't finished construction the extra hops
 in "
 >                "this circuit!  This is odd.  Dropping the cell.  Path
 is:",
 >                 cell_command_to_string(cell->command),
 >                 LOOSE_TO_CIRCUIT(loose_circ)->global_circuitlist_idx);
 >       loose_circuit_log_path(LOG_WARN, LD_OR, loose_circ);
 >       return 0;
 > }
 > }}}
 >
 > Although, now that I look at it again, it looks wrong. The part which
 does `return -END_CIRC_REASON_TORPROTOCOL` should actually do `return 0`
 to drop extra outgoing relay_early from the OP side. Also, the `return 0`
 part should instead do something like:
 >
 > {{{
 > if (crypto_cipher_crypt_inplace(loose_circ->cpath->b_crypto, cell,
 >                                 CELL_PAYLOAD_SIZE) &&
 (cell->recognised)) {
 >   // XXX I would need to double check that loose_circ_process_relay_cell
 >   // would still do the right thing in this case.
 >   loose_circuit_process_relay_cell(loose_circ, NULL, cell,
 CELL_DIRECTION_IN, 1);
 > } else {
 >   // Avoid passing any relay_early cells from the loose hops backwards
 to the OP.
 >   return -END_CIRC_REASON_TORPROTOCOL;
 > }
 > }}}
 >
 > Aside from all this, this logic becomes ''totally incorrect'' when we
 have prop!#249 large/split create cells.  I'll make two patches, one to
 fix the above mistakes, and another to revise the code to handle post-
 prop!#249 create cells. Does that sound okay?
 >

 Yes, please!

 > >    * I think some of the tor_asserts() in the OR_TO_LOOSE_CIRCUIT_()
 functions need to check for LOOSE_CIRCUIT_MAGIC, not OR_CIRCUIT_MAGIC.
 >
 > Yikes! Good catch. O_o'

 I'm a little worried that the testing didn't catch this. :/

 > > relay.c:
 > >    * I wonder whether there is really no shared code with
 circuit_receive_relay_cell and loose_circuit_process_relay_cell.  I guess
 I'll find out when I review loose.c
 >
 > There's plenty. I wasn't sure what to do about that, since otherwise I
 would be refactoring large sections of the cell processing code.

 Yeah; I think we need to combine that if possible, either as part of
 merging this or right after.  Having duplicate code leads to maintenance
 troubles.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/7144#comment:29>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs