[tor-bugs] #7144 [Tor]: Implement Bridge Guards and other anti-enumeration defenses
Tor Bug Tracker & Wiki
blackhole at torproject.org
Thu Feb 11 16:07:06 UTC 2016
#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
More information about the tor-bugs
mailing list