p2p: Try to preserve outbound block-relay-only connections during restart #17428

This is an implementation of #17326:

• all (currently 2) outbound block-relay-only connections (#15759) are dumped to anchors.dat file
• on restart a node tries to connect to the addresses from anchors.dat

This PR prevents a type of eclipse attack when an attacker exploits a victim node restart to force it to connect to new, probably adversarial, peers.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #19869 (Better intervals between feelers by naumenkogs)

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.

It would be really good to limit it so that an honest node won’t attempt to connect twice to the same peer through this mechanism for some prudent amount of time… like two minutes.

I don’t think it would cause any harm to the intent of this mechanism to not bring them up as fast as possible, and it would avoid tripping up detection of aggressive mass connectors.

Not sure if I’m misreading this code (I haven’t tested it), but it looks like on restart we would only attempt to connect once to each anchor, and then it gets removed from consideration. Does that satisfy the concern about rapid reconnects? I guess if a user rapidly restarted their node, we could see rapid connections to the same peers, but that seems pretty weird.

Anyway I just skimmed the discussion in #17326. I think Greg or Matt commented that we shouldn’t do this with all our block-relay-only connections, but since we only have 2 at the moment, perhaps it’s fine for now. And then in the future if we expand to say 8 block-relay peers we might still cap the anchors at 2. Does that reflect the thinking of others as well?

@sdaftuar

Thank you for your review. Your comment has been addressed.

Also, an additional safety check added to “p2p: Integrate ReadAnchors() into CConnman()” commit in the latest push.

Concept ACK on the idea but see my concerns in #17326 (comment).

IMO before to go forward we need to agree on a) the scope of anchors: full-relay, block-relay, both, b) anchors selection & lifecyle, i.e after how much time do we select them or based on any metrics, do we prune them from anchors.dat, do we keep a bigger file and select randomly from it

d405145e8f935b454ba77e23cf7327515b85151e (jamesob/ackr/17428.1.hebasto.p2p_try_to_preserve_outb)

Code looks fine, but seeing some strange behavior locally.

Built, started bitcoind, and noted the current blocks-only peers:

0$ ./src/bitcoin-cli -datadir=/data2/bitcoin getpeerinfo | jq '.[] | select(.relaytxes==false) | .addr'
1
2"185.217.241.142:8333"
3"2.203.128.83:8333"

Verified the creation of anchors.dat:

0$ du -hs /data2/bitcoin/anchors.dat
1
24.0K    /data2/bitcoin/anchors.dat

Shutdown bitcoind, restarted, checked blocks-only peers:

0$ ./src/bitcoin-cli -datadir=/data2/bitcoin getpeerinfo | jq '.[] | select(.relaytxes==false) | .addr'
1
2"2.203.128.83:8333"
3"95.216.198.89:8333"

One of the peers has persisted from the first run, but the second is different. Unexpectedly, my logs don’t indicate the anchor connections were ever reused - the only occurrence of anchor in debug.log is when flushing to anchors.dat.

0Tested on Linux-4.15.0-47-generic-x86_64-with-Ubuntu-18.04-bionic
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-std=c++11 -Wthread-safety-analysis -Werror -Wthread-safety-analysis -Werror --enable-wallet --enable-debug --with-daemon CXX=/usr/bin/clang++-4.0 CC=/usr/bin/clang-4.0 PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --disable-jni --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --disable-jni --no-create --no-recursion
3
4Compiled with /usr/bin/ccache /usr/bin/clang++-4.0 -std=c++11 -mavx -mavx2 -std=c++11 -Wthread-safety-analysis -Werror -Wthread-safety-analysis -Werror -O0 -g3 -ftrapv  -Wstack-protector -fstack-protector-all -msse4 -msha -msse4.1 -msse4.2  i
5
6Compiler version: clang version 4.0.1-10 (tags/RELEASE_401/final)

@ariard

The only idea of this PR is to make a node to try restore outbound block-relay-only connections as they were just before node restart. It is expected that this change in node behavior mitigates restart-based eclipse attacks without significant other tradeoffs.

IMO before to go forward we need to agree on a) the scope of anchors: full-relay, block-relay, both…

Anchoring as a new procedure, or anchors as “seemingly safe” nodes are out of the scope of this PR.

b) anchors selection & lifecyle, i.e after how much time do we select them or based on any metrics, do we prune them from anchors.dat, do we keep a bigger file and select randomly from it

With an idea to make a node to try restore outbound block-relay-only connections as they were just before node restart, anchors are the last known outbound block-relay-only connections only.

#17428 (review)

What do you think about waiting some time like 24hours or one block announcement before to qualify node as anchor? That would require at least some good behavior from it, that said and may favor too much well-reliable nodes and decay network-wide connectivity..

In this PR anchors are considered as the last known outbound block-relay-only connections, not as “presumable safe” ones.

Updated d405145e8f935b454ba77e23cf7327515b85151e -> f2b4cfdc75952e61ba98eeffe26b2f86a78a45c9 (pr17428.02 -> pr17428.03, compare). @ariard @jamesob

Thank you for your reviews. Your comments have been addressed. @jamesob

… seeing some strange behavior locally.

