[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