Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#9063 closed defect (fixed)

#6252 didn't go far enough

Reported by: arma Owned by: arma
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay 023-backport
Cc: robgjansen, Flo Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In #6252 we noted that a client can send sendme cells preemptively to the exit relay, thus cheating on her circuit window (and getting more than her fair share of throughput). We fixed it at the exit relay (now it checks if the window is higher than it's allowed to be), but we didn't fix it at other relays in the circuit -- so she can still send a sendme after the cells have been packaged by the exit relay, but before they've made it all the way to the client, and do a more subtle version of cheating. This one's trickier to execute, but still worth fixing because it opens up all the issues that we thought we'd closed in #6252.

Reported by Rob Jansen and Florian Tschorsch.

Child Tickets

Change History (24)

comment:1 Changed 5 years ago by arma

Cc: robgjansen Flo added

comment:2 Changed 5 years ago by arma

Simple answer is for intermediate nodes, especially the entry guard, to check if they ever see a set of cells that's bigger than the circuit window should allow. If so, kill the circuit.

(We might want to check a slightly higher number, like 1100, to leave some allowance for future design changes, and to leave some tolerance in case there are bugs -- we see (infrequent) warnings in relay logs about the #6252 fix, perhaps by non-conformant relays but who knows.)

comment:3 Changed 5 years ago by arma

Damon suggested an "authenticated sendme" design -- the sendme payload has to prove receipt of all the cells in the window, else it's not accepted. That would be a good way to avoid the third version of this bug, whatever it will turn out to be.

But it also requires some more design work, since you'd need to put randomness into the flow somewhere so the client can't just preemptively come up with the answer. Or some other smart idea.

comment:4 Changed 5 years ago by andrea

Status: newneeds_review

See fixes for this in branches in my repo:

  • circuit_queue_cap-0.2.5 branched from master
  • circuit_queue_cap-0.2.4-2 branched from maint-0.2.4
  • circuit_queue_cap-0.2.3-2 branched from maint-0.2.3

comment:5 Changed 5 years ago by nickm

In 0.2.4:

src/or/relay.c:2491: warning: format ‘%lu’ expects type ‘long unsigned int’, but argument 8 has type ‘uint64_t’

The 0.2.3 one probably has a similar issue. Use the U64_FORMAT and U64_PRINTF_ARG macros to log 64-bit uints.

comment:6 in reply to:  5 Changed 5 years ago by andrea

Replying to nickm:

In 0.2.4:

src/or/relay.c:2491: warning: format ‘%lu’ expects type ‘long unsigned int’, but argument 8 has type ‘uint64_t’

The 0.2.3 one probably has a similar issue. Use the U64_FORMAT and U64_PRINTF_ARG macros to log 64-bit uints.

D'oh!

The 0.2.3 one doesn't have a similar issue because 0.2.3 doesn't have channel IDs to log. The 0.2.5 one will, though.

comment:7 Changed 5 years ago by andrea

Okay, format arg thing fixed. Any further objections, or shall I go ahead and prepare squashed versions for merging?

comment:8 Changed 5 years ago by andrea

The squashed branches are ready to go:

  • circuit_queue_cap-0.2.5-squashed branched from master
  • circuit_queue_cap-0.2.4-squashed branched from maint-0.2.4
  • circuit_queue_cap-0.2.3-squashed branched from maint-0.2.3

comment:9 in reply to:  2 Changed 5 years ago by arma

Replying to arma:

(We might want to check a slightly higher number, like 1100, to leave some allowance for future design changes, and to leave some tolerance in case there are bugs -- we see (infrequent) warnings in relay logs about the #6252 fix, perhaps by non-conformant relays but who knows.)

Oops -- we forgot about the leaky pipe topology. We should probably raise this number to 2000, to let us do things like 'adding padding cells from intermediate hops'. And we'll want a spec change, since we're disallowing behaviors that our spec used to allow.

comment:10 Changed 5 years ago by nickm

Owner: set to arma
Status: needs_reviewassigned

Patched and merged into 0.2.3 and later.

leaving open and assigning to arma, since somebody needs to write the spec patch. :)

comment:11 Changed 5 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.5.x-final

comment:12 Changed 5 years ago by andrea

Status: assignednew

Fix reverted due to possible anonymity break; see #9072

comment:13 Changed 5 years ago by nickm

(There's a bunch of discussion on that ticket about what the right fix could be)

comment:14 Changed 5 years ago by nickm

Keywords: tor-relay 023-backport added
Milestone: Tor: 0.2.5.x-finalTor: 0.2.4.x-final
Status: newneeds_review

Branch "bug9063_redux_023" in my public repository has a sketch implementation of "algorithm 1" from #9072. It's not ideal, but it could work. Please review?

(I'm not suggesting this as a permanent fix, but as a "good enough for now" fix for 0.2.3 and 0.2.4.)

comment:15 Changed 5 years ago by nickm

I've added more to that branch, to make it more production ready. It's still "good enough for now", and at least as good as we thought the previous #9063 patch was, I think.

comment:16 Changed 5 years ago by nickm

(FWIW, the merge into 0.2.4 isn't completely clean, but resolving the conflict appears to be trivial.)

comment:17 Changed 5 years ago by andrea

The bug9063_redux_023 branch looks okay; recommend testing it by running it as a relay for a while with an unnaturally low MaxMemInCellQueues setting to force some OOMs to occur.

comment:18 Changed 4 years ago by nickm

The branch is now bug9063_redux_023_squashed. No changes from the above, except to add a changes file.

comment:19 Changed 4 years ago by nickm

Merging into 0.2.3 and later. I'll hold this open till we can open a new ticket for "solve all the remaining issues in our #9063 / #9072 fixes"

comment:20 Changed 4 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.3.x-final
Resolution: fixed
Status: needs_reviewclosed

comment:21 Changed 4 years ago by nickm

I opened #9093 as a place to figure out the remaining issues.

comment:22 Changed 4 years ago by arma

Am I reading this right, that Tor 0.2.5.1-alpha is going to include commit 459aada4d, and that the commit will cause intermediate relays to kill any circuit that has more than 1100 cells queued?

comment:23 Changed 4 years ago by nickm

Have a look at relay.c : that code is, AFAICT, disabled with some "#if 0" directives.

comment:24 Changed 4 years ago by arma

Ok.

Note: See TracTickets for help on using tickets.