Added additional logging when connecting to an anchor fails.

ACK 37671dd540b919f2b1bd4defeed701b6021c933e (jamesob/ackr/17428.2.hebasto.p2p_try_to_preserve_outb)

Built locally, verified that anchors were connected to on restart using the same test as above.

0Tested on Linux-4.15.0-47-generic-x86_64-with-Ubuntu-18.04-bionic
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-std=c++11 -Wthread-safety-analysis -Werror -Wthread-safety-analysis -Werror --enable-wallet --enable-debug --with-daemon CXX=/usr/bin/clang++-4.0 CC=/usr/bin/clang-4.0 PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --disable-jni --disable-shared --with-pic --enable-benchmark=no --with-bignum=no --enable-module-recovery --disable-jni --no-create --no-recursion
3
4Compiled with /usr/bin/ccache /usr/bin/clang++-4.0 -std=c++11 -mavx -mavx2 -std=c++11 -Wthread-safety-analysis -Werror -Wthread-safety-analysis -Werror -O0 -g3 -ftrapv  -Wstack-protector -fstack-protector-all -msse4 -msha -msse4.1 -msse4.2  i
5
6Compiler version: clang version 4.0.1-10 (tags/RELEASE_401/final)

@luke-jr

Couldn’t there be a risk here, that if one of the anchor peers is malicious and knows a DoS vulnerability, this ensures they can keep repeatedly crashing your node, even if you setup a watchdog to restart it?

~Could manual deletion of the anchors.dat file be a remedy?~ See: #17428 (comment)

@hebasto

Concept ACK, reading your comment I do understand that this PR is really-scoped, so don’t mind #17326 (comment) and #17428 (review) but would happily review future work to make anchors better (sorry for the noise)

A note explaining the exact attack scenario mitigated near to m_anchors or somewhere else would be cool to avoid future reviewers relying on wrong-scoped security assumptions.

Goign to test PR soon.

@luke-jr

Couldn’t there be a risk here, that if one of the anchor peers is malicious and knows a DoS vulnerability, this ensures they can keep repeatedly crashing your node, even if you setup a watchdog to restart it?

If an exploitable DoS vulnerability would exist, this risk would come true, unfortunately. As a DoS attack timing is unpredictable, it seems infeasible to prevent such a risk in the scope of this PR. This might be prevented by means of detecting a particular DoS attack, no?

If an exploitable DoS vulnerability would exist, this risk would come true, unfortunately.

This is probably among the more common vulnerability types.

(Let’s go with @gmaxwell’s suggestion to address it.)

After a consideration of @luke-jr’s concerns and @gmaxwell’s suggestion, I’m going to close this PR.

The initial idea to mitigate eclipse attacks based on node restarts, included ones initiated by adversary, introduces a new risk. @gmaxwell’s suggestion nullifies the initial idea by making anchoring usable only for clean (user initiated) restarts. The long-term impact of such changes on the entire p2p network is unclear, and requires further study.

@gmaxwell

I think closing this is a mistake. Not saving state across crashes in no way nullifies the advantage of preserving connections, and it’s also a natural implementation– e.g. the software will fail to write out the latest peers.dat across a crash.

Reopened. Going to implement your suggestion soon.

Definitely, the OP requires an update.

@gmaxwell

Not saving state across crashes in no way nullifies the advantage of preserving connections

my understanding is this PR was intended to mitigate a restart-based eclipse attack. so, if an attacker crashes a victim node, the anchors help reduce the chance of victim being eclipsed. if we change to only persisting connections on clean restarts, we lose the benefit of that scenario. ( I understand the risk of implementing that solution )

I’m still thinking it through & reading relevant material, but wondering.. what are the advantages you see of having anchors for clean restarts? (vs not having anchors). Is it to mitigate a smaller subset of eclipse attacks that occur with users intentionally restarting their nodes?

I was thinking along the same lines as Amiti and would like to understand if the DoS attack possibility mentioned above is serious enough to give up the anchors in a unclean restart, because it seems to be especially desirable to have them in this scenario: 

1. I think that the suggestion to delete anchor.dat on crashes would just change the big picture in case of a subset of DoS attacks working only on the victim’s outbound connections - for more general DoS attacks, the attacker could just as well connect to us after we have restarted (if we allow incoming connections) and attack again.
2. Such a repeated DoS attack would be tough to apply in a targeted way, because the connection is outgoing from the victim, reducing the incentives.
3. If an eclipse attacker somehow manages to control our addrman for an extended time (long enough to wait for our clean restart) it seems to me that we are pretty much screwed anyway: Even if anchors protect us from our restart, the attacker could also choose to wait until one by one, our anchors disconnect from us via clean restarts and are replaced by attacker nodes.
4. On the other hand, an attack on our addrman that would only be effective for a short while and recoverable long-time, combined with a DoS attack that forces us to restart now uncleanly, would be a scenario in which having anchors would seem really great.

Couldn’t there be a risk here, that if one of the anchor peers is malicious and knows a DoS vulnerability, this ensures they can keep repeatedly crashing your node, even if you setup a watchdog to restart it?

