p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting

mzumsande commented at 7:56 pm on April 3, 2023: contributor

The current logic for self-advertisements works such that we detect as many local addresses as we can, and then, using the scoring matrix from CNetAddr::GetReachabilityFrom(), self-advertise with the address that fits best to our peer. It is in general not hard for our peers to distinguish our self-advertisements from other addrs we send them, because we self-advertise every ~24h and because the first addr we send over a connection is likely our self-advertisement.

GetReachabilityFrom() currently only takes into account actual reachability, but not whether we’d want to announce our identity for one network to peers from other networks, which is not straightforward in connection with privacy networks.

While the general approach is to prefer self-advertising with the address for the network our peer is on, there are several special situations in which we don’t have one, and as a result could allow self-advertise other local addresses, for example:

A) We run i2p and clearnet, use -i2pacceptincoming=0 (so we have no local i2p address), and we have a local ipv4 address. In this case, we’d advertise the ipv4 address to our outbound i2p peers.

B) Our -discover logic cannot detect any local clearnet addresses in our network environment, but we are actually reachable over clearnet. If we ran bitcoind clearnet-only, we’d always advertise the address our peer sees us with instead, and could get inbound peers this way. Now, if we also have an onion service running (but aren’t using tor as a proxy for clearnet connections), we could advertise our onion address to clearnet peers, so that they would be able to connect our clearnet and onion identities.

This PR tries to avoid these situations by 1.) never advertising our local Tor or I2P address to peers from other networks. 2.) never advertising local addresses from non-anonymity networks to peers from Tor or I2P

Note that this affects only our own self-advertisements, the rules to forward other people’s addrs are not changed.

[Edit] after Initial discussion: CJDNS is not being treated like Tor and I2P at least for now, because it has different privacy properties and for the practical reason that it has still very few bitcoin nodes.

DrahtBot commented at 7:56 pm on April 3, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild, luke-jr, achow101
Concept ACK	dergoegge, amitiuttarwar

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:

#27071 (Handle CJDNS from LookupSubNet() by vasild)
#25390 (sync: introduce a thread-safe generic container and use it to remove a bunch of “GlobalMutex"es 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.

DrahtBot added the label P2P on Apr 3, 2023

mzumsande marked this as a draft on Apr 3, 2023

dergoegge commented at 10:06 am on April 4, 2023: member

Concept ACK

Personally, I have given up a little bit on fighting fingerprinting (at least I’ve de-prioritized it) because at this point there are so many fingerprinting techniques that we should probably either give up or figure out a way to avoid them ~entirely by design.

we could advertise our onion address to clearnet peers

But this is such an obvious fingerprint that we should address it.

vasild commented at 10:23 am on April 4, 2023: contributor

Concept ACK

On the approach - this PR expands CNetAddr::GetReachabilityFrom() with some logic whether we want to advertise or not. Such a logic is already in GetLocal() and CNetAddr::GetReachabilityFrom() is simply about reachability. It would be good to keep this separation.

