fix: warn on mismatched underlay #2464
Conversation
notanatol
force-pushed
the
cure53-handshake
branch
2 times, most recently
from
7198e9f to
2c27287
Compare
notanatol
force-pushed
the
cure53-handshake
branch
from
2c27287 to
f981aa2
Compare
notanatol
force-pushed
the
cure53-handshake
branch
from
f981aa2 to
12e81a3
Compare
|
SonarCloud Quality Gate failed.
|
|
|
||
| if s.libp2pID != observedUnderlayAddrInfo.ID { | ||
| //NOTE eventually we will return error here, but for now we want to gather some statistics | ||
| s.logger.Warningf("received peer ID %s does not match ours: %s", observedUnderlayAddrInfo.ID, s.libp2pID) |
There was a problem hiding this comment.
@acud normally we should return error here but we thought about maybe leaving it as a warning to gather some statistics about how often it happens.
Do you think this kind of data would be valuable or should we error here right away?
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @acud and @janos)
pkg/p2p/libp2p/internal/handshake/handshake.go, line 160 at r2 (raw file):
Previously, notanatol (Anatol) wrote…
@acud normally we should return error here but we thought about maybe leaving it as a warning to gather some statistics about how often it happens.
Do you think this kind of data would be valuable or should we error here right away?
I think you can return an error after the logline!?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @acud and @janos)
pkg/p2p/libp2p/internal/handshake/handshake.go, line 160 at r2 (raw file):
Previously, mrekucci wrote…
I think you can return an error after the logline!?
Yes, technically it's doable, but it's just a general concern when it comes to introducing such a change to the behavior of the handshake protocol.
We are not sure how often this condition would be met "in the wild" where some peers might be behind NAT and the underlays might not match.
|
|
||
| if s.libp2pID != observedUnderlayAddrInfo.ID { | ||
| //NOTE eventually we will return error here, but for now we want to gather some statistics | ||
| s.logger.Warningf("received peer ID %s does not match ours: %s", observedUnderlayAddrInfo.ID, s.libp2pID) |
SWA-01-008 WP2: Injection of corrupted underlay into P2P handshake (Medium)
While auditing libp2p and the handshake protocol, it was observed that the underlay address is retrieved from the unsigned part of the
SynAckstructure (second packet within the handshake), thereby being unprotected.An attacker could leverage this and modify that part of the message without the client noticing during the initial handshake, which would result in corrupting the underlay network information provided to the client.
This change is