If an exploitable DoS vulnerability would exist, this risk would come true, unfortunately. As a DoS attack timing is unpredictable, it seems infeasible to prevent such a risk in the scope of this PR. This might be prevented by means of detecting a particular DoS attack, no?

If there was a known remote crash vulnerability and the behaviour was “okay, in case of a crash, we don’t try anchors” wouldn’t an attacker trying to eclipse a node use that exploit to ensure they can completely eclipse the node?

If your node is remote crashable, you want to fix it pretty quickly anyway because that often means you’re vulnerable to remote code execution as well; so not sure the fix here is actually better?

An adversary has two distinct ways to force a victim node to restart:

1. online activity that exploits a DoS vulnerability (i.e., a remote-crashable node)
2. offline activity: e.g., power failures, node operator’s delusion via social engineering etc.

The current PR implementation works well against the latter adversary activity.

Regarding the former case, I agree with @ajtowns:

If your node is remote crashable, you want to fix it pretty quickly anyway…

Also an adversary could just wait for a user-initiated (clean) restart of a victim node, which includes restart after a node upgrade. In such cases the current PR implementation works well too.

Updated 78b2f950350688f11b544d24ec44816554c67631 -> 780107e41fe86e31392b3ef76b917e623483259f (pr17428.06 -> pr17428.07, compare):

• a new “anchors.dat” file is created only during clean shutdown now

Also see:

• #17428 (comment)
• #17428 (comment)
• https://bitcoincore.reviews/17428.html

hm, if my understanding of the code is correct, I think we also need to wipe the anchors.dat file on startup to prevent the malicious-anchor-connection-keeps-crashing-your-node issue.

my understanding is that currently you only (potentially) remove connections from anchors.dat when you next invoke DumpAnchors() and replace them. But I might be missing something?

scenario: one of your outbound blocks-relay-only peers is malicious, but patient. eventually you restart your node for whatever reason. they are persisted to your anchors. when you startup you reconnect to them & they crash your node. and then you’re caught in the same loop?

@amitiuttarwar

hm, if my understanding of the code is correct, I think we also need to wipe the anchors.dat file on startup to prevent the malicious-anchor-connection-keeps-crashing-your-node issue.

my understanding is that currently you only (potentially) remove connections from anchors.dat when you next invoke DumpAnchors() and replace them. But I might be missing something?

scenario: one of your outbound blocks-relay-only peers is malicious, but patient. eventually you restart your node for whatever reason. they are persisted to your anchors. when you startup you reconnect to them & they crash your node. and then you’re caught in the same loop?

It seems we need to re-write the anchors.dat file every time when another anchor is dropped in m_anchors.pop_back();, no?

Concept ACK on using the two blocks-only connections as anchors & trying to reconnect to them on startup.

I’m working to better understand addrman management and the blocks-only connection logic, since that underpins the security provided by this implementation of anchors.

But this seems like a strict improvement because it increases the difficulty of executing an eclipse attack, and avoiding the major downsides of too much anchoring / increasing static-ness of transaction relay topology.

RE: malicious anchors & persisting anchors on crashes (or not)

I am still unconvinced that we shouldn’t persist anchors on crashes. I understand the risk is that a malicious anchor could exploit a DoS vulnerability (which is possibly more likely than executing an eclipse attack) and prevent the victim from being able to sync their node. I feel that if the node is DoS-attackable, it should be fixed immediately regardless. So something that prevents using it as a normal node doesn’t seem like a huge downside. And obviously the upside of persisting anchors across crashes is the extra protection against certain types of eclipse attacks.

But scoping this PR to focus only on persisting anchors on clean restarts seems reasonable for getting an improvement merged since it is less controversial.

I am still unconvinced that we shouldn’t persist anchors on crashes. I understand the risk is that a malicious anchor could exploit a DoS vulnerability (which is possibly more likely than executing an eclipse attack) and prevent the victim from being able to sync their node. I feel that if the node is DoS-attackable, it should be fixed immediately regardless.

I agree with @amitiuttarwar right now. Additionally, it’s very difficult to imagine a DoS vulnerability, which is opened only to outbound blocks-only connections. So, if it is broad DoS issue an attacker will be able to shut down all the reachable nodes, because connecting to them in-bound is free. Much bigger trouble than persistent-anchor attack. Okay, if it applies to only outbound connections, then it’s also very easy: an attacker spawns another 2,000 of reachable sybil nodes, and thus has a very high chance of a random node to connect to an attacker as a regular connection (not block-relay-only), and then restarts it. Also bigger trouble than persistent-anchor attack.

Considering these observations, I think such a specific risk (specifically block-relay-only outbound links are able to crush a node, and none other can) should not prevent us from getting benefits of anchor-persistence after crashing. (If we are really worried, one can try counting restarts ). And those benefits (anti-eclipse) are quite important I think, as this scenario (restart-to-attempt-eclipse) seems much more feasible and dangerous then just restart-node-repetitively.

But scoping this PR to focus only on persisting anchors on clean restarts seems reasonable for getting an improvement merged since it is less controversial.

I would prefer to keep the discussion going. I think we may miss an important improvement here, if we just merge clean-restarts, because the conversation will die.

ACK 780107e41fe86e31392b3ef76b917e623483259f

Reviewed code and tested locally.