Is it not possible to achieve the same with something simpler, like the following?

 0--- i/src/net.cpp
 1+++ w/src/net.cpp
 2@@ -164,12 +164,22 @@ bool GetLocal(CService& addr, const CNetAddr *paddrPeer)
 3     int nBestScore = -1;
 4     int nBestReachability = -1;
 5     {
 6         LOCK(g_maplocalhost_mutex);
 7         for (const auto& entry : mapLocalHost)
 8         {
 9+            // For privacy reasons, don't advertise our privacy-network address
10+            // to other networks and don't advertise our other-network address
11+            // to privacy networks.
12+            const auto& local_addr = entry.first;
13+            if ((local_addr.IsTor() && !paddrPeer->IsTor()) ||
14+                (!local_addr.IsTor() && paddrPeer->IsTor()) ||
15+                (local_addr.IsI2P() && !paddrPeer->IsI2P()) ||
16+                (!local_addr.IsI2P() && paddrPeer->IsI2P())) {
17+                continue;
18+            }
19             int nScore = entry.second.nScore;

vasild commented at 10:29 am on April 4, 2023: contributor

Or why not even expand this logic to all networks and delete CNetAddr::GetReachabilityFrom()? That is - advertise our X address only to peers from network X (for X being any of IPv4, IPv6, Tor, I2P, CJDNS).

amitiuttarwar commented at 2:37 pm on April 4, 2023: contributor

concept ACK. good catch!

mzumsande commented at 9:51 pm on April 4, 2023: contributor

Is it not possible to achieve the same with something simpler, like the following? (…)

We’d need one more if so that we don’t stop advertising onion addresses to 127.0.0.1 peers (inbounds from tor).

But yes, I will try this alternative out. Alternatively, I also thought of renaming GetReachabilityFrom to something like GetAdvertisingScore - since we don’t use it anywhere else.

Or why not even expand this logic to all networks and delete CNetAddr::GetReachabilityFrom()? That is - advertise our X address only to peers from network X (for X being any of IPv4, IPv6, Tor, I2P, CJDNS).

I think that would be too drastic: I don’t see a reason to never advertise IPv4 addresses to IPv6 peers, and vice versa - for clearnet, better propagation seems more important than fingerprinting concerns. What do you think about CJDNS?

vasild commented at 9:17 am on April 5, 2023: contributor

Right, incoming Tor need special handling. That is already what CNode::ConnectedThroughNetwork() does. So it would look like:

0            // For privacy reasons, don't advertise our privacy-network address
1            // to other networks and don't advertise our other-network address
2            // to privacy networks.
3            const Network our_net{entry.first.GetNetwork()};
4            const Network peers_net{node.ConnectedThroughNetwork()};
5            if (our_net != peers_net && 
6                (our_net == NET_ONION || our_net == NET_I2P || 
7                 peers_net == NET_ONION || peers_net == NET_I2P)) {
8                continue;                                                  
9            }

(and pass CNode& instead of CNetAddr*).

I am not sure about CJDNS. Normally I would say treat it like Tor and I2P (but I even had the heretic idea to do that with IPv4 and IPv6 too, so maybe I lean a bit to the paranoid side, don’t listen to me too much). However there are very few CJDNS addresses right now on mainnet and if we only advertise our CJDNS address to CJDNS peers that could impede its adoption.

Ultimately, the perfect solution would be to broadcast our address exactly like we broadcast foreign addresses. So that a spying peer cannot distinguish which one is it. Right now we are more aggressive in broadcasting our address than broadcasting foreign addresses. To make it the same, either:

Stop being more aggressive with our address, or
Broadcast more aggressively also foreign addresses.

either one would be a serious change and would need some simulation to be justified. Just thinking aloud.

mzumsande commented at 8:58 pm on April 11, 2023: contributor

I am not sure about CJDNS. Normally I would say treat it like Tor and I2P (but I even had the heretic idea to do that with IPv4 and IPv6 too, so maybe I lean a bit to the paranoid side, don’t listen to me too much). However there are very few CJDNS addresses right now on mainnet and if we only advertise our CJDNS address to CJDNS peers that could impede its adoption.

Makes sense to me - if bitcoin over CJDNS gets more popular we could revisit this later.

Ultimately, the perfect solution would be to broadcast our address exactly like we broadcast foreign addresses. (…)

Yes, this would probably require an overhaul of the entire address relay mechanism, and I’m not sure if the benefits would be that great - after all, we don’t really want to keep our address private, just not fingerprint ourselves in the most obvious way.

Right, incoming Tor need special handling. That is already what CNode::ConnectedThroughNetwork() does. So it would look like: (…)

I think I will switch to this approach, keeping reachability and privacy concerns separate might be good. Will also try to add some unit test coverage for this area of code (there seems to be none so far), and then push an update in the next days.

mzumsande force-pushed on Apr 12, 2023

mzumsande commented at 6:06 pm on April 12, 2023: contributor

Updated the PR to separate privacy checks from reachability checks.
Passed a CNode to GetLocal() and replaced pointers with references in GetLocal() and GetReachabilityFrom()
Added a unit test

mzumsande marked this as ready for review on Apr 12, 2023

in src/netaddress.cpp:726 in 6be8b6d810 outdated

724@@ -725,17 +725,15 @@ std::vector<unsigned char> CNetAddr::GetAddrBytes() const
725 // and only used in GetReachabilityFrom
726 static const int NET_UNKNOWN = NET_MAX + 0;

vasild commented at 6:12 am on April 13, 2023:

You may expand commit b3261389eadf050c0de590c2d9cd69ad9f62acff net, refactor: pass reference for peer address in GetReachabilityFrom to remove NET_UNKNOWN since it is unused after that commit.

mzumsande commented at 6:43 pm on April 13, 2023:

good catch, removed NET_UNKNOWN and decreased NET_TEREDO by 1.

in src/test/net_tests.cpp:955 in 6be8b6d810 outdated

950+    BOOST_REQUIRE(addr_onion.SetSpecial("pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd.onion"));
951+    BOOST_REQUIRE(addr_onion.IsValid());
952+    BOOST_REQUIRE(addr_onion.IsTor());
953+
954+    CAddress addr_i2p;
955+    BOOST_REQUIRE(addr_i2p.SetSpecial("UDHDrtrcetjm5sxzskjyr5ztpeszydbh4dpl3pl4utgqqw2v4jna.b32.I2P"));

vasild commented at 6:34 am on April 13, 2023:

nit: it is case-insensitive, so no point in using a mixture of lower case and upper case.

mzumsande commented at 6:43 pm on April 13, 2023:

fixed

in src/test/net_tests.cpp:980 in 6be8b6d810 outdated

975+    BOOST_CHECK(GetLocalAddress(*peer_ipv4) == addr_ipv4);
976+    BOOST_CHECK(GetLocalAddress(*peer_ipv6) == addr_ipv4);
977+    BOOST_CHECK(GetLocalAddress(*peer_ipv6_tunnel) == addr_ipv4);
978+    BOOST_CHECK(GetLocalAddress(*peer_teredo) == addr_ipv4);
979+    BOOST_CHECK(GetLocalAddress(*peer_cjdns) == addr_ipv4);
980+    BOOST_CHECK(GetLocalAddress(*peer_onion) == addr_empty);

vasild commented at 6:42 am on April 13, 2023:

nit: might as well ditch addr_empty and use

0    BOOST_CHECK(!GetLocalAddress(*peer_onion).IsValid());

mzumsande commented at 6:44 pm on April 13, 2023:

done

in src/test/net_tests.cpp:997 in 6be8b6d810 outdated

992+    BOOST_CHECK(GetLocalAddress(*peer_onion) == addr_onion);
993+    BOOST_CHECK(GetLocalAddress(*peer_i2p) == addr_i2p);
994+    RemoveLocal(addr_onion);
995+    RemoveLocal(addr_i2p);
996+
997+    // local peers from all networks

vasild commented at 6:43 am on April 13, 2023:

0    // local addresses from all networks

mzumsande commented at 6:44 pm on April 13, 2023:

fixed

vasild approved

vasild commented at 6:58 am on April 13, 2023: contributor

ACK 6be8b6d8106210c6f44bc9c0124bfd1476d2952e

mzumsande force-pushed on Apr 13, 2023

vasild approved

vasild commented at 5:20 pm on April 14, 2023: contributor

ACK 3f93ea39da26558cc731b6e4ecd8fa0152eaf94c

DrahtBot added the label CI failed on Jun 5, 2023

net, refactor: pass CNode instead of CNetAddr to GetLocalAddress

Access to CNode will be needed in the following commits.

62d73f5370

net, refactor: pass reference for peer address in GetReachabilityFrom

The address of the peer always exists (because addr is a member of
CNode), so it was not possible to pass a nullptr before.
Also remove NET_UNKNOWN, which is unused now.

e4d541c7cf

net: restrict self-advertisements with privacy networks

Stop advertising
1) our i2p/onion address to peers from other networks
2) Local addresses of non-privacy networks to i2p/onion peers
Doing so could lead to fingerprinting ourselves.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

