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

mzumsande · 2023-04-03T19:56:42Z

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 · 2023-04-03T19:56:45Z

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.

dergoegge · 2023-04-04T10:06:39Z

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

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?

--- i/src/net.cpp
+++ w/src/net.cpp
@@ -164,12 +164,22 @@ bool GetLocal(CService& addr, const CNetAddr *paddrPeer)
     int nBestScore = -1;
     int nBestReachability = -1;
     {
         LOCK(g_maplocalhost_mutex);
         for (const auto& entry : mapLocalHost)
         {
+            // For privacy reasons, don't advertise our privacy-network address
+            // to other networks and don't advertise our other-network address
+            // to privacy networks.
+            const auto& local_addr = entry.first;
+            if ((local_addr.IsTor() && !paddrPeer->IsTor()) ||
+                (!local_addr.IsTor() && paddrPeer->IsTor()) ||
+                (local_addr.IsI2P() && !paddrPeer->IsI2P()) ||
+                (!local_addr.IsI2P() && paddrPeer->IsI2P())) {
+                continue;
+            }
             int nScore = entry.second.nScore;

vasild · 2023-04-04T10:29:59Z

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 · 2023-04-04T14:37:16Z

concept ACK. good catch!

mzumsande · 2023-04-04T21:51:55Z

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 · 2023-04-05T09:17:35Z

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

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

(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 · 2023-04-11T20:58:17Z

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 · 2023-04-12T18:06:58Z

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

vasild

ACK 6be8b6d

vasild · 2023-04-13T06:12:50Z

src/netaddress.cpp

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


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

good catch, removed NET_UNKNOWN and decreased NET_TEREDO by 1.

src/test/net_tests.cpp

vasild

ACK 3f93ea3

Access to CNode will be needed in the following commits.

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.

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>

mzumsande · 2023-06-05T17:05:05Z

Needs rebase?

yes. Rebased due to silent conflict with #26261.

vasild

ACK e7cf865

mzumsande · 2023-06-28T20:33:29Z

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

utACK e7cf865

Approach: Unsure about the refactoring, but ok

achow101 · 2023-07-13T17:41:32Z

ACK e7cf865

jonatack

Post-merge ACK

jonatack · 2023-07-14T17:21:33Z

src/net.cpp

+            const Network peers_net{peer.ConnectedThroughNetwork()};
+            if (our_net != peers_net &&
+                (our_net == NET_ONION || our_net == NET_I2P ||
+                 peers_net == NET_ONION || peers_net == NET_I2P)) {


f4754b9 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

jonatack · 2023-07-14T17:23:12Z

src/net.h

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


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

…etworks…

MarnixCroes · 2023-08-09T11:49:10Z

Should this be added to the release notes?

Access to CNode will be needed in the following commits. Github-Pull: bitcoin#27411 Rebased-From: 62d73f5

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. Github-Pull: bitcoin#27411 Rebased-From: e4d541c

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> Github-Pull: bitcoin#27411 Rebased-From: f4754b9

Github-Pull: bitcoin#27411 Rebased-From: 3f93ea3

mzumsande · 2023-08-17T13:31:04Z

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!

…use helpers over low-level code, use optional 4ecfd3e Inline short, often-called, rarely-changed basic CNetAddr getters (Jon Atack) 5316ae5 Convert GetLocal() to std::optional and remove out-param (Jon Atack) f1304db Use higher-level CNetAddr and CNode helpers in net.cpp (Jon Atack) 07f5891 Add CNetAddr::IsPrivacyNet() and CNode::IsConnectedThroughPrivacyNet() (Jon Atack) df48856 GetLocal() type-safety, naming, const, and formatting cleanups (stickies-v) fb42657 Add and use CNetAddr::HasCJDNSPrefix() helper (Jon Atack) 5ba73cd Move GetLocal() declaration from header to implementation (Jon Atack) 11426f6 Move CaptureMessageToFile() declaration from header to implementation (Jon Atack) deccf1c Move IsPeerAddrLocalGood() declaration from header to implementation (Jon Atack) Pull request description: and other improvements noticed while reviewing #27411. Addresses bitcoin/bitcoin#27411 (comment) and bitcoin/bitcoin#27411 (comment). See commit messages for details. ACKs for top commit: achow101: ACK 4ecfd3e vasild: ACK 4ecfd3e stickies-v: ACK 4ecfd3e Tree-SHA512: d792bb2cb24690aeae9bedf97e92b64fb6fd080c39385a4bdb8ed05f37f3134d85bda99da025490829c03017fd56382afe7951cdd039aede1c08ba98fb1aa7f9

…ks to avoid fingerprinting

…ks to avoid fingerprinting The old `GetLocal()` has been moved to `masternode/node.cpp` due to its use in determining a node's external address. We don't want the old variant to be used otherwise so we'll move it out of `net.{cpp,h}` for good measure. Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

, bitcoin#25962, bitcoin#26888, bitcoin#27264, bitcoin#27257, bitcoin#27324, bitcoin#27374, bitcoin#27467, bitcoin#27411, partial bitcoin#25472 (networking backports: part 8) 8320e0c merge bitcoin#27411: Restrict self-advertisements with privacy networks to avoid fingerprinting (Kittywhiskers Van Gogh) 1376289 merge bitcoin#27467: skip netgroup diversity follow-up (Kittywhiskers Van Gogh) a52b3a3 merge bitcoin#27374: skip netgroup diversity of new connections for tor/i2p/cjdns (Kittywhiskers Van Gogh) ab11e0f merge bitcoin#27324: bitcoin#27257 follow-ups (Kittywhiskers Van Gogh) 9023dd2 merge bitcoin#27257: End friendship of CNode, CConnman and ConnmanTestMsg (Kittywhiskers Van Gogh) 3465df2 merge bitcoin#27264: Improve diversification of new connections (Kittywhiskers Van Gogh) d3f5b38 merge bitcoin#26888: simplify the call to vProcessMsg.splice() (Kittywhiskers Van Gogh) d9e56f3 merge bitcoin#25962: Add CNodeOptions and increase constness (Kittywhiskers Van Gogh) 79e67fd merge bitcoin#25814: simplify GetLocalAddress() (Kittywhiskers Van Gogh) 6d49454 partial bitcoin#25472: Increase MS Visual Studio minimum version (Kittywhiskers Van Gogh) 54bb3a4 merge bitcoin#25500: Move inbound eviction logic to its own translation unit (Kittywhiskers Van Gogh) b50febc merge bitcoin#24531: Use designated initializers (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6254 * When backporting [bitcoin#27411](bitcoin#27411), the `CNetAddr*` variant of `GetLocal()` was not removed (upstream it was replaced by the `CNode&` variant with additional checks that rely on fields in `CNode`) as `CActiveMasternodeManager` relies on `GetLocal()` to detect a valid external address. * While it can also rely on other nodes to determine that, removing code that tests against a well-known public address would increase the number of reported failures esp. if the checks are run _before_ the node has a chance to connect to any peers. ## Breaking Changes None observed. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 8320e0c PastaPastaPasta: utACK 8320e0c Tree-SHA512: 1d02bc33c8d62c392960d4dd044edf3de08515a5e8c8794d95cd95e9654da91b20e7290436cf9c79b0ea8dbd42b27dcc61c8eb17e573902574d7b281b8874584

DrahtBot added the P2P label Apr 3, 2023

mzumsande marked this pull request as draft April 3, 2023 19:58

vasild reviewed Apr 4, 2023

View reviewed changes

mzumsande force-pushed the 202303_advertise_nets branch from a98feda to 6be8b6d Compare April 12, 2023 18:02

mzumsande marked this pull request as ready for review April 12, 2023 18:30

This was referenced Apr 13, 2023

net: support unix domain sockets for -proxy and -onion #27375

Merged

Handle CJDNS from LookupSubNet() #27071

Merged

sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es #25390

Closed

vasild approved these changes Apr 13, 2023

View reviewed changes

DrahtBot mentioned this pull request Apr 13, 2023

BIP324: Add encrypted p2p transport {de}serializer #23233

Closed

mzumsande force-pushed the 202303_advertise_nets branch 2 times, most recently from eda6e87 to 3f93ea3 Compare April 13, 2023 18:46

vasild approved these changes Apr 14, 2023

View reviewed changes

DrahtBot mentioned this pull request May 17, 2023

ZMQ: Support UNIX domain sockets #27679

Merged

DrahtBot added the CI failed label Jun 5, 2023

mzumsande and others added 4 commits June 5, 2023 11:02

net, refactor: pass CNode instead of CNetAddr to GetLocalAddress

62d73f5

Access to CNode will be needed in the following commits.

net, refactor: pass reference for peer address in GetReachabilityFrom

e4d541c

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.

net: restrict self-advertisements with privacy networks

f4754b9

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>

test: add unit test for local address advertising

e7cf865

mzumsande force-pushed the 202303_advertise_nets branch from 3f93ea3 to e7cf865 Compare June 5, 2023 17:01

DrahtBot removed the CI failed label Jun 5, 2023

vasild approved these changes Jun 28, 2023

View reviewed changes

luke-jr approved these changes Jul 1, 2023

View reviewed changes

achow101 merged commit 05ad4de into bitcoin:master Jul 13, 2023

jonatack reviewed Jul 14, 2023

View reviewed changes

jonatack mentioned this pull request Jul 14, 2023

net, refactor: remove unneeded exports, use helpers over low-level code, use optional #28078

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 15, 2023

Merge bitcoin#27411: p2p: Restrict self-advertisements with privacy n…

86950b7

…etworks…

benthecarman mentioned this pull request Jul 19, 2023

July Topics austinbitdevs/austinbitdevs.com#22

Closed

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023

net, refactor: pass CNode instead of CNetAddr to GetLocalAddress

0942619

Access to CNode will be needed in the following commits. Github-Pull: bitcoin#27411 Rebased-From: 62d73f5

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023

test: add unit test for local address advertising

699ebc3

Github-Pull: bitcoin#27411 Rebased-From: 3f93ea3

vasild mentioned this pull request Aug 17, 2023

p2p sends local-scope addresses when IPv4 is behind NAT #8616

Closed

This was referenced Sep 7, 2023

Socratic Session: Bitcoin Core - Cherry Picked PRs and Project Updates TABConf/2023.tabconf.com#189

Closed

Socratic Seminar #11 Topics NashBitDevs/nashbitdevs.org#12

Closed

mzumsande deleted the 202303_advertise_nets branch November 7, 2023 20:29

kwvg added a commit to kwvg/dash that referenced this pull request Sep 6, 2024

merge bitcoin#27411: Restrict self-advertisements with privacy networ…

c5eb3ad

…ks to avoid fingerprinting

kwvg added a commit to kwvg/dash that referenced this pull request Sep 7, 2024

merge bitcoin#27411: Restrict self-advertisements with privacy networ…

358d94d

…ks to avoid fingerprinting

kwvg added a commit to kwvg/dash that referenced this pull request Sep 11, 2024

merge bitcoin#27411: Restrict self-advertisements with privacy networ…

b5ba2e4

…ks to avoid fingerprinting

kwvg added a commit to kwvg/dash that referenced this pull request Sep 12, 2024

merge bitcoin#27411: Restrict self-advertisements with privacy networ…

a949dcb

…ks to avoid fingerprinting

kwvg mentioned this pull request Sep 12, 2024

backport: merge bitcoin#24531, #25500, #25814, #25962, #26888, #27264, #27257, #27324, #27374, #27467, #27411, partial bitcoin#25472 (networking backports: part 8) dashpay/dash#6255

Merged

5 tasks

bitcoin locked and limited conversation to collaborators Nov 6, 2024

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

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

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

Uh oh!

Conversation

mzumsande commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

dergoegge commented Apr 4, 2023

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

vasild commented Apr 4, 2023

Uh oh!

amitiuttarwar commented Apr 4, 2023

Uh oh!

mzumsande commented Apr 4, 2023

Uh oh!

vasild commented Apr 5, 2023

Uh oh!

mzumsande commented Apr 11, 2023

Uh oh!

mzumsande commented Apr 12, 2023

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

vasild Apr 13, 2023

Choose a reason for hiding this comment

Uh oh!

mzumsande Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vasild left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzumsande commented Jun 5, 2023

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande commented Jun 28, 2023

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 13, 2023

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarnixCroes commented Aug 9, 2023

Uh oh!

mzumsande commented Aug 17, 2023

Uh oh!

Uh oh!

mzumsande commented Apr 3, 2023 •

edited

Loading

DrahtBot commented Apr 3, 2023 •

edited

Loading

mzumsande Apr 13, 2023 •

edited

Loading

vasild left a comment •

edited

Loading

jonatack Jul 14, 2023 •

edited

Loading

jonatack Jul 14, 2023 •

edited

Loading