[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 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.

 > onion.c:
 >    * What does the `+  // prop#188` comment mean?  More detail please.
 >

 Whoops. That should not have been committed. That was me making a mark in
 the code at the very beginning of hacking on this, to tell myself that
 this is a spot I might need to pay attention to.  I ended up not needing
 to though. The comment has been removed in:

     *
 [https://gitweb.torproject.org/user/isis/tor.git/commit/?h=bug7144_r1&id=f076f639f685775368ba1e7b1d5d7c568a3807b9
 f076f639] Remove prop!#188 comment from onion.c.

 > or.h:
 >    * 0x13371515, huh? :)  Fair enough.

 It works as magic because it's highly improbable. :)

 >    * 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?

 >    * 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'

     *
 [https://gitweb.torproject.org/user/isis/tor.git/commit/?h=bug7144_r1&id=749d559ac3cea1a0ab5591b94385a238a9eb40dc
 749d559a] Change [CONST_]OR_TO_LOOSE_CIRCUIT to check correct magic.

 >    * Did you grep for other uses of OR_CIRCUIT_MAGIC to see if they
 needed to change as well?

 It's used in to see if a `rend_splice` is actually an `or_circuit_t` in
 `circuit_free()`.  And also in circuitmux.c (as mentioned in my last
 comment) to as part of a check to see if a circuit should have a
 `p_circ_id`.

 > 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.

 >    * What does the `+  // prop#188` comment mean?  More detail please.

 Removed in
     *
 [https://gitweb.torproject.org/user/isis/tor.git/commit/?h=bug7144_r1&id=46de8ce45a50d2b460cccf1e77f728247c4969af
 46de8ce4] Remove prop!#188 comment from relay.c.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/7144#comment:28>
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