I think this can be merged as a strict improvement and persisting anchors across crashes can be done as a follow-up. I don’t think the conversation would die if the follow-up PR is opened soon. But I also fail to see an issue with persisting across crashes so far and I am also happy to re-review if it gets added as part of this PR as well.

From the discussions (#17326, this PR, https://bitcoincore.reviews/17428.html, https://diyhpl.us/wiki/transcripts/la-bitdevs/2020-04-16-amiti-uttarwar-attacking-bitcoin-core/) it follows that the worst case could happen in the following setup:

1. a remote crash vulnerability is known to an adversary
2. that adversary is a block-relay-only anchor peer of the victim node
3. there is a watch-dog script that restarts the victim node in case of its crash

Please note that the message set accepted by a node from a block-relay-only peer is a strict subset of messages accepted from a full-relay outgoing connection. Therefore, the mentioned in (1) vulnerability could affect any node, regardless of anchoring. Having a such vulnerability makes other attack vectors, including all types of eclipsing, much less dangerous in comparison.

The mentioned in (3) script could be optimized to prevent continuous restarts, e.g. from @naumenkogs’ comment:

If we are really worried, one can try counting restarts

Therefore, I believe the described worst case is out of scope of this PR.

Going to update this pull soon.

Updated 780107e41fe86e31392b3ef76b917e623483259f -> dacb155eaaa1775385bc4a2d82a3e0307f68ce7d (pr17428.07 -> pr17428.09, diff):

• addressed @amitiuttarwar’s comment:

consider introducing a new constant (eg. m_max_anchors) that currently matches the value of m_max_outbound_block_relay, so if we change the number of block-relay-only connections in the future we have to explicitly decide if we want to increase our anchor connections as well

• addressed @naumenkogs’s comment:

So, if it is broad DoS issue an attacker will be able to shut down all the reachable nodes, because connecting to them in-bound is free. Much bigger trouble than persistent-anchor attack.

• addressed @fjahr’s comment:

nit: would have been nice to add a little more context information on the purpose of these anchors

• addressed @luke-jr’s comment:

wonders if anchors should be reduced/skipped entirely if the user has manually configured addnode peers

Currently, the anchors.dat contains IP addresses of the block-relay-only peers to which the node is connected, and IP addresses that were loaded from anchors.dat into memory at startup and that have not been tried to re-connect to yet.

The implementation is very simple and fairly straightforward, which is good.

After all discussions I also see that this version of the PR falls into the broader idea of anti-eclipse measures (many more to come).

It’s true that fresh connections are often a good idea, but in this case we should achieve them by peer rotation and proactive feeler tip sync, not by relying on random restarts. Maybe this can be delayed by then, but I don’t see why. This PR currently doesn’t bring any concerns (in my opinion): spying, restart attack and whatnot.

utACK 1d9264e296f1a03abbfa6c30363628a85cf33ce4 modulo those minor things I commented.

Updated 1d9264e296f1a03abbfa6c30363628a85cf33ce4 -> 0a74c7c3686d4b03a27815c6107b3300954641cc (pr17428.11 -> pr17428.13, diff):

• addressed the recent @naumenkogs’ comments
• small style improvements

Concept ACK, just a few questions above about execution.

This was tricky to test because on regtest all nodes have the same IP and my development environment doesn’t have a synced chain. I don’t think the extra 2 blocks-only nodes are connected during IBD (?)

But I hot wired bitcoind with

static const int MAX_OUTBOUND_FULL_RELAY_CONNECTIONS = 0;

…forcing my only connections to be the two anchors. Verified the anchors.dat file, tried corrupting it and tested for that error (check!) and restarted bitcoind to check that the anchor connections were loaded and connected correctly (check!).

debug.log | grep anchor:

 02020-07-03T16:54:59Z DumpAnchors: Flush 1 outbound block-relay-only peer addresses to anchors.dat started
 12020-07-03T16:54:59Z DumpAnchors: Flush 1 outbound block-relay-only peer addresses to anchors.dat completed (0.01s)
 22020-07-03T16:55:16Z DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat started
 32020-07-03T16:55:16Z DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat completed (0.06s)
 42020-07-03T16:55:32Z DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat started
 52020-07-03T16:55:32Z DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat completed (0.05s)
 62020-07-03T16:57:42Z Loaded 2 addresses from "anchors.dat"
 72020-07-03T16:57:42Z 2 block-relay-only anchors will be tried to connect to.
 82020-07-03T16:57:43Z Trying to make an anchor connection to 120.150.206.137:8333
 92020-07-03T16:57:43Z DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat started
102020-07-03T16:57:43Z DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat completed (0.00s)
112020-07-03T16:57:48Z Trying to make an anchor connection to 174.138.35.229:8333
122020-07-03T16:57:48Z DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat started
132020-07-03T16:57:48Z DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat completed (0.01s)

Updated 0a74c7c3686d4b03a27815c6107b3300954641cc -> ceb9d212716fe1134f9ac179d46e14e4847dd8b1 (pr17428.13 -> pr17428.14, diff):

• addressed @pinheadmz’ comment

nit: a bit awkward, maybe "%i block-relay-only anchors will be tried for connections.\n" ?

ACK ceb9d212716fe1134f9ac179d46e14e4847dd8b1

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK ceb9d212716fe1134f9ac179d46e14e4847dd8b1
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAl7/gI4ACgkQ5+KYS2KJ
 7yTqwOA//WN7fSijzEJXxIBImvSn9JloIV+La/INpckzxYNfT3TxfqNYZ3JU7XToT
 8yfBuMK6GjXcP3G7Ol46uOESmiJn7yAlJJinZwhmmERjZfRFFGFaXHw83LxY/qS13
 98KqFipTNTG+zBsW8BmGsEfikqLdrwWkejcIf1DaAZhct/dehe2V45L3ih5gOI7yr
10drS+dc+6wC7riA15C97Q851CpFe2F+y1aRc57Ii4M7O/u1O01TSPSI1Ip5YocA7/
11LmkHHnLXnaPTCSQIeWOjIURcIsqtMFfPJu29pBrLxI+M10QZyepqxNkB+b3oqBud
12XqyNXuayyJWyqVcZg/zR5G5pdeY5q4YInPVkF5nOt2KQHGMYKK2xVwU2TaOLqw4e
13fd7s2BKl7ZO3a0NhQKAzXhLNilXXbdbwFN1mk8TG29JhsUdvAw9r4cK/LkxkNCdh
14wn6kOW3XH2u1fmEqUpo1fkO84TzqPmrKiRSGHt94+yDUimYqqULsEUtXbUV8ISlv
1533KbJY+2glLUBx+vEnla/JB8cSlG5NHE++HfIZUTOwIkl0Z/6L39mYdwSiZ6sJA0
165ZaNaZeCFbygrNOtOiLCihmy7iPlhB+0qWscQzFO/Tufal0p5VLcqhrl+z9gj7rx
17tnksSx8rdr16O7GsDDNkpsm6NXjUSHn0+XKf1+4fHumM6BfrEu8=
18=Bw61
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

ACK 005a46dc3a4b84b045d3877ee757a162acf1f5ca

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 005a46dc3a4b84b045d3877ee757a162acf1f5ca
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAl8NyM8ACgkQ5+KYS2KJ
 7yToIBhAAgVTXUzjuIrkugUhWiSYy5daBqBCvWtc1CC9P9h6vY+MfdLdvA/KuBlF4
 8dU8oQ7Dzz0sdTRCFvXSHDrYQSjF8hKd1JY3KrYVWgdMGIwITK7VMDpMWk5SCuWBo
 9VQsaFPZmEn8x4HV2+OkCOMM+f02fKrERJ4l5QKEX76CehIt/eJN6NaSOGo2Y7JcQ
100bxoK8iWFIvKkqQV6fatraxwig40bw6Jxuj7OhlN25xnWgI2TwSGL54d+r/9viHV
112OORkqF3frgqQNP6DODQw6ROcQhhyJdfAoUWefrD2qbbaBQ4nV6sovTs91HQq5lr
12oyIX9UMSVPewMdiVsS9YO6tcL5WKq2CVu63KmGOt+jZ6WZl8Y9Xb5oehJdc9BwiL
13wkJL2LbB40Me3eA8pszIAknFrLiyDTl1zMkuooGMQ61jtHJi9ILT+uLYTdNIdEH6
14iQZkX+HujAW0RUWln3ZMeummxgd/VBF/xnxZ35QMSg8ktIzsqrKNgkshmDfNzWOF
15glh99AFuBMTUmM11ShG5icEd07h4Jh3SQ3eOwzXVXcDfGXo5DYpbs7GXGo5o109E
165jeJyw0gIpWMeJoU/26VhD5VyUmRFLUaVDiWCJlvD4GyzY2FOSf30YvAxospjCTu
17uI8+thlo+0GSrKuRwTRXoRKXpp2tlA3UC1CKkv/Sy8iZdS7PzZY=
18=mG97
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

Confirmed rebase is minimal

 0
 11:  529827588 = 1:  3370328ac p2p: Integrate ReadAnchors() into CConnman()
 22:  e3b817832 = 2:  64d1887fd p2p: Add CConnman::GetCurrentAnchors()
 33:  4f0a4c93e ! 3:  27864da6a p2p: Add DumpAnchors()
 4    @@ Commit message
 5
 6      ## src/addrdb.cpp ##
 7     @@
 8    - #include <chainparams.h>
 9      #include <clientversion.h>
10    + #include <cstdint>
11      #include <hash.h>
12     +#include <logging/timer.h>
13      #include <random.h>
144:  a4292fdd0 ! 4:  8e6eff7e4 p2p: Integrate DumpAnchors() into ProcessMessage()
15    @@ Commit message
16         p2p: Integrate DumpAnchors() into ProcessMessage()
17
18      ## src/net_processing.cpp ##
19    -@@ src/net_processing.cpp: bool ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRec
20    +@@ src/net_processing.cpp: void ProcessMessage(
21                            pfrom.nVersion.load(), pfrom.nStartingHeight,
22                            pfrom.GetId(), (fLogIPs ? strprintf(", peeraddr=%s", pfrom.addr.ToString()) : ""),
23                            pfrom.m_tx_relay == nullptr ? "block-relay" : "full-relay");
245:  e69c4d048 = 5:  9c17f92db p2p: Fix off-by-one error in fetching address loop
256:  ceb9d2127 ! 6:  005a46dc3 p2p: Try to connect to anchors once
26    @@ src/net.cpp
27
28     +#include <algorithm>
29     +#include <cmath>
30    + #include <cstdint>
31      #include <unordered_map>
32     -
33     -#include <math.h>
34    @@ src/net.cpp: void CConnman::ThreadOpenConnections(const std::vector<std::string>
35      }
36
37      ## src/net_processing.cpp ##
38    -@@ src/net_processing.cpp: bool ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRec
39    +@@ src/net_processing.cpp: void ProcessMessage(
40                            pfrom.GetId(), (fLogIPs ? strprintf(", peeraddr=%s", pfrom.addr.ToString()) : ""),
41                            pfrom.m_tx_relay == nullptr ? "block-relay" : "full-relay");
42                  if (!pfrom.m_tx_relay) {

Summary:

This PR is an anti-eclipse attack measure for the scenario when an attacker exploits a victim node restart to force it to connect to new, probably adversarial, peers. Other discussion surrounding this issue can be found on #17326. This patch helps a node attempt to restore pre-restart outbound block-relay-only connections by dumping the (currently 2) outbound block-relay-only connections to an anchors.dat file and connecting to those addresses on start-up.

laanwj noted:

…reconnecting to peers ‘aggressively’, was, up to now, the reserve of spy nodes and mass-connectors and such… Historically this has been one of the ways used to detect these.

gmaxwell responded:

It would be really good to limit it so that an honest node won’t attempt to connect twice to the same peer through this mechanism for some prudent amount of time… like two minutes." But that “I don’t think it would cause any harm to the intent of this mechanism to not bring them up as fast as possible, and it would avoid tripping up detection of aggressive mass connectors.

gmaxwell and theBlueMatt commented (on #17326) that we shouldn’t do this with all our block-relay-only connections because it would guarantee capture persistence. sdaftuar noted that since we only have two of those connections at the moment, perhaps it’s fine for now.

luke-jr wondered:

Couldn’t there be a risk here, that if one of the anchor peers is malicious and knows a DoS vulnerability, this ensures they can keep repeatedly crashing your node, even if you setup a watchdog to restart it?

To that, gmaxwell suggested that one might forget the anchors if you uncleanly exit, which luke-jr supported.

At that point (Feb 2020), the author closed the PR because:

…forgetting the peers nullifies the initial idea by making anchoring usable only for clean (user initiated) restarts.

gmaxwell however encouraged the PR to be re-opened:

not saving state across crashes in no way nullifies the advantage of preserving connections, and it’s also a natural implementation– e.g. the software will fail to write out the latest peers.dat across a crash.

Since then others made arguments for reconnecting to anchor connections after unclean restarts:

The risk is that a malicious anchor could exploit a DoS vulnerability (which is possibly more likely than executing an eclipse attack) and prevent the victim from being able to sync their node. However, if the node is DoS-attackable, it should be fixed immediately regardless. Something that prevents using it as a normal node doesn’t seem like a huge downside. However, the upside of persisting anchors across crashes is the extra protection against certain types of eclipse attacks. (adapted from amitiuttarwar)

Additionally, it’s very difficult to imagine a DoS vulnerability opened only to outbound blocks-only connections. If there is a broad DoS issue, an attacker will be able to shut down all the reachable nodes with ease because in-bound connections are free. If a vulnerability only applies to outbound connections, then it’s also easy: an attacker spawns 2,000 reachable sybil nodes, and thus has a high chance of becoming a regular outbound connection (not just a block-relay-only). The specific risk of block-relay-only outbound links being able to crash a node while no other connection can, should not prevent us from getting benefits of anchor-persistence after crashing. (If we are worried, we can try counting restarts). The anti-eclipse benefits of reconnecting after an unclean exit are quite valuable since the restart-to-attempt-eclipse scenario seems much more feasible and dangerous then just restart-node-repetitively scenario. (adapted from naumenkogs)

In its current state, the anchors are persisted and reconnected to on unclean shutdowns. (h/t amitiuttarwar and hebasto for clarifying)

In sum, there appears to be broad support for the concept of outbound block-relay-only anchor connections. However, opinions differ on the approach around whether connections are persisted and connected to after unclean restarts. ajtowns, amitiuttarwar, naumenkogs, mzumsande support keeping the connections while luke-jr and gmaxwell are in favor of forgetting them.

Additionally, there were (now invalided) ACKs from fjahr, jamesob, pinheadmz and an utACK from naumenkogs.

On the eclipses scenario mitigated, as a first step, it assumes the attacker to perform a addrman takover thus controlling the set of futures peers that the victim node will be able to connect to. This takeover may be either short-lived/long-lived or even persistent at whim of attacker, depending on the theoritical addr-relay vulnerability leveraged. To effectively activate the eclipse, an attacker needs to benefit from a favorable event, a) a restart or b) a full rotation of your outbounds (stale tip, lagging chain, eviction logic of connected peers, …). In case of a), independently of the restart source (spontaneous or attacker triggered), anchors connections will prevent your attacker to win control of your network view, and so are worthy on this ground.

