CIRC_BW event miscounts, should count all circ data
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.
Activity
changed milestone to %Tor: 0.3.4.x-final
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).
Trac:
Status: assigned to needs_review-
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.
Trac:
Parent: N/A to #25546 (moved)
Good stuff Mike. I want to ask and clarify couple of things here.
- Why not put this counting in
command_process_relay_cell()? I'm asking because that function, before callingcircuit_receive_relay_cell()can close the circuit because of invalidRELAY_EARLYcell 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.-
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'tget_cell_network_size(chan->wide_circ_ids)be used instead ofCELL_PAYLOAD_SIZE? -
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_bworn_written_circ_bwfor 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.
Trac:
Status: needs_review to needs_information- Why not put this counting in
-
Replying to dgoulet:
Good stuff Mike. I want to ask and clarify couple of things here.
- Why not put this counting in
command_process_relay_cell()? I'm asking because that function, before callingcircuit_receive_relay_cell()can close the circuit because of invalidRELAY_EARLYcell 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. }}}
- 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'tget_cell_network_size(chan->wide_circ_ids)be used instead ofCELL_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.
- 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_bworn_written_circ_bwfor 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)
Trac:
Status: needs_information to needs_review - Why not put this counting in
moved to tpo/core/tor#25400 (closed)
