Opened 9 months ago

Closed 8 months ago

#27678 closed enhancement (implemented)

Emit CIRC_BW event early for dropped cells

Reported by: mikeperry Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


While reviewing the dropmark paper ( and asking the authors to test it against vanguards's dropped cell detection, I realized that since we only normally emit CIRC_BW events once per second, the attack may still have enough time succeed against vanguards.

The fix is to emit the CIRC_BW event as soon as we get a relay cell that does not cause us to update our delivered or overhead byte counts. It's pretty simply to do this. Patch incoming.

Child Tickets

Change History (4)

comment:1 Changed 9 months ago by mikeperry

Status: newneeds_review

Ok, this is a very simple change:

Even though it is simple, tests for it are unfortunately pretty complicated to do, because we don't currently seem to test this codepath. Doing so means creating a fake channel, valid circuitmuxes, valid cpaths with working circuit crypto, and probably some other things. I filed #27679 for that. I can do that one separately, but I might need someone to help me, and it will probably take a while.

Upside to lack of testing: The sooner we decide on a version of this we like, the sooner I can ask the DropMark authors to test their attack against this w/ a vanguards attached (which will exercise the code).

Last edited 9 months ago by mikeperry (previous) (diff)

comment:2 Changed 8 months ago by mikeperry

Ok I addressed asn's questions and requests for clarifying code comments in that PR. I pushed fixups to mikeperry-github/ticket27678 and mentioned the fixups in the PR. is the new PR with the squashed branch mikeperry-github/ticket27678-squashed (0 delta to mikeperry-github/ticket27678).

comment:3 Changed 8 months ago by asn

Status: needs_reviewmerge_ready

Thanks for adding those comments. Things are more clear now. Marking this as merge_ready.

comment:4 Changed 8 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Seems reasonable to me too. Merging!

Note: See TracTickets for help on using tickets.