Fix security issue #1362. #1377
Conversation
|
Where are you rejecting new incoming peers when the corresponding limit is reached? |
| result.peerPool = newPeerPool[Peer, PeerID](maxPeers = conf.maxPeers) | ||
| result.wantedPeers = (conf.maxPeers - conf.maxIncomingPeers) | ||
| result.peerPool = newPeerPool[Peer, PeerID]( | ||
| maxPeers = conf.maxPeers, maxIncomingPeers = conf.maxIncomingPeers) |
There was a problem hiding this comment.
AFAIK this is one of the mitigation recommended in #1362.
This should explicitly be linked to the issue, maybe with "SEC" (like TODO), so that 6 months from now, after a refactoring, this mitigation does not disappear.
There was a problem hiding this comment.
@mratsim this is something we are missing in our implementation, it will be not refactored and we do not perform refactoring without minds.
|
why increase to 100? the odd number is more fun |
I'm not sure a tag is needed really (such things are hard to maintain consistently), but an explanatory comment with background / rationale makes sense regardless if it's a security issue or not every time there's a bug with an underlying tricky condition |
Fix #1362. Fix variable names and 80 chars per line. Removal of unnecessary sleepAsync.
Add comment about why we limiting number of incoming connections.
|
this LGTM, but there is a similar issue with outgoing peers: if we hit limit, they will be queued meaning they'll take up resources as they're stuck in the await, but not be disconnected. #1468 slightly reorganizes when peers are added to peer pool, hopefully it will help make this branch better |
| @@ -106,10 +106,15 @@ type | |||
| name: "udp-port" }: Port | |||
|
|
|||
| maxPeers* {. | |||
| defaultValue: 79 # The Wall gets released | |||
| desc: "The maximum number of peers to connect to" | |||
| defaultValue: 101 # The Wall gets released | |||
There was a problem hiding this comment.
need to leave at 79 or remove cultural reference
|
This needs rebasing now. |
maxIncomingPeerswith default value of21maxPeersfrom79to100.