Re: [tor-bugs] #16861 [Tor]: Pad Tor connections to collapse netflow records

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs