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

 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.
   * When this is merged we should open a ticket to remove the redundant
 fields from channel_t.  I see a comment in channel.h saying that we could
 consolodate it ; more on that below.

 channel.h:
    * Looking at next_padding_time ... you need to document that 0 means
 "never".
    * You say that we set padding_timeout_low/high, but you never actually
 say what they do or mean or who checks them.  Also, "0==unset" isn't
 documented.
    * 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".

 channelpadding.h:
    * On many of these functions, more args could be const
    * The units of channelpadding_get_netflow_inactive_timeout_ms aren't
 documented in the function description; also, the 0 return value
 isn'tdocumented.
    * The documentation for channelpadding_get_netflow_inactive_timeout_ms
 doesn't seem right.  It says that it gets the value from the consensus,
 but really it generates a random value for the channel based on the value
 from the consensus, the negotiated value (if any), and some defaults.
    * the warnings in channelpadding_update_padding_for_channel should be
 rate-limited, or LOG_PROTOCOL_WARN, or both.
    * 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?
    * Trunnel idiom: You don't need to check the return values from
 chanelpadding_netgotiate_set*().  If any one of those fails, the encode()
 will fail later on.
    * In channelpadding_send_padding_cell_for_callback () -- that static
 cell_t makes me a little nervous.  memset() is much faster than our
 crypto; is this a premature optimization?
    * 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''').
       * To the checks in channelpadding_channelinfo_to_channel let's add
 "it is an OR_CONN".
    * '''NM, NOTE''' I'm going to come back to this "timerslot"
 abstraction; I wonder if there's a cleaner way to do this.
    * I worry about clock jumps here; is this a use case for our monotonic
 timer?
    * This whole timer thing is a pretty huge amount of engineering to
 escape libevent's limitations when it comes to thousands of timers.  I
 wonder if there isn't some better approach... ('''NM, NOTE''')
    * On the other hand, sending padding via timers and callbacks is a
 pretty clean approach.

 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?

 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.

 command.c:
    * What other ramifications are there from setting chan->is_client=0
 like this?

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

 Meta:
    * Is there a torspec patch for merging the proposal's new behavior into
 tor-spec.txt?

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