From 7ec7baafc35b0823d14f94d60dd97b8d51192d83 Mon Sep 17 00:00:00 2001 From: James Hiew Date: Sun, 24 Nov 2024 03:08:02 +0000 Subject: [PATCH 1/7] info.public_key can belong to remote peer --- protocols/identify/src/protocol.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocols/identify/src/protocol.rs b/protocols/identify/src/protocol.rs index 33aeedb7c4f..257ec1f88d2 100644 --- a/protocols/identify/src/protocol.rs +++ b/protocols/identify/src/protocol.rs @@ -39,7 +39,7 @@ pub const PUSH_PROTOCOL_NAME: StreamProtocol = StreamProtocol::new("/ipfs/id/pus /// Identify information of a peer sent in protocol messages. #[derive(Debug, Clone)] pub struct Info { - /// The public key of the local peer. + /// The public key of the peer. pub public_key: PublicKey, /// Application-specific version of the protocol family used by the peer, /// e.g. `ipfs/1.0.0` or `polkadot/1.0.0`. From b5ea90a2dabb3653f0a4a5bcff6a3b557a1e1f01 Mon Sep 17 00:00:00 2001 From: James Hiew Date: Sun, 24 Nov 2024 03:36:11 +0000 Subject: [PATCH 2/7] Only handle incoming info if matches remote --- protocols/identify/src/handler.rs | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/protocols/identify/src/handler.rs b/protocols/identify/src/handler.rs index cda49f992b8..7309b00d606 100644 --- a/protocols/identify/src/handler.rs +++ b/protocols/identify/src/handler.rs @@ -242,10 +242,18 @@ impl Handler { } } - fn handle_incoming_info(&mut self, info: &Info) { + /// If the public key matches the remote peer, handles the given `info` and returns `true`. + fn handle_incoming_info(&mut self, info: &Info) -> bool { + let derived_peer_id = info.public_key.to_peer_id(); + if self.remote_peer_id != derived_peer_id { + tracing::warn!(%self.remote_peer_id, ?info.public_key, %derived_peer_id, "Discarding received identify message as public key does not match remote peer ID"); + return false; + } + self.remote_info.replace(info.clone()); self.update_supported_protocols_for_remote(info); + true } fn update_supported_protocols_for_remote(&mut self, remote_info: &Info) { @@ -346,11 +354,11 @@ impl ConnectionHandler for Handler { match self.active_streams.poll_unpin(cx) { Poll::Ready(Ok(Ok(Success::ReceivedIdentify(remote_info)))) => { - self.handle_incoming_info(&remote_info); - - return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour(Event::Identified( - remote_info, - ))); + if self.handle_incoming_info(&remote_info) { + return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( + Event::Identified(remote_info), + )); + } } Poll::Ready(Ok(Ok(Success::SentIdentifyPush(info)))) => { return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( @@ -365,11 +373,12 @@ impl ConnectionHandler for Handler { Poll::Ready(Ok(Ok(Success::ReceivedIdentifyPush(remote_push_info)))) => { if let Some(mut info) = self.remote_info.clone() { info.merge(remote_push_info); - self.handle_incoming_info(&info); - return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( - Event::Identified(info), - )); + if self.handle_incoming_info(&info) { + return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( + Event::Identified(info), + )); + } }; } Poll::Ready(Ok(Err(e))) => { From 616ceaf3da6d52124c30522a7a189e9cb91f0c11 Mon Sep 17 00:00:00 2001 From: James Hiew Date: Sun, 24 Nov 2024 04:37:18 +0000 Subject: [PATCH 3/7] Add smoke test --- protocols/identify/tests/smoke.rs | 43 +++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/protocols/identify/tests/smoke.rs b/protocols/identify/tests/smoke.rs index dd48b314173..0d2818df0a4 100644 --- a/protocols/identify/tests/smoke.rs +++ b/protocols/identify/tests/smoke.rs @@ -6,6 +6,7 @@ use std::{ use futures::StreamExt; use libp2p_identify as identify; +use libp2p_identity::Keypair; use libp2p_swarm::{Swarm, SwarmEvent}; use libp2p_swarm_test::SwarmExt; use tracing_subscriber::EnvFilter; @@ -440,3 +441,45 @@ async fn configured_interval_starts_after_first_identify() { assert!(time_to_first_identify < identify_interval) } + +#[async_std::test] +async fn reject_mismatched_public_key() { + let _ = tracing_subscriber::fmt() + .with_env_filter(EnvFilter::from_default_env()) + .try_init(); + + let mut honest_swarm = Swarm::new_ephemeral(|identity| { + identify::Behaviour::new( + identify::Config::new("a".to_string(), identity.public()) + .with_interval(Duration::from_secs(1)), + ) + }); + let mut spoofing_swarm = Swarm::new_ephemeral(|_unused_identity| { + let arbitrary_public_key = Keypair::generate_ed25519().public(); + identify::Behaviour::new( + identify::Config::new("a".to_string(), arbitrary_public_key) + .with_interval(Duration::from_secs(1)), + ) + }); + + honest_swarm.listen().with_memory_addr_external().await; + spoofing_swarm.connect(&mut honest_swarm).await; + + spoofing_swarm + .wait(|event| { + matches!(event, SwarmEvent::Behaviour(identify::Event::Sent { .. })).then_some(()) + }) + .await; + + let honest_swarm_events = futures::stream::poll_fn(|cx| honest_swarm.poll_next_unpin(cx)) + .take(4) + .collect::>() + .await; + + assert!( + !honest_swarm_events + .iter() + .any(|e| matches!(e, SwarmEvent::Behaviour(identify::Event::Received { .. }))), + "should emit no received events as received public key won't match remote peer", + ); +} From 1fad51065a6e0b2eadecc7834570ac35f21eddb6 Mon Sep 17 00:00:00 2001 From: James Hiew Date: Sun, 24 Nov 2024 17:12:05 +0000 Subject: [PATCH 4/7] Add changelog and bump crate version --- Cargo.lock | 2 +- Cargo.toml | 2 +- protocols/identify/CHANGELOG.md | 4 ++++ protocols/identify/Cargo.toml | 2 +- 4 files changed, 7 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4093d49504b..a2053e9a779 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2793,7 +2793,7 @@ dependencies = [ [[package]] name = "libp2p-identify" -version = "0.46.0" +version = "0.46.1" dependencies = [ "async-std", "asynchronous-codec", diff --git a/Cargo.toml b/Cargo.toml index dfa32628dbc..41d91429da9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -81,7 +81,7 @@ libp2p-dcutr = { version = "0.12.1", path = "protocols/dcutr" } libp2p-dns = { version = "0.42.0", path = "transports/dns" } libp2p-floodsub = { version = "0.45.0", path = "protocols/floodsub" } libp2p-gossipsub = { version = "0.48.0", path = "protocols/gossipsub" } -libp2p-identify = { version = "0.46.0", path = "protocols/identify" } +libp2p-identify = { version = "0.46.1", path = "protocols/identify" } libp2p-identity = { version = "0.2.10" } libp2p-kad = { version = "0.47.0", path = "protocols/kad" } libp2p-mdns = { version = "0.46.0", path = "protocols/mdns" } diff --git a/protocols/identify/CHANGELOG.md b/protocols/identify/CHANGELOG.md index 9051c331bbc..e8b4cde8f2b 100644 --- a/protocols/identify/CHANGELOG.md +++ b/protocols/identify/CHANGELOG.md @@ -1,3 +1,7 @@ +## 0.46.1 +- Discard `Info`s received from remote peers that contain a public key that doesn't match their peer ID. + See [PR TODO](https://github.com/libp2p/rust-libp2p/pull/TODO). + ## 0.46.0 - Make `identify::Config` fields private and add getter functions. diff --git a/protocols/identify/Cargo.toml b/protocols/identify/Cargo.toml index d7f6b6eca76..d2aeb74e626 100644 --- a/protocols/identify/Cargo.toml +++ b/protocols/identify/Cargo.toml @@ -3,7 +3,7 @@ name = "libp2p-identify" edition = "2021" rust-version = { workspace = true } description = "Nodes identification protocol for libp2p" -version = "0.46.0" +version = "0.46.1" authors = ["Parity Technologies "] license = "MIT" repository = "https://github.com/libp2p/rust-libp2p" From 55a3093526da2637df84ba86300a075d9279f44d Mon Sep 17 00:00:00 2001 From: James Hiew Date: Sat, 30 Nov 2024 12:17:11 +0000 Subject: [PATCH 5/7] Update PR reference in changelog --- protocols/identify/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocols/identify/CHANGELOG.md b/protocols/identify/CHANGELOG.md index e8b4cde8f2b..2b136740156 100644 --- a/protocols/identify/CHANGELOG.md +++ b/protocols/identify/CHANGELOG.md @@ -1,6 +1,6 @@ ## 0.46.1 - Discard `Info`s received from remote peers that contain a public key that doesn't match their peer ID. - See [PR TODO](https://github.com/libp2p/rust-libp2p/pull/TODO). + See [PR 5707](https://github.com/libp2p/rust-libp2p/pull/5707). ## 0.46.0 From efaa95f0bee88981f7584cbd45eab611d8181319 Mon Sep 17 00:00:00 2001 From: James Hiew Date: Wed, 4 Dec 2024 16:54:31 +0000 Subject: [PATCH 6/7] Use while let Poll::Ready --- protocols/identify/src/handler.rs | 73 ++++++++++++++++--------------- 1 file changed, 37 insertions(+), 36 deletions(-) diff --git a/protocols/identify/src/handler.rs b/protocols/identify/src/handler.rs index 7309b00d606..ef3095343af 100644 --- a/protocols/identify/src/handler.rs +++ b/protocols/identify/src/handler.rs @@ -352,46 +352,47 @@ impl ConnectionHandler for Handler { return Poll::Ready(event); } - match self.active_streams.poll_unpin(cx) { - Poll::Ready(Ok(Ok(Success::ReceivedIdentify(remote_info)))) => { - if self.handle_incoming_info(&remote_info) { - return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( - Event::Identified(remote_info), - )); - } - } - Poll::Ready(Ok(Ok(Success::SentIdentifyPush(info)))) => { - return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( - Event::IdentificationPushed(info), - )); - } - Poll::Ready(Ok(Ok(Success::SentIdentify))) => { - return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( - Event::Identification, - )); - } - Poll::Ready(Ok(Ok(Success::ReceivedIdentifyPush(remote_push_info)))) => { - if let Some(mut info) = self.remote_info.clone() { - info.merge(remote_push_info); - - if self.handle_incoming_info(&info) { + while let Poll::Ready(ready) = self.active_streams.poll_unpin(cx) { + match ready { + Ok(Ok(Success::ReceivedIdentify(remote_info))) => { + if self.handle_incoming_info(&remote_info) { return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( - Event::Identified(info), + Event::Identified(remote_info), )); } - }; - } - Poll::Ready(Ok(Err(e))) => { - return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( - Event::IdentificationError(StreamUpgradeError::Apply(e)), - )); - } - Poll::Ready(Err(Timeout { .. })) => { - return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( - Event::IdentificationError(StreamUpgradeError::Timeout), - )); + } + Ok(Ok(Success::SentIdentifyPush(info))) => { + return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( + Event::IdentificationPushed(info), + )); + } + Ok(Ok(Success::SentIdentify)) => { + return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( + Event::Identification, + )); + } + Ok(Ok(Success::ReceivedIdentifyPush(remote_push_info))) => { + if let Some(mut info) = self.remote_info.clone() { + info.merge(remote_push_info); + + if self.handle_incoming_info(&info) { + return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( + Event::Identified(info), + )); + } + }; + } + Ok(Err(e)) => { + return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( + Event::IdentificationError(StreamUpgradeError::Apply(e)), + )); + } + Err(Timeout { .. }) => { + return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( + Event::IdentificationError(StreamUpgradeError::Timeout), + )); + } } - Poll::Pending => {} } Poll::Pending From 16f08914efbd37f82d4c27a06aa97dd1db3f06ad Mon Sep 17 00:00:00 2001 From: James Hiew Date: Tue, 10 Dec 2024 21:16:09 +0000 Subject: [PATCH 7/7] Move logging to poll() --- protocols/identify/src/handler.rs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/protocols/identify/src/handler.rs b/protocols/identify/src/handler.rs index ef3095343af..6e5af290cd2 100644 --- a/protocols/identify/src/handler.rs +++ b/protocols/identify/src/handler.rs @@ -246,7 +246,6 @@ impl Handler { fn handle_incoming_info(&mut self, info: &Info) -> bool { let derived_peer_id = info.public_key.to_peer_id(); if self.remote_peer_id != derived_peer_id { - tracing::warn!(%self.remote_peer_id, ?info.public_key, %derived_peer_id, "Discarding received identify message as public key does not match remote peer ID"); return false; } @@ -359,6 +358,13 @@ impl ConnectionHandler for Handler { return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( Event::Identified(remote_info), )); + } else { + tracing::warn!( + %self.remote_peer_id, + ?remote_info.public_key, + derived_peer_id=%remote_info.public_key.to_peer_id(), + "Discarding received identify message as public key does not match remote peer ID", + ); } } Ok(Ok(Success::SentIdentifyPush(info))) => { @@ -379,8 +385,15 @@ impl ConnectionHandler for Handler { return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour( Event::Identified(info), )); + } else { + tracing::warn!( + %self.remote_peer_id, + ?info.public_key, + derived_peer_id=%info.public_key.to_peer_id(), + "Discarding received identify message as public key does not match remote peer ID", + ); } - }; + } } Ok(Err(e)) => { return Poll::Ready(ConnectionHandlerEvent::NotifyBehaviour(