diff --git a/bin/fuzz/fuzz_targets/network-connection-encrypted.rs b/bin/fuzz/fuzz_targets/network-connection-encrypted.rs index 62024bd505..2f673e500e 100644 --- a/bin/fuzz/fuzz_targets/network-connection-encrypted.rs +++ b/bin/fuzz/fuzz_targets/network-connection-encrypted.rs @@ -141,6 +141,7 @@ libfuzzer_sys::fuzz_target!(|data: &[u8]| { handshake::Handshake::Success { connection, .. } => connection .into_connection::<_, (), ()>(Config { first_out_ping: Duration::new(60, 0), + max_inbound_substreams: 10, notifications_protocols: Vec::new(), request_protocols: vec![ConfigRequestResponse { inbound_allowed: true, diff --git a/bin/fuzz/fuzz_targets/network-connection-raw.rs b/bin/fuzz/fuzz_targets/network-connection-raw.rs index c74dbdb75f..c33e0bfb1c 100644 --- a/bin/fuzz/fuzz_targets/network-connection-raw.rs +++ b/bin/fuzz/fuzz_targets/network-connection-raw.rs @@ -34,6 +34,7 @@ libfuzzer_sys::fuzz_target!(|data: &[u8]| { smoldot::libp2p::collection::Network::new(smoldot::libp2p::collection::Config { randomness_seed: [0; 32], capacity: 0, + max_inbound_substreams: 10, notification_protocols: Vec::new(), request_response_protocols: Vec::new(), // This timeout doesn't matter as we pass dummy time values. diff --git a/bin/wasm-node/CHANGELOG.md b/bin/wasm-node/CHANGELOG.md index 6cff939c36..d77e4ed031 100644 --- a/bin/wasm-node/CHANGELOG.md +++ b/bin/wasm-node/CHANGELOG.md @@ -6,6 +6,10 @@ - Add support for the `system_nodeRoles` JSON-RPC method. ([#2725](https://github.com/paritytech/smoldot/pull/2725)) +### Changed + +- A limit to the number of substreams a remote can maintain open over a connection is now enforced. ([#2724](https://github.com/paritytech/smoldot/pull/2724)) + ## 0.6.32 - 2022-09-07 ### Fixed diff --git a/src/libp2p/collection.rs b/src/libp2p/collection.rs index d7658ace4a..5cf1163ec1 100644 --- a/src/libp2p/collection.rs +++ b/src/libp2p/collection.rs @@ -81,6 +81,12 @@ pub struct Config { /// Number of connections containers should initially allocate for. pub capacity: usize, + /// Maximum number of substreams that each remote can have simultaneously opened. + /// + /// > **Note**: This limit is necessary in order to avoid DoS attacks where a remote opens too + /// > many substreams. + pub max_inbound_substreams: usize, + pub notification_protocols: Vec, pub request_response_protocols: Vec, @@ -217,6 +223,9 @@ pub struct Network { /// Generator for randomness seeds given to the established connections. randomness_seeds: ChaCha20Rng, + /// See [`Config::max_inbound_substreams`]. + max_inbound_substreams: usize, + /// See [`Config::handshake_timeout`]. handshake_timeout: Duration, @@ -320,6 +329,7 @@ where ingoing_notification_substreams_by_connection: BTreeMap::new(), randomness_seeds: ChaCha20Rng::from_seed(config.randomness_seed), noise_key: Arc::new(config.noise_key), + max_inbound_substreams: config.max_inbound_substreams, notification_protocols, request_response_protocols: config.request_response_protocols.into_iter().collect(), // TODO: stupid overhead ping_protocol: config.ping_protocol.into(), @@ -347,6 +357,7 @@ where is_initiator, when_connected + self.handshake_timeout, self.noise_key.clone(), + self.max_inbound_substreams, self.notification_protocols.clone(), self.request_response_protocols.clone(), self.ping_protocol.clone(), @@ -382,6 +393,7 @@ where let connection_task = MultiStreamConnectionTask::new( self.randomness_seeds.gen(), now, + self.max_inbound_substreams, self.notification_protocols.clone(), self.request_response_protocols.clone(), self.ping_protocol.clone(), diff --git a/src/libp2p/collection/multi_stream.rs b/src/libp2p/collection/multi_stream.rs index b899d127b1..332649e0ee 100644 --- a/src/libp2p/collection/multi_stream.rs +++ b/src/libp2p/collection/multi_stream.rs @@ -92,6 +92,7 @@ where pub(super) fn new( randomness_seed: [u8; 32], now: TNow, + max_inbound_substreams: usize, notification_protocols: Arc<[OverlayNetwork]>, request_response_protocols: Arc<[ConfigRequestResponse]>, ping_protocol: Arc, @@ -108,6 +109,7 @@ where }) .collect(), request_protocols: request_response_protocols.to_vec(), // TODO: overhead + max_inbound_substreams, randomness_seed, ping_protocol: ping_protocol.to_string(), // TODO: cloning :-/ ping_interval: Duration::from_secs(20), // TODO: hardcoded diff --git a/src/libp2p/collection/single_stream.rs b/src/libp2p/collection/single_stream.rs index e0f0909143..608039d5d9 100644 --- a/src/libp2p/collection/single_stream.rs +++ b/src/libp2p/collection/single_stream.rs @@ -64,6 +64,9 @@ enum SingleStreamConnectionTaskInner { /// See [`super::Config::noise_key`]. noise_key: Arc, + /// See [`super::Config::max_inbound_substreams`]. + max_inbound_substreams: usize, + /// See [`OverlayNetwork`]. notification_protocols: Arc<[OverlayNetwork]>, @@ -130,6 +133,7 @@ where is_initiator: bool, handshake_timeout: TNow, noise_key: Arc, + max_inbound_substreams: usize, notification_protocols: Arc<[OverlayNetwork]>, request_response_protocols: Arc<[ConfigRequestResponse]>, ping_protocol: Arc, @@ -140,6 +144,7 @@ where randomness_seed, timeout: handshake_timeout, noise_key, + max_inbound_substreams, notification_protocols, request_response_protocols, ping_protocol, @@ -627,6 +632,7 @@ where randomness_seed, timeout, noise_key, + max_inbound_substreams, notification_protocols, request_response_protocols, ping_protocol, @@ -690,6 +696,7 @@ where randomness_seed, timeout, noise_key, + max_inbound_substreams, notification_protocols, request_response_protocols, ping_protocol, @@ -718,6 +725,7 @@ where }) .collect(), request_protocols: request_response_protocols.to_vec(), // TODO: overhead + max_inbound_substreams, randomness_seed, ping_protocol: ping_protocol.to_string(), // TODO: cloning :-/ ping_interval: Duration::from_secs(20), // TODO: hardcoded diff --git a/src/libp2p/connection/established.rs b/src/libp2p/connection/established.rs index 8a25966baf..0d5492751e 100644 --- a/src/libp2p/connection/established.rs +++ b/src/libp2p/connection/established.rs @@ -193,6 +193,8 @@ pub enum Event { // TODO: this struct isn't zero-cost, but making it zero-cost is kind of hard and annoying #[derive(Debug, Clone)] pub struct Config { + /// Maximum number of substreams that the remote can have simultaneously opened. + pub max_inbound_substreams: usize, /// List of request-response protocols supported for incoming substreams. pub request_protocols: Vec, /// List of notifications protocols supported for incoming substreams. diff --git a/src/libp2p/connection/established/multi_stream.rs b/src/libp2p/connection/established/multi_stream.rs index 8425db1045..70811092d6 100644 --- a/src/libp2p/connection/established/multi_stream.rs +++ b/src/libp2p/connection/established/multi_stream.rs @@ -81,6 +81,9 @@ pub struct MultiStream { /// unnecessary code being included in the binary and reduces the binary size. ping_payload_randomness: rand_chacha::ChaCha20Rng, + /// See [`Config::max_inbound_substreams`]. + // TODO: not enforced at the moment + _max_inbound_substreams: usize, /// See [`Config::request_protocols`]. request_protocols: Vec, /// See [`Config::notifications_protocols`]. @@ -139,6 +142,7 @@ where ping_substream: None, next_ping: config.first_out_ping, ping_payload_randomness: randomness, + _max_inbound_substreams: config.max_inbound_substreams, request_protocols: config.request_protocols, notifications_protocols: config.notifications_protocols, ping_protocol: config.ping_protocol, diff --git a/src/libp2p/connection/established/single_stream.rs b/src/libp2p/connection/established/single_stream.rs index 0ba4ec2d84..c0e7b5afa8 100644 --- a/src/libp2p/connection/established/single_stream.rs +++ b/src/libp2p/connection/established/single_stream.rs @@ -114,6 +114,8 @@ struct Inner { /// unnecessary code being included in the binary and reduces the binary size. ping_payload_randomness: rand_chacha::ChaCha20Rng, + /// See [`Config::max_inbound_substreams`]. + max_inbound_substreams: usize, /// See [`Config::request_protocols`]. request_protocols: Vec, /// See [`Config::notifications_protocols`]. @@ -308,9 +310,20 @@ where Some(yamux::IncomingDataDetail::IncomingSubstream) => { debug_assert!(!self.inner.yamux.goaway_queued_or_sent()); + self.encryption + .consume_inbound_data(yamux_decode.bytes_read); + // Receive a request from the remote for a new incoming substream. - // These requests are automatically accepted. - // TODO: add a limit to the number of substreams + // These requests are automatically accepted unless the total limit to the + // number of substreams has been reached. + // Note that `num_inbound()` counts substreams that have been closed but not + // yet removed from the state machine. This can affect the actual limit in a + // subtle way. At the time of writing of this comment the limit should be + // properly enforced, however it is not considered problematic if it weren't. + if self.inner.yamux.num_inbound() >= self.inner.max_inbound_substreams { + self.inner.yamux.reject_pending_substream(); + continue; + } let supported_protocols = self .inner @@ -332,8 +345,6 @@ where .accept_pending_substream(Some(substream::Substream::ingoing( supported_protocols, ))); - self.encryption - .consume_inbound_data(yamux_decode.bytes_read); } Some( @@ -1059,6 +1070,7 @@ impl ConnectionPrototype { outgoing_pings, next_ping: config.first_out_ping, ping_payload_randomness: randomness, + max_inbound_substreams: config.max_inbound_substreams, request_protocols: config.request_protocols, notifications_protocols: config.notifications_protocols, ping_protocol: config.ping_protocol, diff --git a/src/libp2p/connection/yamux.rs b/src/libp2p/connection/yamux.rs index 29a10e0a91..cd70f6315d 100644 --- a/src/libp2p/connection/yamux.rs +++ b/src/libp2p/connection/yamux.rs @@ -89,6 +89,9 @@ pub struct Yamux { /// A `SipHasher` is used in order to avoid hash collision attacks on substream IDs. substreams: hashbrown::HashMap, SipHasherBuild>, + /// Number of substreams within [`Yamux::substreams`] whose [`Substream::inbound`] is `true`. + num_inbound: usize, + /// `Some` if a `GoAway` frame has been received in the past. received_goaway: Option, @@ -127,6 +130,8 @@ pub struct Yamux { struct Substream { /// State of the substream. state: SubstreamState, + /// `true` if the substream has been opened by the remote. + inbound: bool, /// Data chosen by the user. user_data: T, } @@ -282,6 +287,7 @@ impl Yamux { config.capacity, SipHasherBuild::new(randomness.gen()), ), + num_inbound: 0, received_goaway: None, incoming: Incoming::Header(arrayvec::ArrayVec::new()), outgoing: Outgoing::Idle, @@ -307,6 +313,23 @@ impl Yamux { self.substreams.is_empty() } + /// Returns the number of substreams in the Yamux state machine. Includes substreams that are + /// dead but haven't been removed yet. + pub fn len(&self) -> usize { + self.substreams.len() + } + + /// Returns the number of inbound substreams in the Yamux state machine. Includes substreams + /// that are dead but haven't been removed yet. + pub fn num_inbound(&self) -> usize { + debug_assert_eq!( + self.num_inbound, + self.substreams.values().filter(|s| s.inbound).count() + ); + + self.num_inbound + } + /// Opens a new substream. /// /// This method only modifies the state of `self` and reserves an identifier. No message needs @@ -380,6 +403,7 @@ impl Yamux { write_buffers: Vec::with_capacity(16), first_write_buffer_offset: 0, }, + inbound: false, user_data, }); @@ -542,6 +566,10 @@ impl Yamux { let substream = self.substreams.remove(&id_to_remove).unwrap(); + if substream.inbound { + self.num_inbound -= 1; + } + Some(( SubstreamId(id_to_remove), if let SubstreamState::Reset = substream.state { @@ -1118,11 +1146,14 @@ impl Yamux { write_buffers: Vec::new(), first_write_buffer_offset: 0, }, + inbound: true, user_data, }, ); debug_assert!(_was_before.is_none()); + self.num_inbound += 1; + self.incoming = if data_frame_size == 0 { Incoming::Header(arrayvec::ArrayVec::new()) } else { diff --git a/src/libp2p/peers.rs b/src/libp2p/peers.rs index 2b40bbe098..d5190e3241 100644 --- a/src/libp2p/peers.rs +++ b/src/libp2p/peers.rs @@ -80,6 +80,16 @@ pub struct Config { /// Capacity to initially reserve to the list of peers. pub peers_capacity: usize, + /// Maximum number of substreams that each remote can have simultaneously opened on each + /// connection. + /// + /// If there exists multiple connections with the same remote, the limit is enforced for + /// each connection separately. + /// + /// > **Note**: This limit is necessary in order to avoid DoS attacks where a remote opens too + /// > many substreams. + pub max_inbound_substreams: usize, + pub notification_protocols: Vec, pub request_response_protocols: Vec, @@ -206,6 +216,7 @@ where inner: collection::Network::new(collection::Config { capacity: config.connections_capacity, noise_key: config.noise_key, + max_inbound_substreams: config.max_inbound_substreams, notification_protocols: config.notification_protocols, request_response_protocols: config.request_response_protocols, ping_protocol: config.ping_protocol, diff --git a/src/network/service.rs b/src/network/service.rs index 71ddd9f0b8..fbdb787fcb 100644 --- a/src/network/service.rs +++ b/src/network/service.rs @@ -379,10 +379,22 @@ where }) .collect::>(); + // Maximum number that each remote is allowed to open. + // Note that this maximum doesn't have to be precise. There only needs to be *a* limit + // that is not exaggerately large, and this limit shouldn't be too low as to cause + // legitimate substreams to be refused. + // According to the protocol, a remote can only open one substream of each protocol at + // a time. However, we multiply this value by 2 in order to be generous. We also add 1 + // to account for the ping protocol. + let max_inbound_substreams = chains.len() + * (1 + REQUEST_RESPONSE_PROTOCOLS_PER_CHAIN + NOTIFICATIONS_PROTOCOLS_PER_CHAIN) + * 2; + ChainNetwork { inner: peers::Peers::new(peers::Config { connections_capacity: config.connections_capacity, peers_capacity: config.peers_capacity, + max_inbound_substreams, request_response_protocols, noise_key: config.noise_key, randomness_seed: randomness.sample(rand::distributions::Standard),