nim-libp2p
nim-libp2p copied to clipboard
[SEC] libp2p - terminate connection if securedHandler fails
Description
Performing an invalid handshake hinting a zero-length noise packet results in the connection to hang. The connection is not closed and there does not seem to be a timeout.
It seems like failing to handshake /noise
is treated as a recoverable error while it should be fatal. see line 371 below.
⇒ python eth2node.py
=> '\x13/multistream/1.0.0\n\x07/noise\n'
<= '\x13/multistream/1.0.0\n'
=> '\x00\x00'
<= '\x07/noise\n'
/noise
=> '\x13/multistream/1.0.0\n'
=> '\x07/noise\n'
=> '\x03ls\n'
⇒ build/beacon_node --network=altona --log-level="WARN; TRACE:networking" --data-dir=build/data/shared_altona_0 --dump --web3-url=wss://goerli.infura.io/ws/v3/6224f3c792cc443fafb64e70a98f871e --tcp-port=9000 --udp-port=9000 --metrics --metrics-port=8008 --rpc --rpc-port=9190 --help
...
TRC 2020-07-22 13:27:12+02:00 upgrading incoming connection topics="switch" tid=9594725 file=switch.nim:345 conn= oid=5f182290641ecd6028e91e73
TRC 2020-07-22 13:27:12+02:00 initiating handshake topics="multistream" tid=9594725 file=multistream.nim:48 codec="\x13/multistream/1.0.0\n"
TRC 2020-07-22 13:27:12+02:00 registering proto handler topics="switch" tid=9594725 file=multistream.nim:179 codec=/noise
TRC 2020-07-22 13:27:12+02:00 handle: starting multistream handling topics="multistream" tid=9594725 file=multistream.nim:115
TRC 2020-07-22 13:27:12+02:00 handle: got request for topics="multistream" tid=9594725 file=multistream.nim:121 ms=/noise
TRC 2020-07-22 13:27:12+02:00 found handler for topics="multistream" tid=9594725 file=multistream.nim:143 protocol=/noise
TRC 2020-07-22 13:27:12+02:00 Securing connection topics="switch" tid=9594725 file=switch.nim:354 oid=5f182290641ecd6028e91e73
TRC 2020-07-22 13:27:12+02:00 Starting Noise handshake topics="noise" tid=9594725 file=noise.nim:424 initiator=false peer=
TRC 2020-07-22 13:27:12+02:00 mixHash topics="noise" tid=9594725 file=noise.nim:161 hash=f3d15e6108ed...2e0958dc002d
TRC 2020-07-22 13:27:12+02:00 receiveHSMessage topics="noise" tid=9594725 file=noise.nim:270 size=0
TRC 2020-07-22 13:27:12+02:00 noise read e topics="noise" tid=9594725 file=noise.nim:237 size=0
DBG 2020-07-22 13:27:12+02:00 ending secured handler topics="switch" tid=9594725 file=switch.nim:375 err="Noise E, expected more data"
TRC 2020-07-22 13:27:12+02:00 leaving multistream loop topics="multistream" tid=9594725 file=multistream.nim:156
Exploit Scenario
Failing to upgrade to a secure connection leaves the connection unencrypted. In our tests, the connection appears to hang while it might still be in another select
loop. The risk is that a connection might be continued without ensuring that transport security was properly initialized and there is risk that nodes might use the hanging connection for resource exhaustion attacks.
Note: the muxer is only attached if the security protocol returns success. Additional commands will not be available until a secured transport was established.
Mitigation Recommendation
- terminate the connection on protocol errors. consider adjusting the clients score on foul play.
Refrences
https://github.com/status-im/nim-libp2p/blob/b8b0a2b4bce8448b3aabc67fda96534124073c1c/libp2p/switch.nim#L345-L379
@dryajov I need your help with this one actually since you wrote most of the code.
I cannot see how this is happening.
The above linked return
will not happen actually, what happens is an exception, the exception will be handled (inside switch.nim / secureHandler), and that flow is done... the previous multi stream flow is also done (return in ms handler) after the secureHandler exits..., and this will go back into switch->start->handler which has a finally clause with conn.close()
Am I missing something?
Would help to have the test scenario scripts.
The above log shows what I wrote above btw. One point I don't get is also how to trigger this:
Failing to upgrade to a secure connection leaves the connection unencrypted. In our tests, the connection appears to hang while it might still be in another select loop
I cannot see how this is happening...
@sinkingsugar yes, I believe you are correct. It is unfortunate however that exceptions make this flow a lot more complicated than what it needs be.
@tintinweb To make reasoning about the flow a bit easier, the rule of thumb is to follow the exception flow to see if a handler higher up will do the required cleanup/closing/exit/etc... Ideally, this would have been enforced through the {.raises: [...].}
pragma, which enables checked exceptions in Nim, but it currently doesn't work with our async subsystem (chronos).
The lack of support for checked exceptions with chronos might in fact be a security issues in and out of itself - something to consider perhaps.
Update: This issue was kinda invalid back then but the error handling behavior has also changed in the meantime, for the better (altho not final) https://github.com/status-im/nim-libp2p/blob/96ed9fbe85de18bbb13dfe8cf16e79fe9c5c0906/libp2p/protocols/secure/secure.nim#L133-L140 During handshakes now we completely disconnect if something wrong happens. This applies also to the deprecated Secio as it's on the base class.
This has been addressed but not properly reviewed by auditors. I'm leaving this around for a future round of audits.
I assume this will never be reviewed by auditors