On the peer rotation cases, especially leverage of the eviction logic, I think it would bypass this new counter-measure. The connection starvation tactic could be used to hijack your inbound slots towards your selected anchors peers and thus forcing you to initiate new connections to malicious peers. But a) it’s likely to be harder after #17428, as those anchors are going to be protected as block-relay peers and b) whereas lower bound of restart-eclipse presume for an attacker only injecting p2p messages and leveraging a natural event, starvation requires from it to actively occupy network slots and game on eviction criterias. Really when we analyze eclipse attacks, we should have a hierarchy of them classified with regards to resources they require from the attacker to successfully perform them. The fact that a stronger attack A can easily bypass counter-measure for attack B doesn’t invalidate worthiness of counter-measure B. At the end of the day, any of our actual eclipse counter-measure is likely to perfom poorly with regards to active infrastructure attacks.

On the DoS issue, I don’t think that granting an anchor privilege to our outbound-block-relay-only give any new DoS privilege to them. The set of messages we talk with them is restrained, thus theorically reducing the attack surface we expose to them. At the limit, we may favor them to upgrade them as compact blocks peers, and they may opportunity to exploit compact code. But that’s only exception I can see ? Not persisting anchors through unclear restart will actually reduce effectiveness of counter-measure, and I think that silent eclipsing might be more worrying than noisy DoS. A crash is likely to be notified more or less quickly by the operator and should halt any higher application logic whereas keeping operating on a attacker-controlled view of the network can be even more harmful funds-loss wise.