f4754b9dfb

test: add unit test for local address advertising e7cf8657e1

mzumsande force-pushed on Jun 5, 2023

mzumsande commented at 5:05 pm on June 5, 2023: contributor

Needs rebase?

yes. Rebased due to silent conflict with #26261.

DrahtBot removed the label CI failed on Jun 5, 2023

luke-jr commented at 10:28 pm on June 22, 2023: member

Note that this affects only our own self-advertisements, the rules to forward other people’s addrs are not changed.

Would this make our own addr stand out too? Or could we still forward our own addr by coincidence outside of self-advertising?

vasild approved

vasild commented at 5:00 pm on June 28, 2023: contributor

ACK e7cf8657e1165ea5da3911a9e543837cd8938f97

mzumsande commented at 8:33 pm on June 28, 2023: contributor

Note that this affects only our own self-advertisements, the rules to forward other people’s addrs are not changed.

Would this make our own addr stand out too? Or could we still forward our own addr by coincidence outside of self-advertising?

Yes, we could still forward it: When we receive a gossipped address we choose 1-2 random peers (that stay the same for 24 hours, see RelayAddress) and forward it to them if we don’t think they already know it. So if we receive our own address from someone else we could forward it to the chosen peer, even if that peer wasn’t eligible for the original self-announcement because it’s from another network.

