implements ping-pong packets between nodes #12794

behzadnouri · 2020-10-10T17:47:09Z

Problem

It’s possible to use UDP gossip protocol to amplify DDoS attacks. An attacker can spoof IP address in UDP packet when sending PullRequest to the node. There's no any validation if provided source IP address is not spoofed and the node can send much larger PullResponse to victim's IP. As I checked, PullRequest is about 290 bytes, while PullResponse is about 10 kB. It means that amplification is about 34x. This way an attacker can easily perform DDoS attack both on Solana node and third-party server.

To prevent it, need for example to implement ping-pong mechanism similar as in Ethereum: Before accepting requests from remote client needs to validate his IP. Local node sends Ping packet to the remote node and it needs to reply with Pong packet that contains hash of matching Ping packet. Content of Ping packet is unpredictable. If hash from Pong packet matches, local node can remember IP where Ping packet was sent as correct and allow further communication.

More info:
https://github.com/ethereum/devp2p/blob/master/discv4.md#endpoint-proof
https://github.com/ethereum/devp2p/blob/master/discv4.md#wire-protocol

Summary of Changes

The commit adds a PingCache, which maintains records of remote nodes
which have returned a valid response to a ping message, and on-the-fly
ping messages pending a pong response from the remote node.

When handling pull-requests, those from addresses which have not passed
the ping-pong check are filtered out, and additionally ping packets are
added for addresses which need to be (re)verified.

codecov · 2020-10-10T19:33:20Z

Codecov Report

Merging #12794 into master will increase coverage by 0.0%.
The diff coverage is 86.7%.

@@           Coverage Diff            @@
##           master   #12794    +/-   ##
========================================
  Coverage    82.1%    82.1%            
========================================
  Files         372      373     +1     
  Lines       86562    86904   +342     
========================================
+ Hits        71109    71405   +296     
- Misses      15453    15499    +46

behzadnouri · 2020-10-10T19:35:15Z

@sakridge @carllin The code is complete except for tests. I wanted to get the initial review before adding more tests.

also @mvines since this is an abi change.

aeyakovenko · 2020-10-10T20:27:26Z

Can you expand the problem? the hacker one links are behind a login. I'd like to have the details in the GitHub PR

behzadnouri · 2020-10-10T20:48:56Z

Can you expand the problem? the hacker one links are behind a login. I'd like to have the details in the GitHub PR

sure, done

aeyakovenko · 2020-10-10T21:33:22Z

@behzadnouri doesn't the PullRequest/PullResponse have the same effect? Do we need another set of messages, or can we infer the same information form the pulls?

behzadnouri · 2020-10-11T12:43:14Z

@behzadnouri doesn't the PullRequest/PullResponse have the same effect? Do we need another set of messages, or can we infer the same information form the pulls?

I am not sure we can. An attacker can keep sending fake pull/push traffic, without an indication that it is spoofing an ip address. With ping/pong packets, the pong package can be verified as soon as it is received.

leoluk · 2020-10-11T14:34:40Z

That Hackerone report is a duplicate of #9492.

Glad to see it's getting fixed!

aeyakovenko · 2020-10-11T17:24:57Z

Yea. You age right. We need a signed cookie to be sent back to authenticate the address.

sakridge · 2020-10-12T20:15:01Z

Looks like a good direction to me, just need to figure out the backwards-compatibility part.

core/src/ping_pong.rs

core/src/cluster_info.rs

carllin · 2020-10-12T21:48:05Z

core/src/cluster_info.rs