On the network staticness increase, I think gmaxwell made the point in the other thread that persitence may favor physically-near peers on the long term as the far from us one as more likely to be dropped from they anchor privilege due to network/link layer aleas. That way slightly degrading network robustness with regards to network-partition geographically based. It should be balanced with the fact this anchor, physically-near peers will also known natural restart or other event, likely reshuflling the cards to anyone. This should be something interesting to monitor by coupling anchor rotations with ASN infos provided by ASMAP. And if there is any meaningful geographical bias introduced.

That said a concern I may still have with proposed implementation is with regards to hindering non-observability of block-relay-only. By persisting connections across restarts, given we already protect this connections type from stale tip/lagging logic and the fact block-relay connections are going to be even more stable after #17428, you might improve a potential oracle to guess their presence. An attacker with connection-starvation/tx-relay capabilities could a) map your full-relay outbound with some conflicts b) evict those outbound from your connected peers by leveraging eviction c) observe you never pick new outbounds in some netgroup because this one is already occupied by your anchors.

I would be more confident if we either a) restrain anchors to one of our outbound-block-relay peers or b) add 2 news anchors peers outbound-block-relay-only. We should try to keep the non-observability of our block-relay-only peers as good we can, at least for a subset of them.

An attacker with connection-starvation/tx-relay capabilities could a) map your full-relay outbound with some conflicts b) evict those outbound from your connected peers by leveraging eviction c) observe you never pick new outbounds in some netgroup because this one is already occupied by your anchors.