luke-jr approved

luke-jr commented at 1:34 am on July 1, 2023: member

utACK e7cf8657e1165ea5da3911a9e543837cd8938f97

Approach: Unsure about the refactoring, but ok

achow101 commented at 5:41 pm on July 13, 2023: member

ACK e7cf8657e1165ea5da3911a9e543837cd8938f97

achow101 merged this on Jul 13, 2023

achow101 closed this on Jul 13, 2023

in src/net.cpp:174 in e7cf8657e1

169+            // to privacy networks.
170+            const Network our_net{entry.first.GetNetwork()};
171+            const Network peers_net{peer.ConnectedThroughNetwork()};
172+            if (our_net != peers_net &&
173+                (our_net == NET_ONION || our_net == NET_I2P ||
174+                 peers_net == NET_ONION || peers_net == NET_I2P)) {

jonatack commented at 5:21 pm on July 14, 2023:

f4754b9dfb84859166843fb2a1888fb3cfebf73c It would be nice to avoid low-level Network enum value comparisons when we have built-in higher level helpers (IsTor, IsI2P) we can use.

Edit: done in #28078

in src/net.h:167 in e7cf8657e1

163@@ -164,8 +164,8 @@ bool AddLocal(const CNetAddr& addr, int nScore = LOCAL_NONE);
164 void RemoveLocal(const CService& addr);
165 bool SeenLocal(const CService& addr);
166 bool IsLocal(const CService& addr);
167-bool GetLocal(CService &addr, const CNetAddr *paddrPeer = nullptr);
168-CService GetLocalAddress(const CNetAddr& addrPeer);
169+bool GetLocal(CService& addr, const CNode& peer);

jonatack commented at 5:23 pm on July 14, 2023:

While touching this, the GetLocal() getter helper is unused outside the class and could be

moved from the header to the implementation
made static and nodiscard
converted from a bool with an out-param to a std::optional without an out-param

Edit: done in #28078

jonatack commented at 5:26 pm on July 14, 2023: member

Post-merge ACK

sidhujag referenced this in commit 86950b76c2 on Jul 15, 2023

MarnixCroes commented at 11:49 am on August 9, 2023: contributor

Should this be added to the release notes?

luke-jr referenced this in commit 0942619167 on Aug 16, 2023

luke-jr referenced this in commit 7cd427f867 on Aug 16, 2023

luke-jr referenced this in commit ca290a5fad on Aug 16, 2023

luke-jr referenced this in commit 699ebc356a on Aug 16, 2023

mzumsande commented at 1:31 pm on August 17, 2023: contributor

Should this be added to the release notes?

Not sure. I think the impact on users isn’t that strong (it fixes behavior in rather rare constellations), so I wouldn’t have added it. But happy to add it, if others would like it included!

achow101 referenced this in commit 41cb17fdb6 on Sep 21, 2023

mzumsande deleted the branch on Nov 7, 2023

kwvg referenced this in commit c5eb3ad19d on Sep 6, 2024

kwvg referenced this in commit 358d94dc20 on Sep 7, 2024

kwvg referenced this in commit b5ba2e4774 on Sep 11, 2024

kwvg referenced this in commit a949dcb169 on Sep 12, 2024

kwvg referenced this in commit ec8263ba74 on Sep 13, 2024

kwvg referenced this in commit b9432787a2 on Sep 13, 2024

kwvg referenced this in commit 8320e0ca8e on Sep 13, 2024

PastaPastaPasta referenced this in commit ca4b9eaad1 on Sep 16, 2024

bitcoin locked this on Nov 6, 2024

p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting #27411

Reviews

Conflicts