search

waku-org / nwaku

Waku node and protocol.
Other
202 stars 53 forks source link

bug: ENR shouldn't be used for pruning #2594

Open AlejandroCabeza opened 6 months ago

AlejandroCabeza commented 6 months ago

Problem

PeerManager.prunePeer should not use ENR as it's easy to fake.

Impact

Low

To reproduce

https://github.com/waku-org/nwaku/blob/master/waku/node/peer_manager/peer_manager.nim#L852-L853

Expected behavior

Instead of ENR, GossipSub.peerStats could be an option.

nwaku version/commit hash

wakunode2: v0.24.0-rc.0-126-g4117fe branch: test-peer-connection-management commit: bbebbac5fb1edd230eaaa2043e4a78caf3794884 (might change in the future)

CC: @alrevuelta

gabrielmer commented 1 month ago

Took a look and want to better understand the issue. IIUC, we choosepeerId's for which we failed to connect recently and we remove them from the peer store.

What's the vulnerability? that someone hardcodes and advertises the peerId of someone else, creates failed connections to other nodes so the nodes remove it from their peerStores?

At the end of the day, this happens for disconnected peers, not for connected peers. Not sure what are the implications of removing some node that we're disconnected to from the peer store. I think it still can establish connections with other peers, it won't make the peer disconnect from existing connections, and the node should still be in other nodes peerStores.

Also want to better understand the idea of using PeerStats

AlejandroCabeza commented 1 month ago

Took a look and want to better understand the issue. IIUC, we choosepeerId's for which we failed to connect recently and we remove them from the peer store.

What's the vulnerability? that someone hardcodes and advertises the peerId of someone else, creates failed connections to other nodes so the nodes remove it from their peerStores?

At the end of the day, this happens for disconnected peers, not for connected peers. Not sure what are the implications of removing some node that we're disconnected to from the peer store. I think it still can establish connections with other peers, it won't make the peer disconnect from existing connections, and the node should still be in other nodes peerStores.

Also want to better understand the idea of using PeerStats

I can't recall 😅 This is something that arised out of a conversation with @alrevuelta, IRRC, so given his background he might have a better answer for it.

gabrielmer commented 1 month ago

Talking to @alrevuelta, the issue lies not on the risk of impersonating another node, it lies on nodes being able to advertise in their ENRs that they support shards that they don't actually support. For that, instead of looking at the ENR we can try look in GossipSub's parameters for that same information