It feels to be a way too advanced threat model (especially since (c) should be really hard) to deal with it right now. We can address it later once we’re done with more basic issues like those discussed above :)

Btw, another solution to (c)-like issues might be to choose the tx-relay connections (semi-?)independently from block-relay-only connections (in terms of netgroups/asmap).

Updated d7482556c98b76e73f64d04400a703fdbb9c189f -> 4eff692eeb861018f1f44b6284891a1d0599dd88 (pr17428.18 -> pr17428.20):

• rebased due to the conflicts with #19704 and #19724
• addressed @jnewbery’s comment:

I don’t think we should copy bad patterns just because they already exist in the codebase. Options is used to pass parameters into CConnman’s initialize function, but CConnman already has everything it needs to calculate the max block relay anchor connections (the MAX_BLOCK_RELAY_ONLY_ANCHORS constant is defined in net, and m_max_outbound_block_relay is already passed in).

Adding this here is essentially placing logic for how to construct CConnman inside init.

• addressed @jnewbery’s comment:

This logic shouldn’t be in net_processing. It’s all bound up in connection-layer logic rather than application-layer logic. There’s nothing here that’s specific to the pnode that we’re in the context of in this ProcessMessages() call. I think it makes sense for this to be in CConnman rather than here.

Updated 4eff692eeb861018f1f44b6284891a1d0599dd88 -> 922c65cfd9ea31b00fdc76514dbdd68129169467 (pr17428.20 -> pr17428.21, diff):

• addressed @jnewbery’s comments

Updated 922c65cfd9ea31b00fdc76514dbdd68129169467 -> 06c96f27e70a7d397b5980497dfe615a1f57aeb4 (pr17428.21 -> pr17428.22, diff).

Addressed more @jnewbery’s comments:

• #17428 (review):

I think we have to assume that our data directory is not compromised. Otherwise anything could happen.

• #17428 (review):

oh, I didn’t realise that these are unconditional logs. I definitely think they should be in the NET category.

