electrum icon indicating copy to clipboard operation
electrum copied to clipboard

Stream isolation for commands

Open JeremyRand opened this issue 6 years ago • 3 comments

~~(Marked as WIP because it needs a rebase to fix some merge conflicts that accumulated while I was working on this PR. No need to concern yourself with this PR until I fix that.)~~

This PR is a fairly bare-bones implementation of stream isolation for network-utilizing commands. It is intended to be used in combination with https://github.com/spesmilo/electrum/pull/5470 .

Currently, new secondary interfaces are created for new stream ID's on-demand. This isn't great for latency but is the simplest way to do it. A follow-up PR will create a pool of secondary interfaces in advance, so that they can later be assigned to stream ID's instantly; the reason I'm not doing that in the initial PR is that creating multiple TCP connections as soon as Electrum is opened can leak some shared state via timing side channels, which is bad if the goal is to keep those TCP connections stream-isolated. Tor has some well-audited logic for creating circuits in advance without leaking shared state; I intend to copy their logic for Electrum's usage (but doing so is enough of a bother that I don't think we should block this PR over the performance issues).

This PR also only applies the new behavior to the getaddresshistory command. A follow-up PR will apply it to the other network-utilizing commands.

JeremyRand avatar Aug 20 '19 14:08 JeremyRand

Fixed merge conflict; feel free to review.

JeremyRand avatar Aug 22 '19 06:08 JeremyRand

Tor has some well-audited logic for creating circuits in advance without leaking shared state; I intend to copy their logic for Electrum's usage

Correction -- I just talked to the Tor devs and they inform me that Tor actually doesn't try to prevent this class of attacks right now. So, I'll move forward with the naive approach of having a pool of interfaces without worrying about this attack class. If it's good enough for Tor, it's probably good enough for us.

JeremyRand avatar Sep 20 '19 20:09 JeremyRand

This PR now creates a pool of clean Interfaces for use by stream-isolated commands, which eliminates the latency issue. I've also added support for the remaining commands where stream isolation is likely to be relevant.

AFAICT, the only two things left for this PR are:

  1. Wait for https://github.com/spesmilo/electrum/pull/5470 to be merged, since this PR is mostly useless with it.
  2. Right now, this PR activates based on whether a proxy is configured. It would be better to activate based on whether the configured proxy has stream isolation enabled. Since it's not yet clear in https://github.com/spesmilo/electrum/pull/5470 exactly what circumstances will enable stream isolation, I haven't yet done anything for that in this PR.

That said, since the above 2 things are pretty minor, definitely feel free to start reviewing this.

JeremyRand avatar Sep 22 '19 03:09 JeremyRand