[tor-bugs] #16861 [Tor]: Pad Tor connections to collapse netflow records
Tor Bug Tracker & Wiki
blackhole at torproject.org
Sat Jan 23 01:19:31 UTC 2016
#16861: Pad Tor connections to collapse netflow records
-------------------------------------------------+-------------------------
Reporter: mikeperry | Owner:
Type: enhancement | mikeperry
Priority: High | Status:
Component: Tor | needs_review
Severity: Normal | Milestone: Tor:
Keywords: 028-triage, 028-triaged, | 0.2.8.x-final
pre028-patch, 201511-deferred, | Version:
TorCoreTeam201601, 201512-deferred | Resolution:
Parent ID: | Actual Points:
Sponsor: | Points: medium
-------------------------------------------------+-------------------------
Comment (by mikeperry):
Replying to [comment:39 nickm]:
> Reviewing the patch squashed as
"8944ad2cb526f1f895d35290a86c8c546e2d4f44 : Netflow record collapsing
defense".
>
> I'll start with low-level review, looking for high-level questions as I
go.
Ok, my fixes for most of this are now in mikeperry/netflow_padding-
v4_squashed in a fixup commit (721999a3e3e4bd51c2552bf03ea126b8ce7180e6).
Stuff I fixed I just removed from your comment. Inline below are any
questions or comments about remaining issues.
> channel.c:
> * The channel_timestamp_active call to tor_gettimeofday() is a little
worrisome; don't we have expensive gettimeofday() implementations in some
places? Can we get away with tor_gettimeofday_cached() ? Probably that's
premature optimization though.
I asked Yawning about this a while ago, and did some reading, and I think
the takeaway was that anywhere gettimeofday is slow, time() will also be
slow (which we were already doing). I think I'd also rather not lose the
resolution, unless tor_gettimeofday_cached() is only cached if
gettimeofday is slow...
> * You need to document what timestamp_active_hires actually means.
Also, it's confusing to have it mean something different from "a more
high-resolution version of timestamp_active".
I improved the comment. I am not sure what to call it other than active. I
could not think of another word that means "send or received actual data",
which is what this means. Open to suggestions. xmit is also already
taken..
> channelpadding.h:
> * On many of these functions, more args could be const
Fixed. Left one as a comment that can become const after we fix the
channel lookup stuff.
> * Wrt the second check in channelpadding_update_padding_for_channel
... is there a check to make sure we only receive the cell as an outbound
cell?
How do you mean? These are link-level cells. I thought directionality was
only for circuit-level?
> * WRT the whole channelpadding_channelinfo_* stuff:
> * When a connection is removed from the connection array, another
connection gets its old index. This happens at the end of
connection_remove(). In other words, a connection's index can change
after it is created! Where do we handle that? ('''probable bug''').
Yikes. Yes. In this case, we would see that the global identifiers don't
match, log at info level, and not send a padding packet. We should improve
this so that such that we don't expect a connection to disappear while the
timer is still pending. I have not done that yet. Also I still think for
safety we want to do an identifier-based lookup here, rather than throwing
a pointer around. But if you end up not haveing time for that O(1)
datastructure, we could possibly just to the cancel-on-free thing only and
send a pointer. That probably is the sane pattern for this type of thing.
> * To the checks in channelpadding_channelinfo_to_channel let's add
"it is an OR_CONN".
Isn't it enough that TO_OR_CONN that we use already has a tor_assert in
it? I think if some other mystery connection gets all the way down to that
type conversion, I want the tor process to stop dead. Something went
horribly wrong at that point.
> * I worry about clock jumps here; is this a use case for our
monotonic timer?
I remember looking into this before and I don't think clock jumps can do
too much damage to us. I certainly had an XXX there about this that I
removed after thinking about it. I can do so again. Would also be helpful
to know what happens if the clock jumps while libevent has timers pending.
> channeltls.c:
> * I don't think that CELL_PADDING and CELL_PADDING_NEGOTIATE are
transport-specific; I think they probably apply to most channel types,
yeah?
You mean move it to channel.c, or command.c? CELL_PADDING was already
specific to channeltls.c. In fact, I don't think there is any generalized
cell handling at all in channel.c. It just forwards it down to
channeltls.c or command.c. I'm actually not 100% sure why some cell types
are both in command.c *and* channeltls.c, or how that decision works in
the channel code.
> circuitbuild.c:
> * Wedging this test into circuit_send_next_onion_skin seems a bit
wrong to me. Maybe we can set it earlier in circuit construction.
Suggestions?
> command.c:
> * What other ramifications are there from setting chan->is_client=0
like this?
I found is_client to be an unreliable indicator of actual client status
(and possibly confused about CREATE_FAST being still used), despite the
comment for the field in channel_s. I guess I probably accidentally fixed
some other bugs? The only other important use was to decide not to send an
extend cell down the channel. Which seems fine to be more correct about.
> relay.c:
> * circuit_receive_relay_cell() is already a huge function. Can we
extract the usage-tracking block to a new function of its own?
Done.
> * This is_client check for the relay case there also looks a
little kludgey; I wonder if we don't need a protocol-level improvement.
Not sure what you want here?
> Meta:
> * Is there a torspec patch for merging the proposal's new behavior
into tor-spec.txt?
Yes, I will do that as we discussed in the meeting.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/16861#comment:40>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list