Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use SOCKSRandomAuth for stream isolation #5470

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JeremyRand
Copy link
Contributor

This PR adds an optional feature (enabled by default if a SOCKS proxy is in use) that makes each outgoing connection to an Electrum server go over an isolated Tor circuit. This improves Sybil-resistance by preventing a single Tor exit relay from having full control over Electrum's view of the network. It may also improve anonymity (by making certain types of traffic analysis more difficult), and it may also improve performance (by avoiding bottlenecked Tor relays).

Stream isolation can be toggled in the Qt GUI's proxy dialog. I didn't attempt to add a Kivy GUI toggle for this, as I don't have an easy way to test Kivy changes.

This PR is conceptually similar to bitcoin/bitcoin#5911 .

This PR is dependent on kyuupichan/aiorpcX#23 ; don't merge this PR until Electrum upgrades to a version of aiorpcX that includes that PR.

@SomberNight
Copy link
Member

Good stuff, idea looks simple but useful.

serialize_proxy and deserialize_proxy are a mess
I would rather not make them even more complex.
It's just a bad idea to try to fit all this data in a single string. Do people invoke electrum from terminal with a proxy string? That might be like the only usecase for it. The str <-> dict conversion should be killed by fire. And the delimiters are colons so e.g. note FIXME "raw IPv6 address fails here"
We should use a dict everywhere... @ecdsa ?

Should this setting really be exposed to the GUI? If we knew for a proxy whether it is a Tor proxy, could we just turn it on for Tor proxies and off otherwise?

@SomberNight SomberNight added the topic-network 🕸 related to logic in network.py (etc) label Jun 29, 2019
@SomberNight
Copy link
Member

The TorDetector class in the Qt network_dialog has logic to tell if a proxy is a Tor proxy, specifically this method:

def is_tor_port(net_addr: Tuple[str, int]) -> bool:
try:
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.settimeout(0.1)
s.connect(net_addr)
# Tor responds uniquely to HTTP-like requests
s.send(b"GET\n")
if b"Tor is not an HTTP Proxy" in s.recv(1024):
return True
except socket.error:
pass
return False

So this method could be moved out of Qt to e.g. util.

proxy could be a dict everywhere, with a field signalling whether it is a Tor proxy. If the field was missing, we would use above method is_tor_port to populate it.
Or maybe proxy could be some custom class so that this populate logic could be a method on it that automatically gets called on demand. (and then we would convert between proxy dict and proxy class; but strings should go)

@JeremyRand
Copy link
Contributor Author

Do people invoke electrum from terminal with a proxy string? That might be like the only usecase for it.

I suspect that this is a somewhat common use case (especially among Tor users who are trying to be certain to avoid proxy leaks), but I'm pretty sure there are better ways to let users specify proxy settings on the command line than the current strategy of combining all the proxy data into one string. Using a separate command-line flag for each component of the proxy data would probably be okay. (I don't know if that's compatible with the idea of using a dict for everything inside the config file... thoughts?)

I definitely agree that the current encoding method is ugly and error-prone, and I'd be in favor of replacing it.

Should this setting really be exposed to the GUI? If we knew for a proxy whether it is a Tor proxy, could we just turn it on for Tor proxies and off otherwise?

I suspect that the detection method you suggest will break on some edge cases. For example, I believe SubgraphOS ships with an intermediate SOCKS proxy that sits between the application and Tor's SOCKS proxy (the intermediate proxy does stuff like TLS policy enforcement); such an intermediate proxy will probably pass through the SOCKS authentication data to Tor but it might not mimic Tor's HTTP proxy detection. This kind of breakage is especially dangerous because it would cause stream isolation to silently be disabled.

There's probably a safer approach though. If the SOCKS proxy returns an error indicating that the username/password was incorrect, then that's an indication that we're not talking to Tor (either directly or via an intermediate proxy), and we can retry the connection without using a SOCKS username/password. Unfortunately, aiorpcX doesn't have a dedicated exception type for this; it uses SOCKSFailure, which is ambiguous. Should I add a commit to my aiorpcX PR that adds a more specific exception type so that we can detect this case reliably?

@JeremyRand
Copy link
Contributor Author

Fixed merge conflict.

@SomberNight
Copy link
Member

see #9250

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
privacy 🕵️‍♂️ kinda broad... network/blockchain/OS/... topic-network 🕸 related to logic in network.py (etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants