Re: [tor-bugs] #25400 [Core Tor/Tor]: CIRC_BW event miscounts, should count all circ data

[Tor Bug Tracker & Wiki]

#25400: CIRC_BW event miscounts, should count all circ data
-+-
 Reporter:  mikeperry|  Owner:
 |  mikeperry
 Type:  defect   | Status:  closed
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.4.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:  fixed
 Keywords:  tor-stats, review-group-34, 034  |  Actual Points:
  -roadmap-subtask, 034-triage-20180328, |
  034-included-20180328  |
Parent ID:  #25546   | Points:
 Reviewer:  dgoulet  |Sponsor:
-+-
Changes (by nickm):

 * status:  merge_ready => closed
 * resolution:   => fixed


Comment:

 lgtm; merged to master!

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25400 [Core Tor/Tor]: CIRC_BW event miscounts, should count all circ data

[Tor Bug Tracker & Wiki]

#25400: CIRC_BW event miscounts, should count all circ data
-+-
 Reporter:  mikeperry|  Owner:
 |  mikeperry
 Type:  defect   | Status:
 |  merge_ready
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.4.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-stats, review-group-34, 034  |  Actual Points:
  -roadmap-subtask, 034-triage-20180328, |
  034-included-20180328  |
Parent ID:  #25546   | Points:
 Reviewer:  dgoulet  |Sponsor:
-+-

Comment (by mikeperry):

 Ok I rebased on to latest origin/master, and squashed and changed that
 second fixup commit message.

 Branch is mikeperry/bug25400_squashed.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25400 [Core Tor/Tor]: CIRC_BW event miscounts, should count all circ data

[Tor Bug Tracker & Wiki]

#25400: CIRC_BW event miscounts, should count all circ data
-+-
 Reporter:  mikeperry|  Owner:
 |  mikeperry
 Type:  defect   | Status:
 |  merge_ready
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.4.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-stats, review-group-34, 034  |  Actual Points:
  -roadmap-subtask, 034-triage-20180328, |
  034-included-20180328  |
Parent ID:  #25546   | Points:
 Reviewer:  dgoulet  |Sponsor:
-+-
Changes (by dgoulet):

 * status:  needs_review => merge_ready


Comment:

 Good stuff!

 Your top fixup commit that uses `tor_add_u32_nowrap()` won't squash
 cleanly since that function is added in its own commit so you might want
 to either do a new commit or base it on that helper function commit.

 Happy with the fixups! lgtm;

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25400 [Core Tor/Tor]: CIRC_BW event miscounts, should count all circ data

[Tor Bug Tracker & Wiki]

#25400: CIRC_BW event miscounts, should count all circ data
-+-
 Reporter:  mikeperry|  Owner:
 |  mikeperry
 Type:  defect   | Status:
 |  needs_review
 Priority:  Medium   |  Milestone:  Tor:
 |  0.3.4.x-final
Component:  Core Tor/Tor |Version:
 Severity:  Normal   | Resolution:
 Keywords:  tor-stats, review-group-34, 034  |  Actual Points:
  -roadmap-subtask, 034-triage-20180328, |
  034-included-20180328  |
Parent ID:  #25546   | Points:
 Reviewer:  dgoulet  |Sponsor:
-+-
Changes (by mikeperry):

 * status:  needs_information => needs_review


Comment:

 Replying to [comment:9 dgoulet]:
 > Good stuff Mike. I want to ask and clarify couple of things here.
 >
 > 1. Why not put this counting in `command_process_relay_cell()`? I'm
 asking because that function, before calling
 `circuit_receive_relay_cell()` can close the circuit because of invalid
 `RELAY_EARLY` cell or too many of them or if cells are received but the
 circuit state doesn't allow it. Don't you want to catch those also in your
 calculation? Looking at this comment, it seems you might?

 Actually, yes, let's move this block up towards the top of
 circuit_receive_relay_cell(). I had put it below since those violations
 closed the circuit, but you're right, let's count them too. Thanks!

 