I disagree with the decision to call DumpAnchors on every block-relay-only connection. I largely agree with the reasoning given by @luke-jr and @gmaxwell; if the software is not working correctly and we have an unclean shutdown, it’s better to reset the state by throwing away old connections than it is to just resume our old connections.

Obviously if there’s a bug in our code that can lead to a crash, that’s something we’d want to fix ASAP. But if you’re a node operator, being able to reset state in the event of the unforeseen in order to mitigate a problem while you implement a better fix (like upgrading your code, if a fix is available) just seems like a common sense behavior to have. This is still strictly better from an eclipse-attack-prevention standpoint than what we have today, and in some circumstances may provide better usability in the event of an unknown future problem than what is proposed here.

I also disagree with having an interaction between -addnode and anchors, which I commented on separately.

If we resolve those two issues I’d be on board with the rest of the behavior proposed here (though I still plan to test).

@sdaftuar

Thank you for reviewing.

But if you’re a node operator, being able to reset state in the event of the unforeseen in order to mitigate a problem while you implement a better fix (like upgrading your code, if a fix is available) just seems like a common sense behavior to have.

A node operator could just remove anchors.dat file to mitigate a problem. If such behavior could not be considered as a remedy, please consider the following.

Another approach is to introduce a configure option, say -keepanchors, with the default value -keepanchors=clean_shutdown_only, and a node operator could opt in -keepanchors=always.

UPDATE: This suggestion could be leaved for follow ups.

Updated 06c96f27e70a7d397b5980497dfe615a1f57aeb4 -> 21d9d4e29ac09cb74afa24bed0509a928210d974 (pr17428.22 -> pr17428.23, diff).

Addressed @sdaftuar’s comments:

I disagree with the decision to call DumpAnchors on every block-relay-only connection. I largely agree with the reasoning given by @luke-jr and @gmaxwell; if the software is not working correctly and we have an unclean shutdown, it’s better to reset the state by throwing away old connections than it is to just resume our old connections.

I also disagree with having an interaction between -addnode and anchors, which I commented on separately.

Also other @sdaftuar’s and @jnewbery’s comments are addressed.

When feature freeze for 0.21.0 is around the coner the non-controversial and the safest approach seems the most reasonable. My suggestion could be considered later.

Updated 21d9d4e29ac09cb74afa24bed0509a928210d974 -> be82629cc00961325182f03493d84f704b709020 (pr17428.23 -> pr17428.24, diff):

• added anchors.dat to files.md

Updated be82629cc00961325182f03493d84f704b709020 -> 24129a6b1966d71c7bd77ea029f17fb7796e2bc2 (pr17428.24 -> pr17428.25, diff):

• addressed all recent @jnewbery’s comments

Updated 24129a6b1966d71c7bd77ea029f17fb7796e2bc2 -> 68870383c35922d3a5aad1cedd9df002568995a0 (pr17428.25 -> pr17428.27, diff):

• improved the comment as suggested by @jnewbery:

I’d suggest “Addresses that were saved during the previous shutdown. We’ll attempt to make block-relay-only connections to them.”

• addressed @jnewbery’s comment:

I wonder if the call to ReadAnchors() should be within this if block. If ReadAnchors() is outside the block, then when the node is started with -noconnect, not only will the node not connect to the anchors, it’ll also throw away the file, so that when it’s restarted without -noconnect, it won’t try to reconnect to the old anchors. Is that what we want?

I’ve tested the following manually:

• start bitcoind, leave it long enough to make block-relay-only connections, stop, manually inspect anchors.dat, restart and observe that bitcoind connects to the same block-relay-only connections :heavy_check_mark:
• corrupt anchors.dat file and make sure that it’s ignored, and other addresses are chosen for block-relay-only connections :heavy_check_mark:
• start bitcoind with -noconnect -> anchors.dat file doesn’t get read, but gets overwritten with a new file containing no connections on shutdown. I think we might want to change this behaviour to ensure anchors aren’t thrown away unnecessarily (although this scenario seems quite unlikely)

I think this is ready for merge now, and smaller improvements can be added later. It’ll also be great to add regression tests once #19315 has been merged.

Tested ACK 68870383c3

Updated 68870383c35922d3a5aad1cedd9df002568995a0 -> 27113fc53d0537393fc6c61a8a3dea0eced7f588 (pr17428.27 -> pr17428.28, diff):

• rebased due to the conflict with #19697

• addressed @jnewbery’s comment:

start bitcoind with -noconnect -> anchors.dat file doesn’t get read, but gets overwritten with a new file containing no connections on shutdown. I think we might want to change this behaviour to ensure anchors aren’t thrown away unnecessarily (although this scenario seems quite unlikely)

I think this and this are worthy to fix to avoid future code changes silently breaking peer diversity as post-anchor the enforcement of distinct outbound network groups isn’t that straightforward, IMO.

Also, it may introduce a slight fingerprint which could be worthy to solve as a follow-up.

Otherwise, review leftovers are just nice-to-fix.

Updated 27113fc53d0537393fc6c61a8a3dea0eced7f588 -> b397e49b0c390970713b4387e6dd254b926c70d0 (pr17428.28 -> pr17428.30, diff):

• fixed bug when an anchor does not pass all checks

• addressed @ariard’s comments