[SEC] libp2p - multistream.select should terminate on invalid <multistream-header-for-multistream>

[tintinweb]

Description

The first protocol message in multistream-select (spec) should be the <multistream-header-for-multistream> (e.g. /multistream/1.0.0). This allows both parties to negotiate the multistream protocol version to use for further messages.

Unfortunately, the spec is not very clear about when the <multistream-header-for-multistream> msg is allowed to be sent or requiring <multistream-header-for-multistream> being the first message in the protocol. Here're the relevant sections of the spec:

In the case of nim-libp2p this leads to the following inconsistencies (with other clients):

1) nim-libp2p multistream.select does not terminate if handshake fails

nim-libp2p requires the multiselect protocol negotiation (mutually exchanging /multistream/1.0.0\n) to take place with the first message. If a client fails to provide a valid <multistream-header-for-multistream> with the first message, the connection is not terminated. However, the multistream-select implementation cannot recover from this case anymore even if the peer where to send /multistream/1.0.0\n with a later packet.

Since the handshake failed (i.e. multistream version selection) and the protocol does not seem (even though it is not clear about that) to allow other messages to come first, the connection should be terminated immediately. This would safe resources that would otherwise be bound to a stale multiselect connection that cannot be used anymore.

here's an example interaction with nimbus (using nim-libp2p) continuing to keep the connection open even though the channel cannot be used anymore.

⇒  python eth2node.py
=> '\x16invalid-first-message\n'
<= '\x13/multistream/1.0.0\n'

=> '\x13/multistream/1.0.0\n'
<= '\x03na\n'

=> '\x03ls\n'
<= '\x03na\n'

=> '\x07/noise\n'
<= '\x03na\n'

=> '\x03ls\n'
<= '\x03na\n'

here's an example interaction with an anonymous public libp2p node (212.117.59.12) terminating the handshake:

⇒  python eth2node.py
=> '\x16invalid-first-message\n'
<= '\x13/multistream/1.0.0\n'
/multistream/1.0.0

=> '\x13/multistream/1.0.0\n'
Traceback (most recent call last):
  File "eth2node.py", line 125, in <module>
    x.sendMsg("/multistream/1.0.0")
  File "eth2node.py", line 99, in sendMsg
    resp = self.socket.recv(1024)
socket.error: [Errno 54] Connection reset by peer

2) nim-libp2p allows multiple <multistream-header-for-multistream>

The spec also does not state whether subsequent <multistream-header-for-multistream> are allowed even though the protocol version was already negotiated. However, it does not appear to make sense to allow subsequent header messages unless they are supposed to be used as a keep-alive.

nim-libp2p:

⇒  python eth2node.py
=> '\x13/multistream/1.0.0\n'
<= '\x13/multistream/1.0.0\n'

=> '\x13/multistream/1.0.0\n'
<= '\x13/multistream/1.0.0\n'

=> '\x13/multistream/1.0.0\n'
<= '\x13/multistream/1.0.0\n'

=> '\x13/multistream/1.0.0\n'
<= '\x13/multistream/1.0.0\n'

=> '\x13/multistream/1.0.0\n'
<= '\x13/multistream/1.0.0\n'

=> '\x03ls\n'
<= '\x07/noise\n'

an anonymous public node:

⇒  python eth2node.py
=> '\x13/multistream/1.0.0\n'
<= '\x13/multistream/1.0.0\n'

=> '\x13/multistream/1.0.0\n'
<= '\x03na\n'

=> '\x13/multistream/1.0.0\n'
<= '\x03na\n'

=> '\x13/multistream/1.0.0\n'
<= '\x03na\n'

=> '\x13/multistream/1.0.0\n'
<= '\x03na\n'

=> '\x03ls\n'
<= '\x1b\r/secio/1.0.0\n\x0b/tls/1.0.0\n\n'

3) nim-libp2p does not strictly validate the protocol message terminating character

The spec requires a message to be terminated with a \n. nim-libp2p, however, accepts messages without a trailing \n.

see: https://github.com/status-im/nim-libp2p/blob/b8b0a2b4bce8448b3aabc67fda96534124073c1c/libp2p/multistream.nim#L55

⇒  python eth2node.py
=> '\x12/multistream/1.0.0'
<= '\x13/multistream/1.0.0\n'

=> '\x02ls'
<= '\x07/noise\n'

an anonymous public node terminating the handshake due to protocol violation:

⇒  python eth2node.py
=> '\x12/multistream/1.0.0'
<= '\x13/multistream/1.0.0\n'

=> '\x02ls'
Traceback (most recent call last):
  File "eth2node.py", line 127, in <module>
    x.sendMsg("ls")
  File "eth2node.py", line 98, in sendMsg
    self.socket.send(x)
socket.error: [Errno 32] Broken pipe

Exploit Scenario

Spec deviations with potential resource (file handles/connections) exhaustion caused by connections not being freed immediately when they become unusable/unrecoverable.

Mitigation Recommendation

• terminate early if protocol violation is detected (e.g. trailing \n)
• terminate if first message is not the multistream header message
• discuss whether to return na for multistream header messages that are received in an already negotiated stream (I guess the only reason for this would be keep-alives e.g. for non tcp connections)

[tintinweb]

I dont seem to have permissions to add the nbc-audit label.

[sinkingsugar]

@dryajov I can take care of those if you got your hands full or busy in general. I don't mind at all.

[dryajov]

@dryajov I can take care of those if you got your hands full or busy in general. I don't mind at all.

go ahead @sinkingsugar 👍

[sinkingsugar]

Fixed in #291

[dryajov]

This has been addressed but not properly reviewed by auditors. I'm leaving this around for a future round of audits.

[Menduist]

I assume this will never be reviewed by auditors