Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Peer ID Verification within Noise Protocol Handshake Instead of Connection Pool Poll #4726

Closed
frystal opened this issue Oct 25, 2023 · 1 comment

Comments

@frystal
Copy link

frystal commented Oct 25, 2023

Summary

In the rust-libp2p Noise protocol implementation, when the current node operates as an initiator for the handshake, it fails to verify if the remote connection has the same public key as the node it is attempting to dial.
https://github.com/libp2p/rust-libp2p/blob/master/transports/noise/src/lib.rs#L158

Instead, the node ID validation is performed during the connection pool stage when the "PendingConnectionEvent::ConnectionEstablished" event is triggered. https://github.com/libp2p/rust-libp2p/blob/master/swarm/src/connection/pool.rs#L675

I am concerned that this approach might be inadequate, as the connection information has already been logged into the swarm by this point. Thus it might potentially increase the risk to identity theft attacks. While I am relatively new to Rust, I observed that in both go-libp2p and js-libp2p, this check is performed during the Noise protocol stage, and the connection is not established if an inconsistency in the peer ID is detected. I would be grateful to be told the reasoning behind conducting this check later in the connection pool.

Expected behavior

The peer ID verification should take place during the Noise protocol handshake stage, and the connection should be rejected if the peer ID is not consistent with expectations, all before the "PendingConnectionEvent::ConnectionEstablished" event is triggered.

Actual behavior

The peer ID check is currently conducted in the swarm pool after the Noise protocol handshake has been established.

Relevant log output

No response

Possible Solution

No response

Version

No response

Would you like to work on fixing this bug ?

Maybe

@thomaseizinger
Copy link
Contributor

You are fully correct in your analysis! I am going to close this as a dupe of #2946 :)

@thomaseizinger thomaseizinger closed this as not planned Won't fix, can't repro, duplicate, stale Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants