p2p/libp2p: fix potential denial of service

[zhiqiangxu]

trusting Message.From can be leveraged to cause denial of service, use peer.ID instead

[iulianpascalau]

Thanks for pointing this out 👍
I would do it a little bit different though: I would make the node reject the message if the msg.GetFrom() is different from fromConnectedPeer value. So, I would revert the checkAndSetSeenMessage function and I would add the following condition on L114:

	if !bytes.Equal(message.GetFrom(), []byte(fromConnectedPeer)){
		return fmt.Errorf("%w mismatch between From and fromConnectedPeer values", p2p.ErrInvalidMessage)		
	}

In this way we would have a stricter way to check the messages.

[zhiqiangxu]

fromConnectedPeer may be either local ip or public ip, depending on whether two nodes are in the same net, so such strict condition may cause false negative?

[iulianpascalau]

libp2p split the peer info into 2: peer ID and multi address data. For example, suppose that we have a connection info such as /ip4/192.168.0.1/tcp/9999/p2p/16Uiu2HAkw5SNNtSvH1zJiQ6Gc3WoGNSxiyNueRKe6fuAuh57G3Bk. The string 16Uiu2HAkw5SNNtSvH1zJiQ6Gc3WoGNSxiyNueRKe6fuAuh57G3Bk is the base58 encoded peer id and the /ip4/192.168.0.1/tcp/9999 is one entry in the multi-address slice as there can be more entries there (/ip4/127.0.0.1/tcp/9999, /ip4/5.2.221.243/tcp/9999 and so on). So that peer.ID value will always contain an unique string that will have to identify the peer regardless the connection it has to that peer.

[zhiqiangxu]

Good to know! I've updated accordingly.

[iulianpascalau]

You need to fix the unit tests (directSender_test.go file) as following:
Step 1: replace with id the hardcoded string values on L156, L176, L199, L219, L244, L268

Step 2:
In the TestDirectSender_ReceivedSentMessageShouldCallMessageHandlerTestFullCycle function you need to:

• replace the return from L530 with return remotePeer
• add this on L538:

	cs.LocalPeerCalled = func() peer.ID {
		return cs.RemotePeer()
	}

Step 3:
Will be nice to also have a test for this new if branch, so you might add:

func TestDirectSender_ProcessReceivedDirectMessageFromMismatchesFromConnectedPeerShouldErr(t *testing.T) {
	ds, _ := libp2p.NewDirectSender(
		context.Background(),
		generateHostStub(),
		blankMessageHandler,
	)

	id, _ := createLibP2PCredentialsDirectSender()

	msg := &pubsub_pb.Message{}
	msg.Data = []byte("data")
	msg.Seqno = []byte("111")
	msg.From = []byte(id)
	msg.TopicIDs = make([]string, 0)

	err := ds.ProcessReceivedDirectMessage(msg, peer.ID("not the same peer id"))

	assert.True(t, errors.Is(err, p2p.ErrInvalidValue))
}

[zhiqiangxu]

Updated:)

[iulianpascalau]

you forgot the TestDirectSender_ReceivedSentMessageShouldCallMessageHandlerTestFullCycle function :)
(an that test surly fails after your change)

[zhiqiangxu]

Updated, sorry for blindly copy-paste:(

[iulianpascalau]

No worries :)
Still, TestDirectSender_ReceivedSentMessageShouldCallMessageHandlerTestFullCycle is not fixed (you can try running the unit tests by yourself). I have updated the original comment regarding the tests with steps. So, step 1 & 3 are done, step 2 is not done.

[zhiqiangxu]

I didn't follow the mock code too much yet, just followed your advice above, hope the test should pass this time:)

[iulianpascalau]

GJ!

The base branch was changed.