+        // Because pull-responses are sent back to packet.meta.addr() of
+        // incoming pull-requests, pings are also sent to request.from_addr (as
+        // opposed to caller.gossip address).
+        move |request| {


cool, currying!

carllin · 2020-10-12T21:54:05Z

core/src/cluster_info.rs

@@ -95,6 +96,9 @@ const MAX_GOSSIP_TRAFFIC: usize = 128_000_000 / PACKET_DATA_SIZE;

 /// Keep the number of snapshot hashes a node publishes under MAX_PROTOCOL_PAYLOAD_SIZE
 pub const MAX_SNAPSHOT_HASHES: usize = 16;
+const GOSSIP_PING_TOKEN_SIZE: usize = 32;
+const GOSSIP_PING_CACHE_CAPACITY: usize = 16384;


Should this be based on the number of peers in gossip instead of a constant?

It should just be large enough not to kick out ping/pong packets too early. That includes scenarios where there may be spurious pull-requests from attackers. There might also be multiple packets associated with the same peer (e.g. multiple on-the-fly ping packets, or packets which were dropped).

Is there an easy/efficient way to get the number of peers here? may dynamically adjust it to account for larger network.

@behzadnouri, got it, thanks! I think the most convenient available way would be to use cluster_info.gossip_peers(). It's not the most efficient because it scans the table (broadcast does something similar with cluster_info.tvu_peers()) but may not be too bad if done periodically.

thanks, since the alternatives would require table scan and locking, i lean toward keeping this constant for now, and later test alternatives.

core/src/ping_pong.rs

carllin · 2020-10-12T22:19:31Z

@behzadnouri looks good! Maybe we can use the pongs to do the upgrade? I.e. once a peer responds to a pong message, then we can mark that peer as upgraded and use the hard verify path in check_pull_request for that peer

mvines · 2020-10-13T05:54:42Z

core/src/cluster_info.rs

+            // TODO: For backward compatibility, this unconditionally returns
+            // true for now. It has to return _check, once nodes start
+            // responding to ping messages.
+            true


We can plumb in the final code here as well, making it conditional on a new feature added to runtime/src/feature_set.rs. This way we can dynamically activate this feature on clusters once 95% of the validator fleet has upgraded. Right now FeatureSet is not plumbed though cluster_info though, so I suggest adding this plumbing first as a forerunner PR to this one. core/ should probably use the FeatureSet from the highest rooted bank.

sure, working on it.

updated the diff to use the feature-set added in #12878
I think in addition of 95% fleet upgrade we also need to ensure that the newly added metric pull_request_ping_pong_check_failed_count looks fine.

mvines · 2020-10-13T05:58:05Z

metrics/src/counter.rs

@@ -76,7 +76,7 @@ macro_rules! inc_counter_info {
 #[macro_export]
 macro_rules! inc_new_counter {
    ($name:expr, $count:expr, $level:expr, $lograte:expr, $metricsrate:expr) => {{
-        if log_enabled!($level) {
+        if $count != 0 && log_enabled!($level) {


nit: separate out little changes like this into their own PRs. They can land independently, and sooner

I actually reverted this change and instead put the if ... != 0 at the call-site.
Though the counter value does not change when event count is zero, some of the logging logic in the implementation relies on how often that function is called regardless of events counts being zero or not:
https://github.com/solana-labs/solana/blob/f5ed017d6/metrics/src/counter.rs#L172-L203
It might be somewhere in the code relies on that behavior.

core/src/cluster_info.rs

This reverts commit bae6195.

core/src/ping_pong.rs

sakridge · 2020-10-26T22:55:17Z

core/src/cluster_info.rs

@@ -97,6 +98,10 @@ const MAX_GOSSIP_TRAFFIC: usize = 128_000_000 / PACKET_DATA_SIZE;

 /// Keep the number of snapshot hashes a node publishes under MAX_PROTOCOL_PAYLOAD_SIZE
 pub const MAX_SNAPSHOT_HASHES: usize = 16;
+/// Number of bytes in the randomly generated token sent with ping messages.
+const GOSSIP_PING_TOKEN_SIZE: usize = 32;


Just 32-bytes seems like overkill. What about just u64 which seems like enough and allows for better memory efficiency and easy atomic use if needed, less cycles for compare/randomization generation etc.

I am worried that will compromise security in some way, and limit usability of these tokens in the future for other security related applications beyond the current one (i.e. ip-spoofing prevention). For example responding with sha256 hash of ping token which itself was only 64 bits is kind of degenerate.

These ping/pong packets will be super low traffic, since once received they will be cached for a while. So I doubt that this will cause any noticeable performance difference.

sakridge

lgtm

Pull request has been modified.

https://hackerone.com/reports/991106 > It’s possible to use UDP gossip protocol to amplify DDoS attacks. An attacker > can spoof IP address in UDP packet when sending PullRequest to the node. > There's no any validation if provided source IP address is not spoofed and > the node can send much larger PullResponse to victim's IP. As I checked, > PullRequest is about 290 bytes, while PullResponse is about 10 kB. It means > that amplification is about 34x. This way an attacker can easily perform DDoS > attack both on Solana node and third-party server. > > To prevent it, need for example to implement ping-pong mechanism similar as > in Ethereum: Before accepting requests from remote client needs to validate > his IP. Local node sends Ping packet to the remote node and it needs to reply > with Pong packet that contains hash of matching Ping packet. Content of Ping > packet is unpredictable. If hash from Pong packet matches, local node can > remember IP where Ping packet was sent as correct and allow further > communication. > > More info: > https://github.com/ethereum/devp2p/blob/master/discv4.md#endpoint-proof > https://github.com/ethereum/devp2p/blob/master/discv4.md#wire-protocol The commit adds a PingCache, which maintains records of remote nodes which have returned a valid response to a ping message, and on-the-fly ping messages pending a pong response from the remote node. When handling pull-requests, those from addresses which have not passed the ping-pong check are filtered out, and additionally ping packets are added for addresses which need to be (re)verified. (cherry picked from commit ae91270)

https://hackerone.com/reports/991106 > It’s possible to use UDP gossip protocol to amplify DDoS attacks. An attacker > can spoof IP address in UDP packet when sending PullRequest to the node. > There's no any validation if provided source IP address is not spoofed and > the node can send much larger PullResponse to victim's IP. As I checked, > PullRequest is about 290 bytes, while PullResponse is about 10 kB. It means > that amplification is about 34x. This way an attacker can easily perform DDoS > attack both on Solana node and third-party server. > > To prevent it, need for example to implement ping-pong mechanism similar as > in Ethereum: Before accepting requests from remote client needs to validate > his IP. Local node sends Ping packet to the remote node and it needs to reply > with Pong packet that contains hash of matching Ping packet. Content of Ping > packet is unpredictable. If hash from Pong packet matches, local node can > remember IP where Ping packet was sent as correct and allow further > communication. > > More info: > https://github.com/ethereum/devp2p/blob/master/discv4.md#endpoint-proof > https://github.com/ethereum/devp2p/blob/master/discv4.md#wire-protocol The commit adds a PingCache, which maintains records of remote nodes which have returned a valid response to a ping message, and on-the-fly ping messages pending a pong response from the remote node. When handling pull-requests, those from addresses which have not passed the ping-pong check are filtered out, and additionally ping packets are added for addresses which need to be (re)verified. (cherry picked from commit ae91270) Co-authored-by: behzad nouri <behzadnouri@gmail.com>

https://hackerone.com/reports/991106 > It’s possible to use UDP gossip protocol to amplify DDoS attacks. An attacker > can spoof IP address in UDP packet when sending PullRequest to the node. > There's no any validation if provided source IP address is not spoofed and > the node can send much larger PullResponse to victim's IP. As I checked, > PullRequest is about 290 bytes, while PullResponse is about 10 kB. It means > that amplification is about 34x. This way an attacker can easily perform DDoS > attack both on Solana node and third-party server. > > To prevent it, need for example to implement ping-pong mechanism similar as > in Ethereum: Before accepting requests from remote client needs to validate > his IP. Local node sends Ping packet to the remote node and it needs to reply > with Pong packet that contains hash of matching Ping packet. Content of Ping > packet is unpredictable. If hash from Pong packet matches, local node can > remember IP where Ping packet was sent as correct and allow further > communication. > > More info: > https://github.com/ethereum/devp2p/blob/master/discv4.md#endpoint-proof > https://github.com/ethereum/devp2p/blob/master/discv4.md#wire-protocol The commit adds a PingCache, which maintains records of remote nodes which have returned a valid response to a ping message, and on-the-fly ping messages pending a pong response from the remote node. When handling pull-requests, those from addresses which have not passed the ping-pong check are filtered out, and additionally ping packets are added for addresses which need to be (re)verified. (cherry picked from commit ae91270) # Conflicts: # Cargo.lock # core/src/cluster_info.rs

* implements ping-pong packets between nodes (#12794) https://hackerone.com/reports/991106 > It’s possible to use UDP gossip protocol to amplify DDoS attacks. An attacker > can spoof IP address in UDP packet when sending PullRequest to the node. > There's no any validation if provided source IP address is not spoofed and > the node can send much larger PullResponse to victim's IP. As I checked, > PullRequest is about 290 bytes, while PullResponse is about 10 kB. It means > that amplification is about 34x. This way an attacker can easily perform DDoS > attack both on Solana node and third-party server. > > To prevent it, need for example to implement ping-pong mechanism similar as > in Ethereum: Before accepting requests from remote client needs to validate > his IP. Local node sends Ping packet to the remote node and it needs to reply > with Pong packet that contains hash of matching Ping packet. Content of Ping > packet is unpredictable. If hash from Pong packet matches, local node can > remember IP where Ping packet was sent as correct and allow further > communication. > > More info: > https://github.com/ethereum/devp2p/blob/master/discv4.md#endpoint-proof > https://github.com/ethereum/devp2p/blob/master/discv4.md#wire-protocol The commit adds a PingCache, which maintains records of remote nodes which have returned a valid response to a ping message, and on-the-fly ping messages pending a pong response from the remote node. When handling pull-requests, those from addresses which have not passed the ping-pong check are filtered out, and additionally ping packets are added for addresses which need to be (re)verified. (cherry picked from commit ae91270) # Conflicts: # Cargo.lock # core/src/cluster_info.rs * resolves mergify merge conflicts Co-authored-by: behzad nouri <behzadnouri@gmail.com>

behzadnouri marked this pull request as ready for review October 10, 2020 19:33

behzadnouri requested review from sakridge, carllin and mvines October 10, 2020 19:33

behzadnouri changed the title ~~implements ping-pong mechanism between nodes~~ implements ping-pong packets between nodes Oct 10, 2020

behzadnouri force-pushed the ping-pong branch from d540841 to 33687f1 Compare October 11, 2020 14:27

behzadnouri force-pushed the ping-pong branch 3 times, most recently from 8044b38 to 8772b4e Compare October 12, 2020 19:43

behzadnouri force-pushed the ping-pong branch from 8772b4e to d15675a Compare October 12, 2020 20:15

carllin reviewed Oct 12, 2020

View reviewed changes

core/src/ping_pong.rs Outdated Show resolved Hide resolved

behzadnouri force-pushed the ping-pong branch from d15675a to 4f3ed4a Compare October 12, 2020 21:12

carllin reviewed Oct 12, 2020

View reviewed changes

core/src/cluster_info.rs Show resolved Hide resolved

carllin reviewed Oct 12, 2020

View reviewed changes

core/src/ping_pong.rs Outdated Show resolved Hide resolved

mvines reviewed Oct 13, 2020

View reviewed changes

core/src/cluster_info.rs Show resolved Hide resolved

behzadnouri force-pushed the ping-pong branch from 4f3ed4a to 2d21ee3 Compare October 13, 2020 16:10

behzadnouri added 8 commits October 26, 2020 10:35

reverts change to metrics/src/counter.rs

8960e5c

adds a stats counter for pull requests which fail ping/pong check

7a634e0

minor renames

4a16435

adds more tests

31be432

enables ping/pong check using feature-set

2010d47

verifies packets outside the lock

2e45ff6

Revert "verifies packets outside the lock"

23a42c2

This reverts commit bae6195.

removes signature verification since it is already done

4f1791c

behzadnouri force-pushed the ping-pong branch from ace958b to 4f1791c Compare October 26, 2020 14:37

behzadnouri requested review from mvines and carllin October 26, 2020 17:13

sakridge reviewed Oct 26, 2020

View reviewed changes

core/src/ping_pong.rs Outdated Show resolved Hide resolved

sakridge reviewed Oct 26, 2020

View reviewed changes

sakridge previously approved these changes Oct 27, 2020

View reviewed changes

removes Clone impl for PingCache

7b35654

behzadnouri merged commit ae91270 into solana-labs:master Oct 28, 2020

behzadnouri deleted the ping-pong branch October 28, 2020 17:03

behzadnouri added the v1.4 label Oct 28, 2020

mergify bot mentioned this pull request Oct 28, 2020

implements ping-pong packets between nodes (bp #12794) #13234

Merged

behzadnouri added v1.3 and removed v1.3 labels Oct 29, 2020

mergify bot mentioned this pull request Oct 29, 2020

implements ping-pong packets between nodes (bp #12794) #13281

Merged

behzadnouri mentioned this pull request Nov 19, 2020

[RfC] Node-to-node network latency measurement #10084

Open

aeyakovenko mentioned this pull request Nov 23, 2020

Compromising all validators of the Solana network #13757

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implements ping-pong packets between nodes #12794

implements ping-pong packets between nodes #12794

behzadnouri commented Oct 10, 2020 •

edited

Loading

codecov bot commented Oct 10, 2020 •

edited

Loading

behzadnouri commented Oct 10, 2020

aeyakovenko commented Oct 10, 2020

behzadnouri commented Oct 10, 2020

aeyakovenko commented Oct 10, 2020

behzadnouri commented Oct 11, 2020

leoluk commented Oct 11, 2020

aeyakovenko commented Oct 11, 2020

sakridge commented Oct 12, 2020

carllin Oct 12, 2020 •

edited

Loading

carllin Oct 12, 2020 •

edited

Loading

behzadnouri Oct 13, 2020

carllin Oct 13, 2020

behzadnouri Oct 17, 2020

carllin commented Oct 12, 2020

mvines Oct 13, 2020

behzadnouri Oct 13, 2020

behzadnouri Oct 17, 2020 •

edited

Loading

mvines Oct 13, 2020

behzadnouri Oct 13, 2020

sakridge Oct 26, 2020

behzadnouri Oct 27, 2020

sakridge left a comment

implements ping-pong packets between nodes #12794

implements ping-pong packets between nodes #12794

Conversation

behzadnouri commented Oct 10, 2020 • edited Loading

Problem

Summary of Changes

codecov bot commented Oct 10, 2020 • edited Loading

Codecov Report

behzadnouri commented Oct 10, 2020

aeyakovenko commented Oct 10, 2020

behzadnouri commented Oct 10, 2020

aeyakovenko commented Oct 10, 2020

behzadnouri commented Oct 11, 2020

leoluk commented Oct 11, 2020

aeyakovenko commented Oct 11, 2020

sakridge commented Oct 12, 2020

carllin Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

carllin Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carllin commented Oct 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behzadnouri Oct 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakridge left a comment

Choose a reason for hiding this comment

behzadnouri commented Oct 10, 2020 •

edited

Loading

codecov bot commented Oct 10, 2020 •

edited

Loading

carllin Oct 12, 2020 •

edited

Loading

carllin Oct 12, 2020 •

edited

Loading

behzadnouri Oct 17, 2020 •

edited

Loading