net: Prevent accidental circuit sharing when using Tor stream isolation #32176

pull laanwj wants to merge 2 commits into bitcoin:master from laanwj:2025-03-tor-stream-isolation changing 8 files +55 −19
  1. laanwj commented at 4:23 pm on March 31, 2025: member

    Add a class TorsStreamIsolationCredentialsGenerator that generates unique credentials based on a randomly generated session prefix and an atomic counter. Use this in ConnectThroughProxy instead of a simple atomic int counter.

    This makes sure that different launches of the application won’t share the same credentials, and thus circuits, even in edge cases.

    Example with -debug=proxy:

    02025-03-31T16:30:27Z [proxy] SOCKS5 sending proxy authentication 0afb2da441f5c105-0:0afb2da441f5c105-0
12025-03-31T16:30:31Z [proxy] SOCKS5 sending proxy authentication 0afb2da441f5c105-1:0afb2da441f5c105-1

    Thanks to hodlinator in #32166 (review) for the idea.

  2. laanwj added the label P2P on Mar 31, 2025
  3. DrahtBot commented at 4:23 pm on March 31, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32176.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK hodlinator, jonatack, danielabrozzoni
    Stale ACK 1440000bytes, eval-exec

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31650 (refactor: Avoid copies by using const references or by move-construction by maflcko)
    • #28584 (Fuzz: extend CConnman tests by vasild)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  4. laanwj force-pushed on Mar 31, 2025
  7. in src/init.cpp:1581 in 3ab2cb2c29 outdated 
    1577@@ -1578,14 +1578,14 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1578     if (proxyArg != "" && proxyArg != "0") {
1579         Proxy addrProxy;
1580         if (IsUnixSocketPath(proxyArg)) {
1581-            addrProxy = Proxy(proxyArg, proxyRandomize);
1582+            addrProxy = Proxy(proxyArg, /*tor_stream_isolation=*/proxyRandomize);
    eval-exec commented at 6:19 am on April 1, 2025:
    How about change proxyRandomize to tor_stream_isolation too.
    laanwj commented at 6:29 am on April 1, 2025:

    i’m not sure. i don’t want to go as far as to change interfaces: configuration option name -proxyrandomize or the RPC field proxy_randomize_credential of getnetworkinfo. It would be clearer but is not worth the disruption. And this variable name seems a direct reflection of the option name.

    E.g. this line just looks consistent as-is:

    0bool proxyRandomize = args.GetBoolArg("-proxyrandomize", DEFAULT_PROXYRANDOMIZE);
  8. in src/netbase.cpp:747 in 3ab2cb2c29 outdated 
    742+        auth.username = auth.password = strprintf("%s%i", m_prefix, m_counter);
743+        ++m_counter;
744+        return auth;
745+    }
746+
747+    /* Size of session prefix in bytes. */
    laanwj commented at 6:37 am on April 1, 2025:
    Self-nit: should be /** comment for doxygen.
  9. in src/netbase.cpp:730 in 3ab2cb2c29 outdated 
    724@@ -725,6 +725,41 @@ bool IsProxy(const CNetAddr &addr) {
725     return false;
726 }
727 
728+/**
729+ * Generate unique credentials for Tor stream isolation. Tor will create
730+ * seperate circuits for socks5 proxy connections with different credentials, which
    laanwj commented at 6:37 am on April 1, 2025:
    Self-nit: SOCKS5
  10. ryanofsky requested review from hodlinator on Apr 1, 2025
  11. net: Rename `_randomize_credentials` Proxy parameter to `tor_stream_isolation` 
    Rename the `_randomize_credentials` parameter to Proxy's constructor to
`tor_stream_isolation` to make it more clear, and more specific what its
purpose is.

Also change all call sites to use a named parameter.
    c47f81e8ac
  12. DrahtBot added the label Needs rebase on Apr 1, 2025
  13. laanwj force-pushed on Apr 1, 2025
  14. in src/netbase.h:64 in 297ddaa430 outdated 
    62-    explicit Proxy(const CService& _proxy, bool _randomize_credentials = false) : proxy(_proxy), m_is_unix_socket(false), m_randomize_credentials(_randomize_credentials) {}
63-    explicit Proxy(const std::string path, bool _randomize_credentials = false) : m_unix_socket_path(path), m_is_unix_socket(true), m_randomize_credentials(_randomize_credentials) {}
64+    Proxy() : m_is_unix_socket(false), m_tor_stream_isolation(false) {}
65+    explicit Proxy(const CService& _proxy, bool tor_stream_isolation = false) : proxy(_proxy), m_is_unix_socket(false), m_tor_stream_isolation(tor_stream_isolation) {}
66+    explicit Proxy(const std::string path, bool tor_stream_isolation = false) : m_unix_socket_path(path), m_is_unix_socket(true), m_tor_stream_isolation(tor_stream_isolation) {}
67
    hodlinator commented at 8:32 pm on April 1, 2025:

    (Inline comment to avoid spamming main thread).

    nit: Missing named parameter comments in i2p test/fuzz files:

    https://github.com/bitcoin/bitcoin/blob/297ddaa430e99ad62e6bbdcc475ede7e04da547f/src/test/i2p_tests.cpp#L55

    https://github.com/bitcoin/bitcoin/blob/297ddaa430e99ad62e6bbdcc475ede7e04da547f/src/test/i2p_tests.cpp#L117

    https://github.com/bitcoin/bitcoin/blob/297ddaa430e99ad62e6bbdcc475ede7e04da547f/src/test/i2p_tests.cpp#L160

    https://github.com/bitcoin/bitcoin/blob/297ddaa430e99ad62e6bbdcc475ede7e04da547f/src/test/fuzz/i2p.cpp#L37

    (Was unable to see them while grepping, added an extra CService argument in the Proxy ctor before bool tor_stream_isolation to see what the compiler would find).

  15. DrahtBot removed the label Needs rebase on Apr 1, 2025
  16. in src/netbase.cpp:733 in 297ddaa430 outdated 
    724@@ -725,6 +725,41 @@ bool IsProxy(const CNetAddr &addr) {
725     return false;
726 }
727 
728+/**
729+ * Generate unique credentials for Tor stream isolation. Tor will create
730+ * seperate circuits for SOCKS5 proxy connections with different credentials, which
731+ * makes it harder to correlate the connections.
732+ */
733+class TorStreamIsolationCredentialsGenerator {
  17. in src/netbase.cpp:730 in 297ddaa430 outdated 
    724@@ -725,6 +725,41 @@ bool IsProxy(const CNetAddr &addr) {
725     return false;
726 }
727 
728+/**
729+ * Generate unique credentials for Tor stream isolation. Tor will create
730+ * seperate circuits for SOCKS5 proxy connections with different credentials, which
    hodlinator commented at 8:49 pm on April 1, 2025:

    nit:

    0 * separate circuits for SOCKS5 proxy connections with different credentials, which
  18. in src/netbase.cpp:763 in 297ddaa430 outdated 
    756+    static std::string GenerateUniquePrefix() {
757+        std::array<uint8_t, PREFIX_BYTE_LENGTH> prefix_bytes;
758+        GetRandBytes(prefix_bytes);
759+        return HexStr(prefix_bytes) + "-";
760+    }
761+};
    hodlinator commented at 8:55 pm on April 1, 2025:

    nit: Boiling the class down to a shorter function simplifies the code IMO, but don’t feel strongly about it:

     0/**
 1 * Generate unique credentials for Tor stream isolation. Tor will create
 2 * separate circuits for SOCKS5 proxy connections with different credentials, which
 3 * makes it harder to correlate the connections.
 4 */
 5static ProxyCredentials GenerateTorStreamIsolationCredentials()
 6{
 7    /** Generate a random prefix for each of the credentials returned by this generator.
 8     * This makes sure that different launches of the application will not share the same circuits.
 9     */
10    static const std::string prefix{[] {
11        /** Size of session prefix in bytes. */
12        constexpr size_t PREFIX_BYTE_LENGTH = 8;
13
14        std::array<uint8_t, PREFIX_BYTE_LENGTH> prefix_bytes;
15        GetRandBytes(prefix_bytes);
16        return HexStr(prefix_bytes) + "-";
17    }()};
18
19    static std::atomic<uint64_t> counter;
20
21    ProxyCredentials auth;
22    auth.username = auth.password = strprintf("%s%i", prefix, counter);
23    ++counter;
24    return auth;
25}
    laanwj commented at 9:54 am on April 3, 2025:
    tbf i prefer a class, i find that much more readable than a magic function. Even if slightly longer it makes things like encapsulation explicit.
    hodlinator commented at 9:01 pm on April 3, 2025:
    I admit that the lambda does require a double-take. Maybe I’m having a touch of unexpected C nostalgia.
  19. in src/netbase.h:68 in 297ddaa430 outdated 
    67 
68     CService proxy;
69     std::string m_unix_socket_path;
70     bool m_is_unix_socket;
71-    bool m_randomize_credentials;
72+    bool m_tor_stream_isolation;
    hodlinator commented at 9:15 pm on April 1, 2025:

    Q regarding rename to [m_]tor_stream_isolation:

    There does seem to be proxy support for CJDNS, but maybe that is TOR+CJDNS, so the name is still correct? https://github.com/bitcoin/bitcoin/blob/297ddaa430e99ad62e6bbdcc475ede7e04da547f/src/init.cpp#L1574-L1596

    (There seems to be some limited support for SOCKS proxies in I2P: https://geti2p.net/en/docs/api/socks, but I get the impression it may not be supported by bitcoind).

    laanwj commented at 6:53 am on April 2, 2025:

    CJDNS doesn’t use SOCKS (it creates a virtual network device), neither does I2P (it has its own, bidirectional proxy protocol called SAM, which is preferable to the very limited SOCKS support).

    In any case, randomized credentials leading to circuit isolation is a Tor specific feature. Other uses of SOCKS don’t want this (it’d probably either break auth or do nothing), so i still think the new name is better.

    laanwj commented at 7:07 am on April 2, 2025:
    There’s also a deeper conceptual issue that -proxyrandomize is a global setting (used for all proxies) instead of a per-proxy setting. In practice no one uses SOCKS for anything else than Tor, let alone multiple different SOCKS proxies at the same time, so it’s not something that particularly needs to be addressed.
    hodlinator commented at 9:03 am on April 2, 2025:

    CJDNS doesn’t use SOCKS In practice no one uses SOCKS for anything else than Tor

    https://github.com/bitcoin/bitcoin/blob/297ddaa430e99ad62e6bbdcc475ede7e04da547f/src/init.cpp#L1596

    Seems like the intent behind de01e312b333b65b09c8dc72f0cea6295ab8e43f was to support it. You mean the code allows it, but likely nobody really uses them in combination?

    laanwj commented at 9:06 am on April 2, 2025:

    No you’re right, looks like that does add functionality to set a SOCKS proxy for CJDNS.

    Still, Tor stream isolation is a Tor specific SOCKS extension and is not relevant to that.

    ryanofsky commented at 7:23 pm on April 14, 2025:

    In commit “net: Rename _randomize_credentials Proxy parameter to tor_stream_isolation” (c47f81e8ac110b1d7f78bce0232e8015366d13e7)

    Note: There seems to be a reference to m_randomize_credentials still in a comment https://github.com/bitcoin/bitcoin/blob/51166559808c3528f148b5c0c38cb4481e536dd8/src/torcontrol.cpp#L407

    laanwj commented at 4:00 pm on April 15, 2025:
    Looks like i missed that during rebase as it was introduced in #32166. Probably this entire line could be removed as it doesn’t add anything beyond what is already documented elsewhere.
  20. hodlinator commented at 9:18 pm on April 1, 2025: contributor

    Code review 297ddaa430e99ad62e6bbdcc475ede7e04da547f

    Remaining inline Q regarding naming + nits.

  21. in src/init.cpp:1590 in 297ddaa430 outdated 
    1587             }
1588 
1589-            addrProxy = Proxy(proxyAddr.value(), proxyRandomize);
1590+            addrProxy = Proxy(proxyAddr.value(), /*tor_stream_isolation=*/proxyRandomize);
1591         }
1592
    hodlinator commented at 9:26 pm on April 1, 2025:

    nit: Could mention edge-cases from #32166 (review) in PR description:

    0- This makes sure that different launches of the application won't share the same credentials, and thus circuits, even in edge cases.
1+ This makes sure that different launches of the application won't share the same credentials, and thus circuits, even in edge cases like:
2+ * restarting bitcoind in quick succession, with counter always resetting to 0
3+ * multiple bitcoinds running in parallel

    (Uses “bitcoind” over “daemon” as the former is more specific, avoids any confusion with Tor daemon).

    laanwj commented at 6:54 am on April 2, 2025:

    This code is also used by the GUI, not just bitcoind. This is why i say “the application” at the beginning of the sentence instead of being more specific.

    Edit: in this case, even other software that uses the same stream isolation code, such as forks, could interfere. It doesn’t have to be bitcoin.

    But yes might be useful to list some edge cases here, so that this is more clear outside of the context of this PR.

  22. hodlinator approved
  23. hodlinator commented at 9:42 am on April 2, 2025: contributor

    ACK 297ddaa430e99ad62e6bbdcc475ede7e04da547f

    Concept

    Good to avoid Tor circuit sharing for edge-cases like those described in #32166 (review).

    Renaming of randomize_credentials to tor_stream_isolation appears correct. (We support running CJDNS in conjunction with Tor SOCKS proxy, #32176 (review)).

    Test

    0build/bin/bitcoind -debug=proxy -debug=net -proxy=127.0.0.1:9050

    Log:

     0...
 12025-04-02T09:27:27Z [proxy] SOCKS5 sending proxy authentication ec7a807c67cd27ab-0:ec7a807c67cd27ab-0
 2...
 32025-04-02T09:27:47Z [proxy] SOCKS5 sending proxy authentication ec7a807c67cd27ab-1:ec7a807c67cd27ab-1
 4
 52025-04-02T09:27:51Z [proxy] Using proxy: 127.0.0.1:9050 to connect to <redacted>:8333
 62025-04-02T09:27:51Z [net] SOCKS5 connecting <redacted>
 72025-04-02T09:27:51Z [proxy] SOCKS5 sending proxy authentication ec7a807c67cd27ab-2:ec7a807c67cd27ab-2
 8...
 92025-04-02T09:27:58Z [net] SOCKS5 connected <redacted>
102025-04-02T09:27:58Z [net] Added connection peer=1
112025-04-02T09:27:58Z [net] sending version (103 bytes) peer=1
122025-04-02T09:27:58Z [net] send version message: version 70016, blocks=880756, txrelay=1, peer=1
132025-04-02T09:27:58Z [net] start sending v2 handshake to peer=1
142025-04-02T09:27:58Z [net] received: version (102 bytes) peer=1
152025-04-02T09:27:58Z [net] sending wtxidrelay (0 bytes) peer=1
162025-04-02T09:27:58Z [net] sending sendaddrv2 (0 bytes) peer=1
172025-04-02T09:27:58Z [net] sending verack (0 bytes) peer=1
182025-04-02T09:27:58Z [net] sending getaddr (0 bytes) peer=1
192025-04-02T09:27:58Z [net] receive version message: /Satoshi:28.0.0/: version 70016, blocks=890528, us=<redacted>:42840, txrelay=1, peer=1
  24. DrahtBot requested review from eval-exec on Apr 2, 2025
  25. DrahtBot requested review from 1440000bytes on Apr 2, 2025
  26. net: Add randomized prefix to Tor stream isolation credentials 
    Add a class TorsStreamIsolationCredentialsGenerator that generates
unique credentials based on a randomly generated session prefix
and an atomic counter.

This makes sure that different launches of the application won't share
the same credentials, and thus circuits, even in edge cases.

Example with `-debug=proxy`:
```
2025-03-31T16:30:27Z [proxy] SOCKS5 sending proxy authentication 0afb2da441f5c105-0:0afb2da441f5c105-0
2025-03-31T16:30:31Z [proxy] SOCKS5 sending proxy authentication 0afb2da441f5c105-1:0afb2da441f5c105-1
```

Thanks to hodlinator for the idea.
    ec81a72b36
  27. laanwj force-pushed on Apr 3, 2025
  28. laanwj commented at 10:15 am on April 3, 2025: member

    297ddaa430e99ad62e6bbdcc475ede7e04da547f → ec81a72b369ab9efe23681ebb6e8fab34ce2e0f2

  29. laanwj force-pushed on Apr 3, 2025
  30. laanwj commented at 2:55 pm on April 3, 2025: member
    Temporarily cherry-picked #32203 into here for testing fix for CI failure. Will revert this branch back to the old commit afterwards.
  31. in src/test/fuzz/i2p.cpp:37 in a20327dcd3 outdated 
    33@@ -34,7 +34,7 @@ FUZZ_TARGET(i2p, .init = initialize_i2p)
34 
35     const fs::path private_key_path = gArgs.GetDataDirNet() / "fuzzed_i2p_private_key";
36     const CService addr{in6_addr(IN6ADDR_LOOPBACK_INIT), 7656};
37-    const Proxy sam_proxy{addr, false};
38+    const Proxy sam_proxy{addr, /*tor_stream_isolation=*/false};
    hodlinator commented at 9:03 pm on April 3, 2025:
    (Thanks for changing this, I have a strong preference for documenting bool parameters like you’ve done).
    laanwj commented at 8:47 am on April 7, 2025:
    Same! Boolean parameters are the least self-descriptive. While making this change i also realized that this field is completely pointless for SAM proxies, which aren’t SOCKS, nor have a concept of stream isolation. But using an interchangeable Proxy type makes sense to keep SetProxy logic simpler, and it’s not worth defining a whole type hierarchy for a simple data record.
  32. laanwj force-pushed on Apr 4, 2025
  33. hodlinator approved
  34. hodlinator commented at 6:35 pm on April 4, 2025: contributor

    re-ACK ec81a72b369ab9efe23681ebb6e8fab34ce2e0f2

    Changes since first/previous ACK:

    • C++ style closer to doc/developer-notes.md (fixed class, still has braces on same line in methods).
    • Improved comment about edge-cases fixed by generating unique prefix, and fixed typo.
    • Added missing named parameter comments in i2p tests.
  35. laanwj closed this on Apr 7, 2025
  36. laanwj reopened this on Apr 7, 2025
  37. jonatack commented at 6:40 pm on April 7, 2025: member

    ACK ec81a72b369ab9efe23681ebb6e8fab34ce2e0f2

    Adds a randomized prefix as advertised, re-generated on node startup, with an incrementing counter as before for each peer connection. Tested with all p2p networks on (clearnet, tor, i2p, cjdns).

    before

    02025-04-07T18:36:00.698770Z [opencon] [netbase.cpp:415] [Socks5] [proxy] SOCKS5 sending proxy authentication 0:0
1...
22025-04-07T18:36:05.967300Z [opencon] [netbase.cpp:415] [Socks5] [proxy] SOCKS5 sending proxy authentication 1:1

    after

    02025-04-07T18:34:05.429803Z [opencon] [netbase.cpp:415] [Socks5] [proxy] SOCKS5 sending proxy authentication d1abb33709dc223a-0:d1abb33709dc223a-0
1...
22025-04-07T18:34:15.577895Z [opencon] [netbase.cpp:415] [Socks5] [proxy] SOCKS5 sending proxy authentication d1abb33709dc223a-1:d1abb33709dc223a-1

    restart node, new prefix

    02025-04-07T18:40:31.064061Z [opencon] [netbase.cpp:415] [Socks5] [proxy] SOCKS5 sending proxy authentication d0b6aa15cd1b1757-0:d0b6aa15cd1b1757-0
1...
22025-04-07T18:40:49.239803Z [opencon] [netbase.cpp:415] [Socks5] [proxy] SOCKS5 sending proxy authentication d0b6aa15cd1b1757-1:d0b6aa15cd1b1757-1
3...
42025-04-07T18:41:05.524990Z [opencon] [netbase.cpp:415] [Socks5] [proxy] SOCKS5 sending proxy authentication d0b6aa15cd1b1757-2:d0b6aa15cd1b1757-2
5...
62025-04-07T18:41:18.697931Z [opencon] [netbase.cpp:415] [Socks5] [proxy] SOCKS5 sending proxy authentication d0b6aa15cd1b1757-3:d0b6aa15cd1b1757-3
7...
82025-04-07T18:41:47.225044Z [opencon] [netbase.cpp:415] [Socks5] [proxy] SOCKS5 sending proxy authentication d0b6aa15cd1b1757-4:d0b6aa15cd1b1757-4

    another restart, new prefix

    02025-04-07T18:44:18.570628Z [opencon] [netbase.cpp:415] [Socks5] [proxy] SOCKS5 sending proxy authentication 8012260f6062176e-0:8012260f6062176e-0
  38. danielabrozzoni commented at 6:02 pm on April 9, 2025: contributor

    tACK ec81a72b369ab9efe23681ebb6e8fab34ce2e0f2 The code looks good to me. The way I understand it, Bitcoin Core uses different credentials for each request in order to enable the legacy tor stream isolation, thus using different circuits for each request. This PR introduces a randomized prefix to the credentials that is unique to each session, preventing multiple instances of bitcoin core from reusing the same circuits.

    I tested manually and verified that the circuit credentials vary at each run:

    02025-04-09T17:53:44Z [proxy] SOCKS5 sending proxy authentication fad3434ea2af7c7b-0:fad3434ea2af7c7b-3
1[...]
22025-04-09T17:53:47Z [proxy] SOCKS5 sending proxy authentication fad3434ea2af7c7b-1:fad3434ea2af7c7b-4

    After restarting:

    02025-04-09T17:53:55Z [proxy] SOCKS5 sending proxy authentication 8ffd3299d2634f06-0:8ffd3299d2634f06-0
  39. glozow merged this on Apr 10, 2025
  40. glozow closed this on Apr 10, 2025


laanwj DrahtBot 1440000bytes eval-exec hodlinator ryanofsky jonatack danielabrozzoni


eval-exec 1440000bytes

Labels
P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-04-16 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me