https://oniongit.eu/mikeperry/tor/commit/4371b54854545a3962f9455fe318711dc7b799f9

 > {{{
 > +  /* Count all circuit bytes here for control port accuracy. We want
 > +   * to count even invalid/dropped relay cells, hence counting
 > +   * before the recognized check and the connection_edge_process_relay
 > +   * cell checks.
 > }}}
 >
 > 2. The `CELL_PAYLOAD_SIZE`, as you stated in your previous comment,
 doesn't take into account the header size but that header _can_ bring the
 cell size up to 514 bytes (`CELL_MAX_NETWORK_SIZE`). I'm not sure I
 understand the reasoning behind not counting the header. You mention that
 the controller application should subtract the header size but it really
 shouldn't if tor is not counting them in the first place? Actually, the
 cell size can vary depending on the use of "wide circ ids" so shouldn't
 `get_cell_network_size(chan->wide_circ_ids)` be used instead of
 `CELL_PAYLOAD_SIZE`?

 I'm using the definition of "total bandwidth carried by the circuit". This
 means that the payload size is the natural value here. For purposes of the
 payload carried, it does not matter if the *circuit* headers are different
 sizes. My earlier comment was about *relay* headers, which are part of the
 (encrypted) circuit payload, and I think should be counted in this stat.

 > 3. The counting bytes code for read/written is really the same so I
 would be much happier with a function that does that and could take a
 pointer to `n_read_circ_bw` or `n_written_circ_bw` for the calculation (or
 not a pointer, just return the new value). Seems to me will be easier to
 unit test but also better code semantic as well.

 Good point. I made this u32 overflow add into a util.c helper, since it is
 a common pattern, independent of this circuit accounting:

 
https://oniongit.eu/mikeperry/tor/commit/b9f28f475ecc546503bd85d1d4a0564f09b638a3
 (impl + tests)
 
https://oniongit.eu/mikeperry/tor/commit/4e86178122c7aa20af91838c59ee5dd6fe6bb7d5
 (use)

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25400 [Core Tor/Tor]: CIRC_BW event miscounts, should count all circ data

[Tor Bug Tracker & Wiki]

#25400: CIRC_BW event miscounts, should count all circ data
+--
 Reporter:  mikeperry   |  Owner:  mikeperry
 Type:  defect  | Status:
|  needs_information
 Priority:  Medium  |  Milestone:  Tor:
|  0.3.4.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-stats, review-group-34  |  Actual Points:
Parent ID:  #25546  | Points:
 Reviewer:  dgoulet |Sponsor:
+--
Changes (by dgoulet):

 * status:  needs_review => needs_information


Comment:

 Good stuff Mike. I want to ask and clarify couple of things here.

 1. Why not put this counting in `command_process_relay_cell()`? I'm asking
 because that function, before calling `circuit_receive_relay_cell()` can
 close the circuit because of invalid `RELAY_EARLY` cell or too many of
 them or if cells are received but the circuit state doesn't allow it.
 Don't you want to catch those also in your calculation? Looking at this
 comment, it seems you might?

 {{{
 +  /* Count all circuit bytes here for control port accuracy. We want
 +   * to count even invalid/dropped relay cells, hence counting
 +   * before the recognized check and the connection_edge_process_relay
 +   * cell checks.
 }}}

 2. The `CELL_PAYLOAD_SIZE`, as you stated in your previous comment,
 doesn't take into account the header size but that header _can_ bring the
 cell size up to 514 bytes (`CELL_MAX_NETWORK_SIZE`). I'm not sure I
 understand the reasoning behind not counting the header. You mention that
 the controller application should subtract the header size but it really
 shouldn't if tor is not counting them in the first place? Actually, the
 cell size can vary depending on the use of "wide circ ids" so shouldn't
 `get_cell_network_size(chan->wide_circ_ids)` be used instead of
 `CELL_PAYLOAD_SIZE`?

 3. The counting bytes code for read/written is really the same so I would
 be much happier with a function that does that and could take a pointer to
 `n_read_circ_bw` or `n_written_circ_bw` for the calculation (or not a
 pointer, just return the new value). Seems to me will be easier to unit
 test but also better code semantic as well.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25400 [Core Tor/Tor]: CIRC_BW event miscounts, should count all circ data

[Tor Bug Tracker & Wiki]

#25400: CIRC_BW event miscounts, should count all circ data
+--
 Reporter:  mikeperry   |  Owner:  mikeperry
 Type:  defect  | Status:  needs_review
 Priority:  Medium  |  Milestone:  Tor:
|  0.3.4.x-final
Component:  Core Tor/Tor|Version:
 Severity:  Normal  | Resolution:
 Keywords:  tor-stats, review-group-34  |  Actual Points:
Parent ID:  #25546  | Points:
 Reviewer:  dgoulet |Sponsor:
+--
Changes (by asn):

 * parent:   => #25546


--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25400 [Core Tor/Tor]: CIRC_BW event miscounts, should count all circ data

[Tor Bug Tracker & Wiki]

#25400: CIRC_BW event miscounts, should count all circ data
--+
 Reporter:  mikeperry |  Owner:  mikeperry
 Type:  defect| Status:  needs_review
 Priority:  Medium|  Milestone:  Tor: 0.3.4.x-final
Component:  Core Tor/Tor  |Version:
 Severity:  Normal| Resolution:
 Keywords:  tor-stats |  Actual Points:
Parent ID:| Points:
 Reviewer:|Sponsor:
--+

Comment (by atagar):

 > atagar may also have more information on CIRC_BW usage by Nyx and other
 stem apps

 Stem and Nyx don't use CIRC_BW. Stem has parsing code for the events, but
 that's all.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25400 [Core Tor/Tor]: CIRC_BW event miscounts, should count all circ data

[Tor Bug Tracker & Wiki]

#25400: CIRC_BW event miscounts, should count all circ data
--+
 Reporter:  mikeperry |  Owner:  mikeperry
 Type:  defect| Status:  needs_review
 Priority:  Medium|  Milestone:  Tor: 0.3.4.x-final
Component:  Core Tor/Tor  |Version:
 Severity:  Normal| Resolution:
 Keywords:  tor-stats |  Actual Points:
Parent ID:| Points:
 Reviewer:|Sponsor:
--+

Comment (by mikeperry):

 Another additional wrinkle to discuss: I chose to use CELL_PAYLOAD_SIZE as
 the number of bytes to count per cell. That value is the total bytes in a
 cell, less cell header size. I also briefly debated subtracting the common
 relay header size, but since the true relay header size varies by relay
 cell type, I decided not to subtract the common relay header from the
 totals. The Tor controller can decide to subtract the common relay header
 out itself (as it represents a constant rate of overhead). The vanguards
 controller will do this subtraction.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25400 [Core Tor/Tor]: CIRC_BW event miscounts, should count all circ data

[Tor Bug Tracker & Wiki]

#25400: CIRC_BW event miscounts, should count all circ data
--+
 Reporter:  mikeperry |  Owner:  mikeperry
 Type:  defect| Status:  needs_review
 Priority:  Medium|  Milestone:  Tor: 0.3.4.x-final
Component:  Core Tor/Tor  |Version:
 Severity:  Normal| Resolution:
 Keywords:  tor-stats |  Actual Points:
Parent ID:| Points:
 Reviewer:|Sponsor:
--+
Changes (by mikeperry):

 * cc: atagar (added)


Comment:

 To Teor's question, Karsten implemented the event, possibly for metrics
 usage. I don't know if metrics relies upon the existing behavior in a
 critical way. atagar may also have more information on CIRC_BW usage by
 Nyx and other stem apps.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25400 [Core Tor/Tor]: CIRC_BW event miscounts, should count all circ data

[Tor Bug Tracker & Wiki]

#25400: CIRC_BW event miscounts, should count all circ data
--+
 Reporter:  mikeperry |  Owner:  mikeperry
 Type:  defect| Status:  needs_review
 Priority:  Medium|  Milestone:  Tor: 0.3.4.x-final
Component:  Core Tor/Tor  |Version:
 Severity:  Normal| Resolution:
 Keywords:  tor-stats |  Actual Points:
Parent ID:| Points:
 Reviewer:|Sponsor:
--+
Changes (by mikeperry):

 * status:  assigned => needs_review


Comment:

 Patch in mikeperry/bug25400. Spec patch in my torspec remote, also branch
 bug25400.

 Right now this branch only counts relay cells, which is similar to the
 behavior before. I decided against counting CREATED cells, and VAR cells,
 because those are a bit weird since there is not a circuit ID on the
 outgoing cell, only upon the response. Let me know what you think.

 For what it's worth, counting all cells (including
 padding/dropped/rejected/partially full ones) in circuit bandwidth totals
 allows a Tor controller to check for side channel attacks by doing
 accounting on STREAM_BW totals on a circuit and comparing that to the
 CIRC_BW totals. Large differences indicate side channel abuse (depending
 on the application protocol).

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #25400 [Core Tor/Tor]: CIRC_BW event miscounts, should count all circ data

[Tor Bug Tracker & Wiki]

#25400: CIRC_BW event miscounts, should count all circ data
--+
 Reporter:  mikeperry |  Owner:  mikeperry
 Type:  defect| Status:  assigned
 Priority:  Medium|  Milestone:  Tor: 0.3.4.x-final
Component:  Core Tor/Tor  |Version:
 Severity:  Normal| Resolution:
 Keywords:  tor-stats |  Actual Points:
Parent ID:| Points:
 Reviewer:|Sponsor:
--+
Changes (by teor):

 * keywords:   => tor-stats
 * milestone:   => Tor: 0.3.4.x-final


Comment:

 How is this event used?
 I'd like to know how important it is, so I can prioritise this ticket.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

[tor-bugs] #25400 [Core Tor/Tor]: CIRC_BW event miscounts, should count all circ data

[Tor Bug Tracker & Wiki]

#25400: CIRC_BW event miscounts, should count all circ data
--+---
 Reporter:  mikeperry |  Owner:  mikeperry
 Type:  defect| Status:  assigned
 Priority:  Medium|  Milestone:
Component:  Core Tor/Tor  |Version:
 Severity:  Normal|   Keywords:
Actual Points:|  Parent ID:
   Points:|   Reviewer:
  Sponsor:|
--+---
 The CIRC_BW event as implemented seems to be totaling only explicit
 application data on the circuit. To have an accurate representation of
 circuit usage, it should total all bytes sent on the circuit, including
 padding, dropped cells, and partially full cells.

 Furthermore, it currently counts bytes differently depending on if you
 have the STREAM event enabled (see control_event_stream_bandwidth()). This
 is definitely a bug.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs